Repository: zalando-incubator/kube-ingress-aws-controller
Pull request #424 (2021-07-26 09:24:20)

Handle deleted certs by removing them from the LB stacks
If a certificate defined on a stack is manually deleted from ACM the controller will end up in a situation where it keeps the certificate on the stack and continuously tries to update the stack which fails because of the non-existing certificate.

This PR aims to resolve this by checking if the certificate still exists when it builds an internal representation of the load balancer based on the stack, if it no longer exists it's not added and will be removed from the stack in the next update.

Additionally to this we also need to run the controller with a lower `--cert-polling-interval` to reduce the time where the controller thinks a deleted certificate exists. I don't change the default (`30m`) in this PR, but we may consider changing it after running with a lower value in Zalando for some time.

szuecs: @mikkeloscar regarding `--cert-polling-interval`, I would say that a restart would trigger an immediate update, so this should be fine.

Patch:
diff --git a/worker.go b/worker.go
index 10d6d35..6b46a97 100644
--- a/worker.go
+++ b/worker.go
@@ -22,19 +22,20 @@ import (
 )
 
 type loadBalancer struct {
-	ingresses        map[string][]*kubernetes.Ingress
-	scheme           string
-	stack            *aws.Stack
-	shared           bool
-	http2            bool
-	clusterLocal     bool
-	securityGroup    string
-	sslPolicy        string
-	ipAddressType    string
-	wafWebACLID      string
-	certTTL          time.Duration
-	cwAlarms         aws.CloudWatchAlarmList
-	loadBalancerType string
+	ingresses                    map[string][]*kubernetes.Ingress
+	scheme                       string
+	stack                        *aws.Stack
+	existingStackCertificateARNs map[string]time.Time
+	shared                       bool
+	http2                        bool
+	clusterLocal                 bool
+	securityGroup                string
+	sslPolicy                    string
+	ipAddressType                string
+	wafWebACLID                  string
+	certTTL                      time.Duration
+	cwAlarms                     aws.CloudWatchAlarmList
+	loadBalancerType             string
 }
 
 const (
@@ -146,7 +147,7 @@ func (l *loadBalancer) CertificateARNs() map[string]time.Time {
 		}
 	}
 
