Repository: kubernetes-csi/external-snapshotter
Pull request #995 (2024-01-26 09:01:03)

Selector in VolumeGroupSnapshotSource API should be optional
**What type of PR is this?**

/kind api-change

**What this PR does / why we need it**:

When using a VolumeGroupSnapshotContent for a pre-preprovisioned VolumeGroupSnapshotSource, there is no correct value for the `selector` property. However, the original API requires the `selector` to be set, even though it is not used.

**Which issue(s) this PR fixes**:

This problem was identified in #837.

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:

```release-note
The VolumeGroupSnapshotSource.Selector is now an optional attribute, so that a pre-provisioned VolumeGroupSnapshotContent can be specified which does not require a matching label-selector.
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-snapshotter/pull/995#" title="Author self-approved">nixpanic</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign [msau42](https://github.com/msau42) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-snapshotter).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-snapshotter/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["msau42"]} -->
nixpanic: /cc xing-yang RaunakShah

Please have a look, combined with #837 the VolumeGroupSnapshot feature should be pretty much ready for users to try out.
k8s-ci-robot: @nixpanic: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-kubernetes-csi-external-snapshotter-unit | 1d6126caef921d11b95c518b88b18578c8b789ea | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/995/pull-kubernetes-csi-external-snapshotter-unit/1750806362390007808) | true | `/test pull-kubernetes-csi-external-snapshotter-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-snapshotter&pr=995). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Anixpanic). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-snapshotter/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @nixpanic: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-kubernetes-csi-external-snapshotter-unit | 153a0e701349319e6ae15066a06abe2eeff44112 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/995/pull-kubernetes-csi-external-snapshotter-unit/1750907210461876224) | true | `/test pull-kubernetes-csi-external-snapshotter-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-snapshotter&pr=995). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Anixpanic). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-snapshotter/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/client/apis/volumegroupsnapshot/v1alpha1/types.go b/client/apis/volumegroupsnapshot/v1alpha1/types.go
index 7e27b793a..531867e6b 100644
--- a/client/apis/volumegroupsnapshot/v1alpha1/types.go
+++ b/client/apis/volumegroupsnapshot/v1alpha1/types.go
@@ -52,8 +52,8 @@ type VolumeGroupSnapshotSource struct {
 	// is created, the existing group snapshots won't be modified.
 	// Once a VolumeGroupSnapshotContent is created and the sidecar starts to process
 	// it, the volume list will not change with retries.
-	// Required.
-	Selector metav1.LabelSelector `json:"selector" protobuf:"bytes,1,opt,name=selector"`
+	// +optional
+	Selector *metav1.LabelSelector `json:"selector,omitempty" protobuf:"bytes,1,opt,name=selector"`
 
 	// VolumeGroupSnapshotContentName specifies the name of a pre-existing VolumeGroupSnapshotContent
 	// object representing an existing volume group snapshot.
diff --git a/client/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go b/client/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go
index 5dc16cda3..43a55b164 100644
--- a/client/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go
+++ b/client/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go
@@ -24,6 +24,7 @@ package v1alpha1
 import (
 	v1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
@@ -337,7 +338,11 @@ func (in *VolumeGroupSnapshotList) DeepCopyObject() runtime.Object {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *VolumeGroupSnapshotSource) DeepCopyInto(out *VolumeGroupSnapshotSource) {
 	*out = *in
-	in.Selector.DeepCopyInto(&out.Selector)
+	if in.Selector != nil {
+		in, out := &in.Selector, &out.Selector
+		*out = new(metav1.LabelSelector)
+		(*in).DeepCopyInto(*out)
+	}
 	if in.VolumeGroupSnapshotContentName != nil {
 		in, out := &in.VolumeGroupSnapshotContentName, &out.VolumeGroupSnapshotContentName
 		*out = new(string)
diff --git a/client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yaml b/client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yaml
index facfe7ff5..b8bf6366e 100644
--- a/client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yaml
+++ b/client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yaml
@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    api-approved.kubernetes.io: "https://github.com/kubernetes-csi/external-snapshotter/pull/814"
+    api-approved.kubernetes.io: "https://github.com/kubernetes-csi/external-snapshotter/pull/995"
     controller-gen.kubebuilder.io/version: v0.12.0
   creationTimestamp: null
   name: volumegroupsnapshots.groupsnapshot.storage.k8s.io
@@ -78,7 +78,6 @@ spec:
                       is created, the existing group snapshots won't be modified.
                       Once a VolumeGroupSnapshotContent is created and the sidecar
                       starts to process it, the volume list will not change with retries.
-                      Required.
                     properties:
                       matchExpressions:
                         description: matchExpressions is a list of label selector
@@ -129,8 +128,6 @@ spec:
                       if the volume group snapshot already exists and only needs a
                       representation in Kubernetes. This field is immutable.
                     type: string
-                required:
-                - selector
                 type: object
               volumeGroupSnapshotClassName:
                 description: VolumeGroupSnapshotClassName is the name of the VolumeGroupSnapshotClass
diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go
index 26cc44d63..1fe7f870c 100644
--- a/pkg/common-controller/groupsnapshot_controller_helper.go
+++ b/pkg/common-controller/groupsnapshot_controller_helper.go
@@ -295,8 +295,8 @@ func (ctrl *csiSnapshotCommonController) syncGroupSnapshot(groupSnapshot *crdv1a
 	}
 	// Keep this check in the controller since the validation webhook may not have been deployed.
 	klog.V(5).Infof("syncGroupSnapshot[%s]: validate group snapshot to make sure source has been correctly specified", utils.GroupSnapshotKey(groupSnapshot))
-	if (&groupSnapshot.Spec.Source.Selector == nil && groupSnapshot.Spec.Source.VolumeGroupSnapshotContentName == nil) ||
-		(&groupSnapshot.Spec.Source.Selector != nil && groupSnapshot.Spec.Source.VolumeGroupSnapshotContentName != nil) {
+	if (groupSnapshot.Spec.Source.Selector == nil && groupSnapshot.Spec.Source.VolumeGroupSnapshotContentName == nil) ||
+		(groupSnapshot.Spec.Source.Selector != nil && groupSnapshot.Spec.Source.VolumeGroupSnapshotContentName != nil) {
 		err := fmt.Errorf("Exactly one of Selector and VolumeGroupSnapshotContentName should be specified")
 		klog.Errorf("syncGroupSnapshot[%s]: validation error, %s", utils.GroupSnapshotKey(groupSnapshot), err.Error())
 		ctrl.updateGroupSnapshotErrorStatusWithEvent(groupSnapshot, true, v1.EventTypeWarning, "GroupSnapshotValidationError", err.Error())
diff --git a/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/types.go b/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/types.go
index 7e27b793a..531867e6b 100644
--- a/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/types.go
+++ b/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/types.go
@@ -52,8 +52,8 @@ type VolumeGroupSnapshotSource struct {
 	// is created, the existing group snapshots won't be modified.
 	// Once a VolumeGroupSnapshotContent is created and the sidecar starts to process
 	// it, the volume list will not change with retries.
-	// Required.
-	Selector metav1.LabelSelector `json:"selector" protobuf:"bytes,1,opt,name=selector"`
+	// +optional
+	Selector *metav1.LabelSelector `json:"selector,omitempty" protobuf:"bytes,1,opt,name=selector"`
 
 	// VolumeGroupSnapshotContentName specifies the name of a pre-existing VolumeGroupSnapshotContent
 	// object representing an existing volume group snapshot.
diff --git a/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go b/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go
index 5dc16cda3..43a55b164 100644
--- a/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go
+++ b/vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumegroupsnapshot/v1alpha1/zz_generated.deepcopy.go
@@ -24,6 +24,7 @@ package v1alpha1
 import (
 	v1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
@@ -337,7 +338,11 @@ func (in *VolumeGroupSnapshotList) DeepCopyObject() runtime.Object {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *VolumeGroupSnapshotSource) DeepCopyInto(out *VolumeGroupSnapshotSource) {
 	*out = *in
-	in.Selector.DeepCopyInto(&out.Selector)
+	if in.Selector != nil {
+		in, out := &in.Selector, &out.Selector
+		*out = new(metav1.LabelSelector)
+		(*in).DeepCopyInto(*out)
+	}
 	if in.VolumeGroupSnapshotContentName != nil {
 		in, out := &in.VolumeGroupSnapshotContentName, &out.VolumeGroupSnapshotContentName
 		*out = new(string)


Test patch:
diff --git a/pkg/validation-webhook/groupsnapshot_test.go b/pkg/validation-webhook/groupsnapshot_test.go
index e1f00fe3a..c555c0797 100644
--- a/pkg/validation-webhook/groupsnapshot_test.go
+++ b/pkg/validation-webhook/groupsnapshot_test.go
@@ -90,7 +90,7 @@ func TestAdmitVolumeGroupSnapshotV1Alpha1(t *testing.T) {
 			volumeGroupSnapshot: &volumegroupsnapshotv1alpha1.VolumeGroupSnapshot{
 				Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{
 					Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{
-						Selector: selector,
+						Selector: &selector,
 					},
 				},
 			},
@@ -172,13 +172,13 @@ func TestAdmitVolumeGroupSnapshotV1Alpha1(t *testing.T) {
 				Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{
 					Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{
 						VolumeGroupSnapshotContentName: &contentname,
-						Selector:                       selector,
+						Selector:                       &selector,
 					},
 				},
 			},
 			shouldAdmit: false,
 			operation:   v1.Update,
-			msg:         fmt.Sprintf("Spec.Source.Selector is immutable but was changed from %v to %v", selector, metav1.LabelSelector{}),
+			msg:         fmt.Sprintf("Spec.Source.Selector is immutable but was changed from %v to %v", &selector, "nil"),
 		},
 		{
 			// will be handled by schema validation
@@ -187,7 +187,7 @@ func TestAdmitVolumeGroupSnapshotV1Alpha1(t *testing.T) {
 				Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{
 					Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{
 						VolumeGroupSnapshotContentName: &contentname,
-						Selector:                       selector,
+						Selector:                       &selector,
 					},
 				},
 			},
@@ -195,7 +195,7 @@ func TestAdmitVolumeGroupSnapshotV1Alpha1(t *testing.T) {
 				Spec: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSpec{
 					Source: volumegroupsnapshotv1alpha1.VolumeGroupSnapshotSource{
 						VolumeGroupSnapshotContentName: &contentname,
-						Selector:                       selector,
+						Selector:                       &selector,
 					},
 				},
 			},