Merge pull request #804 from M00nF1sh/issue-655

fix issue-655

Author: k8s-ci-robot

internal/alb/lb/attributes.go

@@ -156,29 +156,32 @@ func (c *attributesController) Reconcile(ctx context.Context, lbArn string, attr
 }
 
 // attributesChangeSet returns a list of elbv2.LoadBalancerAttribute required to change a into b
-func attributesChangeSet(a, b *Attributes) (changeSet []*elbv2.LoadBalancerAttribute) {
-	if a.DeletionProtectionEnabled != b.DeletionProtectionEnabled {
-		changeSet = append(changeSet, lbAttribute(DeletionProtectionEnabledKey, fmt.Sprintf("%v", b.DeletionProtectionEnabled)))
+func attributesChangeSet(current, desired *Attributes) (changeSet []*elbv2.LoadBalancerAttribute) {
+	if current.DeletionProtectionEnabled != desired.DeletionProtectionEnabled {
+		changeSet = append(changeSet, lbAttribute(DeletionProtectionEnabledKey, fmt.Sprintf("%v", desired.DeletionProtectionEnabled)))
 	}
 
-	if a.AccessLogsS3Enabled != b.AccessLogsS3Enabled {
-		changeSet = append(changeSet, lbAttribute(AccessLogsS3EnabledKey, fmt.Sprintf("%v", b.AccessLogsS3Enabled)))
+	if current.AccessLogsS3Enabled != desired.AccessLogsS3Enabled {
+		changeSet = append(changeSet, lbAttribute(AccessLogsS3EnabledKey, fmt.Sprintf("%v", desired.AccessLogsS3Enabled)))
 	}
 
-	if a.AccessLogsS3Bucket != b.AccessLogsS3Bucket {
-		changeSet = append(changeSet, lbAttribute(AccessLogsS3BucketKey, b.AccessLogsS3Bucket))
-	}
+	// ELBV2 API forbids us to set bucket to an empty bucket, so we keep it unchanged if AccessLogsS3Enabled==false.
+	if desired.AccessLogsS3Enabled {
+		if current.AccessLogsS3Bucket != desired.AccessLogsS3Bucket {
+			changeSet = append(changeSet, lbAttribute(AccessLogsS3BucketKey, desired.AccessLogsS3Bucket))
+		}
 
-	if a.AccessLogsS3Prefix != b.AccessLogsS3Prefix {
-		changeSet = append(changeSet, lbAttribute(AccessLogsS3PrefixKey, b.AccessLogsS3Prefix))
+		if current.AccessLogsS3Prefix != desired.AccessLogsS3Prefix {
+			changeSet = append(changeSet, lbAttribute(AccessLogsS3PrefixKey, desired.AccessLogsS3Prefix))
+		}
 	}
 
-	if a.IdleTimeoutTimeoutSeconds != b.IdleTimeoutTimeoutSeconds {
-		changeSet = append(changeSet, lbAttribute(IdleTimeoutTimeoutSecondsKey, fmt.Sprintf("%v", b.IdleTimeoutTimeoutSeconds)))
+	if current.IdleTimeoutTimeoutSeconds != desired.IdleTimeoutTimeoutSeconds {
+		changeSet = append(changeSet, lbAttribute(IdleTimeoutTimeoutSecondsKey, fmt.Sprintf("%v", desired.IdleTimeoutTimeoutSeconds)))
 	}
 
-	if a.RoutingHTTP2Enabled != b.RoutingHTTP2Enabled {
-		changeSet = append(changeSet, lbAttribute(RoutingHTTP2EnabledKey, fmt.Sprintf("%v", b.RoutingHTTP2Enabled)))
+	if current.RoutingHTTP2Enabled != desired.RoutingHTTP2Enabled {
+		changeSet = append(changeSet, lbAttribute(RoutingHTTP2EnabledKey, fmt.Sprintf("%v", desired.RoutingHTTP2Enabled)))
 	}
 
 	return

internal/alb/lb/attributes_test.go

@@ -114,17 +114,6 @@ func Test_attributesChangeSet(t *testing.T) {
 		b         *Attributes
 		changeSet []*elbv2.LoadBalancerAttribute
 	}{
-		{
-			name: "a and b contain a default AccessLogsS3Bucket value, expect no change",
-			a:    MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3BucketKey, "true")}),
-			b:    MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3BucketKey, "true")}),
-		},
-		{
-			name:      "a contains a non-default AccessLogsS3Bucket, b contains default, change to default",
-			a:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3BucketKey, "some bucket")}),
-			b:         MustNewAttributes(nil),
-			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3BucketKey, "")},
-		},
 		{
 			name: "a and b contain empty defaults, no change",
 			a:    MustNewAttributes(nil),
@@ -137,22 +126,22 @@ func Test_attributesChangeSet(t *testing.T) {
 			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(DeletionProtectionEnabledKey, "true")},
 		},
 		{
-			name:      fmt.Sprintf("a contains default, b contains non-default AccessLogsS3EnabledKey, make a change"),
-			a:         MustNewAttributes(nil),
-			b:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "true")}),
-			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "true")},
+			name:      fmt.Sprintf("enable AccessLogS3"),
+			a:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "false"), lbAttribute(AccessLogsS3BucketKey, ""), lbAttribute(AccessLogsS3PrefixKey, "")}),
+			b:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "true"), lbAttribute(AccessLogsS3BucketKey, "bucket"), lbAttribute(AccessLogsS3PrefixKey, "prefix")}),
+			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "true"), lbAttribute(AccessLogsS3BucketKey, "bucket"), lbAttribute(AccessLogsS3PrefixKey, "prefix")},
 		},
 		{
-			name:      fmt.Sprintf("a contains default, b contains non-default AccessLogsS3BucketKey, make a change"),
-			a:         MustNewAttributes(nil),
-			b:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3BucketKey, "some bucket")}),
-			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3BucketKey, "some bucket")},
+			name:      fmt.Sprintf("disable AccessLogS3, don't change bucket/prefix when it's unchanged."),
+			a:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "true"), lbAttribute(AccessLogsS3BucketKey, "bucket"), lbAttribute(AccessLogsS3PrefixKey, "prefix")}),
+			b:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "false"), lbAttribute(AccessLogsS3BucketKey, "bucket"), lbAttribute(AccessLogsS3PrefixKey, "prefix")}),
+			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "false")},
 		},
 		{
-			name:      fmt.Sprintf("a contains default, b contains non-default AccessLogsS3PrefixKey, make a change"),
-			a:         MustNewAttributes(nil),
-			b:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3PrefixKey, "some prefix")}),
-			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3PrefixKey, "some prefix")},
+			name:      fmt.Sprintf("disable AccessLogS3, don't change bucket/prefix when it's changed"),
+			a:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "true"), lbAttribute(AccessLogsS3BucketKey, "bucket"), lbAttribute(AccessLogsS3PrefixKey, "prefix")}),
+			b:         MustNewAttributes([]*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "false"), lbAttribute(AccessLogsS3BucketKey, ""), lbAttribute(AccessLogsS3PrefixKey, "")}),
+			changeSet: []*elbv2.LoadBalancerAttribute{lbAttribute(AccessLogsS3EnabledKey, "false")},
 		},
 		{
 			name:      fmt.Sprintf("a contains default, b contains non-default IdleTimeoutTimeoutSecondsKey, make a change"),