Skip to content
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.

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

fix(elasticloadbalancingv2): always set stickiness #17111

Merged
merged 10 commits into from
Oct 29, 2021
Merged

fix(elasticloadbalancingv2): always set stickiness #17111

merged 10 commits into from
Oct 29, 2021

Conversation

iRoachie
Copy link
Contributor

CloudFormation does not process the removal of the stickiness attribute from the template as a delta that needs to be processed.

This results in scenarios where if a property was set in an application, that removing it would have no effect.

The fix to this is to always explicitly set the property so that a delta is always processed.

Fixes #16620


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

CloudFormation does not process the removal of the stickiness attribute from the template as a delta that needs to be processed.

This results in scenarios where if a property was set in an application, that removing it would have no effect.

The fix to this is to always explicitly set the property so that a delta is always processed.

Fixes #16620
@gitpod-io
Copy link

gitpod-io bot commented Oct 22, 2021

@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing label Oct 22, 2021
@iRoachie
Copy link
Contributor Author

@njlynch @rix0rrr any idea how I would update this integ? It creates a record on route53 and a cert but this fails obviously cause I don't have example.com in my route53 especially with an id of fakeId. https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.alb-fargate-service-https.ts#L25

The deployment fails with:

DNS Record Set is not available. Certificate is in FAILED status.

@njlynch
Copy link
Contributor

njlynch commented Oct 28, 2021

@iRoachie -

This is one of those cases where it's impossible to have an integ test that everyone can validate. I believe the standard is that if you happen to have a route53 hosted zone and certificate you can use/request, fill in the blanks, verify deployment, and then revert the changes to the zone/cert. In this case, given the rest of the integ tests have been done, I think we can cheat this one a bit and use the --dry-run flag of cdk-integ to update:

% npx cdk-integ --dry-run integ.alb-fargate-service-https.ts

This updates the expected file without deploying; not optimal, but acceptable in this case (I think).

@iRoachie
Copy link
Contributor Author

Ahh good to know. Went with the --dry-run option

@iRoachie
Copy link
Contributor Author

iRoachie commented Oct 28, 2021

Alright @njlynch i think we have finally updated all the integ tests 😆

Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

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

Thanks for the mountain of integ test updates!

Looking at the implementation again, I'm not totally happy with the public API changes. Take a look at my proposal, and let me know what you think.

@mergify mergify bot dismissed njlynch’s stale review October 28, 2021 15:24

Pull request has been modified.

@iRoachie
Copy link
Contributor Author

@njlynch passing 🚀

@mergify
Copy link
Contributor

mergify bot commented Oct 29, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 76ae067
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 0a23953 into aws:master Oct 29, 2021
@mergify
Copy link
Contributor

mergify bot commented Oct 29, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@iRoachie iRoachie deleted the fix/elb-stickiness branch October 29, 2021 14:07
@pradeepviswanathan
Copy link

This breaks the creation of lambda based target groups. Seems like lambda based target groups doesnt expect stickiness to be provided at all. This is the related error - The provided target group attribute is not supported (Service: AmazonElasticLoadBalancing

@iRoachie
Copy link
Contributor Author

iRoachie commented Nov 1, 2021

@pradeepviswanathan let's track it another issue

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
CloudFormation does not process the removal of the stickiness attribute from the template as a delta that needs to be processed.

This results in scenarios where if a property was set in an application, that removing it would have no effect.

The fix to this is to always explicitly set the property so that a delta is always processed.

Fixes aws#16620


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(ELB): Unable to disable cookie stickiness by removing enableCookieStickiness
4 participants