add groupsnapshot related webhooks #825

Rakshith-R · 2023-03-29T06:56:12Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Adds support for webhooks for groupsnapshot CRs.

KEP - https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/3476-volume-group-snapshot

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Webhooks for VolumeGroupSnapshot, VolumeGroupSnapshotContent and VolumeGroupSnapshotClass.

/cc @xing-yang @nixpanic @RaunakShah

linux-foundation-easycla · 2023-03-29T06:56:16Z

The committers listed above are authorized under a signed CLA.

✅ login: Rakshith-R / name: Rakshith R (7f39877)

k8s-ci-robot · 2023-03-29T06:56:21Z

Welcome @Rakshith-R!

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 · 2023-03-29T06:56:22Z

Hi @Rakshith-R. 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.

RaunakShah · 2023-03-29T07:10:28Z

/ok-to-test

RaunakShah · 2023-03-29T07:12:10Z

deploy/kubernetes/webhook-example/admission-configuration-template

+      name: "snapshot-validation-service"
+      path: "/volumegroupsnapshot"
+    caBundle: ${CA_BUNDLE}
+  admissionReviewVersions: ["v1"]


should this be v1alpha1?

It is the AdmissionReview versions, not apiversions of CR.
We're using v1 version in webhook server.
I've tested it and it works fine.
admissionReviewVersions (string[]) AdmissionReviewVersions is an ordered list of preferred AdmissionReview versions the Webhook expects. API server will try to use first version in the list which it supports. If none of the versions specified in this list supported by API server, validation will fail for this object. If a persisted webhook configuration specifies allowed versions and does not include any versions known to the API Server, calls to the webhook will fail and be subject to the failure policy.

RaunakShah · 2023-03-29T07:13:33Z

pkg/validation-webhook/groupsnapshot.go

+	Admit(v1.AdmissionReview) *v1.AdmissionResponse
+}
+
+func NewGroupSnapshotAdmitter(lister groupSnapshotListers.VolumeGroupSnapshotClassLister) GroupSnapshotAdmitter {


Is this VolumeGroupSnapshotClassLister or VolumeGroupSnapshotLister

its VolumeGroupSnapshotClassLister

RaunakShah · 2023-03-29T07:15:26Z

pkg/validation-webhook/groupsnapshot.go

+		Result:  &metav1.Status{},
+	}
+
+	// Only Validate when a new snapClass is being set as a default.


nit: Only validate when a new group snapshot class is being set as a default.

RaunakShah · 2023-03-29T10:42:14Z

pkg/validation-webhook/snapshot.go

 	return &admitter{
-		lister: lister,
+		snapshotLister: lister,


does this also need to initialise the group snapshot lister?

does this also need to initialise the group snapshot lister?

refactored a bit, This struct is not shared with group snapshot now.

RaunakShah · 2023-03-29T10:44:18Z

pkg/validation-webhook/snapshot.go

@@ -107,9 +110,47 @@ func (a admitter) Admit(ar v1.AdmissionReview) *v1.AdmissionResponse {
 			klog.Error(err)
 			return toV1AdmissionResponse(err)
 		}
-		return decideSnapshotClassV1(snapClass, oldSnapClass, a.lister)
+		return decideSnapshotClassV1(snapClass, oldSnapClass, a.snapshotLister)
+	case GroupSnapshotV1Alpha1GVR:


do we need these? there's only v1alpha1 right now

do we need these? there's only v1alpha1 right now

I think its good to have it now, so we don't have to change it all over the place when
we have newer versions.

it is consistent with naming of snapshot v1 resources currently present.

RaunakShah · 2023-03-29T10:44:58Z

pkg/validation-webhook/snapshot.go

 	default:
-		err := fmt.Errorf("expect resource to be %s, %s or %s", SnapshotV1GVR, SnapshotContentV1GVR, SnapshotClassV1GVR)
+		err := fmt.Errorf("expect resource to be %s, %s, %s, %s, %s, or %s, but found %v",
+			SnapshotV1GVR, SnapshotContentV1GVR, SnapshotClassV1GVR, &GroupSnapshotV1Alpha1GVR,


Need to remove the & here

Rakshith-R · 2023-03-30T06:12:14Z

🤔 re-requesting for reviews seems to have removed review-request of other reviewers.
restoring it.

/cc @nixpanic @xing-yang

xing-yang · 2023-05-03T23:44:31Z

pkg/validation-webhook/groupsnapshot.go

+	GroupSnapshotV1Alpha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshots"}
+	// GroupSnapshotContentV1Apha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshotContents
+	GroupSnapshotContentV1Apha1GVR = metav1.GroupVersionResource{Group: volumegroupsnapshotv1alpha1.GroupName, Version: "v1alpha1", Resource: "volumegroupsnapshotcontents"}
+	// GroupSnapshotContentV1Apha1GVR is GroupVersionResource for v1alpha1 VolumeGroupSnapshotContents


GroupSnapshotContentV1Apha1GVR -> GroupSnapshotClassV1Apha1GVR
VolumeGroupSnapshotContents > VolumeGroupSnapshotClasses

xing-yang · 2023-05-04T00:00:04Z

pkg/validation-webhook/groupsnapshot.go

+	}
+
+	// Only Validate when a new group snapshot class is being set as a default.
+	if snapClass.Annotations[utils.IsDefaultSnapshotClassAnnotation] != "true" {


We should add a utils.IsDefaultGroupSnapshotClassAnnotation.
Add the following definition in utils.go:

IsDefaultGroupSnapshotClassAnnotation = "groupsnapshot.storage.kubernetes.io/is-default-class"

xing-yang · 2023-05-04T00:00:51Z

pkg/validation-webhook/groupsnapshot.go

+	return reviewResponse
+}
+
+func decideGroupSnapshotClassV1Alpha1(snapClass, oldSnapClass *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotClass, lister groupSnapshotListers.VolumeGroupSnapshotClassLister) *v1.AdmissionResponse {


snapClass -> groupSnapClass
oldSnapClass -> oldGroupSnapClass

xing-yang · 2023-05-04T00:02:16Z

pkg/validation-webhook/groupsnapshot.go

+		return reviewResponse
+	}
+
+	for _, snapshotClass := range ret {


snapshotClass -> groupSnapshotClass

xing-yang · 2023-05-04T00:02:32Z

pkg/validation-webhook/groupsnapshot.go

+	}
+
+	for _, snapshotClass := range ret {
+		if snapshotClass.Annotations[utils.IsDefaultSnapshotClassAnnotation] != "true" {


utils.IsDefaultGroupSnapshotClassAnnotation

xing-yang · 2023-05-04T00:03:16Z

pkg/validation-webhook/groupsnapshot.go

+		if snapshotClass.Annotations[utils.IsDefaultSnapshotClassAnnotation] != "true" {
+			continue
+		}
+		if snapshotClass.Driver == snapClass.Driver {


snapshotClass -> groupSnapshotClass
snapClass -> groupSnapClass

xing-yang · 2023-05-04T00:07:21Z

Can you add a release note?

RaunakShah · 2023-05-04T09:03:29Z

pkg/validation-webhook/groupsnapshot.go

+)
+
+var (
+	// GroupSnapshotV1Alpha1GVR is GroupVersionResource for v1alpha1 VolumeSnapshots


VolumeSnapshots -> VolumeGroupSnapshots

RaunakShah · 2023-05-04T09:05:12Z

pkg/validation-webhook/groupsnapshot.go

+	}
+
+	if isUpdate {
+		// if it is an UPDATE and oldSnapshot is valid, check immutable fields


oldSnapshot -> oldGroupSnapshot

RaunakShah · 2023-05-04T09:05:54Z

pkg/validation-webhook/groupsnapshot.go

+	return reviewResponse
+}
+
+func decideGroupSnapshotContentV1Alpha1(groupSnapcontent, oldSnapcontent *volumegroupsnapshotv1alpha1.VolumeGroupSnapshotContent, isUpdate bool) *v1.AdmissionResponse {


oldSnapcontent -> oldGroupSnapcontent

RaunakShah · 2023-05-04T09:06:04Z

pkg/validation-webhook/groupsnapshot.go

+	}
+
+	if isUpdate {
+		// if it is an UPDATE and oldSnapcontent is valid, check immutable fields


oldSnapcontent -> oldGroupSnapcontent

RaunakShah · 2023-05-04T09:06:36Z

pkg/validation-webhook/groupsnapshot.go

+		return reviewResponse
+	}
+
+	// If Old group snapshot class has this, then we can assume that it was validated if driver is the same.


nit: If the old ...

RaunakShah · 2023-05-04T09:17:30Z

pkg/validation-webhook/snapshot_test.go

+			}},
+		},
+		{
+			name: "new snapshot for class with existing default classes",


new snapshot for class with existing default classes -> new group snapshot for group snapshot class with existing default classes

RaunakShah · 2023-05-04T09:17:49Z

pkg/validation-webhook/snapshot_test.go

+			}},
+		},
+		{
+			name: "update snapshot class to new driver with existing default classes",


update snapshot class to new driver with existing default classes -> update group snapshot class to new driver with existing default group snapshot classes

RaunakShah · 2023-05-04T09:20:05Z

pkg/validation-webhook/snapshot_test.go

@@ -644,3 +663,614 @@ func TestAdmitVolumeSnapshotClassV1(t *testing.T) {
 		})
 	}
 }
