fix for #1930, make sessions sticky, for ingress with multiple rules … #2451

nusx · 2018-04-30T14:22:17Z

…and backends

for an ingress with session affinity cookie, set the location as path on the cookie when unique
the previous behaviour ( path=/ ) is preserved for ingresses with multiple rules for the same backend (locations not unique)

What this PR does / why we need it:
The current template generates upstreams without setting the path parameter on the sticky config. This leads to a new cookie beeing generated whenever the user requests a different path from the ingress. This change sets the path parameter whenever the backend contains only a single location. It falls back to the previous behaviour (not setting the path parameter) if multiple ingress rules exist for the same backend (leading to multiple locations for the backend).
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1930

Special notes for your reviewer:

k8s-ci-robot · 2018-04-30T14:22:29Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

nusx · 2018-04-30T14:53:03Z

/check-cla

nusx · 2018-04-30T15:58:12Z

/assign @ingress-nginx-maintainers

k8s-ci-robot · 2018-04-30T15:58:13Z

@nusx: GitHub didn't allow me to assign the following users: ingress-nginx-maintainers.

Note that only kubernetes members and repo collaborators can be assigned.

In response to this:

/assign @ingress-nginx-maintainers

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nusx · 2018-04-30T15:59:23Z

/assign @thockin

codecov-io · 2018-04-30T16:14:19Z

Codecov Report

Merging #2451 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2451   +/-   ##
=======================================
  Coverage   40.68%   40.68%           
=======================================
  Files          75       75           
  Lines        5108     5108           
=======================================
  Hits         2078     2078           
  Misses       2751     2751           
  Partials      279      279

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f74d063...1a320ae. Read the comment docs.

aledbf · 2018-04-30T16:15:44Z

@nusx please add e2e tests

antoineco

antoineco · 2018-05-03T12:50:14Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -358,7 +358,7 @@ http {
    {{ range $name, $upstream := $backends }}
    {{ if eq $upstream.SessionAffinity.AffinityType "cookie" }}
    upstream sticky-{{ $upstream.Name }} {
-        sticky hash={{ $upstream.SessionAffinity.CookieSessionAffinity.Hash }} name={{ $upstream.SessionAffinity.CookieSessionAffinity.Name }}  httponly;
+        sticky hash={{ $upstream.SessionAffinity.CookieSessionAffinity.Hash }} name={{ $upstream.SessionAffinity.CookieSessionAffinity.Name }}{{ range $locationName, $locationPaths := $upstream.SessionAffinity.CookieSessionAffinity.Locations }}{{ if eq (len $locationPaths) 1 }}{{ range $locationPath := $locationPaths }} path={{ $locationPath }}{{ end }}{{ end }}{{ end}} httponly;


if eq (len $locationPaths) 1 }}{{ range $locationPath := $locationPaths }} why using a range if you expect a single element?

nusx · 2018-05-03T14:02:17Z

@aledbf added e2e tests and updated the template according to @antoineco s comment (now accessing the locationPaths array via index action).

nusx · 2018-05-07T12:06:29Z

/assign @aledbf

