Merge pull request #74858 from leakingtapan/ebs-id

Add CSI migration logic for EBS Volume ID format

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",

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
 }
 

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"],
+)

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://<zone>/<awsVolumeId>
+//  * aws:///<awsVolumeId>
+//  * <awsVolumeId>
+// EBS Volume ID form:
+//  * vol-<alphanumberic>
+// 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
+}

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)
+				}
+			}
+		})
+	}
+}