Add better handling for cookie affinity with preserve option #667

sigseg1v · 2020-09-15T17:03:40Z

First off, wanted to say, great project and I appreciate you making this open source and all your hard work maintaining it. I didn't see a contributors file but I've attempted to make this submission as complete as possible. If you are interested in including these changes but want me to make any modifications, please let me know and I'd be happy to accommodate.

The use case that I had for adding these is when using the "preserve" option of cookie affinity, I want to allow the servers to assign their own cookie using Set-Cookie, and have haproxy simply route it to the server specified by a Set-Cookie header. An example may be something like a MMO server where there is a load balancer in front of many servers, but the users character should always be routed to the server their character exists on after they log in. This requires the servers to specify what cookie value is set, and it requires that cookie value to always be valid; it will break if a dynamic update was to reassign things in such a way that the cookie matched with a server changes.

This is supported by haproxy via the "preserve" option, which preserves the cookie set by the server.

Obviously for this to work properly, the servers as well as the ingress both need to agree on what the cookie values for a particular server are, which is easy to do as long as you use something accessible via the kubernetes Downward API, such as a pod name, or in the case I wanted, a pod UID.

The haproxy config can set the cookie values to a known unique field that is accessible to the servers (eg. pod name, or a pod UID)
When no cookie is present, haproxy will automatically route you to a server based on the balance mode.
Any server can respond with a Set-Cookie to change the server, eg. say if it was a MMO server that wanted to redirect the client to which server their character is on after they log in.

Without any changes, this doesn't work when dynamic updating is on because the cookie will just be something like srv001 until reloading. By making the dynamic updater understand when "preserve" cookie mode is enabled, we can tell it to not do a dynamic update (and do a reload instead) if it would cause a cookie value mismatch, fixing this, while allowing dynamic updates to still be used if the cookie matches.

Changes:

add backend option session-cookie-value-strategy which allows changing how the cookie value is calculated. Right now it supports choosing between naming the cookie value as the endpoint Name (the default, and current behavior), and a pod UID.
add backend option session-cookie-preserve which sets preserve option on the cookie. This was already achievable by setting preserve as part of session-cookie-keywords, however there wasn't a good way to make special behavior for it such as having different logic for dynamic updates without disabling dynamic updating altogether, and the existing documentation wasn't totally clear that dynamic updating breaks cookies when used like this. With this option, when dynamic updating, it knows to reload if a cookie value change is inevitable, making sure that servers are always assigned the correct cookie.
documented the above
added tests for the above

The changes in here don't affect backwards compatibility and aren't breaking:

if "preserve" is set as "session-cookie-keywords" it keeps the old behavior and just logs a message suggesting to consider using the new option
since the new preserve option is off by default, it doesn't affect dynamic reload behavior unless you turn it on
the old cookie values for disabled servers were set to the default naming scheme, srv001, srv002, etc, and those are what got used when using dynamic update; this logic has been preserved when using the old existing configurations

Tested:

✔️ cookie affinity on, multiple servers, with and without the new preserve option and cookie naming UID option
✔️ cookie affinity on, dynamic cookie enabled, multiple servers
✔️ cookie affinity off, multiple servers
✔️ dynamic update with and without the new preserve option via scaling services up/down
✔️ make test all pass

jcmoraisjr · 2020-09-16T13:33:50Z

Thank you for the kind words and for supporting this humble controller. Btw thank you also for such a detailed description and a carefully built pr.

Sorry about the long delay, I'll need a few more days to have a look in the changes. In the mean time there is some suggestions I'd like to share:

there is a new backend.CookieAffinity() func that I think you planned to use in the template but didn't use;
try to separate non functional but needed changes in another commit in order to clearly see what's a requisite and what's the functionality itself. You can eg create a new commit to change how dynupdate references an endpoint (name to object). Types+code+doc+tests however is preferable into a single commit;
try also to use up to 50 chars in the commit title and up to 72 chars in the commit message. A commit message isn't mandatory but it's a good addition if isn't that clear what's the intention behind the change.

