Adding validation rules into CRDs for validation

[cici37]

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]

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. :smiley:

[k8s-ci-robot]

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](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/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[cici37]

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

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]

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]

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

/ok-to-test

[cici37]

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

@cici37 I updated the manifests based on your PR: https://github.com/kubernetes-csi/external-snapshotter/pull/1076

[cici37]

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

[xing-yang]

@cici37 I need to do some testing. After that we can get this PR merged. I think webhook logic can be removed after that in a followup PR.

[xing-yang]

I thought one of the newly added rules will allow only one of persistentVolumeClaimName and volumeSnapshotContentName to be specified in VolumeSnapshot source but not both, but it didn't happen. I'm running K8s 1.30.

---
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  name: invalid-snapshot-demo-v1
spec:
  volumeSnapshotClassName: csi-hostpath-snapclass-v1
  source: # Only one of the two fields should be set for a snapshot. Therefore this snapshot is invalid.
    persistentVolumeClaimName: pvc
    volumeSnapshotContentName: vsc

kubectl create -f invalid-snapshot-v1.yaml
volumesnapshot.snapshot.storage.k8s.io/invalid-snapshot-demo-v1 created

kubectl get volumesnapshot invalid-snapshot-demo-v1
NAME                       READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT   RESTORESIZE   SNAPSHOTCLASS               SNAPSHOTCONTENT   CREATIONTIME   AGE
invalid-snapshot-demo-v1   false        pvc         vsc                                   csi-hostpath-snapclass-v1                                    4m55s

@cici37, maybe I misunderstood the following rule:

                x-kubernetes-validations:
                - message: persistentVolumeClaimName is required once set
                  rule: '!has(oldSelf.persistentVolumeClaimName) || has(self.persistentVolumeClaimName)'
                - message: volumeSnapshotContentName is required once set
                  rule: '!has(oldSelf.volumeSnapshotContentName) || has(self.volumeSnapshotContentName)'

Does this rule only makes sure that persistentVolumeClaimName and volumeSnapshotContentName are immutable once set, but it does not make sure that only one of them can be set? If so, we'll need to keep the "oneOf" rule which was added manually unless if there's a way to add this oneOf rule using CEL?

                oneOf:
                - required: ["persistentVolumeClaimName"]
                - required: ["volumeSnapshotContentName"]

[cici37]

Thanks for the review!

Does this rule only makes sure that persistentVolumeClaimName and volumeSnapshotContentName are immutable once set, but it does not make sure that only one of them can be set? If so, we'll need to keep the "oneOf" rule which was added manually unless if there's a way to add this oneOf rule using CEL?

                oneOf:
                - required: ["persistentVolumeClaimName"]
                - required: ["volumeSnapshotContentName"]

Yes the current rule only enforce the immutability of the field. I didn't see the oneOf rule in the webhook. Maybe I missed it-_-. Will update the PR to address this use case :)

[xing-yang]

@cici37 Those Oneof are not in the webhook. We manually added them to the manifests. There are 3 of them. See below: (1) https://github.com/kubernetes-csi/external-snapshotter/blob/v7.0.2/client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yaml#L140

                type: object
                oneOf:
                - required: ["volumeHandles"]
                - required: ["groupSnapshotHandles"]

(2) https://github.com/kubernetes-csi/external-snapshotter/blob/v7.0.2/client/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml#L120

                type: object
                oneOf:
                - required: ["snapshotHandle"]
                - required: ["volumeHandle"]

(3) https://github.com/kubernetes-csi/external-snapshotter/blob/v7.0.2/client/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml#L104

                type: object
                oneOf:
                - required: ["persistentVolumeClaimName"]
                - required: ["volumeSnapshotContentName"]

[leonardoce]

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.

[cici37]

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

/hold cancel

[xing-yang]

@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 (https://github.com/kubernetes-csi/external-snapshotter/pull/1076) which updates the manifests based on this PR once this one is merged. After that I'll remove the webhook.

[xing-yang]

/lgtm /approve

[k8s-ci-robot]

[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](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