Skip to content

Commit

Permalink
Add nil check for groupSnapshotContent in deleteCSIGroupSnapshotOpera…
Browse files Browse the repository at this point in the history
…tion 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.

Signed-off-by: Manish <[email protected]>
  • Loading branch information
manishym committed Oct 9, 2024
1 parent 5ad0cfb commit 9752e2a
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 0 deletions.
7 changes: 7 additions & 0 deletions pkg/sidecar-controller/groupsnapshot_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 *crdv1alpha1.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)
Expand All @@ -253,6 +256,10 @@ func (ctrl *csiSnapshotSideCarController) deleteCSIGroupSnapshotOperation(groupS
if err != nil {
return fmt.Errorf("failed to get snapshot content %s from snapshot content store: %v", contentRef.VolumeSnapshotContentRef.Name, err)
}
if snapshotContent.Status == nil || snapshotContent.Status.SnapshotHandle == nil {
klog.V(5).Infof("snapshot content %s does not have snapshot handle", snapshotContent.Name)
continue
}
snapshotIDs = append(snapshotIDs, *snapshotContent.Status.SnapshotHandle)
}
}
Expand Down
54 changes: 54 additions & 0 deletions pkg/sidecar-controller/groupsnapshot_helper_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package sidecar_controller

import (
"testing"

crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1alpha1"

v1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
core_v1 "k8s.io/api/core/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 := crdv1alpha1.VolumeGroupSnapshotContent{
Status: &crdv1alpha1.VolumeGroupSnapshotContentStatus{
PVVolumeSnapshotContentList: []crdv1alpha1.PVVolumeSnapshotContentPair{
{
PersistentVolumeRef: core_v1.LocalObjectReference{Name: "test-pv"},
VolumeSnapshotContentRef: core_v1.LocalObjectReference{Name: "test-vsc"},
},
},
},
}
err = ctrl.deleteCSIGroupSnapshotOperation(&gsc)
if err == nil {
t.Errorf("expected deleteCSIGroupSnapshotOperation to return error when groupsnapshotContent is empty: %v", err)
}
}

0 comments on commit 9752e2a

Please sign in to comment.