Regarding a contributions guideline it's a concept being built along the years writing the code but never translated to words. Just to point an example, the git commit title and message has about one week - this is both a common practice and I observed that gitlab doesn't break lines in the commit preview, so we need to do it ourselves. I need to stop a few days to revise some docs for new users and new contributors, this should help in the visibility of the project.

sigseg1v · 2020-09-16T16:57:49Z

Thanks for the guidelines! I can definitely follow those. Did you want me to reconstruct the commits of this pull request directly by rewriting the history, or do you want me to make a new branch and PR with the rewritten commits? Some projects are ok with rewriting pull request branches but I just wanted to check with you first.

jcmoraisjr · 2020-09-16T17:35:35Z

No, lets make it simple. Just a ~~rebase~~ amend/rewrite is fine.

To access fields other than the name in an empty endpoint, temporarily store the object itself, rather than just the name string.

Currently, the CookieValue will just be set to Name when cookie affinity is enabled, which is how it already works. Separating CookieValue out to a different field allows us to add future logic which will compute the CookieValue differently from the name.

The 'session-cookie-preserve` annotation is able to be used when cookie affinity is enabled. The purpose is to be able to intelligently control dynamic updating in such a way where it avoids causing an update that would make the cookie value for a server no longer match an Enabled server (mismatch on disabling a server is fine). The reason cookie values matching their assigned backend is important, is because the "preserve" mode of HAProxy allows servers to emit a "Set-Cookie" header to effectively assign their own server. Because of this, the specific cookie value assigned to each backend server must be consistent and known by both HAProxy as well as the backend server itself. This change makes usage of "preserve" mode in "session-cookie-keywords" no longer (or at least, less) desirable, but it should still be allowed for backward compatibility and maximum customizability, therefore a warning will simply be logged when it is used in that way.

This new backend annotation allows the way that a cookie value for a backend server is calculated to be controlled more precisely. Prior to this, the value of a cookie for a backend server was always set to the Name of an endpoint, which was in turn controlled by 'backend-server-naming'. While that gives a few options for changing the cookie value, the downside was that it required changing the name of the server. Additionally, there are possible cookie values which would make poor backend server names (such as a guid/uuid) but may make good cookie values. The new annotation defaults to setting the naming strategy to the Name of the server, which is the existing behaviour. In addition, it adds the ability to set the value to a Pod UID.

sigseg1v · 2020-09-17T00:39:50Z

Done. When recreating the commits, to make things simple, the following changed from the original submission:

removed some unnecessary changes to the test endpoints in pkg/haproxy/instance_test.go, to keep number of changes down
used the backend.CookieAffinity() func in haproxy.tmpl

jcmoraisjr

Thanks again for the contribution and sorry about the long delay. I needed to familiarize myself again with sticky session related options. Below has a few minor suggestions and doubts.

jcmoraisjr · 2020-09-23T00:02:09Z

pkg/converters/ingress/annotations/backend.go

