Repository: kubernetes-csi/external-snapshotter (go)

Issue:
Controller does not detect new Volume[Group]SnapshotClass set as default
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!
-->


**What happened**:

If multiple default `Volume[Group]SnapshotClasses` are created before the external-snapshotter controller Pod starts, the controller will detect them correctly and when webhook is not installed dynamically provisioned VolumeSnapshot and VolumeGroupSnapshots will fail with:

```
apiVersion: groupsnapshot.storage.k8s.io/v1alpha1
kind: VolumeGroupSnapshot
metadata:
  name: new-groupsnapshot-demo
spec:
  source:
    selector:
      matchLabels:
        cnpg.io/instanceName: cluster-example-1
status:
  error:
    message: Failed to set default group snapshot class with error 2 default snapshot
      classes were found
    time: "2024-05-28T08:24:42Z"
```

This doesn't happen if the external-snapshotter controller starts with a correct configuration (having just one default class per CSI driver) and a new one is added later.
In that case, the Volume[Group]Snapshot will be provisioned by the CSI driver specified in the Volume[Group]SnapshotClass that was present before the controller started.

**What you expected to happen**:

I expect the behavior to be the same, regardless of when the Volume[Group]SnapshotClass was created.

**How to reproduce it**:

On an empty cluster with just the hostpath provisioner installed, create one default VolumeSnapshotClass.
Install the external-snapshotter controller and, when it's ready, create a new VolumeSnapshotClass.

You'll see the VolumeSnapshot objects being created from the first VolumeSnapshotClass you created and set as default.


Pull request description:
Correctly detect newly created Volume[Group]SnapshotClasses
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**

/kind bug

> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
> /kind flake

**What this PR does / why we need it**:

This PR makes the controller process newly created default VolumeSnapshotClasses and VolumeGroupSnapshotClasses, correctly using them or raising an error when appropriate.

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->

Fixes #1099 

**Special notes for your reviewer**:

