Add a workaround for CloudFormation update issues #162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A bug in how CloudFormation processes updates for LBs can result in complete misconfiguration. The AWS::ElasticLoadBalancingV2::Listener resource must have exactly one certificate in the CertificateArn property, which is then treated as the default certificate. All the rest are managed in a separate resource of type AWS::ElasticLoadBalancingV2::ListenerCertificate. Unfortunately, the update is not performed atomically. CloudFormation updates the Listener first, and only after it's done it creates a new ListenerCertificate resource and deletes the old one. However, if the previously default certificate swaps positions with one from the ListenerCertificate list, the resulting race conditions in AWS will result in a completely broken configuration. One of the certificates will be completely missing from the LB and you might or might not get update errors on the CF stack. Implement the workaround suggested by AWS support, which is to change the name of the ListenerCertificate resource instead of updating an existing one. To make the resource name predictable, certificate ARNs are now sorted and their hash is used in the resource name. A nice side effect is that the default certificate will always be a wildcard one instead of an arbitrary one. Additionally, the first certificate is now present in both Listener and ListenerCertificate resources. This doesn't seem to break anything, but will stop CloudFormation from deleting the default certificate if it stops being the default one. Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
|
👍 |
1 similar comment
|
👍 |
|
What if you have a second wildcard certifcate created that is lexicographically sorted before the first one? |
|
@szuecs the certificates were already configured in an arbitrary order (depending on hashmap ordering), so nobody should've depended on it. Making a wildcard certificate the default one wasn't really a goal, just a nice side-effect. |
|
@aermakov-zalando @szuecs and this should not really break as I understand it right? Because the "main" certificate will be stored in the |
|
@mikkeloscar @szuecs If we want to implement some logic to determine the best default certificate we can do it later, but this change shouldn't break anything. |
mikkeloscar
added a commit
that referenced
this pull request
Jun 29, 2018
This fixes a couple of bugs related to exceeding the maximum number of certificates per ALB. It limits the max size to 24 instead of 25. This is done because we need to duplicate the default certificate to work around a CloudFormation bug (#162) and therefore need one extra space for this limiting the maximum unique certificates per ALB to 24 instead of the AWS limit of 25. It also fixes a bug in `AddIngress` which could potentially add some of the certificates for a single ingress to a stack and the rest to another stack resulting in an undesired state. Thirdly it adds rollback complete states to `IsComplete()` to automatically attempt to update stacks that are in a rollback complete state. Lastly it removes some of the getter methods from structs and instead exports the fields that are needed in other packages. This was done to make it easier to use the structs in tests and because we are not writing Java. Fix #176, #175 Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net>
mikkeloscar
added a commit
that referenced
this pull request
Jun 29, 2018
This fixes a couple of bugs related to exceeding the maximum number of certificates per ALB. It limits the max size to 24 instead of 25. This is done because we need to duplicate the default certificate to work around a CloudFormation bug (#162) and therefore need one extra space for this limiting the maximum unique certificates per ALB to 24 instead of the AWS limit of 25. It also fixes a bug in `AddIngress` which could potentially add some of the certificates for a single ingress to a stack and the rest to another stack resulting in an undesired state. Thirdly it adds rollback complete states to `IsComplete()` to automatically attempt to update stacks that are in a rollback complete state. Lastly it removes some of the getter methods from structs and instead exports the fields that are needed in other packages. This was done to make it easier to use the structs in tests and because we are not writing Java. Fix #176, #175 Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net>
mikkeloscar
added a commit
that referenced
this pull request
Jul 8, 2018
This fixes a couple of bugs related to exceeding the maximum number of certificates per ALB. It limits the max size to 24 instead of 25. This is done because we need to duplicate the default certificate to work around a CloudFormation bug (#162) and therefore need one extra space for this limiting the maximum unique certificates per ALB to 24 instead of the AWS limit of 25. It also fixes a bug in `AddIngress` which could potentially add some of the certificates for a single ingress to a stack and the rest to another stack resulting in an undesired state. Thirdly it adds rollback complete states to `IsComplete()` to automatically attempt to update stacks that are in a rollback complete state. Fix #176, #175 Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net>
mikkeloscar
added a commit
that referenced
this pull request
Jul 8, 2018
This fixes a couple of bugs related to exceeding the maximum number of certificates per ALB. It limits the max size to 24 instead of 25. This is done because we need to duplicate the default certificate to work around a CloudFormation bug (#162) and therefore need one extra space for this limiting the maximum unique certificates per ALB to 24 instead of the AWS limit of 25. It also fixes a bug in `AddIngress` which could potentially add some of the certificates for a single ingress to a stack and the rest to another stack resulting in an undesired state. Thirdly it adds rollback complete states to `IsComplete()` to automatically attempt to update stacks that are in a rollback complete state. Fix #176, #175 Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net>
mikkeloscar
added a commit
that referenced
this pull request
Jul 9, 2018
* Remove getter and setters for easier testing Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net> * Fix handling limit of certificates per ALB This fixes a couple of bugs related to exceeding the maximum number of certificates per ALB. It limits the max size to 24 instead of 25. This is done because we need to duplicate the default certificate to work around a CloudFormation bug (#162) and therefore need one extra space for this limiting the maximum unique certificates per ALB to 24 instead of the AWS limit of 25. It also fixes a bug in `AddIngress` which could potentially add some of the certificates for a single ingress to a stack and the rest to another stack resulting in an undesired state. Thirdly it adds rollback complete states to `IsComplete()` to automatically attempt to update stacks that are in a rollback complete state. Fix #176, #175 Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net> * Vendor with dep Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net> * Check for nil stack Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net> * Sync load balancer state with stack state Signed-off-by: Mikkel Oscar Lyderik Larsen <m@moscar.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A bug in how CloudFormation processes updates for LBs can result in complete misconfiguration. The
AWS::ElasticLoadBalancingV2::Listenerresource must have exactly one certificate in the CertificateArn property, which is then treated as the default certificate. All the rest are managed in a separate resource of typeAWS::ElasticLoadBalancingV2::ListenerCertificate. Unfortunately, the update is not performed atomically. CloudFormation updates theListenerfirst, and only after it's done it creates a newListenerCertificateresource and deletes the old one.However, if the previously default certificate swaps positions with one from the
ListenerCertificatelist, the resulting race conditions in AWS will result in a completely broken configuration. One of the certificates will be completely missing from the LB and you might or might not get update errors on the CF stack.Implement the workaround suggested by AWS support, which is to change the name of the
ListenerCertificateresource instead of updating an existing one. To make the resource name predictable, certificate ARNs are now sorted and their hash is used in the resource name. A nice side effect is that the default certificate will always be a wildcard one instead of an arbitrary one.Additionally, the first certificate is now present in both
ListenerandListenerCertificateresources. This doesn't seem to break anything, but will stop CloudFormation from deleting the default certificate if it stops being the default one.