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

Issue:
snapshotter panic in volumegroupsnapshot processing


**What happened**:
got this in the 8.0.1 snapshotter while developing our CSI drivers volumegroupsnapshot logic, even though my code
is problematic and not complete, I would expect the snapshotter to not panic.

0730 20:20:28.055041       1 snapshot_controller.go:595] Removed protection finalizer from volume snapshot content snapcontent-13f1a3900670d351195c9d814444c5957f3cc787f79e36f934eef7f669bff83b-2024-07-30-7.52.43
I0730 20:20:28.055064       1 util.go:264] storeObjectUpdate updating content "snapcontent-13f1a3900670d351195c9d814444c5957f3cc787f79e36f934eef7f669bff83b-2024-07-30-7.52.43" with version 2385585
E0730 20:20:28.055275       1 groupsnapshot_helper.go:157] could not sync group snapshot content "groupsnapcontent-b09c477c-0e10-4900-888a-e5d676fbe236": failed to delete group snapshot "groupsnapcontent-b09c477c-0e10-4900-888a-e5d676fbe236", err: cannot delete group snapshot content groupsnapcontent-b09c477c-0e10-4900-888a-e5d676fbe236. No snapshots found in the group snapshot
I0730 20:20:28.055297       1 groupsnapshot_helper.go:79] Failed to sync group snapshot content "groupsnapcontent-b09c477c-0e10-4900-888a-e5d676fbe236", will retry again: failed to delete group snapshot "groupsnapcontent-b09c477c-0e10-4900-888a-e5d676fbe236", err: cannot delete group snapshot content groupsnapcontent-b09c477c-0e10-4900-888a-e5d676fbe236. No snapshots found in the group snapshot
I0730 20:20:28.055328       1 event.go:377] Event(v1.ObjectReference{Kind:"VolumeGroupSnapshotContent", Namespace:"", Name:"groupsnapcontent-b09c477c-0e10-4900-888a-e5d676fbe236", UID:"392530a9-7938-4e96-aecd-7ad7c7c93452", APIVersion:"groupsnapshot.storage.k8s.io/v1alpha1", ResourceVersion:"2304318", FieldPath:""}): type: 'Warning' reason: 'GroupSnapshotDeleteError' Failed to delete group snapshot
E0730 20:20:28.056504       1 groupsnapshot_helper.go:157] could not sync group snapshot content "groupsnapcontent-cf45513b-9cf3-4c36-9566-b2c15889decc": failed to delete group snapshot "groupsnapcontent-cf45513b-9cf3-4c36-9566-b2c15889decc", err: cannot delete group snapshot content groupsnapcontent-cf45513b-9cf3-4c36-9566-b2c15889decc. No snapshots found in the group snapshot
I0730 20:20:28.056521       1 groupsnapshot_helper.go:79] Failed to sync group snapshot content "groupsnapcontent-cf45513b-9cf3-4c36-9566-b2c15889decc", will retry again: failed to delete group snapshot "groupsnapcontent-cf45513b-9cf3-4c36-9566-b2c15889decc", err: cannot delete group snapshot content groupsnapcontent-cf45513b-9cf3-4c36-9566-b2c15889decc. No snapshots found in the group snapshot
I0730 20:20:28.056545       1 event.go:377] Event(v1.ObjectReference{Kind:"VolumeGroupSnapshotContent", Namespace:"", Name:"groupsnapcontent-cf45513b-9cf3-4c36-9566-b2c15889decc", UID:"494c793f-7d00-4b4c-acb4-b917716d5570", APIVersion:"groupsnapshot.storage.k8s.io/v1alpha1", ResourceVersion:"2314362", FieldPath:""}): type: 'Warning' reason: 'GroupSnapshotDeleteError' Failed to delete group snapshot
I0730 20:20:28.057393       1 snapshot_controller_base.go:206] enqueued "snapcontent-13f1a3900670d351195c9d814444c5957f3cc787f79e36f934eef7f669bff83b-2024-07-30-7.52.43" for sync
I0730 20:20:28.057414       1 snapshot_controller_base.go:247] syncContentByKey[snapcontent-13f1a3900670d351195c9d814444c5957f3cc787f79e36f934eef7f669bff83b-2024-07-30-7.52.43]
I0730 20:20:28.057428       1 snapshot_controller_base.go:367] content "snapcontent-13f1a3900670d351195c9d814444c5957f3cc787f79e36f934eef7f669bff83b-2024-07-30-7.52.43" deleted
E0730 20:20:28.059942       1 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 53 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x19f7de0, 0x2dbdf00})
	/workspace/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x85
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc0004d20f0?})
	/workspace/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x6b
panic({0x19f7de0?, 0x2dbdf00?})
	/go/pkg/csiprow.XXXXhhNCeB/go-1.22.3/src/runtime/panic.go:770 +0x132
github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).deleteCSIGroupSnapshotOperation(0xc000180a20, 0xc00040e000)
	/workspace/pkg/sidecar-controller/groupsnapshot_helper.go:256 +0x4c8
github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).syncGroupSnapshotContent(0xc000180a20, 0xc00040e000)
	/workspace/pkg/sidecar-controller/groupsnapshot_helper.go:184 +0x47d
github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).updateGroupSnapshotContentInInformerCache(0xc000180a20, 0xc00040e000)
	/workspace/pkg/sidecar-controller/groupsnapshot_helper.go:150 +0x12d
github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).syncGroupSnapshotContentByKey(0xc000180a20, {0xc000059980, 0x35})
	/workspace/pkg/sidecar-controller/groupsnapshot_helper.go:102 +0x707
github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).groupSnapshotContentWorker(0xc000180a20)
	/workspace/pkg/sidecar-controller/groupsnapshot_helper.go:75 +0xf3
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x30?)
	/workspace/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:226 +0x33
k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc00080a340, {0x1f58d40, 0xc0003e5dd0}, 0x1, 0xc0000b77a0)
	/workspace/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:227 +0xaf
k8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc00080a340, 0x0, 0x0, 0x1, 0xc0000b77a0)
	/workspace/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:204 +0x7f
k8s.io/apimachinery/pkg/util/wait.Until(...)
	/workspace/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:161
created by github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).Run in goroutine 76
	/workspace/pkg/sidecar-controller/snapshot_controller_base.go:187 +0x3d0
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x17de408]

snapshotter sidecar panic

**What you expected to happen**:
just throw errors

**How to reproduce it**:
construct CreateVolumeGroupSnapshotResponse with invalid snapshots or remove snapshots on the storage device or have resources not cleaned up properly due to errors.

**Anything else we need to know?**:

**Environment**:
- Driver version: 8.0.1
- Kubernetes version (use `kubectl version`): v1.30.1+k3s1
- OS (e.g. from /etc/os-release): ubuntu
- Kernel (e.g. `uname -a`):
- Install tools: k3s
- Others:


Pull request description:
Add nil check for groupSnapshotContent in deleteCSIGroupSnapshotOperaion and unit tests
- Add a check to ensure `groupSnapshotContent` is not nil in `deleteCSIGroupSnapshotOperation` to prevent panics.
- Enhance error handling for missing `SnapshotHandle` in `VolumeSnapshotContent` objects during deletion.
- Create initial unit tests in `groupsnapshot_helper_test.go` to cover cases where `groupSnapshotContent` is nil or incomplete.

<!--  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?**
> 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**:
Before fix:
```bash
[Running: /usr/local/go/bin/go test -timeout 30s -run ^TestDeleteCSIGroupSnapshotOperation$ [github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller](http://github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller)]
--- FAIL: TestDeleteCSIGroupSnapshotOperation (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x105c44fd4]
goroutine 7 [running]:
testing.tRunner.func1.2({0x1060d4860, 0x107102300})
        /Users/manish/.gvm/pkgsets/go1.22.5/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.5.darwin-arm64/src/testing/testing.go:1631 +0x1c4
testing.tRunner.func1()
        /Users/manish/.gvm/pkgsets/go1.22.5/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.5.darwin-arm64/src/testing/testing.go:1634 +0x33c
panic({0x1060d4860?, 0x107102300?})
        /Users/manish/.gvm/pkgsets/go1.22.5/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.5.darwin-arm64/src/runtime/panic.go:770 +0x124
[github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).deleteCSIGroupSnapshotOperation(0x14000094e48](http://github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.(*csiSnapshotSideCarController).deleteCSIGroupSnapshotOperation(0x14000094e48), 0x140000d65a0)
        /Users/manish/work/openshift/external-snapshotter/pkg/sidecar-controller/groupsnapshot_helper.go:263 +0x404
[github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.TestDeleteCSIGroupSnapshotOperation(0x140002df520)](http://github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller.TestDeleteCSIGroupSnapshotOperation(0x140002df520))
        /Users/manish/work/openshift/external-snapshotter/pkg/sidecar-controller/groupsnapshot_helper_test.go:50 +0x1d4
testing.tRunner(0x140002df520, 0x10634dda8)
        /Users/manish/.gvm/pkgsets/go1.22.5/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.5.darwin-arm64/src/testing/testing.go:1689 +0xec
created by testing.(*T).Run in goroutine 1
        /Users/manish/.gvm/pkgsets/go1.22.5/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.5.darwin-arm64/src/testing/testing.go:1742 +0x318
FAIL    [github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller](http://github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller)        1.298s
FAIL
[Command exited with 1]
```

