Nginx sticky annotations #258

rikatz · 2017-02-10T14:34:53Z

This PR adds support for configuring the 'sticky' directive per Ingress, with annotations, instead of enabling or disabling it globally through ConfigMap.

Also it adds support for configuring the name of the cookie that will be used to route the requests, and the hash that will be used to generate the cookie.

In the future, as this is a per-Ingress feature, I think it would be better to disable and remove this configuration from ConfigMap and keep it only per Ingress.

k8s-reviewable · 2017-02-10T14:34:58Z

This change is

coveralls · 2017-02-10T14:53:51Z

Coverage increased (+0.1%) to 46.145% when pulling 6809319 on rikatz:nginx-sticky-annotations into 01c4301 on kubernetes:master.

cmluciano · 2017-02-10T18:58:00Z

related #259

bprashanth · 2017-02-10T19:04:24Z

examples/stickysession/nginx/README.md

@@ -0,0 +1,58 @@
+# Sticky Session


Suggest a directory structure like

examples/ affinity/ cookie/ nginx/ haproxy/ client nginx/ gce/ ...

bprashanth · 2017-02-10T22:04:57Z

examples/stickysession/nginx/README.md

@@ -0,0 +1,58 @@
+# Sticky Session
+
+This example demonstrates how to Stickness in a Ingress.


Suggest: how to achieve session affinity using cookies
Though unlike haproxy we're not allowed to pick the cookie right? nginx just uses a hash of the request IIRC

Hum, actually this session affinity is through cookie. The sticky module captures the cookie, and routes the request based on this. I didn't deployed more than one NGINX to check if the hash that the cookie assigns keeps the same, between them, but I think it did, as the hash is based on the upstream name.

But I will change the description as suggested.

bprashanth · 2017-02-10T22:07:40Z

examples/stickysession/nginx/README.md

