e2e tests for dynamic configuration and Lua features and a bug fix (#2254)

* e2e tests for dynamic configuration and Lua features

* do not rely on force reload to dynamically configure when reload is needed

* fix misspelling

* skip dynamic configuration in the first template rendering

* dont error on first sync

internal/ingress/controller/controller.go

@@ -169,41 +169,41 @@ func (n *NGINXController) syncIngress(item interface{}) error {
 	if !n.isForceReload() && n.runningConfig.Equal(&pcfg) {
 		glog.V(3).Infof("skipping backend reload (no changes detected)")
 		return nil
-	} else if !n.isForceReload() && n.cfg.DynamicConfigurationEnabled && n.IsDynamicallyConfigurable(&pcfg) {
-		err := n.ConfigureDynamically(&pcfg)
-		if err == nil {
-			glog.Infof("dynamic reconfiguration succeeded, skipping reload")
-			n.runningConfig = &pcfg
-			return nil
+	}
+
+	if n.cfg.DynamicConfigurationEnabled && n.IsDynamicConfigurationEnough(&pcfg) && !n.isForceReload() {
+		glog.Infof("skipping reload")
+	} else {
+		glog.Infof("backend reload required")
+
+		err := n.OnUpdate(pcfg)
+		if err != nil {
+			incReloadErrorCount()
+			glog.Errorf("unexpected failure restarting the backend: \n%v", err)
+			return err
 		}
 
-		glog.Warningf("falling back to reload, could not dynamically reconfigure: %v", err)
+		glog.Infof("ingress backend successfully reloaded...")
+		incReloadCount()
+		setSSLExpireTime(servers)
 	}
 
-	glog.Infof("backend reload required")
+	if n.cfg.DynamicConfigurationEnabled {
+		isFirstSync := n.runningConfig.Equal(&ingress.Configuration{})
+		go func(isFirstSync bool) {
+			if isFirstSync {
+				glog.Infof("first sync of Nginx configuration")
 
-	err := n.OnUpdate(pcfg)
-	if err != nil {
-		incReloadErrorCount()
-		glog.Errorf("unexpected failure restarting the backend: \n%v", err)
-		return err
-	}
-
-	glog.Infof("ingress backend successfully reloaded...")
-	incReloadCount()
-	setSSLExpireTime(servers)
-
-	if n.isForceReload() && n.cfg.DynamicConfigurationEnabled {
-		go func() {
-			// it takes time for Nginx to start listening on the port
-			time.Sleep(1 * time.Second)
+				// it takes time for Nginx to start listening on the port
+				time.Sleep(1 * time.Second)
+			}
 			err := n.ConfigureDynamically(&pcfg)
 			if err == nil {
 				glog.Infof("dynamic reconfiguration succeeded")
 			} else {
 				glog.Warningf("could not dynamically reconfigure: %v", err)
 			}
-		}()
+		}(isFirstSync)
 	}
 
 	n.runningConfig = &pcfg

internal/ingress/controller/nginx.go

@@ -751,8 +751,8 @@ func (n *NGINXController) setupSSLProxy() {
 	}()
 }
 
-// IsDynamicallyConfigurable decides if the new configuration can be dynamically configured without reloading
-func (n *NGINXController) IsDynamicallyConfigurable(pcfg *ingress.Configuration) bool {
+// IsDynamicConfigurationEnough decides if the new configuration changes can be dynamically applied without reloading
+func (n *NGINXController) IsDynamicConfigurationEnough(pcfg *ingress.Configuration) bool {
 	var copyOfRunningConfig ingress.Configuration = *n.runningConfig
 	var copyOfPcfg ingress.Configuration = *pcfg
 

internal/ingress/controller/nginx_test.go

@@ -22,7 +22,7 @@ import (
 	"k8s.io/ingress-nginx/internal/ingress"
 )
 
-func TestIsDynamicallyConfigurable(t *testing.T) {
+func TestIsDynamicConfigurationEnough(t *testing.T) {
 	backends := []*ingress.Backend{{
 		Name: "fakenamespace-myapp-80",
 		Endpoints: []ingress.Endpoint{
@@ -60,7 +60,7 @@ func TestIsDynamicallyConfigurable(t *testing.T) {
 	}
 
 	newConfig := commonConfig
-	if !n.IsDynamicallyConfigurable(newConfig) {
+	if !n.IsDynamicConfigurationEnough(newConfig) {
 		t.Errorf("When new config is same as the running config it should be deemed as dynamically configurable")
 	}
 
@@ -68,7 +68,7 @@ func TestIsDynamicallyConfigurable(t *testing.T) {
 		Backends: []*ingress.Backend{{Name: "another-backend-8081"}},
 		Servers:  []*ingress.Server{{Hostname: "myapp1.fake"}},
 	}
-	if n.IsDynamicallyConfigurable(newConfig) {
+	if n.IsDynamicConfigurationEnough(newConfig) {
 		t.Errorf("Expected to not be dynamically configurable when there's more than just backends change")
 	}
 
@@ -76,7 +76,7 @@ func TestIsDynamicallyConfigurable(t *testing.T) {
 		Backends: []*ingress.Backend{{Name: "a-backend-8080"}},
 		Servers:  servers,
 	}
-	if !n.IsDynamicallyConfigurable(newConfig) {
+	if !n.IsDynamicConfigurationEnough(newConfig) {
 		t.Errorf("Expected to be dynamically configurable when only backends change")
 	}
 

test/e2e/e2e.go

@@ -32,6 +32,7 @@ import (
 	// tests to run
 	_ "k8s.io/ingress-nginx/test/e2e/annotations"
 	_ "k8s.io/ingress-nginx/test/e2e/defaultbackend"
+	_ "k8s.io/ingress-nginx/test/e2e/lua"
 	_ "k8s.io/ingress-nginx/test/e2e/settings"
 	_ "k8s.io/ingress-nginx/test/e2e/ssl"
 )

test/e2e/framework/framework.go

@@ -19,14 +19,17 @@ import (
 	"strings"
 	"time"
 
+	appsv1beta1 "k8s.io/api/apps/v1beta1"
 	"k8s.io/api/core/v1"
 	apiextcs "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
 	restclient "k8s.io/client-go/rest"
 
 	"github.com/golang/glog"
+	"github.com/pkg/errors"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -255,3 +258,24 @@ func (f *Framework) matchNginxConditions(name string, matcher func(cfg string) b
 		return false, nil
 	}
 }
+
+// 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 *appsv1beta1.Deployment) error) error {
+	deployment, err := kubeClientSet.AppsV1beta1().Deployments(namespace).Get(name, metav1.GetOptions{})
+	if err != nil {
+		return err
+	}
+
+	if err = updateFunc(deployment); err != nil {
+		return err
+	}
+
+	err = WaitForPodsReady(kubeClientSet, 60*time.Second, replicas, namespace, metav1.ListOptions{
+		LabelSelector: fields.SelectorFromSet(fields.Set(deployment.Spec.Template.ObjectMeta.Labels)).String(),
+	})
+	if err != nil {
+		return errors.Wrap(err, "failed to wait for nginx-ingress-controller pods to restart")
+	}
+
+	return nil
+}

test/e2e/framework/k8s.go

@@ -25,6 +25,7 @@ import (
 	k8sErrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/kubernetes"
 )
 
 // EnsureSecret creates a Secret object or returns it if it already exists.
@@ -75,10 +76,15 @@ func (f *Framework) EnsureDeployment(deployment *extensions.Deployment) (*extens
 	return d, nil
 }
 
-// WaitForPodsReady waits for a given amount of time until a group of Pods is running.
+// WaitForPodsReady waits for a given amount of time until a group of Pods is running in the framework's namespace.
 func (f *Framework) WaitForPodsReady(timeout time.Duration, expectedReplicas int, opts metav1.ListOptions) error {
+	return WaitForPodsReady(f.KubeClientSet, timeout, expectedReplicas, f.Namespace.Name, opts)
+}
+
+// WaitForPodsReady waits for a given amount of time until a group of Pods is running in the given namespace.
+func WaitForPodsReady(kubeClientSet kubernetes.Interface, timeout time.Duration, expectedReplicas int, namespace string, opts metav1.ListOptions) error {
 	return wait.Poll(time.Second, timeout, func() (bool, error) {
-		pl, err := f.KubeClientSet.CoreV1().Pods(f.Namespace.Name).List(opts)
+		pl, err := kubeClientSet.CoreV1().Pods(namespace).List(opts)
 		if err != nil {
 			return false, err
 		}

test/e2e/lua/dynamic_configuration.go

@@ -0,0 +1,248 @@
+/*
+Copyright 2018 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 lua
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+	"time"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/parnurzeal/gorequest"
+
+	appsv1beta1 "k8s.io/api/apps/v1beta1"
+	extensions "k8s.io/api/extensions/v1beta1"
+	v1beta1 "k8s.io/api/extensions/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/client-go/kubernetes"
+
+	"k8s.io/ingress-nginx/test/e2e/framework"
+)
+
+var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
+	f := framework.NewDefaultFramework("dynamic-configuration")
+
+	BeforeEach(func() {
+		err := enableDynamicConfiguration(f.KubeClientSet)
+		Expect(err).NotTo(HaveOccurred())
+
+		err = f.NewEchoDeploymentWithReplicas(1)
+		Expect(err).NotTo(HaveOccurred())
+
+		host := "foo.com"
+		ing, err := ensureIngress(f, host)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ing).NotTo(BeNil())
+
+		time.Sleep(5 * time.Second)
+
+		err = f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, "proxy_pass http://upstream_balancer;")
+			})
+		Expect(err).NotTo(HaveOccurred())
+
+		resp, _, errs := gorequest.New().
+			Get(f.NginxHTTPURL).
+			Set("Host", host).
+			End()
+		Expect(len(errs)).Should(BeNumerically("==", 0))
+		Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+
+		log, err := f.NginxLogs()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(log).ToNot(ContainSubstring("could not dynamically reconfigure"))
+		Expect(log).To(ContainSubstring("first sync of Nginx configuration"))
+	})
+
+	AfterEach(func() {
+		err := disableDynamicConfiguration(f.KubeClientSet)
+		Expect(err).NotTo(HaveOccurred())
+	})
+
+	Context("when only backends change", func() {
+		It("should handle endpoints only changes", func() {
+			resp, _, errs := gorequest.New().
+				Get(fmt.Sprintf("%s?id=endpoints_only_changes", f.NginxHTTPURL)).
+				Set("Host", "foo.com").
+				End()
+			Expect(len(errs)).Should(BeNumerically("==", 0))
+			Expect(resp.StatusCode).Should(Equal(http.StatusOK))
+
+			replicas := 2
+			err := framework.UpdateDeployment(f.KubeClientSet, f.Namespace.Name, "http-svc", replicas,
+				func(deployment *appsv1beta1.Deployment) error {
+					deployment.Spec.Replicas = framework.NewInt32(int32(replicas))
+					_, err := f.KubeClientSet.AppsV1beta1().Deployments(f.Namespace.Name).Update(deployment)
+					return err
+				})
+			Expect(err).NotTo(HaveOccurred())
+
+			time.Sleep(5 * time.Second)
+			log, err := f.NginxLogs()
+			Expect(err).ToNot(HaveOccurred())
+			Expect(log).ToNot(BeEmpty())
+			index := strings.Index(log, "id=endpoints_only_changes")
+			restOfLogs := log[index:]
+
+			By("POSTing new backends to Lua endpoint")
+			Expect(restOfLogs).To(ContainSubstring("dynamic reconfiguration succeeded"))
+			Expect(restOfLogs).ToNot(ContainSubstring("could not dynamically reconfigure"))
+
+			By("skipping Nginx reload")
+			Expect(restOfLogs).ToNot(ContainSubstring("backend reload required"))
+			Expect(restOfLogs).ToNot(ContainSubstring("ingress backend successfully reloaded"))
+			Expect(restOfLogs).To(ContainSubstring("skipping reload"))
+			Expect(restOfLogs).ToNot(ContainSubstring("first sync of Nginx configuration"))
+		})
+
+		It("should handle annotation changes", func() {
+			ingress, err := f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.Namespace.Name).Get("foo.com", metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+
+			ingress.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/load-balance"] = "round_robin"
+			_, err = f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.Namespace.Name).Update(ingress)
+			Expect(err).ToNot(HaveOccurred())
+
+			time.Sleep(5 * time.Second)
+			log, err := f.NginxLogs()
+			Expect(err).ToNot(HaveOccurred())
+			Expect(log).ToNot(BeEmpty())
+			index := strings.Index(log, fmt.Sprintf("reason: 'UPDATE' Ingress %s/foo.com", f.Namespace.Name))
+			restOfLogs := log[index:]
+
+			By("POSTing new backends to Lua endpoint")
+			Expect(restOfLogs).To(ContainSubstring("dynamic reconfiguration succeeded"))
+			Expect(restOfLogs).ToNot(ContainSubstring("could not dynamically reconfigure"))
+
+			By("skipping Nginx reload")
+			Expect(restOfLogs).ToNot(ContainSubstring("backend reload required"))
+			Expect(restOfLogs).ToNot(ContainSubstring("ingress backend successfully reloaded"))
+			Expect(restOfLogs).To(ContainSubstring("skipping reload"))
+			Expect(restOfLogs).ToNot(ContainSubstring("first sync of Nginx configuration"))
+		})
+	})
+
+	It("should handle a non backend update", func() {
+		ingress, err := f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.Namespace.Name).Get("foo.com", metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+
+		ingress.Spec.TLS = []v1beta1.IngressTLS{
+			{
+				Hosts:      []string{"foo.com"},
+				SecretName: "foo.com",
+			},
+		}
+
+		_, _, _, err = framework.CreateIngressTLSSecret(f.KubeClientSet,
+			ingress.Spec.TLS[0].Hosts,
+			ingress.Spec.TLS[0].SecretName,
+			ingress.Namespace)
+		Expect(err).ToNot(HaveOccurred())
+
+		_, err = f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.Namespace.Name).Update(ingress)
+		Expect(err).ToNot(HaveOccurred())
+
+		time.Sleep(5 * time.Second)
+		log, err := f.NginxLogs()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(log).ToNot(BeEmpty())
+
+		By("reloading Nginx")
+		Expect(log).To(ContainSubstring("ingress backend successfully reloaded"))
+
+		By("POSTing new backends to Lua endpoint")
+		Expect(log).To(ContainSubstring("dynamic reconfiguration succeeded"))
+
+		By("still be proxying requests through Lua balancer")
+		err = f.WaitForNginxServer("foo.com",
+			func(server string) bool {
+				return strings.Contains(server, "proxy_pass http://upstream_balancer;")
+			})
+		Expect(err).NotTo(HaveOccurred())
+
+		By("generating the respective ssl listen directive")
+		err = f.WaitForNginxServer("foo.com",
+			func(server string) bool {
+				return strings.Contains(server, "server_name foo.com") &&
+					strings.Contains(server, "listen 443")
+			})
+		Expect(err).ToNot(HaveOccurred())
+	})
+})
+
+func enableDynamicConfiguration(kubeClientSet kubernetes.Interface) error {
+	return framework.UpdateDeployment(kubeClientSet, "ingress-nginx", "nginx-ingress-controller", 1,
+		func(deployment *appsv1beta1.Deployment) error {
+			args := deployment.Spec.Template.Spec.Containers[0].Args
+			args = append(args, "--enable-dynamic-configuration")
+			deployment.Spec.Template.Spec.Containers[0].Args = args
+			_, err := kubeClientSet.AppsV1beta1().Deployments("ingress-nginx").Update(deployment)
+			return err
+		})
+}
+
+func disableDynamicConfiguration(kubeClientSet kubernetes.Interface) error {
+	return framework.UpdateDeployment(kubeClientSet, "ingress-nginx", "nginx-ingress-controller", 1,
+		func(deployment *appsv1beta1.Deployment) error {
+			args := deployment.Spec.Template.Spec.Containers[0].Args
+			var newArgs []string
+			for _, arg := range args {
+				if arg != "--enable-dynamic-configuration" {
+					newArgs = append(newArgs, arg)
+				}
+			}
+			deployment.Spec.Template.Spec.Containers[0].Args = newArgs
+			_, err := kubeClientSet.AppsV1beta1().Deployments("ingress-nginx").Update(deployment)
+			return err
+		})
+}
+
+func ensureIngress(f *framework.Framework, host string) (*extensions.Ingress, error) {
+	return f.EnsureIngress(&v1beta1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      host,
+			Namespace: f.Namespace.Name,
+			Annotations: map[string]string{
+				"nginx.ingress.kubernetes.io/load-balance": "ewma",
+			},
+		},
+		Spec: v1beta1.IngressSpec{
+			Rules: []v1beta1.IngressRule{
+				{
+					Host: host,
+					IngressRuleValue: v1beta1.IngressRuleValue{
+						HTTP: &v1beta1.HTTPIngressRuleValue{
+							Paths: []v1beta1.HTTPIngressPath{
+								{
+									Path: "/",
+									Backend: v1beta1.IngressBackend{
+										ServiceName: "http-svc",
+										ServicePort: intstr.FromInt(80),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	})
+}