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

khyew · 2019-02-14T02:41:55Z

(I'm picking up a PR from @cv that was lost when the master branch of this project was re-based: #600)

This change adds a fallback behaviour to the controller's handling of TLS certificates. If the ingress spec has a HTTPS listener but no alb.ingress.kubernetes.io/certificate-arn annotation is found, then the controller will attempt to attach certificates from ACM that best match what is required by the listener. Matching is done via the listeners' spec.rules[].host entries, as well as any hostnames in spec.tls.hosts[]. Wildcard certificates are correctly matched.

e.g.
If I have successfully issued a cert for *.cv.dev.example.com and for website.qifan.example.com, then the following ingress spec will attach both certs to all listeners of the ALB:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    alb.ingress.kubernetes.io/scheme: internet-facing
    alb.ingress.kubernetes.io/target-type: ip
    alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS": 443}]'
    kubernetes.io/ingress.class: alb
  name: echoserver
  namespace: echoserver
spec:
  tls:
  - hosts:
    - echoserver.cv.dev.example.com
  rules:
  - host: website.qifan.dev.example.com
    http:
      paths:
      - backend:
          serviceName: echoserver
          servicePort: 80
        path: /
  ...

Potential TODOs:

When multiple certs are attached to a listener, the correct certificate will be used via SNI. Clients that do not support SNI will use the default cert, which is chosen arbitrarily. The controller should instead try to attach the "best" cert as the default for a particular listener.
ALBs only support 25 certificate attachments per load balancer (https://aws.amazon.com/blogs/aws/new-application-load-balancer-sni/). There is no specific messaging for users whose ingress specs exceed this limitation.

* for querying ACM for certificates

* TODO: merge his unit tests back in too

* instead of crashing out with a NPE

k8s-ci-robot · 2019-02-14T02:41:59Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2019-02-14T02:42:08Z

Hi @khyew. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

khyew · 2019-02-14T22:59:06Z

I have signed the CNCF CLA. Mister k8s-ci-robot, please re-verify me 🤖🙏

khyew · 2019-02-15T22:48:55Z

/assign @M00nF1sh

khyew · 2019-02-18T23:05:33Z

/assign @bigkraig

M00nF1sh · 2019-02-19T00:07:22Z

/ok-to-test

M00nF1sh · 2019-02-19T00:20:59Z

internal/alb/ls/listener.go

+			return false
+		}
+
+		for i, dp := range ds {


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

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

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

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

strings.HasPrefix(domainName, "*."), ensures ds begins with "*",

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 🤣

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

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.

M00nF1sh · 2019-02-19T00:22:03Z

internal/alb/ls/listener.go

+}
+
+func uniqueHosts(ingress *extensions.Ingress) []string {
+	var result []string


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

Gotcha, fixed.

M00nF1sh · 2019-02-19T00:25:20Z

internal/alb/ls/listener.go

@@ -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


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

the log object can be extracted within inferCertARNs.

We can pass context to ListCertificates to carry timeout info in the future.

M00nF1sh · 2019-02-19T00:27:24Z

Thanks so much for make this change 😄
Overall lgtm with minimal comments.

There also should be works to be do in future PRs:

documentation for this feature
resolve conflicts when there are multiple certificates for same domain.

M00nF1sh · 2019-02-19T23:08:27Z

/retest

M00nF1sh · 2019-02-19T23:45:15Z

/lgtm
/approve

k8s-ci-robot · 2019-02-19T23:45:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khyew, M00nF1sh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [M00nF1sh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

khyew · 2019-02-20T00:19:50Z

@M00nF1sh I was in the middle of addressing the PR fixes that you proposed. Do you still want me to submit them (by cutting a branch and opening a new PR)?

M00nF1sh · 2019-02-20T00:28:35Z

@M00nF1sh I was in the middle of addressing the PR fixes that you proposed. Do you still want me to submit them (by cutting a branch and opening a new PR)?

That would be awesome 😄 . I merged this since it's just some nitpicks.

khyew · 2019-02-20T00:35:46Z

Unfortunately I'm having difficulty building the project now. make clean && make errors out with module/dependency related issues. Any tips? Building on OS X.

CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -installsuffix cgo -ldflags '-s -w -X github.com/kubernetes-sigs/aws-alb-ingress-controller/version.COMMIT=git-f3ae919b -X github.com/kubernetes-sigs/aws-alb-ingress-controller/version.RELEASE=v1.1.0 -X github.com/kubernetes-sigs/aws-alb-ingress-controller/[email protected]:kubernetes-sigs/aws-alb-ingress-controller.git' -o server ./cmd
go: finding github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi latest
go: finding github.com/aws/aws-sdk-go/service/iam/iamiface latest
go: finding github.com/aws/aws-sdk-go/service/ec2/ec2iface latest
go: finding github.com/aws/aws-sdk-go/service/wafregional/wafregionaliface latest
go: finding github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi/resourcegroupstaggingapiiface latest
go: finding github.com/aws/aws-sdk-go/service/elbv2/elbv2iface latest
go: finding github.com/aws/aws-sdk-go/service/iam latest
go: finding github.com/aws/aws-sdk-go/service/acm latest
go: finding github.com/aws/aws-sdk-go/internal/shareddefaults latest
go: finding github.com/aws/aws-sdk-go/service latest
go: finding github.com/aws/aws-sdk-go/service/ec2 latest
go: finding github.com/aws/aws-sdk-go/internal/sdkuri latest
go: finding github.com/aws/aws-sdk-go/service/wafregional latest
go: finding github.com/aws/aws-sdk-go/service/elbv2 latest
go: finding github.com/aws/aws-sdk-go/internal latest
go: finding github.com/aws/aws-sdk-go/service/sts latest
go: finding github.com/aws/aws-sdk-go/internal/ini latest
go: finding github.com/aws/aws-sdk-go/service/waf latest
go: finding github.com/aws/aws-sdk-go/internal/sdkio latest
go: finding github.com/aws/aws-sdk-go/service/acm/acmiface latest
go: finding github.com/aws/aws-sdk-go/internal/sdkrand latest
internal/aws/acm.go:10:2: unknown import path "github.com/aws/aws-sdk-go/service/acm": cannot find module providing package github.com/aws/aws-sdk-go/service/acm
internal/aws/cloud.go:9:2: unknown import path "github.com/aws/aws-sdk-go/service/acm/acmiface": cannot find module providing package github.com/aws/aws-sdk-go/service/acm/acmiface
cmd/main.go:29:2: unknown import path "github.com/aws/aws-sdk-go/service/ec2": cannot find module providing package github.com/aws/aws-sdk-go/service/ec2
internal/aws/cloud.go:11:2: unknown import path "github.com/aws/aws-sdk-go/service/ec2/ec2iface": cannot find module providing package github.com/aws/aws-sdk-go/service/ec2/ec2iface
internal/aws/cloud.go:12:2: unknown import path "github.com/aws/aws-sdk-go/service/elbv2": cannot find module providing package github.com/aws/aws-sdk-go/service/elbv2
internal/aws/cloud.go:13:2: unknown import path "github.com/aws/aws-sdk-go/service/elbv2/elbv2iface": cannot find module providing package github.com/aws/aws-sdk-go/service/elbv2/elbv2iface
internal/aws/cloud.go:14:2: unknown import path "github.com/aws/aws-sdk-go/service/iam": cannot find module providing package github.com/aws/aws-sdk-go/service/iam
internal/aws/cloud.go:15:2: unknown import path "github.com/aws/aws-sdk-go/service/iam/iamiface": cannot find module providing package github.com/aws/aws-sdk-go/service/iam/iamiface
cmd/main.go:30:2: unknown import path "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi": cannot find module providing package github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi
internal/aws/cloud.go:17:2: unknown import path "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi/resourcegroupstaggingapiiface": cannot find module providing package github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi/resourcegroupstaggingapiiface
../../../../pkg/mod/github.com/aws/[email protected]/aws/credentials/stscreds/assume_role_provider.go:89:2: unknown import path "github.com/aws/aws-sdk-go/service/sts": cannot find module providing package github.com/aws/aws-sdk-go/service/sts
internal/aws/wafregional.go:8:2: unknown import path "github.com/aws/aws-sdk-go/service/waf": cannot find module providing package github.com/aws/aws-sdk-go/service/waf
internal/aws/cloud.go:18:2: unknown import path "github.com/aws/aws-sdk-go/service/wafregional": cannot find module providing package github.com/aws/aws-sdk-go/service/wafregional
internal/aws/cloud.go:19:2: unknown import path "github.com/aws/aws-sdk-go/service/wafregional/wafregionaliface": cannot find module providing package github.com/aws/aws-sdk-go/service/wafregional/wafregionaliface
make: *** [server] Error 1

M00nF1sh · 2019-02-20T00:43:52Z

umm, are you based on the new master branch(i bump dependencies two commits ago), and using a go version that supports modules?

khyew · 2019-02-20T00:46:40Z

go version go1.11.3 darwin/amd64

Module support shouldn't be a problem. Worked fine before I pulled master to get up-to-date today.

khyew · 2019-02-20T00:47:26Z

Hold on, I'm going to git bisect to narrow down the offending commit

M00nF1sh · 2019-02-20T00:48:52Z

i think you can try do the following:

delete the vendor directory in repo(if any, i guess it's mostly caused by this)
delete everything from ~/go/pkg/ (module caches)

khyew · 2019-02-20T00:54:55Z

There is no vendor directory in the repo. I deleted the package cache (the entire contents of ~/go/pkg/) but that didn't resolve the problem.

According to the results of the bisect, commit a28d0d0 is the first to introduce the issues so I assume it's related to bumping dependency versions. When I try to build from that commit, I get the same error that I posted. When I build from the previous commit, it succeeds without issues.

khyew · 2019-02-20T01:49:39Z

Update: deleting the actual module cache directory worked. My $GOPATH is set to ~ so for me it was ~/pkg and not ~/go/pkg. I did a rm -rf ~/go/pkg and it didn't report any errors (deleting a non-existing file with -f is considered a success). Instead I should have either checked manually or done a rm -rf $GOPATH/pkg.

M00nF1sh · 2019-02-20T02:03:28Z

There is some bugs in go's module support. Seems multiple cocurrent gets via go module might corrupt the module directory. I guess that's what you encountered 😄

khyew added 6 commits February 13, 2019 16:29

add ListCertificates wrapper into Cloud API struct

1265568

* for querying ACM for certificates

manually merged Carlos' auto-cert PR into master

f6de372

* TODO: merge his unit tests back in too

updated mocks for CloudAPI

af21454

unit tests for auto-loading cert ARNs

a5512d7

Reconcile should fail if ARN inference returns 0 results

b9fdbb8

* instead of crashing out with a NPE

Increased logging

b0e0b08

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 14, 2019

k8s-ci-robot requested review from bigkraig and M00nF1sh February 14, 2019 02:42

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 14, 2019

k8s-ci-robot assigned M00nF1sh Feb 15, 2019

k8s-ci-robot assigned bigkraig Feb 18, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 19, 2019

M00nF1sh reviewed Feb 19, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2019

k8s-ci-robot merged commit f3ae919 into kubernetes-sigs:master Feb 20, 2019

khyew mentioned this pull request Feb 20, 2019

Refactor/PR comments for auto-cert matching + documentation #864

Merged

echoboomer mentioned this pull request Mar 15, 2019

Fallback automatic certificate assignment not working #896

Closed

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

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

Uh oh!

Conversation

khyew commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 14, 2019

Uh oh!

k8s-ci-robot commented Feb 14, 2019

Uh oh!

khyew commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khyew commented Feb 15, 2019

Uh oh!

khyew commented Feb 18, 2019

Uh oh!

M00nF1sh commented Feb 19, 2019

Uh oh!

M00nF1sh Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khyew Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khyew Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

khyew Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

khyew Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

M00nF1sh commented Feb 19, 2019

Uh oh!

M00nF1sh commented Feb 19, 2019

Uh oh!

M00nF1sh commented Feb 19, 2019

Uh oh!

k8s-ci-robot commented Feb 19, 2019

Uh oh!

khyew commented Feb 20, 2019

Uh oh!

M00nF1sh commented Feb 20, 2019

Uh oh!

khyew commented Feb 20, 2019

Uh oh!

M00nF1sh commented Feb 20, 2019

Uh oh!

khyew commented Feb 20, 2019

Uh oh!

khyew commented Feb 20, 2019

Uh oh!

M00nF1sh commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khyew commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khyew commented Feb 20, 2019

Uh oh!

M00nF1sh commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

khyew commented Feb 14, 2019 •

edited

Loading

khyew commented Feb 14, 2019 •

edited

Loading

M00nF1sh Feb 19, 2019 •

edited

Loading

M00nF1sh Feb 20, 2019 •

edited

Loading

M00nF1sh commented Feb 20, 2019 •

edited

Loading

khyew commented Feb 20, 2019 •

edited

Loading

M00nF1sh commented Feb 20, 2019 •

edited

Loading