kubernetes-csi / external-provisioner

Sidecar container that watches Kubernetes PersistentVolumeClaim objects and triggers CreateVolume/DeleteVolume against a CSI endpoint
Apache License 2.0
328 stars 318 forks source link

add patch verb for persistentvolumes resources in the external-provisioner-runner clusterrole #1155

Closed carlory closed 2 months ago

carlory commented 5 months ago

What type of PR is this?

/kind bug

What this PR does / why we need it:

the feature-gate HonorPVReclaimPolicy is enabled, create a pvc with a delete relaim policy, then delete the pvc, the pv stuck in deleting status, the error message is:

csi-mockplugin-0/csi-provisioner@kind..lane: I0206 11:08:41.536819       1 controller.go:1523] delete "pvc-e4b6e20e-4d77-4bf0-8d96-9f47212e4b38": failed to remove finalizer for persistentvolume: persistentvolumes "pvc-e4b6e20e-4d77-4bf0-8d96-9f47212e4b38" is forbidden: User "system:serviceaccount:csi-mock-honor-pv-reclaim-policy-99-996:csi-mock" cannot update resource "persistentvolumes" in API group "" at the cluster scope: RBAC: [clusterrole.rbac.authorization.k8s.io "cluster-driver-registrar-runner-csi-mock-honor-pv-reclaim-policy-99" not found, clusterrole.rbac.authorization.k8s.io "e2e-test-privileged-psp" not found]
csi-mockplugin-0/csi-provisioner@kind..lane: W0206 11:08:41.536850       1 controller.go:989] Retrying syncing volume "pvc-e4b6e20e-4d77-4bf0-8d96-9f47212e4b38", failure 0
csi-mockplugin-0/csi-provisioner@kind..lane: E0206 11:08:41.536880       1 controller.go:1007] error syncing volume "pvc-e4b6e20e-4d77-4bf0-8d96-9f47212e4b38": persistentvolumes "pvc-e4b6e20e-4d77-4bf0-8d96-9f47212e4b38" is forbidden: User "system:serviceaccount:csi-mock-honor-pv-reclaim-policy-99-996:csi-mock" cannot update resource "persistentvolumes" in API group "" at the cluster scope: RBAC: [clusterrole.rbac.authorization.k8s.io "cluster-driver-registrar-runner-csi-mock-honor-pv-reclaim-policy-99" not found, clusterrole.rbac.authorization.k8s.io "e2e-test-privileged-psp" not found]
I0206 19:08:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Action required: the external-provisioner now needs permissions to patch persistentvolumes. Please update your RBACs appropriately. See the linked pull request for an example.  
carlory commented 5 months ago

/cc @deepakkinni @xing-yang

xing-yang commented 5 months ago

To add a new RBAC rule, we need to bump the major version.

xing-yang commented 5 months ago

To add a new RBAC rule, we need to bump the major version.

I think we should probably do this following the K8s 1.30 release.

msau42 commented 4 months ago

Can we use patch instead of update?

carlory commented 4 months ago

Can we use patch instead of update?

cc @deepakkinni

deepakkinni commented 3 months ago

Can we use patch instead of update?

Unfortunately, we use update https://github.com/kubernetes-sigs/sig-storage-lib-external-provisioner/blob/master/controller/controller.go#L1322-L1328

Having read about Patch now, I do see its benefits. Would you prefer if we switched out the update to patch?

xing-yang commented 3 months ago

Yes, try to switch to patch, please.

carlory commented 3 months ago

/hold

Waiting for https://github.com/kubernetes-sigs/sig-storage-lib-external-provisioner/pull/164 merged.

xing-yang commented 3 months ago

Can you change from "update verb" to "patch verb" in the release note?

carlory commented 3 months ago

@xing-yang updated.

deepakkinni commented 3 months ago

/approve

xing-yang commented 2 months ago

@carlory Can we cancel the hold?

xing-yang commented 2 months ago

/hold cancel

xing-yang commented 2 months ago

/lgtm /approve

k8s-ci-robot commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, deepakkinni, xing-yang

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: - ~~[OWNERS](https://github.com/kubernetes-csi/external-provisioner/blob/master/OWNERS)~~ [xing-yang] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
carlory commented 2 months ago

/hold

carlory commented 2 months ago

we need to bump the vendor

carlory commented 2 months ago

cc @xing-yang

carlory commented 2 months ago

/hold cancel