Improve path rule (#8623)

* Improve path rule

* Add nginx configuration tests

* Revert framework changes

* Add test to patched directives

* Fix root conf test

* Add comment in new function

internal/ingress/inspector/rules.go

@@ -22,8 +22,8 @@ import (
 )
 
 var (
-	invalidAliasDirective = regexp.MustCompile(`\s*alias\s*.*;`)
+	invalidAliasDirective = regexp.MustCompile(`(?s)\s*alias\s*.*;`)
-	invalidRootDirective  = regexp.MustCompile(`\s*root\s*.*;`)
+	invalidRootDirective  = regexp.MustCompile(`(?s)\s*root\s*.*;`)
 	invalidEtcDir         = regexp.MustCompile(`/etc/(passwd|shadow|group|nginx|ingress-controller)`)
 	invalidSecretsDir     = regexp.MustCompile(`/var/run/secrets`)
 	invalidByLuaDirective = regexp.MustCompile(`.*_by_lua.*`)

internal/ingress/inspector/rules_test.go

@@ -35,6 +35,11 @@ func TestCheckRegex(t *testing.T) {
 			wantErr: true,
 			value:   "   alias    blabla/lala ;",
 		},
+		{
+			name:    "must refuse invalid alias with line break",
+			wantErr: true,
+			value:   "alias #\n/lalala/1/;",
+		},
 		{
 			name:    "must refuse invalid attempt to call /etc",
 			wantErr: true,

test/e2e/e2e.go

@@ -40,6 +40,7 @@ import (
 	_ "k8s.io/ingress-nginx/test/e2e/leaks"
 	_ "k8s.io/ingress-nginx/test/e2e/loadbalance"
 	_ "k8s.io/ingress-nginx/test/e2e/lua"
+	_ "k8s.io/ingress-nginx/test/e2e/nginx"
 	_ "k8s.io/ingress-nginx/test/e2e/security"
 	_ "k8s.io/ingress-nginx/test/e2e/servicebackend"
 	_ "k8s.io/ingress-nginx/test/e2e/settings"

test/e2e/framework/deployment.go

@@ -150,8 +150,9 @@ http {
 	f.NGINXWithConfigDeployment(SlowEchoService, cfg)
 }
 
-// NGINXWithConfigDeployment creates an NGINX deployment using a configmap containing the nginx.conf configuration
+// NGINXDeployment creates a new simple NGINX Deployment using NGINX base image
-func (f *Framework) NGINXWithConfigDeployment(name string, cfg string) {
+// and passing the desired configuration
+func (f *Framework) NGINXDeployment(name string, cfg string, waitendpoint bool) {
 	cfgMap := map[string]string{
 		"nginx.conf": cfg,
 	}
@@ -213,8 +214,15 @@ func (f *Framework) NGINXWithConfigDeployment(name string, cfg string) {
 
 	f.EnsureService(service)
 
-	err = WaitForEndpoints(f.KubeClientSet, DefaultTimeout, name, f.Namespace, 1)
+	if waitendpoint {
-	assert.Nil(ginkgo.GinkgoT(), err, "waiting for endpoints to become ready")
+		err = WaitForEndpoints(f.KubeClientSet, DefaultTimeout, name, f.Namespace, 1)
+		assert.Nil(ginkgo.GinkgoT(), err, "waiting for endpoints to become ready")
+	}
+}
+
+// NGINXWithConfigDeployment creates an NGINX deployment using a configmap containing the nginx.conf configuration
+func (f *Framework) NGINXWithConfigDeployment(name string, cfg string) {
+	f.NGINXDeployment(name, cfg, true)
 }
 
 // NewGRPCBinDeployment creates a new deployment of the

test/e2e/framework/framework.go

@@ -88,8 +88,22 @@ func NewDefaultFramework(baseName string) *Framework {
 	return f
 }
 
-// BeforeEach gets a client and makes a namespace.
+// NewSimpleFramework makes a new framework that allows the usage of a namespace
-func (f *Framework) BeforeEach() {
+// for arbitraty tests.
+func NewSimpleFramework(baseName string) *Framework {
+	defer ginkgo.GinkgoRecover()
+
+	f := &Framework{
+		BaseName: baseName,
+	}
+
+	ginkgo.BeforeEach(f.CreateEnvironment)
+	ginkgo.AfterEach(f.DestroyEnvironment)
+
+	return f
+}
+
+func (f *Framework) CreateEnvironment() {
 	var err error
 
 	if f.KubeClientSet == nil {
@@ -107,6 +121,21 @@ func (f *Framework) BeforeEach() {
 
 	f.Namespace, err = CreateKubeNamespace(f.BaseName, f.KubeClientSet)
 	assert.Nil(ginkgo.GinkgoT(), err, "creating namespace")
+}
+
+func (f *Framework) DestroyEnvironment() {
+	go func() {
+		defer ginkgo.GinkgoRecover()
+		err := DeleteKubeNamespace(f.KubeClientSet, f.Namespace)
+		assert.Nil(ginkgo.GinkgoT(), err, "deleting namespace %v", f.Namespace)
+	}()
+}
+
+// BeforeEach gets a client and makes a namespace.
+func (f *Framework) BeforeEach() {
+	var err error
+
+	f.CreateEnvironment()
 
 	f.IngressClass, err = CreateIngressClass(f.Namespace, f.KubeClientSet)
 	assert.Nil(ginkgo.GinkgoT(), err, "creating IngressClass")
@@ -122,13 +151,7 @@ func (f *Framework) BeforeEach() {
 
 // AfterEach deletes the namespace, after reading its events.
 func (f *Framework) AfterEach() {
-	defer func(kubeClient kubernetes.Interface, ns string) {
+	defer f.DestroyEnvironment()
-		go func() {
-			defer ginkgo.GinkgoRecover()
-			err := DeleteKubeNamespace(kubeClient, ns)
-			assert.Nil(ginkgo.GinkgoT(), err, "deleting namespace %v", f.Namespace)
-		}()
-	}(f.KubeClientSet, f.Namespace)
 
 	defer func(kubeClient kubernetes.Interface, ingressclass string) {
 		go func() {
@@ -425,26 +448,35 @@ func (f *Framework) DeleteNGINXPod(grace int64) {
 	assert.Nil(ginkgo.GinkgoT(), err, "while waiting for ingress nginx pod to come up again")
 }
 
+// HTTPDumbTestClient returns a new httpexpect client without BaseURL.
+func (f *Framework) HTTPDumbTestClient() *httpexpect.Expect {
+	return f.newTestClient(nil, false)
+}
+
 // HTTPTestClient returns a new httpexpect client for end-to-end HTTP testing.
 func (f *Framework) HTTPTestClient() *httpexpect.Expect {
-	return f.newTestClient(nil)
+	return f.newTestClient(nil, true)
 }
 
 // HTTPTestClientWithTLSConfig returns a new httpexpect client for end-to-end
 // HTTP testing with a custom TLS configuration.
 func (f *Framework) HTTPTestClientWithTLSConfig(config *tls.Config) *httpexpect.Expect {
-	return f.newTestClient(config)
+	return f.newTestClient(config, true)
 }
 
-func (f *Framework) newTestClient(config *tls.Config) *httpexpect.Expect {
+func (f *Framework) newTestClient(config *tls.Config, setIngressURL bool) *httpexpect.Expect {
 	if config == nil {
 		config = &tls.Config{
 			InsecureSkipVerify: true,
 		}
 	}
+	var baseURL string
+	if setIngressURL {
+		baseURL = f.GetURL(HTTP)
+	}
 
 	return httpexpect.WithConfig(httpexpect.Config{
-		BaseURL: f.GetURL(HTTP),
+		BaseURL: baseURL,
 		Client: &http.Client{
 			Transport: &http.Transport{
 				TLSClientConfig: config,
@@ -485,6 +517,19 @@ func (f *Framework) WaitForNginxListening(port int) {
 	assert.Nil(ginkgo.GinkgoT(), err, "waiting for ingress controller pod listening on port 80")
 }
 
+// WaitForPod waits for a specific Pod to be ready, using a label selector
+func (f *Framework) WaitForPod(selector string, timeout time.Duration, shouldFail bool) {
+	err := waitForPodsReady(f.KubeClientSet, timeout, 1, f.Namespace, metav1.ListOptions{
+		LabelSelector: selector,
+	})
+
+	if shouldFail {
+		assert.NotNil(ginkgo.GinkgoT(), err, "waiting for pods to be ready")
+	} else {
+		assert.Nil(ginkgo.GinkgoT(), err, "waiting for pods to be ready")
+	}
+}
+
 // UpdateDeployment runs the given updateFunc on the deployment and waits for it to be updated
 func UpdateDeployment(kubeClientSet kubernetes.Interface, namespace string, name string, replicas int, updateFunc func(d *appsv1.Deployment) error) error {
 	deployment, err := kubeClientSet.AppsV1().Deployments(namespace).Get(context.TODO(), name, metav1.GetOptions{})

test/e2e/nginx/nginx.go

@@ -0,0 +1,132 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package nginx
+
+import (
+	"fmt"
+	"net/http"
+	"time"
+
+	"github.com/onsi/ginkgo"
+
+	"k8s.io/ingress-nginx/test/e2e/framework"
+)
+
+var (
+	cfgOK = `#
+	events {
+		worker_connections  1024;
+		multi_accept on;
+	}
+	
+	http {
+		default_type 'text/plain';
+		client_max_body_size 0;
+	
+		server {
+			access_log on;
+			access_log /dev/stdout;
+	
+			listen 80;
+	
+			location / {
+				content_by_lua_block {
+					ngx.print("ok")
+				}
+			}
+		}
+	}
+	`
+
+	cfgAlias = `#
+	events {
+		worker_connections  1024;
+		multi_accept on;
+	}
+	
+	http {
+		default_type 'text/plain';
+		client_max_body_size 0;
+	
+		server {
+			access_log on;
+			access_log /dev/stdout;
+	
+			listen 80;
+	
+			location / {
+				alias /www/html;
+			}
+		}
+	}
+	`
+
+	cfgRoot = `#
+	events {
+		worker_connections  1024;
+		multi_accept on;
+	}
+	
+	http {
+		default_type 'text/plain';
+		client_max_body_size 0;
+		root /srv/www;
+		server {
+			access_log on;
+			access_log /dev/stdout;
+	
+			listen 80;
+	
+		}
+	}
+	`
+)
+
+var _ = framework.DescribeSetting("nginx-configuration", func() {
+	f := framework.NewSimpleFramework("nginxconfiguration")
+
+	ginkgo.It("start nginx with default configuration", func() {
+
+		f.NGINXWithConfigDeployment("default-nginx", cfgOK)
+		f.WaitForPod("app=default-nginx", 60*time.Second, false)
+		framework.Sleep(5 * time.Second)
+
+		f.HTTPDumbTestClient().
+			GET("/").
+			WithURL(fmt.Sprintf("http://default-nginx.%s", f.Namespace)).
+			Expect().
+			Status(http.StatusOK).Body().Contains("ok")
+	})
+
+	ginkgo.It("fails when using alias directive", func() {
+
+		f.NGINXDeployment("alias-nginx", cfgAlias, false)
+		// This should fail with a crashloopback because our NGINX does not have
+		// alias directive!
+		f.WaitForPod("app=alias-nginx", 60*time.Second, true)
+
+	})
+
+	ginkgo.It("fails when using root directive", func() {
+
+		f.NGINXDeployment("root-nginx", cfgRoot, false)
+		// This should fail with a crashloopback because our NGINX does not have
+		// root directive!
+		f.WaitForPod("app=root-nginx", 60*time.Second, true)
+
+	})
+})