Add keepalive support for auth requests (#8219)

* Add keepalive support for auth requests * Fix typo * Address PR comments * Log warning when auth-url contains variable in its host:port * Generate upstream name without replacing dots to underscores in server name * Add comment in the nginx template when the keepalive upstream block is referenced * Workaround for auth_request module ignores keepalive in upstream block * The `auth_request` module does not support HTTP keepalives in upstream block: https://trac.nginx.org/nginx/ticket/1579 * As a workaround we use ngx.location.capture but unfortunately it does not support HTTP/2 so `use-http2` configuration parameter is needed. * Handle PR comments * Address PR comments * Handle invalid values for int parameters * Handle PR comments * Fix e2e test
2022-04-09 05:22:04 +02:00 · 2022-04-09 05:22:04 +02:00 · 83ce21b4dd
commit 83ce21b4dd
parent 5e322f79a1
7 changed files with 570 additions and 12 deletions
--- a/internal/ingress/annotations/authreq/main.go
+++ b/internal/ingress/annotations/authreq/main.go
@ -44,12 +44,22 @@ type Config struct {
 	AuthSnippet            string            `json:"authSnippet"`
 	AuthCacheKey           string            `json:"authCacheKey"`
 	AuthCacheDuration      []string          `json:"authCacheDuration"`
+	KeepaliveConnections   int               `json:"keepaliveConnections"`
+	KeepaliveRequests      int               `json:"keepaliveRequests"`
+	KeepaliveTimeout       int               `json:"keepaliveTimeout"`
 	ProxySetHeaders        map[string]string `json:"proxySetHeaders,omitempty"`
 }

 // DefaultCacheDuration is the fallback value if no cache duration is provided
 const DefaultCacheDuration = "200 202 401 5m"

+// fallback values when no keepalive parameters are set
+const (
+	defaultKeepaliveConnections = 0
+	defaultKeepaliveRequests    = 1000
+	defaultKeepaliveTimeout     = 60
+)
+
 // Equal tests for equality between two Config types
 func (e1 *Config) Equal(e2 *Config) bool {
 	if e1 == e2 {
@ -90,6 +100,18 @@ func (e1 *Config) Equal(e2 *Config) bool {
 		return false
 	}

+	if e1.KeepaliveConnections != e2.KeepaliveConnections {
+		return false
+	}
+
+	if e1.KeepaliveRequests != e2.KeepaliveRequests {
+		return false
+	}
+
+	if e1.KeepaliveTimeout != e2.KeepaliveTimeout {
+		return false
+	}
+
 	return sets.StringElementsMatch(e1.AuthCacheDuration, e2.AuthCacheDuration)
 }

@ -193,6 +215,43 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) {
 		klog.V(3).InfoS("auth-cache-key annotation is undefined and will not be set")
 	}

+	keepaliveConnections, err := parser.GetIntAnnotation("auth-keepalive", ing)
+	if err != nil {
+		klog.V(3).InfoS("auth-keepalive annotation is undefined and will be set to its default value")
+		keepaliveConnections = defaultKeepaliveConnections
+	}
+	switch {
+	case keepaliveConnections < 0:
+		klog.Warningf("auth-keepalive annotation (%s) contains a negative value, setting auth-keepalive to 0", authURL.Host)
+		keepaliveConnections = 0
+	case keepaliveConnections > 0:
+		// NOTE: upstream block cannot reference a variable in the server directive
+		if strings.IndexByte(authURL.Host, '$') != -1 {
+			klog.Warningf("auth-url annotation (%s) contains $ in the host:port part, setting auth-keepalive to 0", authURL.Host)
+			keepaliveConnections = 0
+		}
+	}
+
+	keepaliveRequests, err := parser.GetIntAnnotation("auth-keepalive-requests", ing)
+	if err != nil {
+		klog.V(3).InfoS("auth-keepalive-requests annotation is undefined and will be set to its default value")
+		keepaliveRequests = defaultKeepaliveRequests
+	}
+	if keepaliveRequests <= 0 {
+		klog.Warningf("auth-keepalive-requests annotation (%s) should be greater than zero, setting auth-keepalive to 0", authURL.Host)
+		keepaliveConnections = 0
+	}
+
+	keepaliveTimeout, err := parser.GetIntAnnotation("auth-keepalive-timeout", ing)
+	if err != nil {
+		klog.V(3).InfoS("auth-keepalive-timeout annotation is undefined and will be set to its default value")
+		keepaliveTimeout = defaultKeepaliveTimeout
+	}
+	if keepaliveTimeout <= 0 {
+		klog.Warningf("auth-keepalive-timeout annotation (%s) should be greater than zero, setting auth-keepalive 0", authURL.Host)
+		keepaliveConnections = 0
+	}
+
 	durstr, _ := parser.GetStringAnnotation("auth-cache-duration", ing)
 	authCacheDuration, err := ParseStringToCacheDurations(durstr)
 	if err != nil {
@ -249,6 +308,9 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) {
 		AuthSnippet:            authSnippet,
 		AuthCacheKey:           authCacheKey,
 		AuthCacheDuration:      authCacheDuration,
+		KeepaliveConnections:   keepaliveConnections,
+		KeepaliveRequests:      keepaliveRequests,
+		KeepaliveTimeout:       keepaliveTimeout,
 		ProxySetHeaders:        proxySetHeaders,
 	}, nil
 }
