-
Notifications
You must be signed in to change notification settings - Fork 179
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 dedicated ServiceAccount for perftune jobs #1971
Conversation
81b5246
to
25f1cf9
Compare
@zimnx @rzetelskik ptal |
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.
lgtm
/assign zimnx
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rzetelskik, tnozicka 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:
Approvers can indicate their approval by writing |
|
||
// Delete any excessive Roles. | ||
// Delete has to be the first action to avoid getting stuck on quota. | ||
err := controllerhelpers.Prune( |
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.
shouldn't Prune complain about missing controllerref on deleted objects? We are not allowed to delete them. There should be an option to disable it on justified cases similar to apply functions.
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.
You already give it a set of claimed and pre-existing objects so it's assumed you own them by how you've claimed them in the fist place. If you were to do a patch or update instead you'd not have rechecked again. For the case of the helper it's cheap but I'd not couple it to this PR.
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 meant to add a safety check so we won't delete objects we weren't meant to delete. Filing an issue for future improvement is fine.
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.
}, | ||
ncc.eventRecorder) | ||
if err != nil { | ||
return fmt.Errorf("can't prune RoleBinding(s): %w", err) |
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.
return fmt.Errorf("can't prune RoleBinding(s): %w", err) | |
return fmt.Errorf("can't prune Role(s): %w", err) |
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.
fixed, thx
|
||
// Delete any excessive Roles. | ||
// Delete has to be the first action to avoid getting stuck on quota. | ||
err := controllerhelpers.Prune( |
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 meant to add a safety check so we won't delete objects we weren't meant to delete. Filing an issue for future improvement is fine.
Thanks for updates |
#1996 (comment) |
#1996 (comment) |
#1996 (comment) |
Description of your changes:
This PR adds a dedicated ServiceAccount to use for perftune jobs which enhances security isolation and allows some platforms (like OpenShift) to grant it extra permissions.
Which issue is resolved by this Pull Request:
Resolves #1975
Requires: