Add sni support #111

mikkeloscar · 2017-12-17T14:55:12Z

This adds SNI support by adding up to 25 certificates to a single ALB. If multiple ALBs exist with less than 25 certificates each then it will aggregate the certificates to as few ALBs as possible.

To support multiple certs this also fixes a bunch of other issues:

Changes how stacks are updated. Before they were scheduled for update every hour (even if no updates were needed) and now it will check if there are any updates needed and update straight away. Without this change it would have taken 1 hour to add a new certificate to an existing ALB stack.
Changes cloudformation template from a yaml template to use the library https://github.com/mweagle/go-cloudformation to generate the template using Go. This is needed because we want to dynamically add certificates to the stack, which is not possible with pure cloudformation.
Changes stack naming. Before the name was something like cluster-id-<hash-of-cert-arn>-scheme but since we can now have 25 certs per stack the naming no longer makes sense. Instead it just gets the name cluster-id-<uuid>.

Migration

This PR also takes care to ensure safe migration from stacks created by the <=v0.5.0 version of the controller. The migration works like this:

Find old stacks with only one certificate.
Aggregate certs into one of the stacks (assuming less than 25 certs in total).
Mark old stacks to be deleted with a ttl on the existing cert the stack had defined.
Delete old stacks after the ttl has expired, allowing enough time for e.g. external-dns to move to the new stack.

Each certificate is defined on the stack with a tag ingress:certificate-arn/<cert-arn>=ttl where ttl is a timestamp for when the cert should be removed from the stack. This way it's possible to create multiple stacks in case there are more than 25 certs requested. And it's possible to later merge the stacks back to one when the demand for certs are less than 25 again.

Note: with this approach it's not possible to explicitly get an ALB that is unique per ingress/certificate we could add support for this either through a flag or as an annotation added to ingresses which want to have their own ALB, however this PR is already quite big so I would add these features separately.

Fix #94

coveralls · 2017-12-17T15:06:06Z

Coverage decreased (-0.8%) to 52.539% when pulling 65fd11b on sni-support into 806f9df on master.

szuecs · 2017-12-18T09:14:23Z

aws/adapter.go

@@ -64,7 +64,7 @@ const (

 	nameTag = "Name"

-	certificateARNTag = "ingress:certificate-arn"
+	certificateARNsTag = "ingress:certificate-arns"


this will break existing installations, please make it upgradeable and set a deprecated comment and log a warning if used

szuecs · 2017-12-18T09:17:03Z

aws/cf.go

+	dnsName         string
+	scheme          string
+	targetGroupARN  string
+	CertificateARNs []string


please do not change visibility here, better add a method, for example: Add(certarns ...string)

szuecs · 2017-12-18T09:17:57Z

aws/cf.go

+	CertificateArn string `yaml:"CertificateArn"`
+}
+
+// cfTemplate is an opauqe structure for unmarshaling a yaml cloudformation


opauqe -> opaque

szuecs · 2017-12-18T09:21:02Z

aws/cf.go

