Skip to content

Commit

Permalink
Add user tags to AWS load balancers
Browse files Browse the repository at this point in the history 
If the infrastructure object (on AWS) has additional user tags
specified then annotate services of type LoadBlanancer as follows:

 "service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags": "Key1=Value1,KeyN=ValueN"

https://issues.redhat.com/browse/NE-563
  • Loading branch information
@frobware
frobware committed Mar 25, 2021
1 parent f0431d3 commit 7225afa
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 10 deletions.
7 changes: 7 additions & 0 deletions pkg/operator/controller/ingress/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,13 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to get infrastructure 'cluster': %v", err)
}

// DELETE THIS - currently waiting on resolution of https://github.com/openshift/api/pull/864
infraConfig.Spec.PlatformSpec.AWS.UserTags = []configv1.AWSUserTag{{
Key: "NE-563",
Value: "unlocked",
}}

ingressConfig := &configv1.Ingress{}
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, ingressConfig); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to get ingress 'cluster': %v", err)
Expand Down
31 changes: 29 additions & 2 deletions pkg/operator/controller/ingress/load_balancer_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"strconv"
"strings"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
Expand All @@ -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.
//
Expand Down Expand Up @@ -145,7 +153,7 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
if err != nil {
return false, nil, fmt.Errorf("failed to determine if proxy protocol is proxyNeeded for ingresscontroller %q: %v", ci.Name, err)
}
wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platform, proxyNeeded)
wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platform, &infraConfig.Spec.PlatformSpec, proxyNeeded)
if err != nil {
return false, nil, err
}
Expand Down Expand Up @@ -198,7 +206,7 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
// ingresscontroller, or nil if an LB service isn't desired. An LB service is
// desired if the high availability type is Cloud. An LB service will declare an
// owner reference to the given deployment.
func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, proxyNeeded bool) (bool, *corev1.Service, error) {
func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, platformSpec *configv1.PlatformSpec, proxyNeeded bool) (bool, *corev1.Service, error) {
if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType {
return false, nil, nil
}
Expand Down Expand Up @@ -257,6 +265,17 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
} else {
service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalDefault
}

if platformSpec != nil && platformSpec.AWS != nil && len(platformSpec.AWS.UserTags) > 0 {
var additionalTags []string
for _, userTag := range platformSpec.AWS.UserTags {
if len(userTag.Key) > 0 {
additionalTags = append(additionalTags, userTag.Key+"="+userTag.Value)
}
}
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
Expand Down Expand Up @@ -394,6 +413,14 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
changed = true
}

// TODO/TBD(frobware): this can be deleted if we're only going
// to consider user tags at creation time (i.e., no changes
// can be made post that).
if current.Annotations[awsLBAdditionalResourceTags] != expected.Annotations[awsLBAdditionalResourceTags] {
updated.Annotations[awsLBAdditionalResourceTags] = expected.Annotations[awsLBAdditionalResourceTags]
changed = true
}

if !changed {
return false, nil
}
Expand Down
92 changes: 84 additions & 8 deletions pkg/operator/controller/ingress/load_balancer_service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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
platformSpec configv1.PlatformSpec
expectedUserTags string
}{
{
description: "external classic load balancer with scope for aws platform",
Expand All @@ -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,
platformSpec: configv1.PlatformSpec{
AWS: &configv1.AWSPlatformSpec{
UserTags: []configv1.AWSUserTag{{
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",
}},
},
},
expectedUserTags: "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,
Expand Down Expand Up @@ -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,

platformSpec: configv1.PlatformSpec{
AWS: &configv1.AWSPlatformSpec{
UserTags: []configv1.AWSUserTag{{
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",
}},
},
},
expectedUserTags: "network-load-balancer-key-with-value=200,network-load-balancer-key-with-empty-value=",
},
{
description: "nodePort service for aws platform",
platform: configv1.AWSPlatformType,
Expand Down Expand Up @@ -233,7 +291,7 @@ func TestDesiredLoadBalancerService(t *testing.T) {
t.Errorf("test %q failed; expected IsProxyProtocolNeeded to return %v, got %v", tc.description, tc.proxyNeeded, proxyNeeded)
}

haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, proxyNeeded)
haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, &tc.platformSpec, proxyNeeded)
switch {
case err != nil:
t.Errorf("test %q failed; unexpected error from desiredLoadBalancerService for endpoint publishing strategy type %v: %v", tc.description, tc.strategyType, err)
Expand Down Expand Up @@ -265,13 +323,31 @@ func TestDesiredLoadBalancerService(t *testing.T) {
classicLB := tc.lbStrategy.ProviderParameters == nil || tc.lbStrategy.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer
switch {
case classicLB:
if len(tc.expectedUserTags) > 0 {
if err := checkServiceHasAnnotation(svc, awsLBAdditionalResourceTags, true, tc.expectedUserTags); 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.expectedUserTags) > 0 {
if err := checkServiceHasAnnotation(svc, awsLBAdditionalResourceTags, true, tc.expectedUserTags); 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)
}
Expand Down

0 comments on commit 7225afa

Please sign in to comment.