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

Issue:
Is the check for default volumegroupsnapshotclass looking for the correct annotation?
<!-- 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 I create a volumegroupsnapshot without specifying the `volumeGroupSnapshotClassName`, the controller tries to find the default volumegroupsnapshot class but I'm not sure it's looking for the correct annotation.

**What you expected to happen**:

From here: https://github.com/kubernetes-csi/external-snapshotter/blob/0ce7955655dc6fd2533a941ebe4b64257b418b9c/pkg/utils/util.go#L85

It looks to me like the expected annotation to use on a VolumeGroupSnapshotClass is: `groupsnapshot.storage.kubernetes.io/is-default-class`

But in fact it's still checking for the snapshot annotation: `snapshot.storage.kubernetes.io/is-default-class` instead.


It looks like the common IsDefaultAnnotation() function is called here: https://github.com/kubernetes-csi/external-snapshotter/blob/0ce7955655dc6fd2533a941ebe4b64257b418b9c/pkg/common-controller/groupsnapshot_controller_helper.go#L143

Which ends up looking for the snapshot annotation :

https://github.com/kubernetes-csi/external-snapshotter/blob/0ce7955655dc6fd2533a941ebe4b64257b418b9c/pkg/utils/util.go#L263-L269

**How to reproduce it**:

**Anything else we need to know?**:

**Environment**:
- Driver version: Using latest code in master
- Kubernetes version (use `kubectl version`):
- OS (e.g. from /etc/os-release):
- Kernel (e.g. `uname -a`):
- Install tools:
- Others:


Pull request:
Fix detecting default annotation on VolumeGroupSnapshotClass
**What type of PR is this?**
> 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 bug

> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
> /kind flake

**What this PR does / why we need it**:

The annotation to use the selected VolumeGroupSnapshotClass by default is expected to be `groupsnapshot.storage.kubernetes.io/is-default-class=true`, but the check was done on `snapshot.storage.kubernetes.io/is-default-class` instead.

**Which issue(s) this PR fixes**:

Fixes #968

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:

```release-note
Adding the annotation `groupsnapshot.storage.kubernetes.io/is-default-class=true` on a VolumeGroupSnapshotClass will cause that VolumeGroupSnapshotClass to be used by default.
```


Patch:
diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go
index 13b3672e4..3768f1547 100644
--- a/pkg/common-controller/groupsnapshot_controller_helper.go
+++ b/pkg/common-controller/groupsnapshot_controller_helper.go
@@ -140,7 +140,7 @@ func (ctrl *csiSnapshotCommonController) SetDefaultGroupSnapshotClass(groupSnaps
 
 	defaultClasses := []*crdv1alpha1.VolumeGroupSnapshotClass{}
 	for _, groupSnapshotClass := range list {
-		if utils.IsDefaultAnnotation(groupSnapshotClass.ObjectMeta) && pvDriver == groupSnapshotClass.Driver {
+		if utils.IsDefaultAnnotation(groupSnapshotClass.TypeMeta, 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 2a79ec4ab..ae76461c6 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -1416,7 +1416,7 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
 
 	defaultClasses := []*crdv1.VolumeSnapshotClass{}
 	for _, class := range list {
-		if utils.IsDefaultAnnotation(class.ObjectMeta) && pvDriver == class.Driver {
+		if utils.IsDefaultAnnotation(class.TypeMeta, 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 f47de22a8..26f4ba77d 100644
--- a/pkg/utils/util.go
+++ b/pkg/utils/util.go
@@ -262,9 +262,16 @@ func GetDynamicSnapshotContentNameForSnapshot(snapshot *crdv1.VolumeSnapshot) st
 
 // IsDefaultAnnotation returns a boolean if
 // the annotation is set
-func IsDefaultAnnotation(obj metav1.ObjectMeta) bool {
-	if obj.Annotations[IsDefaultSnapshotClassAnnotation] == "true" {
-		return true
+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
+		}
 	}
 
 	return false


Test patch:
diff --git a/pkg/utils/util_test.go b/pkg/utils/util_test.go
index 1d7dd7d9b..fd2127fa0 100644
--- a/pkg/utils/util_test.go
+++ b/pkg/utils/util_test.go
@@ -207,3 +207,98 @@ func TestRemovePrefixedCSIParams(t *testing.T) {
 		}
 	}
 }
+
+func TestIsDefaultAnnotation(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{},
+			},
+			isDefault: false,
+		},
+		{
+			name: "with default annotation in snapshot class",
+			typeMeta: metav1.TypeMeta{
+				Kind: "VolumeSnapshotClass",
+			},
+			objectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					IsDefaultSnapshotClassAnnotation: "true",
+				},
+			},
+			isDefault: true,
+		},
+		{
+			name: "with default=false annotation in snapshot class",
+			typeMeta: metav1.TypeMeta{
+				Kind: "VolumeSnapshotClass",
+			},
+			objectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					IsDefaultSnapshotClassAnnotation: "false",
+				},
+			},
+			isDefault: false,
+		},
+		{
+			name: "no default annotation in group snapshot class",
+			typeMeta: metav1.TypeMeta{
+				Kind: "VolumeGroupSnapshotClass",
+			},
+			objectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{},
+			},
+			isDefault: false,
+		},
+		{
+			name: "with default annotation in group snapshot class",
+			typeMeta: metav1.TypeMeta{
+				Kind: "VolumeGroupSnapshotClass",
+			},
+			objectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					IsDefaultGroupSnapshotClassAnnotation: "true",
+				},
+			},
+			isDefault: true,
+		},
+		{
+			name: "with default=false annotation in group snapshot class",
+			typeMeta: metav1.TypeMeta{
+				Kind: "VolumeGroupSnapshotClass",
+			},
+			objectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{
+					IsDefaultGroupSnapshotClassAnnotation: "false",
+				},
+			},
+			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)
+		if tc.isDefault != isDefault {
+			t.Fatalf("default annotation on class incorrectly detected: %v != %v", isDefault, tc.isDefault)
+		}
+	}
+}