Merge pull request #3372 from Shopify/session-cookie-path

Add annotation for session affinity path

docs/user-guide/nginx-configuration/annotations.md

@@ -70,6 +70,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz
 |[nginx.ingress.kubernetes.io/service-upstream](#service-upstream)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/session-cookie-name](#cookie-affinity)|string|
 |[nginx.ingress.kubernetes.io/session-cookie-hash](#cookie-affinity)|string|
+|[nginx.ingress.kubernetes.io/session-cookie-path](#cookie-affinity)|string|
 |[nginx.ingress.kubernetes.io/ssl-redirect](#server-side-https-enforcement-through-redirect)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/ssl-passthrough](#ssl-passthrough)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/upstream-hash-by](#custom-nginx-upstream-hashing)|string|
@@ -145,6 +146,8 @@ If you use the ``cookie`` affinity type you can also specify the name of the coo
 
 In case of NGINX the annotation `nginx.ingress.kubernetes.io/session-cookie-hash` defines which algorithm will be used to hash the used upstream. Default value is `md5` and possible values are `md5`, `sha1` and `index`.
 
+The NGINX annotation `nginx.ingress.kubernetes.io/session-cookie-path` defines the path that will be set on the cookie. This is optional unless the annotation `nginx.ingress.kubernetes.io/use-regex` is set to true; Session cookie paths do not support regex.  
+
 !!! attention
     The `index` option is not an actual hash; an in-memory index is used instead, which has less overhead.
     However, with `index`, matching against a changing upstream server list is inconsistent.
@@ -717,6 +720,9 @@ nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
 
 ### Use Regex
 
+!!! attention
+When using this annotation with the NGINX annotation `nginx.ingress.kubernetes.io/affinity` of type `cookie`,  `nginx.ingress.kubernetes.io/session-cookie-path` must be also set; Session cookie paths do not support regex.  
+
 Using the `nginx.ingress.kubernetes.io/use-regex` annotation will indicate whether or not the paths defined on an Ingress use regular expressions.  The default value is `false`.
 
 The following will indicate that regular expression paths are being used:

internal/ingress/annotations/sessionaffinity/main.go

@@ -47,6 +47,9 @@ const (
 	// This is used to control the cookie expires, its value is a number of seconds until the
 	// cookie expires
 	annotationAffinityCookieMaxAge = "session-cookie-max-age"
+
+	// This is used to control the cookie path when use-regex is set to true
+	annotationAffinityCookiePath = "session-cookie-path"
 )
 
 var (
@@ -71,6 +74,8 @@ type Cookie struct {
 	Expires string `json:"expires"`
 	// The number of seconds until the cookie expires
 	MaxAge string `json:"maxage"`
+	// The path that a cookie will be set on
+	Path string `json:"path"`
 }
 
 // cookieAffinityParse gets the annotation values related to Cookie Affinity
@@ -106,6 +111,12 @@ func (a affinity) cookieAffinityParse(ing *extensions.Ingress) *Cookie {
 		sm = ""
 	}
 	cookie.MaxAge = sm
+
+	sp, err := parser.GetStringAnnotation(annotationAffinityCookiePath, ing)
+	if err != nil || sp == "" {
+		glog.V(3).Infof("Invalid or no annotation value found in Ingress %v: %v. Ignoring it", ing.Name, annotationAffinityCookieMaxAge)
+	}
+	cookie.Path = sp
 	return cookie
 }
 

internal/ingress/annotations/sessionaffinity/main_test.go

@@ -71,6 +71,7 @@ func TestIngressAffinityCookieConfig(t *testing.T) {
 	data[parser.GetAnnotationWithPrefix(annotationAffinityCookieName)] = "INGRESSCOOKIE"
 	data[parser.GetAnnotationWithPrefix(annotationAffinityCookieExpires)] = "4500"
 	data[parser.GetAnnotationWithPrefix(annotationAffinityCookieMaxAge)] = "3000"
+	data[parser.GetAnnotationWithPrefix(annotationAffinityCookiePath)] = "/foo"
 	ing.SetAnnotations(data)
 
 	affin, _ := NewParser(&resolver.Mock{}).Parse(ing)
@@ -80,22 +81,26 @@ func TestIngressAffinityCookieConfig(t *testing.T) {
 	}
 
 	if nginxAffinity.Type != "cookie" {
-		t.Errorf("expected cookie as sticky-type but returned %v", nginxAffinity.Type)
+		t.Errorf("expected cookie as affinity but returned %v", nginxAffinity.Type)
 	}
 
 	if nginxAffinity.Cookie.Hash != "md5" {
-		t.Errorf("expected md5 as sticky-hash but returned %v", nginxAffinity.Cookie.Hash)
+		t.Errorf("expected md5 as session-cookie-hash but returned %v", nginxAffinity.Cookie.Hash)
 	}
 
 	if nginxAffinity.Cookie.Name != "INGRESSCOOKIE" {
-		t.Errorf("expected route as sticky-name but returned %v", nginxAffinity.Cookie.Name)
+		t.Errorf("expected route as session-cookie-name but returned %v", nginxAffinity.Cookie.Name)
 	}
 
 	if nginxAffinity.Cookie.Expires != "4500" {
-		t.Errorf("expected 1h as sticky-expires but returned %v", nginxAffinity.Cookie.Expires)
+		t.Errorf("expected 1h as session-cookie-expires but returned %v", nginxAffinity.Cookie.Expires)
 	}
 
 	if nginxAffinity.Cookie.MaxAge != "3000" {
-		t.Errorf("expected 3000 as sticky-max-age but returned %v", nginxAffinity.Cookie.MaxAge)
+		t.Errorf("expected 3000 as session-cookie-max-age but returned %v", nginxAffinity.Cookie.MaxAge)
+	}
+
+	if nginxAffinity.Cookie.Path != "/foo" {
+		t.Errorf("expected /foo as session-cookie-path but returned %v", nginxAffinity.Cookie.Path)
 	}
 }

internal/ingress/controller/controller.go

@@ -533,16 +533,21 @@ func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([]
 				}
 
 				if anns.SessionAffinity.Type == "cookie" {
+					cookiePath := anns.SessionAffinity.Cookie.Path
+					if anns.Rewrite.UseRegex && cookiePath == "" {
+						glog.Warningf("session-cookie-path should be set when use-regex is true")
+					}
+
 					ups.SessionAffinity.CookieSessionAffinity.Name = anns.SessionAffinity.Cookie.Name
 					ups.SessionAffinity.CookieSessionAffinity.Hash = anns.SessionAffinity.Cookie.Hash
 					ups.SessionAffinity.CookieSessionAffinity.Expires = anns.SessionAffinity.Cookie.Expires
 					ups.SessionAffinity.CookieSessionAffinity.MaxAge = anns.SessionAffinity.Cookie.MaxAge
+					ups.SessionAffinity.CookieSessionAffinity.Path = cookiePath
 
 					locs := ups.SessionAffinity.CookieSessionAffinity.Locations
 					if _, ok := locs[host]; !ok {
 						locs[host] = []string{}
 					}
-
 					locs[host] = append(locs[host], path.Path)
 				}
 			}

internal/ingress/types.go

@@ -143,6 +143,7 @@ type CookieSessionAffinity struct {
 	Expires   string              `json:"expires,omitempty"`
 	MaxAge    string              `json:"maxage,omitempty"`
 	Locations map[string][]string `json:"locations,omitempty"`
+	Path      string              `json:"path,omitempty"`
 }
 
 // Endpoint describes a kubernetes endpoint in a backend

internal/ingress/types_equals.go

@@ -223,6 +223,9 @@ func (csa1 *CookieSessionAffinity) Equal(csa2 *CookieSessionAffinity) bool {
 	if csa1.Hash != csa2.Hash {
 		return false
 	}
+	if csa1.Path != csa2.Path {
+		return false
+	}
 
 	return true
 }

rootfs/etc/nginx/lua/balancer/sticky.lua

@@ -17,6 +17,7 @@ function _M.new(self, backend)
     cookie_name = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["name"] or "route",
     cookie_expires = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["expires"],
     cookie_max_age = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["maxage"],
+    cookie_path = backend["sessionAffinityConfig"]["cookieSessionAffinity"]["path"],
     digest_func = digest_func,
     traffic_shaping_policy = backend.trafficShapingPolicy,
     alternative_backends = backend.alternativeBackends,
@@ -41,10 +42,15 @@ local function set_cookie(self, value)
     ngx.log(ngx.ERR, err)
   end
 
+  local cookie_path = self.cookie_path
+  if not cookie_path then
+    cookie_path = ngx.var.location_path
+  end
+
   local cookie_data = {
     key = self.cookie_name,
     value = value,
-    path = ngx.var.location_path,
+    path = cookie_path,
     domain = ngx.var.host,
     httponly = true,
   }

test/e2e/annotations/affinity.go

@@ -223,4 +223,65 @@ var _ = framework.IngressNginxDescribe("Annotations - Affinity/Sticky Sessions",
 		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring(fmt.Sprintf("Expires=%s", expected)))
 		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Max-Age=259200"))
 	})
+
+	It("should work with use-regex annotation and session-cookie-path", func() {
+		host := "cookie.foo.com"
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/affinity":            "cookie",
+			"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
+			"nginx.ingress.kubernetes.io/use-regex":           "true",
+			"nginx.ingress.kubernetes.io/session-cookie-path": "/foo/bar",
+		}
+
+		ing := framework.NewSingleIngress(host, "/foo/.*", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
+			})
+
+		resp, _, errs := gorequest.New().
+			Get(f.IngressController.HTTPURL+"/foo/bar").
+			Set("Host", host).
+			End()
+
+		md5Regex := regexp.MustCompile("SERVERID=[0-9a-f]{32}")
+		match := md5Regex.FindStringSubmatch(resp.Header.Get("Set-Cookie"))
+		Expect(len(match)).Should(BeNumerically("==", 1))
+
+		Expect(errs).Should(BeEmpty())
+		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring(match[0]))
+		Expect(resp.Header.Get("Set-Cookie")).Should(ContainSubstring("Path=/foo/bar"))
+	})
+
+	It("should warn user when use-regex is true and session-cookie-path is not set", func() {
+		host := "cookie.foo.com"
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/affinity":            "cookie",
+			"nginx.ingress.kubernetes.io/session-cookie-name": "SERVERID",
+			"nginx.ingress.kubernetes.io/use-regex":           "true",
+		}
+
+		ing := framework.NewSingleIngress(host, "/foo/.*", host, f.IngressController.Namespace, "http-svc", 80, &annotations)
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
+			})
+
+		resp, _, errs := gorequest.New().
+			Get(f.IngressController.HTTPURL+"/foo/bar").
+			Set("Host", host).
+			End()
+
+		Expect(errs).Should(BeEmpty())
+		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+
+		logs, err := f.NginxLogs()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(logs).To(ContainSubstring(`session-cookie-path should be set when use-regex is true`))
+	})
 })