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

helm: additional cluster roles for secretproviderclasses #836

Merged

Conversation

anapsix
Copy link
Contributor

@anapsix anapsix commented Jan 11, 2022

What this PR does / why we need it:
At the moment, there are no ClusterRoles created for SecretProviderClasses resources with aggregation to view and admin default roles. This prevents ClusterRole/admin and ClusterRole/view from accessing SecretProviderClasses resources.

Which issue(s) this PR fixes:
Fixes #835

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 11, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @anapsix!

It looks like this is your first PR to kubernetes-sigs/secrets-store-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/secrets-store-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 11, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @anapsix. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 11, 2022
@anapsix anapsix force-pushed the adding-admin-and-viewer-roles branch 2 times, most recently from 98bf1f1 to cae278f Compare January 11, 2022 16:34
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jan 11, 2022
Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anapsix Thank you for the PR.

Please update the yamls in the manifest_staging/ folder, where we host the staging charts and deployment yamls. All the yaml changes will then be promoted into the released charts folder (charts/) with the next release.

@aramase
Copy link
Member

aramase commented Jan 12, 2022

/retitle helm: additional cluster roles for secretproviderclasses

@k8s-ci-robot k8s-ci-robot changed the title additional cluster roles for secretproviderclasses helm: additional cluster roles for secretproviderclasses Jan 12, 2022
@anapsix
Copy link
Contributor Author

anapsix commented Jan 12, 2022

@anapsix Thank you for the PR.

Please update the yamls in the manifest_staging/ folder, where we host the staging charts and deployment yamls. All the yaml changes will then be promoted into the released charts folder (charts/) with the next release.

I believe I did @aramase. Please clarify if I should remove changes to manifest templates in charts/secrets-store-csi-driver/templates/ in favor of manifest_staging/charts/secrets-store-csi-driver/templates/?

@aramase
Copy link
Member

aramase commented Jan 12, 2022

I believe I did @aramase. Please clarify if I should remove changes to manifest templates in charts/secrets-store-csi-driver/templates/ in favor of manifest_staging/charts/secrets-store-csi-driver/templates/?

that's right! the changes are only made in manifest_staging. So you would need to remove the changes from charts/secrets-store-csi-driver/templates/.

Adding Admin and Viewer cluster roles, aggregaring to default "admin"
and "view" ClusterRoles.
@anapsix anapsix force-pushed the adding-admin-and-viewer-roles branch from cae278f to e359b31 Compare January 12, 2022 19:44
@anapsix
Copy link
Contributor Author

anapsix commented Jan 12, 2022

Roger that, @aramase. Thanks for the clarification. Done.

@aramase
Copy link
Member

aramase commented Jan 12, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 12, 2022
@tam7t
Copy link
Contributor

tam7t commented Jan 25, 2022

I think this is good but we'd probably also want to add something for the manifest_staging/deploy/ so that they could be added with just the kubectl apply -f deploy/* in the future. (we can do that as a separate PR)

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2022
@anapsix
Copy link
Contributor Author

anapsix commented Jan 25, 2022

I think this is good but we'd probably also want to add something for the manifest_staging/deploy/ so that they could be added with just the kubectl apply -f deploy/* in the future. (we can do that as a separate PR)

/lgtm

@tam7t, added the new ClusterRoles to the manifest_staging/deploy. Please let me know if I did not do it the way you've expected.

@anapsix
Copy link
Contributor Author

anapsix commented Jan 25, 2022

Also, should I add the manifests to test/bats/tests/e2e_provider/?
And the following to the Test rbac roles and role bindings exist test in test/bats/e2e-provider.bats?:

  run kubectl get clusterrole/secretproviderclasses-admin-role
  assert_success

  run kubectl get clusterrole/secretproviderclasses-viewer-role
  assert_success

@tam7t
Copy link
Contributor

tam7t commented Jan 26, 2022

Thanks! And yeah adding a case for test/bats/tests/e2e_provider would be great as a sanity check that the roles apply successfully

@tam7t
Copy link
Contributor

tam7t commented Jan 26, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2022
Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anapsix, aramase

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2022
@aramase
Copy link
Member

aramase commented Jan 26, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 26, 2022
@k8s-ci-robot k8s-ci-robot merged commit d32ca72 into kubernetes-sigs:main Jan 26, 2022
@anapsix
Copy link
Contributor Author

anapsix commented Jan 26, 2022

Thanks! And yeah adding a case for test/bats/tests/e2e_provider would be great as a sanity check that the roles apply successfully

eh.. too late, @tam7t .. sorry I didn't make in time before the merge
Btw, wouldn't make more sense to symlink some of those e2e manifest to the ones in manifest_staging?

anapsix added a commit to anapsix/secrets-store-csi-driver that referenced this pull request Jan 28, 2022
Adding simple tests to verify the new aggregation roles are applied
k8s-ci-robot pushed a commit that referenced this pull request Jan 28, 2022
* adding naive tests as folllow-up to #836

Adding simple tests to verify the new aggregation roles are applied

* remove extra manifests, install new roles in e2e-deploy-manifest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing aggregation ClusterRoles for SecretProviderClasses
4 participants