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.

Sign up for GitHub

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

Jump to bottom

remove validation-webhook as it is deprecated #1186

Merged
merged 1 commit into from
Nov 21, 2024
Merged

remove validation-webhook as it is deprecated #1186

merged 1 commit into from
Nov 21, 2024

Conversation

yati1998
Copy link
Contributor

@yati1998 yati1998 commented Nov 6, 2024 •
edited by xing-yang
Loading

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 k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 Nov 6, 2024
@k8s-ci-robot
Copy link
Contributor

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 6, 2024
@leonardoce
Copy link
Contributor

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
Copy link
Collaborator

/ok-to-test

Please fix your commit message.

@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 Nov 6, 2024
xing-yang
xing-yang reviewed Nov 6, 2024
View reviewed changes
README.md Show resolved Hide resolved
@yati1998 yati1998 force-pushed the webhook branch 3 times, most recently from 7eaada7 to a893053 Compare November 7, 2024 08:52
@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 Nov 7, 2024
@yati1998 yati1998 requested a review from xing-yang November 7, 2024 08:53
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2024
xing-yang
xing-yang reviewed Nov 18, 2024
View reviewed changes
README.md
@@ -6,9 +6,6 @@ The volume snapshot feature supports CSI v1.0 and higher. It was introduced as a

The volume group snapshot feature supports CSI v1.10.0 and higher, and have been introduced in [Kubernetes 1.27 as an alpha feature](https://kubernetes.io/blog/2023/05/08/kubernetes-1-27-volume-group-snapshot-alpha/).

> :warning: **WARNING**: There is a new validating webhook server which provides tightened validation on snapshot objects. This SHOULD be installed by all users of this feature. More details [below](#validating-webhook).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a warning here:
The validation webhook was deprecated in v8.0.0 and it is now removed.

README.md Show resolved Hide resolved
@xing-yang
Copy link
Collaborator

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?

xing-yang
xing-yang reviewed Nov 20, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@leonardoce
Copy link
Contributor

leonardoce commented Nov 20, 2024 •
edited
Loading

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

remove validation-webhook as it is being depriciated

@leonardoce
Copy link
Contributor

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

external-snapshotter/pkg/common-controller/groupsnapshot_controller_helper.go

Line 306 in 9cb9f0f

// Keep this check in the controller since the validation webhook may not have been deployed.

external-snapshotter/pkg/common-controller/groupsnapshot_controller_helper.go

Line 1123 in 9cb9f0f

// Keep this check in the controller since the validation webhook may not have been deployed.

external-snapshotter/pkg/common-controller/snapshot_controller.go

Line 95 in 9cb9f0f

// Keep this check in the controller since the validation webhook may not have been deployed.

external-snapshotter/pkg/common-controller/snapshot_controller.go

Line 196 in 9cb9f0f

// Keep this check in the controller since the validation webhook may not have been deployed.

Are we still using these labels?

external-snapshotter/pkg/utils/util.go

Lines 153 to 158 in 9cb9f0f

// VolumeSnapshotContentInvalidLabel is applied to invalid content as a label key. The value does not matter.
// See https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/177-volume-snapshot/tighten-validation-webhook-crd.md#automatic-labelling-of-invalid-objects
VolumeSnapshotContentInvalidLabel = "snapshot.storage.kubernetes.io/invalid-snapshot-content-resource"
// VolumeSnapshotInvalidLabel is applied to invalid snapshot as a label key. The value does not matter.
// See https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/177-volume-snapshot/tighten-validation-webhook-crd.md#automatic-labelling-of-invalid-objects
VolumeSnapshotInvalidLabel = "snapshot.storage.kubernetes.io/invalid-snapshot-resource"

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
Copy link
Contributor Author

Are we still using these labels?

external-snapshotter/pkg/utils/util.go

Lines 153 to 158 in 9cb9f0f

// VolumeSnapshotContentInvalidLabel is applied to invalid content as a label key. The value does not matter.
// See https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/177-volume-snapshot/tighten-validation-webhook-crd.md#automatic-labelling-of-invalid-objects
VolumeSnapshotContentInvalidLabel = "snapshot.storage.kubernetes.io/invalid-snapshot-content-resource"
// VolumeSnapshotInvalidLabel is applied to invalid snapshot as a label key. The value does not matter.
// See https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/177-volume-snapshot/tighten-validation-webhook-crd.md#automatic-labelling-of-invalid-objects
VolumeSnapshotInvalidLabel = "snapshot.storage.kubernetes.io/invalid-snapshot-resource"

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
Copy link
Collaborator

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

@yati1998 yati1998 changed the title remove validation-webhook as it is remove validation-webhook as it is depreciated Nov 21, 2024
leonardoce
leonardoce reviewed Nov 21, 2024
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@yati1998 yati1998 changed the title remove validation-webhook as it is depreciated remove validation-webhook as it is deprecated Nov 21, 2024
@yati1998 yati1998 force-pushed the webhook branch 2 times, most recently from 16cdd20 to 5c0cfe3 Compare November 21, 2024 08:26
@yati1998 yati1998 requested a review from leonardoce November 21, 2024 09:10
@leonardoce
Copy link
Contributor

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

@yati1998
remove validation-webhook as it is being deprecated
7697342 
This commit is the part of issue kubernetes-csi#1181

Signed-off-by: yati1998 <[email protected]>
@yati1998
Copy link
Contributor Author

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

@leonardoce
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2024
@xing-yang
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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 Nov 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit 0708ea1 into kubernetes-csi:master Nov 21, 2024
8 checks passed
@nikhilbarge nikhilbarge mentioned this pull request Nov 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@pohly pohly Awaiting requested review from pohly

@xing-yang xing-yang Awaiting requested review from xing-yang

@leonardoce leonardoce Awaiting requested review from leonardoce

Assignees

@xing-yang xing-yang

@leonardoce leonardoce

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove validation webhook
4 participants
@yati1998 @k8s-ci-robot @leonardoce @xing-yang