Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLOUDP-178750: Add deletion protection to deployments #1036

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

josvazg
Copy link
Collaborator

@josvazg josvazg commented Jul 13, 2023

All Submissions:

  • Have you signed our CLA?
  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if there is one).
  • Update docs/release-notes/release-notes.md if your changes should be included in the release notes for the next release.

@josvazg josvazg marked this pull request as draft July 13, 2023 15:47
@josvazg josvazg changed the base branch from CLOUDP-178750/deployment-deletion-protection to deletion-protection-main July 13, 2023 15:54
@josvazg josvazg marked this pull request as ready for review July 13, 2023 15:54
@josvazg josvazg marked this pull request as draft July 13, 2023 15:55
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from fa7545d to 69e14af Compare July 14, 2023 15:37
@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2023

@josvazg josvazg marked this pull request as ready for review July 14, 2023 15:56
@josvazg josvazg changed the title [WIP] CLOUDP-178750: Add deletion protection to deployments CLOUDP-178750: Add deletion protection to deployments Jul 14, 2023
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch 5 times, most recently from e030e9e to 740f9d5 Compare July 17, 2023 13:06
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from 740f9d5 to 20353fa Compare July 24, 2023 16:14
@josvazg josvazg requested a review from helderjs July 24, 2023 16:15
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from 20353fa to 0d5aa16 Compare July 25, 2023 15:59
@josvazg
Copy link
Collaborator Author

josvazg commented Jul 26, 2023

Ownership check is incomplete, will follow up on it soon.

@josvazg josvazg marked this pull request as draft July 26, 2023 06:08
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from 0d5aa16 to 5351929 Compare July 26, 2023 15:53
@josvazg
Copy link
Collaborator Author

josvazg commented Jul 27, 2023

Ownership checks are now included, I wonder if we need any custom data transformation in the matching functions, like data format normalization.

@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from 5351929 to e79b4c2 Compare July 28, 2023 12:11
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch 2 times, most recently from dd342cd to 8d041e7 Compare August 3, 2023 11:10
@josvazg
Copy link
Collaborator Author

josvazg commented Aug 3, 2023

Just migrated unit tests to use Atlas service mocks instead of the http client mock.

@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from 8d041e7 to 350b948 Compare August 3, 2023 14:47
@josvazg josvazg requested a review from helderjs August 3, 2023 14:48
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch 3 times, most recently from d72eebf to 87a2aeb Compare August 4, 2023 09:17
@josvazg josvazg requested a review from helderjs August 4, 2023 09:19
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch 2 times, most recently from 7678189 to e4841d0 Compare August 7, 2023 14:34
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from e4841d0 to 693ee61 Compare August 9, 2023 09:54
@josvazg
Copy link
Collaborator Author

josvazg commented Aug 10, 2023

The current CI issues with deployment-annotation-ns do not seem flakyness, there is an issue with ownership checking. It finds differences:

  	... // 13 identical fields
  	PitEnabled: &false,
  	StateName:  "IDLE",
  	ReplicationSpecs: []*mongodbatlas.AdvancedReplicationSpec{
  		&{
  			NumShards: 1,
  			ID:        "64d4091c69c4e9090c82f155",
  			ZoneName:  "Zone 1",
  			RegionConfigs: []*mongodbatlas.AdvancedRegionConfig{
  				&{
  					AnalyticsAutoScaling: nil,
- 					AnalyticsSpecs:       nil,
+ 					AnalyticsSpecs:       &mongodbatlas.Specs{InstanceSize: "M2", NodeCount: &0},
  					ElectableSpecs: &mongodbatlas.Specs{
  						DiskIOPS:      nil,
  						EbsVolumeType: "",
  						InstanceSize:  "M2",
- 						NodeCount:     nil,
+ 						NodeCount:     &3,
  					},
- 					ReadOnlySpecs:       nil,
+ 					ReadOnlySpecs:       &mongodbatlas.Specs{InstanceSize: "M2", NodeCount: &0},
  					AutoScaling:         nil,
  					BackingProviderName: "AWS",
  					... // 3 identical fields
  				},
  			},
  		},
  	},
  	CreateDate:   "2023-08-09T21:46:04Z",
  	RootCertType: "ISRGROOTX1",
  	... // 3 identical fields