+
+func TestAdmitVolumeGroupSnapshotV1Alpha1(t *testing.T) {


Can these tests be moved to pkg/validation-webhook/groupsnapshot_test.go

RaunakShah · 2023-05-04T09:21:01Z

pkg/validation-webhook/webhook.go

@@ -26,7 +26,8 @@ import (
 	"os"

 	clientset "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned"
-	storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumesnapshot/v1"
+	groupSnapshotListers "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumegroupsnapshot/v1alpha1"


nit: groupsnapshotlisters

RaunakShah · 2023-05-04T09:21:09Z

pkg/validation-webhook/webhook.go

@@ -26,7 +26,8 @@ import (
 	"os"

 	clientset "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned"
-	storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumesnapshot/v1"
+	groupSnapshotListers "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumegroupsnapshot/v1alpha1"
+	snapshotListers "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumesnapshot/v1"


snapshotlisters

xing-yang · 2023-05-08T14:15:37Z

/lgtm
/approve

k8s-ci-robot · 2023-05-08T14:15:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rakshith-R, 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

xing-yang · 2023-10-12T17:53:07Z

@Rakshith-R Can you take a look and see if you can make the groupsnapshot related webhook optional?
#921

k8s-ci-robot requested review from nixpanic and RaunakShah March 29, 2023 06:56

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 29, 2023

k8s-ci-robot requested a review from xing-yang March 29, 2023 06:56

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 29, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 29, 2023

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 29, 2023

Rakshith-R force-pushed the add-group-snapshot-webhooks branch from 9c8d94c to 7f39877 Compare March 29, 2023 07:07

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 29, 2023

RaunakShah reviewed Mar 29, 2023

View reviewed changes

Rakshith-R force-pushed the add-group-snapshot-webhooks branch from 7f39877 to 2f27cb0 Compare March 30, 2023 05:54

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 30, 2023

Rakshith-R force-pushed the add-group-snapshot-webhooks branch from 2f27cb0 to f6ee988 Compare March 30, 2023 05:58

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 30, 2023

Rakshith-R requested review from RaunakShah and removed request for nixpanic and xing-yang March 30, 2023 06:02

k8s-ci-robot requested review from nixpanic and xing-yang March 30, 2023 06:12

xing-yang reviewed May 4, 2023

View reviewed changes

RaunakShah reviewed May 4, 2023

View reviewed changes

Rakshith-R force-pushed the add-group-snapshot-webhooks branch from f6ee988 to 43728d2 Compare May 8, 2023 10:17

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 8, 2023

Rakshith-R requested review from RaunakShah and xing-yang May 8, 2023 10:18

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 8, 2023

add groupsnapshot related webhooks

a3cb5a9

Rakshith-R force-pushed the add-group-snapshot-webhooks branch from 43728d2 to a3cb5a9 Compare May 8, 2023 12:55

k8s-ci-robot assigned xing-yang May 8, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2023

k8s-ci-robot merged commit a942b5b into kubernetes-csi:master May 8, 2023

mpatlasov mentioned this pull request Feb 5, 2024

STOR-1700: Rebase external-snapshotter to v7.0.0 to get VolumeGroupSnapshot openshift/csi-external-snapshotter#139

Merged

add groupsnapshot related webhooks #825

add groupsnapshot related webhooks #825

Conversation

Rakshith-R commented Mar 29, 2023 • edited Loading

linux-foundation-easycla bot commented Mar 29, 2023 • edited Loading

k8s-ci-robot commented Mar 29, 2023

k8s-ci-robot commented Mar 29, 2023

RaunakShah commented Mar 29, 2023

Choose a reason for hiding this comment

Rakshith-R Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented May 8, 2023

k8s-ci-robot commented May 8, 2023

xing-yang commented Oct 12, 2023

Rakshith-R commented Mar 29, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 29, 2023 •

edited

Loading

Rakshith-R Mar 30, 2023 •

edited

Loading