Fix handling limit of certificates per ALB

This fixes a couple of bugs related to exceeding the maximum number of certificates per ALB. It limits the max size to 24 instead of 25. This is done because we need to duplicate the default certificate to work around a CloudFormation bug (#162) and therefore need one extra space for this limiting the maximum unique certificates per ALB to 24 instead of the AWS limit of 25. It also fixes a bug in `AddIngress` which could potentially add some of the certificates for a single ingress to a stack and the rest to another stack resulting in an undesired state. Thirdly it adds rollback complete states to `IsComplete()` to automatically attempt to update stacks that are in a rollback complete state. Lastly it removes some of the getter methods from structs and instead exports the fields that are needed in other packages. This was done to make it easier to use the structs in tests and because we are not writing Java. Fix #176, #175 Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net>
zalando-incubator · Jun 29, 2018 · 203463a · 203463a
1 parent 66fd7c7
commit 203463a
Show file tree

Hide file tree

Showing 10 changed files with 260 additions and 208 deletions.
diff --git a/aws/adapter.go b/aws/adapter.go
@@ -75,7 +75,11 @@ const (
 	DefaultStackTTL                  = 5 * time.Minute
 	DefaultIdleConnectionTimeout     = 1 * time.Minute
 	DefaultControllerID              = "kube-ingress-aws-controller"
-	DefaultMaxCertsPerALB            = 25
+	// DefaultMaxCertsPerALB defines the maximum number of certificates per
+	// ALB. AWS limit is 25 but one space is needed to work around
+	// CloudFormation bug:
+	// https://github.com/zalando-incubator/kube-ingress-aws-controller/pull/162
+	DefaultMaxCertsPerALB = 24
 
 	nameTag = "Name"
 
@@ -317,7 +321,7 @@ func (a *Adapter) FindManagedStacks() ([]*Stack, error) {
 func (a *Adapter) UpdateTargetGroupsAndAutoScalingGroups(stacks []*Stack) {
 	targetGroupARNs := make([]string, len(stacks))
 	for i, stack := range stacks {
-		targetGroupARNs[i] = stack.targetGroupARN
+		targetGroupARNs[i] = stack.TargetGroupARN
 	}
 
 	// don't do anything if there are no target groups
@@ -425,12 +429,12 @@ func (a *Adapter) GetStack(stackID string) (*Stack, error) {
 // DeleteStack deletes the CloudFormation stack with the given name
 func (a *Adapter) DeleteStack(stack *Stack) error {
 	for _, asg := range a.autoScalingGroups {
-		if err := detachTargetGroupsFromAutoScalingGroup(a.autoscaling, []string{stack.TargetGroupARN()}, asg.name); err != nil {
+		if err := detachTargetGroupsFromAutoScalingGroup(a.autoscaling, []string{stack.TargetGroupARN}, asg.name); err != nil {
 			return fmt.Errorf("DeleteStack failed to detach: %v", err)
 		}
 	}
 
-	return deleteStack(a.cloudformation, stack.Name())
+	return deleteStack(a.cloudformation, stack.Name)
 }
 
 func buildManifest(awsAdapter *Adapter) (*manifest, error) {

diff --git a/aws/cf.go b/aws/cf.go
@@ -19,55 +19,28 @@ const (
 
 // Stack is a simple wrapper around a CloudFormation Stack.
 type Stack struct {
-	name            string
+	Name            string
 	status          string
-	dnsName         string
-	scheme          string
-	targetGroupARN  string
-	certificateARNs map[string]time.Time
-	ownerIngress    string
+	DNSName         string
+	Scheme          string
+	TargetGroupARN  string
+	CertificateARNs map[string]time.Time
+	OwnerIngress    string
 	tags            map[string]string
 }
 
-func (s *Stack) Name() string {
-	return s.name
-}
-
-func (s *Stack) CertificateARNs() map[string]time.Time {
-	return s.certificateARNs
-}
-
-func (s *Stack) DNSName() string {
-	return s.dnsName
-}
-
-func (s *Stack) Scheme() string {
-	return s.scheme
-}
-
-func (s *Stack) TargetGroupARN() string {
-	return s.targetGroupARN
-}
-
-func (s *Stack) OwnerIngress() string {
-	if s == nil {
-		return ""
-	}
-	return s.ownerIngress
-}
-
 // IsComplete returns true if the stack status is a complete state.
 func (s *Stack) IsComplete() bool {
 	if s == nil {
 		return false
 	}
 
 	switch s.status {
-	case cloudformation.StackStatusCreateComplete:
+	case cloudformation.StackStatusCreateComplete,
+		cloudformation.StackStatusUpdateComplete,
+		cloudformation.StackStatusRollbackComplete,
+		cloudformation.StackStatusUpdateRollbackComplete:
 		return true
-	case cloudformation.StackStatusUpdateComplete:
-		return true
-
 	}
 	return false
 }
@@ -80,7 +53,7 @@ func (s *Stack) ShouldDelete() bool {
 	}
 
 	now := time.Now().UTC()
-	for _, t := range s.certificateARNs {
+	for _, t := range s.CertificateARNs {
 		if t.IsZero() || t.After(now) {
 			return false
 		}
@@ -354,13 +327,13 @@ func mapToManagedStack(stack *cloudformation.Stack) *Stack {
 	}
 
 	return &Stack{
-		name:            aws.StringValue(stack.StackName),
-		dnsName:         outputs.dnsName(),
-		targetGroupARN:  outputs.targetGroupARN(),
-		scheme:          parameters[parameterLoadBalancerSchemeParameter],
-		certificateARNs: certificateARNs,
+		Name:            aws.StringValue(stack.StackName),
+		DNSName:         outputs.dnsName(),
+		TargetGroupARN:  outputs.targetGroupARN(),
+		Scheme:          parameters[parameterLoadBalancerSchemeParameter],
+		CertificateARNs: certificateARNs,
 		tags:            tags,
-		ownerIngress:    ownerIngress,
+		OwnerIngress:    ownerIngress,
 		status:          aws.StringValue(stack.StackStatus),
 	}
 }

diff --git a/aws/cf_test.go b/aws/cf_test.go
@@ -112,7 +112,7 @@ func TestIsComplete(t *testing.T) {
 		{cloudformation.StackStatusDeleteFailed, false},
 		{cloudformation.StackStatusDeleteInProgress, false},
 		{cloudformation.StackStatusReviewInProgress, false},
-		{cloudformation.StackStatusRollbackComplete, false},
+		{cloudformation.StackStatusRollbackComplete, true},
 		{cloudformation.StackStatusRollbackFailed, false},
 		{cloudformation.StackStatusRollbackInProgress, false},
 		{cloudformation.StackStatusUpdateCompleteCleanupInProgress, false},
@@ -286,12 +286,12 @@ func TestFindManagedStacks(t *testing.T) {
 			},
 			want: []*Stack{
 				{
-					name:    "managed-stack-not-ready",
-					dnsName: "example-notready.com",
-					certificateARNs: map[string]time.Time{
+					Name:    "managed-stack-not-ready",
+					DNSName: "example-notready.com",
+					CertificateARNs: map[string]time.Time{
 						"cert-arn": time.Time{},
 					},
-					targetGroupARN: "tg-arn",
+					TargetGroupARN: "tg-arn",
 					tags: map[string]string{
 						kubernetesCreatorTag:                 DefaultControllerID,
 						clusterIDTagPrefix + "test-cluster":  resourceLifecycleOwned,
@@ -300,12 +300,12 @@ func TestFindManagedStacks(t *testing.T) {
 					status: cloudformation.StackStatusUpdateInProgress,
 				},
 				{
-					name:    "managed-stack",
-					dnsName: "example.com",
-					certificateARNs: map[string]time.Time{
+					Name:    "managed-stack",
+					DNSName: "example.com",
+					CertificateARNs: map[string]time.Time{
 						"cert-arn": time.Time{},
 					},
-					targetGroupARN: "tg-arn",
+					TargetGroupARN: "tg-arn",
 					tags: map[string]string{
 						kubernetesCreatorTag:                 DefaultControllerID,
 						clusterIDTagPrefix + "test-cluster":  resourceLifecycleOwned,
@@ -314,8 +314,8 @@ func TestFindManagedStacks(t *testing.T) {
 					status: cloudformation.StackStatusCreateComplete,
 				},
 				{
-					name:            "managed-stack-not-ready",
-					certificateARNs: map[string]time.Time{},
+					Name:            "managed-stack-not-ready",
+					CertificateARNs: map[string]time.Time{},
 					tags: map[string]string{
 						kubernetesCreatorTag:                DefaultControllerID,
 						clusterIDTagPrefix + "test-cluster": resourceLifecycleOwned,
@@ -360,21 +360,21 @@ func TestFindManagedStacks(t *testing.T) {
 			},
 			want: []*Stack{
 				{
-					name:            "managed-stack-not-ready",
-					dnsName:         "example-notready.com",
-					targetGroupARN:  "tg-arn",
-					certificateARNs: map[string]time.Time{},
+					Name:            "managed-stack-not-ready",
+					DNSName:         "example-notready.com",
+					TargetGroupARN:  "tg-arn",
+					CertificateARNs: map[string]time.Time{},
 					tags: map[string]string{
 						kubernetesCreatorTag:                DefaultControllerID,
 						clusterIDTagPrefix + "test-cluster": resourceLifecycleOwned,
 					},
 					status: cloudformation.StackStatusReviewInProgress,
 				},
 				{
-					name:            "managed-stack",
-					dnsName:         "example.com",
-					targetGroupARN:  "tg-arn",
-					certificateARNs: map[string]time.Time{},
+					Name:            "managed-stack",
+					DNSName:         "example.com",
+					TargetGroupARN:  "tg-arn",
+					CertificateARNs: map[string]time.Time{},
 					tags: map[string]string{
 						kubernetesCreatorTag:                DefaultControllerID,
 						clusterIDTagPrefix + "test-cluster": resourceLifecycleOwned,
@@ -448,12 +448,12 @@ func TestGetStack(t *testing.T) {
 				}, nil),
 			},
 			want: &Stack{
-				name:    "managed-stack",
-				dnsName: "example.com",
-				certificateARNs: map[string]time.Time{
+				Name:    "managed-stack",
+				DNSName: "example.com",
+				CertificateARNs: map[string]time.Time{
 					"cert-arn": time.Time{},
 				},
-				targetGroupARN: "tg-arn",
+				TargetGroupARN: "tg-arn",
 				tags: map[string]string{
 					kubernetesCreatorTag:                 DefaultControllerID,
 					clusterIDTagPrefix + "test-cluster":  resourceLifecycleOwned,
@@ -516,22 +516,22 @@ func TestShouldDelete(t *testing.T) {
 	}{
 		{
 			"DeleteInProgress",
-			&Stack{certificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(1 * time.Minute)}},
+			&Stack{CertificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(1 * time.Minute)}},
 			false,
 		},
 		{
 			"DeleteInProgressSecond",
-			&Stack{certificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(1 * time.Second)}},
+			&Stack{CertificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(1 * time.Second)}},
 			false,
 		},
 		{
 			"ShouldDelete",
-			&Stack{certificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(-1 * time.Second)}},
+			&Stack{CertificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(-1 * time.Second)}},
 			true,
 		},
 		{
 			"ShouldDeleteMinute",
-			&Stack{certificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(-1 * time.Minute)}},
+			&Stack{CertificateARNs: map[string]time.Time{"test-arn": time.Now().UTC().Add(-1 * time.Minute)}},
 			true,
 		},
 		{

diff --git a/controller.go b/controller.go
@@ -226,7 +226,7 @@ func main() {
 	log.Printf("\tInternal subnet IDs: %s", awsAdapter.FindLBSubnets(elbv2.LoadBalancerSchemeEnumInternal))
 	log.Printf("\tPublic subnet IDs: %s", awsAdapter.FindLBSubnets(elbv2.LoadBalancerSchemeEnumInternetFacing))
 	log.Printf("\tEC2 filters: %s", awsAdapter.FiltersString())
-	log.Printf("\tCertificates per ALB (SNI: %t): %d", certificatesPerALB > 1, certificatesPerALB)
+	log.Printf("\tCertificates per ALB: %d (SNI: %t)", certificatesPerALB, certificatesPerALB > 1)
 	log.Printf("\tIngress class filters: %s", kubeAdapter.IngressFiltersString())
 
 	go serveMetrics(metricsAddress)

diff --git a/glide.lock b/glide.lock
diff --git a/glide.yaml b/glide.yaml
@@ -30,3 +30,7 @@ import:
 - package: github.com/mweagle/go-cloudformation
 - package: github.com/google/uuid
   version: ~0.2.0
+- package: github.com/stretchr/testify
+  version: ~1.2.2
+  subpackages:
+  - assert