antoineco · 2018-06-10T10:54:25Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -358,7 +358,7 @@ http {
    {{ range $name, $upstream := $backends }}
    {{ if eq $upstream.SessionAffinity.AffinityType "cookie" }}
    upstream sticky-{{ $upstream.Name }} {
-        sticky hash={{ $upstream.SessionAffinity.CookieSessionAffinity.Hash }} name={{ $upstream.SessionAffinity.CookieSessionAffinity.Name }}  httponly;
+        sticky hash={{ $upstream.SessionAffinity.CookieSessionAffinity.Hash }} name={{ $upstream.SessionAffinity.CookieSessionAffinity.Name }}{{if eq (len $upstream.SessionAffinity.CookieSessionAffinity.Locations) 1 }}{{ range $locationName, $locationPaths := $upstream.SessionAffinity.CookieSessionAffinity.Locations }}{{ if eq (len $locationPaths) 1 }} path={{ index $locationPaths 0 }}{{ end }}{{ end }}{{ end }} httponly;


As a bonus, you could wrap this logic inside a template function (see internal/ingress/controller/template/template.go) that receives a *ingress.CookieSessionAffinity and returns either path=/xyz; or nothing.
You can then call that function inside the template like {{ cookiepath $upstream.SessionAffinity.CookieSessionAffinity }}.

No hard feeling on my side though, the Go template language also does the trick here.

antoineco · 2018-06-10T10:55:40Z

@nusx thanks for addressing the different comments, the tests look good to me 👍
Could you please fix the conflict on internal/file/bindata.go(just delete the file) so we can move on?

nusx · 2018-06-10T20:56:57Z

@antoineco updated the pull request (deleted bindata.go)

aledbf · 2018-06-10T21:08:38Z

@nusx please squash the commits

…ple rules and backends * for an ingress with session affinity cookie, set the location as path on the cookie when unique * the previous behaviour ( cookie path=/ ) is preserved for ingresses with multiple rules for the same backend (locations not unique) added e2e tests for session affinity, setting path on sticky config added tests: * it should set the path to /something on the generated cookie * it should set the path to / on the generated cookie if there's more than one rule referring to the same backend

nusx · 2018-06-13T13:38:37Z

@aledbf squashed the commits

antoineco · 2018-06-13T14:52:23Z

/lgtm

aledbf · 2018-06-15T03:46:23Z

/approve

k8s-ci-robot · 2018-06-15T03:46:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, antoineco, nusx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aledbf · 2018-06-15T03:46:34Z

@nusx thanks!

JrCs · 2018-06-15T06:47:32Z

Sorry but is it not break if i have multiple path and want only ONE cookie hash for all ?

antoineco · 2018-06-15T06:52:40Z

Cookies are available to all subpaths, but not superpaths.

With this fix, next time you'll send a request to /myapp/myapi your sticky cookie will be created for the path /, making it available to all sublocations.

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 30, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 30, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 30, 2018

k8s-ci-robot assigned thockin Apr 30, 2018

aledbf unassigned thockin Apr 30, 2018

antoineco reviewed May 3, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 3, 2018

k8s-ci-robot assigned aledbf May 7, 2018

antoineco reviewed Jun 10, 2018

View reviewed changes

nusx force-pushed the set-sticky-path-for-backend branch from 54a6985 to b08a178 Compare June 11, 2018 08:17

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 11, 2018

nusx force-pushed the set-sticky-path-for-backend branch from b08a178 to 1a320ae Compare June 11, 2018 08:45

k8s-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 11, 2018

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 11, 2018

aledbf assigned ElvinEfendi and antoineco Jun 13, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2018

k8s-ci-robot merged commit dfca2a0 into kubernetes:master Jun 15, 2018

ElvinEfendi mentioned this pull request Jun 18, 2018

Adjust sticky session logic in dynamic mode to match static mode #2662

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for #1930, make sessions sticky, for ingress with multiple rules … #2451

fix for #1930, make sessions sticky, for ingress with multiple rules … #2451

nusx commented Apr 30, 2018

k8s-ci-robot commented Apr 30, 2018

nusx commented Apr 30, 2018

nusx commented Apr 30, 2018

k8s-ci-robot commented Apr 30, 2018

nusx commented Apr 30, 2018

codecov-io commented Apr 30, 2018 •

edited

Loading

aledbf commented Apr 30, 2018

antoineco left a comment •

edited

Loading

antoineco May 3, 2018

nusx commented May 3, 2018

nusx commented May 7, 2018

antoineco Jun 10, 2018

antoineco commented Jun 10, 2018

nusx commented Jun 10, 2018

aledbf commented Jun 10, 2018

nusx commented Jun 13, 2018

antoineco commented Jun 13, 2018

aledbf commented Jun 15, 2018

k8s-ci-robot commented Jun 15, 2018

aledbf commented Jun 15, 2018

JrCs commented Jun 15, 2018

antoineco commented Jun 15, 2018 •

edited

Loading

fix for #1930, make sessions sticky, for ingress with multiple rules … #2451

fix for #1930, make sessions sticky, for ingress with multiple rules … #2451

Conversation

nusx commented Apr 30, 2018

k8s-ci-robot commented Apr 30, 2018

nusx commented Apr 30, 2018

nusx commented Apr 30, 2018

k8s-ci-robot commented Apr 30, 2018

nusx commented Apr 30, 2018

codecov-io commented Apr 30, 2018 • edited Loading

Codecov Report

aledbf commented Apr 30, 2018

antoineco left a comment • edited Loading

Choose a reason for hiding this comment

antoineco May 3, 2018

Choose a reason for hiding this comment

nusx commented May 3, 2018

nusx commented May 7, 2018

antoineco Jun 10, 2018

Choose a reason for hiding this comment

antoineco commented Jun 10, 2018

nusx commented Jun 10, 2018

aledbf commented Jun 10, 2018

nusx commented Jun 13, 2018

antoineco commented Jun 13, 2018

aledbf commented Jun 15, 2018

k8s-ci-robot commented Jun 15, 2018

aledbf commented Jun 15, 2018

JrCs commented Jun 15, 2018

antoineco commented Jun 15, 2018 • edited Loading

codecov-io commented Apr 30, 2018 •

edited

Loading

antoineco left a comment •

edited

Loading

antoineco commented Jun 15, 2018 •

edited

Loading