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.

Sign up for GitHub

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

Jump to bottom

Stickiness cookie name. #2251

Merged
merged 7 commits into from
Oct 12, 2017
Merged

Stickiness cookie name. #2251

merged 7 commits into from
Oct 12, 2017

Conversation

ldez
Copy link
Contributor

@ldez ldez commented Oct 11, 2017 •
edited
Loading

Description

  • Add missing quotes. 😞
  • Add backawards compatibility with 1.3

Related to #2232
Fixes #2249

@ldez ldez requested a review from a team as a code owner October 11, 2017 19:13
@traefiker traefiker added this to the 1.4 milestone Oct 11, 2017
@ldez ldez mentioned this pull request Oct 11, 2017
Merged
@ldez ldez requested review from a team as code owners October 11, 2017 20:24
nmengin
nmengin approved these changes Oct 12, 2017
View reviewed changes
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 🐔

emilevauge
emilevauge reviewed Oct 12, 2017
View reviewed changes
Copy link
Member

@emilevauge emilevauge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reading this PR and #2232 again, I think I'm missing something on few things:

  • hasStickinessLabel implementations differ quite a lot between providers. Why?
  • in some providers, the cookie name can be generated, and in some others, can just be set by labels (getStickinessCookieName)
  • only few providers seems to be tested on this

Again, I may be wrong or missed something!

juliens
juliens approved these changes Oct 12, 2017
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez
Copy link
Contributor Author

ldez commented Oct 12, 2017

@emilevauge

  1. hasStickinessLabel implementations differ because each providers have a different way to get label
  2. only one provider: k8s (double hash)
  3. yes because all providers are not testable on this part

@ldez ldez removed the bot/no-merge label Oct 12, 2017
@traefiker traefiker force-pushed the fix/template-cookie-name branch from e1d07c3 to a528776 Compare October 12, 2017 14:36
@ldez ldez force-pushed the fix/template-cookie-name branch from a528776 to 7c9b68d Compare October 12, 2017 15:32
@ldez ldez removed the bot/no-merge label Oct 12, 2017
@traefiker traefiker merged commit 8cb3f08 into traefik:v1.4 Oct 12, 2017
@traefiker traefiker mentioned this pull request Oct 12, 2017
Closed
@ldez ldez deleted the fix/template-cookie-name branch October 12, 2017 15:51
@MichaelErmer
Copy link
Contributor

Is this on docker yet?

@ldez
Copy link
Contributor Author

ldez commented Oct 13, 2017

@MichaelErmer @gabrielfsousa could you try ldez/traefik:sticky ?

@MichaelErmer
Copy link
Contributor

MichaelErmer commented Oct 13, 2017 •
edited
Loading

@ldez it "works" (starts) however it doesnt enable stickiness using these labels:

    - "traefik.backend.loadbalancer.stickiness=true"
    - "traefik.backend.loadbalancer.stickiness.cookieName=_TRAEFIK_BACKEND"

nor backwards compatible:

    - "traefik.backend.loadbalancer.sticky=true"

@lvnilesh
Copy link

Need stickiness and cookieName working with Docker Swarm backend as well, please. Thank you.

@MichaelErmer
Copy link
Contributor

oh yes, im using docker swarm maybe thats the issue...

@ldez ldez mentioned this pull request Oct 15, 2017
Merged
@ldez
Copy link
Contributor Author

ldez commented Oct 15, 2017

please look #2266 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilevauge emilevauge emilevauge left review comments

@traefiker traefiker traefiker approved these changes

@nmengin nmengin nmengin approved these changes

@timoreimann timoreimann timoreimann approved these changes

@juliens juliens juliens approved these changes

Assignees
No one assigned
Labels
area/provider/consulcatalog area/provider/docker area/provider/ecs area/provider/k8s/ingress area/provider/marathon area/provider/rancher area/sticky-session kind/bug/fix a bug fix size/S
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.
8 participants
@ldez @MichaelErmer @lvnilesh @juliens @timoreimann @emilevauge @nmengin @traefiker