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

Issue:
Failed to remove finalizer on VolumeAttachment
After https://github.com/kubernetes-csi/external-attacher/pull/200 merged, our pd driver tests started failing/flaking, especially the test cases that create a 2nd pod on the same node.

It seems like the attacher keeps trying to processing the VolumeAttachment object, but VolumeAttachment object is not able to be deleted. These messages are repeated forever:
```
1124 I0420 21:40:11.832369       1 csi_handler.go:549] Detached "csi-4b1f34f83ecd1528d4aafd1104099e9aaf3e0d81222ab41ccf999555c3c1ff7b"
1125 I0420 21:40:11.832606       1 util.go:80] Marking as detached "csi-4b1f34f83ecd1528d4aafd1104099e9aaf3e0d81222ab41ccf999555c3c1ff7b"
1126 I0420 21:40:11.837749       1 util.go:95] Finalizer removed from "csi-4b1f34f83ecd1528d4aafd1104099e9aaf3e0d81222ab41ccf999555c3c1ff7b"
1127 I0420 21:40:11.837787       1 csi_handler.go:280] Fully detached "csi-4b1f34f83ecd1528d4aafd1104099e9aaf3e0d81222ab41ccf999555c3c1ff7b"
1128 I0420 21:40:11.837817       1 csi_handler.go:225] CSIHandler: finished processing "csi-4b1f34f83ecd1528d4aafd1104099e9aaf3e0d81222ab41ccf999555c3c1ff7b"

```

Pull request description:
Update VolumeAttachment.Finalizers as separate subresource in markAsDetatched
/kind bug

**What this PR does / why we need it**:
The update of the VolumeAttachment version from v1beta to v1 changed its status to be a subresource. markAsDetached() updated both the status and the finalizers in a single patch, and the update to v1 blindly put in a "status" subresource restriction, causing the finalizer removal to be silently ignored. This change removes the finalizer in a second patch.

**Which issue(s) this PR fixes**:
Fixes #225

```release-note
Fixed #225
```


