Skip to content

Auto-add certs from ACM by hostname if none are specified via annotation #851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 20, 2019
Merged

Auto-add certs from ACM by hostname if none are specified via annotation #851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1265568
add ListCertificates wrapper into Cloud API struct
khyew Feb 13, 2019
f6de372
manually merged Carlos' auto-cert PR into master
khyew Feb 13, 2019
af21454
updated mocks for CloudAPI
khyew Feb 13, 2019
a5512d7
unit tests for auto-loading cert ARNs
khyew Feb 13, 2019
b9fdbb8
Reconcile should fail if ARN inference returns 0 results
khyew Feb 14, 2019
b0e0b08
Increased logging
khyew Feb 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 88 additions & 1 deletion internal/alb/ls/listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package ls
import (
"context"
"fmt"
"strings"

"k8s.io/apimachinery/pkg/util/sets"

Expand All @@ -12,13 +13,15 @@ import (
"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/ingress/auth"

"github.com/aws/aws-sdk-go/aws/awsutil"
"github.com/aws/aws-sdk-go/service/acm"
"github.com/aws/aws-sdk-go/service/elbv2"
"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/alb/tg"
"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/albctx"
"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/aws"
"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"
)
Expand Down Expand Up @@ -220,7 +223,18 @@ func (controller *defaultController) buildListenerConfig(ctx context.Context, op
var certificateARNs []string
_ = annotations.LoadStringSliceAnnotation(AnnotationCertificateARN, &certificateARNs, options.Ingress.Annotations)
if len(certificateARNs) == 0 {
return config, errors.Errorf("annotation %v must be specified for https listener", parser.GetAnnotationWithPrefix(AnnotationCertificateARN))
certs, err := inferCertARNs(controller.cloud, options.Ingress, albctx.GetLogger(ctx))
if err != nil {
return config, errors.Errorf("missing certificates annotation %v and could not auto-load certificates from ACM: %v",
parser.GetAnnotationWithPrefix(AnnotationCertificateARN), err)
}
if len(certs) == 0 {
return config, errors.Errorf("missing certificates annotation %v could not find any matching certificates from ACM to auto-load",
parser.GetAnnotationWithPrefix(AnnotationCertificateARN))
}

albctx.GetLogger(ctx).Infof("Auto-detected and added %d certificates to listener", len(certs))
certificateARNs = certs
}
config.DefaultCertificate = []*elbv2.Certificate{
{
Expand Down Expand Up @@ -250,3 +264,76 @@ func (controller *defaultController) buildDefaultActions(ctx context.Context, op
}
return buildActions(ctx, authCfg, options.IngressAnnos, backend, options.TGGroup)
}

// inferCertARNs retrieves a set of certificates from ACM that matches the ingress' hosts list
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will be better to make inferCertArns an member function of controller, so it don't need to pass singtons like (acmsvc aws.ACMAPI) around.
Alternatively, i sugges make the signature below:
func (controller *defaultController) inferCertARNs(ctx context.Context, ingress *extension.Ingress)
So

  1. the log object can be extracted within inferCertARNs.
  2. We can pass context to ListCertificates to carry timeout info in the future.

func inferCertARNs(acmsvc aws.ACMAPI, ingress *extensions.Ingress, logger *log.Logger) ([]string, error) {
var certArns []string
var seen = map[string]bool{}
var ingressHosts = uniqueHosts(ingress)

certs, err := acmsvc.ListCertificates([]string{acm.CertificateStatusIssued})
if err != nil {
return nil, err
}

for _, c := range certs {
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
}
} else {
logger.Debugf("Ignoring domain name '%s', doesn't match '%s'", aws.StringValue(c.DomainName), h)
}
}
}

return certArns, nil
}

func domainMatchesHost(domainName string, tlsHost string) bool {
if strings.HasPrefix(domainName, "*.") {
ds := strings.Split(domainName, ".")
hs := strings.Split(tlsHost, ".")

if len(ds) != len(hs) {
return false
}

for i, dp := range ds {
Copy link
Collaborator

@M00nF1sh M00nF1sh Feb 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it for-loop can be simplied to return reflect.DeepEqual(ds[1:],hs[1:])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would cause it to erroneously return true for the following trivial case:

ds := []string{"google", "com"}
hs := []string{"facebook", "com"}

Copy link
Collaborator

@M00nF1sh M00nF1sh Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think so, since you already have two precondition before:

  1. strings.HasPrefix(domainName, "*."), ensures ds begins with "*",
  2. if len(ds) != len(hs), ensures hs is at least 1. (so hs[1:] is safe).

use reflect.DeepEqual(ds[1:], hs[1:]) after the two condition seems easier to read 🤣

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh whoops, I missed the pre-condition. Good catch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, strings.Split("*.", ".") actually evaluates to ["*", ""] which is size 2, so if ds and hs are equal then they are both at least size 2 and ds[1:] and hs[1:] will be safe operations.

if i == 0 && dp == "*" {
continue
}
if dp != hs[i] {
return false
}
}
return true
}

return domainName == tlsHost
}

func uniqueHosts(ingress *extensions.Ingress) []string {
var result []string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all these set operations via seen trick can be simplified via sets.NewString()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, fixed.

seen := map[string]bool{}

for _, r := range ingress.Spec.Rules {
if !seen[r.Host] {
result = append(result, r.Host)
seen[r.Host] = true
}
}
for _, t := range ingress.Spec.TLS {
for _, h := range t.Hosts {
if !seen[h] {
result = append(result, h)
seen[h] = true
}
}
}

return result
}
Loading