Redirect for app-root should preserve current scheme (#5266)

internal/ingress/annotations/rewrite/main.go

@@ -17,9 +17,13 @@ limitations under the License.
 package rewrite
 
 import (
+	"net/url"
+
 	networking "k8s.io/api/networking/v1beta1"
+	"k8s.io/klog"
 
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
+	"k8s.io/ingress-nginx/internal/ingress/errors"
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
@@ -90,8 +94,29 @@ func (a rewrite) Parse(ing *networking.Ingress) (interface{}, error) {
 		config.ForceSSLRedirect = a.r.GetDefaultBackend().ForceSSLRedirect
 	}
 
-	config.AppRoot, _ = parser.GetStringAnnotation("app-root", ing)
 	config.UseRegex, _ = parser.GetBoolAnnotation("use-regex", ing)
 
+	config.AppRoot, err = parser.GetStringAnnotation("app-root", ing)
+	if err != nil {
+		if !errors.IsMissingAnnotations(err) && !errors.IsInvalidContent(err) {
+			klog.Warningf("Annotation app-root contains an invalid value: %v", err)
+		}
+
+		return config, nil
+	}
+
+	u, err := url.ParseRequestURI(config.AppRoot)
+	if err != nil {
+		klog.Warningf("Annotation app-root contains an invalid value: %v", err)
+		config.AppRoot = ""
+		return config, nil
+	}
+
+	if u.IsAbs() {
+		klog.Warningf("Annotation app-root only allows absolute paths (%v)", config.AppRoot)
+		config.AppRoot = ""
+		return config, nil
+	}
+
 	return config, nil
 }

internal/ingress/annotations/rewrite/main_test.go

@@ -41,8 +41,9 @@ func buildIngress() *networking.Ingress {
 
 	return &networking.Ingress{
 		ObjectMeta: meta_v1.ObjectMeta{
-			Name:      "foo",
-			Namespace: api.NamespaceDefault,
+			Name:        "foo",
+			Namespace:   api.NamespaceDefault,
+			Annotations: map[string]string{},
 		},
 		Spec: networking.IngressSpec{
 			Backend: &networking.IngressBackend{
@@ -163,19 +164,42 @@ func TestForceSSLRedirect(t *testing.T) {
 	}
 }
 func TestAppRoot(t *testing.T) {
-	ing := buildIngress()
+	ap := NewParser(mockBackend{redirect: true})
 
-	data := map[string]string{}
-	data[parser.GetAnnotationWithPrefix("app-root")] = "/app1"
-	ing.SetAnnotations(data)
-
-	i, _ := NewParser(mockBackend{redirect: true}).Parse(ing)
-	redirect, ok := i.(*Config)
-	if !ok {
-		t.Errorf("expected a App Context")
+	testCases := []struct {
+		title       string
+		path        string
+		expected    string
+		errExpected bool
+	}{
+		{"Empty path should return an error", "", "", true},
+		{"Relative paths are not allowed", "demo", "", true},
+		{"Path / should pass", "/", "/", false},
+		{"Path /demo should pass", "/demo", "/demo", false},
 	}
-	if redirect.AppRoot != "/app1" {
-		t.Errorf("Unexpected value got in AppRoot")
+
+	for _, testCase := range testCases {
+		t.Run(testCase.title, func(t *testing.T) {
+			ing := buildIngress()
+			ing.Annotations[parser.GetAnnotationWithPrefix("app-root")] = testCase.path
+			i, err := ap.Parse(ing)
+			if err != nil {
+				if testCase.errExpected {
+					return
+				}
+
+				t.Fatalf("%v: unexpected error obtaining running address/es: %v", testCase.title, err)
+			}
+
+			rewrite, ok := i.(*Config)
+			if !ok {
+				t.Fatalf("expected a rewrite Config")
+			}
+
+			if testCase.expected != rewrite.AppRoot {
+				t.Fatalf("%v: expected AppRoot with value %v but was returned: %v", testCase.title, testCase.expected, rewrite.AppRoot)
+			}
+		})
 	}
 }
 

rootfs/etc/nginx/template/nginx.tmpl

@@ -862,7 +862,7 @@ stream {
         proxy_ssl_verify_depth                  {{ $server.ProxySSL.VerifyDepth }};
         {{ if not (empty $server.ProxySSL.ProxySSLName) }}
         proxy_ssl_name                           {{ $server.ProxySSL.ProxySSLName }};
-        {{ end }}        
+        {{ end }}
         {{ end }}
 
         {{ if not (empty $server.ProxySSL.PemFileName) }}
@@ -896,9 +896,9 @@ stream {
         {{ $externalAuth = $all.Cfg.GlobalExternalAuth }}
         {{ end }}
 
-        {{ if not (empty $location.Rewrite.AppRoot)}}
+        {{ if not (empty $location.Rewrite.AppRoot) }}
         if ($uri = /) {
-            return 302 {{ $location.Rewrite.AppRoot }};
+            return 302 $scheme://$http_host{{ $location.Rewrite.AppRoot }};
         }
         {{ end }}
 

test/e2e/annotations/approot.go

@@ -45,7 +45,7 @@ var _ = framework.DescribeAnnotation("app-root", func() {
 		f.WaitForNginxServer(host,
 			func(server string) bool {
 				return strings.Contains(server, `if ($uri = /) {`) &&
-					strings.Contains(server, `return 302 /foo;`)
+					strings.Contains(server, `return 302 $scheme://$http_host/foo;`)
 			})
 
 		f.HTTPTestClient().