Use an annotation to provide a route cookie #16454

pecameron · 2017-09-20T17:13:16Z

Route annotation:
router.openshift.io/router.cookie.name
can be used to set a cookie name for the route.

Added ROUTER_COOKIE_NAME for setting cookie name for all routes.

The cookie must pass regex filter "[a-zA-Z0-9_-]+"

https://trello.com/c/dZpU26W9
(3) Allow routes to set the cookie names for session stickiness

openshift/openshift-docs PR 5309

pecameron · 2017-09-20T17:13:33Z

@knobunc PTAL

knobunc · 2017-09-20T17:28:15Z

images/router/haproxy/conf/haproxy-config.template

@@ -22,6 +22,9 @@
 {{/* cidrListPattern: Match a space separated list of CIDRs; e.g. 192.168.21.23/24 192.10.2.12 */}}
 {{- $cidrListPattern := printf `(?:%s(?: +%s)*)` $cidrPattern $cidrPattern -}}

+{{/* cookie name pattern: */}}
+{{- $cookieName := "^[a-zA-Z0-9_-][a-zA-Z0-9_-]*" -}}


No need for the leading ^ since our functions add ^(...)$ around the provided regexes.

Can you name the variable $cookieNameRE to match the others

knobunc · 2017-09-20T17:31:01Z

images/router/haproxy/conf/haproxy-config.template

@@ -377,7 +380,7 @@ backend be_secure:{{$cfgIdx}}
  {{- end }}

  {{- if not (isTrue (index $cfg.Annotations "haproxy.router.openshift.io/disable_cookies")) }}
-  cookie {{$cfg.RoutingKeyName}} insert indirect nocache httponly
+  cookie {{firstMatch $cookieName (index $cfg.Annotations "router.openshift.io/route.cookie.name") $cfg.RoutingKeyName}} insert indirect nocache httponly


The annotation should probably be:
haproxy.router.openshift.io/cookie_name

Or cookie-name... we have both in the conf file :-( Seems redundant to say route there though since it is on the route.

Also, you lost the global env... do we want to have that as an option?

Went with cookie_name

Added environment variable ROUTER_COOKIE_NAME

knobunc · 2017-09-20T17:58:02Z

/assign @knobunc

knobunc · 2017-09-20T18:28:30Z

/cc @JacobTanenbaum @rajatchopra

knobunc · 2017-09-20T20:22:36Z

/retest

Flaked on #16414 (logs https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/16454/test_pull_request_origin_end_to_end/2852/)

knobunc · 2017-09-20T20:23:55Z

images/router/haproxy/conf/haproxy-config.template

@@ -22,6 +22,9 @@
 {{/* cidrListPattern: Match a space separated list of CIDRs; e.g. 192.168.21.23/24 192.10.2.12 */}}
 {{- $cidrListPattern := printf `(?:%s(?: +%s)*)` $cidrPattern $cidrPattern -}}

+{{/* cookie name pattern: */}}
+{{- $cookieNameRE := "[a-zA-Z0-9_-][a-zA-Z0-9_-]*" -}}


Just do [a-zA-Z0-9_-]+

Route annotation: router.openshift.io/router.cookie.name can be used to set a cookie name for the route. Added ROUTER_COOKIE_NAME for setting cookie name for all routes. The cookie must pass regex filter "[a-zA-Z0-9_-]+" https://trello.com/c/dZpU26W9 (3) Allow routes to set the cookie names for session stickiness origin PR 16454 openshift/origin#16454

rajatchopra · 2017-09-20T21:19:57Z

images/router/haproxy/conf/haproxy-config.template

@@ -22,6 +22,9 @@
 {{/* cidrListPattern: Match a space separated list of CIDRs; e.g. 192.168.21.23/24 192.10.2.12 */}}
 {{- $cidrListPattern := printf `(?:%s(?: +%s)*)` $cidrPattern $cidrPattern -}}

+{{/* cookie name pattern: */}}
+{{- $cookieNameRE := "[a-zA-Z0-9_-]+" -}}


Can it start with a '-' or '_' ?
Is it to follow the TEXT as defined in https://tools.ietf.org/html/rfc2616#section-2.2

@rajatchopra It supports leading '-' and ''. It is a subset of https://tools.ietf.org/html/rfc2616#section-2.2. In particular embedded , and punctuation (other than '-' and ..) are excluded.
This is as far as @knobunc wanted to go.

@rajatchopra we can allow more if you want. But I couldn't work out how to find out what was safe in the haproxy file at that point... or how to escape things.

@pecameron I realized the other regexes are named *Pattern not *RE... can we change this to match please.

Also... the comment in the commit message does not match the new regex...

And, the pattern should be surrounded by ``s not ""s

pecameron · 2017-09-21T14:00:34Z

@knobunc do you want further changes? No test should fail because of this change in the template.

rajatchopra · 2017-09-21T16:39:47Z

/lgtm

openshift-bot · 2017-09-21T21:19:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-22T05:23:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-09-22T20:08:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, pecameron, rajatchopra

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

Needs approval from an approver in each of these OWNERS Files:

~~images/router/OWNERS~~ [knobunc,rajatchopra]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

pecameron · 2017-09-22T23:05:35Z

/retest

openshift-bot · 2017-09-23T08:29:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-23T16:33:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-24T02:38:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-24T14:44:21Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-25T00:49:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-25T10:54:21Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-25T16:57:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-25T22:59:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-26T03:01:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-26T07:03:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

0xmichalis · 2017-09-26T09:54:44Z

/retest

openshift-bot · 2017-09-26T13:06:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

knobunc · 2017-09-26T15:27:17Z

/retest

openshift-merge-robot · 2017-09-26T17:16:44Z

/test all [submit-queue is verifying that this PR is safe to merge]

knobunc · 2017-09-26T18:45:03Z

/retest

openshift-merge-robot · 2017-09-26T19:21:36Z

Automatic merge from submit-queue (batch tested with PRs 16454, 16444, 16522)

openshift-ci-robot · 2017-09-26T19:25:28Z

@pecameron: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/end_to_end	`86171fe`	link	`/test end_to_end`
ci/openshift-jenkins/cmd	`86171fe`	link	`/test cmd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Route annotation: router.openshift.io/router.cookie.name can be used to set a cookie name for the route. Added ROUTER_COOKIE_NAME for setting cookie name for all routes. The cookie must pass regex filter "[a-zA-Z0-9_-]+" https://trello.com/c/dZpU26W9 (3) Allow routes to set the cookie names for session stickiness origin PR 16454 openshift/origin#16454 (cherry picked from commit 4971e9d)

openshift-merge-robot assigned JacobTanenbaum and smarterclayton Sep 20, 2017

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 20, 2017

pecameron force-pushed the cookie-name branch from dfc3aa6 to 351aedc Compare September 20, 2017 17:21

knobunc requested changes Sep 20, 2017

View reviewed changes

pecameron mentioned this pull request Sep 20, 2017

Use an annotation to provide a route cookie openshift/openshift-docs#5309

Merged

openshift-ci-robot assigned knobunc Sep 20, 2017

knobunc unassigned smarterclayton and JacobTanenbaum Sep 20, 2017

pecameron force-pushed the cookie-name branch from 351aedc to 16d0c74 Compare September 20, 2017 18:16

openshift-ci-robot requested review from JacobTanenbaum and rajatchopra September 20, 2017 18:28

knobunc reviewed Sep 20, 2017

View reviewed changes

pecameron force-pushed the cookie-name branch from 16d0c74 to 3fda80b Compare September 20, 2017 20:32

rajatchopra reviewed Sep 20, 2017

View reviewed changes

openshift-ci-robot assigned rajatchopra Sep 21, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2017

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 22, 2017

knobunc approved these changes Sep 22, 2017

View reviewed changes

openshift-merge-robot merged commit 51d62aa into openshift:master Sep 26, 2017

pecameron deleted the cookie-name branch September 26, 2017 19:32

Use an annotation to provide a route cookie #16454

Use an annotation to provide a route cookie #16454

Conversation

pecameron commented Sep 20, 2017 • edited Loading

pecameron commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented Sep 20, 2017

knobunc commented Sep 20, 2017

knobunc commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecameron commented Sep 21, 2017

rajatchopra commented Sep 21, 2017

openshift-bot commented Sep 21, 2017

openshift-bot commented Sep 22, 2017

openshift-merge-robot commented Sep 22, 2017

pecameron commented Sep 22, 2017

openshift-bot commented Sep 23, 2017

openshift-bot commented Sep 23, 2017

openshift-bot commented Sep 24, 2017

openshift-bot commented Sep 24, 2017

openshift-bot commented Sep 25, 2017

openshift-bot commented Sep 25, 2017

openshift-bot commented Sep 25, 2017

openshift-bot commented Sep 25, 2017

openshift-bot commented Sep 26, 2017

openshift-bot commented Sep 26, 2017

0xmichalis commented Sep 26, 2017

openshift-bot commented Sep 26, 2017

knobunc commented Sep 26, 2017

openshift-merge-robot commented Sep 26, 2017

knobunc commented Sep 26, 2017

openshift-merge-robot commented Sep 26, 2017

openshift-ci-robot commented Sep 26, 2017

pecameron commented Sep 20, 2017 •

edited

Loading