From 1d53984a8808eb35b49d44edf30a1ae48d131ddc Mon Sep 17 00:00:00 2001 From: "Tore S. Loenoey" Date: Thu, 23 Nov 2023 20:35:10 +0100 Subject: [PATCH] fix rework --- .../ingress/annotations/authreqglobal/main.go | 6 +- internal/ingress/controller/config/config.go | 4 +- .../ingress/controller/template/configmap.go | 2 +- .../controller/template/configmap_test.go | 30 ------- .../ingress/controller/template/template.go | 23 ++---- .../controller/template/template_test.go | 79 +++++++++++++------ internal/ingress/defaults/main.go | 5 ++ rootfs/etc/nginx/template/nginx.tmpl | 6 +- 8 files changed, 76 insertions(+), 79 deletions(-) diff --git a/internal/ingress/annotations/authreqglobal/main.go b/internal/ingress/annotations/authreqglobal/main.go index e8a259047..733f5637c 100644 --- a/internal/ingress/annotations/authreqglobal/main.go +++ b/internal/ingress/annotations/authreqglobal/main.go @@ -39,6 +39,10 @@ var globalAuthAnnotations = parser.Annotation{ }, } +type Config struct { + GlobalAuthDefaultEnable bool `json:"global-auth-default-enable,omitempty"` +} + type authReqGlobal struct { r resolver.Resolver annotationConfig parser.Annotation @@ -57,7 +61,7 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation { func (a authReqGlobal) Parse(ing *networking.Ingress) (interface{}, error) { enableGlobalAuth, err := parser.GetBoolAnnotation(enableGlobalAuthAnnotation, ing, a.annotationConfig.Annotations) if err != nil { - enableGlobalAuth = true + enableGlobalAuth = a.r.GetDefaultBackend().GlobalAuthDefaultEnable } return enableGlobalAuth, nil diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 8723265bc..9f0b7252c 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -762,7 +762,7 @@ func NewDefault() Configuration { defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1") defNginxStatusIpv6Whitelist = append(defNginxStatusIpv6Whitelist, "::1") defProxyDeadlineDuration := time.Duration(5) * time.Second - defGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", "", append(defResponseHeaders, ""), "", "", "", []string{}, map[string]string{}, false, true} + defGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", "", append(defResponseHeaders, ""), "", "", "", []string{}, map[string]string{}, false} cfg := Configuration{ AllowSnippetAnnotations: false, @@ -872,6 +872,7 @@ func NewDefault() Configuration { DisableProxyInterceptErrors: false, DenylistSourceRange: []string{}, WhitelistSourceRange: []string{}, + GlobalAuthDefaultEnable: true, SkipAccessLogURLs: []string{}, LimitRate: 0, LimitRateAfter: 0, @@ -976,5 +977,4 @@ type GlobalExternalAuth struct { AuthCacheDuration []string `json:"authCacheDuration"` ProxySetHeaders map[string]string `json:"proxySetHeaders,omitempty"` AlwaysSetCookie bool `json:"alwaysSetCookie,omitempty"` - DefaultEnable bool `json:"defaultEnable,omitempty"` } diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go index 493df7c21..d02024884 100644 --- a/internal/ingress/controller/template/configmap.go +++ b/internal/ingress/controller/template/configmap.go @@ -352,7 +352,7 @@ func ReadConfig(src map[string]string) config.Configuration { if err != nil { klog.Warningf("Global auth location denied - %s", fmt.Errorf("cannot convert %s to bool: %v", globalAuthDefaultEnable, err)) } - to.GlobalExternalAuth.DefaultEnable = authDefaultEnable + to.GlobalAuthDefaultEnable = authDefaultEnable } // Verify that the configured timeout is parsable as a duration. if not, set the default value diff --git a/internal/ingress/controller/template/configmap_test.go b/internal/ingress/controller/template/configmap_test.go index 39f811336..dad841694 100644 --- a/internal/ingress/controller/template/configmap_test.go +++ b/internal/ingress/controller/template/configmap_test.go @@ -263,36 +263,6 @@ func TestGlobalExternalAlwaysSetCookie(t *testing.T) { } } -func TestGlobalExternalDefaultEnable(t *testing.T) { - testCases := map[string]struct { - defaultEnable string - result bool - }{ - "true": { - defaultEnable: "true", - result: true, - }, - "false": { - defaultEnable: "false", - result: false, - }, - "set empty": { - defaultEnable: "", - result: true, - }, - "error": { - defaultEnable: "error string", - }, - } - - for n, tc := range testCases { - cfg := ReadConfig(map[string]string{"global-auth-default-enable": tc.defaultEnable}) - if cfg.GlobalExternalAuth.DefaultEnable != tc.result { - t.Errorf("Testing %v. Expected \"%v\" but \"%v\" was returned", n, tc.result, cfg.GlobalExternalAuth.DefaultEnable) - } - } -} - func TestGlobalExternalAuthSigninRedirectParamParsing(t *testing.T) { testCases := map[string]struct { param string diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 0309abe41..32eb0ddf7 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -541,14 +541,14 @@ func buildLocation(input interface{}, enforceRegex bool) string { return path } -func buildAuthLocation(input interface{}, globalExternalAuthURL string, c interface{}) string { +func buildAuthLocation(input interface{}, globalExternalAuthURL string) string { location, ok := input.(*ingress.Location) if !ok { klog.Errorf("expected an '*ingress.Location' type but %T was returned", input) return "" } - if (location.ExternalAuth.URL == "") && (!shouldApplyGlobalAuth(input, globalExternalAuthURL, c)) { + if (location.ExternalAuth.URL == "") && (!shouldApplyGlobalAuth(input, globalExternalAuthURL)) { return "" } @@ -566,19 +566,13 @@ func buildAuthLocation(input interface{}, globalExternalAuthURL string, c interf // shouldApplyGlobalAuth returns true only in case when ExternalAuth.URL is not set and // GlobalExternalAuth is set, or if GlobalExternalAuth.DefaultEnable is true. -func shouldApplyGlobalAuth(input interface{}, globalExternalAuthURL string, c interface{}) bool { +func shouldApplyGlobalAuth(input interface{}, globalExternalAuthURL string) bool { location, ok := input.(*ingress.Location) if !ok { klog.Errorf("expected an '*ingress.Location' type but %T was returned", input) } - 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 == "") && (globalExternalAuthURL != "") && ((cfg.GlobalExternalAuth.DefaultEnable) && (location.EnableGlobalAuth)) { + if (location.ExternalAuth.URL == "") && (globalExternalAuthURL != "") && (location.EnableGlobalAuth) { return true } @@ -639,14 +633,9 @@ func buildAuthProxySetHeaders(headers map[string]string) []string { return res } -func buildAuthUpstreamName(input interface{}, host string, c interface{}) string { - cfg, ok := c.(config.Configuration) - if !ok { - klog.Errorf("expected a 'config.Configuration' type but %T was returned", c) - return "" - } +func buildAuthUpstreamName(input interface{}, host string) string { - authPath := buildAuthLocation(input, "", cfg) + authPath := buildAuthLocation(input, "") if authPath == "" || host == "" { return "" } diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 3cd80d377..2e60e3203 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -408,10 +408,9 @@ func TestBuildProxyPassAutoHttp(t *testing.T) { } func TestBuildAuthLocation(t *testing.T) { - cfg := config.Configuration{} invalidType := &ingress.Ingress{} expected := "" - actual := buildAuthLocation(invalidType, "", cfg) + actual := buildAuthLocation(invalidType, "") if !reflect.DeepEqual(expected, actual) { t.Errorf("Expected '%v' but returned '%v'", expected, actual) @@ -461,7 +460,7 @@ func TestBuildAuthLocation(t *testing.T) { loc.ExternalAuth.URL = testCase.authURL loc.EnableGlobalAuth = testCase.enableglobalExternalAuth - str := buildAuthLocation(loc, testCase.globalAuthURL, cfg) + str := buildAuthLocation(loc, testCase.globalAuthURL) if str != testCase.expected { t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, str) } @@ -471,7 +470,6 @@ func TestBuildAuthLocation(t *testing.T) { func TestShouldApplyGlobalAuth(t *testing.T) { authURL := fooAuthHost globalAuthURL := "foo.com/global-auth" - cfg := config.Configuration{} loc := &ingress.Location{ ExternalAuth: authreq.Config{ @@ -486,38 +484,70 @@ func TestShouldApplyGlobalAuth(t *testing.T) { authURL string globalAuthURL string enableglobalExternalAuth bool - globalAuthDefaultEnable bool expected bool }{ - {"authURL, globalAuthURL and enabled", authURL, globalAuthURL, true, true, false}, - {"authURL, globalAuthURL and disabled", authURL, globalAuthURL, false, true, false}, - {"authURL, empty globalAuthURL and enabled", authURL, "", true, true, false}, - {"authURL, empty globalAuthURL and disabled", authURL, "", false, true, false}, - {"globalAuthURL and enabled", "", globalAuthURL, true, true, true}, - {"globalAuthURL and disabled", "", globalAuthURL, false, true, false}, - {"all empty and enabled", "", "", true, true, false}, - {"all empty and disabled", "", "", false, true, false}, - {"authURL, globalAuthURL and enabled, defaultEnable is false", authURL, globalAuthURL, true, false, false}, - {"authURL, globalAuthURL and disabled, defaultEnable is false", authURL, globalAuthURL, false, false, false}, - {"authURL, empty globalAuthURL and enabled, defaultEnable is false", authURL, "", true, false, false}, - {"authURL, empty globalAuthURL and disabled, defaultEnable is false", authURL, "", false, false, false}, - {"globalAuthURL and enabled, defaultEnable is false", "", globalAuthURL, true, true, true}, - {"globalAuthURL and disabled, defaultEnable is false", "", globalAuthURL, false, false, false}, - {"all empty and enabled, defaultEnable is false", "", "", true, false, false}, - {"all empty and disabled, defaultEnable is false", "", "", false, false, false}, + {"authURL, globalAuthURL and enabled", authURL, globalAuthURL, true, false}, + {"authURL, globalAuthURL and disabled", authURL, globalAuthURL, false, false}, + {"authURL, empty globalAuthURL and enabled", authURL, "", true, false}, + {"authURL, empty globalAuthURL and disabled", authURL, "", false, false}, + {"globalAuthURL and enabled", "", globalAuthURL, true, true}, + {"globalAuthURL and disabled", "", globalAuthURL, false, false}, + {"all empty and enabled", "", "", true, false}, + {"all empty and disabled", "", "", false, false}, } for _, testCase := range testCases { loc.ExternalAuth.URL = testCase.authURL loc.EnableGlobalAuth = testCase.enableglobalExternalAuth - result := shouldApplyGlobalAuth(loc, testCase.globalAuthURL, cfg) + result := shouldApplyGlobalAuth(loc, testCase.globalAuthURL) if result != testCase.expected { t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, result) } } } +// func TestShouldApplyGlobalAuthWhenEnableDefaultIsFalse(t *testing.T) { +// authURL := fooAuthHost +// globalAuthURL := "foo.com/global-auth" + +// loc := &ingress.Location{ +// ExternalAuth: authreq.Config{ +// URL: authURL, +// }, +// Path: "/cat", +// EnableGlobalAuth: true, +// } + +// testCases := []struct { +// title string +// authURL string +// globalAuthURL string +// enableglobalExternalAuth bool +// globalAuthDefaultEnable bool +// expected bool +// }{ +// {"authURL, globalAuthURL and enabled", authURL, globalAuthURL, true, true, false}, +// {"authURL, globalAuthURL and disabled", authURL, globalAuthURL, false, true, false}, +// {"authURL, empty globalAuthURL and enabled", authURL, "", true, true, false}, +// {"authURL, empty globalAuthURL and disabled", authURL, "", false, true, false}, +// {"globalAuthURL and enabled", "", globalAuthURL, true, true, true}, +// {"globalAuthURL and disabled", "", globalAuthURL, false, true, false}, +// {"all empty and enabled", "", "", true, true, false}, +// {"all empty and disabled", "", "", false, true, false}, +// } + +// for _, testCase := range testCases { +// loc.ExternalAuth.URL = testCase.authURL +// loc.EnableGlobalAuth = testCase.enableglobalExternalAuth + +// result := shouldApplyGlobalAuth(loc, testCase.globalAuthURL) +// if result != testCase.expected { +// t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, result) +// } +// } +// } + func TestBuildAuthResponseHeaders(t *testing.T) { externalAuthResponseHeaders := []string{"h1", "H-With-Caps-And-Dashes"} tests := []struct { @@ -589,8 +619,7 @@ func TestBuildAuthProxySetHeaders(t *testing.T) { func TestBuildAuthUpstreamName(t *testing.T) { invalidType := &ingress.Ingress{} expected := "" - cfg := config.Configuration{} - actual := buildAuthUpstreamName(invalidType, "", cfg) + actual := buildAuthUpstreamName(invalidType, "") if !reflect.DeepEqual(expected, actual) { t.Errorf("Expected '%v' but returned '%v'", expected, actual) @@ -617,7 +646,7 @@ func TestBuildAuthUpstreamName(t *testing.T) { } for _, testCase := range testCases { - str := buildAuthUpstreamName(loc, testCase.host, cfg) + str := buildAuthUpstreamName(loc, testCase.host) if str != testCase.expected { t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expected, str) } diff --git a/internal/ingress/defaults/main.go b/internal/ingress/defaults/main.go index 2bb58c858..dba4927d9 100644 --- a/internal/ingress/defaults/main.go +++ b/internal/ingress/defaults/main.go @@ -176,6 +176,11 @@ type Backend struct { // By default, the NGINX ingress controller uses a list of all endpoints (Pod IP/port) in the NGINX upstream configuration. // It disables that behavior and instead uses a single upstream in NGINX, the service's Cluster IP and port. ServiceUpstream bool `json:"service-upstream"` + + // By default, the NGINX ingress controller applies global-auth configuration to all Ingress resources, + // if global-auth-url (ConfigMap) is set, and auth-url is not set (Ingess). Default is `true`. By setting this to + // `false`, global-auth is only applied to Ingress resources when global-auth-url (ConfigMap) is set and enable-global-auth is set (Ingress). + GlobalAuthDefaultEnable bool `json:"global-auth-default-enable"` } type SecurityConfiguration struct { diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index ac06af242..94dc12412 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -636,7 +636,7 @@ http { {{ range $server := $servers }} {{ range $location := $server.Locations }} - {{ $applyGlobalAuth := shouldApplyGlobalAuth $location $all.Cfg.GlobalExternalAuth.URL $all.Cfg.GlobalExternalAuth.DefaultEnable }} + {{ $applyGlobalAuth := shouldApplyGlobalAuth $location $all.Cfg.GlobalExternalAuth.URL }} {{ $applyAuthUpstream := shouldApplyAuthUpstream $location $all.Cfg }} {{ if and (eq $applyAuthUpstream true) (eq $applyGlobalAuth false) }} ## start auth upstream {{ $server.Hostname }}{{ $location.Path }} @@ -1057,8 +1057,8 @@ stream { {{ range $location := $server.Locations }} {{ $path := buildLocation $location $enforceRegex }} {{ $proxySetHeader := proxySetHeader $location }} - {{ $authPath := buildAuthLocation $location $all.Cfg.GlobalExternalAuth.URL $all.Cfg.GlobalExternalAuth.DefaultEnable }} - {{ $applyGlobalAuth := shouldApplyGlobalAuth $location $all.Cfg.GlobalExternalAuth.URL $all.Cfg.GlobalExternalAuth.DefaultEnable }} + {{ $authPath := buildAuthLocation $location $all.Cfg.GlobalExternalAuth.URL }} + {{ $applyGlobalAuth := shouldApplyGlobalAuth $location $all.Cfg.GlobalExternalAuth.URL }} {{ $applyAuthUpstream := shouldApplyAuthUpstream $location $all.Cfg }} {{ $externalAuth := $location.ExternalAuth }}