Commit 3cdbfc98 authored by Justin Santa Barbara's avatar Justin Santa Barbara

AWS: strong-typing for k8s vs aws volume ids

We are more liberal in what we accept as a volume id in k8s, and indeed we ourselves generate names that look like `aws://<zone>/<id>` for dynamic volumes. This volume id (hereafter a KubernetesVolumeID) cannot directly be compared to an AWS volume ID (hereafter an awsVolumeID). We introduce types for each, to prevent accidental comparison or confusion. Issue #35746
parent 8ca348a7
......@@ -21,6 +21,7 @@ go_library(
"log_handler.go",
"retry_handler.go",
"sets_ippermissions.go",
"volumes.go",
],
tags = ["automanaged"],
deps = [
......
......@@ -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{
......
/*
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
}
......@@ -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 {
......
......@@ -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")
}
......@@ -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,
......
......@@ -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
......
......@@ -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
......
......@@ -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 {
......
......@@ -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
}
......
......@@ -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")
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment