Adding validation rules into CRDs for validation #1073

cici37 · 2024-04-24T21:14:09Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This is to add validation rules to CRDs which did the same validation as written in https://github.com/kubernetes-csi/external-snapshotter/blob/master/pkg/validation-webhook/validation.go

Which issue(s) this PR fixes:

Fixes #1072

Special notes for your reviewer:
The feature is available since Kubernetes 1.25. Is there any version compatibility concern there? Could I remove the existing validating webhook in the same PR? Thank you!

Does this PR introduce a user-facing change?:

Adds validation rules into CRDs. Minimum required Kubernetes version is 1.25 for these validation rules.

k8s-ci-robot · 2024-04-24T21:14:18Z

Welcome @cici37!

It looks like this is your first PR to kubernetes-csi/external-snapshotter 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-snapshotter has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-24T21:14:18Z

Hi @cici37. 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/test-infra repository.

cici37 · 2024-04-24T22:00:54Z

/cc @msau42
This is the changes I mentioned earlier. Would you mind help with adding ok-to-test label?
A quick question: is that ok to remove the webhook directly from this repo? The feature is general available since 1.25, is there any concern with compatibility or others? Or if we would love to have both running for one release? Thank you!

carlory · 2024-04-25T04:27:21Z

When I run client/hack/update-crd.sh on the master branch, I find the issue that the content in client/config/crds is overwritten and inconsistent with the script. Compared to the current state, the re-generated content is missing some fields, and some fields' definitions are changed. i.e. additionalPrinterColumns is wiped out. restoreSize type is changed from string to string or integer.

We should fix the issue first and then do other steps. Modifying the generated manifest files is not a good practice.

/hold

/cc @xing-yang @jsafrane

xing-yang · 2024-04-25T15:17:50Z

We made manual changes and documented what need to be changed in the README after running the scripts. It's possible that some of those changes are no longer needed. I'll take a look.

/assign

xing-yang · 2024-04-25T15:20:47Z

@cici37 Can these validation rules be generated automatically by adding some marker in the API definitions? https://github.com/kubernetes-csi/external-snapshotter/blob/master/client/apis/volumesnapshot/v1/types.go

If not, we'll have to document these as manual steps to modify the manifest after running update-crd.sh: https://github.com/kubernetes-csi/external-snapshotter/tree/master/client/hack#update-crdsh

xing-yang · 2024-04-26T15:09:30Z

/ok-to-test

cici37 · 2024-04-29T03:48:53Z

@cici37 Can these validation rules be generated automatically by adding some marker in the API definitions? https://github.com/kubernetes-csi/external-snapshotter/blob/master/client/apis/volumesnapshot/v1/types.go

If not, we'll have to document these as manual steps to modify the manifest after running update-crd.sh: https://github.com/kubernetes-csi/external-snapshotter/tree/master/client/hack#update-crdsh

Thanks for the review! I have added the validation through kubebuilder marker. Do we still add the same rules under CRDs or it will be auto generated through kubebuilder? Thanks

xing-yang · 2024-05-01T02:01:06Z

@cici37 I updated the manifests based on your PR: #1076

cici37 · 2024-05-01T16:32:13Z

@xing-yang Thank you for updating the manifests! Is there any other changes needed in order to merge this PR?

client/apis/volumesnapshot/v1/types.go

leonardoce · 2024-05-04T19:59:27Z

I collected a set of test cases here: https://github.com/leonardoce/volumesnapshot-cel/
For now, I only tested VolumeSnapshot and VolumeSnapshotContent.
Should it be helpful, we can move them where they fit best, or rewrite them, or throw them away altogether.

client/apis/volumegroupsnapshot/v1alpha1/types.go

cici37 · 2024-05-06T18:28:44Z

@xing-yang @leonardoce Thanks for the review! I have addressed all the comments above. Please take a look when you have time. I could have a followup PR to remove the webhook after this one merged or please feel free to let me know if someone else is gonna do so :)

xing-yang · 2024-05-06T18:40:46Z

/hold cancel

xing-yang · 2024-05-06T18:43:22Z

@xing-yang @leonardoce Thanks for the review! I have addressed all the comments above. Please take a look when you have time. I could have a followup PR to remove the webhook after this one merged or please feel free to let me know if someone else is gonna do so :)

Thanks @cici37 for addressing the comments! I'll update my PR (#1076) which updates the manifests based on this PR once this one is merged. After that I'll remove the webhook.

xing-yang · 2024-05-06T18:43:43Z

/lgtm
/approve

k8s-ci-robot · 2024-05-06T18:44:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, 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~~ [xing-yang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This patch removes the logic to admit VolumeSnapshots, VolumeSnapshotContents, VolumeGroupSnapshots and VolumeGroupSnapshotContents in the validation webhook. This logic is already implemented via CEL expressions (see kubernetes-csi#1073) The logic to admit VolumeSnapshotClasses and VolumeGroupSnapshotClasses is still implemented in the webhook and avoids having multiple classes for the same CSI Driver.

This patch removes the logic to admit VolumeSnapshots, VolumeSnapshotContents, VolumeGroupSnapshots and VolumeGroupSnapshotContents in the validation webhook. That logic is already implemented via CEL expressions (see kubernetes-csi#1073) The logic to admit VolumeSnapshotClasses and VolumeGroupSnapshotClasses is still implemented in the webhook and avoids having multiple classes for the same CSI Driver.

This patch removes the logic to admit VolumeSnapshots, VolumeSnapshotContents, VolumeGroupSnapshots and VolumeGroupSnapshotContents in the validation webhook. That logic is already implemented via CEL expressions (see kubernetes-csi#1073) The logic to admit VolumeSnapshotClasses and VolumeGroupSnapshotClasses is still implemented in the webhook and avoids having multiple default classes for the same CSI Driver.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 24, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 24, 2024

k8s-ci-robot requested review from carlory and ggriffiths April 24, 2024 21:14

cici37 force-pushed the master branch from 62af87b to 70a9cc3 Compare April 24, 2024 21:59

k8s-ci-robot requested a review from msau42 April 24, 2024 22:00

k8s-ci-robot requested review from jsafrane and xing-yang April 25, 2024 04:27

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2024

k8s-ci-robot assigned xing-yang Apr 25, 2024

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 Apr 26, 2024

Adding validation rules into types

4d6c52e

cici37 force-pushed the master branch from 70a9cc3 to 4d6c52e Compare April 29, 2024 03:47

xing-yang mentioned this pull request May 1, 2024

Update manifests based on new CRD validation rules #1076

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 1, 2024

leonardoce reviewed May 4, 2024

View reviewed changes

client/apis/volumesnapshot/v1/types.go Outdated Show resolved Hide resolved

leonardoce reviewed May 4, 2024

View reviewed changes

client/apis/volumesnapshot/v1/types.go Show resolved Hide resolved