I will check the test sequence in detail. In theory, this should not happen, as the operator should have taken control of the deployment, by annotating the previous config. Once that happens, it should not have to re-check for differences. Also, this deployment is created by the operator, so not sure how it can lose ownership like in this test. Will debug.

@josvazg josvazg marked this pull request as draft August 10, 2023 06:29
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch 2 times, most recently from 1a26d2d to 0deaf1e Compare August 10, 2023 09:31
@josvazg
Copy link
Collaborator Author

josvazg commented Aug 10, 2023

Fix applied before I got the go.mod issue:

  • The ownership check must only be affected by the protection configuration flag, not by the annotations to keep or not the object. That way protection off bypass all ownership checks, as expected.
  • The test that was failing would fail with protection off. The Atlas deployment left there cannot be re-sync at redeployment time, as it will find differences and thus will fail to grab ownership of the object.
% git diff
diff --git a/pkg/controller/atlasdeployment/atlasdeployment_controller.go b/pkg/controller/atlasdeployment/atlasdeployment_controller.go
index 6276f9c..5aa1843 100644
--- a/pkg/controller/atlasdeployment/atlasdeployment_controller.go
+++ b/pkg/controller/atlasdeployment/atlasdeployment_controller.go
@@ -236,13 +236,13 @@ func (r *AtlasDeploymentReconciler) checkDeploymentIsManaged(
                dply.Spec.DeploymentSpec = nil
        }
 
-       isProtected := customresource.IsResourceProtected(dply, r.ObjectDeletionProtection)
        owner, err := customresource.IsOwner(
                dply,
-               isProtected,
+               r.ObjectDeletionProtection,
                customresource.IsResourceManagedByOperator,
                managedByAtlas(context, workflowCtx.Client, project.ID(), log),
        )
+
        if err != nil {
                result := workflow.Terminate(workflow.Internal, fmt.Sprintf("unable to resolve ownership for deletion protection: %s", err))
                workflowCtx.SetConditionFromResult(status.DatabaseUserReadyType, result)
diff --git a/test/e2e/annotations_test.go b/test/e2e/annotations_test.go
index 03b895b..84a71c9 100644
--- a/test/e2e/annotations_test.go
+++ b/test/e2e/annotations_test.go
@@ -31,6 +31,7 @@ var _ = Describe("Annotations base test.", Label("deployment-annotations-ns"), f
                        testData = test
                        mainCycle(test)
                },
+               // TODO: fix test for deletion protection on, as it would fail to re-take the cluster after deletion
                Entry("Simple configuration with keep resource policy annotation on deployment", Label("ns-crd"),
                        model.DataProvider(
                                "operator-ns-crd",

Signed-off-by: Jose Vazquez <jose.vazquez@mongodb.com>
@josvazg josvazg force-pushed the CLOUDP-178750/deployment-del-protect branch from 0deaf1e to 94bb692 Compare August 10, 2023 09:40
@josvazg
Copy link
Collaborator Author

josvazg commented Aug 10, 2023

go.mod issue fixed (missed a go mod tidy)

@josvazg josvazg marked this pull request as ready for review August 10, 2023 12:09
Copy link
Collaborator

@igor-karpukhin igor-karpukhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Copy link
Collaborator

@roothorp roothorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love all the testing :D

@josvazg josvazg merged commit 7b4825e into main Aug 10, 2023
38 checks passed
@josvazg josvazg deleted the CLOUDP-178750/deployment-del-protect branch August 10, 2023 14:29
@josvazg josvazg mentioned this pull request Aug 10, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants