Merge pull request #421 from kubernetes-sigs/annotation-cleanup

Normalized load balancer attributes annotation

Author: bigkraig

docs/ingress-resources.md

@@ -51,7 +51,7 @@ Required annotations are:
 ### Optional Annotations
 
 ```
-alb.ingress.kubernetes.io/attributes
+alb.ingress.kubernetes.io/load-balancer-attributes
 alb.ingress.kubernetes.io/backend-protocol
 alb.ingress.kubernetes.io/certificate-arn
 alb.ingress.kubernetes.io/connection-idle-timeout
@@ -75,7 +75,7 @@ alb.ingress.kubernetes.io/ssl-policy
 
 Optional annotations are:
 
-- **attributes**: Defines [Load Balancer Attributes](http://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/API_LoadBalancerAttribute.html) that should be applied to the ALB. This can be used to enable the S3 access logs feature of the ALB. Example: `alb.ingress.kubernetes.io/attributes: access_logs.s3.enabled=true,access_logs.s3.bucket=my-access-log-bucket`
+- **load-balancer-attributes**: Defines [Load Balancer Attributes](http://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/API_LoadBalancerAttribute.html) that should be applied to the ALB. This can be used to enable the S3 access logs feature of the ALB. Example: `alb.ingress.kubernetes.io/attributes: access_logs.s3.enabled=true,access_logs.s3.bucket=my-access-log-bucket`
 
 - **backend-protocol**: Enables selection of protocol for ALB to use to connect to backend service. When omitted, `HTTP` is used.
 

pkg/alb/lb/loadbalancer.go

@@ -46,7 +46,7 @@ func NewDesiredLoadBalancer(o *NewDesiredLoadBalancerOptions) (newLoadBalancer *
 
 	newLoadBalancer = &LoadBalancer{
 		id:         name,
-		attributes: attributes{desired: o.Annotations.Attributes},
+		attributes: attributes{desired: o.Annotations.LoadBalancerAttributes},
 		tags:       tags{desired: o.Tags},
 		options: options{
 			desired: opts{

pkg/alb/lb/loadbalancer_test.go

@@ -1,7 +1,6 @@
 package lb
 
 import (
-	"fmt"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -96,8 +95,7 @@ func TestNewDesiredLoadBalancer(t *testing.T) {
 	}
 
 	expectedID := createLBName(namespace, ingressName, clusterName)
-	l, err := NewDesiredLoadBalancer(lbOpts)
-	fmt.Println(err)
+	l, _ := NewDesiredLoadBalancer(lbOpts)
 
 	key1, _ := l.tags.desired.Get(tag1Key)
 	switch {

pkg/annotations/annotations.go

@@ -38,6 +38,8 @@ const (
 	healthyThresholdCountKey      = "alb.ingress.kubernetes.io/healthy-threshold-count"
 	unhealthyThresholdCountKey    = "alb.ingress.kubernetes.io/unhealthy-threshold-count"
 	inboundCidrsKey               = "alb.ingress.kubernetes.io/security-group-inbound-cidrs"
+	loadbalancerAttributesKey     = "alb.ingress.kubernetes.io/load-balancer-attributes"
+	loadbalancerAttributesAltKey  = "alb.ingress.kubernetes.io/attributes"
 	portKey                       = "alb.ingress.kubernetes.io/listen-ports"
 	schemeKey                     = "alb.ingress.kubernetes.io/scheme"
 	sslPolicyKey                  = "alb.ingress.kubernetes.io/ssl-policy"
@@ -53,7 +55,6 @@ const (
 	clusterTagValue               = "shared"
 	albRoleTagKey                 = "tag:kubernetes.io/role/alb-ingress"
 	albManagedSubnetsCacheKey     = "alb-managed-subnets"
-	attributesKey                 = "alb.ingress.kubernetes.io/attributes"
 )
 
 // Annotations contains all of the annotation configuration for an ingress
@@ -81,7 +82,7 @@ type Annotations struct {
 	TargetGroupAttributes      albelbv2.TargetGroupAttributes
 	SslPolicy                  *string
 	VPCID                      *string
-	Attributes                 []*elbv2.LoadBalancerAttribute
+	LoadBalancerAttributes     []*elbv2.LoadBalancerAttribute
 }
 
 type PortData struct {
@@ -147,7 +148,7 @@ func (vf ValidatingAnnotationFactory) ParseAnnotations(ingress *extensions.Ingre
 		a.setTags(annotations),
 		a.setIgnoreHostHeader(annotations),
 		a.setWafACLID(annotations, vf.validator),
-		a.setAttributes(annotations),
+		a.setLoadBalancerAttributes(annotations),
 		a.setTargetGroupAttributes(annotations),
 		a.setSslPolicy(annotations, vf.validator),
 	} {
@@ -159,10 +160,15 @@ func (vf ValidatingAnnotationFactory) ParseAnnotations(ingress *extensions.Ingre
 	return a, nil
 }
 
-func (a *Annotations) setAttributes(annotations map[string]string) error {
+func (a *Annotations) setLoadBalancerAttributes(annotations map[string]string) error {
 	var attrs []*elbv2.LoadBalancerAttribute
 	var badAttrs []string
-	rawAttrs := util.NewAWSStringSlice(annotations[attributesKey])
+	v, ok := annotations[loadbalancerAttributesKey]
+	if !ok {
+		v = annotations[loadbalancerAttributesAltKey]
+	}
+
+	rawAttrs := util.NewAWSStringSlice(v)
 
 	for _, rawAttr := range rawAttrs {
 		parts := strings.Split(*rawAttr, "=")
@@ -178,7 +184,7 @@ func (a *Annotations) setAttributes(annotations map[string]string) error {
 			Value: aws.String(parts[1]),
 		})
 	}
-	a.Attributes = attrs
+	a.LoadBalancerAttributes = attrs
 
 	if len(badAttrs) > 0 {
 		return fmt.Errorf("Unable to parse `%s` into Key=Value pair(s)", strings.Join(badAttrs, ", "))

pkg/annotations/annotations_test.go

@@ -198,23 +198,99 @@ func TestConnectionIdleTimeoutValidation(t *testing.T) {
 	}
 }
 
-func TestSetAttributesAsList(t *testing.T) {
-	annotations := &Annotations{}
-	expected := elbv2.LoadBalancerAttribute{}
-	expected.SetKey("access_logs.s3.enabled")
-	expected.SetValue("true")
-
-	attributes := map[string]string{attributesKey: "access_logs.s3.enabled=true"}
-	err := annotations.setAttributes(attributes)
-
-	if err != nil || len(annotations.Attributes) != 1 {
-		t.Errorf("setAttributes - number of attributes incorrect")
+func TestSetLoadBalancerAttributes(t *testing.T) {
+	var tests = []struct {
+		annotations map[string]string
+		expected    []elbv2.LoadBalancerAttribute
+		length      int
+		pass        bool
+	}{
+		{
+			map[string]string{loadbalancerAttributesKey: "access_logs.s3.enabled=true"},
+			func() []elbv2.LoadBalancerAttribute {
+				e := elbv2.LoadBalancerAttribute{}
+				e.SetKey("access_logs.s3.enabled")
+				e.SetValue("true")
+				return []elbv2.LoadBalancerAttribute{e}
+			}(),
+			1,
+			true,
+		},
+		{
+			map[string]string{loadbalancerAttributesAltKey: "access_logs.s3.enabled=true"},
+			func() []elbv2.LoadBalancerAttribute {
+				e := elbv2.LoadBalancerAttribute{}
+				e.SetKey("access_logs.s3.enabled")
+				e.SetValue("true")
+				return []elbv2.LoadBalancerAttribute{e}
+			}(),
+			1,
+			true,
+		},
+		{
+			map[string]string{
+				loadbalancerAttributesKey:    "access_logs.s3.enabled=true",
+				loadbalancerAttributesAltKey: "deletion_protection.enabled=true",
+			},
+			func() []elbv2.LoadBalancerAttribute {
+				e := elbv2.LoadBalancerAttribute{}
+				e.SetKey("access_logs.s3.enabled")
+				e.SetValue("true")
+				return []elbv2.LoadBalancerAttribute{e}
+			}(),
+			1,
+			true,
+		},
+		{
+			map[string]string{loadbalancerAttributesKey: "access_logs.s3.enabled=true,deletion_protection.enabled=true"},
+			func() (v []elbv2.LoadBalancerAttribute) {
+				e := elbv2.LoadBalancerAttribute{}
+				e.SetKey("access_logs.s3.enabled")
+				e.SetValue("true")
+				v = append(v, e)
+				e = elbv2.LoadBalancerAttribute{}
+				e.SetKey("deletion_protection.enabled")
+				e.SetValue("true")
+				v = append(v, e)
+				return v
+			}(),
+			2,
+			true,
+		},
+		{
+			map[string]string{loadbalancerAttributesKey: "access_logs.s3.enabled=false"},
+			func() []elbv2.LoadBalancerAttribute {
+				e := elbv2.LoadBalancerAttribute{}
+				e.SetKey("access_logs.s3.enabled")
+				e.SetValue("true")
+				return []elbv2.LoadBalancerAttribute{e}
+			}(),
+			1,
+			false,
+		},
 	}
+	for v, tt := range tests {
+		a := &Annotations{}
 
-	actual := annotations.Attributes[0]
-
-	if err == nil && *actual.Key != *expected.Key || *actual.Value != *expected.Value {
-		t.Errorf("setAttributes - values did not match")
+		err := a.setLoadBalancerAttributes(tt.annotations)
+		if err != nil && tt.pass {
+			t.Errorf("setLoadBalancerAttributes(%v): expected %v, errored: %v", tt.annotations, tt.expected, err)
+		}
+		if err != nil && !tt.pass {
+			t.Errorf("setLoadBalancerAttributes(%v): should have errored", tt.annotations)
+		}
+		if err == nil && len(a.LoadBalancerAttributes) != tt.length {
+			t.Errorf("setLoadBalancerAttributes(%v): expected %v attributes, actual %v", tt.annotations, tt.length, len(a.LoadBalancerAttributes))
+			continue
+		}
+		for i := range a.LoadBalancerAttributes {
+			if tt.pass && (*a.LoadBalancerAttributes[i].Key != *tt.expected[i].Key ||
+				*a.LoadBalancerAttributes[i].Value != *tt.expected[i].Value) {
+				t.Errorf("setLoadBalancerAttributes(%v): [test %v, attribute %v] passed but did not match (%v != %v) or (%v != %v)",
+					tt.annotations, v, i, *a.LoadBalancerAttributes[i].Key, *tt.expected[i].Key,
+					*a.LoadBalancerAttributes[i].Value, *tt.expected[i].Value)
+			}
+		}
 	}
 }