After fix:
```bash
[Running: /usr/local/go/bin/go test -timeout 30s -run ^TestDeleteCSIGroupSnapshotOperation$ [github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller](http://github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller)]
ok      [github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller](http://github.com/kubernetes-csi/external-snapshotter/v8/pkg/sidecar-controller) 
```

**Which issue(s) this PR fixes**:


Fixes #1123

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:

```release-note
NONE
```


Patch:
diff --git a/pkg/sidecar-controller/groupsnapshot_helper.go b/pkg/sidecar-controller/groupsnapshot_helper.go
index bfd94b435..34a6ed67c 100644
--- a/pkg/sidecar-controller/groupsnapshot_helper.go
+++ b/pkg/sidecar-controller/groupsnapshot_helper.go
@@ -238,6 +238,9 @@ func (ctrl csiSnapshotSideCarController) removeGroupSnapshotContentFinalizer(gro
 
 // Delete a groupsnapshot: Ask the backend to remove the groupsnapshot device
 func (ctrl *csiSnapshotSideCarController) deleteCSIGroupSnapshotOperation(groupSnapshotContent *crdv1beta1.VolumeGroupSnapshotContent) error {
+	if groupSnapshotContent == nil {
+		return fmt.Errorf("groupSnapshotContent is nil")
+	}
 	klog.V(5).Infof("deleteCSIGroupSnapshotOperation [%s] started", groupSnapshotContent.Name)
 
 	snapshotterCredentials, err := ctrl.GetCredentialsFromAnnotationForGroupSnapshot(groupSnapshotContent)
@@ -258,6 +261,7 @@ func (ctrl *csiSnapshotSideCarController) deleteCSIGroupSnapshotOperation(groupS
 		} else if groupSnapshotContent.Spec.Source.GroupSnapshotHandles != nil {
 			ids := groupSnapshotContent.Spec.Source.GroupSnapshotHandles.VolumeSnapshotHandles
 			snapshotIDs = slices.Clone(ids)
+
 		}
 	}
 


Test patch:
diff --git a/pkg/sidecar-controller/groupsnapshot_helper_test.go b/pkg/sidecar-controller/groupsnapshot_helper_test.go
new file mode 100644
index 000000000..86af6f7dc
--- /dev/null
+++ b/pkg/sidecar-controller/groupsnapshot_helper_test.go
@@ -0,0 +1,53 @@
+package sidecar_controller
+
+import (
+	"testing"
+
+	crdv1beta1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1beta1"
+
+	v1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/client-go/tools/record"
+)
+
+type fakeContentLister struct {
+}
+
+func (f *fakeContentLister) List(selector labels.Selector) (ret []*v1.VolumeSnapshotContent, err error) {
+	return nil, nil
+}
+func (f *fakeContentLister) Get(name string) (*v1.VolumeSnapshotContent, error) {
+	return &v1.VolumeSnapshotContent{}, nil
+}
+
+func TestDeleteCSIGroupSnapshotOperation(t *testing.T) {
+	ctrl := &csiSnapshotSideCarController{
+		contentLister: &fakeContentLister{},
+		handler:       &csiHandler{},
+		eventRecorder: &record.FakeRecorder{},
+	}
+
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("deleteCSIGroupSnapshotOperation() panicked with: %v", r)
+		}
+	}()
+	err := ctrl.deleteCSIGroupSnapshotOperation(nil)
+	if err == nil {
+		t.Errorf("expected deleteCSIGroupSnapshotOperation to return error when groupsnapshotContent is nil: %v", err)
+	}
+	gsc := crdv1beta1.VolumeGroupSnapshotContent{
+		Status: &crdv1beta1.VolumeGroupSnapshotContentStatus{
+			VolumeSnapshotHandlePairList: []crdv1beta1.VolumeSnapshotHandlePair{
+				{
+					VolumeHandle:   "test-pv",
+					SnapshotHandle: "test-vsc",
+				},
+			},
+		},
+	}
+	err = ctrl.deleteCSIGroupSnapshotOperation(&gsc)
+	if err == nil {
+		t.Errorf("expected deleteCSIGroupSnapshotOperation to return error when groupsnapshotContent is empty: %v", err)
+	}
+}