Patch:
diff --git a/cmd/csi-attacher/main.go b/cmd/csi-attacher/main.go
index e695b4c9..839c6502 100644
--- a/cmd/csi-attacher/main.go
+++ b/cmd/csi-attacher/main.go
@@ -155,7 +155,7 @@ func main() {
 		if supportsAttach {
 			pvLister := factory.Core().V1().PersistentVolumes().Lister()
 			vaLister := factory.Storage().V1().VolumeAttachments().Lister()
-			csiNodeLister := factory.Storage().V1().CSINodes().Lister()
+			csiNodeLister := factory.Storage().V1beta1().CSINodes().Lister()
 			volAttacher := attacher.NewAttacher(csiConn)
 			CSIVolumeLister := attacher.NewVolumeLister(csiConn)
 			handler = controller.NewCSIHandler(clientset, csiAttacher, volAttacher, CSIVolumeLister, pvLister, csiNodeLister, vaLister, timeout, supportsReadOnly, csitrans.New())
diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go
index 3b68d7b1..75a4e3b0 100644
--- a/pkg/controller/csi_handler.go
+++ b/pkg/controller/csi_handler.go
@@ -32,6 +32,7 @@ import (
 	"k8s.io/client-go/kubernetes"
 	corelisters "k8s.io/client-go/listers/core/v1"
 	storagelisters "k8s.io/client-go/listers/storage/v1"
+	storagelistersv1beta1 "k8s.io/client-go/listers/storage/v1beta1"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog"
 )
@@ -61,7 +62,7 @@ type csiHandler struct {
 	attacher                attacher.Attacher
 	CSIVolumeLister         VolumeLister
 	pvLister                corelisters.PersistentVolumeLister
-	csiNodeLister           storagelisters.CSINodeLister
+	csiNodeLister           storagelistersv1beta1.CSINodeLister
 	vaLister                storagelisters.VolumeAttachmentLister
 	vaQueue, pvQueue        workqueue.RateLimitingInterface
 	forceSync               map[string]bool
@@ -80,7 +81,7 @@ func NewCSIHandler(
 	attacher attacher.Attacher,
 	CSIVolumeLister VolumeLister,
 	pvLister corelisters.PersistentVolumeLister,
-	csiNodeLister storagelisters.CSINodeLister,
+	csiNodeLister storagelistersv1beta1.CSINodeLister,
 	vaLister storagelisters.VolumeAttachmentLister,
 	timeout *time.Duration,
 	supportsPublishReadOnly bool,
diff --git a/pkg/controller/util.go b/pkg/controller/util.go
index 10e24e45..57ba2d11 100644
--- a/pkg/controller/util.go
+++ b/pkg/controller/util.go
@@ -24,9 +24,10 @@ import (
 	"regexp"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
-	jsonpatch "github.com/evanphx/json-patch"
-	v1 "k8s.io/api/core/v1"
+	"github.com/evanphx/json-patch"
+	"k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
+	storagev1beta1 "k8s.io/api/storage/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
@@ -78,7 +79,6 @@ func markAsDetached(client kubernetes.Interface, va *storage.VolumeAttachment) (
 
 	klog.V(4).Infof("Marking as detached %q", va.Name)
 	clone := va.DeepCopy()
-	clone.Finalizers = newFinalizers
 	clone.Status.Attached = false
 	clone.Status.DetachError = nil
 	clone.Status.AttachmentMetadata = nil
@@ -91,6 +91,18 @@ func markAsDetached(client kubernetes.Interface, va *storage.VolumeAttachment) (
 	if err != nil {
 		return va, err
 	}
+
+	// As Finalizers is not in the status subresource it must be patched separately. It is removed after the status update so the resource is not prematurely deleted.
+	clone = newVA.DeepCopy()
+	clone.Finalizers = newFinalizers
+	patch, err = createMergePatch(newVA, clone)
+	if err != nil {
+		return newVA, err
+	}
+	newVA, err = client.StorageV1().VolumeAttachments().Patch(context.TODO(), newVA.Name, types.MergePatchType, patch, metav1.PatchOptions{}, "")
+	if err != nil {
+		return newVA, err
+	}
 	klog.V(4).Infof("Finalizer removed from %q", va.Name)
 	return newVA, nil
 }
@@ -118,7 +130,7 @@ func GetFinalizerName(driver string) string {
 }
 
 // GetNodeIDFromCSINode returns nodeID from CSIDriverInfoSpec
-func GetNodeIDFromCSINode(driver string, csiNode *storage.CSINode) (string, bool) {
+func GetNodeIDFromCSINode(driver string, csiNode *storagev1beta1.CSINode) (string, bool) {
 	for _, d := range csiNode.Spec.Drivers {
 		if d.Name == driver {
 			return d.NodeID, true


Test patch:
diff --git a/pkg/controller/csi_handler_test.go b/pkg/controller/csi_handler_test.go
index 757316e4..228fbc4f 100644
--- a/pkg/controller/csi_handler_test.go
+++ b/pkg/controller/csi_handler_test.go
@@ -26,6 +26,7 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
+	storagev1beta1 "k8s.io/api/storage/v1beta1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -57,7 +58,7 @@ func csiHandlerFactory(client kubernetes.Interface, informerFactory informers.Sh
 		csi,
 		lister,
 		informerFactory.Core().V1().PersistentVolumes().Lister(),
-		informerFactory.Storage().V1().CSINodes().Lister(),
+		informerFactory.Storage().V1beta1().CSINodes().Lister(),
 		informerFactory.Storage().V1().VolumeAttachments().Lister(),
 		&timeout,
 		true, /* supports PUBLISH_READONLY */
@@ -72,7 +73,7 @@ func csiHandlerFactoryNoReadOnly(client kubernetes.Interface, informerFactory in
 		csi,
 		lister,
 		informerFactory.Core().V1().PersistentVolumes().Lister(),
-		informerFactory.Storage().V1().CSINodes().Lister(),
+		informerFactory.Storage().V1beta1().CSINodes().Lister(),
 		informerFactory.Storage().V1().VolumeAttachments().Lister(),
 		&timeout,
 		false, /* does not support PUBLISH_READONLY */
@@ -204,13 +205,13 @@ func nodeWithAnnotations() *v1.Node {
 	return node
 }
 
-func csiNode() *storage.CSINode {
-	return &storage.CSINode{
+func csiNode() *storagev1beta1.CSINode {
+	return &storagev1beta1.CSINode{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: testNodeName,
 		},
-		Spec: storage.CSINodeSpec{
-			Drivers: []storage.CSINodeDriver{
+		Spec: storagev1beta1.CSINodeSpec{
+			Drivers: []storagev1beta1.CSINodeDriver{
 				{
 					Name:   testAttacherName,
 					NodeID: testNodeID,
@@ -220,12 +221,12 @@ func csiNode() *storage.CSINode {
 	}
 }
 
-func csiNodeEmpty() *storage.CSINode {
-	return &storage.CSINode{
+func csiNodeEmpty() *storagev1beta1.CSINode {
+	return &storagev1beta1.CSINode{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: testNodeName,
 		},
-		Spec: storage.CSINodeSpec{Drivers: []storage.CSINodeDriver{}},
+		Spec: storagev1beta1.CSINodeSpec{Drivers: []storagev1beta1.CSINodeDriver{}},
 	}
 }
 
@@ -918,8 +919,11 @@ func TestCSIHandler(t *testing.T) {
 			addedVA:        deleted(va(true, fin, ann)),
 			expectedActions: []core.Action{
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(va(true, fin, ann)),
+					types.MergePatchType, patch(deleted(va(true, "", ann)),
 						deleted(va(false /*attached*/, "", ann))), "status"),
+				core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(va(false, fin, ann)),
+						deleted(va(false /*attached*/, "", ann)))),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, 0},
@@ -931,8 +935,11 @@ func TestCSIHandler(t *testing.T) {
 			addedVA:        deleted(vaWithInlineSpec(va(true, fin, ann))),
 			expectedActions: []core.Action{
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(vaWithInlineSpec(va(true, fin, ann))),
+					types.MergePatchType, patch(deleted(vaWithInlineSpec(va(true, "", ann))),
 						deleted(vaWithInlineSpec(va(false /*attached*/, "", ann)))), "status"),
+				core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(vaWithInlineSpec(va(false, fin, ann))),
+						deleted(vaWithInlineSpec(va(false /*attached*/, "", ann))))),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, 0},
@@ -945,8 +952,11 @@ func TestCSIHandler(t *testing.T) {
 			expectedActions: []core.Action{
 				core.NewGetAction(secretGroupResourceVersion, "default", "secret"),
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(va(true, fin, ann)),
+					types.MergePatchType, patch(deleted(va(true, "", ann)),
 						deleted(va(false /*attached*/, "", ann))), "status"),
+				core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(vaWithInlineSpec(va(false, fin, ann))),
+						deleted(vaWithInlineSpec(va(false /*attached*/, "", ann))))),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{"foo": "bar"}, readWrite, success, ignored, noMetadata, 0},
@@ -959,9 +969,14 @@ func TestCSIHandler(t *testing.T) {
 			expectedActions: []core.Action{
 				core.NewGetAction(secretGroupResourceVersion, "default", "secret"),
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(true, fin, ann)), "secret")),
+					types.MergePatchType, patch(deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(true, "", ann)), "secret")),
 						deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false /*attached*/, "", ann)),
-							"secret"))), "status")},
+							"secret"))), "status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false, fin, ann)), "secret")),
+						deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false /*attached*/, "", ann)),
+							"secret"))), ""),
+			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{"foo": "bar"}, readWrite, success, ignored, noMetadata, 0},
 			},