The controller is using an informer to get VolumeGroupSnapshotClasses and VolumeSnapshotClasses.
Unfortunately, objects returned by that will not have the API Kind info set (see https://github.com/kubernetes/client-go/issues/308).

This prevented the `IsDefaultAnnotation` function from working correctly.

This patch avoids relying on the object Kind to know whether a VolumeGroupSnapshotClass or a VolumeSnapshotClass is set as default.

The semantics of the `groupsnapshot.storage.kubernetes.io/is-default-class` have been preserved, with `true` being true and everything else being `false` (this is slightly different from Kubernetes).

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
The controller will now detect new default VolumeSnapshotClasses and VolumeGroupSnapshotClasses.
If multiple classes exist for the same CSI driver, VolumeSnapshot and VolumeGroupSnapshots
will be marked as failed when provisioned dynamically.
```


Patch:
diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go
index c539d5480..4357d769e 100644
--- a/pkg/common-controller/groupsnapshot_controller_helper.go
+++ b/pkg/common-controller/groupsnapshot_controller_helper.go
@@ -141,7 +141,7 @@ func (ctrl *csiSnapshotCommonController) SetDefaultGroupSnapshotClass(groupSnaps
 
 	defaultClasses := []*crdv1alpha1.VolumeGroupSnapshotClass{}
 	for _, groupSnapshotClass := range list {
-		if utils.IsDefaultAnnotation(groupSnapshotClass.TypeMeta, groupSnapshotClass.ObjectMeta) && pvDriver == groupSnapshotClass.Driver {
+		if utils.IsVolumeGroupSnapshotClassDefaultAnnotation(groupSnapshotClass.ObjectMeta) && pvDriver == groupSnapshotClass.Driver {
 			defaultClasses = append(defaultClasses, groupSnapshotClass)
 			klog.V(5).Infof("get defaultGroupClass added: %s, driver: %s", groupSnapshotClass.Name, pvDriver)
 		}
diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
index 0662237a6..595986c3a 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -1400,7 +1400,7 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
 
 	defaultClasses := []*crdv1.VolumeSnapshotClass{}
 	for _, class := range list {
-		if utils.IsDefaultAnnotation(class.TypeMeta, class.ObjectMeta) && pvDriver == class.Driver {
+		if utils.IsVolumeSnapshotClassDefaultAnnotation(class.ObjectMeta) && pvDriver == class.Driver {
 			defaultClasses = append(defaultClasses, class)
 			klog.V(5).Infof("get defaultClass added: %s, driver: %s", class.Name, pvDriver)
 		}
diff --git a/pkg/utils/util.go b/pkg/utils/util.go
index 0058795a9..1c7c7c949 100644
--- a/pkg/utils/util.go
+++ b/pkg/utils/util.go
@@ -274,21 +274,16 @@ func GetDynamicSnapshotContentNameForSnapshot(snapshot *crdv1.VolumeSnapshot) st
 	return "snapcontent-" + string(snapshot.UID)
 }
 
-// IsDefaultAnnotation returns a boolean if
-// the annotation is set
-func IsDefaultAnnotation(tm metav1.TypeMeta, obj metav1.ObjectMeta) bool {
-	switch tm.Kind {
-	case "VolumeSnapshotClass":
-		if obj.Annotations[IsDefaultSnapshotClassAnnotation] == "true" {
-			return true
-		}
-	case "VolumeGroupSnapshotClass":
-		if obj.Annotations[IsDefaultGroupSnapshotClassAnnotation] == "true" {
-			return true
-		}
-	}
+// IsVolumeSnapshotClassDefaultAnnotation returns a true boolean if
+// a VolumeSnapshotClass is marked as the default one
+func IsVolumeSnapshotClassDefaultAnnotation(obj metav1.ObjectMeta) bool {
+	return obj.Annotations[IsDefaultSnapshotClassAnnotation] == "true"
+}
 
-	return false
+// IsVolumeGroupSnapshotClassDefaultAnnotation returns a true boolean if
+// a VolumeGroupSnapshotClass is marked as the default one
+func IsVolumeGroupSnapshotClassDefaultAnnotation(obj metav1.ObjectMeta) bool {
+	return obj.Annotations[IsDefaultGroupSnapshotClassAnnotation] == "true"
 }
 
 // verifyAndGetSecretNameAndNamespaceTemplate gets the values (templates) associated


Test patch:
diff --git a/pkg/utils/util_test.go b/pkg/utils/util_test.go
index b8e0be2f1..62b1b7fc7 100644
--- a/pkg/utils/util_test.go
+++ b/pkg/utils/util_test.go
@@ -201,28 +201,21 @@ func TestRemovePrefixedCSIParams(t *testing.T) {
 	}
 }
 
-func TestIsDefaultAnnotation(t *testing.T) {
+func TestIsVolumeSnapshotClassDefaultAnnotation(t *testing.T) {
 	testcases := []struct {
 		name       string
-		typeMeta   metav1.TypeMeta
 		objectMeta metav1.ObjectMeta
 		isDefault  bool
 	}{
 		{
 			name: "no default annotation in snapshot class",
-			typeMeta: metav1.TypeMeta{
-				Kind: "VolumeSnapshotClass",
-			},
 			objectMeta: metav1.ObjectMeta{
-				Annotations: map[string]string{},
+				Annotations: nil,
 			},
 			isDefault: false,
 		},
 		{
 			name: "with default annotation in snapshot class",
-			typeMeta: metav1.TypeMeta{
-				Kind: "VolumeSnapshotClass",
-			},
 			objectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{
 					IsDefaultSnapshotClassAnnotation: "true",
@@ -232,9 +225,6 @@ func TestIsDefaultAnnotation(t *testing.T) {
 		},
 		{
 			name: "with default=false annotation in snapshot class",
-			typeMeta: metav1.TypeMeta{
-				Kind: "VolumeSnapshotClass",
-			},
 			objectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{
 					IsDefaultSnapshotClassAnnotation: "false",
@@ -242,21 +232,31 @@ func TestIsDefaultAnnotation(t *testing.T) {
 			},
 			isDefault: false,
 		},
+	}
+	for _, tc := range testcases {
+		t.Logf("test: %s", tc.name)
+		isDefault := IsVolumeSnapshotClassDefaultAnnotation(tc.objectMeta)
+		if tc.isDefault != isDefault {
+			t.Fatalf("default annotation on class incorrectly detected: %v != %v", isDefault, tc.isDefault)
+		}
+	}
+}
+
+func TestIsVolumeGroupSnapshotClassDefaultAnnotation(t *testing.T) {
+	testcases := []struct {
+		name       string
+		objectMeta metav1.ObjectMeta
+		isDefault  bool
+	}{
 		{
 			name: "no default annotation in group snapshot class",
-			typeMeta: metav1.TypeMeta{
-				Kind: "VolumeGroupSnapshotClass",
-			},
 			objectMeta: metav1.ObjectMeta{
-				Annotations: map[string]string{},
+				Annotations: nil,
 			},
 			isDefault: false,
 		},
 		{
 			name: "with default annotation in group snapshot class",
-			typeMeta: metav1.TypeMeta{
-				Kind: "VolumeGroupSnapshotClass",
-			},
 			objectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{
 					IsDefaultGroupSnapshotClassAnnotation: "true",
@@ -266,9 +266,6 @@ func TestIsDefaultAnnotation(t *testing.T) {
 		},
 		{
 			name: "with default=false annotation in group snapshot class",
-			typeMeta: metav1.TypeMeta{
-				Kind: "VolumeGroupSnapshotClass",
-			},
 			objectMeta: metav1.ObjectMeta{
 				Annotations: map[string]string{
 					IsDefaultGroupSnapshotClassAnnotation: "false",
@@ -276,20 +273,10 @@ func TestIsDefaultAnnotation(t *testing.T) {
 			},
 			isDefault: false,
 		},
-		{
-			name: "unknown kind, not a snapshot or group snapshot class",
-			typeMeta: metav1.TypeMeta{
-				Kind: "PersistentVolume",
-			},
-			objectMeta: metav1.ObjectMeta{
-				Annotations: map[string]string{},
-			},
-			isDefault: false,
-		},
 	}
 	for _, tc := range testcases {
 		t.Logf("test: %s", tc.name)
-		isDefault := IsDefaultAnnotation(tc.typeMeta, tc.objectMeta)
+		isDefault := IsVolumeGroupSnapshotClassDefaultAnnotation(tc.objectMeta)
 		if tc.isDefault != isDefault {
 			t.Fatalf("default annotation on class incorrectly detected: %v != %v", isDefault, tc.isDefault)
 		}