--- a/internal/ingress/annotations/authreq/main_test.go
+++ b/internal/ingress/annotations/authreq/main_test.go
@ -243,6 +243,71 @@ func TestCacheDurationAnnotations(t *testing.T) {
 	}
 }

+func TestKeepaliveAnnotations(t *testing.T) {
+	ing := buildIngress()
+
+	data := map[string]string{}
+	ing.SetAnnotations(data)
+
+	tests := []struct {
+		title                string
+		url                  string
+		keepaliveConnections string
+		keepaliveRequests    string
+		keepaliveTimeout     string
+		expectedConnections  int
+		expectedRequests     int
+		expectedTimeout      int
+	}{
+		{"all set", "http://goog.url", "5", "500", "50", 5, 500, 50},
+		{"no annotation", "http://goog.url", "", "", "", defaultKeepaliveConnections, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"default for connections", "http://goog.url", "x", "500", "50", defaultKeepaliveConnections, 500, 50},
+		{"default for requests", "http://goog.url", "5", "x", "50", 5, defaultKeepaliveRequests, 50},
+		{"default for invalid timeout", "http://goog.url", "5", "500", "x", 5, 500, defaultKeepaliveTimeout},
+		{"variable in host", "http://$host:5000/a/b", "5", "", "", 0, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"variable in path", "http://goog.url:5000/$path", "5", "", "", 5, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"negative connections", "http://goog.url", "-2", "", "", 0, defaultKeepaliveRequests, defaultKeepaliveTimeout},
+		{"negative requests", "http://goog.url", "5", "-1", "", 0, -1, defaultKeepaliveTimeout},
+		{"negative timeout", "http://goog.url", "5", "", "-1", 0, defaultKeepaliveRequests, -1},
+		{"negative request and timeout", "http://goog.url", "5", "-2", "-3", 0, -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-timeout")] = test.keepaliveTimeout
+		data[parser.GetAnnotationWithPrefix("auth-keepalive-requests")] = test.keepaliveRequests
+
+		i, err := NewParser(&resolver.Mock{}).Parse(ing)
+		if err != nil {
+			t.Errorf("%v: unexpected error: %v", test.title, err)
+			continue
+		}
+
+		u, ok := i.(*Config)
+		if !ok {
+			t.Errorf("%v: expected an External type", test.title)
+			continue
+		}
+
+		if u.URL != test.url {
+			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.url, u.URL)
+		}
+
+		if u.KeepaliveConnections != test.expectedConnections {
+			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.expectedConnections, u.KeepaliveConnections)
+		}
+
+		if u.KeepaliveRequests != test.expectedRequests {
+			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.expectedRequests, u.KeepaliveRequests)
+		}
+
+		if u.KeepaliveTimeout != test.expectedTimeout {
+			t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.expectedTimeout, u.KeepaliveTimeout)
+		}
+	}
+}
+
 func TestParseStringToCacheDurations(t *testing.T) {

 	tests := []struct {
--- a/internal/ingress/controller/template/template.go
+++ b/internal/ingress/controller/template/template.go
@ -42,6 +42,7 @@ import (

 	"k8s.io/ingress-nginx/internal/ingress"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/influxdb"
+	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/ratelimit"
 	"k8s.io/ingress-nginx/internal/ingress/controller/config"
 	ing_net "k8s.io/ingress-nginx/internal/net"
@ -227,7 +228,12 @@ var (
 		"buildAuthLocation":               buildAuthLocation,
 		"shouldApplyGlobalAuth":           shouldApplyGlobalAuth,
 		"buildAuthResponseHeaders":        buildAuthResponseHeaders,
+		"buildAuthUpstreamLuaHeaders":     buildAuthUpstreamLuaHeaders,
 		"buildAuthProxySetHeaders":        buildAuthProxySetHeaders,
+		"buildAuthUpstreamName":           buildAuthUpstreamName,
+		"shouldApplyAuthUpstream":         shouldApplyAuthUpstream,
+		"extractHostPort":                 extractHostPort,
+		"changeHostPort":                  changeHostPort,
 		"buildProxyPass":                  buildProxyPass,
 		"filterRateLimits":                filterRateLimits,
 		"buildRateLimitZones":             buildRateLimitZones,
@ -572,7 +578,14 @@ func shouldApplyGlobalAuth(input interface{}, globalExternalAuthURL string) bool
 	return false
 }

-func buildAuthResponseHeaders(proxySetHeader string, headers []string) []string {
+// buildAuthResponseHeaders sets HTTP response headers when `auth-url` is used.
+// Based on `auth-keepalive` value we use auth_request_set Nginx directives, or
+// we use Lua and Nginx variables instead.
+//
+// NOTE: Unfortunately auth_request module ignores the keepalive directive (see:
+// https://trac.nginx.org/nginx/ticket/1579), that is why we mimic the same
+// functionality with access_by_lua_block.
+func buildAuthResponseHeaders(proxySetHeader string, headers []string, lua bool) []string {
 	res := []string{}

 	if len(headers) == 0 {
@ -580,14 +593,31 @@ func buildAuthResponseHeaders(proxySetHeader string, headers []string) []string
 	}

 	for i, h := range headers {
-		hvar := strings.ToLower(h)
-		hvar = strings.NewReplacer("-", "_").Replace(hvar)
-		res = append(res, fmt.Sprintf("auth_request_set $authHeader%v $upstream_http_%v;", i, hvar))
+		if lua {
+			res = append(res, fmt.Sprintf("set $authHeader%d '';", i))
+		} else {
+			hvar := strings.ToLower(h)
+			hvar = strings.NewReplacer("-", "_").Replace(hvar)
+			res = append(res, fmt.Sprintf("auth_request_set $authHeader%v $upstream_http_%v;", i, hvar))
+		}
 		res = append(res, fmt.Sprintf("%s '%v' $authHeader%v;", proxySetHeader, h, i))
 	}
 	return res
 }

+func buildAuthUpstreamLuaHeaders(headers []string) []string {
+	res := []string{}
+
+	if len(headers) == 0 {
+		return res
+	}
+
+	for i, h := range headers {
+		res = append(res, fmt.Sprintf("ngx.var.authHeader%d = res.header['%s']", i, h))
+	}
+	return res
+}
+
 func buildAuthProxySetHeaders(headers map[string]string) []string {
 	res := []string{}

@ -602,6 +632,76 @@ func buildAuthProxySetHeaders(headers map[string]string) []string {
 	return res
 }

+func buildAuthUpstreamName(input interface{}, host string) string {
+	authPath := buildAuthLocation(input, "")
+	if authPath == "" || host == "" {
+		return ""
+	}
+
+	return fmt.Sprintf("%s-%s", host, authPath[2:])
+}
+
+// shouldApplyAuthUpstream returns true only in case when ExternalAuth.URL and
+// ExternalAuth.KeepaliveConnections are all set
+func shouldApplyAuthUpstream(l interface{}, c interface{}) bool {
+	location, ok := l.(*ingress.Location)
+	if !ok {
+		klog.Errorf("expected an '*ingress.Location' type but %T was returned", l)
+		return false
+	}
+
+	cfg, ok := c.(config.Configuration)
+	if !ok {
+		klog.Errorf("expected a 'config.Configuration' type but %T was returned", c)
+		return false
+	}
+
+	if location.ExternalAuth.URL == "" || location.ExternalAuth.KeepaliveConnections == 0 {
+		return false
+	}
+
+	// Unfortunately, `auth_request` module ignores keepalive in upstream block: https://trac.nginx.org/nginx/ticket/1579
+	// The workaround is to use `ngx.location.capture` Lua subrequests but it is not supported with HTTP/2
+	if cfg.UseHTTP2 {
+		klog.Warning("Upstream keepalive is not supported with HTTP/2")
+		return false
+	}
+
+	return true
+}
+
+// extractHostPort will extract the host:port part from the URL specified by url
+func extractHostPort(url string) string {
+	if url == "" {
+		return ""
+	}
+
+	authURL, err := parser.StringToURL(url)
+	if err != nil {
+		klog.Errorf("expected a valid URL but %s was returned", url)
+		return ""
+	}
+
+	return authURL.Host
+}
+
+// changeHostPort will change the host:port part of the url to value
+func changeHostPort(url string, value string) string {
+	if url == "" {
+		return ""
+	}
+
+	authURL, err := parser.StringToURL(url)
+	if err != nil {
+		klog.Errorf("expected a valid URL but %s was returned", url)
+		return ""
+	}
+
+	authURL.Host = value
+
+	return authURL.String()
+}
+
 // buildProxyPass produces the proxy pass string, if the ingress has redirects
 // (specified through the nginx.ingress.kubernetes.io/rewrite-target annotation)
 // If the annotation nginx.ingress.kubernetes.io/add-base-url:"true" is specified it will
--- a/internal/ingress/controller/template/template_test.go
+++ b/internal/ingress/controller/template/template_test.go
@ -502,14 +502,49 @@ func TestShouldApplyGlobalAuth(t *testing.T) {

 func TestBuildAuthResponseHeaders(t *testing.T) {
 	externalAuthResponseHeaders := []string{"h1", "H-With-Caps-And-Dashes"}
-	expected := []string{
-		"auth_request_set $authHeader0 $upstream_http_h1;",
-		"proxy_set_header 'h1' $authHeader0;",
-		"auth_request_set $authHeader1 $upstream_http_h_with_caps_and_dashes;",
-		"proxy_set_header 'H-With-Caps-And-Dashes' $authHeader1;",
+	tests := []struct {
+		headers  []string
+		expected []string
+		lua      bool
+	}{
+		{
+			headers: externalAuthResponseHeaders,
+			lua:     false,
+			expected: []string{
+				"auth_request_set $authHeader0 $upstream_http_h1;",
+				"proxy_set_header 'h1' $authHeader0;",
+				"auth_request_set $authHeader1 $upstream_http_h_with_caps_and_dashes;",
+				"proxy_set_header 'H-With-Caps-And-Dashes' $authHeader1;",
+			},
+		},
+		{
+			headers: externalAuthResponseHeaders,
+			lua:     true,
+			expected: []string{
+				"set $authHeader0 '';",
+				"proxy_set_header 'h1' $authHeader0;",
+				"set $authHeader1 '';",
+				"proxy_set_header 'H-With-Caps-And-Dashes' $authHeader1;",
+			},
+		},
 	}

-	headers := buildAuthResponseHeaders(proxySetHeader(nil), externalAuthResponseHeaders)
+	for _, test := range tests {
+		got := buildAuthResponseHeaders(proxySetHeader(nil), test.headers, test.lua)
+		if !reflect.DeepEqual(test.expected, got) {
+			t.Errorf("Expected \n'%v'\nbut returned \n'%v'", test.expected, got)
+		}
+	}
+}
+
+func TestBuildAuthResponseLua(t *testing.T) {
+	externalAuthResponseHeaders := []string{"h1", "H-With-Caps-And-Dashes"}
+	expected := []string{
+		"ngx.var.authHeader0 = res.header['h1']",
+		"ngx.var.authHeader1 = res.header['H-With-Caps-And-Dashes']",
+	}
+
+	headers := buildAuthUpstreamLuaHeaders(externalAuthResponseHeaders)

 	if !reflect.DeepEqual(expected, headers) {
 		t.Errorf("Expected \n'%v'\nbut returned \n'%v'", expected, headers)
@ -533,6 +568,139 @@ func TestBuildAuthProxySetHeaders(t *testing.T) {
 	}
 }

+func TestBuildAuthUpstreamName(t *testing.T) {
+	invalidType := &ingress.Ingress{}
+	expected := ""
+	actual := buildAuthUpstreamName(invalidType, "")
+
+	if !reflect.DeepEqual(expected, actual) {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	loc := &ingress.Location{
+		ExternalAuth: authreq.Config{
+			URL: "foo.com/auth",
+		},
+		Path: "/cat",
+	}
+
+	encodedAuthURL := strings.Replace(base64.URLEncoding.EncodeToString([]byte(loc.Path)), "=", "", -1)
+	externalAuthPath := fmt.Sprintf("external-auth-%v-default", encodedAuthURL)
+
+	testCases := []struct {
+		title    string
+		host     string
+		expected string
+	}{
+		{"valid host", "auth.my.site", fmt.Sprintf("%s-%s", "auth.my.site", externalAuthPath)},
+		{"valid host", "your.auth.site", fmt.Sprintf("%s-%s", "your.auth.site", externalAuthPath)},
+		{"empty host", "", ""},
+	}
+
+	for _, testCase := range testCases {
+		str := buildAuthUpstreamName(loc, testCase.host)
+		if str != testCase.expected {
+			t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, str)
+		}
+	}
+}
+
+func TestShouldApplyAuthUpstream(t *testing.T) {
+	authURL := "foo.com/auth"
+
+	loc := &ingress.Location{
+		ExternalAuth: authreq.Config{
+			URL:                  authURL,
+			KeepaliveConnections: 0,
+		},
+		Path: "/cat",
+	}
+
+	cfg := config.Configuration{
+		UseHTTP2: false,
+	}
+
+	testCases := []struct {
+		title                string
+		authURL              string
+		keepaliveConnections int
+		expected             bool
+	}{
+		{"authURL, no keepalive", authURL, 0, false},
+		{"authURL, keepalive", authURL, 10, true},
+		{"empty, no keepalive", "", 0, false},
+		{"empty, keepalive", "", 10, false},
+	}
+
+	for _, testCase := range testCases {
+		loc.ExternalAuth.URL = testCase.authURL
+		loc.ExternalAuth.KeepaliveConnections = testCase.keepaliveConnections
+
+		result := shouldApplyAuthUpstream(loc, cfg)
+		if result != testCase.expected {
+			t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, result)
+		}
+	}
+
+	// keepalive is not supported with UseHTTP2
+	cfg.UseHTTP2 = true
+	for _, testCase := range testCases {
+		loc.ExternalAuth.URL = testCase.authURL
+		loc.ExternalAuth.KeepaliveConnections = testCase.keepaliveConnections
+
+		result := shouldApplyAuthUpstream(loc, cfg)
+		if result != false {
+			t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, false, result)
+		}
+	}
+}
+
+func TestExtractHostPort(t *testing.T) {
+	testCases := []struct {
+		title    string
+		url      string
+		expected string
+	}{
+		{"full URL", "https://my.auth.site:5000/path", "my.auth.site:5000"},
+		{"URL with no port", "http://my.auth.site/path", "my.auth.site"},
+		{"URL with no path", "https://my.auth.site:5000", "my.auth.site:5000"},
+		{"URL no port and path", "http://my.auth.site", "my.auth.site"},
+		{"missing method", "my.auth.site/path", ""},
+		{"all empty", "", ""},
+	}
+
+	for _, testCase := range testCases {
+		result := extractHostPort(testCase.url)
+		if result != testCase.expected {
+			t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, result)
+		}
+	}
+}
+
+func TestChangeHostPort(t *testing.T) {
+	testCases := []struct {
+		title    string
+		url      string
+		host     string
+		expected string
+	}{
+		{"full URL", "https://my.auth.site:5000/path", "your.domain", "https://your.domain/path"},
+		{"URL with no port", "http://my.auth.site/path", "your.domain", "http://your.domain/path"},
+		{"URL with no path", "http://my.auth.site:5000", "your.domain:8888", "http://your.domain:8888"},
+		{"URL with no port and path", "https://my.auth.site", "your.domain:8888", "https://your.domain:8888"},
+		{"invalid host", "my.auth.site/path", "", ""},
+		{"missing method", "my.auth.site/path", "your.domain", ""},
+		{"all empty", "", "", ""},
+	}
+
+	for _, testCase := range testCases {
+		result := changeHostPort(testCase.url, testCase.host)
+		if result != testCase.expected {
+			t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, result)
+		}
+	}
+}
+
 func TestTemplateWithData(t *testing.T) {
 	pwd, _ := os.Getwd()
 	f, err := os.Open(path.Join(pwd, "../../../../test/data/config.json"))