Skip to content

Commit b5229bd

Browse files
nicolehanjingnicolehanjing
committed
tags: initial implementation of tags
1 parent b4c79df commit b5229bd

File tree

4 files changed

+180
-68
lines changed

4 files changed

+180
-68
lines changed

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

+22-16
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@ limitations under the License.
1919
package v2
2020

2121
import (
22+
"errors"
2223
"fmt"
2324
"io"
2425
"io/ioutil"
26+
"k8s.io/klog/v2"
2527
"regexp"
2628

2729
"github.com/aws/aws-sdk-go/aws"
@@ -43,7 +45,7 @@ func init() {
4345
return nil, fmt.Errorf("failed to read AWS cloud provider config file: %v", err)
4446
}
4547

46-
return newCloud(*cfg)
48+
return newCloud(cfg)
4749
})
4850
}
4951

@@ -61,7 +63,7 @@ type cloud struct {
6163
region string
6264
ec2 EC2
6365
metadata EC2Metadata
64-
tagging awsTagging
66+
tags awsTagging
6567
}
6668

6769
// EC2 is an interface defining only the methods we call from the AWS EC2 SDK.
@@ -79,7 +81,7 @@ type EC2Metadata interface {
7981

8082
func readAWSCloudConfig(config io.Reader) (*v1alpha1.AWSCloudConfig, error) {
8183
if config == nil {
82-
return nil, fmt.Errorf("no AWS cloud provider config file given")
84+
return nil, errors.New("no AWS cloud provider config file given")
8385
}
8486

8587
// read the config file
@@ -95,7 +97,7 @@ func readAWSCloudConfig(config io.Reader) (*v1alpha1.AWSCloudConfig, error) {
9597
return nil, err
9698
}
9799
if cfg.Kind != "AWSCloudConfig" {
98-
return nil, fmt.Errorf("invalid cloud configuration object %q", cfg.Kind)
100+
return nil, fmt.Errorf("invalid Kind for cloud config: %q", cfg.Kind)
99101
}
100102

101103
return &cfg, nil
@@ -122,7 +124,7 @@ func azToRegion(az string) (string, error) {
122124
}
123125

124126
// newCloud creates a new instance of AWSCloud.
125-
func newCloud(cfg v1alpha1.AWSCloudConfig) (cloudprovider.Interface, error) {
127+
func newCloud(cfg *v1alpha1.AWSCloudConfig) (cloudprovider.Interface, error) {
126128
sess, err := session.NewSession(&aws.Config{})
127129
if err != nil {
128130
return nil, fmt.Errorf("unable to initialize AWS session: %v", err)
@@ -170,21 +172,25 @@ func newCloud(cfg v1alpha1.AWSCloudConfig) (cloudprovider.Interface, error) {
170172
return nil, fmt.Errorf("error creating AWS ec2 client: %q", err)
171173
}
172174

173-
awsCloud := &cloud{
174-
creds: creds,
175-
instances: instances,
176-
region: region,
177-
metadata: metadataClient,
178-
ec2: ec2Service,
179-
}
180-
175+
var tags awsTagging
181176
if cfg.Config.ClusterName != "" {
182-
if err := awsCloud.tagging.init(cfg.Config.ClusterName); err != nil {
177+
tags, err = newAWSTags(cfg.Config.ClusterName)
178+
if err != nil {
183179
return nil, err
184180
}
181+
} else {
182+
// TODO: infer cluster name and add it automatically
183+
klog.Warning("misconfigured cluster: no clusterName")
185184
}
186185

187-
return awsCloud, nil
186+
return &cloud{
187+
creds: creds,
188+
instances: instances,
189+
region: region,
190+
metadata: metadataClient,
191+
ec2: ec2Service,
192+
tags: tags,
193+
}, nil
188194
}
189195

190196
// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
@@ -223,7 +229,7 @@ func (c *cloud) Routes() (cloudprovider.Routes, bool) {
223229

224230
// HasClusterID returns true if the cluster has a clusterID
225231
func (c *cloud) HasClusterID() bool {
226-
return len(c.tagging.clusterName()) > 0
232+
return len(c.tags.clusterName()) > 0
227233
}
228234

229235
// InstancesV2 is an implementation for instances and should only be implemented by external cloud providers.

Diff for: ‎pkg/providers/v2/cloud_test.go

+98
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,14 @@ limitations under the License.
1919
package v2
2020

2121
import (
22+
"bytes"
23+
"reflect"
2224
"testing"
2325

2426
"github.com/stretchr/testify/assert"
27+
28+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
29+
"k8s.io/cloud-provider-aws/pkg/apis/config/v1alpha1"
2530
)
2631

2732
func TestAzToRegion(t *testing.T) {
@@ -44,3 +49,96 @@ func TestAzToRegion(t *testing.T) {
4449
assert.Equal(t, testCase.region, ret)
4550
}
4651
}
52+
53+
func TestReadAWSCloudConfig(t *testing.T) {
54+
testcases := []struct {
55+
name string
56+
configData string
57+
config *v1alpha1.AWSCloudConfig
58+
expectErr bool
59+
}{
60+
{
61+
name: "config with valid cluster name",
62+
configData: `---
63+
kind: AWSCloudConfig
64+
apiVersion: config.aws.io/v1alpha1
65+
config:
66+
clusterName: test
67+
`,
68+
config: &v1alpha1.AWSCloudConfig{
69+
TypeMeta: metav1.TypeMeta{
70+
Kind: "AWSCloudConfig",
71+
APIVersion: "config.aws.io/v1alpha1",
72+
},
73+
Config: v1alpha1.AWSConfig{
74+
ClusterName: "test",
75+
},
76+
},
77+
},
78+
{
79+
name: "config with empty cluster name",
80+
configData: `---
81+
kind: AWSCloudConfig
82+
apiVersion: config.aws.io/v1alpha1
83+
config:
84+
clusterName: ""
85+
`,
86+
config: &v1alpha1.AWSCloudConfig{
87+
TypeMeta: metav1.TypeMeta{
88+
Kind: "AWSCloudConfig",
89+
APIVersion: "config.aws.io/v1alpha1",
90+
},
91+
Config: v1alpha1.AWSConfig{
92+
ClusterName: "",
93+
},
94+
},
95+
},
96+
{
97+
name: "config with wrong Kind",
98+
configData: `---
99+
kind: WrongCloudConfig
100+
apiVersion: config.aws.io/v1alpha1
101+
config:
102+
clusterName: test
103+
`,
104+
config: nil,
105+
expectErr: true,
106+
},
107+
{
108+
name: "config with wrong apiversion",
109+
configData: `---
110+
kind: AWSCloudConfig
111+
apiVersion: wrong.aws.io/v1alpha1
112+
config:
113+
clusterName: test
114+
`,
115+
config: nil,
116+
expectErr: true,
117+
},
118+
}
119+
120+
for _, testcase := range testcases {
121+
t.Run(testcase.name, func(t *testing.T) {
122+
var buffer bytes.Buffer
123+
_, err := buffer.WriteString(testcase.configData)
124+
if err != nil {
125+
t.Fatal(err)
126+
}
127+
128+
cloudConfig, err := readAWSCloudConfig(&buffer)
129+
if err != nil && !testcase.expectErr {
130+
t.Fatal(err)
131+
}
132+
133+
if err == nil && testcase.expectErr {
134+
t.Error("expected error but got none")
135+
}
136+
137+
if !reflect.DeepEqual(cloudConfig, testcase.config) {
138+
t.Logf("actual cloud config: %#v", cloudConfig)
139+
t.Logf("expected cloud config: %#v", testcase.config)
140+
t.Error("AWS cloud config did not match")
141+
}
142+
})
143+
}
144+
}

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

+32-43
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ limitations under the License.
1616
package v2
1717

1818
import (
19+
"errors"
1920
"fmt"
2021
"strings"
2122
"time"
@@ -30,14 +31,17 @@ import (
3031
const (
3132
// TagNameKubernetesClusterPrefix is the tag name we use to differentiate multiple
3233
// logically independent clusters running in the same AZ.
33-
// tag format: kubernetes.io/cluster/=<clusterName>
34+
// tag format: kubernetes.io/cluster/<clusterID> = shared|owned
35+
// The tag key = TagNameKubernetesClusterPrefix + clusterID
36+
// The tag value is an ownership value
3437
TagNameKubernetesClusterPrefix = "kubernetes.io/cluster/"
3538

3639
// createTag* is configuration of exponential backoff for CreateTag call. We
3740
// retry mainly because if we create an object, we cannot tag it until it is
3841
// "fully created" (eventual consistency). Starting with 1 second, doubling
3942
// it every step and taking 9 steps results in 255 second total waiting
4043
// time.
44+
// TODO: revisit these values
4145
createTagInitialDelay = 1 * time.Second
4246
createTagFactor = 2.0
4347
createTagSteps = 9
@@ -49,6 +53,19 @@ type awsTagging struct {
4953
ClusterName string
5054
}
5155

56+
// newAWSTags is a constructor function for awsTagging
57+
func newAWSTags(clusterName string) (awsTagging, error) {
58+
if clusterName != "" {
59+
klog.Infof("AWS cloud filtering on ClusterName: %v", clusterName)
60+
} else {
61+
return awsTagging{}, errors.New("No ClusterName found in the config")
62+
}
63+
64+
return awsTagging{
65+
ClusterName: clusterName,
66+
}, nil
67+
}
68+
5269
// Extracts the cluster name from the given tags, if they are present
5370
// If duplicate tags are found, returns an error
5471
func findClusterName(tags []*ec2.Tag) (string, error) {
@@ -57,7 +74,7 @@ func findClusterName(tags []*ec2.Tag) (string, error) {
5774
for _, tag := range tags {
5875
tagKey := aws.StringValue(tag.Key)
5976
if strings.HasPrefix(tagKey, TagNameKubernetesClusterPrefix) {
60-
name := aws.StringValue(tag.Value)
77+
name := strings.TrimPrefix(tagKey, TagNameKubernetesClusterPrefix)
6178
if clusterName != "" {
6279
return "", fmt.Errorf("Found multiple cluster tags with prefix %s (%q and %q)", TagNameKubernetesClusterPrefix, clusterName, name)
6380
}
@@ -68,64 +85,36 @@ func findClusterName(tags []*ec2.Tag) (string, error) {
6885
return clusterName, nil
6986
}
7087

71-
func (t *awsTagging) init(clusterName string) error {
72-
t.ClusterName = clusterName
73-
74-
if clusterName != "" {
75-
klog.Infof("AWS cloud filtering on ClusterName: %v", clusterName)
76-
} else {
77-
return fmt.Errorf("AWS cloud failed to find ClusterName")
78-
}
79-
80-
return nil
81-
}
82-
83-
// Extracts a cluster name from the given tags, if one is present
84-
// If no clusterName is found, returns "", nil
85-
// If multiple (different) clusterNames are found, returns an error
86-
func (t *awsTagging) initFromTags(tags []*ec2.Tag) error {
87-
clusterName, err := findClusterName(tags)
88-
if err != nil {
89-
return err
90-
}
91-
92-
if clusterName == "" {
93-
klog.Errorf("Tag %q not found; Kubernetes may behave unexpectedly.", TagNameKubernetesClusterPrefix)
94-
}
95-
96-
return t.init(clusterName)
97-
}
98-
99-
func (t *awsTagging) hasClusterTag(tags []*ec2.Tag) bool {
100-
// if the clusterName is not configured -- we consider all instances.
88+
func (t *awsTagging) hasClusterTag(tags []*ec2.Tag) error {
10189
if len(t.ClusterName) == 0 {
102-
return true
90+
return errors.New("cluster name is not configured (an empty value)")
10391
}
10492

10593
for _, tag := range tags {
10694
tagKey := aws.StringValue(tag.Key)
107-
if (tagKey == TagNameKubernetesClusterPrefix) && (aws.StringValue(tag.Value) == t.ClusterName) {
108-
return true
95+
if tagKey == (TagNameKubernetesClusterPrefix + t.ClusterName) {
96+
return nil
10997
}
11098
}
11199

112-
return false
100+
return errors.New("cluster tag does not exist")
113101
}
114102

115103
func (t *awsTagging) buildTags(additionalTags map[string]string, lifecycle string) map[string]string {
116104
tags := make(map[string]string)
117-
for k, v := range additionalTags {
118-
tags[k] = v
105+
for tagKey, tagValue := range additionalTags {
106+
tags[tagKey] = tagValue
119107
}
120108

121109
// no clusterName is a sign of misconfigured cluster, but we can't be tagging the resources with empty
122110
// strings
111+
// TODO: revise the logic
123112
if len(t.ClusterName) == 0 {
124113
return tags
125114
}
126115

127-
// tag format: kubernetes.io/cluster/=<clusterName>
128-
tags[TagNameKubernetesClusterPrefix] = t.ClusterName
116+
// tag format: kubernetes.io/cluster/<clusterID> = shared|owned
117+
tags[TagNameKubernetesClusterPrefix+t.ClusterName] = lifecycle
129118

130119
return tags
131120
}
@@ -141,10 +130,10 @@ func (t *awsTagging) createTags(ec2Client EC2, resourceID string, lifecycle stri
141130
}
142131

143132
var awsTags []*ec2.Tag
144-
for k, v := range tags {
133+
for tagKey, tagValue := range tags {
145134
tag := &ec2.Tag{
146-
Key: aws.String(k),
147-
Value: aws.String(v),
135+
Key: aws.String(tagKey),
136+
Value: aws.String(tagValue),
148137
}
149138
awsTags = append(awsTags, tag)
150139
}

0 commit comments

Comments
 (0)