-
Notifications
You must be signed in to change notification settings - Fork 110
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 preflight check implementation, add permission validation preflight check #887
Add preflight check implementation, add permission validation preflight check #887
Conversation
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.
Looks pretty good, thank you so much for putting in the effort @everettraven ❤️
I do want to double check my understanding on the different validations. We are trying to ensure that:
- User has permissions to create the resources that they want to create.
- For rbac resources, they should only be able to add the permissions that they currently have?
I didn't think of the second use case before, really glad that you have covered this as well 🙇🏻
3178400
to
77bd776
Compare
6c76651
to
d4f2210
Compare
d4f2210
to
8630103
Compare
Signed-off-by: everettraven <everettraven@gmail.com>
8630103
to
91f533f
Compare
@praveenrewar I've updated the e2e tests to fix the CI failure. Checked that it passed locally with a fresh cluster this time around. |
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! Thank you so much for working on this!!
@100mik I am keeping it open in case you are still taking a look.
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! Thanks for the contribution ❤️
@everettraven we will need some documentation changes to carvel-dev/carvel around the --preflight
flag. I do not think it should block this PR! However, we should make sure we have these in place before we push out a release.
I created a separate issue to track the required documentation changes 🙌🏼 |
What this PR does / why we need it:
--preflight
) tokapp deploy
that can be used to enable/disable preflight checks. Can be used in the format--preflight=CheckName=true,...
--preflight=PermissionValidation=true
(Cluster)Roles
it will ensure that a user has all the necessary permissions to create/update the resource without privilege escalation or has the "escalate" permissions(Cluster)RoleBindings
it will ensure that a user has all the necessary permissions to create/update the resource without privilege escalation or has the "bind" permissionsWhich issue(s) this PR fixes:
Fixes #855
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: