Bug 1950284: Annotate services of type LoadBalancer with user tags (AWS only)

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/go-logr/zapr v0.4.0
 	github.com/google/go-cmp v0.5.2
 	github.com/kevinburke/go-bindata v3.11.0+incompatible
-	github.com/openshift/api v0.0.0-20210405165116-47be53705a13
+	github.com/openshift/api v0.0.0-20210415190711-38058be7d6ef
 	github.com/openshift/build-machinery-go v0.0.0-20210409131504-b1828cc0cdad
 	github.com/openshift/library-go v0.0.0-20210331235027-66936e2fcc52
 	github.com/pkg/errors v0.9.1

go.sum

@@ -481,8 +481,8 @@ github.com/opencontainers/go-digest v1.0.0-rc1/go.mod h1:cMLVZDEM3+U2I4VmLI6N8jQ
 github.com/opencontainers/image-spec v1.0.1/go.mod h1:BtxoFyWECRxE4U/7sNtV5W15zMzWCbyJoFRP3s7yZA0=
 github.com/openshift/api v0.0.0-20210331162552-3e31249e6a55/go.mod h1:dZ4kytOo3svxJHNYd0J55hwe/6IQG5gAUHUE0F3Jkio=
 github.com/openshift/api v0.0.0-20210331193751-3acddb19d360/go.mod h1:dZ4kytOo3svxJHNYd0J55hwe/6IQG5gAUHUE0F3Jkio=
-github.com/openshift/api v0.0.0-20210405165116-47be53705a13 h1:/om0/+5m4SY41YngmEjV53cqeamZ2BE5Qfz7QCXA8BQ=
-github.com/openshift/api v0.0.0-20210405165116-47be53705a13/go.mod h1:dZ4kytOo3svxJHNYd0J55hwe/6IQG5gAUHUE0F3Jkio=
+github.com/openshift/api v0.0.0-20210415190711-38058be7d6ef h1:fRI8eMn+jg3SwrA3tnrKqbYS1EVESlehVzghhQ2Oyl4=
+github.com/openshift/api v0.0.0-20210415190711-38058be7d6ef/go.mod h1:dZ4kytOo3svxJHNYd0J55hwe/6IQG5gAUHUE0F3Jkio=
 github.com/openshift/build-machinery-go v0.0.0-20210209125900-0da259a2c359/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=
 github.com/openshift/build-machinery-go v0.0.0-20210409131504-b1828cc0cdad h1:3sJkKZdEapxFstGRRycvL1VPJ1gT5/fpfV//KejKmzQ=
 github.com/openshift/build-machinery-go v0.0.0-20210409131504-b1828cc0cdad/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=

pkg/operator/controller/ingress/load_balancer_service.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"strconv"
+	"strings"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
@@ -25,6 +26,13 @@ import (
 )
 
 const (
+	// awsLBAdditionalResourceTags is a comma separated list of
+	// Key=Value pairs that are additionally recorded on
+	// load balancer resources and security groups.
+	//
+	// https://kubernetes.io/docs/concepts/services-networking/service/#aws-load-balancer-additional-resource-tags
+	awsLBAdditionalResourceTags = "service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags"
+
 	// awsLBProxyProtocolAnnotation is used to enable the PROXY protocol on any
 	// AWS load balancer services created.
 	//
@@ -261,6 +269,19 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 			} else {
 				service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalDefault
 			}
+
+			if platform.AWS != nil && len(platform.AWS.ResourceTags) > 0 {
+				var additionalTags []string
+				for _, userTag := range platform.AWS.ResourceTags {
+					if len(userTag.Key) > 0 {
+						additionalTags = append(additionalTags, userTag.Key+"="+userTag.Value)
+					}
+				}
+				if len(additionalTags) > 0 {
+					service.Annotations[awsLBAdditionalResourceTags] = strings.Join(additionalTags, ",")
+				}
+			}
+
 			// Set the load balancer for AWS to be as aggressive as Azure (2 fail @ 5s interval, 2 healthy)
 			service.Annotations[awsLBHealthCheckTimeoutAnnotation] = awsLBHealthCheckTimeoutDefault
 			service.Annotations[awsLBHealthCheckUnhealthyThresholdAnnotation] = awsLBHealthCheckUnhealthyThresholdDefault

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -2,9 +2,10 @@ package ingress
 
 import (
 	"fmt"
-	corev1 "k8s.io/api/core/v1"
 	"testing"
 
+	corev1 "k8s.io/api/core/v1"
+
 	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 
@@ -14,12 +15,14 @@ import (
 
 func TestDesiredLoadBalancerService(t *testing.T) {
 	testCases := []struct {
-		description  string
-		platform     configv1.PlatformType
-		strategyType operatorv1.EndpointPublishingStrategyType
-		lbStrategy   operatorv1.LoadBalancerStrategy
-		proxyNeeded  bool
-		expect       bool
+		description          string
+		platform             configv1.PlatformType
+		strategyType         operatorv1.EndpointPublishingStrategyType
+		lbStrategy           operatorv1.LoadBalancerStrategy
+		proxyNeeded          bool
+		expect               bool
+		platformStatus       configv1.PlatformStatus
+		expectedResourceTags string
 	}{
 		{
 			description:  "external classic load balancer with scope for aws platform",
@@ -31,6 +34,30 @@ func TestDesiredLoadBalancerService(t *testing.T) {
 			proxyNeeded: true,
 			expect:      true,
 		},
+		{
+			description:  "external classic load balancer with scope for aws platform and custom user tags",
+			platform:     configv1.AWSPlatformType,
+			strategyType: operatorv1.LoadBalancerServiceStrategyType,
+			lbStrategy: operatorv1.LoadBalancerStrategy{
+				Scope: operatorv1.ExternalLoadBalancer,
+			},
+			proxyNeeded: true,
+			expect:      true,
+			platformStatus: configv1.PlatformStatus{
+				AWS: &configv1.AWSPlatformStatus{
+					ResourceTags: []configv1.AWSResourceTag{{
+						Key:   "classic-load-balancer-key-with-value",
+						Value: "100",
+					}, {
+						Key:   "classic-load-balancer-key-with-empty-value",
+						Value: "",
+					}, {
+						Value: "classic-load-balancer-value-without-key",
+					}},
+				},
+			},
+			expectedResourceTags: "classic-load-balancer-key-with-value=100,classic-load-balancer-key-with-empty-value=",
+		},
 		{
 			description:  "external classic load balancer without scope for aws platform",
 			platform:     configv1.AWSPlatformType,
@@ -86,6 +113,37 @@ func TestDesiredLoadBalancerService(t *testing.T) {
 			proxyNeeded: false,
 			expect:      true,
 		},
+		{
+			description:  "external network load balancer with scope for aws platform and custom user tags",
+			platform:     configv1.AWSPlatformType,
+			strategyType: operatorv1.LoadBalancerServiceStrategyType,
+			lbStrategy: operatorv1.LoadBalancerStrategy{
+				Scope: operatorv1.ExternalLoadBalancer,
+				ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
+					Type: operatorv1.AWSLoadBalancerProvider,
+					AWS: &operatorv1.AWSLoadBalancerParameters{
+						Type: operatorv1.AWSNetworkLoadBalancer,
+					},
+				},
+			},
+			proxyNeeded: false,
+			expect:      true,
+
+			platformStatus: configv1.PlatformStatus{
+				AWS: &configv1.AWSPlatformStatus{
+					ResourceTags: []configv1.AWSResourceTag{{
+						Key:   "network-load-balancer-key-with-value",
+						Value: "200",
+					}, {
+						Key:   "network-load-balancer-key-with-empty-value",
+						Value: "",
+					}, {
+						Value: "network-load-balancer-value-without-key",
+					}},
+				},
+			},
+			expectedResourceTags: "network-load-balancer-key-with-value=200,network-load-balancer-key-with-empty-value=",
+		},
 		{
 			description:  "nodePort service for aws platform",
 			platform:     configv1.AWSPlatformType,
@@ -219,11 +277,10 @@ func TestDesiredLoadBalancerService(t *testing.T) {
 		}
 		infraConfig := &configv1.Infrastructure{
 			Status: configv1.InfrastructureStatus{
-				PlatformStatus: &configv1.PlatformStatus{
-					Type: tc.platform,
-				},
+				PlatformStatus: &tc.platformStatus,
 			},
 		}
+		infraConfig.Status.PlatformStatus.Type = tc.platform
 
 		proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus)
 		switch {
@@ -265,13 +322,31 @@ func TestDesiredLoadBalancerService(t *testing.T) {
 				classicLB := tc.lbStrategy.ProviderParameters == nil || tc.lbStrategy.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer
 				switch {
 				case classicLB:
+					if len(tc.expectedResourceTags) > 0 {
+						if err := checkServiceHasAnnotation(svc, awsLBAdditionalResourceTags, true, tc.expectedResourceTags); err != nil {
+							t.Errorf("annotation check for test %q failed: %v, unexpected value", tc.description, err)
+						}
+					} else {
+						if err := checkServiceHasAnnotation(svc, awsLBAdditionalResourceTags, false, ""); err == nil {
+							t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, awsLBAdditionalResourceTags)
+						}
+					}
 					if err := checkServiceHasAnnotation(svc, awsLBHealthCheckIntervalAnnotation, true, awsLBHealthCheckIntervalDefault); err != nil {
 						t.Errorf("annotation check for test %q failed: %v", tc.description, err)
 					}
 					if err := checkServiceHasAnnotation(svc, awsLBProxyProtocolAnnotation, true, "*"); err != nil {
 						t.Errorf("annotation check for test %q failed: %v", tc.description, err)
 					}
 				case tc.lbStrategy.ProviderParameters.AWS.Type == operatorv1.AWSNetworkLoadBalancer:
+					if len(tc.expectedResourceTags) > 0 {
+						if err := checkServiceHasAnnotation(svc, awsLBAdditionalResourceTags, true, tc.expectedResourceTags); err != nil {
+							t.Errorf("annotation check for test %q failed: %v, unexpected value", tc.description, err)
+						}
+					} else {
+						if err := checkServiceHasAnnotation(svc, awsLBAdditionalResourceTags, false, ""); err == nil {
+							t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, awsLBAdditionalResourceTags)
+						}
+					}
 					if err := checkServiceHasAnnotation(svc, awsLBHealthCheckIntervalAnnotation, true, awsLBHealthCheckIntervalNLB); err != nil {
 						t.Errorf("annotation check for test %q failed: %v", tc.description, err)
 					}