Merge pull request #35883 from justinsb/aws_strong_volumetype

Automatic merge from submit-queue AWS: strong-typing for k8s vs aws volume ids
2016-11-05 02:29:17 -07:00
parent 82ecd1619c 3cdbfc98af
commit f4738ff575
12 changed files with 212 additions and 138 deletions
--- a/pkg/cloudprovider/providers/aws/BUILD
+++ b/pkg/cloudprovider/providers/aws/BUILD
@@ -21,6 +21,7 @@ go_library(
        "log_handler.go",
        "retry_handler.go",
        "sets_ippermissions.go",
+        "volumes.go",
    ],
    tags = ["automanaged"],
    deps = [
--- a/pkg/cloudprovider/providers/aws/aws.go
+++ b/pkg/cloudprovider/providers/aws/aws.go
@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"io"
 	"net"
-	"net/url"
 	"regexp"
 	"strconv"
 	"strings"
@@ -299,31 +298,31 @@ type Volumes interface {
 	// Attach the disk to the node with the specified NodeName
 	// nodeName can be empty to mean "the instance on which we are running"
 	// Returns the device (e.g. /dev/xvdf) where we attached the volume
-	AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error)
+	AttachDisk(diskName KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error)
 	// Detach the disk from the node with the specified NodeName
 	// nodeName can be empty to mean "the instance on which we are running"
 	// Returns the device where the volume was attached
-	DetachDisk(diskName string, nodeName types.NodeName) (string, error)
+	DetachDisk(diskName KubernetesVolumeID, nodeName types.NodeName) (string, error)

 	// Create a volume with the specified options
-	CreateDisk(volumeOptions *VolumeOptions) (volumeName string, err error)
+	CreateDisk(volumeOptions *VolumeOptions) (volumeName KubernetesVolumeID, err error)
 	// Delete the specified volume
 	// Returns true iff the volume was deleted
 	// If the was not found, returns (false, nil)
-	DeleteDisk(volumeName string) (bool, error)
+	DeleteDisk(volumeName KubernetesVolumeID) (bool, error)

 	// Get labels to apply to volume on creation
-	GetVolumeLabels(volumeName string) (map[string]string, error)
+	GetVolumeLabels(volumeName KubernetesVolumeID) (map[string]string, error)

 	// Get volume's disk path from volume name
 	// return the device path where the volume is attached
-	GetDiskPath(volumeName string) (string, error)
+	GetDiskPath(volumeName KubernetesVolumeID) (string, error)

 	// Check if the volume is already attached to the node with the specified NodeName
-	DiskIsAttached(diskName string, nodeName types.NodeName) (bool, error)
+	DiskIsAttached(diskName KubernetesVolumeID, nodeName types.NodeName) (bool, error)

 	// Check if a list of volumes are attached to the node with the specified NodeName
-	DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error)
+	DisksAreAttached(diskNames []KubernetesVolumeID, nodeName types.NodeName) (map[KubernetesVolumeID]bool, error)
 }

 // InstanceGroups is an interface for managing cloud-managed instance groups / autoscaling instance groups
@@ -365,7 +364,7 @@ type Cloud struct {
 	// attached, to avoid a race condition where we assign a device mapping
 	// and then get a second request before we attach the volume
 	attachingMutex sync.Mutex
-	attaching      map[types.NodeName]map[mountDevice]string
+	attaching      map[types.NodeName]map[mountDevice]awsVolumeID
 }

 var _ Volumes = &Cloud{}
@@ -797,7 +796,7 @@ func newAWSCloud(config io.Reader, awsServices Services) (*Cloud, error) {
 		cfg:      cfg,
 		region:   regionName,

-		attaching: make(map[types.NodeName]map[mountDevice]string),
+		attaching: make(map[types.NodeName]map[mountDevice]awsVolumeID),
 	}

 	selfAWSInstance, err := awsCloud.buildSelfAWSInstance()
