kubernetes-csi / external-snapshotter

Sidecar container that watches Kubernetes Snapshot CRD objects and triggers CreateSnapshot/DeleteSnapshot against a CSI endpoint.
Apache License 2.0
502 stars 375 forks source link

remove validation-webhook as it is deprecated #1186

Closed yati1998 closed 5 days ago

yati1998 commented 2 weeks ago

removes validation webhook as it is being deprecated

fixes: #1181

The validation webhook was deprecated in v8.0.0 and it is now removed.
The validation webhook would prevent creating multiple default volume snapshot classes and multiple default volume group snapshot classes for the same CSI driver. With the removal of the validation webhook, an error will still be raised when dynamically provisioning a VolumeSnapshot or VolumeGroupSnapshot when multiple default volume snapshot classes or multiple default volume group snapshot classes for the same CSI driver exist.
k8s-ci-robot commented 2 weeks ago

Hi @yati1998. Thanks for your PR.

I'm waiting for a kubernetes-csi 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
leonardoce commented 2 weeks ago

The validation webhook would prevent creating multiple default volume snapshot classes (and multiple volume group snapshot classes) for the same CSI driver. The CEL rules we included won't do that, and the corresponding error will be raised when dynamically provisioning a VolumeSnapshot or VolumeGroupSnapshot.

In my opinion, this is a behavior change that we should appropriately describe in the documentation and the release notes.

xing-yang commented 2 weeks ago

/ok-to-test

Please fix your commit message.

xing-yang commented 1 week ago

The validation webhook would prevent creating multiple default volume snapshot classes (and multiple volume group snapshot classes) for the same CSI driver. The CEL rules we included won't do that, and the corresponding error will be raised when dynamically provisioning a VolumeSnapshot or VolumeGroupSnapshot.

In my opinion, this is a behavior change that we should appropriately describe in the documentation and the release notes.

@leonardoce Can you suggest some wording for this change in the documentation and the release notes?

leonardoce commented 6 days ago

@yati1998 can you please fix the commit message? The subject line has a typo:

remove validation-webhook as it is being depriciated
leonardoce commented 6 days ago

The following comment lines need to be updated because they are still referring to the webhook:

https://github.com/kubernetes-csi/external-snapshotter/blob/9cb9f0f1a7059f9af97f16173ac9aab912a2b989/pkg/common-controller/groupsnapshot_controller_helper.go#L306 https://github.com/kubernetes-csi/external-snapshotter/blob/9cb9f0f1a7059f9af97f16173ac9aab912a2b989/pkg/common-controller/groupsnapshot_controller_helper.go#L1123 https://github.com/kubernetes-csi/external-snapshotter/blob/9cb9f0f1a7059f9af97f16173ac9aab912a2b989/pkg/common-controller/snapshot_controller.go#L95 https://github.com/kubernetes-csi/external-snapshotter/blob/9cb9f0f1a7059f9af97f16173ac9aab912a2b989/pkg/common-controller/snapshot_controller.go#L196

Are we still using these labels? https://github.com/kubernetes-csi/external-snapshotter/blob/9cb9f0f1a7059f9af97f16173ac9aab912a2b989/pkg/utils/util.go#L153-L158

At first glance, they seem to be used only by a few functions in the testing framework, and those functions appear to be not used anymore.

yati1998 commented 6 days ago

Are we still using these labels?

https://github.com/kubernetes-csi/external-snapshotter/blob/9cb9f0f1a7059f9af97f16173ac9aab912a2b989/pkg/utils/util.go#L153-L158

At first glance, they seem to be used only by a few functions in the testing framework, and those functions appear to be not used anymore.

is this related webhook? maybe we can cover this in different PR

xing-yang commented 6 days ago

@yati1998 Can you open an issue for the invalid labels to be removed in a different PR? Can you please address other comments?

leonardoce commented 5 days ago

It looks fine; we still need the release notes block in the PR's body.

yati1998 commented 5 days ago

@leonardoce @xing-yang can you please review the PR again?

leonardoce commented 5 days ago

/lgtm

xing-yang commented 5 days ago

/lgtm /approve

k8s-ci-robot commented 5 days ago

[APPROVALNOTIFIER] This PR is APPROVED

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

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