Fix AWS block volume reconstruction to be like file

pkg/volume/awsebs/aws_ebs.go

@@ -263,41 +263,13 @@ func (plugin *awsElasticBlockStorePlugin) ConstructVolumeSpec(volName, mountPath
 	if err != nil {
 		return nil, err
 	}
-	// This is a workaround to fix the issue in converting aws volume id from globalPDPath
+	volumeID, err = formatVolumeID(volumeID)
-	// There are three aws volume id formats and their volumeID from GetDeviceNameFromMount() are:
+	if err != nil {
-	// aws:///vol-1234 (aws/vol-1234)
+		return nil, fmt.Errorf("failed to get AWS volume id from mount path %q: %v", mountPath, err)
-	// aws://us-east-1/vol-1234 (aws/us-east-1/vol-1234)
-	// vol-1234 (vol-1234)
-	// This code is for converting volume id to aws style volume id for the first two cases.
-	sourceName := volumeID
-	if strings.HasPrefix(volumeID, "aws/") {
-		names := strings.Split(volumeID, "/")
-		length := len(names)
-		if length < 2 || length > 3 {
-			return nil, fmt.Errorf("Failed to get AWS volume id from mount path %q: invalid volume name format %q", mountPath, volumeID)
-		}
-		volName := names[length-1]
-		if !strings.HasPrefix(volName, "vol-") {
-			return nil, fmt.Errorf("Invalid volume name format for AWS volume (%q) retrieved from mount path %q", volName, mountPath)
-		}
-		if length == 2 {
-			sourceName = awsURLNamePrefix + "" + "/" + volName // empty zone label
-		}
-		if length == 3 {
-			sourceName = awsURLNamePrefix + names[1] + "/" + volName // names[1] is the zone label
-		}
-		klog.V(4).Infof("Convert aws volume name from %q to %q ", volumeID, sourceName)
 	}
 
-	awsVolume := &v1.Volume{
+	file := v1.PersistentVolumeFilesystem
-		Name: volName,
+	return newAWSVolumeSpec(volName, volumeID, file), nil
-		VolumeSource: v1.VolumeSource{
-			AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
-				VolumeID: sourceName,
-			},
-		},
-	}
-	return volume.NewSpecFromVolume(awsVolume), nil
 }
 
 func (plugin *awsElasticBlockStorePlugin) RequiresFSResize() bool {

pkg/volume/awsebs/aws_ebs_block.go

@@ -25,7 +25,6 @@ import (
 	"strings"
 
 	v1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog"
 	"k8s.io/kubernetes/pkg/util/mount"
@@ -52,37 +51,27 @@ func (plugin *awsElasticBlockStorePlugin) ConstructBlockVolumeSpec(podUID types.
 		return nil, fmt.Errorf("failed to get volume plugin information from globalMapPathUUID: %v", globalMapPathUUID)
 	}
 
-	return getVolumeSpecFromGlobalMapPath(volumeName, globalMapPath)
+	return plugin.getVolumeSpecFromGlobalMapPath(volumeName, globalMapPath)
 }
 
-func getVolumeSpecFromGlobalMapPath(volumeName string, globalMapPath string) (*volume.Spec, error) {
+func (plugin *awsElasticBlockStorePlugin) getVolumeSpecFromGlobalMapPath(volumeName string, globalMapPath string) (*volume.Spec, error) {
 	// Get volume spec information from globalMapPath
 	// globalMapPath example:
 	//   plugins/kubernetes.io/{PluginName}/{DefaultKubeletVolumeDevicesDirName}/{volumeID}
 	//   plugins/kubernetes.io/aws-ebs/volumeDevices/vol-XXXXXX
-	vID := filepath.Base(globalMapPath)
+	pluginDir := plugin.host.GetVolumeDevicePluginDir(awsElasticBlockStorePluginName)
-	if len(vID) <= 1 {
+	if !strings.HasPrefix(globalMapPath, pluginDir) {
-		return nil, fmt.Errorf("failed to get volumeID from global path=%s", globalMapPath)
+		return nil, fmt.Errorf("volume symlink %s is not in global plugin directory", globalMapPath)
 	}
-	if !strings.Contains(vID, "vol-") {
+	fullVolumeID := strings.TrimPrefix(globalMapPath, pluginDir) // /vol-XXXXXX
-		return nil, fmt.Errorf("failed to get volumeID from global path=%s, invalid volumeID format = %s", globalMapPath, vID)
+	fullVolumeID = strings.TrimLeft(fullVolumeID, "/")           // vol-XXXXXX
-	}
+	vID, err := formatVolumeID(fullVolumeID)
-	block := v1.PersistentVolumeBlock
+	if err != nil {
-	awsVolume := &v1.PersistentVolume{
+		return nil, fmt.Errorf("failed to get AWS volume id from map path %q: %v", globalMapPath, err)
-		ObjectMeta: metav1.ObjectMeta{
-			Name: volumeName,
-		},
-		Spec: v1.PersistentVolumeSpec{
-			PersistentVolumeSource: v1.PersistentVolumeSource{
-				AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
-					VolumeID: vID,
-				},
-			},
-			VolumeMode: &block,
-		},
 	}
 
-	return volume.NewSpecFromPersistentVolume(awsVolume, true), nil
+	block := v1.PersistentVolumeBlock
+	return newAWSVolumeSpec(volumeName, vID, block), nil
 }
 
 // NewBlockVolumeMapper creates a new volume.BlockVolumeMapper from an API specification.

pkg/volume/awsebs/aws_ebs_block_test.go

@@ -51,14 +51,22 @@ func TestGetVolumeSpecFromGlobalMapPath(t *testing.T) {
 
 	expectedGlobalPath := filepath.Join(tmpVDir, testGlobalPath)
 
+	plugMgr := volume.VolumePluginMgr{}
+	plugMgr.InitPlugins(ProbeVolumePlugins(), nil /* prober */, volumetest.NewFakeVolumeHost(tmpVDir, nil, nil))
+	plug, err := plugMgr.FindMapperPluginByName(awsElasticBlockStorePluginName)
+	if err != nil {
+		os.RemoveAll(tmpVDir)
+		t.Fatalf("Can't find the plugin by name: %q", awsElasticBlockStorePluginName)
+	}
+
 	//Bad Path
-	badspec, err := getVolumeSpecFromGlobalMapPath("", "")
+	badspec, err := plug.(*awsElasticBlockStorePlugin).getVolumeSpecFromGlobalMapPath("", "")
 	if badspec != nil || err == nil {
 		t.Fatalf("Expected not to get spec from GlobalMapPath but did")
 	}
 
 	// Good Path
-	spec, err := getVolumeSpecFromGlobalMapPath("myVolume", expectedGlobalPath)
+	spec, err := plug.(*awsElasticBlockStorePlugin).getVolumeSpecFromGlobalMapPath("myVolume", expectedGlobalPath)
 	if spec == nil || err != nil {
 		t.Fatalf("Failed to get spec from GlobalMapPath: %v", err)
 	}

pkg/volume/awsebs/aws_ebs_test.go

@@ -414,3 +414,36 @@ func TestGetCandidateZone(t *testing.T) {
 		assert.Equal(t, test.expectedZones, zones)
 	}
 }
+
+func TestFormatVolumeID(t *testing.T) {
+	tests := []struct {
+		volumeIDFromPath string
+		expectedVolumeID string
+	}{
+		{
+			"aws/vol-1234",
+			"aws:///vol-1234",
+		},
+		{
+			"aws:/vol-1234",
+			"aws:///vol-1234",
+		},
+		{
+			"aws/us-east-1/vol-1234",
+			"aws://us-east-1/vol-1234",
+		},
+		{
+			"aws:/us-east-1/vol-1234",
+			"aws://us-east-1/vol-1234",
+		},
+		{
+			"vol-1234",
+			"vol-1234",
+		},
+	}
+	for _, test := range tests {
+		volumeID, err := formatVolumeID(test.volumeIDFromPath)
+		assert.Nil(t, err)
+		assert.Equal(t, test.expectedVolumeID, volumeID, test.volumeIDFromPath)
+	}
+}

pkg/volume/awsebs/aws_util.go

@@ -30,6 +30,7 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	cloudprovider "k8s.io/cloud-provider"
 	volumehelpers "k8s.io/cloud-provider/volume/helpers"
@@ -287,3 +288,50 @@ func findNvmeVolume(findName string) (device string, err error) {
 
 	return resolved, nil
 }
+
+func formatVolumeID(volumeID string) (string, error) {
+	// This is a workaround to fix the issue in converting aws volume id from globalPDPath and globalMapPath
+	// There are three formats for AWSEBSVolumeSource.VolumeID and they are stored on disk in paths like so:
+	// VolumeID									mountPath								mapPath
+	// aws:///vol-1234					aws/vol-1234						aws:/vol-1234
+	// aws://us-east-1/vol-1234 aws/us-east-1/vol-1234  aws:/us-east-1/vol-1234
+	// vol-1234									vol-1234								vol-1234
+	// This code is for converting volume ids from paths back to AWS style VolumeIDs
+	sourceName := volumeID
+	if strings.HasPrefix(volumeID, "aws/") || strings.HasPrefix(volumeID, "aws:/") {
+		names := strings.Split(volumeID, "/")
+		length := len(names)
+		if length < 2 || length > 3 {
+			return "", fmt.Errorf("invalid volume name format %q", volumeID)
+		}
+		volName := names[length-1]
+		if !strings.HasPrefix(volName, "vol-") {
+			return "", fmt.Errorf("Invalid volume name format for AWS volume (%q)", volName)
+		}
+		if length == 2 {
+			sourceName = awsURLNamePrefix + "" + "/" + volName // empty zone label
+		}
+		if length == 3 {
+			sourceName = awsURLNamePrefix + names[1] + "/" + volName // names[1] is the zone label
+		}
+		klog.V(4).Infof("Convert aws volume name from %q to %q ", volumeID, sourceName)
+	}
+	return sourceName, nil
+}
+
+func newAWSVolumeSpec(volumeName, volumeID string, mode v1.PersistentVolumeMode) *volume.Spec {
+	awsVolume := &v1.PersistentVolume{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: volumeName,
+		},
+		Spec: v1.PersistentVolumeSpec{
+			PersistentVolumeSource: v1.PersistentVolumeSource{
+				AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
+					VolumeID: volumeID,
+				},
+			},
+			VolumeMode: &mode,
+		},
+	}
+	return volume.NewSpecFromPersistentVolume(awsVolume, false)
+}