diff --git a/cmd/plugin/lints/ingress.go b/cmd/plugin/lints/ingress.go index 7d7b36f9c..0de4661f4 100644 --- a/cmd/plugin/lints/ingress.go +++ b/cmd/plugin/lints/ingress.go @@ -66,6 +66,7 @@ func GetIngressLints() []IngressLint { removedAnnotation("add-base-url", 3174, "0.22.0"), removedAnnotation("base-url-scheme", 3174, "0.22.0"), removedAnnotation("session-cookie-hash", 3743, "0.24.0"), + removedAnnotation("mirror-uri", 5015, "0.28.1"), { message: "The rewrite-target annotation value does not reference a capture group", issue: 3174, diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index 3bc2c45b3..109a5cc3f 100755 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -118,8 +118,8 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/enable-owasp-core-rules](#modsecurity)|bool| |[nginx.ingress.kubernetes.io/modsecurity-transaction-id](#modsecurity)|string| |[nginx.ingress.kubernetes.io/modsecurity-snippet](#modsecurity)|string| -|[nginx.ingress.kubernetes.io/mirror-uri](#mirror)|string| |[nginx.ingress.kubernetes.io/mirror-request-body](#mirror)|string| +|[nginx.ingress.kubernetes.io/mirror-target](#mirror)|string| ### Canary @@ -869,19 +869,10 @@ nginx.ingress.kubernetes.io/satisfy: "any" Enables a request to be mirrored to a mirror backend. Responses by mirror backends are ignored. This feature is useful, to see how requests will react in "test" backends. -You can mirror a request to the `/mirror` path on your ingress, by applying the below: +The mirror backend can be set by applying: ```yaml -nginx.ingress.kubernetes.io/mirror-uri: "/mirror" -``` - -The mirror path can be defined as a separate ingress resource: - -``` -location = /mirror { - internal; - proxy_pass http://test_backend; -} +nginx.ingress.kubernetes.io/mirror-target: https://test.env.com/$request_uri ``` By default the request-body is sent to the mirror backend, but can be turned off by applying: diff --git a/internal/ingress/annotations/authreq/main.go b/internal/ingress/annotations/authreq/main.go index 0e76fcc46..12d232b27 100644 --- a/internal/ingress/annotations/authreq/main.go +++ b/internal/ingress/annotations/authreq/main.go @@ -18,7 +18,6 @@ package authreq import ( "fmt" - "net/url" "regexp" "strings" @@ -159,9 +158,9 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) { return nil, err } - authURL, message := ParseStringToURL(urlString) - if authURL == nil { - return nil, ing_errors.NewLocationDenied(message) + authURL, err := parser.StringToURL(urlString) + if err != nil { + return nil, ing_errors.InvalidContent{Name: err.Error()} } authMethod, _ := parser.GetStringAnnotation("auth-method", ing) @@ -244,25 +243,6 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) { }, nil } -// ParseStringToURL parses the provided string into URL and returns error -// message in case of failure -func ParseStringToURL(input string) (*url.URL, string) { - - parsedURL, err := url.Parse(input) - if err != nil { - return nil, fmt.Sprintf("%v is not a valid URL: %v", input, err) - } - if parsedURL.Scheme == "" { - return nil, "url scheme is empty." - } else if parsedURL.Host == "" { - return nil, "url host is empty." - } else if strings.Contains(parsedURL.Host, "..") { - return nil, "invalid url host." - } - return parsedURL, "" - -} - // ParseStringToCacheDurations parses and validates the provided string // into a list of cache durations. // It will always return at least one duration (the default duration) diff --git a/internal/ingress/annotations/authreq/main_test.go b/internal/ingress/annotations/authreq/main_test.go index 4f0e7c640..c57344e19 100644 --- a/internal/ingress/annotations/authreq/main_test.go +++ b/internal/ingress/annotations/authreq/main_test.go @@ -18,7 +18,6 @@ package authreq import ( "fmt" - "net/url" "reflect" "testing" @@ -173,7 +172,6 @@ func TestHeaderAnnotations(t *testing.T) { continue } - t.Log(i) u, ok := i.(*Config) if !ok { t.Errorf("%v: expected an External type", test.title) @@ -221,7 +219,6 @@ func TestCacheDurationAnnotations(t *testing.T) { continue } - t.Log(i) u, ok := i.(*Config) if !ok { t.Errorf("%v: expected an External type", test.title) @@ -234,41 +231,6 @@ func TestCacheDurationAnnotations(t *testing.T) { } } -func TestParseStringToURL(t *testing.T) { - validURL := "http://bar.foo.com/external-auth" - validParsedURL, _ := url.Parse(validURL) - - tests := []struct { - title string - url string - message string - parsed *url.URL - expErr bool - }{ - {"empty", "", "url scheme is empty.", nil, true}, - {"no scheme", "bar", "url scheme is empty.", nil, true}, - {"invalid host", "http://", "url host is empty.", nil, true}, - {"invalid host (multiple dots)", "http://foo..bar.com", "invalid url host.", nil, true}, - {"valid URL", validURL, "", validParsedURL, false}, - } - - for _, test := range tests { - - i, err := ParseStringToURL(test.url) - if test.expErr { - if err != test.message { - t.Errorf("%v: expected error \"%v\" but \"%v\" was returned", test.title, test.message, err) - } - continue - } - - if i.String() != test.parsed.String() { - t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.parsed, i) - } - } - -} - func TestParseStringToCacheDurations(t *testing.T) { tests := []struct { @@ -331,7 +293,6 @@ func TestProxySetHeaders(t *testing.T) { configMapResolver.ConfigMaps["proxy-headers-map"] = &api.ConfigMap{Data: test.headers} } - t.Log(configMapResolver) i, err := NewParser(configMapResolver).Parse(ing) if test.expErr { if err == nil { @@ -340,7 +301,6 @@ func TestProxySetHeaders(t *testing.T) { continue } - t.Log(i) u, ok := i.(*Config) if !ok { t.Errorf("%v: expected an External type", test.title) diff --git a/internal/ingress/annotations/mirror/main.go b/internal/ingress/annotations/mirror/main.go index 67a4367d9..dcd6244be 100644 --- a/internal/ingress/annotations/mirror/main.go +++ b/internal/ingress/annotations/mirror/main.go @@ -17,6 +17,8 @@ limitations under the License. package mirror import ( + "fmt" + networking "k8s.io/api/networking/v1beta1" "k8s.io/ingress-nginx/internal/ingress/annotations/parser" @@ -25,8 +27,34 @@ import ( // Config returns the mirror to use in a given location type Config struct { - URI string `json:"uri"` + Source string `json:"source"` RequestBody string `json:"requestBody"` + Target string `json:"target"` +} + +// Equal tests for equality between two Configuration types +func (m1 *Config) Equal(m2 *Config) bool { + if m1 == m2 { + return true + } + + if m1 == nil || m2 == nil { + return false + } + + if m1.Source != m2.Source { + return false + } + + if m1.RequestBody != m2.RequestBody { + return false + } + + if m1.Target != m2.Target { + return false + } + + return true } type mirror struct { @@ -41,18 +69,20 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation { // ParseAnnotations parses the annotations contained in the ingress // rule used to configure mirror func (a mirror) Parse(ing *networking.Ingress) (interface{}, error) { - config := &Config{} - var err error - - config.URI, err = parser.GetStringAnnotation("mirror-uri", ing) - if err != nil { - config.URI = "" + config := &Config{ + Source: fmt.Sprintf("/_mirror-%v", ing.UID), } + var err error config.RequestBody, err = parser.GetStringAnnotation("mirror-request-body", ing) if err != nil || config.RequestBody != "off" { config.RequestBody = "on" } + config.Target, err = parser.GetStringAnnotation("mirror-target", ing) + if err != nil { + config.Target = "" + } + return config, nil } diff --git a/internal/ingress/annotations/mirror/main_test.go b/internal/ingress/annotations/mirror/main_test.go index e9bae8786..3712a0a11 100644 --- a/internal/ingress/annotations/mirror/main_test.go +++ b/internal/ingress/annotations/mirror/main_test.go @@ -17,7 +17,6 @@ limitations under the License. package mirror import ( - "fmt" "reflect" "testing" @@ -29,41 +28,28 @@ import ( ) func TestParse(t *testing.T) { - uri := parser.GetAnnotationWithPrefix("mirror-uri") requestBody := parser.GetAnnotationWithPrefix("mirror-request-body") + backendURL := parser.GetAnnotationWithPrefix("mirror-target") ap := NewParser(&resolver.Mock{}) if ap == nil { t.Fatalf("expected a parser.IngressAnnotation but returned nil") } + ngxURI := "/_mirror-c89a5111-b2e9-4af8-be19-c2a4a924c256" testCases := []struct { annotations map[string]string expected *Config }{ - {map[string]string{uri: "/mirror", requestBody: ""}, &Config{ - URI: "/mirror", + {map[string]string{backendURL: "https://test.env.com/$request_uri"}, &Config{ + Source: ngxURI, RequestBody: "on", + Target: "https://test.env.com/$request_uri", }}, - {map[string]string{uri: "/mirror", requestBody: "off"}, &Config{ - URI: "/mirror", + {map[string]string{requestBody: "off"}, &Config{ + Source: ngxURI, RequestBody: "off", - }}, - {map[string]string{uri: "", requestBody: "ahh"}, &Config{ - URI: "", - RequestBody: "on", - }}, - {map[string]string{uri: "", requestBody: ""}, &Config{ - URI: "", - RequestBody: "on", - }}, - {map[string]string{}, &Config{ - URI: "", - RequestBody: "on", - }}, - {nil, &Config{ - URI: "", - RequestBody: "on", + Target: "", }}, } @@ -71,6 +57,7 @@ func TestParse(t *testing.T) { ObjectMeta: meta_v1.ObjectMeta{ Name: "foo", Namespace: api.NamespaceDefault, + UID: "c89a5111-b2e9-4af8-be19-c2a4a924c256", }, Spec: networking.IngressSpec{}, } @@ -78,7 +65,6 @@ func TestParse(t *testing.T) { for _, testCase := range testCases { ing.SetAnnotations(testCase.annotations) result, _ := ap.Parse(ing) - fmt.Printf("%t", result) if !reflect.DeepEqual(result, testCase.expected) { t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations) } diff --git a/internal/ingress/annotations/parser/main.go b/internal/ingress/annotations/parser/main.go index 844e39ba4..6fd98dcf5 100644 --- a/internal/ingress/annotations/parser/main.go +++ b/internal/ingress/annotations/parser/main.go @@ -18,6 +18,7 @@ package parser import ( "fmt" + "net/url" "strconv" "strings" @@ -152,3 +153,22 @@ func AnnotationsReferencesConfigmap(ing *networking.Ingress) bool { return false } + +// StringToURL parses the provided string into URL and returns error +// message in case of failure +func StringToURL(input string) (*url.URL, error) { + parsedURL, err := url.Parse(input) + if err != nil { + return nil, fmt.Errorf("%v is not a valid URL: %v", input, err) + } + + if parsedURL.Scheme == "" { + return nil, fmt.Errorf("url scheme is empty") + } else if parsedURL.Host == "" { + return nil, fmt.Errorf("url host is empty") + } else if strings.Contains(parsedURL.Host, "..") { + return nil, fmt.Errorf("invalid url host") + } + + return parsedURL, nil +} diff --git a/internal/ingress/annotations/parser/main_test.go b/internal/ingress/annotations/parser/main_test.go index a89f3b16a..218565183 100644 --- a/internal/ingress/annotations/parser/main_test.go +++ b/internal/ingress/annotations/parser/main_test.go @@ -17,6 +17,7 @@ limitations under the License. package parser import ( + "net/url" "testing" api "k8s.io/api/core/v1" @@ -93,8 +94,8 @@ func TestGetStringAnnotation(t *testing.T) { {"valid - B", "string", " B", "B", false}, {"empty", "string", " ", "", true}, {"valid multiline", "string", ` - rewrite (?i)/arcgis/rest/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/rest/services/Utilities/Geometry/GeometryServer$1 break; - rewrite (?i)/arcgis/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/services/Utilities/Geometry/GeometryServer$1 break; + rewrite (?i)/arcgis/rest/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/rest/services/Utilities/Geometry/GeometryServer$1 break; + rewrite (?i)/arcgis/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/services/Utilities/Geometry/GeometryServer$1 break; `, ` rewrite (?i)/arcgis/rest/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/rest/services/Utilities/Geometry/GeometryServer$1 break; rewrite (?i)/arcgis/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/services/Utilities/Geometry/GeometryServer$1 break; @@ -162,3 +163,40 @@ func TestGetIntAnnotation(t *testing.T) { delete(data, test.field) } } + +func TestStringToURL(t *testing.T) { + validURL := "http://bar.foo.com/external-auth" + validParsedURL, _ := url.Parse(validURL) + + tests := []struct { + title string + url string + message string + parsed *url.URL + expErr bool + }{ + {"empty", "", "url scheme is empty", nil, true}, + {"no scheme", "bar", "url scheme is empty", nil, true}, + {"invalid host", "http://", "url host is empty", nil, true}, + {"invalid host (multiple dots)", "http://foo..bar.com", "invalid url host", nil, true}, + {"valid URL", validURL, "", validParsedURL, false}, + } + + for _, test := range tests { + i, err := StringToURL(test.url) + if test.expErr { + if err == nil { + t.Fatalf("%v: expected error but none returned", test.title) + } + + if err.Error() != test.message { + t.Errorf("%v: expected error \"%v\" but \"%v\" was returned", test.title, test.message, err) + } + continue + } + + if i.String() != test.parsed.String() { + t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.title, test.parsed, i) + } + } +} diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go index 862754a55..25a0f75d7 100644 --- a/internal/ingress/controller/template/configmap.go +++ b/internal/ingress/controller/template/configmap.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/ingress-nginx/internal/ingress/annotations/authreq" + "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/internal/ingress/controller/config" ing_net "k8s.io/ingress-nginx/internal/net" "k8s.io/ingress-nginx/internal/runtime" @@ -211,7 +212,7 @@ func ReadConfig(src map[string]string) config.Configuration { if val, ok := conf[globalAuthURL]; ok { delete(conf, globalAuthURL) - authURL, message := authreq.ParseStringToURL(val) + authURL, message := parser.StringToURL(val) if authURL == nil { klog.Warningf("Global auth location denied - %v.", message) } else { @@ -235,7 +236,7 @@ func ReadConfig(src map[string]string) config.Configuration { if val, ok := conf[globalAuthSignin]; ok { delete(conf, globalAuthSignin) - signinURL, _ := authreq.ParseStringToURL(val) + signinURL, _ := parser.StringToURL(val) if signinURL == nil { klog.Warningf("Global auth location denied - %v.", "global-auth-signin setting is undefined and will not be set") } else { diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index f4683a8b0..3ffe91cb1 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -180,6 +180,7 @@ var ( "buildOpentracingForLocation": buildOpentracingForLocation, "shouldLoadOpentracingModule": shouldLoadOpentracingModule, "buildModSecurityForLocation": buildModSecurityForLocation, + "buildMirrorLocations": buildMirrorLocations, } ) @@ -1377,3 +1378,29 @@ modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; return buffer.String() } + +func buildMirrorLocations(locs []*ingress.Location) string { + var buffer bytes.Buffer + + mapped := sets.String{} + + for _, loc := range locs { + if loc.Mirror.Source == "" || loc.Mirror.Target == "" { + continue + } + + if mapped.Has(loc.Mirror.Source) { + continue + } + + mapped.Insert(loc.Mirror.Source) + buffer.WriteString(fmt.Sprintf(`location = %v { +internal; +proxy_pass %v; +} + +`, loc.Mirror.Source, loc.Mirror.Target)) + } + + return buffer.String() +} diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go index f9f119030..c7fd8f2a3 100644 --- a/internal/ingress/types_equals.go +++ b/internal/ingress/types_equals.go @@ -440,18 +440,14 @@ func (l1 *Location) Equal(l2 *Location) bool { return false } - if l1.Mirror.URI != l2.Mirror.URI { - return false - } - - if l1.Mirror.RequestBody != l2.Mirror.RequestBody { - return false - } - if !l1.Opentracing.Equal(&l2.Opentracing) { return false } + if !l1.Mirror.Equal(&l2.Mirror) { + return false + } + return true } diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 357ac5425..ece6f658d 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -798,7 +798,7 @@ stream { {{ if not (empty $server.CertificateAuth.CRLFileName) }} # PEM sha: {{ $server.CertificateAuth.CRLSHA }} - ssl_crl {{ $server.CertificateAuth.CRLFileName }}; + ssl_crl {{ $server.CertificateAuth.CRLFileName }}; {{ end }} {{ if not (empty $server.CertificateAuth.ErrorPage)}} @@ -832,6 +832,7 @@ stream { {{ template "CUSTOM_ERRORS" (buildCustomErrorDeps $errorLocation.UpstreamName $errorLocation.Codes $all.EnableMetrics) }} {{ end }} + {{ buildMirrorLocations $server.Locations }} {{ $enforceRegex := enforceRegexModifier $server.Locations }} {{ range $location := $server.Locations }} @@ -965,8 +966,8 @@ stream { {{ buildOpentracingForLocation $all.Cfg.EnableOpentracing $location }} - {{ if $location.Mirror.URI }} - mirror {{ $location.Mirror.URI }}; + {{ if $location.Mirror.Source }} + mirror {{ $location.Mirror.Source }}; mirror_request_body {{ $location.Mirror.RequestBody }}; {{ end }} diff --git a/test/e2e/annotations/mirror.go b/test/e2e/annotations/mirror.go index f38731d6b..9ada90974 100644 --- a/test/e2e/annotations/mirror.go +++ b/test/e2e/annotations/mirror.go @@ -17,6 +17,7 @@ limitations under the License. package annotations import ( + "fmt" "strings" . "github.com/onsi/ginkgo" @@ -35,32 +36,50 @@ var _ = framework.IngressNginxDescribe("Annotations - Mirror", func() { AfterEach(func() { }) - It("should set mirror-uri to /mirror", func() { + It("should set mirror-target to http://localhost/mirror", func() { annotations := map[string]string{ - "nginx.ingress.kubernetes.io/mirror-uri": "/mirror", + "nginx.ingress.kubernetes.io/mirror-target": "http://localhost/mirror", } ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) - f.EnsureIngress(ing) + ing = f.EnsureIngress(ing) f.WaitForNginxServer(host, func(server string) bool { - return strings.Contains(server, "mirror /mirror;") && strings.Contains(server, "mirror_request_body on;") + return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) && + strings.Contains(server, "mirror_request_body on;") + }) + }) + + It("should set mirror-target to https://test.env.com/$request_uri", func() { + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/mirror-target": "https://test.env.com/$request_uri", + } + + ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) + ing = f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) && + strings.Contains(server, "mirror_request_body on;") && + strings.Contains(server, "proxy_pass https://test.env.com/$request_uri;") }) }) It("should disable mirror-request-body", func() { annotations := map[string]string{ - "nginx.ingress.kubernetes.io/mirror-uri": "/mirror", + "nginx.ingress.kubernetes.io/mirror-uri": "http://localhost/mirror", "nginx.ingress.kubernetes.io/mirror-request-body": "off", } ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) - f.EnsureIngress(ing) + ing = f.EnsureIngress(ing) f.WaitForNginxServer(host, func(server string) bool { - return strings.Contains(server, "mirror /mirror;") && strings.Contains(server, "mirror_request_body off;") + return strings.Contains(server, fmt.Sprintf("mirror /_mirror-%v;", ing.UID)) && + strings.Contains(server, "mirror_request_body off;") }) }) }) diff --git a/test/e2e/framework/deployment.go b/test/e2e/framework/deployment.go index 97a923ed9..53b9c64f1 100644 --- a/test/e2e/framework/deployment.go +++ b/test/e2e/framework/deployment.go @@ -306,7 +306,7 @@ func newDeployment(name, namespace, image string, port int32, replicas int32, co volumeMounts []corev1.VolumeMount, volumes []corev1.Volume) *appsv1.Deployment { probe := &corev1.Probe{ InitialDelaySeconds: 1, - PeriodSeconds: 5, + PeriodSeconds: 10, SuccessThreshold: 1, TimeoutSeconds: 1, Handler: corev1.Handler{