Merge pull request #55731 from georgebuckerfield/elb-tagging

Automatic merge from submit-queue (batch tested with PRs 50457, 55558, 53483, 55731, 52842). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Ensure new tags are created on existing ELBs

**What this PR does / why we need it**:

When editing an existing service of type LoadBalancer in an AWS environment and adding the `service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags` annotation, you would expect the new tags to be set on the load balancer, however this doesn't happen currently. The annotation only takes effect if specified when the service is created.

This PR adds an AddTags method to the ELB interface and uses this to ensure tags set in the annotation are present on the ELB. If the tag key is already present, the value will be updated.

This PR does not remove tags that have been removed from the annotation, it only add/updates tags.

**Which issue(s) this PR fixes**:
Fixes #54642 

**Special notes for your reviewer**:
The change requires that the IAM policy of the master instance(s) has the `elasticloadbalancing:AddTags` permission.

**Release note**:

```release-note
Ensure additional resource tags are set/updated AWS load balancers
```

pkg/cloudprovider/providers/aws/aws.go

@@ -271,6 +271,7 @@ type ELB interface {
 	CreateLoadBalancer(*elb.CreateLoadBalancerInput) (*elb.CreateLoadBalancerOutput, error)
 	DeleteLoadBalancer(*elb.DeleteLoadBalancerInput) (*elb.DeleteLoadBalancerOutput, error)
 	DescribeLoadBalancers(*elb.DescribeLoadBalancersInput) (*elb.DescribeLoadBalancersOutput, error)
+	AddTags(*elb.AddTagsInput) (*elb.AddTagsOutput, error)
 	RegisterInstancesWithLoadBalancer(*elb.RegisterInstancesWithLoadBalancerInput) (*elb.RegisterInstancesWithLoadBalancerOutput, error)
 	DeregisterInstancesFromLoadBalancer(*elb.DeregisterInstancesFromLoadBalancerInput) (*elb.DeregisterInstancesFromLoadBalancerOutput, error)
 	CreateLoadBalancerPolicy(*elb.CreateLoadBalancerPolicyInput) (*elb.CreateLoadBalancerPolicyOutput, error)
@@ -2248,6 +2249,27 @@ func (c *Cloud) describeLoadBalancer(name string) (*elb.LoadBalancerDescription,
 	return ret, nil
 }
 
+func (c *Cloud) addLoadBalancerTags(loadBalancerName string, requested map[string]string) error {
+	var tags []*elb.Tag
+	for k, v := range requested {
+		tag := &elb.Tag{
+			Key:   aws.String(k),
+			Value: aws.String(v),
+		}
+		tags = append(tags, tag)
+	}
+
+	request := &elb.AddTagsInput{}
+	request.LoadBalancerNames = []*string{&loadBalancerName}
+	request.Tags = tags
+
+	_, err := c.elb.AddTags(request)
+	if err != nil {
+		return fmt.Errorf("error adding tags to load balancer: %v", err)
+	}
+	return nil
+}
+
 // Retrieves instance's vpc id from metadata
 func (c *Cloud) findVPCID() (string, error) {
 	macs, err := c.metadata.GetMetadata("network/interfaces/macs/")

pkg/cloudprovider/providers/aws/aws_fakes.go

@@ -312,6 +312,10 @@ func (elb *FakeELB) DescribeLoadBalancers(input *elb.DescribeLoadBalancersInput)
 	panic("Not implemented")
 }
 
+func (elb *FakeELB) AddTags(input *elb.AddTagsInput) (*elb.AddTagsOutput, error) {
+	panic("Not implemented")
+}
+
 func (elb *FakeELB) RegisterInstancesWithLoadBalancer(*elb.RegisterInstancesWithLoadBalancerInput) (*elb.RegisterInstancesWithLoadBalancerOutput, error) {
 	panic("Not implemented")
 }

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

@@ -317,6 +317,18 @@ func (c *Cloud) ensureLoadBalancer(namespacedName types.NamespacedName, loadBala
 				}
 			}
 		}
+
+		{
+			// Add additional tags
+			glog.V(2).Infof("Creating additional load balancer tags for %s", loadBalancerName)
+			tags := getLoadBalancerAdditionalTags(annotations)
+			if len(tags) > 0 {
+				err := c.addLoadBalancerTags(loadBalancerName, tags)
+				if err != nil {
+					return nil, fmt.Errorf("unable to create additional load balancer tags: %v", err)
+				}
+			}
+		}
 	}
 
 	// Whether the ELB was new or existing, sync attributes regardless. This accounts for things

pkg/cloudprovider/providers/aws/aws_test.go

@@ -82,6 +82,11 @@ func (m *MockedFakeELB) expectDescribeLoadBalancers(loadBalancerName string) {
 	})
 }
 
+func (m *MockedFakeELB) AddTags(input *elb.AddTagsInput) (*elb.AddTagsOutput, error) {
+	args := m.Called(input)
+	return args.Get(0).(*elb.AddTagsOutput), nil
+}
+
 func TestReadAWSCloudConfig(t *testing.T) {
 	tests := []struct {
 		name string
@@ -1130,6 +1135,31 @@ func TestLBExtraSecurityGroupsAnnotation(t *testing.T) {
 	}
 }
 
+// Test that we can add a load balancer tag
+func TestAddLoadBalancerTags(t *testing.T) {
+	loadBalancerName := "test-elb"
+	awsServices := newMockedFakeAWSServices(TestClusterId)
+	c, _ := newAWSCloud(strings.NewReader("[global]"), awsServices)
+
+	want := make(map[string]string)
+	want["tag1"] = "val1"
+
+	expectedAddTagsRequest := &elb.AddTagsInput{
+		LoadBalancerNames: []*string{&loadBalancerName},
+		Tags: []*elb.Tag{
+			{
+				Key:   aws.String("tag1"),
+				Value: aws.String("val1"),
+			},
+		},
+	}
+	awsServices.elb.(*MockedFakeELB).On("AddTags", expectedAddTagsRequest).Return(&elb.AddTagsOutput{})
+
+	err := c.addLoadBalancerTags(loadBalancerName, want)
+	assert.Nil(t, err, "Error adding load balancer tags: %v", err)
+	awsServices.elb.(*MockedFakeELB).AssertExpectations(t)
+}
+
 func newMockedFakeAWSServices(id string) *FakeAWSServices {
 	s := NewFakeAWSServices(id)
 	s.ec2 = &MockedFakeEC2{FakeEC2Impl: s.ec2.(*FakeEC2Impl)}