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

feat(elasticloadbalancingv2): Add support for application cookies #13142

Merged
merged 23 commits into from
Feb 24, 2021

Conversation

robertd
Copy link
Contributor

@robertd robertd commented Feb 19, 2021

https://aws.amazon.com/about-aws/whats-new/2021/02/application-load-balancer-supports-application-cookie-stickiness/

Maybe I shouldn't have deprecated stickinessCookieDuration ... which is essentially a rename to loadBalancerStickinessCookieDuration.... but that can be easily reverted based on the feedback I recieve.

As always... feedback is much appreciated. Thanks! 👍

edit:

lb_cookie

Screen Shot 2021-02-18 at 5 14 31 PM

app_cookie
Screen Shot 2021-02-18 at 5 14 16 PM


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

@gitpod-io
Copy link

gitpod-io bot commented Feb 19, 2021

@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 label Feb 19, 2021
@robertd
Copy link
Contributor Author

robertd commented Feb 19, 2021

The more I think about this... the more I want to simply reuse stickinessCookieDuration and update all the docs. Pretty much if you provide stickinessCookieDuration param then it's lb_cookie, and if you provide both stickinessCookieDuration & appCookieName params then it becomes app_cookie. Not sure if that would confuse the users though :).

@robertd
Copy link
Contributor Author

robertd commented Feb 23, 2021

The more I think about this... the more I want to simply reuse stickinessCookieDuration and update all the docs. Pretty much if you provide stickinessCookieDuration param then it's lb_cookie, and if you provide both stickinessCookieDuration & appCookieName params then it becomes app_cookie. Not sure if that would confuse the users though :).

@njlynch When time permits let me know what your initial thoughts are and I'll definitely modify this PR based on your guidance/feedback. Thanks!

@njlynch
Copy link
Contributor

njlynch commented Feb 23, 2021

I agree -- I think we can keep the stickinessCookieDuration as working for both, and the user can set the appCookieName if they want to base stickiness based on the application cookie. Given you can't set both (LB- and app-based stickiness), I think overloading the cookie duration won't be needlessly confusing.

@robertd
Copy link
Contributor Author

robertd commented Feb 23, 2021

Thanks @njlynch... I'm on it.

@robertd
Copy link
Contributor Author

robertd commented Feb 24, 2021

@njlynch fixed... please review.

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.

Looks great. Lots of minor nit-picks.

Co-authored-by: Nick Lynch <nlynch@amazon.com>
@mergify mergify bot dismissed njlynch’s stale review February 24, 2021 15:53

Pull request has been modified.

robertd and others added 4 commits February 24, 2021 08:53
…on-listener.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
…on-listener.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
…on-target-group.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
…roup.test.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
This was referenced Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants