Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add non-admin controller design #18
Add non-admin controller design #18
Changes from 2 commits
edf3b31
d6ddfe9
8dd58cd
815de7f
8f902f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one I am confused of what will happen. Example: if I update after Velero started (or finished) the backup process, nothing will happen, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not even sure what happens to Velero if you update a backup after it starts. Not sure we need to make any promises here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(for backups -- for schedules, those can be updated, but see above as to whether we need schedules for the first iteration)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if you update backup spec, nac could create deleteBackupRequests, mark NAB as pending, then once old backup is gone, NAB will be processing with new backup of the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, updating backup spec makes sense as it just does exactly what a normal velero (admin) user modifying backup spec would do. If there's any modification that makes sense in velero, then it could be done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some validation to ensure that only one DPA in the cluster sets this to
true
. Otherwise, if there are 2 OADP instances, both enabling non-admin, we will have race conditions around which velero instance handles each NonAdminBackup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both instances in a supported scenario would be in separate oadp namespaces, I don't see a possible overlap here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm. agree with sseago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. scenario is this:
If both OADPs have non-admin enabled, it's a race condition as to which one grabs and labels/annotates it first (and creates Velero CR), and possibly in some cases you end up with both attempting to modify it at the same time. If only one of them has non-admin enabled, then that OADP will manage it, and the other OADP will stay out of the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think non admin use case will prop up only in env where multiple users share a cluster. Therefore I think we need to ensure that multiple non admin controllers per velero is supported, so we need a way to limit the scope of each nac so it doesn't overlap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/namepsace/namespace/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham-pampattiwar please update this such that more than one OADP/NAC combo can be installed on the same cluster. Requests will have to be namespace filtered. @mpryc @mateusoliveira43 I suppose the namespaces for the NAC will be configured in the DPA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not discussed, I believe. But one option, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this means users will not be managing their own BSLs/buckets. They will have to know the name of the configured BSL (given by admin), and the NAC will assume that any user who knows the name of a BSL is permitted to back up to that BSL. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, any NAC user who does not specify/know the BSL name will be considered approved to use the default BSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will have a way for admin to scope who can use which bsl, the nab/nar controller will validate accordingly before processing the CRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that require a CR, or will it be managed via annotations/labels/etc on the BSL itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple instances of OADP operator can exist, but we must make sure that no more than one of them enable non-admin. If a second DPA adds enableNonAdmin, that should trigger a validation error.