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

Ensure when non-admin is NOT enabled, the creation of NABCR/NARCR returns a proper error message #43

Open
weshayutin opened this issue Apr 9, 2024 · 11 comments

Comments

@weshayutin
Copy link
Contributor

No description provided.

@shubham-pampattiwar
Copy link
Member

Is the expectation that the NAB/NAR CR will have an error status ? I believe it would not be possible as the NAC controllers (NAB/NAR) would not exist if non-admin feature is not enabled. Is the expectation something different ?

@mateusoliveira43
Copy link
Contributor

as discussed, look what happens when a velero CR is created and no velero deployment is live and have consistent behavior

@shawn-hurley
Copy link

What does happen when that is the case?

@mpryc
Copy link
Collaborator

mpryc commented Apr 10, 2024

@shawn-hurley currently when there is no controller running the NonAdminBackup created by the user will never get update of the Phase field within Status nor Conditions within status will ever get created, so really nothing will happen on that object.

Could you tell me what is the expected behavior in the following cases:
User creates NonAdminBackup:

  1. When OADP is installed, but no DPA is created yet (who takes responsibility of updating Status of NAB object)?
  2. When OADP is installed, DPA is created but without NAC controller ?
  3. OADP Operator is not installed on the system at all

Isn't that overall k8s issue with objects that do not have any controller managing them rather than OADP responsibility?
The above 3 cases are valid and especially in the last one we won't have consistent action on the NonAdminBackup object updates, because missing OADP Operator also includes case where NAC controller won't be running.

@shawn-hurley
Copy link

When OADP is installed, but no DPA is created yet (who takes responsibility of updating Status of NAB object)?

I hope we have some way of alerting users that the resources they will create will have no effect. I would even prefer that we prevent the users from taking this action.

When OADP is installed, DPA is created but without NAC controller ?

I think that the same is true as the above statement.

OADP Operator is not installed on the system at all

I think the only situation where this could happen is:

  1. Uninstall w/o deleting the CRDs
  2. Admin is doing some odd stuff

For 2, I don't think there is anything we can do. For 1, I thought that OLM would delete the aggregation to admin on deletion, so the user should be unable to create. If that is not the case, then just like for 2, there is not a lot we can do.

I don't think that just because we can't do a lot ourselves for 3 doesn't mean we should make the experience worse for the first two.

Inconsistent behavior is fine if it is because it is degrading nicely IM). Which, in this case, given the constraints, we are doing the best we can to alert the user to the situation based on what tools we have available.

Let me know if you want to talk about this more

@mateusoliveira43
Copy link
Contributor

As a note, if you uninstall operator through CLI or UI, CRDs are not deleted. It is a manual extra step

@mateusoliveira43
Copy link
Contributor

@shawn-hurley do you have an example operator that does this warning/error message about CRDs missing controllers deployed?

@shawn-hurley
Copy link

The problem is that we do have a controller installed right, we have OADP, and when OADP is installed now things appear.

@mateusoliveira43
Copy link
Contributor

agree, I think the problem is not a NAC problem, but OADP itself, because this happens to velero objects (velero controller only appears after creating DPA)

@shubham-pampattiwar
Copy link
Member

@shawn-hurley Is the expectation that the DPA status tells that the user's that "Non-Admin feature not enabled but NAB CR found, please enable NAC for processing this NAB request"
OR you want this error on NAB CR status itself but that status update call is done via DPA controller.

For first case, non-admin user will need access to OADP install NS for viewing DPA status, right ?
For second case, isnt this an Anti-Pattern that DPA controller patches NAB CR status ?

@shawn-hurley
Copy link

I was thinking that you could have a web hook that would prevent the user from creating the resource would be the correct thing

The next option, would be to consider a way for the NAB CRD's to not be installed unless the user wanted NAC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

5 participants