-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(cache): adds certificate approver permission to kubeflow-pipelines-cache-deployer-role. Fixes #4138 #4246
fix(cache): adds certificate approver permission to kubeflow-pipelines-cache-deployer-role. Fixes #4138 #4246
Conversation
Hi @eedorenko. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
Hello. What kind of Kubernetes cluster are you using? It looks like the permissions in this PR should have already been granted: https://github.com/kubeflow/pipelines/blob/master/manifests/kustomize/base/cache-deployer/cluster-scoped/cache-deployer-clusterrole.yaml Do you have that role installed? |
/ok-to-test |
AKS 1.18.4. And yes, it looks like it fixes #4138 |
Should this be added to Role or ClusterRole? |
@eedorenko can you adapt your PR to remove duplicates with https://github.com/kubeflow/pipelines/blob/master/manifests/kustomize/base/cache-deployer/cluster-scoped/cache-deployer-clusterrole.yaml? We are deploying both a clusterrole and a role for cache-deployer, note https://kubernetes.io/docs/reference/access-authn-authz/rbac/#role-and-clusterrole, only permissions for cluster scoped resources are declared in the clusterrole. |
@Bobgy Done. |
@@ -15,10 +15,20 @@ rules: | |||
- delete | |||
- get | |||
- update | |||
- list |
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.
Why was list and watch needed?
They were not mentioned in your PR's description error message.
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.
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.
Can you confirm if this works without them?
as far as I know, current permissions are enough, we are not listing or watching them
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 confirm. Updated the PR
Thanks for the contribution! @eedorenko @haibingzhao, this should have fixed the issue you reported. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy 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 |
…s-cache-deployer-role. Fixes kubeflow#4138 (kubeflow#4246) * certificat approval * update pr * update pr * update pr * update pr * remove list&watch
@eedorenko Please check #4383 |
…s-cache-deployer-role. Fixes kubeflow#4138 (kubeflow#4246) (kubeflow#4383) * certificat approval * update pr * update pr * update pr * update pr * remove list&watch Co-authored-by: Eugene Fedorenko <eugene.fedor@gmail.com>
Fixes #4138
Adds certificate approver permission to kubeflow-pipelines-cache-deployer-role.
Without this permission cache-deployer-deployment crashes with the following error:
Error from server (Forbidden): certificatesigningrequests.certificates.k8s.io "cache-server.kubeflow" is forbidden: user not permitted to approve requests with signerName "kubernetes.io/legacy-unknown