Refactor/PR comments for auto-cert matching + documentation

docs/guide/ingress/annotation.md

@@ -340,6 +340,58 @@ SSL support can be controlled with following annotations:
             alb.ingress.kubernetes.io/certificate-arn: arn:aws:acm:us-west-2:xxxxx:certificate/cert1,arn:aws:acm:us-west-2:xxxxx:certificate/cert2,arn:aws:acm:us-west-2:xxxxx:certificate/cert3
             ```
 
+    !!!tip
+        If the `alb.ingress.kubernetes.io/certificate-arn` annotation is not specified, the controller will attempt to add certificates to listeners that require it by matching available certs from ACM with the `host` field in each listener's ingress rule.
+
+    !!!example
+        - attaches a cert for `dev.example.com` or `*.example.com` to the ALB
+            ```yaml
+            apiVersion: extensions/v1beta1
+            kind: Ingress
+            metadata:
+            namespace: default
+            name: ingress
+            annotations:
+              kubernetes.io/ingress.class: alb
+              alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS":443}]'
+            spec:
+            rules:
+            - host: dev.example.com
+              http:
+                paths:
+                - path: /users/*
+                backend:
+                  serviceName: user-service
+                  servicePort: 80
+            ```
+
+    !!!tip
+        Alternatively, domains specified using the `tls` field in the spec will also be matched with listeners and their certs will be attached from ACM. This can be used in conjunction with listener host field matching.
+
+    !!!example
+        - attaches certs for `www.example.com` to the ALB
+            ```yaml
+            apiVersion: extensions/v1beta1
+            kind: Ingress
+            metadata:
+            namespace: default
+            name: ingress
+            annotations:
+              kubernetes.io/ingress.class: alb
+              alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS":443}]'
+            spec:
+              tls:
+              - hosts:
+                - www.example.com
+            rules:
+            - http:
+                paths:
+                - path: /users/*
+                  backend:
+                    serviceName: user-service
+                    servicePort: 80
+            ```
+
 - <a name="ssl-policy">`alb.ingress.kubernetes.io/ssl-policy`</a> specifies the [Security Policy](https://docs.aws.amazon.com/elasticloadbalancing/latest/application/create-https-listener.html#describe-ssl-policies) that should be assigned to the ALB, allowing you to control the protocol and ciphers.
 
     !!!example

internal/alb/ls/listener.go

@@ -3,6 +3,7 @@ package ls
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"strings"
 
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -21,7 +22,6 @@ import (
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/annotations"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/annotations/action"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/annotations/loadbalancer"
-	"github.com/kubernetes-sigs/aws-alb-ingress-controller/pkg/util/log"
 	util "github.com/kubernetes-sigs/aws-alb-ingress-controller/pkg/util/types"
 	extensions "k8s.io/api/extensions/v1beta1"
 )
@@ -223,7 +223,7 @@ func (controller *defaultController) buildListenerConfig(ctx context.Context, op
 		var certificateARNs []string
 		_ = annotations.LoadStringSliceAnnotation(AnnotationCertificateARN, &certificateARNs, options.Ingress.Annotations)
 		if len(certificateARNs) == 0 {
-			certs, err := inferCertARNs(controller.cloud, options.Ingress, albctx.GetLogger(ctx))
+			certs, err := controller.inferCertARNs(ctx, options.Ingress)
 			if err != nil {
 				return config, errors.Errorf("missing certificates annotation %v and could not auto-load certificates from ACM: %v",
 					parser.GetAnnotationWithPrefix(AnnotationCertificateARN), err)
@@ -266,12 +266,13 @@ func (controller *defaultController) buildDefaultActions(ctx context.Context, op
 }
 
 // inferCertARNs retrieves a set of certificates from ACM that matches the ingress' hosts list
-func inferCertARNs(acmsvc aws.ACMAPI, ingress *extensions.Ingress, logger *log.Logger) ([]string, error) {
-	var certArns []string
-	var seen = map[string]bool{}
+func (controller *defaultController) inferCertARNs(ctx context.Context, ingress *extensions.Ingress) ([]string, error) {
 	var ingressHosts = uniqueHosts(ingress)
+	certArns := sets.NewString()
 
-	certs, err := acmsvc.ListCertificates([]string{acm.CertificateStatusIssued})
+	logger := albctx.GetLogger(ctx)
+
+	certs, err := controller.cloud.ListCertificates([]string{acm.CertificateStatusIssued})
 	if err != nil {
 		return nil, err
 	}
@@ -280,17 +281,14 @@ func inferCertARNs(acmsvc aws.ACMAPI, ingress *extensions.Ingress, logger *log.L
 		for _, h := range ingressHosts {
 			if domainMatchesHost(aws.StringValue(c.DomainName), h) {
 				logger.Infof("Domain name '%s', matches TLS host '%v', adding to Listener", aws.StringValue(c.DomainName), h)
-				if !seen[aws.StringValue(c.CertificateArn)] {
-					certArns = append(certArns, aws.StringValue(c.CertificateArn))
-					seen[aws.StringValue(c.CertificateArn)] = true
-				}
+				certArns.Insert(aws.StringValue(c.CertificateArn))
 			} else {
 				logger.Debugf("Ignoring domain name '%s', doesn't match '%s'", aws.StringValue(c.DomainName), h)
 			}
 		}
 	}
 
-	return certArns, nil
+	return certArns.List(), nil
 }
 
 func domainMatchesHost(domainName string, tlsHost string) bool {
@@ -302,38 +300,21 @@ func domainMatchesHost(domainName string, tlsHost string) bool {
 			return false
 		}
 
-		for i, dp := range ds {
-			if i == 0 && dp == "*" {
-				continue
-			}
-			if dp != hs[i] {
-				return false
-			}
-		}
-		return true
+		return reflect.DeepEqual(ds[1:], hs[1:])
 	}
 
 	return domainName == tlsHost
 }
 
 func uniqueHosts(ingress *extensions.Ingress) []string {
-	var result []string
-	seen := map[string]bool{}
+	hosts := sets.NewString()
 
 	for _, r := range ingress.Spec.Rules {
-		if !seen[r.Host] {
-			result = append(result, r.Host)
-			seen[r.Host] = true
-		}
+		hosts.Insert(r.Host)
 	}
 	for _, t := range ingress.Spec.TLS {
-		for _, h := range t.Hosts {
-			if !seen[h] {
-				result = append(result, h)
-				seen[h] = true
-			}
-		}
+		hosts.Insert(t.Hosts...)
 	}
 
-	return result
+	return hosts.List()
 }

internal/alb/ls/listener_test.go

@@ -17,7 +17,6 @@ import (
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/auth"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/mocks"
 	mock_auth "github.com/kubernetes-sigs/aws-alb-ingress-controller/mocks/aws-alb-ingress-controller/ingress/auth"
-	"github.com/kubernetes-sigs/aws-alb-ingress-controller/pkg/util/log"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	extensions "k8s.io/api/extensions/v1beta1"
@@ -996,14 +995,14 @@ func Test_inferCertARNs(t *testing.T) {
 				},
 			},
 			acmResult: []acm.CertificateSummary{
-				{
-					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
-					DomainName:     aws.String("foo.example.com"),
-				},
 				{
 					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:mmm"),
 					DomainName:     aws.String("*.example.com"),
 				},
+				{
+					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
+					DomainName:     aws.String("foo.example.com"),
+				},
 			},
 			expected: 2,
 		}, {
@@ -1054,14 +1053,14 @@ func Test_inferCertARNs(t *testing.T) {
 				},
 			},
 			acmResult: []acm.CertificateSummary{
-				{
-					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
-					DomainName:     aws.String("foo.example.com"),
-				},
 				{
 					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:mmm"),
 					DomainName:     aws.String("*.example.com"),
 				},
+				{
+					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
+					DomainName:     aws.String("foo.example.com"),
+				},
 			},
 			expected: 2,
 		}, {
@@ -1081,14 +1080,14 @@ func Test_inferCertARNs(t *testing.T) {
 				},
 			},
 			acmResult: []acm.CertificateSummary{
-				{
-					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
-					DomainName:     aws.String("foo.example.com"),
-				},
 				{
 					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:mmm"),
 					DomainName:     aws.String("*.example.com"),
 				},
+				{
+					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
+					DomainName:     aws.String("foo.example.com"),
+				},
 			},
 			expected: 2,
 		}, {
@@ -1111,14 +1110,14 @@ func Test_inferCertARNs(t *testing.T) {
 				},
 			},
 			acmResult: []acm.CertificateSummary{
-				{
-					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
-					DomainName:     aws.String("*.bar.example.com"),
-				},
 				{
 					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:mmm"),
 					DomainName:     aws.String("*.baz.example.com"),
 				},
+				{
+					CertificateArn: aws.String("arn:acm:xxx:yyy:zzz/kkk:www"),
+					DomainName:     aws.String("*.bar.example.com"),
+				},
 			},
 			expected: 2,
 		}, {
@@ -1131,12 +1130,14 @@ func Test_inferCertARNs(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			var logger = log.New(test.name)
+			mockedCloud := &mocks.CloudAPI{}
+			mockedCloud.On("ListCertificates", []string{acm.CertificateStatusIssued}).Return(test.acmResult, test.acmErr)
 
-			acmsvc := &mocks.CloudAPI{}
-			acmsvc.On("ListCertificates", []string{acm.CertificateStatusIssued}).Return(test.acmResult, test.acmErr)
+			ctrl := defaultController{
+				cloud: mockedCloud,
+			}
 
-			certificates, err := inferCertARNs(acmsvc, test.ingress, logger)
+			certificates, err := ctrl.inferCertARNs(context.TODO(), test.ingress)
 			if test.acmErr != err {
 				t.Error(err)
 			}