@@ -973,8 +988,11 @@ func TestCSIHandler(t *testing.T) {
 			expectedActions: []core.Action{
 				core.NewGetAction(secretGroupResourceVersion, "default", "emptySecret"),
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(va(true, fin, ann)),
+					types.MergePatchType, patch(deleted(va(true, "", ann)),
 						deleted(va(false /*attached*/, "", ann))), "status"),
+				core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(va(false, fin, ann)),
+						deleted(va(false /*attached*/, "", ann)))),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{}, readWrite, success, ignored, noMetadata, 0},
@@ -988,9 +1006,14 @@ func TestCSIHandler(t *testing.T) {
 				core.NewGetAction(secretGroupResourceVersion, "default", "emptySecret"),
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone,
 					testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(true, fin, ann)), "emptySecret")),
+					types.MergePatchType, patch(deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(true, "", ann)), "emptySecret")),
 						deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false /*attached*/, "", ann)),
 							"emptySecret"))), "status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone,
+					testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false, fin, ann)), "emptySecret")),
+						deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false /*attached*/, "", ann)),
+							"emptySecret"))), ""),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{}, readWrite, success, ignored, noMetadata, 0},
@@ -1018,11 +1041,14 @@ func TestCSIHandler(t *testing.T) {
 			expectedActions: []core.Action{
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone,
 					testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(va(true, fin, ann)),
-						deleted(vaWithDetachError(va(true, fin, ann), "mock error"))), "status"),
+					types.MergePatchType, patch(deleted(va(true, "", ann)),
+						deleted(vaWithDetachError(va(true, "", ann), "mock error"))), "status"),
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(va(true, fin, ann)),
+					types.MergePatchType, patch(deleted(va(true, "", ann)),
 						deleted(va(false, "", ann))), "status"),
+				core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(va(false, fin, ann)),
+						deleted(va(false, "", ann)))),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, fmt.Errorf("mock error"), ignored, noMetadata, 0},
@@ -1035,8 +1061,11 @@ func TestCSIHandler(t *testing.T) {
 			addedVA:        deleted(va(true, fin, ann)),
 			expectedActions: []core.Action{
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(va(true, fin, ann)),
+					types.MergePatchType, patch(deleted(va(true, "", ann)),
 						deleted(va(false /*attached*/, "", ann))), "status"),
+				core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(va(false, fin, ann)),
+						deleted(va(false /*attached*/, "", ann)))),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, 500 * time.Millisecond},
@@ -1057,8 +1086,13 @@ func TestCSIHandler(t *testing.T) {
 			expectedActions: []core.Action{
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone,
 					testPVName+"-"+testNodeName,
-					types.MergePatchType, patch(deleted(va(true, fin, ann)),
-						deleted(vaWithDetachError(va(true, fin, ann), "persistentvolume \"pv1\" not found"))),
+					types.MergePatchType, patch(deleted(va(true, "", ann)),
+						deleted(vaWithDetachError(va(true, "", ann), "persistentvolume \"pv1\" not found"))),
+					"status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone,
+					testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(va(false, "", ann)),
+						deleted(vaWithDetachError(va(false, "", ann), "persistentvolume \"pv1\" not found"))),
 					"status"),
 			},
 		},
