tags: initial implementation of tags

Author: nicolehanjing

Diff for: go.sum

@@ -888,11 +888,14 @@ k8s.io/kube-openapi v0.0.0-20200805222855-6aeccd4b50c6 h1:+WnxoVtG8TMiudHBSEtrVL
 k8s.io/kube-openapi v0.0.0-20200805222855-6aeccd4b50c6/go.mod h1:UuqjUnNftUyPE5H64/qeyjQoUZhGpeFDVdxjTeEVN2o=
 k8s.io/kubernetes v0.0.0-20201023141757-9e8ad8ce9d8a h1:uAU9n6UBa4coDhrOEQ7iCEHrYXAlm39VcI4asrBOkLA=
 k8s.io/kubernetes v0.0.0-20201023141757-9e8ad8ce9d8a/go.mod h1:yhT1/ltQajQsha3tnYc9QPFYSumGM45nlZdjf7WqE1A=
+k8s.io/kubernetes v1.20.0 h1:mnc69esJC3PJgSptxNJomGz2gBthyGLSEy18WiyRH4U=
 k8s.io/kubernetes/staging/src/k8s.io/api v0.0.0-20201023141757-9e8ad8ce9d8a h1:8ilXcbfDzt4rC/OVgFPIwW69aT5K/s1pPz415xSEctA=
 k8s.io/kubernetes/staging/src/k8s.io/api v0.0.0-20201023141757-9e8ad8ce9d8a/go.mod h1:Y4VjjNur38HL6/QxaTVK2yno1zjEQlvcvwbbRQs2DtQ=
 k8s.io/kubernetes/staging/src/k8s.io/apiextensions-apiserver v0.0.0-20201023141757-9e8ad8ce9d8a/go.mod h1:BvtZU215FgO19Oy19K6h8qwajFfjxYqGewgjuYHWGRw=
 k8s.io/kubernetes/staging/src/k8s.io/apimachinery v0.0.0-20201023141757-9e8ad8ce9d8a h1:aA1ABMciJFSVW1goSQyWMtZFE0ZfK/LD9zBsufsjl5Q=
 k8s.io/kubernetes/staging/src/k8s.io/apimachinery v0.0.0-20201023141757-9e8ad8ce9d8a/go.mod h1:4qgwPPTQvmc3E4Ub+c6I9LSsdbujYP3pIQEGuIVy8oQ=
+k8s.io/kubernetes/staging/src/k8s.io/apimachinery v0.0.0-20201209155502-a20aeb8eed76 h1:59Ei4O+iDhHnIq5PvXEakUmdmXXw/pHkATq73z20dM4=
+k8s.io/kubernetes/staging/src/k8s.io/apimachinery v0.0.0-20201209194726-7df379d75199 h1:kiRm1BFDCuMD1v4yUqAYa/gRODVKB0yl+3bT6vqqV+U=
 k8s.io/kubernetes/staging/src/k8s.io/apiserver v0.0.0-20201023141757-9e8ad8ce9d8a h1:rUoFc7OSO1DLTKVGM39sDtfjHlRUezbBFV+St6HmkJQ=
 k8s.io/kubernetes/staging/src/k8s.io/apiserver v0.0.0-20201023141757-9e8ad8ce9d8a/go.mod h1:lpK+uXhJTVOwW6SDiSQiL0LaQaBktrM23VG489uC/U0=
 k8s.io/kubernetes/staging/src/k8s.io/cli-runtime v0.0.0-20201023141757-9e8ad8ce9d8a/go.mod h1:twd45pbv7psOvyGxI8eABhpeoXWW3bCX6aB5NVS6TXs=

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

@@ -19,6 +19,7 @@ limitations under the License.
 package v2
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -31,8 +32,10 @@ import (
 	"github.com/aws/aws-sdk-go/aws/session"
 	"github.com/aws/aws-sdk-go/service/ec2"
 
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/cloud-provider-aws/pkg/apis/config/v1alpha1"
+	"k8s.io/klog/v2"
 	"sigs.k8s.io/yaml"
 )
 
