Merge pull request #5015 from aledbf/mirror

Refactor mirror feature

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,

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"
+nginx.ingress.kubernetes.io/mirror-target: https://test.env.com/$request_uri
-```
-
-The mirror path can be defined as a separate ingress resource:
-
-```
-location = /mirror {
-    internal;
-    proxy_pass http://test_backend;
-}
 ```
 
 By default the request-body is sent to the mirror backend, but can be turned off by applying:

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)
+	authURL, err := parser.StringToURL(urlString)
-	if authURL == nil {
+	if err != nil {
-		return nil, ing_errors.NewLocationDenied(message)
+		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)

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)

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{}
+	config := &Config{
-	var err error
+		Source: fmt.Sprintf("/_mirror-%v", ing.UID),
-
-	config.URI, err = parser.GetStringAnnotation("mirror-uri", ing)
-	if err != nil {
-		config.URI = ""
 	}
 
+	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
 }

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{
+		{map[string]string{backendURL: "https://test.env.com/$request_uri"}, &Config{
-			URI:         "/mirror",
+			Source:      ngxURI,
 			RequestBody: "on",
+			Target:      "https://test.env.com/$request_uri",
 		}},
-		{map[string]string{uri: "/mirror", requestBody: "off"}, &Config{
+		{map[string]string{requestBody: "off"}, &Config{
-			URI:         "/mirror",
+			Source:      ngxURI,
 			RequestBody: "off",
-		}},
+			Target:      "",
-		{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",
 		}},
 	}
 
@@ -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)
 		}

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
+}

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/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/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)
+		}
+	}
+}

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 {

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()
+}

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
 }
 

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 }}
+            {{ if $location.Mirror.Source }}
-            mirror {{ $location.Mirror.URI }};
+            mirror {{ $location.Mirror.Source }};
             mirror_request_body {{ $location.Mirror.RequestBody }};
             {{ end }}
 

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;")
 			})
 	})
 })

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{