tags: initial implementation of tags

Author: nicolehanjing

Diff for: pkg/providers/v2/cloud.go

@@ -19,6 +19,7 @@ limitations under the License.
 package v2
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -43,7 +44,7 @@ func init() {
 			return nil, fmt.Errorf("failed to read AWS cloud provider config file: %v", err)
 		}
 
-		return newCloud(*cfg)
+		return newCloud(cfg)
 	})
 }
 
@@ -61,7 +62,7 @@ type cloud struct {
 	region    string
 	ec2       EC2
 	metadata  EC2Metadata
-	tagging   awsTagging
+	tags      awsTagging
 }
 
 // EC2 is an interface defining only the methods we call from the AWS EC2 SDK.
@@ -79,7 +80,7 @@ type EC2Metadata interface {
 
 func readAWSCloudConfig(config io.Reader) (*v1alpha1.AWSCloudConfig, error) {
 	if config == nil {
-		return nil, fmt.Errorf("no AWS cloud provider config file given")
+		return nil, errors.New("no AWS cloud provider config file given")
 	}
 
 	// read the config file
@@ -95,7 +96,7 @@ func readAWSCloudConfig(config io.Reader) (*v1alpha1.AWSCloudConfig, error) {
 		return nil, err
 	}
 	if cfg.Kind != "AWSCloudConfig" {
-		return nil, fmt.Errorf("invalid cloud configuration object %q", cfg.Kind)
+		return nil, fmt.Errorf("invalid Kind for cloud config: %q", cfg.Kind)
 	}
 
 	return &cfg, nil
@@ -122,7 +123,7 @@ func azToRegion(az string) (string, error) {
 }
 
 // newCloud creates a new instance of AWSCloud.
-func newCloud(cfg v1alpha1.AWSCloudConfig) (cloudprovider.Interface, error) {
+func newCloud(cfg *v1alpha1.AWSCloudConfig) (cloudprovider.Interface, error) {
 	sess, err := session.NewSession(&aws.Config{})
 	if err != nil {
 		return nil, fmt.Errorf("unable to initialize AWS session: %v", err)
@@ -170,21 +171,22 @@ func newCloud(cfg v1alpha1.AWSCloudConfig) (cloudprovider.Interface, error) {
 		return nil, fmt.Errorf("error creating AWS ec2 client: %q", err)
 	}
 
-	awsCloud := &cloud{
-		creds:     creds,
-		instances: instances,
-		region:    region,
-		metadata:  metadataClient,
-		ec2:       ec2Service,
-	}
-
+	var tags awsTagging
 	if cfg.Config.ClusterName != "" {
-		if err := awsCloud.tagging.init(cfg.Config.ClusterName); err != nil {
+		tags, err = newAWSTags(cfg.Config.ClusterName)
+		if err != nil {
 			return nil, err
 		}
 	}
 
-	return awsCloud, nil
+	return &cloud{
+		creds:     creds,
+		instances: instances,
+		region:    region,
+		metadata:  metadataClient,
+		ec2:       ec2Service,
+		tags:      tags,
+	}, nil
 }
 
 // Initialize passes a Kubernetes clientBuilder interface to the cloud provider
@@ -223,7 +225,7 @@ func (c *cloud) Routes() (cloudprovider.Routes, bool) {
 
 // HasClusterID returns true if the cluster has a clusterID
 func (c *cloud) HasClusterID() bool {
-	return len(c.tagging.clusterName()) > 0
+	return len(c.tags.clusterName()) > 0
 }
 
 // InstancesV2 is an implementation for instances and should only be implemented by external cloud providers.

Diff for: pkg/providers/v2/tags.go

@@ -16,6 +16,7 @@ limitations under the License.
 package v2
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -30,7 +31,9 @@ import (
 const (
 	// TagNameKubernetesClusterPrefix is the tag name we use to differentiate multiple
 	// logically independent clusters running in the same AZ.
-	// tag format: kubernetes.io/cluster/=<clusterName>
+	// tag format: kubernetes.io/cluster/<clusterID> = shared|owned
+	// The tag key = TagNameKubernetesClusterPrefix + clusterID
+	// The tag value is an ownership value
 	TagNameKubernetesClusterPrefix = "kubernetes.io/cluster/"
 
 	// createTag* is configuration of exponential backoff for CreateTag call. We
@@ -49,6 +52,19 @@ type awsTagging struct {
 	ClusterName string
 }
 
+// newAWSTags is a constructor function for awsTagging
+func newAWSTags(clusterName string) (awsTagging, error) {
+	if clusterName != "" {
+		klog.Infof("AWS cloud filtering on ClusterName: %v", clusterName)
+	} else {
+		return awsTagging{}, errors.New("AWS cloud failed to find ClusterName")
+	}
+
+	return awsTagging{
+		ClusterName: clusterName,
+	}, nil
+}
+
 // Extracts the cluster name from the given tags, if they are present
 // If duplicate tags are found, returns an error
 func findClusterName(tags []*ec2.Tag) (string, error) {
@@ -57,7 +73,7 @@ func findClusterName(tags []*ec2.Tag) (string, error) {
 	for _, tag := range tags {
 		tagKey := aws.StringValue(tag.Key)
 		if strings.HasPrefix(tagKey, TagNameKubernetesClusterPrefix) {
-			name := aws.StringValue(tag.Value)
+			name := strings.TrimPrefix(tagKey, TagNameKubernetesClusterPrefix)
 			if clusterName != "" {
 				return "", fmt.Errorf("Found multiple cluster tags with prefix %s (%q and %q)", TagNameKubernetesClusterPrefix, clusterName, name)
 			}
@@ -68,34 +84,6 @@ func findClusterName(tags []*ec2.Tag) (string, error) {
 	return clusterName, nil
 }
 
-func (t *awsTagging) init(clusterName string) error {
-	t.ClusterName = clusterName
-
-	if clusterName != "" {
-		klog.Infof("AWS cloud filtering on ClusterName: %v", clusterName)
-	} else {
-		return fmt.Errorf("AWS cloud failed to find ClusterName")
-	}
-
-	return nil
-}
-
-// Extracts a cluster name from the given tags, if one is present
-// If no clusterName is found, returns "", nil
-// If multiple (different) clusterNames are found, returns an error
-func (t *awsTagging) initFromTags(tags []*ec2.Tag) error {
-	clusterName, err := findClusterName(tags)
-	if err != nil {
-		return err
-	}
-
-	if clusterName == "" {
-		klog.Errorf("Tag %q not found; Kubernetes may behave unexpectedly.", TagNameKubernetesClusterPrefix)
-	}
-
-	return t.init(clusterName)
-}
-
 func (t *awsTagging) hasClusterTag(tags []*ec2.Tag) bool {
 	// if the clusterName is not configured -- we consider all instances.
 	if len(t.ClusterName) == 0 {
@@ -104,7 +92,7 @@ func (t *awsTagging) hasClusterTag(tags []*ec2.Tag) bool {
 
 	for _, tag := range tags {
 		tagKey := aws.StringValue(tag.Key)
-		if (tagKey == TagNameKubernetesClusterPrefix) && (aws.StringValue(tag.Value) == t.ClusterName) {
+		if tagKey == (TagNameKubernetesClusterPrefix + t.ClusterName) {
 			return true
 		}
 	}

Diff for: pkg/providers/v2/tags_test.go

@@ -23,7 +23,7 @@ import (
 )
 
 func TestFindClusterName(t *testing.T) {
-	grid := []struct {
+	testCases := []struct {
 		Tags                map[string]string
 		ExpectedClusterName string
 		ExpectError         bool
@@ -33,7 +33,19 @@ func TestFindClusterName(t *testing.T) {
 		},
 		{
 			Tags: map[string]string{
-				TagNameKubernetesClusterPrefix: TestClusterID,
+				TagNameKubernetesClusterPrefix + TestClusterID: "owned",
+			},
+			ExpectedClusterName: TestClusterID,
+		},
+		{
+			Tags: map[string]string{
+				TagNameKubernetesClusterPrefix + TestClusterID: "shared",
+			},
+			ExpectedClusterName: TestClusterID,
+		},
+		{
+			Tags: map[string]string{
+				TagNameKubernetesClusterPrefix + TestClusterID: "",
 			},
 			ExpectedClusterName: TestClusterID,
 		},
@@ -43,26 +55,33 @@ func TestFindClusterName(t *testing.T) {
 			},
 			ExpectedClusterName: "",
 		},
+		{
+			Tags: map[string]string{
+				TagNameKubernetesClusterPrefix + "a": "",
+				TagNameKubernetesClusterPrefix + "b": "",
+			},
+			ExpectError: true,
+		},
 	}
-	for _, g := range grid {
+	for _, testCase := range testCases {
 		var ec2Tags []*ec2.Tag
-		for k, v := range g.Tags {
+		for k, v := range testCase.Tags {
 			ec2Tags = append(ec2Tags, &ec2.Tag{Key: aws.String(k), Value: aws.String(v)})
 		}
 		clusterName, err := findClusterName(ec2Tags)
-		if g.ExpectError {
+		if testCase.ExpectError {
 			if err == nil {
-				t.Errorf("expected error for tags %v", g.Tags)
+				t.Errorf("expected error for tags %v", testCase.Tags)
 				continue
 			}
 		} else {
 			if err != nil {
-				t.Errorf("unexpected error for tags %v: %v", g.Tags, err)
+				t.Errorf("unexpected error for tags %v: %v", testCase.Tags, err)
 				continue
 			}
 
-			if g.ExpectedClusterName != clusterName {
-				t.Errorf("unexpected new clusterName for tags %v: %s vs %s", g.Tags, g.ExpectedClusterName, clusterName)
+			if testCase.ExpectedClusterName != clusterName {
+				t.Errorf("unexpected new clusterName for tags %v: %s vs %s", testCase.Tags, testCase.ExpectedClusterName, clusterName)
 				continue
 			}
 		}