@@ -1068,7 +1102,7 @@ func TestCSIHandler(t *testing.T) {
 			addedVA:        deleted(va(true, fin, ann)),
 			reactors: []reaction{
 				{
-					verb:     "update",
+					verb:     "patch",
 					resource: "volumeattachments",
 					reactor: func(t *testing.T) core.ReactionFunc {
 						i := 0
@@ -1084,6 +1118,10 @@ func TestCSIHandler(t *testing.T) {
 					},
 				},
 			},
+			// The handler will perform the same patch regardless of whether the error save was successful or not. The only
+			// difference is in the errors logged (which are not checked here).
+			// Because the detach never succeeds, the test will loop as long as there are expected actions remaining.
+			// 4 such loops are tested below: two when the error save fails, and then two when the error succeeds.
 			expectedActions: []core.Action{
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
 					types.MergePatchType, patch(deleted(va(true, fin, ann)),
@@ -1097,6 +1135,10 @@ func TestCSIHandler(t *testing.T) {
 					types.MergePatchType, patch(deleted(va(true, fin, ann)),
 						deleted(vaWithDetachError(va(true, fin, ann), "persistentvolume \"pv1\" not found"))),
 					"status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(deleted(va(true, fin, ann)),
+						deleted(vaWithDetachError(va(true, fin, ann), "persistentvolume \"pv1\" not found"))),
+					"status"),
 			},
 		},
 		{
@@ -1142,9 +1184,14 @@ func TestCSIHandler(t *testing.T) {
 			expectedActions: []core.Action{
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
 					types.MergePatchType, patch(
-						deleted(va(true, fin, map[string]string{vaNodeIDAnnotation: "annotatedNodeID"})),
+						deleted(va(true, "", map[string]string{vaNodeIDAnnotation: "annotatedNodeID"})),
 						deleted(va(false /*attached*/, "",
 							map[string]string{vaNodeIDAnnotation: "annotatedNodeID"}))), "status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(
+						deleted(va(false, fin, map[string]string{vaNodeIDAnnotation: "annotatedNodeID"})),
+						deleted(va(false /*attached*/, "",
+							map[string]string{vaNodeIDAnnotation: "annotatedNodeID"}))), ""),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, "annotatedNodeID", noAttrs, noSecrets, readWrite, success, detached, noMetadata, 0},
@@ -1175,21 +1222,25 @@ func TestCSIHandler(t *testing.T) {
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone,
 					testPVName+"-"+testNodeName,
 					types.MergePatchType, patch(
-						deleted(va(false, fin, ann)),
+						deleted(va(false, "", ann)),
 						deleted(va(false, "", ann))), "status"),
 				// Saving error succeeds
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone,
 					testPVName+"-"+testNodeName,
 					types.MergePatchType, patch(
-						deleted(va(false, fin, ann)),
-						vaWithDetachError(deleted(va(false, fin, ann)),
+						deleted(va(false, "", ann)),
+						vaWithDetachError(deleted(va(false, "", ann)),
 							"could not mark as detached: volumeattachments.storage.k8s."+
 								"io \"pv1-node1\" is forbidden: mock error")), "status"),
-				// Second save of attached=false succeeds
+				// Second save of attached=false succeeds and the finalizer is subsequently deleted.
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
 					types.MergePatchType, patch(
-						deleted(va(false, fin, ann)),
+						deleted(va(false, "", ann)),
 						deleted(va(false, "", ann))), "status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(
+						deleted(va(false, fin, ann)),
+						deleted(va(false, "", ann))), ""),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, detached, noMetadata, 0},
@@ -1204,8 +1255,12 @@ func TestCSIHandler(t *testing.T) {
 				// Finalizer is saved first
 				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
 					types.MergePatchType, patch(
-						deleted(va(true, fin, ann)),
+						deleted(va(true, "", ann)),
 						deleted(va(false /*attached*/, "", ann))), "status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(
+						deleted(va(false, fin, ann)),
+						deleted(va(false /*attached*/, "", ann))), ""),
 			},
 			expectedCSICalls: []csiCall{
 				{"detach", "projects/UNSPECIFIED/zones/testZone/disks/testpd", testNodeID,
diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go
index 4f849397..f0194f85 100644
--- a/pkg/controller/framework_test.go
+++ b/pkg/controller/framework_test.go
@@ -31,6 +31,7 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
+	storagev1beta1 "k8s.io/api/storage/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/informers"
@@ -123,7 +124,7 @@ func runTests(t *testing.T, handlerFactory handlerFactory, tests []testCase) {
 		csiObjs := []runtime.Object{}
 		for _, obj := range objs {
 			switch obj.(type) {
-			case *storage.CSINode:
+			case *storagev1beta1.CSINode:
 				csiObjs = append(csiObjs, obj)
 			default:
 				coreObjs = append(coreObjs, obj)
@@ -136,7 +137,7 @@ func runTests(t *testing.T, handlerFactory handlerFactory, tests []testCase) {
 		vaInformer := informers.Storage().V1().VolumeAttachments()
 		pvInformer := informers.Core().V1().PersistentVolumes()
 		nodeInformer := informers.Core().V1().Nodes()
-		csiNodeInformer := informers.Storage().V1().CSINodes()
+		csiNodeInformer := informers.Storage().V1beta1().CSINodes()
 		// Fill the informers with initial objects so controller can Get() them
 		for _, obj := range objs {
 			switch obj.(type) {
@@ -148,7 +149,7 @@ func runTests(t *testing.T, handlerFactory handlerFactory, tests []testCase) {
 				vaInformer.Informer().GetStore().Add(obj)
 			case *v1.Secret:
 				// Secrets are not cached in any informer
-			case *storage.CSINode:
+			case *storagev1beta1.CSINode:
 				csiNodeInformer.Informer().GetStore().Add(obj)
 			default:
 				t.Fatalf("Unknown initalObject type: %+v", obj)