-	for arn, ttl := range l.stack.CertificateARNs {
+	for arn, ttl := range l.existingStackCertificateARNs {
 		if _, ok := certificates[arn]; !ok {
 			if ttl.IsZero() {
 				certificates[arn] = time.Now().UTC().Add(l.certTTL)
@@ -186,9 +187,22 @@ type CertificatesFinder interface {
 
 // Certificates represents a generic list of certificates
 type Certificates struct {
+	certARNSet           map[string]struct{}
 	certificateSummaries []*certs.CertificateSummary
 }
 
+func NewCertificates(certs []*certs.CertificateSummary) *Certificates {
+	certARNSet := make(map[string]struct{}, len(certs))
+	for _, cert := range certs {
+		certARNSet[cert.ID()] = struct{}{}
+	}
+
+	return &Certificates{
+		certARNSet:           certARNSet,
+		certificateSummaries: certs,
+	}
+}
+
 // CertificateSummaries returns summaries of all certificates
 func (c *Certificates) CertificateSummaries() []*certs.CertificateSummary {
 	return c.certificateSummaries
@@ -196,12 +210,8 @@ func (c *Certificates) CertificateSummaries() []*certs.CertificateSummary {
 
 // CertificateExists checks if certificate with given ARN/ID is present in the collection
 func (c *Certificates) CertificateExists(arn string) bool {
-	for _, cert := range c.certificateSummaries {
-		if arn == cert.ID() {
-			return true
-		}
-	}
-	return false
+	_, ok := c.certARNSet[arn]
+	return ok
 }
 
 // FindMatchingCertificateIDs get IDs of all certificates matching to given hostnames
@@ -292,7 +302,7 @@ func doWork(
 	log.Infof("Found %d certificate(s)", len(certificateSummaries))
 	log.Infof("Found %d cloudwatch alarm configuration(s)", len(cwAlarms))
 
-	certs := &Certificates{certificateSummaries: certificateSummaries}
+	certs := NewCertificates(certificateSummaries)
 	model := buildManagedModel(certs, certsPerALB, certTTL, ingresses, stacks, cwAlarms, globalWAFACL)
 	log.Debugf("Have %d model(s)", len(model))
 	for _, loadBalancer := range model {
@@ -322,27 +332,33 @@ func sortStacks(stacks []*aws.Stack) {
 	})
 }
 
-func getAllLoadBalancers(certTTL time.Duration, stacks []*aws.Stack) []*loadBalancer {
+func getAllLoadBalancers(certs CertificatesFinder, certTTL time.Duration, stacks []*aws.Stack) []*loadBalancer {
 	loadBalancers := make([]*loadBalancer, 0, len(stacks))
 
 	for _, stack := range stacks {
 		lb := &loadBalancer{
-			stack:            stack,
-			ingresses:        make(map[string][]*kubernetes.Ingress),
-			scheme:           stack.Scheme,
-			shared:           stack.OwnerIngress == "",
-			securityGroup:    stack.SecurityGroup,
-			sslPolicy:        stack.SSLPolicy,
-			ipAddressType:    stack.IpAddressType,
-			loadBalancerType: stack.LoadBalancerType,
-			http2:            stack.HTTP2,
-			wafWebACLID:      stack.WAFWebACLID,
-			certTTL:          certTTL,
+			stack:                        stack,
+			existingStackCertificateARNs: make(map[string]time.Time, len(stack.CertificateARNs)),
+			ingresses:                    make(map[string][]*kubernetes.Ingress),
+			scheme:                       stack.Scheme,
+			shared:                       stack.OwnerIngress == "",
+			securityGroup:                stack.SecurityGroup,
+			sslPolicy:                    stack.SSLPolicy,
+			ipAddressType:                stack.IpAddressType,
+			loadBalancerType:             stack.LoadBalancerType,
+			http2:                        stack.HTTP2,
+			wafWebACLID:                  stack.WAFWebACLID,
+			certTTL:                      certTTL,
 		}
-		// initialize ingresses map with existing certificates from the
-		// stack.
-		for cert := range stack.CertificateARNs {
-			lb.ingresses[cert] = make([]*kubernetes.Ingress, 0)
+		// initialize ingresses map with existing certificates from the stack.
+		// Also filter the stack certificates so we have a set of
+		// certificates which are still availale, compared to what was
+		// latest stored on the stack.
+		for cert, expiry := range stack.CertificateARNs {
+			if certs.CertificateExists(cert) {
+				lb.existingStackCertificateARNs[cert] = expiry
+				lb.ingresses[cert] = make([]*kubernetes.Ingress, 0)
+			}
 		}
 		loadBalancers = append(loadBalancers, lb)
 	}
@@ -471,7 +487,7 @@ func buildManagedModel(
 ) []*loadBalancer {
 	sortStacks(stacks)
 	attachGlobalWAFACL(ingresses, globalWAFACL)
-	model := getAllLoadBalancers(certTTL, stacks)
+	model := getAllLoadBalancers(certs, certTTL, stacks)
 	model = matchIngressesToLoadBalancers(model, certs, certsPerALB, ingresses)
 	attachCloudWatchAlarms(model, cwAlarms)
 


Test patch:
diff --git a/worker_test.go b/worker_test.go
index 46aee14..7709ff4 100644
--- a/worker_test.go
+++ b/worker_test.go
@@ -279,7 +279,7 @@ func TestCertificateExists(tt *testing.T) {
 				certs.NewCertificate(
 					existingCertificateARN,
 					&x509.Certificate{},
-					[]*x509.Certificate{&x509.Certificate{}},
+					[]*x509.Certificate{{}},
 				),
 			},
 			exists: true,
@@ -290,14 +290,14 @@ func TestCertificateExists(tt *testing.T) {
 				certs.NewCertificate(
 					nonExistingCertificateARN,
 					&x509.Certificate{},
-					[]*x509.Certificate{&x509.Certificate{}},
+					[]*x509.Certificate{{}},
 				),
 			},
 			exists: false,
 		},
 	} {
 		tt.Run(test.name, func(t *testing.T) {
-			certs := &Certificates{certificateSummaries: test.certificateSummaries}
+			certs := NewCertificates(test.certificateSummaries)
 
 			assert.Equal(t, test.exists, certs.CertificateExists(existingCertificateARN))
 		})
@@ -310,23 +310,82 @@ func TestGetAllLoadBalancers(tt *testing.T) {
 	for _, test := range []struct {
 		name          string
 		stacks        []*aws.Stack
+		certs         []*certs.CertificateSummary
 		loadBalancers []*loadBalancer
 	}{
 		{
 			name: "one stack",
 			stacks: []*aws.Stack{
-				&aws.Stack{
+				{
 					Scheme:        "foo",
 					SecurityGroup: "sg-123456",
 				},
 			},
+			certs: []*certs.CertificateSummary{},
 			loadBalancers: []*loadBalancer{
-				&loadBalancer{
+				{
+					existingStackCertificateARNs: map[string]time.Time{},
+					securityGroup:                "sg-123456",
+					scheme:                       "foo",
+					shared:                       true,
+					ingresses:                    map[string][]*kubernetes.Ingress{},
+					certTTL:                      certTTL,
+				},
+			},
+		},
+		{
+			name: "one stack with certificates",
+			stacks: []*aws.Stack{
+				{
+					Scheme:        "foo",
+					SecurityGroup: "sg-123456",
+					CertificateARNs: map[string]time.Time{
+						"cert-arn": {},
+					},
+				},
+			},
+			certs: []*certs.CertificateSummary{
+				certs.NewCertificate(
+					"cert-arn",
+					&x509.Certificate{},
+					[]*x509.Certificate{{}},
+				),
+			},
+			loadBalancers: []*loadBalancer{
+				{
+					existingStackCertificateARNs: map[string]time.Time{
+						"cert-arn": {},
+					},
 					securityGroup: "sg-123456",
 					scheme:        "foo",
 					shared:        true,
-					ingresses:     map[string][]*kubernetes.Ingress{},
-					certTTL:       certTTL,
+					ingresses: map[string][]*kubernetes.Ingress{
+						"cert-arn": {},
+					},
+					certTTL: certTTL,
+				},
+			},
+		},
+		{
+			name: "non existing certificate is not added to LB",
+			stacks: []*aws.Stack{
+				{
+					Scheme:        "foo",
+					SecurityGroup: "sg-123456",
+					CertificateARNs: map[string]time.Time{
+						"cert-arn": {},
+					},
+				},
+			},
+			certs: []*certs.CertificateSummary{},
+			loadBalancers: []*loadBalancer{
+				{
+					existingStackCertificateARNs: map[string]time.Time{},
+					securityGroup:                "sg-123456",
+					scheme:                       "foo",
+					shared:                       true,
+					ingresses:                    map[string][]*kubernetes.Ingress{},
+					certTTL:                      certTTL,
 				},
 			},
 		},
@@ -336,7 +395,7 @@ func TestGetAllLoadBalancers(tt *testing.T) {
 				loadBalancer.stack = test.stacks[i]
 			}
 
-			assert.Equal(t, test.loadBalancers, getAllLoadBalancers(certTTL, test.stacks))
+			assert.Equal(t, test.loadBalancers, getAllLoadBalancers(NewCertificates(test.certs), certTTL, test.stacks))
 		})
 	}
 }