diff --git a/pkg/cloudprovider/providers/aws/BUILD b/pkg/cloudprovider/providers/aws/BUILD index 07371824f9..6087aa7c8b 100644 --- a/pkg/cloudprovider/providers/aws/BUILD +++ b/pkg/cloudprovider/providers/aws/BUILD @@ -45,6 +45,7 @@ go_library( "//staging/src/k8s.io/cloud-provider/volume:go_default_library", "//staging/src/k8s.io/cloud-provider/volume/errors:go_default_library", "//staging/src/k8s.io/cloud-provider/volume/helpers:go_default_library", + "//staging/src/k8s.io/csi-translation-lib/plugins:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/awserr:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/credentials:go_default_library", diff --git a/pkg/cloudprovider/providers/aws/volumes.go b/pkg/cloudprovider/providers/aws/volumes.go index 7031c5a52c..0c8a24fa53 100644 --- a/pkg/cloudprovider/providers/aws/volumes.go +++ b/pkg/cloudprovider/providers/aws/volumes.go @@ -18,20 +18,15 @@ package aws import ( "fmt" - "net/url" - "regexp" - "strings" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + csimigration "k8s.io/csi-translation-lib/plugins" "k8s.io/klog" "k8s.io/apimachinery/pkg/types" ) -// awsVolumeRegMatch represents Regex Match for AWS volume. -var awsVolumeRegMatch = regexp.MustCompile("^vol-[^/]*$") - // EBSVolumeID represents the ID of the volume in the AWS API, e.g. // vol-12345678 The "traditional" format is "vol-12345678" A new longer format // is also being introduced: "vol-12345678abcdef01" We should not assume @@ -62,41 +57,10 @@ type diskInfo struct { // MapToAWSVolumeID extracts the EBSVolumeID from the KubernetesVolumeID func (name KubernetesVolumeID) MapToAWSVolumeID() (EBSVolumeID, 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) + awsID, err := csimigration.KubernetesVolumeIDToEBSVolumeID(string(name)) if err != nil { - // TODO: Maybe we should pass a URL into the Volume functions - return "", fmt.Errorf("Invalid disk name (%s): %v", name, err) + return "", 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 - if !awsVolumeRegMatch.MatchString(awsID) { - return "", fmt.Errorf("Invalid format for AWS volume (%s)", name) - } - return EBSVolumeID(awsID), nil } diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/BUILD b/staging/src/k8s.io/csi-translation-lib/plugins/BUILD index 683440587d..965bbd0175 100644 --- a/staging/src/k8s.io/csi-translation-lib/plugins/BUILD +++ b/staging/src/k8s.io/csi-translation-lib/plugins/BUILD @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", @@ -31,3 +31,9 @@ filegroup( tags = ["automanaged"], visibility = ["//visibility:public"], ) + +go_test( + name = "go_default_test", + srcs = ["aws_ebs_test.go"], + embed = [":go_default_library"], +) diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go b/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go index c8b900a7b7..9cb6176849 100644 --- a/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go +++ b/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs.go @@ -18,7 +18,10 @@ package plugins import ( "fmt" + "net/url" + "regexp" "strconv" + "strings" "k8s.io/api/core/v1" ) @@ -54,9 +57,14 @@ func (t *awsElasticBlockStoreCSITranslator) TranslateInTreePVToCSI(pv *v1.Persis ebsSource := pv.Spec.AWSElasticBlockStore + volumeHandle, err := KubernetesVolumeIDToEBSVolumeID(ebsSource.VolumeID) + if err != nil { + return nil, fmt.Errorf("failed to translate Kubernetes ID to EBS Volume ID %v", err) + } + csiSource := &v1.CSIPersistentVolumeSource{ Driver: AWSEBSDriverName, - VolumeHandle: ebsSource.VolumeID, + VolumeHandle: volumeHandle, ReadOnly: ebsSource.ReadOnly, FSType: ebsSource.FSType, VolumeAttributes: map[string]string{ @@ -113,3 +121,50 @@ func (t *awsElasticBlockStoreCSITranslator) GetInTreePluginName() string { func (t *awsElasticBlockStoreCSITranslator) GetCSIPluginName() string { return AWSEBSDriverName } + +// awsVolumeRegMatch represents Regex Match for AWS volume. +var awsVolumeRegMatch = regexp.MustCompile("^vol-[^/]*$") + +// KubernetesVolumeIDToEBSVolumeID translates Kubernetes volume ID to EBS volume ID +// KubernetsVolumeID forms: +// * aws:/// +// * aws:/// +// * +// EBS Volume ID form: +// * vol- +// This translation shouldn't be needed and should be fixed in long run +// See https://github.com/kubernetes/kubernetes/issues/73730 +func KubernetesVolumeIDToEBSVolumeID(kubernetesID string) (string, 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. + if !strings.HasPrefix(kubernetesID, "aws://") { + // Assume a bare aws volume id (vol-1234...) + return kubernetesID, nil + } + url, err := url.Parse(kubernetesID) + if err != nil { + // TODO: Maybe we should pass a URL into the Volume functions + return "", fmt.Errorf("Invalid disk name (%s): %v", kubernetesID, err) + } + if url.Scheme != "aws" { + return "", fmt.Errorf("Invalid scheme for AWS volume (%s)", kubernetesID) + } + + awsID := url.Path + awsID = strings.Trim(awsID, "/") + + // We sanity check the resulting volume; the two known formats are + // vol-12345678 and vol-12345678abcdef01 + if !awsVolumeRegMatch.MatchString(awsID) { + return "", fmt.Errorf("Invalid format for AWS volume (%s)", kubernetesID) + } + + return awsID, nil +} diff --git a/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs_test.go b/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs_test.go new file mode 100644 index 0000000000..a79023b1a9 --- /dev/null +++ b/staging/src/k8s.io/csi-translation-lib/plugins/aws_ebs_test.go @@ -0,0 +1,66 @@ +/* +Copyright 2019 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 plugins + +import ( + "testing" +) + +func TestKubernetesVolumeIDToEBSVolumeID(t *testing.T) { + testCases := []struct { + name string + kubernetesID string + ebsVolumeID string + expErr bool + }{ + { + name: "Normal ID format", + kubernetesID: "vol-02399794d890f9375", + ebsVolumeID: "vol-02399794d890f9375", + }, + { + name: "aws:///{volumeId} format", + kubernetesID: "aws:///vol-02399794d890f9375", + ebsVolumeID: "vol-02399794d890f9375", + }, + { + name: "aws://{zone}/{volumeId} format", + kubernetesID: "aws://us-west-2a/vol-02399794d890f9375", + ebsVolumeID: "vol-02399794d890f9375", + }, + { + name: "fails on invalid volume ID", + kubernetesID: "aws://us-west-2a/02399794d890f9375", + expErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual, err := KubernetesVolumeIDToEBSVolumeID(tc.kubernetesID) + if err != nil { + if !tc.expErr { + t.Errorf("KubernetesVolumeIDToEBSVolumeID failed %v", err) + } + } else { + if actual != tc.ebsVolumeID { + t.Errorf("Wrong EBS Volume ID. actual: %s expected: %s", actual, tc.ebsVolumeID) + } + } + }) + } +}