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

Add preflight check implementation, add permission validation preflight check #887

Merged

Conversation

everettraven
Copy link
Contributor

@everettraven everettraven commented Feb 6, 2024

What this PR does / why we need it:

  • Adds a flag (--preflight) to kapp deploy that can be used to enable/disable preflight checks. Can be used in the format --preflight=CheckName=true,...
  • Adds a preflight check for permission validation that is disabled by default. Can be enabled with --preflight=PermissionValidation=true
    • For (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
    • For (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" permissions
    • For all other resources and operations, it will ensure it has the necessary permissions to perform the changes required by the change set

Which issue(s) this PR fixes:

Fixes #855

Does this PR introduce a user-facing change?

Adds `--preflight` flag to the `kapp deploy` subcommand. This flag can be used to enable/disable specific preflight checks and follows the format `--preflight=CheckName=true,...`

Adds a PermissionValidation preflight check, disabled by default, that can be enabled with `--preflight=PermissionValidation=true`

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


pkg/kapp/cmd/core/deps_factory.go Outdated Show resolved Hide resolved
pkg/kapp/permissions/validator.go Outdated Show resolved Hide resolved
Copy link
Member

@praveenrewar praveenrewar left a 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 🙇🏻

pkg/kapp/cmd/appgroup/deploy.go Outdated Show resolved Hide resolved
pkg/kapp/preflight/registry.go Outdated Show resolved Hide resolved
pkg/kapp/permissions/preflight.go Outdated Show resolved Hide resolved
pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved
@everettraven everettraven force-pushed the feature/permission-validation branch 2 times, most recently from 3178400 to 77bd776 Compare February 14, 2024 17:24
@everettraven everettraven changed the title (wip): Add permission validation option to kapp deploy subcommand Add preflight check implementation, add permission validation preflight check Feb 14, 2024
@everettraven everettraven marked this pull request as ready for review February 14, 2024 17:32
@everettraven everettraven force-pushed the feature/permission-validation branch 2 times, most recently from 6c76651 to d4f2210 Compare February 15, 2024 20:36
@everettraven
Copy link
Contributor Author

@praveenrewar I've updated the e2e tests to fix the CI failure. Checked that it passed locally with a fresh cluster this time around.

Copy link
Member

@praveenrewar praveenrewar left a 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.

Copy link
Contributor

@100mik 100mik left a 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.

@100mik 100mik merged commit 63faf8a into carvel-dev:develop Feb 20, 2024
5 checks passed
@100mik
Copy link
Contributor

100mik commented Feb 20, 2024

I created a separate issue to track the required documentation changes 🙌🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Optional ability to check if client has permissions to perform CRUD actions before trying them
5 participants