Volume AttachablePlugin.CanAttach() now returns both bool and error

Author: Vladimir Vivien

pkg/controller/volume/attachdetach/testing/testvolumespec.go

@@ -308,8 +308,8 @@ func (plugin *TestPlugin) NewDetacher() (volume.Detacher, error) {
 	return &detacher, nil
 }
 
-func (plugin *TestPlugin) CanAttach(spec *volume.Spec) bool {
-	return true
+func (plugin *TestPlugin) CanAttach(spec *volume.Spec) (bool, error) {
+	return true, nil
 }
 
 func (plugin *TestPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

pkg/volume/awsebs/attacher.go

@@ -277,8 +277,8 @@ func (detacher *awsElasticBlockStoreDetacher) UnmountDevice(deviceMountPath stri
 	return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false)
 }
 
-func (plugin *awsElasticBlockStorePlugin) CanAttach(spec *volume.Spec) bool {
-	return true
+func (plugin *awsElasticBlockStorePlugin) CanAttach(spec *volume.Spec) (bool, error) {
+	return true, nil
 }
 
 func (plugin *awsElasticBlockStorePlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

pkg/volume/azure_dd/azure_dd.go

@@ -238,8 +238,8 @@ func (plugin *azureDataDiskPlugin) NewDetacher() (volume.Detacher, error) {
 	}, nil
 }
 
-func (plugin *azureDataDiskPlugin) CanAttach(spec *volume.Spec) bool {
-	return true
+func (plugin *azureDataDiskPlugin) CanAttach(spec *volume.Spec) (bool, error) {
+	return true, nil
 }
 
 func (plugin *azureDataDiskPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

pkg/volume/cinder/attacher.go

@@ -406,8 +406,8 @@ func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string) error
 	return mount.CleanupMountPoint(deviceMountPath, detacher.mounter, false)
 }
 
-func (plugin *cinderPlugin) CanAttach(spec *volume.Spec) bool {
-	return true
+func (plugin *cinderPlugin) CanAttach(spec *volume.Spec) (bool, error) {
+	return true, nil
 }
 
 func (plugin *cinderPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

pkg/volume/csi/csi_attacher_test.go

@@ -348,7 +348,10 @@ func TestAttacherWithCSIDriver(t *testing.T) {
 			csiAttacher := attacher.(*csiAttacher)
 			spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false)
 
-			pluginCanAttach := plug.CanAttach(spec)
+			pluginCanAttach, err := plug.CanAttach(spec)
+			if err != nil {
+				t.Fatalf("attacher.CanAttach failed: %s", err)
+			}
 			if pluginCanAttach != test.expectVolumeAttachment {
 				t.Errorf("attacher.CanAttach does not match expected attachment status %t", test.expectVolumeAttachment)
 			}
@@ -429,7 +432,10 @@ func TestAttacherWaitForVolumeAttachmentWithCSIDriver(t *testing.T) {
 			csiAttacher := attacher.(*csiAttacher)
 			spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, test.driver, "test-vol"), false)
 
-			pluginCanAttach := plug.CanAttach(spec)
+			pluginCanAttach, err := plug.CanAttach(spec)
+			if err != nil {
+				t.Fatalf("plugin.CanAttach test failed: %s", err)
+			}
 			if !pluginCanAttach {
 				t.Log("plugin is not attachable")
 				return

pkg/volume/csi/csi_plugin.go

@@ -581,34 +581,30 @@ func (p *csiPlugin) NewDetacher() (volume.Detacher, error) {
 	}, nil
 }
 
-// TODO change CanAttach to return error to propagate ability
-// to support Attachment or an error - see https://github.com/kubernetes/kubernetes/issues/74810
-func (p *csiPlugin) CanAttach(spec *volume.Spec) bool {
+func (p *csiPlugin) CanAttach(spec *volume.Spec) (bool, error) {
 	driverMode, err := p.getDriverMode(spec)
 	if err != nil {
-		return false
+		return false, err
 	}
 
 	if driverMode == ephemeralDriverMode {
-		klog.V(4).Info(log("driver ephemeral mode detected for spec %v", spec.Name))
-		return false
+		klog.V(5).Info(log("plugin.CanAttach = false, ephemeral mode detected for spec %v", spec.Name()))
+		return false, nil
 	}
 
 	pvSrc, err := getCSISourceFromSpec(spec)
 	if err != nil {
-		klog.Error(log("plugin.CanAttach failed to get info from spec: %s", err))
-		return false
+		return false, err
 	}
 
 	driverName := pvSrc.Driver
 
 	skipAttach, err := p.skipAttach(driverName)
 	if err != nil {
-		klog.Error(log("plugin.CanAttach error when calling plugin.skipAttach for driver %s: %s", driverName, err))
-		return false
+		return false, err
 	}
 
-	return !skipAttach
+	return !skipAttach, nil
 }
 
 // TODO (#75352) add proper logic to determine device moutability by inspecting the spec.

pkg/volume/csi/csi_plugin_test.go

@@ -880,6 +880,7 @@ func TestPluginCanAttach(t *testing.T) {
 		driverName string
 		spec       *volume.Spec
 		canAttach  bool
+		shouldFail bool
 	}{
 		{
 			name:       "non-attachable inline",
@@ -893,6 +894,19 @@ func TestPluginCanAttach(t *testing.T) {
 			spec:       volume.NewSpecFromPersistentVolume(makeTestPV("test-vol", 20, "attachable-pv", testVol), true),
 			canAttach:  true,
 		},
+		{
+			name:       "incomplete spec",
+			driverName: "attachable-pv",
+			spec:       &volume.Spec{ReadOnly: true},
+			canAttach:  false,
+			shouldFail: true,
+		},
+		{
+			name:       "nil spec",
+			driverName: "attachable-pv",
+			canAttach:  false,
+			shouldFail: true,
+		},
 	}
 
 	for _, test := range tests {
@@ -902,10 +916,80 @@ func TestPluginCanAttach(t *testing.T) {
 			plug, tmpDir := newTestPlugin(t, fakeCSIClient)
 			defer os.RemoveAll(tmpDir)
 
-			pluginCanAttach := plug.CanAttach(test.spec)
+			pluginCanAttach, err := plug.CanAttach(test.spec)
+			if err != nil && !test.shouldFail {
+				t.Fatalf("unexected plugin.CanAttach error: %s", err)
+			}
 			if pluginCanAttach != test.canAttach {
 				t.Fatalf("expecting plugin.CanAttach %t got %t", test.canAttach, pluginCanAttach)
-				return
+			}
+		})
+	}
+}
+
+func TestPluginFindAttachablePlugin(t *testing.T) {
+	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIInlineVolume, true)()
+	tests := []struct {
+		name       string
+		driverName string
+		spec       *volume.Spec
+		canAttach  bool
+		shouldFail bool
+	}{
+		{
+			name:       "non-attachable inline",
+			driverName: "attachable-inline",
+			spec:       volume.NewSpecFromVolume(makeTestVol("test-vol", "attachable-inline")),
+			canAttach:  false,
+		},
+		{
+			name:       "attachable PV",
+			driverName: "attachable-pv",
+			spec:       volume.NewSpecFromPersistentVolume(makeTestPV("test-vol", 20, "attachable-pv", testVol), true),
+			canAttach:  true,
+		},
+		{
+			name:       "incomplete spec",
+			driverName: "attachable-pv",
+			spec:       &volume.Spec{ReadOnly: true},
+			canAttach:  false,
+			shouldFail: true,
+		},
+		{
+			name:       "nil spec",
+			driverName: "attachable-pv",
+			canAttach:  false,
+			shouldFail: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			tmpDir, err := utiltesting.MkTmpdir("csi-test")
+			if err != nil {
+				t.Fatalf("can't create temp dir: %v", err)
+			}
+			defer os.RemoveAll(tmpDir)
+
+			client := fakeclient.NewSimpleClientset(getCSIDriver(test.driverName, nil, &test.canAttach))
+			factory := informers.NewSharedInformerFactory(client, csiResyncPeriod)
+			host := volumetest.NewFakeVolumeHostWithCSINodeName(
+				tmpDir,
+				client,
+				nil,
+				"fakeNode",
+				factory.Storage().V1beta1().CSIDrivers().Lister(),
+			)
+
+			plugMgr := &volume.VolumePluginMgr{}
+			plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, host)
+
+			plugin, err := plugMgr.FindAttachablePluginBySpec(test.spec)
+			if err != nil && !test.shouldFail {
+				t.Fatalf("unexected error calling pluginMgr.FindAttachablePluginBySpec: %s", err)
+			}
+			if (plugin != nil) != test.canAttach {
+				t.Fatal("expecting attachable plugin, but got nil")
 			}
 		})
 	}

pkg/volume/fc/attacher.go

@@ -175,8 +175,8 @@ func (detacher *fcDetacher) UnmountDevice(deviceMountPath string) error {
 	return nil
 }
 
-func (plugin *fcPlugin) CanAttach(spec *volume.Spec) bool {
-	return true
+func (plugin *fcPlugin) CanAttach(spec *volume.Spec) (bool, error) {
+	return true, nil
 }
 
 func (plugin *fcPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

pkg/volume/flexvolume/plugin.go

@@ -256,8 +256,8 @@ func (plugin *flexVolumeAttachablePlugin) NewDeviceUnmounter() (volume.DeviceUnm
 	return plugin.NewDetacher()
 }
 
-func (plugin *flexVolumeAttachablePlugin) CanAttach(spec *volume.Spec) bool {
-	return true
+func (plugin *flexVolumeAttachablePlugin) CanAttach(spec *volume.Spec) (bool, error) {
+	return true, nil
 }
 
 func (plugin *flexVolumeAttachablePlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {

pkg/volume/gcepd/attacher.go

@@ -350,8 +350,8 @@ func (detacher *gcePersistentDiskDetacher) UnmountDevice(deviceMountPath string)
 	return mount.CleanupMountPoint(deviceMountPath, detacher.host.GetMounter(gcePersistentDiskPluginName), false)
 }
 
-func (plugin *gcePersistentDiskPlugin) CanAttach(spec *volume.Spec) bool {
-	return true
+func (plugin *gcePersistentDiskPlugin) CanAttach(spec *volume.Spec) (bool, error) {
+	return true, nil
 }
 
 func (plugin *gcePersistentDiskPlugin) CanDeviceMount(spec *volume.Spec) (bool, error) {