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

fix(cache): adds certificate approver permission to kubeflow-pipelines-cache-deployer-role. Fixes #4138 #4246

Conversation

eedorenko
Copy link
Contributor

@eedorenko eedorenko commented Jul 19, 2020

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

@kubeflow-bot
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

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 /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.

@Ark-kun
Copy link
Contributor

Ark-kun commented Jul 20, 2020

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?

@Ark-kun Ark-kun requested review from rui5i, Ark-kun and Bobgy and removed request for IronPan and rmgogogo July 20, 2020 06:58
@Bobgy
Copy link
Contributor

Bobgy commented Jul 20, 2020

@Ark-kun note that one permission is for signer, that's a new resource in later Kubernetes versions.
The first report of this issue was in #4138.

So this should fix the permission issue for k8s 1.18

@Bobgy Bobgy changed the title fix(cache): adds certificate approver permission to kubeflow-pipelines-cache-deployer-role fix(cache): adds certificate approver permission to kubeflow-pipelines-cache-deployer-role. Fixes #4138 Jul 20, 2020
@Bobgy
Copy link
Contributor

Bobgy commented Jul 20, 2020

/ok-to-test

@eedorenko
Copy link
Contributor Author

eedorenko commented Jul 20, 2020

AKS 1.18.4. And yes, it looks like it fixes #4138

@Ark-kun
Copy link
Contributor

Ark-kun commented Jul 20, 2020

Should this be added to Role or ClusterRole?

@Bobgy
Copy link
Contributor

Bobgy commented Jul 21, 2020

@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.

@eedorenko
Copy link
Contributor Author

@Bobgy Done.

@@ -15,10 +15,20 @@ rules:
- delete
- get
- update
- list
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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

Copy link
Contributor Author

@eedorenko eedorenko Jul 21, 2020

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

@Bobgy
Copy link
Contributor

Bobgy commented Jul 22, 2020

Thanks for the contribution! @eedorenko
/lgtm
/approve

@haibingzhao, this should have fixed the issue you reported.

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot merged commit 0ba88b0 into kubeflow:release-1.0 Jul 22, 2020
@Bobgy Bobgy added the cherrypick-approved area OWNER approves to cherry pick this PR to current active release branch label Jul 22, 2020
@jingzhang36 jingzhang36 added the cherrypicked cherry picked to release branch `release-x.y` label Aug 12, 2020
Ark-kun pushed a commit to Ark-kun/pipelines that referenced this pull request Aug 17, 2020
…s-cache-deployer-role. Fixes kubeflow#4138 (kubeflow#4246)

* certificat approval

* update pr

* update pr

* update pr

* update pr

* remove list&watch
@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 17, 2020

@eedorenko Please check #4383

k8s-ci-robot pushed a commit that referenced this pull request Aug 18, 2020
…s-cache-deployer-role. Fixes #4138 (#4246) (#4383)

* certificat approval

* update pr

* update pr

* update pr

* update pr

* remove list&watch

Co-authored-by: Eugene Fedorenko <eugene.fedor@gmail.com>
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cherrypick-approved area OWNER approves to cherry pick this PR to current active release branch cherrypicked cherry picked to release branch `release-x.y` cla: yes lgtm ok-to-test size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants