CLOUDP-280230: Network peering controller and CRD #1930

josvazg · 2024-11-14T17:04:16Z

Network peering support is sliced into 3 logical changes:

Fixing to the already merged translation layer.
Adding the CRD types and manifest files.
Adding the controller code and tests.
🚧 (WIP) Unit tests

All Submissions:

Have you signed our CLA?

github-actions · 2024-11-15T16:31:21Z

https://app.codecov.io/github/mongodb/mongodb-atlas-kubernetes/commit/c147102f22aab9d186f1bb6144ad79157882af4f

s-urbaniak · 2025-01-23T08:58:26Z

api/v1/atlasnetworkpeering_types_test.go

+)
+
+func TestNetworkPeeringProjectRefCELValidations(t *testing.T) {
+	launchProjectRefCELTests(


q: what is being asserted here? launchProjectRefCELTests has no indication what test cases are being executed.

This is interesting, because all cases were always the same for all the using structs. The thing is, the only change was the dual reference was used by one or another underlying CRD type, but that does not change the behaviour or correctness of how the dual ref is handled.

We could fix this in a separate PR, I think we can have a single test for this in a single place and just keep on adding underlaying CRDs to test as carriers of the dual reference to be tested.
WDYT?

s-urbaniak · 2025-01-23T09:00:16Z

api/v1/atlasnetworkpeering_types.go

+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata,omitempty"`
+
+	Spec   AtlasNetworkPeeringSpec          `json:"spec,omitempty"`


q: are we following the rules described in https://ahmet.im/blog/crd-generation-pitfalls/index.html#explicit-required-or-optional-on-every-field for all spec fields?

I will double check that for when we surface this again, both for the NetworkContainer and the PeeringConnection CRDs.

s-urbaniak · 2025-01-23T09:02:51Z

internal/operator/builder.go

@@ -219,6 +220,18 @@ func (b *Builder) Build(ctx context.Context) (cluster.Cluster, error) {
 		return nil, fmt.Errorf("unable to create indexers: %w", err)
 	}

+	npeReconciler := atlasnetworkpeering.NewAtlasNetworkPeeringsReconciler(


blocking: this needs to be in the controller registry https://github.com/mongodb/mongodb-atlas-kubernetes/blob/main/internal/controller/registry.go

hint: just add the constructor below https://github.com/mongodb/mongodb-atlas-kubernetes/blob/main/internal/controller/registry.go#L79

Right! yep I also need to fix this for comply with the new Cluster support

fixed in next update

s-urbaniak · 2025-01-23T09:09:09Z

internal/controller/atlasnetworkpeering/atlasnetworkpeering_controller.go

+		atlasPeer = peer
+	}
+	inAtlas := atlasPeer != nil
+	deleted := req.networkPeering.DeletionTimestamp != nil


blocking: this doesn't catch the case of deletion timestamp being zero. Please use the IsZero() method which asserts both nil and zero time.

s-urbaniak · 2025-01-23T09:14:39Z

internal/controller/atlasnetworkpeering/atlasnetworkpeering_controller.go

+//+kubebuilder:rbac:groups=atlas.mongodb.com,resources=atlasnetworkpeerings/status,verbs=get;update;patch
+//+kubebuilder:rbac:groups=atlas.mongodb.com,resources=atlasnetworkpeerings/finalizers,verbs=update
+
+// Reconcile is part of the main kubernetes reconciliation loop which aims to


blocking: this seems to be a scaffold comment, please fix ;-)

s-urbaniak · 2025-01-23T09:20:13Z

internal/indexer/indexer.go

@@ -18,9 +18,9 @@ type Indexer interface {
 // RegisterAll registers all known indexers to the given manager.
 // It uses the given logger to create a new named "indexer" logger,
 // passing that to each indexer.
-func RegisterAll(ctx context.Context, c cluster.Cluster, logger *zap.Logger) error {
+func RegisterAll(ctx context.Context, mgr manager.Manager, logger *zap.Logger) error {


blocking: undo this change, we need to retain c cluster.Cluster as a parameter.

Right! I missed this detail on the rebase, I will fix

reverted on next update

s-urbaniak · 2025-01-23T09:22:31Z

internal/controller/atlasnetworkpeering/atlasnetworkpeering_controller.go

+	).ReconcileResult()
+}
+
+func (r *AtlasNetworkPeeringReconciler) sync(req *reconcileRequest, atlasPeer *networkpeering.NetworkPeer) ctrl.Result {


blocking: what is being sync'ed here? as I read the code, updates in the spec are silently ignored. If the spec is immutable, we should talk w/ PM whether it makes sense to make the whole spec immutable (guarded via CEL) if Atlas doesn't support updates to network peers.

I see we have updates in Atlas for both connections https://www.mongodb.com/docs/atlas/reference/api-resources-spec/v2/#tag/Network-Peering/operation/updatePeeringConnection and containers https://www.mongodb.com/docs/atlas/reference/api-resources-spec/v2/#tag/Network-Peering/operation/updatePeeringContainer.

Why aren't we leveraging that?

s-urbaniak · 2025-01-23T09:24:43Z

internal/controller/workflow/reason.go

+// Atlas Network Peering reasons
+const (
+	NetworkPeeringConnectionCreating  ConditionReason = "NetworkPeeringConnectionCreating"
+	NetworkPeeringConnectionResetting ConditionReason = "NetworkPeeringConnectionResetting"


blocking: this status condition is unused.

will remove

done in next update

s-urbaniak

a couple of comments, great work! 👏

josvazg · 2025-01-23T13:16:40Z

Making this a draft again, as we plan to re-design the Peering Connection - Network Container relationship, to make Networks a first class CRD as well.

Still I would like to address most of the comments as I will be using this branch for continuing work and this and will need it to be rebase-able as close to main as possible.

Signed-off-by: jose.vazquez <[email protected]>

josvazg changed the base branch from main to CLOUDP-280230/net-peering-crd November 14, 2024 17:04

josvazg marked this pull request as draft November 14, 2024 17:04

josvazg force-pushed the CLOUDP-280230/net-peering-controller branch 2 times, most recently from 3e4d019 to a1ae865 Compare November 15, 2024 14:40

josvazg added the cloud-tests Run expensive Cloud Tests: Integration & E2E label Nov 15, 2024

josvazg temporarily deployed to release November 15, 2024 16:31 — with GitHub Actions Inactive

josvazg had a problem deploying to gov-test November 15, 2024 16:31 — with GitHub Actions Failure

josvazg had a problem deploying to test November 15, 2024 16:31 — with GitHub Actions Failure

josvazg temporarily deployed to test November 15, 2024 16:31 — with GitHub Actions Inactive

josvazg had a problem deploying to test November 15, 2024 16:31 — with GitHub Actions Failure

josvazg temporarily deployed to test November 15, 2024 16:31 — with GitHub Actions Inactive

josvazg had a problem deploying to openshift-test November 15, 2024 16:31 — with GitHub Actions Failure

josvazg had a problem deploying to test November 15, 2024 20:47 — with GitHub Actions Failure

josvazg had a problem deploying to gov-test November 15, 2024 20:47 — with GitHub Actions Failure

josvazg had a problem deploying to openshift-test November 15, 2024 20:47 — with GitHub Actions Failure

josvazg force-pushed the CLOUDP-280230/net-peering-controller branch from f2a35cc to 1d71818 Compare November 15, 2024 20:59

josvazg temporarily deployed to test November 15, 2024 21:05 — with GitHub Actions Inactive

josvazg had a problem deploying to test November 15, 2024 21:05 — with GitHub Actions Failure

josvazg temporarily deployed to release November 15, 2024 21:05 — with GitHub Actions Inactive

josvazg temporarily deployed to test November 15, 2024 21:05 — with GitHub Actions Inactive

josvazg temporarily deployed to test January 22, 2025 22:55 — with GitHub Actions Inactive

josvazg requested review from helderjs, igor-karpukhin, roothorp and s-urbaniak January 23, 2025 08:21

s-urbaniak reviewed Jan 23, 2025

View reviewed changes

s-urbaniak requested changes Jan 23, 2025

View reviewed changes

josvazg marked this pull request as draft January 23, 2025 13:16

josvazg added 5 commits January 23, 2025 18:40

CLOUDP-280230: Add network peering controller & tests

790ef4b

Signed-off-by: jose.vazquez <[email protected]>

Unit tests draft

efbbee4

Signed-off-by: jose.vazquez <[email protected]>

Treat container in use as a good cleanup exit

45e766c

Remove unused reason

ac73467

Fix to comply to cluster usage over manager

f3ac29e

josvazg force-pushed the CLOUDP-280230/net-peering-controller branch from c147102 to f3ac29e Compare January 23, 2025 17:40

josvazg added the cloud-tests Run expensive Cloud Tests: Integration & E2E label Jan 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLOUDP-280230: Network peering controller and CRD #1930

CLOUDP-280230: Network peering controller and CRD #1930

josvazg commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

s-urbaniak Jan 23, 2025

josvazg Jan 23, 2025 •

edited

Loading

s-urbaniak Jan 23, 2025

josvazg Jan 23, 2025

s-urbaniak Jan 23, 2025

s-urbaniak Jan 23, 2025

josvazg Jan 23, 2025

josvazg Jan 23, 2025

s-urbaniak Jan 23, 2025

s-urbaniak Jan 23, 2025

s-urbaniak Jan 23, 2025

josvazg Jan 23, 2025

josvazg Jan 23, 2025

s-urbaniak Jan 23, 2025

s-urbaniak Jan 23, 2025

s-urbaniak Jan 23, 2025

josvazg Jan 23, 2025

josvazg Jan 23, 2025

s-urbaniak left a comment

josvazg commented Jan 23, 2025 •

edited

Loading

CLOUDP-280230: Network peering controller and CRD #1930

Are you sure you want to change the base?

CLOUDP-280230: Network peering controller and CRD #1930

Conversation

josvazg commented Nov 14, 2024 • edited Loading

All Submissions:

github-actions bot commented Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

josvazg Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-urbaniak left a comment

Choose a reason for hiding this comment

josvazg commented Jan 23, 2025 • edited Loading

josvazg commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

josvazg Jan 23, 2025 •

edited

Loading

josvazg commented Jan 23, 2025 •

edited

Loading