From 0338f63301574ec69a567a2bbe3e6d826e93589d Mon Sep 17 00:00:00 2001 From: Elvin Efendi Date: Sat, 24 Mar 2018 16:04:15 -0400 Subject: [PATCH 1/5] e2e tests for dynamic configuration and Lua features --- test/e2e/e2e.go | 1 + test/e2e/framework/framework.go | 24 +++ test/e2e/framework/k8s.go | 10 +- test/e2e/lua/dynamic_configuration.go | 252 ++++++++++++++++++++++++++ 4 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 test/e2e/lua/dynamic_configuration.go diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go index 4639b7a3d5..6a628d64dd 100644 --- a/test/e2e/e2e.go +++ b/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" ) diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index f007c3d283..2d8831d2fe 100644 --- a/test/e2e/framework/framework.go +++ b/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 +} diff --git a/test/e2e/framework/k8s.go b/test/e2e/framework/k8s.go index 82c34a666e..198bce78d6 100644 --- a/test/e2e/framework/k8s.go +++ b/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 } diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go new file mode 100644 index 0000000000..9cfe9eda25 --- /dev/null +++ b/test/e2e/lua/dynamic_configuration.go @@ -0,0 +1,252 @@ +/* +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 ( + "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 kubeClientSet kubernetes.Interface + +var _ = BeforeSuite(func() { + kubeConfig, err := framework.LoadConfig(framework.TestContext.KubeConfig, framework.TestContext.KubeContext) + Expect(err).NotTo(HaveOccurred()) + + kubeClientSet, err = kubernetes.NewForConfig(kubeConfig) + Expect(err).NotTo(HaveOccurred()) + Expect(kubeClientSet).NotTo(BeNil()) + + err = enableDynamicConfiguration() + Expect(err).NotTo(HaveOccurred()) + + time.Sleep(5 * time.Second) +}) + +var _ = AfterSuite(func() { + Expect(kubeClientSet).NotTo(BeNil()) + + err := disableDynamicConfiguration() + Expect(err).NotTo(HaveOccurred()) +}) + +var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { + f := framework.NewDefaultFramework("dynamic-configuration") + + BeforeEach(func() { + err := f.NewEchoDeploymentWithReplicas(1) + Expect(err).NotTo(HaveOccurred()) + + host := "foo.com" + ing, err := ensureIngress(f, host) + Expect(err).NotTo(HaveOccurred()) + Expect(ing).NotTo(BeNil()) + + err = f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "proxy_pass http://upstream_balancer;") + }) + Expect(err).NotTo(HaveOccurred()) + + // TODO(elvinefendi) consider using Eventually here and in all the following similar assertions + // or another better approach - this is super flaky + time.Sleep(5 * time.Second) + + resp, _, errs := gorequest.New(). + Get(f.NginxHTTPURL). + Set("Host", host). + End() + + Expect(len(errs)).Should(BeNumerically("==", 0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + }) + + Context("when only backends change", func() { + It("should handle endpoints only changes", func() { + replicas := 3 + 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) + + resp, _, errs := gorequest.New(). + Get(f.NginxHTTPURL). + Set("Host", "foo.com"). + End() + + Expect(len(errs)).Should(BeNumerically("==", 0)) + Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + + time.Sleep(5 * time.Second) + + log, err := f.NginxLogs() + Expect(err).ToNot(HaveOccurred()) + Expect(log).ToNot(BeEmpty()) + + By("skipping Nginx reload") + Expect(log).To(ContainSubstring("skipping reload since only backends changed")) + + By("POSTing new backends to Lua endpoint") + Expect(log).To(ContainSubstring("dynamic reconfiguration succeeded")) + }) + + 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()) + + By("skipping Nginx reload") + Expect(log).To(ContainSubstring("skipping reload since only backends changed")) + + By("POSTing new backends to Lua endpoint") + Expect(log).To(ContainSubstring("dynamic reconfiguration succeeded")) + }) + }) + + 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(15 * 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() 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() 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), + }, + }, + }, + }, + }, + }, + }, + }, + }) +} From 3f27e51bb3ff777d234500346aa266dd9452961f Mon Sep 17 00:00:00 2001 From: Elvin Efendi Date: Sat, 31 Mar 2018 12:04:58 -0400 Subject: [PATCH 2/5] do not rely on force reload to dynamically configure when reload is needed --- internal/ingress/controller/controller.go | 31 +++++++++-------------- internal/ingress/controller/nginx.go | 4 +-- internal/ingress/controller/nginx_test.go | 2 +- test/e2e/lua/dynamic_configuration.go | 4 +-- 4 files changed, 17 insertions(+), 24 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 9e7a7f833a..e4e18ebf20 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -169,15 +169,21 @@ 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) { + } + + if n.cfg.DynamicConfigurationEnabled { err := n.ConfigureDynamically(&pcfg) if err == nil { - glog.Infof("dynamic reconfiguration succeeded, skipping reload") - n.runningConfig = &pcfg - return nil - } + glog.Infof("dynamic reconfiguration succeeded") - glog.Warningf("falling back to reload, could not dynamically reconfigure: %v", err) + if !n.isForceReload() && n.IsDynamicConfiguratonEnough(&pcfg) { + glog.Infof("skipping reload") + n.runningConfig = &pcfg + return nil + } + } else { + glog.Warningf("could not dynamically reconfigure: %v", err) + } } glog.Infof("backend reload required") @@ -193,19 +199,6 @@ func (n *NGINXController) syncIngress(item interface{}) error { 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) - err := n.ConfigureDynamically(&pcfg) - if err == nil { - glog.Infof("dynamic reconfiguration succeeded") - } else { - glog.Warningf("could not dynamically reconfigure: %v", err) - } - }() - } - n.runningConfig = &pcfg n.SetForceReload(false) diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 7b8edc53d5..23a2e99244 100644 --- a/internal/ingress/controller/nginx.go +++ b/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 { +// IsDynamicConfiguratonEnough decides if the new configuration changes can be dynamically applied without reloading +func (n *NGINXController) IsDynamicConfiguratonEnough(pcfg *ingress.Configuration) bool { var copyOfRunningConfig ingress.Configuration = *n.runningConfig var copyOfPcfg ingress.Configuration = *pcfg diff --git a/internal/ingress/controller/nginx_test.go b/internal/ingress/controller/nginx_test.go index 7be8ce135d..e671b38f75 100644 --- a/internal/ingress/controller/nginx_test.go +++ b/internal/ingress/controller/nginx_test.go @@ -22,7 +22,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress" ) -func TestIsDynamicallyConfigurable(t *testing.T) { +func IsDynamicConfiguratonEnough(t *testing.T) { backends := []*ingress.Backend{{ Name: "fakenamespace-myapp-80", Endpoints: []ingress.Endpoint{ diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index 9cfe9eda25..9f28f1091a 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -117,7 +117,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(log).ToNot(BeEmpty()) By("skipping Nginx reload") - Expect(log).To(ContainSubstring("skipping reload since only backends changed")) + Expect(log).To(ContainSubstring("skipping reload")) By("POSTing new backends to Lua endpoint") Expect(log).To(ContainSubstring("dynamic reconfiguration succeeded")) @@ -138,7 +138,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(log).ToNot(BeEmpty()) By("skipping Nginx reload") - Expect(log).To(ContainSubstring("skipping reload since only backends changed")) + Expect(log).To(ContainSubstring("skipping reload")) By("POSTing new backends to Lua endpoint") Expect(log).To(ContainSubstring("dynamic reconfiguration succeeded")) From 9d21c42a250c7ce9481c3dc559f44452263b5bb7 Mon Sep 17 00:00:00 2001 From: Elvin Efendi Date: Sat, 31 Mar 2018 12:15:51 -0400 Subject: [PATCH 3/5] fix misspelling --- internal/ingress/controller/controller.go | 2 +- internal/ingress/controller/nginx.go | 4 +- internal/ingress/controller/nginx_test.go | 8 +- test/e2e/lua/dynamic_configuration.go | 146 +++++++++++----------- 4 files changed, 79 insertions(+), 81 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index e4e18ebf20..786031aa1a 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -176,7 +176,7 @@ func (n *NGINXController) syncIngress(item interface{}) error { if err == nil { glog.Infof("dynamic reconfiguration succeeded") - if !n.isForceReload() && n.IsDynamicConfiguratonEnough(&pcfg) { + if !n.isForceReload() && n.IsDynamicConfigurationEnough(&pcfg) { glog.Infof("skipping reload") n.runningConfig = &pcfg return nil diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 23a2e99244..a424f0f0e5 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -751,8 +751,8 @@ func (n *NGINXController) setupSSLProxy() { }() } -// IsDynamicConfiguratonEnough decides if the new configuration changes can be dynamically applied without reloading -func (n *NGINXController) IsDynamicConfiguratonEnough(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 diff --git a/internal/ingress/controller/nginx_test.go b/internal/ingress/controller/nginx_test.go index e671b38f75..7b4877a8c2 100644 --- a/internal/ingress/controller/nginx_test.go +++ b/internal/ingress/controller/nginx_test.go @@ -22,7 +22,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress" ) -func IsDynamicConfiguratonEnough(t *testing.T) { +func TestIsDynamicConfigurationEnough(t *testing.T) { backends := []*ingress.Backend{{ Name: "fakenamespace-myapp-80", Endpoints: []ingress.Endpoint{ @@ -60,7 +60,7 @@ func IsDynamicConfiguratonEnough(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 IsDynamicConfiguratonEnough(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 IsDynamicConfiguratonEnough(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") } diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index 9f28f1091a..e904c00029 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -17,6 +17,7 @@ limitations under the License. package lua import ( + "fmt" "net/http" "strings" "time" @@ -35,34 +36,14 @@ import ( "k8s.io/ingress-nginx/test/e2e/framework" ) -var kubeClientSet kubernetes.Interface - -var _ = BeforeSuite(func() { - kubeConfig, err := framework.LoadConfig(framework.TestContext.KubeConfig, framework.TestContext.KubeContext) - Expect(err).NotTo(HaveOccurred()) - - kubeClientSet, err = kubernetes.NewForConfig(kubeConfig) - Expect(err).NotTo(HaveOccurred()) - Expect(kubeClientSet).NotTo(BeNil()) - - err = enableDynamicConfiguration() - Expect(err).NotTo(HaveOccurred()) - - time.Sleep(5 * time.Second) -}) - -var _ = AfterSuite(func() { - Expect(kubeClientSet).NotTo(BeNil()) - - err := disableDynamicConfiguration() - Expect(err).NotTo(HaveOccurred()) -}) - var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { f := framework.NewDefaultFramework("dynamic-configuration") BeforeEach(func() { - err := f.NewEchoDeploymentWithReplicas(1) + err := enableDynamicConfiguration(f.KubeClientSet) + Expect(err).NotTo(HaveOccurred()) + + err = f.NewEchoDeploymentWithReplicas(1) Expect(err).NotTo(HaveOccurred()) host := "foo.com" @@ -70,57 +51,69 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { 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()) - // TODO(elvinefendi) consider using Eventually here and in all the following similar assertions - // or another better approach - this is super flaky - time.Sleep(5 * time.Second) - resp, _, errs := gorequest.New(). Get(f.NginxHTTPURL). Set("Host", host). End() - Expect(len(errs)).Should(BeNumerically("==", 0)) Expect(resp.StatusCode).Should(Equal(http.StatusOK)) + + // NOTE(elvinefendi) this is to document the not so desired behaviour + // where the controller tries to POST to Lua endpoint right after starting + // Nginx when it does not have the correct endpoint configuration yet + log, err := f.NginxLogs() + Expect(err).ToNot(HaveOccurred()) + index := strings.Index(log, "could not dynamically reconfigure") + Expect(index).Should(BeNumerically(">", -1)) + Expect(strings.Index(log[index+1:], "could not dynamically reconfigure")).Should(Equal(-1)) + }) + + AfterEach(func() { + err := disableDynamicConfiguration(f.KubeClientSet) + Expect(err).NotTo(HaveOccurred()) }) Context("when only backends change", func() { It("should handle endpoints only changes", func() { - replicas := 3 - 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) - resp, _, errs := gorequest.New(). - Get(f.NginxHTTPURL). + 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)) - time.Sleep(5 * time.Second) + 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()) - - By("skipping Nginx reload") - Expect(log).To(ContainSubstring("skipping reload")) + index := strings.Index(log, "id=endpoints_only_changes") + restOfLogs := log[index:] By("POSTing new backends to Lua endpoint") - Expect(log).To(ContainSubstring("dynamic reconfiguration succeeded")) + Expect(restOfLogs).To(ContainSubstring("dynamic reconfiguration succeeded")) + Expect(restOfLogs).ToNot(ContainSubstring("could not dynamically reconfigure")) + + By("skipping Nginx reload") + Expect(restOfLogs).To(ContainSubstring("skipping reload")) + Expect(restOfLogs).ToNot(ContainSubstring("backend reload required")) + Expect(restOfLogs).ToNot(ContainSubstring("ingress backend successfully reloaded")) }) It("should handle annotation changes", func() { @@ -132,16 +125,20 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(err).ToNot(HaveOccurred()) time.Sleep(5 * time.Second) - log, err := f.NginxLogs() Expect(err).ToNot(HaveOccurred()) Expect(log).ToNot(BeEmpty()) - - By("skipping Nginx reload") - Expect(log).To(ContainSubstring("skipping reload")) + 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(log).To(ContainSubstring("dynamic reconfiguration succeeded")) + Expect(restOfLogs).To(ContainSubstring("dynamic reconfiguration succeeded")) + Expect(restOfLogs).ToNot(ContainSubstring("could not dynamically reconfigure")) + + By("skipping Nginx reload") + Expect(restOfLogs).To(ContainSubstring("skipping reload")) + Expect(restOfLogs).ToNot(ContainSubstring("backend reload required")) + Expect(restOfLogs).ToNot(ContainSubstring("ingress backend successfully reloaded")) }) }) @@ -165,8 +162,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { _, err = f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.Namespace.Name).Update(ingress) Expect(err).ToNot(HaveOccurred()) - time.Sleep(15 * time.Second) - + time.Sleep(5 * time.Second) log, err := f.NginxLogs() Expect(err).ToNot(HaveOccurred()) Expect(log).ToNot(BeEmpty()) @@ -194,29 +190,31 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { }) }) -func enableDynamicConfiguration() 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 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() 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) +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 - }) + 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) { From 753b6a3450e254c9aa40b687fd69891a1852b3b2 Mon Sep 17 00:00:00 2001 From: Elvin Efendi Date: Sun, 1 Apr 2018 11:24:32 -0400 Subject: [PATCH 4/5] skip dynamic configuration in the first template rendering --- internal/ingress/controller/controller.go | 2 +- test/e2e/lua/dynamic_configuration.go | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 786031aa1a..4b93abd429 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -171,7 +171,7 @@ func (n *NGINXController) syncIngress(item interface{}) error { return nil } - if n.cfg.DynamicConfigurationEnabled { + if n.cfg.DynamicConfigurationEnabled && !n.runningConfig.Equal(&ingress.Configuration{}) { err := n.ConfigureDynamically(&pcfg) if err == nil { glog.Infof("dynamic reconfiguration succeeded") diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index e904c00029..8b074de117 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -66,14 +66,9 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(len(errs)).Should(BeNumerically("==", 0)) Expect(resp.StatusCode).Should(Equal(http.StatusOK)) - // NOTE(elvinefendi) this is to document the not so desired behaviour - // where the controller tries to POST to Lua endpoint right after starting - // Nginx when it does not have the correct endpoint configuration yet log, err := f.NginxLogs() Expect(err).ToNot(HaveOccurred()) - index := strings.Index(log, "could not dynamically reconfigure") - Expect(index).Should(BeNumerically(">", -1)) - Expect(strings.Index(log[index+1:], "could not dynamically reconfigure")).Should(Equal(-1)) + Expect(log).ToNot(ContainSubstring("could not dynamically reconfigure")) }) AfterEach(func() { From 0f316c7f220e3e570616203032b447bae6dbc8cf Mon Sep 17 00:00:00 2001 From: Elvin Efendi Date: Sun, 1 Apr 2018 13:47:04 -0400 Subject: [PATCH 5/5] dont error on first sync --- internal/ingress/controller/controller.go | 51 +++++++++++++---------- test/e2e/lua/dynamic_configuration.go | 7 +++- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 4b93abd429..0e3d1d24af 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -171,34 +171,41 @@ func (n *NGINXController) syncIngress(item interface{}) error { return nil } - if n.cfg.DynamicConfigurationEnabled && !n.runningConfig.Equal(&ingress.Configuration{}) { - err := n.ConfigureDynamically(&pcfg) - if err == nil { - glog.Infof("dynamic reconfiguration succeeded") - - if !n.isForceReload() && n.IsDynamicConfigurationEnough(&pcfg) { - glog.Infof("skipping reload") - n.runningConfig = &pcfg - return nil - } - } else { - glog.Warningf("could not dynamically reconfigure: %v", err) + 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.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 + // 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) } - glog.Infof("ingress backend successfully reloaded...") - incReloadCount() - setSSLExpireTime(servers) - n.runningConfig = &pcfg n.SetForceReload(false) diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index 8b074de117..3e04749c15 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -69,6 +69,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { 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() { @@ -106,9 +107,10 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(restOfLogs).ToNot(ContainSubstring("could not dynamically reconfigure")) By("skipping Nginx reload") - Expect(restOfLogs).To(ContainSubstring("skipping 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() { @@ -131,9 +133,10 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { Expect(restOfLogs).ToNot(ContainSubstring("could not dynamically reconfigure")) By("skipping Nginx reload") - Expect(restOfLogs).To(ContainSubstring("skipping 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")) }) })