Handle request_id variable correctly in auth requests (#9219)

* Handle $request_id variable correctly in auth requests

* Make share_all_vars configurable

* Fix test name

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

@@ -33,6 +33,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz
 |[nginx.ingress.kubernetes.io/auth-cache-key](#external-authentication)|string|
 |[nginx.ingress.kubernetes.io/auth-cache-duration](#external-authentication)|string|
 |[nginx.ingress.kubernetes.io/auth-keepalive](#external-authentication)|number|
+|[nginx.ingress.kubernetes.io/auth-keepalive-share-vars](#external-authentication)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/auth-keepalive-requests](#external-authentication)|number|
 |[nginx.ingress.kubernetes.io/auth-keepalive-timeout](#external-authentication)|number|
 |[nginx.ingress.kubernetes.io/auth-proxy-set-headers](#external-authentication)|string|
@@ -467,6 +468,9 @@ Additionally it is possible to set:
 > Note: does not work with HTTP/2 listener because of a limitation in Lua [subrequests](https://github.com/openresty/lua-nginx-module#spdy-mode-not-fully-supported).
 > [UseHTTP2](./configmap.md#use-http2) configuration should be disabled!
 
+* `nginx.ingress.kubernetes.io/auth-keepalive-share-vars`:
+  Whether to share Nginx variables among the current request and the auth request. Example use case is to track requests: when set to "true" X-Request-ID HTTP header will be the same for the backend and the auth request.
+  Defaults to "false".
 * `nginx.ingress.kubernetes.io/auth-keepalive-requests`:
   `<Requests>` to specify the maximum number of requests that can be served through one keepalive connection.
   Defaults to `1000` and only applied if `auth-keepalive` is set to higher than `0`.

internal/ingress/annotations/authreq/main.go

@@ -40,6 +40,7 @@ const (
 	authReqSnippetAnnotation            = "auth-snippet"
 	authReqCacheKeyAnnotation           = "auth-cache-key"
 	authReqKeepaliveAnnotation          = "auth-keepalive"
+	authReqKeepaliveShareVarsAnnotation = "auth-keepalive-share-vars"
 	authReqKeepaliveRequestsAnnotation  = "auth-keepalive-requests"
 	authReqKeepaliveTimeout             = "auth-keepalive-timeout"
 	authReqCacheDuration                = "auth-cache-duration"
@@ -97,6 +98,12 @@ var authReqAnnotations = parser.Annotation{
 			Risk:          parser.AnnotationRiskLow,
 			Documentation: `This annotation specifies the maximum number of keepalive connections to auth-url. Only takes effect when no variables are used in the host part of the URL`,
 		},
+		authReqKeepaliveShareVarsAnnotation: {
+			Validator:     parser.ValidateBool,
+			Scope:         parser.AnnotationScopeLocation,
+			Risk:          parser.AnnotationRiskLow,
+			Documentation: `This annotation specifies whether to share Nginx variables among the current request and the auth request`,
+		},
 		authReqKeepaliveRequestsAnnotation: {
 			Validator:     parser.ValidateInt,
 			Scope:         parser.AnnotationScopeLocation,
@@ -158,6 +165,7 @@ type Config struct {
 	AuthCacheKey           string            `json:"authCacheKey"`
 	AuthCacheDuration      []string          `json:"authCacheDuration"`
 	KeepaliveConnections   int               `json:"keepaliveConnections"`
+	KeepaliveShareVars     bool              `json:"keepaliveShareVars"`
 	KeepaliveRequests      int               `json:"keepaliveRequests"`
 	KeepaliveTimeout       int               `json:"keepaliveTimeout"`
 	ProxySetHeaders        map[string]string `json:"proxySetHeaders,omitempty"`
@@ -170,6 +178,7 @@ const DefaultCacheDuration = "200 202 401 5m"
 // fallback values when no keepalive parameters are set
 const (
 	defaultKeepaliveConnections = 0
+	defaultKeepaliveShareVars   = false
 	defaultKeepaliveRequests    = 1000
 	defaultKeepaliveTimeout     = 60
 )
@@ -218,6 +227,10 @@ func (e1 *Config) Equal(e2 *Config) bool {
 		return false
 	}
 
+	if e1.KeepaliveShareVars != e2.KeepaliveShareVars {
+		return false
+	}
+
 	if e1.KeepaliveRequests != e2.KeepaliveRequests {
 		return false
 	}
@@ -357,6 +370,12 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) {
 		}
 	}
 
+	keepaliveShareVars, err := parser.GetBoolAnnotation(authReqKeepaliveShareVarsAnnotation, ing, a.annotationConfig.Annotations)
+	if err != nil {
+		klog.V(3).InfoS("auth-keepalive-share-vars annotation is undefined and will be set to its default value")
+		keepaliveShareVars = defaultKeepaliveShareVars
+	}
+
 	keepaliveRequests, err := parser.GetIntAnnotation(authReqKeepaliveRequestsAnnotation, ing, a.annotationConfig.Annotations)
 	if err != nil {
 		klog.V(3).InfoS("auth-keepalive-requests annotation is undefined or invalid and will be set to its default value")
@@ -467,6 +486,7 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) {
 		AuthCacheKey:           authCacheKey,
 		AuthCacheDuration:      authCacheDuration,
 		KeepaliveConnections:   keepaliveConnections,
+		KeepaliveShareVars:     keepaliveShareVars,
 		KeepaliveRequests:      keepaliveRequests,
 		KeepaliveTimeout:       keepaliveTimeout,
 		ProxySetHeaders:        proxySetHeaders,

internal/ingress/annotations/authreq/main_test.go

@@ -268,28 +268,31 @@ func TestKeepaliveAnnotations(t *testing.T) {
 		title                string
 		url                  string
 		keepaliveConnections string
+		keepaliveShareVars   string
 		keepaliveRequests    string
 		keepaliveTimeout     string
 		expectedConnections  int
+		expectedShareVars    bool
 		expectedRequests     int
 		expectedTimeout      int
 	}{
-		{"all set", "http://goog.url", "5", "500", "50", 5, 500, 50},
+		{"all set", "http://goog.url", "5", "false", "500", "50", 5, false, 500, 50},
-		{"no annotation", "http://goog.url", "", "", "", defaultKeepaliveConnections, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"no annotation", "http://goog.url", "", "", "", "", defaultKeepaliveConnections, defaultKeepaliveShareVars, defaultKeepaliveRequests, defaultKeepaliveTimeout},
-		{"default for connections", "http://goog.url", "x", "500", "50", defaultKeepaliveConnections, 500, 50},
+		{"default for connections", "http://goog.url", "x", "true", "500", "50", defaultKeepaliveConnections, true, 500, 50},
-		{"default for requests", "http://goog.url", "5", "x", "50", 5, defaultKeepaliveRequests, 50},
+		{"default for requests", "http://goog.url", "5", "x", "dummy", "50", 5, defaultKeepaliveShareVars, defaultKeepaliveRequests, 50},
-		{"default for invalid timeout", "http://goog.url", "5", "500", "x", 5, 500, defaultKeepaliveTimeout},
+		{"default for invalid timeout", "http://goog.url", "5", "t", "500", "x", 5, true, 500, defaultKeepaliveTimeout},
-		{"variable in host", "http://$host:5000/a/b", "5", "", "", 0, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"variable in host", "http://$host:5000/a/b", "5", "1", "", "", 0, true, defaultKeepaliveRequests, defaultKeepaliveTimeout},
-		{"variable in path", "http://goog.url:5000/$path", "5", "", "", 5, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"variable in path", "http://goog.url:5000/$path", "5", "t", "", "", 5, true, defaultKeepaliveRequests, defaultKeepaliveTimeout},
-		{"negative connections", "http://goog.url", "-2", "", "", 0, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"negative connections", "http://goog.url", "-2", "f", "", "", 0, false, defaultKeepaliveRequests, defaultKeepaliveTimeout},
-		{"negative requests", "http://goog.url", "5", "-1", "", 0, -1, defaultKeepaliveTimeout},
+		{"negative requests", "http://goog.url", "5", "True", "-1", "", 0, true, -1, defaultKeepaliveTimeout},
-		{"negative timeout", "http://goog.url", "5", "", "-1", 0, defaultKeepaliveRequests, -1},
+		{"negative timeout", "http://goog.url", "5", "0", "", "-1", 0, false, defaultKeepaliveRequests, -1},
-		{"negative request and timeout", "http://goog.url", "5", "-2", "-3", 0, -2, -3},
+		{"negative request and timeout", "http://goog.url", "5", "False", "-2", "-3", 0, false, -2, -3},
 	}
 
 	for _, test := range tests {
 		data[parser.GetAnnotationWithPrefix("auth-url")] = test.url
 		data[parser.GetAnnotationWithPrefix("auth-keepalive")] = test.keepaliveConnections
+		data[parser.GetAnnotationWithPrefix("auth-keepalive-share-vars")] = test.keepaliveShareVars
 		data[parser.GetAnnotationWithPrefix("auth-keepalive-timeout")] = test.keepaliveTimeout
 		data[parser.GetAnnotationWithPrefix("auth-keepalive-requests")] = test.keepaliveRequests
 
@@ -313,6 +316,10 @@ func TestKeepaliveAnnotations(t *testing.T) {
 			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.expectedConnections, u.KeepaliveConnections)
 		}
 
+		if u.KeepaliveShareVars != test.expectedShareVars {
+			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.expectedShareVars, u.KeepaliveShareVars)
+		}
+
 		if u.KeepaliveRequests != test.expectedRequests {
 			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.expectedRequests, u.KeepaliveRequests)
 		}

rootfs/etc/nginx/template/nginx.tmpl

@@ -1334,7 +1334,7 @@ stream {
             # `auth_request` module does not support HTTP keepalives in upstream block:
             # https://trac.nginx.org/nginx/ticket/1579
             access_by_lua_block {
-                local res = ngx.location.capture('{{ $authPath }}', { method = ngx.HTTP_GET, body = '' })
+                local res = ngx.location.capture('{{ $authPath }}', { method = ngx.HTTP_GET, body = '', share_all_vars = {{ $externalAuth.KeepaliveShareVars }} })
                 if res.status == ngx.HTTP_OK then
                     ngx.var.auth_cookie = res.header['Set-Cookie']
                     {{- range $line := buildAuthUpstreamLuaHeaders $externalAuth.ResponseHeaders }}

test/e2e/annotations/auth.go

@@ -628,6 +628,45 @@ http {
 						strings.Contains(server, `keepalive_timeout 789s;`)
 				})
 		})
+
+		ginkgo.It(`should disable set_all_vars when auth-keepalive-share-vars is not set`, func() {
+			f.UpdateNginxConfigMapData("use-http2", "false")
+			defer func() {
+				f.UpdateNginxConfigMapData("use-http2", "true")
+			}()
+			// Sleep a while just to guarantee that the configmap is applied
+			framework.Sleep()
+
+			annotations["nginx.ingress.kubernetes.io/auth-keepalive"] = "10"
+			f.UpdateIngress(ing)
+
+			f.WaitForNginxServer("",
+				func(server string) bool {
+					return strings.Contains(server, `upstream auth-external-auth`) &&
+						strings.Contains(server, `keepalive 10;`) &&
+						strings.Contains(server, `share_all_vars = false`)
+				})
+		})
+
+		ginkgo.It(`should enable set_all_vars when auth-keepalive-share-vars is true`, func() {
+			f.UpdateNginxConfigMapData("use-http2", "false")
+			defer func() {
+				f.UpdateNginxConfigMapData("use-http2", "true")
+			}()
+			// Sleep a while just to guarantee that the configmap is applied
+			framework.Sleep()
+
+			annotations["nginx.ingress.kubernetes.io/auth-keepalive"] = "10"
+			annotations["nginx.ingress.kubernetes.io/auth-keepalive-share-vars"] = "true"
+			f.UpdateIngress(ing)
+
+			f.WaitForNginxServer("",
+				func(server string) bool {
+					return strings.Contains(server, `upstream auth-external-auth`) &&
+						strings.Contains(server, `keepalive 10;`) &&
+						strings.Contains(server, `share_all_vars = true`)
+				})
+		})
 	})
 
 	ginkgo.Context("when external authentication is configured with a custom redirect param", func() {