+
+// injectCertificates injects a list of certificates into the cloudformation
+// template.
+func injectCertificates(cfTmpl string, certs []string) (string, error) {


I am not happy with the solution parsing string, modify object and return string.
This looks more like a hack to me. We should have a type based object that can render to a cloudformation stack.

szuecs · 2017-12-18T09:26:39Z

worker.go

@@ -138,10 +139,18 @@ func doWork(certsProvider certs.CertificatesProvider, awsAdapter *aws.Adapter, k
 	return nil
 }

-func buildManagedModel(certsProvider certs.CertificatesProvider, ingresses []*kubernetes.Ingress, stacks []*aws.Stack) map[string]*managedItem {
-	model := make(map[string]*managedItem)
+func buildManagedModel(certsProvider certs.CertificatesProvider, ingresses []*kubernetes.Ingress, stacks []*aws.Stack) []*managedItem {


the changes in this function look more like we lack of abstraction and power of the abstraction.

szuecs · 2017-12-18T09:27:56Z

👎

greenboxal · 2018-01-15T16:11:04Z

Is this PR active? This is a super important feature for us as we use ingress for basically everything, and having one ALB per service is too expensive.

If not, I'm interested in finishing it.

szuecs · 2018-01-15T16:29:15Z

@greenboxal I think we have not so much pressure as we provision wildcard certificates and mostly use the same ALB for all ingress already. Feel free to work on that.

mikkeloscar · 2018-01-15T21:44:32Z

@greenboxal I have some half finished work locally, which I will try to push one of the next days.

coveralls · 2018-01-16T10:49:28Z

Coverage increased (+1.0%) to 60.804% when pulling c96bb8c on sni-support into 289884b on master.

mikkeloscar · 2018-01-16T10:52:31Z

I have now updated this with all the changes I had locally. It now uses the github.com/mweagle/go-cloudformation library to generate a CF stack template instead of doing some weird yaml unmarshal/marshaling like it did before.

Note that this currently hasn't been tested and there are at least some issues I'm aware of:

Naming of the stacks.
- Currently we do this with a hash over the certificate, but since this can have 25 certs it no longer makes sense.
When aggregating existing stacks into less stacks because of multiple certificates per stack/ALB then we need to do it in a way were the old stacks are kept for a bit during the migration. We have a default of 5 minutes right now (stackTTL).

Both of these things has to be done in a backwardcompatible way such that we can automatically migrate the single cert ALBs to multi cert ALBs without affecting applications behind those ingress ALBs.

@greenboxal If you want to go on from here, you are welcome to do so just let me know, otherwise I will work on this in CW04.

mikkeloscar · 2018-02-18T12:42:06Z

@szuecs I have now updated the description explaining how it works.

I have tested the following scenarios:

Have two existing stacks from the previous version of the controller and switch to the new one.
Have a single stack with two certs and run a (custom) controller with a cert/stack limit of 1. This will split the stack in two.
Have two stacks with a cert each and run the controller with a cert/stack limit of 25 and see the stacks getting merged into one.
Test that all of this doesn't impact clients calling an endpoint behind ingress.

mikkeloscar · 2018-02-18T19:19:57Z

@greenboxal This should now be done. You are very welcome to give it a try.

Here's an image built from this PR: registry.opensource.zalan.do/teapot/kube-ingress-aws-controller-test:pr-111-15

szuecs · 2018-02-19T15:15:23Z

worker.go

+				if err != kubernetes.ErrUpdateNotNeeded {
+					log.Println(err)
+				} else {
+					log.Printf("updated ingress not needed %v with DNS name %q", ing, dnsName)


please switch the error handling and change the message.

if err == kubernetes.ErrUpdateNotNeeded { log.Printf("Ingress update not needed %v with DNS name %q", ing, dnsName) } else { log.Printf("Failed to update ingress: %v", err) }

szuecs · 2018-02-19T15:33:15Z

I discussed offline with @mikkeloscar and checked the code with 4 eyes that it will support more than 2 stacks, too. (I dropped my old comment)

Only small refactoring to have it more readable. Thanks!

szuecs · 2018-02-19T16:11:39Z

👍

mikkeloscar · 2018-02-19T16:17:45Z

👍

mikkeloscar added the do-not-merge label Dec 17, 2017

mikkeloscar changed the title ~~Add sni support~~ [WIP] Add sni support Dec 17, 2017

szuecs reviewed Dec 18, 2017

View reviewed changes

mikkeloscar mentioned this pull request Dec 21, 2017

Internal and Internet-Facing Aws-Load-Balancer-Scheme Cannot Coexist #114

Closed

mikkeloscar force-pushed the sni-support branch from c801e68 to 4c83b4d Compare January 16, 2018 10:44

mikkeloscar added 2 commits January 28, 2018 18:12

Add sni support

17df64c

Generate CF template with Go

c09c2c7

mikkeloscar force-pushed the sni-support branch from 4c83b4d to c09c2c7 Compare January 28, 2018 17:13

mikkeloscar mentioned this pull request Feb 7, 2018

[WIP] Update stacks when needed #123

Closed

mikkeloscar added 7 commits February 15, 2018 08:20

Safely migrate certs between stacks, fix stack naming

2bda203

Add new certs by default

90f736f

Remove old template generation

5288e3a

Fix build

4d10391

Migrate old cert tag to new certs without dropping ALB

bbf0480

Stable sort for stacks with same number of certs

79a15f2

Test adding certs in createStack

c469e11

mikkeloscar force-pushed the sni-support branch from 0e7eb4f to c469e11 Compare February 18, 2018 12:32

mikkeloscar changed the title ~~[WIP] Add sni support~~ Add sni support Feb 18, 2018

mikkeloscar removed the do-not-merge label Feb 18, 2018

mikkeloscar added 2 commits February 18, 2018 13:53

Remove unused variables/functions

156ca49

Cleanup Makefile

f916cee

szuecs reviewed Feb 19, 2018

View reviewed changes

mikkeloscar added 4 commits February 19, 2018 16:34

Change error handling

f0f5305

Refactor add ingress logic

fb62043

Move logic into managedItem methods

3bde145

Document new feature/upgrade path

c96bb8c

mikkeloscar merged commit 6d5a4fe into master Feb 19, 2018

mikkeloscar deleted the sni-support branch February 19, 2018 16:17

mikkeloscar mentioned this pull request Feb 22, 2018

Allow opt out of SNI ALB #129

Closed

ruudk mentioned this pull request Apr 9, 2020

Remove unused stackTTL #343

Merged

AlexanderYastrebov mentioned this pull request Nov 19, 2021

Reconsider use of CloudFormation template parameters #454

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sni support #111

Add sni support #111

mikkeloscar commented Dec 17, 2017 •

edited

Loading

coveralls commented Dec 17, 2017 •

edited

Loading

szuecs Dec 18, 2017 •

edited

Loading

szuecs Dec 18, 2017

szuecs Dec 18, 2017

szuecs Dec 18, 2017

szuecs Dec 18, 2017

szuecs commented Dec 18, 2017

greenboxal commented Jan 15, 2018

szuecs commented Jan 15, 2018

mikkeloscar commented Jan 15, 2018

coveralls commented Jan 16, 2018 •

edited

Loading

mikkeloscar commented Jan 16, 2018

mikkeloscar commented Feb 18, 2018 •

edited

Loading

mikkeloscar commented Feb 18, 2018

szuecs Feb 19, 2018

mikkeloscar Feb 19, 2018

szuecs commented Feb 19, 2018 •

edited

Loading

szuecs commented Feb 19, 2018

mikkeloscar commented Feb 19, 2018

Add sni support #111

Add sni support #111

Conversation

mikkeloscar commented Dec 17, 2017 • edited Loading

Migration

coveralls commented Dec 17, 2017 • edited Loading

szuecs Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

szuecs Dec 18, 2017

Choose a reason for hiding this comment

szuecs Dec 18, 2017

Choose a reason for hiding this comment

szuecs Dec 18, 2017

Choose a reason for hiding this comment

szuecs Dec 18, 2017

Choose a reason for hiding this comment

szuecs commented Dec 18, 2017

greenboxal commented Jan 15, 2018

szuecs commented Jan 15, 2018

mikkeloscar commented Jan 15, 2018

coveralls commented Jan 16, 2018 • edited Loading

mikkeloscar commented Jan 16, 2018

mikkeloscar commented Feb 18, 2018 • edited Loading

mikkeloscar commented Feb 18, 2018

szuecs Feb 19, 2018

Choose a reason for hiding this comment

mikkeloscar Feb 19, 2018

Choose a reason for hiding this comment

szuecs commented Feb 19, 2018 • edited Loading

szuecs commented Feb 19, 2018

mikkeloscar commented Feb 19, 2018

mikkeloscar commented Dec 17, 2017 •

edited

Loading

coveralls commented Dec 17, 2017 •

edited

Loading

szuecs Dec 18, 2017 •

edited

Loading

coveralls commented Jan 16, 2018 •

edited

Loading

mikkeloscar commented Feb 18, 2018 •

edited

Loading

szuecs commented Feb 19, 2018 •

edited

Loading