@@ -43,7 +46,12 @@ func init() {
 			return nil, fmt.Errorf("failed to read AWS cloud provider config file: %v", err)
 		}
 
-		return newCloud(*cfg)
+		errs := validateAWSCloudConfig(cfg)
+		if len(errs) > 0 {
+			return nil, fmt.Errorf("failed to validate AWS cloud config: %v", errs.ToAggregate())
+		}
+
+		return newCloud(cfg)
 	})
 }
 
@@ -61,7 +69,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 +87,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
@@ -94,13 +102,35 @@ func readAWSCloudConfig(config io.Reader) (*v1alpha1.AWSCloudConfig, error) {
 		// we got an error where the decode wasn't related to a missing type
 		return nil, err
 	}
-	if cfg.Kind != "AWSCloudConfig" {
-		return nil, fmt.Errorf("invalid cloud configuration object %q", cfg.Kind)
-	}
 
 	return &cfg, nil
 }
 
+// validateAWSCloudConfig validates AWSCloudConfig
+// clusterName is required
+func validateAWSCloudConfig(config *v1alpha1.AWSCloudConfig) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if config.Kind != "AWSCloudConfig" {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("Kind"), "invalid Kind for cloud config: %q", config.Kind))
+	}
+
+	if config.APIVersion != "config.aws.io/v1alpha1" {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("APIVersion"), "invalid APIVersion for cloud config: %q", config.APIVersion))
+	}
+
+	fieldPath := field.NewPath("Config")
+	if config.Config == (v1alpha1.AWSConfig{}) {
+		allErrs = append(allErrs, field.Required(fieldPath, "Config is required, should not be empty"))
+	}
+
+	if len(config.Config.ClusterName) == 0 {
+		allErrs = append(allErrs, field.Required(fieldPath.Child("ClusterName"), "cluster name cannot be empty"))
+	}
+
+	return allErrs
+}
+
 func getAvailabilityZone(metadata EC2Metadata) (string, error) {
 	return metadata.GetMetadata("placement/availability-zone")
 }
@@ -122,7 +152,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)
@@ -152,11 +182,6 @@ func newCloud(cfg v1alpha1.AWSCloudConfig) (cloudprovider.Interface, error) {
 		return nil, err
 	}
 
-	instances, err := newInstances(az, creds)
-	if err != nil {
-		return nil, err
-	}
-
 	ec2Sess, err := session.NewSession(&aws.Config{
 		Region:      aws.String(region),
 		Credentials: creds,
@@ -170,21 +195,29 @@ 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
 		}
+	} else {
+		klog.Warning("misconfigured cluster: no clusterName")
 	}
 
-	return awsCloud, nil
+	instances, err := newInstances(az, creds, tags)
+	if err != nil {
+		return nil, err
+	}
+
+	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 +256,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/cloud_test.go

@@ -19,9 +19,14 @@ limitations under the License.
 package v2
 
 import (
+	"bytes"
+	"reflect"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/cloud-provider-aws/pkg/apis/config/v1alpha1"
 )
 
 func TestAzToRegion(t *testing.T) {
@@ -44,3 +49,174 @@ func TestAzToRegion(t *testing.T) {
 		assert.Equal(t, testCase.region, ret)
 	}
 }
+
+func TestReadAWSCloudConfig(t *testing.T) {
+	testcases := []struct {
+		name       string
+		configData string
+		config     *v1alpha1.AWSCloudConfig
+		expectErr  bool
+	}{
+		{
+			name: "config with valid cluster name",
+			configData: `---
+kind: AWSCloudConfig
+apiVersion: config.aws.io/v1alpha1
+config:
+    clusterName: test
+    `,
+			config: &v1alpha1.AWSCloudConfig{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AWSCloudConfig",
+					APIVersion: "config.aws.io/v1alpha1",
+				},
+				Config: v1alpha1.AWSConfig{
+					ClusterName: "test",
+				},
+			},
+		},
+		{
+			name: "config with empty cluster name",
+			configData: `---
+kind: AWSCloudConfig
+apiVersion: config.aws.io/v1alpha1
+config:
+    clusterName: ""
+    `,
+			config: &v1alpha1.AWSCloudConfig{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AWSCloudConfig",
+					APIVersion: "config.aws.io/v1alpha1",
+				},
+				Config: v1alpha1.AWSConfig{
+					ClusterName: "",
+				},
+			},
+		},
+		{
+			name: "config with only kind and apiVersion",
+			configData: `---
+kind: AWSCloudConfig
+apiVersion: config.aws.io/v1alpha1
+    `,
+			config: &v1alpha1.AWSCloudConfig{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AWSCloudConfig",
+					APIVersion: "config.aws.io/v1alpha1",
+				},
+				Config: v1alpha1.AWSConfig{
+					ClusterName: "",
+				},
+			},
+		},
+		{
+			name: "config with wrong Kind",
+			configData: `---
+kind: WrongCloudConfig
+apiVersion: config.aws.io/v1alpha1
+config:
+    clusterName: test
+	`,
+			config:    nil,
+			expectErr: true,
+		},
+		{
+			name: "config with wrong apiversion",
+			configData: `---
+kind: AWSCloudConfig
+apiVersion: wrong.aws.io/v1alpha1
+config:
+    clusterName: test
+	`,
+			config:    nil,
+			expectErr: true,
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			buffer := bytes.NewBufferString(testcase.configData)
+			cloudConfig, err := readAWSCloudConfig(buffer)
+			if err != nil && !testcase.expectErr {
+				t.Fatal(err)
+			}
+
+			if err == nil && testcase.expectErr {
+				t.Error("expected error but got none")
+			}
+
+			if !reflect.DeepEqual(cloudConfig, testcase.config) {
+				t.Logf("actual cloud config: %#v", cloudConfig)
+				t.Logf("expected cloud config: %#v", testcase.config)
+				t.Error("AWS cloud config did not match")
+			}
+		})
+	}
+}
+
+func TestValidateAWSCloudConfig(t *testing.T) {
+	testcases := []struct {
+		name      string
+		config    *v1alpha1.AWSCloudConfig
+		expectErr bool
+	}{
+		{
+			name: "valid config",
+			config: &v1alpha1.AWSCloudConfig{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AWSCloudConfig",
+					APIVersion: "config.aws.io/v1alpha1",
+				},
+				Config: v1alpha1.AWSConfig{
+					ClusterName: "test",
+				},
+			},
+		},
+		{
+			name: "empty cluster name",
+			config: &v1alpha1.AWSCloudConfig{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AWSCloudConfig",
+					APIVersion: "config.aws.io/v1alpha1",
+				},
+				Config: v1alpha1.AWSConfig{
+					ClusterName: "",
+				},
+			},
+			expectErr: true,
+		},
+		{
+			name: "empty config",
+			config: &v1alpha1.AWSCloudConfig{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AWSCloudConfig",
+					APIVersion: "config.aws.io/v1alpha1",
+				},
+				Config: v1alpha1.AWSConfig{},
+			},
+			expectErr: true,
+		},
+		{
+			name: "invalid config",
+			config: &v1alpha1.AWSCloudConfig{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "AWSCloudConfig",
+					APIVersion: "config.aws.io/v1alpha1",
+				},
+			},
+			expectErr: true,
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			errs := validateAWSCloudConfig(testcase.config)
+
+			if testcase.expectErr && len(errs) == 0 {
+				t.Errorf("expected error but got none")
+			} else if !testcase.expectErr && len(errs) > 0 {
+				t.Errorf("expected no error but received errors: %v", errs.ToAggregate())
+			}
+		})
+	}
+}