Add better handling for cookie affinity with preserve option

docs/content/en/docs/configuration/command-line.md

@@ -124,7 +124,7 @@ This is a mandatory argument used in the [deployment](https://github.com/jcmorai
 
 Since v0.11
 
-Disables in memory pod list and also pod watch for changes. Pod list and watch is used by `drain-support` option, which will not work if pod list is disabled. Blue/green also uses pod list if enabled, otherwise k8s api is called if needed. The default value is `false`, which means pods will be watched and listed in memory.
+Disables in memory pod list and also pod watch for changes. Pod list and watch is used by `drain-support` option, which will not work if pod list is disabled. Blue/green and `session-cookie-value-strategy` set to `pod-uid` also use pod list if enabled, otherwise k8s api is called if needed. The default value is `false`, which means pods will be watched and listed in memory.
 
 ---
 

docs/content/en/docs/configuration/keys.md

@@ -199,8 +199,10 @@ The table below describes all supported configuration keys.
 | [`session-cookie-dynamic`](#affinity)                | [true\|false]                           | Backend |                    |
 | [`session-cookie-keywords`](#affinity)               | cookie options                          | Backend | `indirect nocache httponly`     |
 | [`session-cookie-name`](#affinity)                   | cookie name                             | Backend |                    |
+| [`session-cookie-preserve`](#affinity)               | [true\|false]                           | Backend | `false`            |
 | [`session-cookie-shared`](#affinity)                 | [true\|false]                           | Backend | `false`            |
 | [`session-cookie-strategy`](#affinity)               | [insert\|prefix\|rewrite]               | Backend |                    |
+| [`session-cookie-value-strategy`](#affinity)         | [server-name\|pod-uid]                  | Backend | `server-name`      |
 | [`slots-min-free`](#dynamic-scaling)                 | minimum number of free slots            | Backend | `0`                |
 | [`ssl-cipher-suites`](#ssl-ciphers)                  | colon-separated list                    | Host    | [see description](#ssl-ciphers) |
 | [`ssl-cipher-suites-backend`](#ssl-ciphers)          | colon-separated list                    | Backend | [see description](#ssl-ciphers) |
@@ -340,25 +342,29 @@ See also:
 
 ## Affinity
 
-| Configuration key           | Scope     | Default                     | Since |
-|-----------------------------|-----------|-----------------------------|-------|
-| `affinity`                  | `Backend` | `false`                     |       |
-| `cookie-key`                | `Global`  | `Ingress`                   |       |
-| `session-cookie-dynamic`    | `Backend` | `true`                      |       |
-| `session-cookie-keywords`   | `Backend` | `indirect nocache httponly` | v0.11 |
-| `session-cookie-name`       | `Backend` | `INGRESSCOOKIE`             |       |
-| `session-cookie-shared`     | `Backend` | `false`                     | v0.8  |
-| `session-cookie-strategy`   | `Backend` | `insert`                    |       |
+| Configuration key               | Scope     | Default                     | Since |
+|---------------------------------|-----------|-----------------------------|-------|
+| `affinity`                      | `Backend` | `false`                     |       |
+| `cookie-key`                    | `Global`  | `Ingress`                   |       |
+| `session-cookie-dynamic`        | `Backend` | `true`                      |       |
+| `session-cookie-keywords`       | `Backend` | `indirect nocache httponly` | v0.11 |
+| `session-cookie-name`           | `Backend` | `INGRESSCOOKIE`             |       |
+| `session-cookie-preserve`       | `Backend` | `false`                     | v0.12 |
+| `session-cookie-shared`         | `Backend` | `false`                     | v0.8  |
+| `session-cookie-strategy`       | `Backend` | `insert`                    |       |
+| `session-cookie-value-strategy` | `Backend` | `server-name`               | v0.12 |
 
 Configure if HAProxy should maintain client requests to the same backend server.
 
 * `affinity`: the only supported option is `cookie`. If declared, clients will receive a cookie with a hash of the server it should be fidelized to.
 * `cookie-key`: defines a secret key used with the IP address and port number of a backend server to dynamically create a cookie to that server. Defaults to `Ingress` if not provided.
 * `session-cookie-dynamic`: indicates whether or not dynamic cookie value will be used. With the default of `true`, a cookie value will be generated by HAProxy using a hash of the server IP address, TCP port, and dynamic cookie secret key. When `false`, the server name will be used as the cookie name. Note that setting this to `false` will have no impact if [use-resolver](#dns-resolvers) is set.
-* `session-cookie-keywords`: additional options to the `cookie` option like `nocache`, `preserve`, `httponly`. For the sake of backwards compatibility the default is `indirect nocache httponly` if not declared and `strategy` is `insert`.
+* `session-cookie-keywords`: additional options to the `cookie` option like `nocache`, `httponly`. For the sake of backwards compatibility the default is `indirect nocache httponly` if not declared and `strategy` is `insert`.
 * `session-cookie-name`: the name of the cookie. `INGRESSCOOKIE` is the default value if not declared.
+* `session-cookie-preserve`: indicates whether the session cookie will be set to `preserve` mode. If this mode is enabled, haproxy will allow backend servers to use a `Set-Cookie` HTTP header to emit their own persistence cookie value, meaning the backend servers have knowledge of which cookie value should route to which server. Since the cookie value is tightly coupled with a particular backend server in this scenario, this mode will cause dynamic updating to understand that it must keep the same cookie value associated with the same backend server. If this is disabled, dynamic updating is free to assign servers in a way that can make their cookie value no longer matching.
 * `session-cookie-shared`: defines if the persistence cookie should be shared between all domains that uses this backend. Defaults to `false`. If `true` the `Set-Cookie` response will declare all the domains that shares this backend, indicating to the HTTP agent that all of them should use the same backend server.
 * `session-cookie-strategy`: the cookie strategy to use (insert, rewrite, prefix). `insert` is the default value if not declared.
+* `session-cookie-value-strategy`: the strategy to use to calculate the cookie value of a server (`server-name`, `pod-uid`). `server-name` is the default if not declared, and indicates that the cookie will be set based on the name defined in `backend-server-naming`. `pod-uid` indicates that the cookie will be set to the `UID` of the pod running the target server.
 
 Note for `dynamic-scaling` users only, v0.5 or older: the hash of the server is built based on it's name.
 When the slots are scaled down, the remaining servers might change it's server name on
@@ -498,7 +504,7 @@ Configures how to name backend servers.
 * `ip`: Uses target's `<ip>:<port>` as the server name.
 
 {{% alert title="Note" %}}
-HAProxy Ingress won't refuse to change the default naming if dynamic update is `true`, this would however lead to undesired behaviour: empty slots would still be named as sequences, old-named backend servers will dynamically receive new workloads with new pod names or IP numbers which does not relates with the name anymore, making the naming useless, if not wrong.
+HAProxy Ingress won't refuse to change the default naming if dynamic update is `true`, this would however lead to undesired behaviour: empty slots would still be named as sequences, old-named backend servers will dynamically receive new workloads with new pod names or IP numbers which do not relate with the name anymore, making the naming useless, if not wrong. If you have [cookie affinity](#affinity) enabled, dynamic updating can cause the cookie values to get out of sync with the servers. This can be avoided by using `session-cookie-preserve` with a value of `true`.
 {{% /alert %}}
 
 ---

pkg/converters/ingress/annotations/backend.go

@@ -55,13 +55,32 @@ func (c *updater) buildBackendAffinity(d *backData) {
 	}
 	d.backend.Cookie.Name = name
 	d.backend.Cookie.Strategy = strategyName
-	keywords := d.mapper.Get(ingtypes.BackSessionCookieKeywords).Value
-	if strategyName == "insert" && keywords == "" {
-		keywords = "indirect nocache httponly"
+	keywords := d.mapper.Get(ingtypes.BackSessionCookieKeywords)
+	keywordsValue := keywords.Value
+	if strategyName == "insert" && keywordsValue == "" {
+		keywordsValue = "indirect nocache httponly"
 	}
-	d.backend.Cookie.Keywords = keywords
+	if strings.Contains(keywordsValue, "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 on %s contains 'preserve'; consider using 'session-cookie-preserve' instead for better dynamic update cookie persistence", keywords.Source)
+	}
+	d.backend.Cookie.Keywords = keywordsValue
 	d.backend.Cookie.Dynamic = d.mapper.Get(ingtypes.BackSessionCookieDynamic).Bool()
+	d.backend.Cookie.Preserve = d.mapper.Get(ingtypes.BackSessionCookiePreserve).Bool()
 	d.backend.Cookie.Shared = d.mapper.Get(ingtypes.BackSessionCookieShared).Bool()
+
+	cookieStrategy := d.mapper.Get(ingtypes.BackSessionCookieValue)
+	switch cookieStrategy.Value {
+	case "pod-uid":
+		d.backend.EpCookieStrategy = hatypes.EpCookiePodUid
+	case "server-name":
+		d.backend.EpCookieStrategy = hatypes.EpCookieName
+	default:
+		c.logger.Warn("invalid session-cookie-value-strategy '%s' on %s, using 'server-name' instead", cookieStrategy.Value, cookieStrategy.Source)
+		fallthrough
+	case "":
+		d.backend.EpCookieStrategy = hatypes.EpCookieName
+	}
 }
 
 func (c *updater) buildBackendAuthHTTP(d *backData) {

pkg/converters/ingress/annotations/backend_test.go

@@ -121,6 +121,41 @@ func TestAffinity(t *testing.T) {
 			expCookie:  hatypes.Cookie{Name: "INGRESSCOOKIE", Strategy: "prefix", Keywords: "nocache"},
 			expLogging: "",
 		},
+		// 10 - test that warning is logged when using "preserve" in the keywords annotation instead of in "session-cookie-preserve" annotation
+		{
+			ann: map[string]string{
+				ingtypes.BackAffinity:              "cookie",
+				ingtypes.BackSessionCookieName:     "serverId",
+				ingtypes.BackSessionCookieStrategy: "insert",
+				ingtypes.BackSessionCookieDynamic:  "false",
+				ingtypes.BackSessionCookieKeywords: "preserve nocache",
+			},
+			expCookie:  hatypes.Cookie{Name: "serverId", Strategy: "insert", Dynamic: false, Preserve: false, Keywords: "preserve nocache"},
+			expLogging: "WARN session-cookie-keywords on ingress 'default/ing1' contains 'preserve'; consider using 'session-cookie-preserve' instead for better dynamic update cookie persistence",
+		},
+		// 11 - test "session-cookie-preserve" cookie annotation is applied
+		{
+			ann: map[string]string{
+				ingtypes.BackAffinity:              "cookie",
+				ingtypes.BackSessionCookieName:     "serverId",
+				ingtypes.BackSessionCookieStrategy: "insert",
+				ingtypes.BackSessionCookieDynamic:  "false",
+				ingtypes.BackSessionCookiePreserve: "true",
+				ingtypes.BackSessionCookieKeywords: "nocache",
+			},
+			expCookie:  hatypes.Cookie{Name: "serverId", Strategy: "insert", Dynamic: false, Preserve: true, Keywords: "nocache"},
+			expLogging: "",
+		},
+		// 12 - test that there is a fallback to using the "name" cookie value strategy
+		{
+			ann: map[string]string{
+				ingtypes.BackAffinity:             "cookie",
+				ingtypes.BackSessionCookieDynamic: "false",
+				ingtypes.BackSessionCookieValue:   "err",
+			},
+			expCookie:  hatypes.Cookie{Name: "INGRESSCOOKIE", Strategy: "insert", Dynamic: false, Keywords: "indirect nocache httponly"},
+			expLogging: "WARN invalid session-cookie-value-strategy 'err' on ingress 'default/ing1', using 'server-name' instead",
+		},
 	}
 
 	source := &Source{

pkg/converters/ingress/defaults.go

@@ -52,6 +52,8 @@ func createDefaults() map[string]string {
 		types.BackHSTSPreload:            "false",
 		types.BackInitialWeight:          "1",
 		types.BackSessionCookieDynamic:   "true",
+		types.BackSessionCookiePreserve:  "false",
+		types.BackSessionCookieValue:     "server-name",
 		types.BackSSLRedirect:            "true",
 		types.BackSSLCipherSuitesBackend: defaultSSLCipherSuites,
 		types.BackSSLCiphersBackend:      defaultSSLCiphers,

pkg/converters/ingress/ingress.go

@@ -169,6 +169,7 @@ func (c *converter) syncFull() {
 		c.syncIngress(ing)
 	}
 	c.fullSyncAnnotations()
+	c.syncEndpointCookies()
 }
 
 func (c *converter) syncPartial() {
@@ -281,6 +282,7 @@ func (c *converter) syncPartial() {
 		c.syncIngress(ing)
 	}
 	c.partialSyncAnnotations()
+	c.syncChangedEndpointCookies()
 }
 
 // trackAddedIngress add tracking hostnames and backends to new ingress objects
@@ -436,6 +438,18 @@ func (c *converter) syncIngress(ing *networking.Ingress) {
 	}
 }
 
+func (c *converter) syncEndpointCookies() {
+	for _, backend := range c.haproxy.Backends().Items() {
+		c.syncBackendEndpointCookies(backend)
+	}
+}
+
+func (c *converter) syncChangedEndpointCookies() {
+	for _, backend := range c.haproxy.Backends().ItemsAdd() {
+		c.syncBackendEndpointCookies(backend)
+	}
+}
+
 func (c *converter) fullSyncAnnotations() {
 	c.updater.UpdateGlobalConfig(c.haproxy, c.globalConfig)
 	for _, host := range c.haproxy.Hosts().Items() {
@@ -597,6 +611,28 @@ func (c *converter) addBackend(source *annotations.Source, hostname, uri, fullSv
 	return backend, nil
 }
 
+func (c *converter) syncBackendEndpointCookies(backend *hatypes.Backend) {
+	cookieAffinity := backend.CookieAffinity()
+	for _, ep := range backend.Endpoints {
+		if cookieAffinity {
+			switch backend.EpCookieStrategy {
+			default:
+				ep.CookieValue = ep.Name
+			case hatypes.EpCookiePodUid:
+				if ep.TargetRef != "" {
+					pod, err := c.cache.GetPod(ep.TargetRef)
+					if err == nil {
+						ep.CookieValue = fmt.Sprintf("%v", pod.UID)
+					} else {
+						ep.CookieValue = ep.Name
+						c.logger.Error("error calculating cookie value for pod %s; falling back to 'server-name' strategy: %v", ep.TargetRef, err)
+					}
+				}
+			}
+		}
+	}
+}
+
 func (c *converter) addTLS(source *annotations.Source, hostname, secretName string) convtypes.CrtFile {
 	if secretName != "" {
 		tlsFile, err := c.cache.GetTLSSecretPath(

pkg/converters/ingress/types/annotations.go

@@ -119,8 +119,10 @@ const (
 	BackSessionCookieDynamic   = "session-cookie-dynamic"
 	BackSessionCookieKeywords  = "session-cookie-keywords"
 	BackSessionCookieName      = "session-cookie-name"
+	BackSessionCookiePreserve  = "session-cookie-preserve"
 	BackSessionCookieShared    = "session-cookie-shared"
 	BackSessionCookieStrategy  = "session-cookie-strategy"
+	BackSessionCookieValue     = "session-cookie-value-strategy"
 	BackSSLCipherSuitesBackend = "ssl-cipher-suites-backend"
 	BackSSLCiphersBackend      = "ssl-ciphers-backend"
 	BackSSLFingerprintLower    = "ssl-fingerprint-lower"

pkg/haproxy/dynupdate.go

@@ -165,13 +165,13 @@ func (d *dynUpdater) checkBackendPair(pair *backendPair) bool {
 	// map endpoints of old and new config together
 	endpoints := make(map[string]*epPair, len(oldBack.Endpoints))
 	targets := make([]string, 0, len(oldBack.Endpoints))
-	var empty []string
+	var empty []*hatypes.Endpoint
 	for _, endpoint := range oldBack.Endpoints {
 		if endpoint.Enabled {
 			endpoints[endpoint.Target] = &epPair{old: endpoint}
 			targets = append(targets, endpoint.Target)
 		} else {
-			empty = append(empty, endpoint.Name)
+			empty = append(empty, endpoint)
 		}
 	}
 
@@ -202,33 +202,42 @@ func (d *dynUpdater) checkBackendPair(pair *backendPair) bool {
 			if !d.execDisableEndpoint(curBack.ID, pair.old) || pair.old.Label != "" {
 				updated = false
 			}
-			empty = append(empty, pair.old.Name)
-		} else if !d.checkEndpointPair(curBack.ID, pair) {
+			empty = append(empty, pair.old)
+		} else if !d.checkEndpointPair(curBack, pair) {
 			updated = false
 		}
 	}
 	for i := range added {
 		// reusing empty slots from oldBack
-		added[i].Name = empty[i]
-		if !d.execEnableEndpoint(curBack.ID, nil, added[i]) || added[i].Label != "" {
+		added[i].Name = empty[i].Name
+		if curBack.Cookie.Preserve && added[i].CookieValue != empty[i].CookieValue {
+			// if cookie doesn't match here and preserving the value is
+			// important, don't even enable the endpoint before reloading
+			updated = false
+		} else if !d.execEnableEndpoint(curBack.ID, nil, added[i]) || added[i].Label != "" {
 			updated = false
 		}
 	}
 
 	// copy remaining empty slots from oldBack to curBack, so it can be used in a future update
 	for i := len(added); i < len(empty); i++ {
-		curBack.AddEmptyEndpoint().Name = empty[i]
+		curBack.AddEmptyEndpoint().Name = empty[i].Name
 	}
 	curBack.SortEndpoints()
 
 	return updated
 }
 
-func (d *dynUpdater) checkEndpointPair(backname string, pair *epPair) bool {
+func (d *dynUpdater) checkEndpointPair(backend *hatypes.Backend, pair *epPair) bool {
 	if reflect.DeepEqual(pair.old, pair.cur) {
 		return true
 	}
-	updated := d.execEnableEndpoint(backname, pair.old, pair.cur)
+	if backend.Cookie.Preserve && pair.old.CookieValue != pair.cur.CookieValue {
+		// if cookie doesn't match here and preserving the value is
+		// important, don't even enable the endpoint before reloading
+		return false
+	}
+	updated := d.execEnableEndpoint(backend.ID, pair.old, pair.cur)
 	if !updated || pair.old.Label != "" || pair.cur.Label != "" {
 		return false
 	}