+
+	if strings.Contains(d.backend.Cookie.Keywords, "preserve") {
+		// just warn, no error, for keeping backwards compatibility where "preserve" may have been used in the "keywords" section
+		c.logger.Warn("session-cookie-keywords contains 'preserve'; consider using 'session-cookie-preserve' instead for better dynamic update cookie persistence")


Add the source of the annotation here, something like "session-cookie-keywords on %s contains 'preserve'...". d.mapper.Get(...).Source has the source in string format and %s does not need to be quoted. This way one can easily find its ingress or service on a large k8s cluster. It'd suggest also to extract to a local var and use .Bool() and Source from there.

Implemented in 474d3c5

By .Bool() I am thinking you meant just .Value as it's a string value. Let me know if I misinterpreted that part.

Sure, yes, .Value. I did a little mess with session-cookie-preserve which would use .Bool().

jcmoraisjr · 2020-09-23T00:03:21Z

pkg/converters/ingress/annotations/backend.go

+	case "server-name":
+		d.backend.EpCookieStrategy = hatypes.EpCookieName
+	default:
+		c.logger.Warn("invalid session-cookie-value-strategy '%s', using 'server-name' instead", cookieStrategy)


Add source here as well.

Implemented in e7cf7f5

jcmoraisjr · 2020-09-23T00:12:32Z

pkg/converters/ingress/ingress.go

+					if err == nil {
+						ep.CookieValue = fmt.Sprintf("%v", pod.UID)
+					} else {
+						c.logger.Error("error calculating cookie value for pod %s: %v", ep.TargetRef, err)


backend.AcquireEndpoint() doesn't fill CookieValue, shouldn't be assigned to ep.Name in the case of an error reading the pod UID?

Implemented in 0d58edb

jcmoraisjr · 2020-09-23T00:13:23Z

pkg/converters/ingress/ingress.go

+				ep.CookieValue = ep.Name
+			case hatypes.EpCookiePodUid:
+				if ep.TargetRef != "" {
+					pod, err := c.cache.GetPod(ep.TargetRef)


Update --disable-pod-list command-line doc that the pod list is also being used by session-cookie-preserve as well.

Implemented in 099b1f6

sigseg1v · 2020-09-25T02:07:27Z

Thank you for the review. I'll check it out and address the comments in a couple days.

jcmoraisjr · 2020-09-26T22:18:16Z

LGTM, thanks! Merging.

jcmoraisjr added the v0.12 label Sep 16, 2020

sigseg1v added 5 commits September 16, 2020 12:23

store all empty endpoint fields, not just name

425d265

To access fields other than the name in an empty endpoint, temporarily store the object itself, rather than just the name string.

extract cookie affinity calculation into function

4ecd687

sigseg1v force-pushed the session-cookie-preserve branch from f3c149d to 379a859 Compare September 17, 2020 00:36

jcmoraisjr reviewed Sep 23, 2020

View reviewed changes

sigseg1v added 4 commits September 26, 2020 10:42

Add source to warning message for cookie keywords

474d3c5

Add source and test for cookie value strategy warn

e7cf7f5

Fallback to name cookie value strategy on error

0d58edb

Docs for cookie naming with disable-pod-list

099b1f6

jcmoraisjr merged commit cb6f88a into jcmoraisjr:master Sep 26, 2020

jcmoraisjr changed the title ~~Add better handling for cookie affinity with preserve option: add 'session-cookie-preserve' and 'session-cookie-value-strategy' options~~ Add better handling for cookie affinity with preserve option Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add better handling for cookie affinity with preserve option #667

Add better handling for cookie affinity with preserve option #667

sigseg1v commented Sep 15, 2020

jcmoraisjr commented Sep 16, 2020

sigseg1v commented Sep 16, 2020

jcmoraisjr commented Sep 16, 2020 •

edited

Loading

sigseg1v commented Sep 17, 2020

jcmoraisjr left a comment

jcmoraisjr Sep 23, 2020

sigseg1v Sep 26, 2020

jcmoraisjr Sep 26, 2020

jcmoraisjr Sep 23, 2020

sigseg1v Sep 26, 2020

jcmoraisjr Sep 23, 2020

sigseg1v Sep 26, 2020

jcmoraisjr Sep 23, 2020

sigseg1v Sep 26, 2020

sigseg1v commented Sep 25, 2020

jcmoraisjr commented Sep 26, 2020

Add better handling for cookie affinity with preserve option #667

Add better handling for cookie affinity with preserve option #667

Conversation

sigseg1v commented Sep 15, 2020

jcmoraisjr commented Sep 16, 2020

sigseg1v commented Sep 16, 2020

jcmoraisjr commented Sep 16, 2020 • edited Loading

sigseg1v commented Sep 17, 2020

jcmoraisjr 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

sigseg1v commented Sep 25, 2020

jcmoraisjr commented Sep 26, 2020

jcmoraisjr commented Sep 16, 2020 •

edited

Loading