+controller by specifying the [ingress.class annotation](/examples/PREREQUISITES.md#ingress-class),
+and that you have an ingress controller [running](/examples/deployment) in your cluster.
+
+Also, you need to have a deployment with replica > 1. Using a deployment with only one replica doesn't set the 'sticky' cookie.


Suggest: you will also need to deploy multiple replicas of your application that show up as endpoints for the Service referenced in the Ingress object, to test session stickyness.

bprashanth · 2017-02-10T22:13:23Z

examples/stickysession/nginx/README.md

+Also, you need to have a deployment with replica > 1. Using a deployment with only one replica doesn't set the 'sticky' cookie.
+
+## Deployment
+


Suggest something like:

Session stickyness is achieved through 3 annotations on the Ingress, as shown in the example.

Name Description Values

ingress.kubernetes.io/sticky-enabled annotation enables stickyness true/false

ingress.kubernetes.io/sticky-name (does what?) route, (and what are the possible values)

ingress.kubernetes.io/sticky-hash does what? sha1 (does what, and what are the possible values)

You can create the ingress to test this

$ kubectl create -f

Actually, I've changed this once again to reflect the suggested in https://github.com/kubernetes/ingress/pull/258/files/68093193184cc7d18d539dfa6876e2e98d445ec8#diff-56f736cfe94c996068145b331c2fd2c6

bprashanth · 2017-02-10T22:13:54Z

examples/stickysession/nginx/README.md

+  7s		7s		1	{nginx-ingress-controller }			Normal		CREATE	default/nginx-test
+
+
+$ curl -I http://stickyingress.example.com


Do they need DNS name or will IPs suffice?
Suggest using only IPs if that'll do

You have to use IP, as this is a per vhost configuration

bprashanth · 2017-02-10T22:51:01Z

core/pkg/ingress/annotations/stickysession/main.go

+)
+
+const (
+	stickyEnabled     = "ingress.kubernetes.io/sticky-enabled"


Do we really need an enabled? Can we achieve this with a single annotation like: ingress.kubernetes.io/affinity=cookie (defaults cookie and hash, unless they're specified, and if affinity is set to eg: client, then hash and cookie values are ignored)

@bprashanth You mean only one annotation, with ingress.kubernetes.io/affinity=cookie,hash=md5,name=route ?

bprashanth · 2017-02-10T22:51:18Z

core/pkg/ingress/annotations/stickysession/main.go

+
+const (
+	stickyEnabled     = "ingress.kubernetes.io/sticky-enabled"
+	stickyName        = "ingress.kubernetes.io/sticky-name"


Please rename to session-cookie-name and leave a comment like:
// If a cookie with this name exists, its value is used as an index into the list of available backends.

bprashanth · 2017-02-10T22:53:00Z

core/pkg/ingress/annotations/stickysession/main.go

+const (
+	stickyEnabled     = "ingress.kubernetes.io/sticky-enabled"
+	stickyName        = "ingress.kubernetes.io/sticky-name"
+	stickyHash        = "ingress.kubernetes.io/sticky-hash"


please rename to session-cookie-hash-algorithm
// This is the algorithm used by nginx to generate a value for the session cookie, if
// one isn't supplied and affintiy is set to "cookie".

bprashanth · 2017-02-10T22:54:11Z

controllers/nginx/configuration.md

@@ -177,6 +180,20 @@ To configure this setting globally for all Ingress rules, the `whitelist-source-
 Please check the [whitelist](examples/whitelist/README.md) example.


+### Sticky Session
+


Please link to the example from here

bprashanth · 2017-02-10T22:55:13Z

examples/stickysession/nginx/README.md

+Content-Type: text/html
+Content-Length: 612
+Connection: keep-alive
+Set-Cookie: route=a9907b79b248140b56bb13723f72b67697baac3d; Path=/; HttpOnly


Please mention that if the cookies is supplied by a user, it is honored (right? but what if it's some random string or int?), but if it isn't supplied, nginx will generate one and the client is expected to remember this value and supply it on the next request to map to the same backend.

Actually this is the behaviour of any sticky session cookie (also when you use them in Apache with mod_proxy). If the user changes the cookie, it looses the affinity and a new one is generated.

rikatz · 2017-02-12T23:16:58Z

@bprashanth Made all the reviews, but couldn't test yet. Will test tomorrow at my corporate environment.

If you could, anyway take a look in the changes to see if I've missed something (but no merging yet) I would appreciate.

Thanks :)

coveralls · 2017-02-12T23:35:07Z

Coverage increased (+0.3%) to 46.267% when pulling a158e5f on rikatz:nginx-sticky-annotations into 01c4301 on kubernetes:master.

bprashanth · 2017-02-13T17:56:23Z

@jaykeshur

bprashanth

LGTM sans nits

bprashanth · 2017-02-13T19:30:29Z

examples/affinity/cookie/nginx/README.md

+
+Session stickyness is achieved through 3 annotations on the Ingress, as shown in the [example](sticky-ingress.yaml).
+
+|Name|Description|Values|


This isn't showing up as a table, I guess you need a little more formatting: https://help.github.com/articles/organizing-information-with-tables/

OK, corrected it

bprashanth · 2017-02-14T01:30:15Z

core/pkg/ingress/annotations/sessionaffinity/main.go

+	// If a cookie with this name exists,
+	// its value is used as an index into the list of available backends.
+	annotationAffinityCookieName = "ingress.kubernetes.io/session-cookie-name"
+	defaultAffinityCookieName    = "route"


Better default name still makes sense, as suggested here #258 (comment)

This is the default name used by nginx sticky module. I don't think using SESSIONID or BACKEND a good idea, as this might also be a bit confusing with something related to the app.

But INGRESSCOOKIE is something we can use here.

Also, there is no guideline (I couldn't find) of best practices when setting a Custom HTTP Header. Amazon uses them all as Lower case. Google uses them all Upper case.

I will make this change here, just in case.

bprashanth · 2017-02-14T01:37:00Z

core/pkg/ingress/annotations/sessionaffinity/main.go

+type AffinityConfig struct {
+	// The type of affinity that will be used
+	AffinityType         string               `json:"type"`
+	CookieAffinityConfig CookieAffinityConfig `json:"cookieconfig"`


please change the name of the field to "Cookie" and make this a pointer. See the pattern here: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/types.go#L642

Doesn't this colide with the requested here: #258 (comment)

Also, isn't a better naming convention using something like 'CookieConfig'? Maybe changing this to 'Cookie' only can cause the impression that he have here something that manages cookies at all, that could lead to some misunderstanding of the code, right?

bprashanth · 2017-02-14T01:37:21Z

core/pkg/ingress/annotations/sessionaffinity/main.go

+// AffinityConfig describes the per ingress session affinity config
+type AffinityConfig struct {
+	// The type of affinity that will be used
+	AffinityType         string               `json:"type"`


I'm fine with including the type as a field since this is an internal api, but we don't really need a string indicating the type when we can just check for a field with the same name being non-nil right?

I was thinking in something like HAProxy.

You can have affinity per IP, or per cookie, and this makes other configurable parameters different, like described here

So I think it's fine to have the selected type, as you can take actions based on it's value.

Also, in nginx.tmpl (as an example), we only have 'cookie' type. So if I only check for nil values, I could misconfigure my ingress, like setting a value like 'affinity: per-ip' and thinking it would work in NGINX ingress

bprashanth · 2017-02-14T01:39:45Z

core/pkg/ingress/annotations/sessionaffinity/main.go

+	if err != nil {
+		at = ""
+	}
+	//cookieAffinityConfig = CookieAffinityParse(ing)


why are you keeping this code?

forgot that when was debugging the code. Just removed it

bprashanth · 2017-02-14T01:46:10Z

core/pkg/ingress/annotations/sessionaffinity/main.go

+	}
+	//cookieAffinityConfig = CookieAffinityParse(ing)
+	switch at {
+	case "cookie":


In other places we just check for non-nil: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/validation/validation.go#L346, but I'm fine with the way you have it right now.

However, please define the string "cookie" as a const, i.e somewhere, probably types.go, put

type Affinity string const ( Cookie Affinity = "cookie" Client Affinity = "client" )

@bprashanth you mean a new types.go inside the 'sessionaffinity' directory where we can put every constant, or directly in 'core/pkg/ingress/types.go' ?

bprashanth · 2017-02-14T01:47:12Z

core/pkg/ingress/annotations/sessionaffinity/main.go

+	Hash string `json:"hash"`
+}
+
+type affinity struct {


please define this under NewParser but above (a affinity) Parse

bprashanth · 2017-02-14T01:56:59Z

examples/affinity/cookie/nginx/sticky-ingress.yaml

+  name: nginx-test
+  annotations:
+    kubernetes.io/ingress.class: "nginx"
+    ingress.kubernetes.io/affinity: "cookie"


So re: my previous comment about one annotation, if just ingress.kubernetes.io/affinity is supplied and nothing else, we still get sticky with default alg and default cookie name correct?

Yes, that's correct. If you supply only affinity=cookie, the default alg and cookie name will be the set here

bprashanth · 2017-02-14T19:02:54Z

Only question I have is about the default name of the cookie, and if we can pick one better than "route"
EDIT: Didn't notice you changed it to INGRESSCOOKIE, only one nit about that name :)

Travis failure looks legit

# k8s.io/ingress/core/pkg/ingress/annotations/sessionaffinity
core/pkg/ingress/annotations/sessionaffinity/main_test.go:81: nginxAffinity.CookieAffinityConfig undefined (type *AffinityConfig has no field or method CookieAffinityConfig)
core/pkg/ingress/annotations/sessionaffinity/main_test.go:82: nginxAffinity.CookieAffinityConfig undefined (type *AffinityConfig has no field or method CookieAffinityConfig)
core/pkg/ingress/annotations/sessionaffinity/main_test.go:85: nginxAffinity.CookieAffinityConfig undefined (type *AffinityConfig has no field or method CookieAffinityConfig)
core/pkg/ingress/annotations/sessionaffinity/main_test.go:86: nginxAffinity.CookieAffinityConfig undefined (type *AffinityConfig has no field or method CookieAffinityConfig)

bprashanth · 2017-02-14T19:04:21Z

core/pkg/ingress/annotations/sessionaffinity/main.go

@@ -31,7 +31,7 @@ const (
 	// If a cookie with this name exists,
 	// its value is used as an index into the list of available backends.
 	annotationAffinityCookieName = "ingress.kubernetes.io/session-cookie-name"
-	defaultAffinityCookieName    = "route"
+	defaultAffinityCookieName    = "INGRESSCOOKIE"


Is there a naming convention we can follow for HTTP headers? ALL CAPS, or first Cap, or X-Blah, would be great if you could dig up some doc or rfc and reference it here, but otherwise just a simple comment about the CAPS is ok

@bprashanth I've got this, but no reference to sticky / cookie session:

https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

I've seen in there also that it's common to use something like CamelCase (IngressCookie) as an example.

Headers containing X- are going to be deprecated, as the following RFC:

https://tools.ietf.org/html/rfc6648

So we can keep this as all caps by now (as does Google), and reopen this discussion later?

The RFC section that deals with Requests and Responses Headers is the following: https://tools.ietf.org/html/rfc2616#section-5.3

Sure, whatever makes sense. Something like this will be hard to change without a deprecation notice, because people will probably have clients written to it, but I suppose they can always set the cookie name explicitly back to INGRESSCOOKIE via annotation.
Google uses all caps? for what?

coveralls · 2017-02-15T14:10:46Z

Coverage increased (+0.2%) to 46.199% when pulling b06ead1 on rikatz:nginx-sticky-annotations into 01c4301 on kubernetes:master.

fanhaozzu · 2017-02-15T16:47:04Z

so cool!
@bprashanth , the pull request should be merge

rikatz · 2017-02-15T19:34:28Z

@bprashanth Please don't merge yet. Couldn't test the behaviour here in my environment (after all the changes). Will do this tomorrow and report you.

Is there any other think I need to change by now?

Tks for the hard work!

bprashanth · 2017-02-15T19:46:19Z

Cool, will wait for you to ack. LGTM otherwise.

fanhaozzu · 2017-02-16T07:19:27Z

controllers/nginx/configuration.md

@@ -51,6 +51,9 @@ The following annotations are supported:
 |[ingress.kubernetes.io/upstream-max-fails](#custom-nginx-upstream-checks)|number|
 |[ingress.kubernetes.io/upstream-fail-timeout](#custom-nginx-upstream-checks)|number|
 |[ingress.kubernetes.io/whitelist-source-range](#whitelist-source-range)|CIDR|
+|[ingress.kubernetes.io/sticky-enabled](#sticky-session)|true or false|
+|[ingress.kubernetes.io/sticky-name](#sticky-session)|string|
+|[ingress.kubernetes.io/sticky-hash](#sticky-session)|string|



i suggest add ingress.kubernetes.io/sticky-expire to control cookie expire time.

@fanhaozzu Agreed. I just think that by now we could merge this, and add this feature later (we can open a new issue for this one).

This is because this item would need some more work to validate it's input, as this is a time pattern. Maybe using this

Can we work in this item latelly?

rikatz · 2017-02-16T10:42:15Z

@bprashanth Some considerations after the tests:

It works fine. The workflow about multiple instances of ingress sharing the same cookie hash works. I've tested agains multiple nginx ingress controllers, the cookie that was set by the first made the other ingresses route through the same backend/upstream.
When I delete a pod from a service (simulating the pod failure), the cookie is regenerated with the hash of the other picked up backend. And keeps the affinity with that new backend.
When you point different ingress objects to the same service name, one containing affinity configuration and the other don't, the first created ingress is respected. So it creates the 'upstream' of the first ingress controller. If it doesn't contain any affinity configuration, the upstream is created without it.

With these observations made, I think it's fine to merge this PR to Master.

@fanhaozzu do you mind opening that new issue with your request of setting also the cookie expiration? I'll work on that ASAP (or you can take that and also make your very own PR)

Thanks guys.

coveralls · 2017-02-16T10:51:33Z

Coverage increased (+0.2%) to 46.237% when pulling 4be502e on rikatz:nginx-sticky-annotations into 01c4301 on kubernetes:master.

coveralls · 2017-02-16T11:09:11Z

Coverage decreased (-0.06%) to 45.945% when pulling 4be502e on rikatz:nginx-sticky-annotations into 01c4301 on kubernetes:master.

bprashanth · 2017-02-16T23:57:15Z

Cool, @rikatz thanks for the pr, merging

…-sessions obsolete

Nginx sticky annotations #258 made the global enable-sticky-sessions obsolete

Ricardo Pchevuzinske Katz added 3 commits February 10, 2017 01:00

Initial support for sticky config in annotations

1dbe65e

New sticky session configuration

79e186c

Adds support for configuring stickness per Ingress

6809319

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2017

bprashanth assigned bprashanth and aledbf Feb 10, 2017

bprashanth reviewed Feb 10, 2017

View reviewed changes

Improve the session affinity feature

a158e5f

bprashanth mentioned this pull request Feb 13, 2017

Add an annotation to enable sticky session per backend #259

Closed

bprashanth reviewed Feb 14, 2017

View reviewed changes

Improve in documentation and naming case

0161ae4

bprashanth reviewed Feb 14, 2017

View reviewed changes

Corrects the affinity test

b06ead1

fanhaozzu reviewed Feb 16, 2017

View reviewed changes

Ricardo Pchevuzinske Katz added 2 commits February 16, 2017 08:31

Correct the configuration.md reference to annotations

e5c9c78

Adds a new situation in affinity config example

4be502e

bprashanth merged commit 698c084 into kubernetes:master Feb 16, 2017

arjanschaaf pushed a commit to arjanschaaf/ingress that referenced this pull request Apr 21, 2017

Nginx sticky annotations kubernetes#258 made the global enable-sticky…

389e0f5

…-sessions obsolete

arjanschaaf mentioned this pull request Apr 21, 2017

Nginx sticky annotations #258 made the global enable-sticky-sessions obsolete #638

Merged

aledbf added a commit that referenced this pull request Apr 21, 2017

Merge pull request #638 from arjanschaaf/nginx-sticky-annotations

4bd0f49

Nginx sticky annotations #258 made the global enable-sticky-sessions obsolete

		@@ -0,0 +1,58 @@
		# Sticky Session

		This example demonstrates how to Stickness in a Ingress.

		Also, you need to have a deployment with replica > 1. Using a deployment with only one replica doesn't set the 'sticky' cookie.

		## Deployment

Name	Description	Values
ingress.kubernetes.io/sticky-enabled annotation	enables stickyness	true/false
ingress.kubernetes.io/sticky-name	(does what?)	route, (and what are the possible values)
ingress.kubernetes.io/sticky-hash	does what?	sha1 (does what, and what are the possible values)

		7s 7s 1 {nginx-ingress-controller } Normal CREATE default/nginx-test


		$ curl -I http://stickyingress.example.com

		@@ -177,6 +180,20 @@ To configure this setting globally for all Ingress rules, the `whitelist-source-
		Please check the [whitelist](examples/whitelist/README.md) example.


		### Sticky Session


		Session stickyness is achieved through 3 annotations on the Ingress, as shown in the [example](sticky-ingress.yaml).

		\|Name\|Description\|Values\|

Nginx sticky annotations #258

Nginx sticky annotations #258

Conversation

rikatz commented Feb 10, 2017

k8s-reviewable commented Feb 10, 2017

coveralls commented Feb 10, 2017

cmluciano commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz Feb 12, 2017 • edited Loading

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

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

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

rikatz commented Feb 12, 2017

coveralls commented Feb 12, 2017

bprashanth commented Feb 13, 2017

bprashanth left a 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

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

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

bprashanth commented Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 15, 2017

fanhaozzu commented Feb 15, 2017

rikatz commented Feb 15, 2017

bprashanth commented Feb 15, 2017

fanhaozzu Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented Feb 16, 2017

coveralls commented Feb 16, 2017

coveralls commented Feb 16, 2017

bprashanth commented Feb 16, 2017

rikatz Feb 12, 2017 •

edited

Loading

bprashanth commented Feb 14, 2017 •

edited

Loading

fanhaozzu Feb 16, 2017 •

edited

Loading