fix: NAC install validation #1577

mateusoliveira43 · 2024-10-25T18:30:21Z

Why the changes were made

Only allow one NAC install per cluster.

Also, add validation for only allowing one DPA per OADP installation namespace.

How to test the changes made

Deploy 2 (or more) OADP operators (make deploy-olm) in different namespaces. Create 2 (or more) DPAs in those namespaces with NAC enabled. Only one NAC should be deployed and DPAs should have error in their statuses.

openshift-ci · 2024-10-25T18:30:25Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-10-25T18:30:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mateusoliveira43]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mateusoliveira43 · 2024-10-25T18:34:21Z

controllers/nonadmin_controller.go

+		return false, err
+	}
+	dpaList := &oadpv1alpha1.DataProtectionApplicationList{}
+	err = r.List(r.Context, dpaList, &client.ListOptions{FieldSelector: selector})


A corner case here should be if 2 (or more) DPAs are created at the same time. But since DPA reconciles every minute, that's not to bad? (DPAs would have error message in their statuses, but multiple NAC would also exist in the cluster. To really avoid this, before erroring out, could delete NAC deployment)

Another question here: since DPA reconciles every minute, its too much to do a cluster wide get call every minute?

I would compare creationTimestamp. when a second reconcile happen if ever, the one with later creationTimestamp would remove its created NAC.

Or a more drastic change would be to move this controller out of the current ReconcileBatch into its own thing.

thinking again, I think we do not need to worry about having multiple NAC deployments after validation

If 2 DPAs with NAC enabled are created at the same time, when they reach this part, both of them will not deploy NAC

If one DPA is created and after some time another is created, only first DPA NAC will be deployed

Right?

controllers/nonadmin_controller.go

controllers/validator.go

controllers/nonadmin_controller.go

weshayutin · 2024-11-13T01:28:14Z

@shubham-pampattiwar it's not clear to me we've come to a conclusion on 1 NAC per cluster vs. a NAC married to a particular OADP deployment. If this is a temporary measure until we can put in the code to pair an oadp deployment with a NAC than I think this is fine.

shubham-pampattiwar · 2024-11-13T01:32:19Z

This is not a temporary measure. The current PR implements the behavior that:

Only one NAC exists per cluster
you can install multiple OADP Operators in the cluster but only of them can deploy/enable NAC feature

weshayutin · 2024-11-13T01:55:32Z

Upon installing OADP in another namespace w/ NAC enabled.. A user gets the following error message.

 lastTransitionTime: '2024-11-13T01:54:18Z'
      message: only a single instance of Non-Admin Controller can be installed across the entire cluster. Non-Admin controller is also configured to be installed in openshift-adp namespace
      reason: Error

weshayutin · 2024-11-13T01:57:32Z

/test ci/prow/4.18-dev-preview-e2e-test-aws

openshift-ci · 2024-11-13T01:57:36Z

@weshayutin: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test 4.14-ci-index
/test 4.14-e2e-test-aws
/test 4.14-e2e-test-kubevirt-aws
/test 4.14-images
/test 4.15-ci-index
/test 4.15-e2e-test-aws
/test 4.15-e2e-test-kubevirt-aws
/test 4.15-images
/test 4.16-ci-index
/test 4.16-e2e-test-aws
/test 4.16-e2e-test-kubevirt-aws
/test 4.16-images
/test 4.17-ci-index
/test 4.17-e2e-test-aws
/test 4.17-e2e-test-kubevirt-aws
/test 4.17-images
/test 4.18-dev-preview-ci-index
/test 4.18-dev-preview-images
/test images
/test unit-test

The following commands are available to trigger optional jobs:

/test 4.18-dev-preview-e2e-test-aws

Use /test all to run all jobs.

In response to this:

/test ci/prow/4.18-dev-preview-e2e-test-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

shubham-pampattiwar · 2024-11-13T04:48:44Z

controllers/nonadmin_controller.go

+	}
+	for _, dpa := range dpaList.Items {
+		if (&DPAReconciler{dpa: &dpa}).checkNonAdminEnabled() {
+			return false, fmt.Errorf("only a single instance of Non-Admin Controller can be installed across the entire cluster. Non-Admin controller is also configured to be installed in %s namespace", dpa.Namespace)


Lets re-frame the error text a little bit: only a single instance of Non-Admin Controller can be installed across the entire cluster. Non-Admin controller is already configured and installed in %s namespace", dpa.Namespace

I would like to keep as it is

from what I tested, NAC will never be deployed 2 or more times. Either one is installed (one was installed and now a second is trying to be installed) or none (2 or more are trying to be installed at the same time)

So, your suggestion would make sense to the case of the DPA that tried to install NAC and it already exists in cluster, but not for the DPA that already installed it, or other cases (error messages will appear in all DPAs, because DPA reconciles every minute). Do you agree?

User does the following:

Creates DAP 1

Enables NAC for DPA 1

Now Another NS, OADP is installed and DPA 2 is created there

User enabled NAC for DPA 2
I want error message only on DPA 2.

To have error only on "wrong" DPAs, I would need to know if NAC is already deployed (do a GET call for deployments in OADP namespace) prior to erroring out. Would this not complicate validation too much? (and every minute, DPA would do GET call)

DPA 1 did nothing wrong, why should DPA 1 be errored out ?

Just because we can not be sure that error is not in DPA 1

examples

DPA 1 is created first with NAC enabled -> DPA 2 created after with NAC enabled -> by doing a deployment get call we can add error only to DPA 2 ✅

DPA 1 and DPA 2 are created (or edited) at the same time with NAC enabled -> in this case, which DPA we error out? ❌

For case 2, we don't decide that, let the DPA controller do its work. Either of the DPAs will get the error for sure, right ?

We could create cluster-wide ConfigMap Lock in the kube-system namespace to ensure one lease/lock is acquired by running NAC or using lease for that until nac is gone https://kubernetes.io/docs/concepts/architecture/leases/.

Then during DPA reconcile check if this lease/lock is acquired and if yes ensure it's actually not orphant and then run the NAC.

Another common option is actually to use leader election to achieve this, but this would be for entire operator rather then one controller unless there is a way to dynamically set it during DPA reconcile:
https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/operators/developing-operators#osdk-leader-election

@shubham-pampattiwar that is my fear. I think with your suggested approach, both will deploy NAC

@mpryc NAC has leader election, I will test what happens

kaovilai · 2024-11-13T05:29:17Z

/test 4.18-dev-preview-e2e-test-aws

mpryc · 2024-11-13T09:16:22Z

controllers/nonadmin_controller.go

-		dpa.Spec.NonAdmin.Enable != nil {
-		return *dpa.Spec.NonAdmin.Enable
+	if r.dpa.Spec.NonAdmin != nil && r.dpa.Spec.NonAdmin.Enable != nil {
+		return *r.dpa.Spec.NonAdmin.Enable && r.dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck] == TrueVal


Do we need to ensure the comparison is against "True", "TRUE" or "true", so regardless of capital/small letters?

import ( "strings" [...] return *r.dpa.Spec.NonAdmin.Enable && strings.EqualFold(r.dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck], TrueVal)

no need, the error message if user is not using right value tells how to set it up

oadp-operator/controllers/validator.go

Line 78 in 4b043ba

return false, errors.New("in order to enable/disable the non-admin feature please set dpa.spec.unsupportedOverrides[tech-preview-ack]: 'true'")

mpryc · 2024-11-13T09:17:38Z

controllers/validator.go

-	if r.checkNonAdminEnabled() {
-		if !(dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck] == TrueVal) {
+	if r.dpa.Spec.NonAdmin != nil && r.dpa.Spec.NonAdmin.Enable != nil && *r.dpa.Spec.NonAdmin.Enable {
+		if !(r.dpa.Spec.UnsupportedOverrides[oadpv1alpha1.TechPreviewAck] == TrueVal) {


Same ask as above (comparison against capital/small letters) ?

mpryc · 2024-11-13T12:30:26Z

/hold as discussed offline NAC controller check should be moved to validations to ensure there won't be situation where "wrong" DPA will allow velero pod to be running and then fail DPA reconcile with error.

Signed-off-by: Mateus Oliveira <[email protected]>

improve error messages Signed-off-by: Mateus Oliveira <[email protected]>

add tests Signed-off-by: Mateus Oliveira <[email protected]>

working example Signed-off-by: Mateus Oliveira <[email protected]>

use another client Signed-off-by: Mateus Oliveira <[email protected]>

validate first Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 · 2024-11-13T13:27:24Z

/unhold

openshift-ci · 2024-11-13T16:28:03Z

@mateusoliveira43: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.17-e2e-test-aws	`8bc854d`	link	true	`/test 4.17-e2e-test-aws`
ci/prow/4.17-e2e-test-kubevirt-aws	`8bc854d`	link	true	`/test 4.17-e2e-test-kubevirt-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2024

mateusoliveira43 commented Oct 25, 2024

View reviewed changes

controllers/nonadmin_controller.go Show resolved Hide resolved

shubham-pampattiwar reviewed Nov 1, 2024

View reviewed changes

controllers/validator.go Outdated Show resolved Hide resolved

controllers/validator.go Show resolved Hide resolved

controllers/validator.go Outdated Show resolved Hide resolved

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 marked this pull request as ready for review November 5, 2024 19:52

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2024

openshift-ci bot requested review from mpryc and shubham-pampattiwar November 5, 2024 19:53

mateusoliveira43 commented Nov 5, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2024

mateusoliveira43 commented Nov 7, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2024

shubham-pampattiwar reviewed Nov 13, 2024

View reviewed changes

mpryc reviewed Nov 13, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024

mateusoliveira43 added 2 commits November 13, 2024 10:23

fix: NAC install validation

001b0d6

Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: NAC install validation

4d10245

improve error messages Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 added 4 commits November 13, 2024 10:23

fixup! fix: NAC install validation

b483662

add tests Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: NAC install validation

96d4056

working example Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: NAC install validation

acab81f

use another client Signed-off-by: Mateus Oliveira <[email protected]>

fixup! fix: NAC install validation

8bc854d

validate first Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 force-pushed the fix/nac-install-validation branch from 6a598cc to 8bc854d Compare November 13, 2024 13:23

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024

mateusoliveira43 mentioned this pull request Nov 13, 2024

Filter which namespaces NAC controller watches migtools/oadp-non-admin#103

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: NAC install validation #1577

fix: NAC install validation #1577

mateusoliveira43 commented Oct 25, 2024 •

edited

Loading

openshift-ci bot commented Oct 25, 2024

openshift-ci bot commented Oct 25, 2024

mateusoliveira43 Oct 25, 2024

kaovilai Oct 25, 2024 •

edited

Loading

kaovilai Oct 25, 2024

mateusoliveira43 Nov 1, 2024

This comment was marked as outdated.

This comment was marked as outdated.

weshayutin commented Nov 13, 2024

shubham-pampattiwar commented Nov 13, 2024

weshayutin commented Nov 13, 2024

weshayutin commented Nov 13, 2024

openshift-ci bot commented Nov 13, 2024

shubham-pampattiwar Nov 13, 2024

mateusoliveira43 Nov 13, 2024

shubham-pampattiwar Nov 13, 2024 •

edited

Loading

mateusoliveira43 Nov 13, 2024

shubham-pampattiwar Nov 13, 2024

mateusoliveira43 Nov 13, 2024

shubham-pampattiwar Nov 13, 2024 •

edited

Loading

mpryc Nov 14, 2024

mateusoliveira43 Nov 14, 2024 •

edited

Loading

kaovilai commented Nov 13, 2024

mpryc Nov 13, 2024

mateusoliveira43 Nov 13, 2024

mpryc Nov 13, 2024

mpryc commented Nov 13, 2024

mateusoliveira43 commented Nov 13, 2024

openshift-ci bot commented Nov 13, 2024

fix: NAC install validation #1577

Are you sure you want to change the base?

fix: NAC install validation #1577

Conversation

mateusoliveira43 commented Oct 25, 2024 • edited Loading

Why the changes were made

How to test the changes made

openshift-ci bot commented Oct 25, 2024

openshift-ci bot commented Oct 25, 2024

Choose a reason for hiding this comment

kaovilai Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

weshayutin commented Nov 13, 2024

shubham-pampattiwar commented Nov 13, 2024

weshayutin commented Nov 13, 2024

weshayutin commented Nov 13, 2024

openshift-ci bot commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham-pampattiwar Nov 13, 2024 • 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

shubham-pampattiwar Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateusoliveira43 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

kaovilai commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpryc commented Nov 13, 2024

mateusoliveira43 commented Nov 13, 2024

openshift-ci bot commented Nov 13, 2024

mateusoliveira43 commented Oct 25, 2024 •

edited

Loading

kaovilai Oct 25, 2024 •

edited

Loading

shubham-pampattiwar Nov 13, 2024 •

edited

Loading

shubham-pampattiwar Nov 13, 2024 •

edited

Loading

mateusoliveira43 Nov 14, 2024 •

edited

Loading