@@ -1168,7 +1167,7 @@ func (i *awsInstance) describeInstance() (*ec2.Instance, error) {
 // Gets the mountDevice already assigned to the volume, or assigns an unused mountDevice.
 // If the volume is already assigned, this will return the existing mountDevice with alreadyAttached=true.
 // Otherwise the mountDevice is assigned by finding the first available mountDevice, and it is returned with alreadyAttached=false.
-func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (assigned mountDevice, alreadyAttached bool, err error) {
+func (c *Cloud) getMountDevice(i *awsInstance, volumeID awsVolumeID, assign bool) (assigned mountDevice, alreadyAttached bool, err error) {
 	instanceType := i.getInstanceType()
 	if instanceType == nil {
 		return "", false, fmt.Errorf("could not get instance type for instance: %s", i.awsID)
@@ -1178,7 +1177,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as
 	if err != nil {
 		return "", false, err
 	}
-	deviceMappings := map[mountDevice]string{}
+	deviceMappings := map[mountDevice]awsVolumeID{}
 	for _, blockDevice := range info.BlockDeviceMappings {
 		name := aws.StringValue(blockDevice.DeviceName)
 		if strings.HasPrefix(name, "/dev/sd") {
@@ -1190,7 +1189,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as
 		if len(name) < 1 || len(name) > 2 {
 			glog.Warningf("Unexpected EBS DeviceName: %q", aws.StringValue(blockDevice.DeviceName))
 		}
-		deviceMappings[mountDevice(name)] = aws.StringValue(blockDevice.Ebs.VolumeId)
+		deviceMappings[mountDevice(name)] = awsVolumeID(aws.StringValue(blockDevice.Ebs.VolumeId))
 	}

 	// We lock to prevent concurrent mounts from conflicting
@@ -1236,7 +1235,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as

 	attaching := c.attaching[i.nodeName]
 	if attaching == nil {
-		attaching = make(map[mountDevice]string)
+		attaching = make(map[mountDevice]awsVolumeID)
 		c.attaching[i.nodeName] = attaching
 	}
 	attaching[chosen] = volumeID
@@ -1247,7 +1246,7 @@ func (c *Cloud) getMountDevice(i *awsInstance, volumeID string, assign bool) (as

 // endAttaching removes the entry from the "attachments in progress" map
 // It returns true if it was found (and removed), false otherwise
-func (c *Cloud) endAttaching(i *awsInstance, volumeID string, mountDevice mountDevice) bool {
+func (c *Cloud) endAttaching(i *awsInstance, volumeID awsVolumeID, mountDevice mountDevice) bool {
 	c.attachingMutex.Lock()
 	defer c.attachingMutex.Unlock()

@@ -1272,44 +1271,16 @@ type awsDisk struct {
 	ec2 EC2

 	// Name in k8s
-	name string
+	name KubernetesVolumeID
 	// id in AWS
-	awsID string
+	awsID awsVolumeID
 }

-func newAWSDisk(aws *Cloud, name string) (*awsDisk, error) {
-	// name looks like aws://availability-zone/id
-
-	// The original idea of the URL-style name was to put the AZ into the
-	// host, so we could find the AZ immediately from the name without
-	// querying the API.  But it turns out we don't actually need it for
-	// multi-AZ clusters, as we put the AZ into the labels on the PV instead.
-	// However, if in future we want to support multi-AZ cluster
-	// volume-awareness without using PersistentVolumes, we likely will
-	// want the AZ in the host.
-
-	if !strings.HasPrefix(name, "aws://") {
-		name = "aws://" + "" + "/" + name
-	}
-	url, err := url.Parse(name)
+func newAWSDisk(aws *Cloud, name KubernetesVolumeID) (*awsDisk, error) {
+	awsID, err := name.mapToAWSVolumeID()
 	if err != nil {
-		// TODO: Maybe we should pass a URL into the Volume functions
-		return nil, fmt.Errorf("Invalid disk name (%s): %v", name, err)
+		return nil, err
 	}
-	if url.Scheme != "aws" {
-		return nil, fmt.Errorf("Invalid scheme for AWS volume (%s)", name)
-	}
-
-	awsID := url.Path
-	if len(awsID) > 1 && awsID[0] == '/' {
-		awsID = awsID[1:]
-	}
-
-	// TODO: Regex match?
-	if strings.Contains(awsID, "/") || !strings.HasPrefix(awsID, "vol-") {
-		return nil, fmt.Errorf("Invalid format for AWS volume (%s)", name)
-	}
-
 	disk := &awsDisk{ec2: aws.ec2, name: name, awsID: awsID}
 	return disk, nil
 }
@@ -1319,7 +1290,7 @@ func (d *awsDisk) describeVolume() (*ec2.Volume, error) {
 	volumeID := d.awsID

 	request := &ec2.DescribeVolumesInput{
-		VolumeIds: []*string{&volumeID},
+		VolumeIds: []*string{volumeID.awsString()},
 	}

 	volumes, err := d.ec2.DescribeVolumes(request)
@@ -1400,7 +1371,7 @@ func (d *awsDisk) waitForAttachmentStatus(status string) (*ec2.VolumeAttachment,

 // Deletes the EBS disk
 func (d *awsDisk) deleteVolume() (bool, error) {
-	request := &ec2.DeleteVolumeInput{VolumeId: aws.String(d.awsID)}
+	request := &ec2.DeleteVolumeInput{VolumeId: d.awsID.awsString()}
 	_, err := d.ec2.DeleteVolume(request)
 	if err != nil {
 		if awsError, ok := err.(awserr.Error); ok {
@@ -1460,7 +1431,7 @@ func (c *Cloud) getAwsInstance(nodeName types.NodeName) (*awsInstance, error) {
 }

 // AttachDisk implements Volumes.AttachDisk
-func (c *Cloud) AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error) {
+func (c *Cloud) AttachDisk(diskName KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error) {
 	disk, err := newAWSDisk(c, diskName)
 	if err != nil {
 		return "", err
@@ -1508,7 +1479,7 @@ func (c *Cloud) AttachDisk(diskName string, nodeName types.NodeName, readOnly bo
 		request := &ec2.AttachVolumeInput{
 			Device:     aws.String(ec2Device),
 			InstanceId: aws.String(awsInstance.awsID),
-			VolumeId:   aws.String(disk.awsID),
+			VolumeId:   disk.awsID.awsString(),
 		}

 		attachResponse, err := c.ec2.AttachVolume(request)
@@ -1547,7 +1518,7 @@ func (c *Cloud) AttachDisk(diskName string, nodeName types.NodeName, readOnly bo
 }

 // DetachDisk implements Volumes.DetachDisk
-func (c *Cloud) DetachDisk(diskName string, nodeName types.NodeName) (string, error) {
+func (c *Cloud) DetachDisk(diskName KubernetesVolumeID, nodeName types.NodeName) (string, error) {
 	disk, err := newAWSDisk(c, diskName)
 	if err != nil {
 		return "", err
@@ -1579,7 +1550,7 @@ func (c *Cloud) DetachDisk(diskName string, nodeName types.NodeName) (string, er

 	request := ec2.DetachVolumeInput{
 		InstanceId: &awsInstance.awsID,
-		VolumeId:   &disk.awsID,
+		VolumeId:   disk.awsID.awsString(),
 	}

 	response, err := c.ec2.DetachVolume(&request)
@@ -1610,7 +1581,7 @@ func (c *Cloud) DetachDisk(diskName string, nodeName types.NodeName) (string, er
 }

 // CreateDisk implements Volumes.CreateDisk
-func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) {
+func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (KubernetesVolumeID, error) {
 	allZones, err := c.getAllZones()
 	if err != nil {
 		return "", fmt.Errorf("error querying for all zones: %v", err)
@@ -1668,10 +1639,11 @@ func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) {
 		return "", err
 	}

-	az := orEmpty(response.AvailabilityZone)
-	awsID := orEmpty(response.VolumeId)
-
-	volumeName := "aws://" + az + "/" + awsID
+	awsID := awsVolumeID(aws.StringValue(response.VolumeId))
+	if awsID == "" {
+		return "", fmt.Errorf("VolumeID was not returned by CreateVolume")
+	}
+	volumeName := KubernetesVolumeID("aws://" + aws.StringValue(response.AvailabilityZone) + "/" + string(awsID))

 	// apply tags
 	tags := make(map[string]string)
@@ -1684,7 +1656,7 @@ func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) {
 	}

 	if len(tags) != 0 {
-		if err := c.createTags(awsID, tags); err != nil {
+		if err := c.createTags(string(awsID), tags); err != nil {
 			// delete the volume and hope it succeeds
 			_, delerr := c.DeleteDisk(volumeName)
 			if delerr != nil {
@@ -1698,7 +1670,7 @@ func (c *Cloud) CreateDisk(volumeOptions *VolumeOptions) (string, error) {
 }

 // DeleteDisk implements Volumes.DeleteDisk
-func (c *Cloud) DeleteDisk(volumeName string) (bool, error) {
+func (c *Cloud) DeleteDisk(volumeName KubernetesVolumeID) (bool, error) {
 	awsDisk, err := newAWSDisk(c, volumeName)
 	if err != nil {
 		return false, err
@@ -1707,7 +1679,7 @@ func (c *Cloud) DeleteDisk(volumeName string) (bool, error) {
 }

 // GetVolumeLabels implements Volumes.GetVolumeLabels
-func (c *Cloud) GetVolumeLabels(volumeName string) (map[string]string, error) {
+func (c *Cloud) GetVolumeLabels(volumeName KubernetesVolumeID) (map[string]string, error) {
 	awsDisk, err := newAWSDisk(c, volumeName)
 	if err != nil {
 		return nil, err
@@ -1733,7 +1705,7 @@ func (c *Cloud) GetVolumeLabels(volumeName string) (map[string]string, error) {
 }

 // GetDiskPath implements Volumes.GetDiskPath
-func (c *Cloud) GetDiskPath(volumeName string) (string, error) {
+func (c *Cloud) GetDiskPath(volumeName KubernetesVolumeID) (string, error) {
 	awsDisk, err := newAWSDisk(c, volumeName)
 	if err != nil {
 		return "", err
@@ -1749,7 +1721,7 @@ func (c *Cloud) GetDiskPath(volumeName string) (string, error) {
 }

 // DiskIsAttached implements Volumes.DiskIsAttached
-func (c *Cloud) DiskIsAttached(diskName string, nodeName types.NodeName) (bool, error) {
+func (c *Cloud) DiskIsAttached(diskName KubernetesVolumeID, nodeName types.NodeName) (bool, error) {
 	awsInstance, err := c.getAwsInstance(nodeName)
 	if err != nil {
 		if err == cloudprovider.InstanceNotFound {
@@ -1764,22 +1736,33 @@ func (c *Cloud) DiskIsAttached(diskName string, nodeName types.NodeName) (bool,
 		return false, err
 	}

+	diskID, err := diskName.mapToAWSVolumeID()
+	if err != nil {
+		return false, fmt.Errorf("error mapping volume spec %q to aws id: %v", diskName, err)
+	}
+
 	info, err := awsInstance.describeInstance()
 	if err != nil {
 		return false, err
 	}
 	for _, blockDevice := range info.BlockDeviceMappings {
-		name := aws.StringValue(blockDevice.Ebs.VolumeId)
-		if name == diskName {
+		id := awsVolumeID(aws.StringValue(blockDevice.Ebs.VolumeId))
+		if id == diskID {
 			return true, nil
 		}
 	}
 	return false, nil
 }

-func (c *Cloud) DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error) {
-	attached := make(map[string]bool)
+func (c *Cloud) DisksAreAttached(diskNames []KubernetesVolumeID, nodeName types.NodeName) (map[KubernetesVolumeID]bool, error) {
+	idToDiskName := make(map[awsVolumeID]KubernetesVolumeID)
+	attached := make(map[KubernetesVolumeID]bool)
 	for _, diskName := range diskNames {
+		volumeID, err := diskName.mapToAWSVolumeID()
+		if err != nil {
+			return nil, fmt.Errorf("error mapping volume spec %q to aws id: %v", diskName, err)
+		}
+		idToDiskName[volumeID] = diskName
 		attached[diskName] = false
 	}
 	awsInstance, err := c.getAwsInstance(nodeName)
@@ -1800,12 +1783,11 @@ func (c *Cloud) DisksAreAttached(diskNames []string, nodeName types.NodeName) (m
 		return attached, err
 	}
 	for _, blockDevice := range info.BlockDeviceMappings {
-		volumeId := aws.StringValue(blockDevice.Ebs.VolumeId)
-		for _, diskName := range diskNames {
-			if volumeId == diskName {
-				// Disk is still attached to node
-				attached[diskName] = true
-			}
+		volumeID := awsVolumeID(aws.StringValue(blockDevice.Ebs.VolumeId))
+		diskName, found := idToDiskName[volumeID]
+		if found {
+			// Disk is still attached to node
+			attached[diskName] = true
 		}
 	}

--- a/pkg/cloudprovider/providers/aws/aws_test.go
+++ b/pkg/cloudprovider/providers/aws/aws_test.go
@@ -1163,16 +1163,16 @@ func TestGetVolumeLabels(t *testing.T) {
 	awsServices := NewFakeAWSServices()
 	c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
 	assert.Nil(t, err, "Error building aws cloud: %v", err)
-	volumeId := aws.String("vol-VolumeId")
-	expectedVolumeRequest := &ec2.DescribeVolumesInput{VolumeIds: []*string{volumeId}}
+	volumeId := awsVolumeID("vol-VolumeId")
+	expectedVolumeRequest := &ec2.DescribeVolumesInput{VolumeIds: []*string{volumeId.awsString()}}
 	awsServices.ec2.On("DescribeVolumes", expectedVolumeRequest).Return([]*ec2.Volume{
 		{
-			VolumeId:         volumeId,
+			VolumeId:         volumeId.awsString(),
 			AvailabilityZone: aws.String("us-east-1a"),
 		},
 	})

-	labels, err := c.GetVolumeLabels(*volumeId)
+	labels, err := c.GetVolumeLabels(KubernetesVolumeID("aws:///" + string(volumeId)))

 	assert.Nil(t, err, "Error creating Volume %v", err)
 	assert.Equal(t, map[string]string{
--- a/pkg/cloudprovider/providers/aws/volumes.go
+++ b/pkg/cloudprovider/providers/aws/volumes.go
@@ -0,0 +1,83 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package aws
+
+import (
+	"fmt"
+	"github.com/aws/aws-sdk-go/aws"
+	"net/url"
+	"strings"
+)
+
+// awsVolumeID represents the ID of the volume in the AWS API, e.g. vol-12345678a
+// The "traditional" format is "vol-12345678"
+// A new longer format is also being introduced: "vol-12345678abcdef01"
+// We should not assume anything about the length or format, though it seems
+// reasonable to assume that volumes will continue to start with "vol-".
+type awsVolumeID string
+
+func (i awsVolumeID) awsString() *string {
+	return aws.String(string(i))
+}
+
+// KubernetesVolumeID represents the id for a volume in the kubernetes API;
+// a few forms are recognized:
+//  * aws://<zone>/<awsVolumeId>
+//  * aws:///<awsVolumeId>
+//  * <awsVolumeId>
+type KubernetesVolumeID string
+
+// mapToAWSVolumeID extracts the awsVolumeID from the KubernetesVolumeID
+func (name KubernetesVolumeID) mapToAWSVolumeID() (awsVolumeID, error) {
+	// name looks like aws://availability-zone/awsVolumeId
+
+	// The original idea of the URL-style name was to put the AZ into the
+	// host, so we could find the AZ immediately from the name without
+	// querying the API.  But it turns out we don't actually need it for
+	// multi-AZ clusters, as we put the AZ into the labels on the PV instead.
+	// However, if in future we want to support multi-AZ cluster
+	// volume-awareness without using PersistentVolumes, we likely will
+	// want the AZ in the host.
+
+	s := string(name)
+
+	if !strings.HasPrefix(s, "aws://") {
+		// Assume a bare aws volume id (vol-1234...)
+		// Build a URL with an empty host (AZ)
+		s = "aws://" + "" + "/" + s
+	}
+	url, err := url.Parse(s)
+	if err != nil {
+		// TODO: Maybe we should pass a URL into the Volume functions
+		return "", fmt.Errorf("Invalid disk name (%s): %v", name, err)
+	}
+	if url.Scheme != "aws" {
+		return "", fmt.Errorf("Invalid scheme for AWS volume (%s)", name)
+	}
+
+	awsID := url.Path
+	awsID = strings.Trim(awsID, "/")
+
+	// We sanity check the resulting volume; the two known formats are
+	// vol-12345678 and vol-12345678abcdef01
+	// TODO: Regex match?
+	if strings.Contains(awsID, "/") || !strings.HasPrefix(awsID, "vol-") {
+		return "", fmt.Errorf("Invalid format for AWS volume (%s)", name)
+	}
+
+	return awsVolumeID(awsID), nil
+}
--- a/pkg/volume/aws_ebs/attacher.go
+++ b/pkg/volume/aws_ebs/attacher.go
@@ -64,7 +64,7 @@ func (attacher *awsElasticBlockStoreAttacher) Attach(spec *volume.Spec, nodeName
 		return "", err
 	}

-	volumeID := volumeSource.VolumeID
+	volumeID := aws.KubernetesVolumeID(volumeSource.VolumeID)

 	// awsCloud.AttachDisk checks if disk is already attached to node and
 	// succeeds in that case, so no need to do that separately.
@@ -79,8 +79,8 @@ func (attacher *awsElasticBlockStoreAttacher) Attach(spec *volume.Spec, nodeName

 func (attacher *awsElasticBlockStoreAttacher) VolumesAreAttached(specs []*volume.Spec, nodeName types.NodeName) (map[*volume.Spec]bool, error) {
 	volumesAttachedCheck := make(map[*volume.Spec]bool)
-	volumeSpecMap := make(map[string]*volume.Spec)
-	volumeIDList := []string{}
+	volumeSpecMap := make(map[aws.KubernetesVolumeID]*volume.Spec)
+	volumeIDList := []aws.KubernetesVolumeID{}
 	for _, spec := range specs {
 		volumeSource, _, err := getVolumeSource(spec)
 		if err != nil {
@@ -88,9 +88,10 @@ func (attacher *awsElasticBlockStoreAttacher) VolumesAreAttached(specs []*volume
 			continue
 		}

-		volumeIDList = append(volumeIDList, volumeSource.VolumeID)
+		name := aws.KubernetesVolumeID(volumeSource.VolumeID)
+		volumeIDList = append(volumeIDList, name)
 		volumesAttachedCheck[spec] = true
-		volumeSpecMap[volumeSource.VolumeID] = spec
+		volumeSpecMap[name] = spec
 	}
 	attachedResult, err := attacher.awsVolumes.DisksAreAttached(volumeIDList, nodeName)
 	if err != nil {
@@ -163,7 +164,7 @@ func (attacher *awsElasticBlockStoreAttacher) GetDeviceMountPath(
 		return "", err
 	}

-	return makeGlobalPDPath(attacher.host, volumeSource.VolumeID), nil
+	return makeGlobalPDPath(attacher.host, aws.KubernetesVolumeID(volumeSource.VolumeID)), nil
 }

 // FIXME: this method can be further pruned.
@@ -221,7 +222,7 @@ func (plugin *awsElasticBlockStorePlugin) NewDetacher() (volume.Detacher, error)
 }

 func (detacher *awsElasticBlockStoreDetacher) Detach(deviceMountPath string, nodeName types.NodeName) error {
-	volumeID := path.Base(deviceMountPath)
+	volumeID := aws.KubernetesVolumeID(path.Base(deviceMountPath))

 	attached, err := detacher.awsVolumes.DiskIsAttached(volumeID, nodeName)
 	if err != nil {
--- a/pkg/volume/aws_ebs/attacher_test.go
+++ b/pkg/volume/aws_ebs/attacher_test.go
@@ -29,37 +29,37 @@ import (
 	"k8s.io/kubernetes/pkg/types"
 )

-func TestGetDeviceName_Volume(t *testing.T) {
+func TestGetVolumeName_Volume(t *testing.T) {
 	plugin := newPlugin()
-	name := "my-aws-volume"
+	name := aws.KubernetesVolumeID("my-aws-volume")
 	spec := createVolSpec(name, false)

-	deviceName, err := plugin.GetVolumeName(spec)
+	volumeName, err := plugin.GetVolumeName(spec)
 	if err != nil {
-		t.Errorf("GetDeviceName error: %v", err)
+		t.Errorf("GetVolumeName error: %v", err)
 	}
-	if deviceName != name {
-		t.Errorf("GetDeviceName error: expected %s, got %s", name, deviceName)
+	if volumeName != string(name) {
+		t.Errorf("GetVolumeName error: expected %s, got %s", name, volumeName)
 	}
 }

-func TestGetDeviceName_PersistentVolume(t *testing.T) {
+func TestGetVolumeName_PersistentVolume(t *testing.T) {
 	plugin := newPlugin()
-	name := "my-aws-pv"
+	name := aws.KubernetesVolumeID("my-aws-pv")
 	spec := createPVSpec(name, true)

-	deviceName, err := plugin.GetVolumeName(spec)
+	volumeName, err := plugin.GetVolumeName(spec)
 	if err != nil {
-		t.Errorf("GetDeviceName error: %v", err)
+		t.Errorf("GetVolumeName error: %v", err)
 	}
-	if deviceName != name {
-		t.Errorf("GetDeviceName error: expected %s, got %s", name, deviceName)
+	if volumeName != string(name) {
+		t.Errorf("GetVolumeName error: expected %s, got %s", name, volumeName)
 	}
 }

 // One testcase for TestAttachDetach table test below
 type testcase struct {
-	name string
+	name aws.KubernetesVolumeID
 	// For fake AWS:
 	attach         attachCall
 	detach         detachCall
@@ -74,7 +74,7 @@ type testcase struct {
 }

 func TestAttachDetach(t *testing.T) {
-	diskName := "disk"
+	diskName := aws.KubernetesVolumeID("disk")
 	nodeName := types.NodeName("instance")
 	readOnly := false
 	spec := createVolSpec(diskName, readOnly)
@@ -111,7 +111,8 @@ func TestAttachDetach(t *testing.T) {
 			detach:         detachCall{diskName, nodeName, "/dev/sda", nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
-				return "", detacher.Detach(diskName, nodeName)
+				mountPath := "/mnt/" + string(diskName)
+				return "", detacher.Detach(mountPath, nodeName)
 			},
 		},

@@ -121,7 +122,8 @@ func TestAttachDetach(t *testing.T) {
 			diskIsAttached: diskIsAttachedCall{diskName, nodeName, false, nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
-				return "", detacher.Detach(diskName, nodeName)
+				mountPath := "/mnt/" + string(diskName)
+				return "", detacher.Detach(mountPath, nodeName)
 			},
 		},

@@ -132,7 +134,8 @@ func TestAttachDetach(t *testing.T) {
 			detach:         detachCall{diskName, nodeName, "/dev/sda", nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
-				return "", detacher.Detach(diskName, nodeName)
+				mountPath := "/mnt/" + string(diskName)
+				return "", detacher.Detach(mountPath, nodeName)
 			},
 		},

@@ -143,7 +146,8 @@ func TestAttachDetach(t *testing.T) {
 			detach:         detachCall{diskName, nodeName, "", detachError},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
-				return "", detacher.Detach(diskName, nodeName)
+				mountPath := "/mnt/" + string(diskName)
+				return "", detacher.Detach(mountPath, nodeName)
 			},
 			expectedError: detachError,
 		},
@@ -185,12 +189,12 @@ func newDetacher(testcase *testcase) *awsElasticBlockStoreDetacher {
 	}
 }

-func createVolSpec(name string, readOnly bool) *volume.Spec {
+func createVolSpec(name aws.KubernetesVolumeID, readOnly bool) *volume.Spec {
 	return &volume.Spec{
 		Volume: &api.Volume{
 			VolumeSource: api.VolumeSource{
 				AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{
-					VolumeID: name,
+					VolumeID: string(name),
 					ReadOnly: readOnly,
 				},
 			},
@@ -198,13 +202,13 @@ func createVolSpec(name string, readOnly bool) *volume.Spec {
 	}
 }

-func createPVSpec(name string, readOnly bool) *volume.Spec {
+func createPVSpec(name aws.KubernetesVolumeID, readOnly bool) *volume.Spec {
 	return &volume.Spec{
 		PersistentVolume: &api.PersistentVolume{
 			Spec: api.PersistentVolumeSpec{
 				PersistentVolumeSource: api.PersistentVolumeSource{
 					AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{
-						VolumeID: name,
+						VolumeID: string(name),
 						ReadOnly: readOnly,
 					},
 				},
@@ -216,7 +220,7 @@ func createPVSpec(name string, readOnly bool) *volume.Spec {
 // Fake AWS implementation

 type attachCall struct {
-	diskName      string
+	diskName      aws.KubernetesVolumeID
 	nodeName      types.NodeName
 	readOnly      bool
 	retDeviceName string
@@ -224,20 +228,20 @@ type attachCall struct {
 }

 type detachCall struct {
-	diskName      string
+	diskName      aws.KubernetesVolumeID
 	nodeName      types.NodeName
 	retDeviceName string
 	ret           error
 }

 type diskIsAttachedCall struct {
-	diskName   string
+	diskName   aws.KubernetesVolumeID
 	nodeName   types.NodeName
 	isAttached bool
 	ret        error
 }

-func (testcase *testcase) AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error) {
+func (testcase *testcase) AttachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error) {
 	expected := &testcase.attach

 	if expected.diskName == "" && expected.nodeName == "" {
@@ -267,7 +271,7 @@ func (testcase *testcase) AttachDisk(diskName string, nodeName types.NodeName, r
 	return expected.retDeviceName, expected.ret
 }

-func (testcase *testcase) DetachDisk(diskName string, nodeName types.NodeName) (string, error) {
+func (testcase *testcase) DetachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (string, error) {
 	expected := &testcase.detach

 	if expected.diskName == "" && expected.nodeName == "" {
@@ -292,7 +296,7 @@ func (testcase *testcase) DetachDisk(diskName string, nodeName types.NodeName) (
 	return expected.retDeviceName, expected.ret
 }

-func (testcase *testcase) DiskIsAttached(diskName string, nodeName types.NodeName) (bool, error) {
+func (testcase *testcase) DiskIsAttached(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (bool, error) {
 	expected := &testcase.diskIsAttached

 	if expected.diskName == "" && expected.nodeName == "" {
@@ -317,22 +321,22 @@ func (testcase *testcase) DiskIsAttached(diskName string, nodeName types.NodeNam
 	return expected.isAttached, expected.ret
 }

-func (testcase *testcase) DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error) {
+func (testcase *testcase) DisksAreAttached(diskNames []aws.KubernetesVolumeID, nodeName types.NodeName) (map[aws.KubernetesVolumeID]bool, error) {
 	return nil, errors.New("Not implemented")
 }

-func (testcase *testcase) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName string, err error) {
+func (testcase *testcase) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName aws.KubernetesVolumeID, err error) {
 	return "", errors.New("Not implemented")
 }

-func (testcase *testcase) DeleteDisk(volumeName string) (bool, error) {
+func (testcase *testcase) DeleteDisk(volumeName aws.KubernetesVolumeID) (bool, error) {
 	return false, errors.New("Not implemented")
 }

-func (testcase *testcase) GetVolumeLabels(volumeName string) (map[string]string, error) {
+func (testcase *testcase) GetVolumeLabels(volumeName aws.KubernetesVolumeID) (map[string]string, error) {
 	return map[string]string{}, errors.New("Not implemented")
 }

-func (testcase *testcase) GetDiskPath(volumeName string) (string, error) {
+func (testcase *testcase) GetDiskPath(volumeName aws.KubernetesVolumeID) (string, error) {
 	return "", errors.New("Not implemented")
 }
--- a/pkg/volume/aws_ebs/aws_ebs.go
+++ b/pkg/volume/aws_ebs/aws_ebs.go
@@ -27,6 +27,7 @@ import (
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/resource"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/aws"
 	"k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util/exec"
 	"k8s.io/kubernetes/pkg/util/mount"
@@ -102,7 +103,7 @@ func (plugin *awsElasticBlockStorePlugin) newMounterInternal(spec *volume.Spec,
 		return nil, err
 	}

-	volumeID := ebs.VolumeID
+	volumeID := aws.KubernetesVolumeID(ebs.VolumeID)
 	fsType := ebs.FSType
 	partition := ""
 	if ebs.Partition != 0 {
@@ -153,7 +154,7 @@ func (plugin *awsElasticBlockStorePlugin) newDeleterInternal(spec *volume.Spec,
 	return &awsElasticBlockStoreDeleter{
 		awsElasticBlockStore: &awsElasticBlockStore{
 			volName:  spec.Name(),
-			volumeID: spec.PersistentVolume.Spec.AWSElasticBlockStore.VolumeID,
+			volumeID: aws.KubernetesVolumeID(spec.PersistentVolume.Spec.AWSElasticBlockStore.VolumeID),
 			manager:  manager,
 			plugin:   plugin,
 		}}, nil
@@ -205,7 +206,7 @@ func (plugin *awsElasticBlockStorePlugin) ConstructVolumeSpec(volName, mountPath

 // Abstract interface to PD operations.
 type ebsManager interface {
-	CreateVolume(provisioner *awsElasticBlockStoreProvisioner) (volumeID string, volumeSizeGB int, labels map[string]string, err error)
+	CreateVolume(provisioner *awsElasticBlockStoreProvisioner) (volumeID aws.KubernetesVolumeID, volumeSizeGB int, labels map[string]string, err error)
 	// Deletes a volume
 	DeleteVolume(deleter *awsElasticBlockStoreDeleter) error
 }
@@ -216,7 +217,7 @@ type awsElasticBlockStore struct {
 	volName string
 	podUID  types.UID
 	// Unique id of the PD, used to find the disk resource in the provider.
-	volumeID string
+	volumeID aws.KubernetesVolumeID
 	// Specifies the partition to mount
 	partition string
 	// Utility interface that provides API calls to the provider to attach/detach disks.
@@ -312,9 +313,9 @@ func (b *awsElasticBlockStoreMounter) SetUpAt(dir string, fsGroup *int64) error
 	return nil
 }

-func makeGlobalPDPath(host volume.VolumeHost, volumeID string) string {
+func makeGlobalPDPath(host volume.VolumeHost, volumeID aws.KubernetesVolumeID) string {
 	// Clean up the URI to be more fs-friendly
-	name := volumeID
+	name := string(volumeID)
 	name = strings.Replace(name, "://", "/", -1)
 	return path.Join(host.GetPluginDir(awsElasticBlockStorePluginName), "mounts", name)
 }
@@ -432,7 +433,7 @@ func (c *awsElasticBlockStoreProvisioner) Provision() (*api.PersistentVolume, er
 			},
 			PersistentVolumeSource: api.PersistentVolumeSource{
 				AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{
-					VolumeID:  volumeID,
+					VolumeID:  string(volumeID),
 					FSType:    "ext4",
 					Partition: 0,
 					ReadOnly:  false,
--- a/pkg/volume/aws_ebs/aws_ebs_test.go
+++ b/pkg/volume/aws_ebs/aws_ebs_test.go
@@ -24,6 +24,7 @@ import (

 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/aws"
 	"k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util/mount"
 	utiltesting "k8s.io/kubernetes/pkg/util/testing"
@@ -91,7 +92,7 @@ type fakePDManager struct {

 // TODO(jonesdl) To fully test this, we could create a loopback device
 // and mount that instead.
-func (fake *fakePDManager) CreateVolume(c *awsElasticBlockStoreProvisioner) (volumeID string, volumeSizeGB int, labels map[string]string, err error) {
+func (fake *fakePDManager) CreateVolume(c *awsElasticBlockStoreProvisioner) (volumeID aws.KubernetesVolumeID, volumeSizeGB int, labels map[string]string, err error) {
 	labels = make(map[string]string)
 	labels["fakepdmanager"] = "yes"
 	return "test-aws-volume-name", 100, labels, nil
--- a/pkg/volume/aws_ebs/aws_util.go
+++ b/pkg/volume/aws_ebs/aws_util.go
@@ -65,7 +65,7 @@ func (util *AWSDiskUtil) DeleteVolume(d *awsElasticBlockStoreDeleter) error {

 // CreateVolume creates an AWS EBS volume.
 // Returns: volumeID, volumeSizeGB, labels, error
-func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (string, int, map[string]string, error) {
+func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner) (aws.KubernetesVolumeID, int, map[string]string, error) {
 	cloud, err := getCloudProvider(c.awsElasticBlockStore.plugin.host.GetCloudProvider())
 	if err != nil {
 		return "", 0, nil, err
--- a/pkg/volume/gce_pd/attacher.go
+++ b/pkg/volume/gce_pd/attacher.go
@@ -242,7 +242,7 @@ func (plugin *gcePersistentDiskPlugin) NewDetacher() (volume.Detacher, error) {
 // attached to the specified node. If the volume is not attached, it succeeds
 // (returns nil). If it is attached, Detach issues a call to the GCE cloud
 // provider to attach it.
-// Callers are responsible for retryinging on failure.
+// Callers are responsible for retrying on failure.
 // Callers are responsible for thread safety between concurrent attach and detach
 // operations.
 func (detacher *gcePersistentDiskDetacher) Detach(deviceMountPath string, nodeName types.NodeName) error {
--- a/plugin/pkg/admission/persistentvolume/label/admission.go
+++ b/plugin/pkg/admission/persistentvolume/label/admission.go
@@ -118,7 +118,8 @@ func (l *persistentVolumeLabel) findAWSEBSLabels(volume *api.PersistentVolume) (

 	// TODO: GetVolumeLabels is actually a method on the Volumes interface
 	// If that gets standardized we can refactor to reduce code duplication
-	labels, err := ebsVolumes.GetVolumeLabels(volume.Spec.AWSElasticBlockStore.VolumeID)
+	spec := aws.KubernetesVolumeID(volume.Spec.AWSElasticBlockStore.VolumeID)
+	labels, err := ebsVolumes.GetVolumeLabels(spec)
 	if err != nil {
 		return nil, err
 	}
--- a/plugin/pkg/admission/persistentvolume/label/admission_test.go
+++ b/plugin/pkg/admission/persistentvolume/label/admission_test.go
@@ -34,35 +34,35 @@ type mockVolumes struct {

 var _ aws.Volumes = &mockVolumes{}

-func (v *mockVolumes) AttachDisk(diskName string, nodeName types.NodeName, readOnly bool) (string, error) {
+func (v *mockVolumes) AttachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName, readOnly bool) (string, error) {
 	return "", fmt.Errorf("not implemented")
 }

-func (v *mockVolumes) DetachDisk(diskName string, nodeName types.NodeName) (string, error) {
+func (v *mockVolumes) DetachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (string, error) {
 	return "", fmt.Errorf("not implemented")
 }

-func (v *mockVolumes) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName string, err error) {
+func (v *mockVolumes) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName aws.KubernetesVolumeID, err error) {
 	return "", fmt.Errorf("not implemented")
 }

-func (v *mockVolumes) DeleteDisk(volumeName string) (bool, error) {
+func (v *mockVolumes) DeleteDisk(volumeName aws.KubernetesVolumeID) (bool, error) {
 	return false, fmt.Errorf("not implemented")
 }

-func (v *mockVolumes) GetVolumeLabels(volumeName string) (map[string]string, error) {
+func (v *mockVolumes) GetVolumeLabels(volumeName aws.KubernetesVolumeID) (map[string]string, error) {
 	return v.volumeLabels, v.volumeLabelsError
 }

-func (c *mockVolumes) GetDiskPath(volumeName string) (string, error) {
+func (c *mockVolumes) GetDiskPath(volumeName aws.KubernetesVolumeID) (string, error) {
 	return "", fmt.Errorf("not implemented")
 }

-func (c *mockVolumes) DiskIsAttached(volumeName string, nodeName types.NodeName) (bool, error) {
+func (c *mockVolumes) DiskIsAttached(volumeName aws.KubernetesVolumeID, nodeName types.NodeName) (bool, error) {
 	return false, fmt.Errorf("not implemented")
 }

-func (c *mockVolumes) DisksAreAttached(diskNames []string, nodeName types.NodeName) (map[string]bool, error) {
+func (c *mockVolumes) DisksAreAttached(diskNames []aws.KubernetesVolumeID, nodeName types.NodeName) (map[aws.KubernetesVolumeID]bool, error) {
 	return nil, fmt.Errorf("not implemented")
 }