From c1a6eabab10f9f64a703bd533694c936b2b2e704 Mon Sep 17 00:00:00 2001 From: dean-coakley Date: Mon, 23 Jul 2018 09:44:00 +0100 Subject: [PATCH 1/2] Fix namespace check for findIngressForSecret --- nginx-controller/controller/controller.go | 16 ++- .../controller/controller_test.go | 110 ++++++++++++++++++ 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/nginx-controller/controller/controller.go b/nginx-controller/controller/controller.go index bf4e94da8f..140ebf683b 100644 --- a/nginx-controller/controller/controller.go +++ b/nginx-controller/controller/controller.go @@ -757,13 +757,13 @@ func (lbc *LoadBalancerController) syncSecret(task Task) { return } - _, name, err := ParseNamespaceName(key) + namespace, name, err := ParseNamespaceName(key) if err != nil { glog.Warningf("Secret key %v is invalid: %v", key, err) return } - ings, err := lbc.findIngressesForSecret(name) + ings, err := lbc.findIngressesForSecret(namespace, name) if err != nil { glog.Warningf("Failed to find Ingress resources for Secret %v: %v", key, err) lbc.syncQueue.requeueAfter(task, err, 5*time.Second) @@ -780,7 +780,7 @@ func (lbc *LoadBalancerController) syncSecret(task Task) { for _, ing := range ings { lbc.syncQueue.enqueue(&ing) - lbc.recorder.Eventf(&ing, api_v1.EventTypeWarning, "Rejected", "%v/%v was rejected due to deleted Secret %v: %v", ing.Namespace, ing.Name, key) + lbc.recorder.Eventf(&ing, api_v1.EventTypeWarning, "Rejected", "%v/%v was rejected due to deleted Secret %v", ing.Namespace, ing.Name, key) } if key == lbc.defaultServerSecret { @@ -839,7 +839,7 @@ func (lbc *LoadBalancerController) syncSecret(task Task) { } } -func (lbc *LoadBalancerController) findIngressesForSecret(secret string) ([]extensions.Ingress, error) { +func (lbc *LoadBalancerController) findIngressesForSecret(secretNamespace string, secretName string) ([]extensions.Ingress, error) { res := []extensions.Ingress{} ings, err := lbc.ingLister.List() if err != nil { @@ -848,6 +848,10 @@ func (lbc *LoadBalancerController) findIngressesForSecret(secret string) ([]exte items: for _, ing := range ings.Items { + if ing.Namespace != secretNamespace { + continue + } + if !lbc.isNginxIngress(&ing) { continue } @@ -855,14 +859,14 @@ items: continue } for _, tls := range ing.Spec.TLS { - if tls.SecretName == secret { + if tls.SecretName == secretName { res = append(res, ing) continue items } } if lbc.nginxPlus { if jwtKey, exists := ing.Annotations[nginx.JWTKeyAnnotation]; exists { - if jwtKey == secret { + if jwtKey == secretName { res = append(res, ing) } } diff --git a/nginx-controller/controller/controller_test.go b/nginx-controller/controller/controller_test.go index c41ada87f0..1d41c96a7a 100644 --- a/nginx-controller/controller/controller_test.go +++ b/nginx-controller/controller/controller_test.go @@ -2,6 +2,7 @@ package controller import ( "fmt" + "net/http" "reflect" "testing" "time" @@ -827,3 +828,112 @@ func TestGetServicePortForIngressPort(t *testing.T) { t.Errorf("Mismatched strings should not return port: %+v", svcPort) } } + +func TestFindIngressesForSecret(t *testing.T) { + testCases := []struct { + secretName string + secretNamespace string + ingressName string + ingressNamespace string + tlsSecretName string + expectResult bool + }{ + {"tls-secret1", "namespace1", "my-ingress1", "namespace1", "tls-secret1", true}, // Try find secret in correct namespace - pass + {"tls-secret2", "namespace1", "my-ingress2", "namespace2", "tls-secret2", false}, // Try find secret in wrong namespace - err + {"no-match3", "namespace1", "not-ingress3", "namespace1", "tls-xxx3", false}, // Try find secret that doesn't exist - err + } + + for _, test := range testCases { + fakeClient := fake.NewSimpleClientset() + + templateExecutor, err := nginx.NewTemplateExecutor("../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl", true) + if err != nil { + t.Errorf("templateExecuter could not start: %v", err) + } + ngxc, err := nginx.NewNginxController("/etc/nginx", true, true, "../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl") + if err != nil { + t.Errorf("NGINX Controller could not start: %v", err) + } + apiCtrl, err := plus.NewNginxAPIController(&http.Client{}, "", true) + if err != nil { + t.Errorf("NGINX API Controller could not start: %v", err) + } + + cnf := nginx.NewConfigurator(ngxc, &nginx.Config{}, apiCtrl, templateExecutor) + lbc := LoadBalancerController{ + client: fakeClient, + ingressClass: "nginx", + cnf: cnf, + nginxPlus: true, + } + + lbc.ingLister.Store, _ = cache.NewInformer( + cache.NewListWatchFromClient(lbc.client.ExtensionsV1beta1().RESTClient(), "ingresses", "default", fields.Everything()), + &extensions.Ingress{}, time.Duration(1), nil) + + lbc.secrLister.Store, lbc.secrController = cache.NewInformer( + cache.NewListWatchFromClient(lbc.client.Core().RESTClient(), "secrets", "default", fields.Everything()), + &v1.Secret{}, time.Duration(1), nil) + + var testIngresses = []extensions.Ingress{ + { + ObjectMeta: meta_v1.ObjectMeta{ + Name: test.ingressName, + Namespace: test.ingressNamespace, + }, + Spec: extensions.IngressSpec{ + TLS: []extensions.IngressTLS{ + extensions.IngressTLS{ + SecretName: test.tlsSecretName, + }, + }, + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Name: test.ingressName, + Namespace: test.ingressNamespace, + Annotations: map[string]string{ + nginx.JWTKeyAnnotation: test.tlsSecretName, + }, + }, + }, + } + + for _, ing := range testIngresses { + ngxIngress := &nginx.IngressEx{Ingress: &ing} + + secret := &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: test.secretName, + Namespace: test.secretNamespace, + }, + } + ngxIngress.TLSSecrets = make(map[string]*v1.Secret) + ngxIngress.TLSSecrets[test.tlsSecretName] = secret + + err = cnf.AddOrUpdateIngress(ngxIngress) + if err != nil { + t.Errorf("Ingress was not added: %v", err) + } + + lbc.ingLister.Add(&ing) + + ingresses, err := lbc.findIngressesForSecret(test.secretNamespace, test.secretName) + if err != nil { + t.Errorf("Couldn't list Ingress resource: %v", err) + } + + if len(ingresses) > 0 { + if !test.expectResult { + t.Errorf("Expected no ingresses. Got: %v/%v", ingresses[0].Namespace, ingresses[0].Name) + } + if ingresses[0].Name != test.ingressName || ingresses[0].Namespace != test.ingressNamespace { + t.Errorf("Expected: %v/%v. Got: %v/%v.", test.ingressNamespace, test.ingressName, ingresses[0].Namespace, ingresses[0].Name) + } + } else if test.expectResult { + t.Errorf("Expected %v/%v. Got: No ingresses. %v", test.ingressNamespace, test.ingressName, err) + } + } + } +} From 38065b7641fa8b611f8164923c98dbc26db2f83f Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Fri, 27 Jul 2018 17:09:30 -0700 Subject: [PATCH 2/2] Improve TestFindIngressesForSecret --- .../controller/controller_test.go | 189 +++++++++++------- 1 file changed, 120 insertions(+), 69 deletions(-) diff --git a/nginx-controller/controller/controller_test.go b/nginx-controller/controller/controller_test.go index 1d41c96a7a..6454a83c10 100644 --- a/nginx-controller/controller/controller_test.go +++ b/nginx-controller/controller/controller_test.go @@ -831,109 +831,160 @@ func TestGetServicePortForIngressPort(t *testing.T) { func TestFindIngressesForSecret(t *testing.T) { testCases := []struct { - secretName string - secretNamespace string - ingressName string - ingressNamespace string - tlsSecretName string - expectResult bool + secret v1.Secret + ingress extensions.Ingress + expectedToFind bool + desc string }{ - {"tls-secret1", "namespace1", "my-ingress1", "namespace1", "tls-secret1", true}, // Try find secret in correct namespace - pass - {"tls-secret2", "namespace1", "my-ingress2", "namespace2", "tls-secret2", false}, // Try find secret in wrong namespace - err - {"no-match3", "namespace1", "not-ingress3", "namespace1", "tls-xxx3", false}, // Try find secret that doesn't exist - err - } - - for _, test := range testCases { - fakeClient := fake.NewSimpleClientset() - - templateExecutor, err := nginx.NewTemplateExecutor("../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl", true) - if err != nil { - t.Errorf("templateExecuter could not start: %v", err) - } - ngxc, err := nginx.NewNginxController("/etc/nginx", true, true, "../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl") - if err != nil { - t.Errorf("NGINX Controller could not start: %v", err) - } - apiCtrl, err := plus.NewNginxAPIController(&http.Client{}, "", true) - if err != nil { - t.Errorf("NGINX API Controller could not start: %v", err) - } - - cnf := nginx.NewConfigurator(ngxc, &nginx.Config{}, apiCtrl, templateExecutor) - lbc := LoadBalancerController{ - client: fakeClient, - ingressClass: "nginx", - cnf: cnf, - nginxPlus: true, - } - - lbc.ingLister.Store, _ = cache.NewInformer( - cache.NewListWatchFromClient(lbc.client.ExtensionsV1beta1().RESTClient(), "ingresses", "default", fields.Everything()), - &extensions.Ingress{}, time.Duration(1), nil) - - lbc.secrLister.Store, lbc.secrController = cache.NewInformer( - cache.NewListWatchFromClient(lbc.client.Core().RESTClient(), "secrets", "default", fields.Everything()), - &v1.Secret{}, time.Duration(1), nil) - - var testIngresses = []extensions.Ingress{ - { + { + secret: v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "my-tls-secret", + Namespace: "namespace-1", + }, + }, + ingress: extensions.Ingress{ ObjectMeta: meta_v1.ObjectMeta{ - Name: test.ingressName, - Namespace: test.ingressNamespace, + Name: "my-ingress", + Namespace: "namespace-1", }, Spec: extensions.IngressSpec{ TLS: []extensions.IngressTLS{ extensions.IngressTLS{ - SecretName: test.tlsSecretName, + SecretName: "my-tls-secret", }, }, }, }, - { + expectedToFind: true, + desc: "an Ingress references a TLS Secret that exists in the Ingress namespace", + }, + { + secret: v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "my-tls-secret", + Namespace: "namespace-1", + }, + }, + ingress: extensions.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "my-ingress", + Namespace: "namespace-2", + }, + Spec: extensions.IngressSpec{ + TLS: []extensions.IngressTLS{ + extensions.IngressTLS{ + SecretName: "my-tls-secret", + }, + }, + }, + }, + expectedToFind: false, + desc: "an Ingress references a TLS Secret that exists in a different namespace", + }, + { + secret: v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "my-jwk-secret", + Namespace: "namespace-1", + }, + }, + ingress: extensions.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "my-ingress", + Namespace: "namespace-1", + Annotations: map[string]string{ + nginx.JWTKeyAnnotation: "my-jwk-secret", + }, + }, + }, + expectedToFind: true, + desc: "an Ingress references a JWK Secret that exists in the Ingress namespace", + }, + { + secret: v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "my-jwk-secret", + Namespace: "namespace-1", + }, + }, + ingress: extensions.Ingress{ ObjectMeta: meta_v1.ObjectMeta{ - Name: test.ingressName, - Namespace: test.ingressNamespace, + Name: "my-ingress", + Namespace: "namespace-2", Annotations: map[string]string{ - nginx.JWTKeyAnnotation: test.tlsSecretName, + nginx.JWTKeyAnnotation: "my-jwk-secret", }, }, }, - } + expectedToFind: false, + desc: "an Ingress references a JWK secret that exists in a different namespace", + }, + } - for _, ing := range testIngresses { - ngxIngress := &nginx.IngressEx{Ingress: &ing} + for _, test := range testCases { + t.Run(test.desc, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset() - secret := &v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: test.secretName, - Namespace: test.secretNamespace, + templateExecutor, err := nginx.NewTemplateExecutor("../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl", true) + if err != nil { + t.Errorf("templateExecuter could not start: %v", err) + } + ngxc, err := nginx.NewNginxController("/etc/nginx", true, true, "../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl") + if err != nil { + t.Errorf("NGINX Controller could not start: %v", err) + } + apiCtrl, err := plus.NewNginxAPIController(&http.Client{}, "", true) + if err != nil { + t.Errorf("NGINX API Controller could not start: %v", err) + } + + cnf := nginx.NewConfigurator(ngxc, &nginx.Config{}, apiCtrl, templateExecutor) + lbc := LoadBalancerController{ + client: fakeClient, + ingressClass: "nginx", + cnf: cnf, + nginxPlus: true, + } + + lbc.ingLister.Store, _ = cache.NewInformer( + cache.NewListWatchFromClient(lbc.client.ExtensionsV1beta1().RESTClient(), "ingresses", "default", fields.Everything()), + &extensions.Ingress{}, time.Duration(1), nil) + + lbc.secrLister.Store, lbc.secrController = cache.NewInformer( + cache.NewListWatchFromClient(lbc.client.Core().RESTClient(), "secrets", "default", fields.Everything()), + &v1.Secret{}, time.Duration(1), nil) + + ngxIngress := &nginx.IngressEx{ + Ingress: &test.ingress, + TLSSecrets: map[string]*v1.Secret{ + test.secret.ObjectMeta.Name: &test.secret, }, } - ngxIngress.TLSSecrets = make(map[string]*v1.Secret) - ngxIngress.TLSSecrets[test.tlsSecretName] = secret err = cnf.AddOrUpdateIngress(ngxIngress) if err != nil { t.Errorf("Ingress was not added: %v", err) } - lbc.ingLister.Add(&ing) + lbc.ingLister.Add(&test.ingress) + lbc.secrLister.Add(&test.secret) - ingresses, err := lbc.findIngressesForSecret(test.secretNamespace, test.secretName) + ingresses, err := lbc.findIngressesForSecret(test.secret.ObjectMeta.Namespace, test.secret.ObjectMeta.Name) if err != nil { t.Errorf("Couldn't list Ingress resource: %v", err) } if len(ingresses) > 0 { - if !test.expectResult { + if !test.expectedToFind { t.Errorf("Expected no ingresses. Got: %v/%v", ingresses[0].Namespace, ingresses[0].Name) } - if ingresses[0].Name != test.ingressName || ingresses[0].Namespace != test.ingressNamespace { - t.Errorf("Expected: %v/%v. Got: %v/%v.", test.ingressNamespace, test.ingressName, ingresses[0].Namespace, ingresses[0].Name) + if ingresses[0].Name != test.ingress.ObjectMeta.Name || ingresses[0].Namespace != test.ingress.ObjectMeta.Namespace { + t.Errorf("Expected: %v/%v. Got: %v/%v.", test.ingress.ObjectMeta.Namespace, test.ingress.ObjectMeta.Name, ingresses[0].Namespace, ingresses[0].Name) } - } else if test.expectResult { - t.Errorf("Expected %v/%v. Got: No ingresses. %v", test.ingressNamespace, test.ingressName, err) + } else if test.expectedToFind { + t.Errorf("Expected %v/%v. Got: No ingresses. %v", test.ingress.ObjectMeta.Namespace, test.ingress.ObjectMeta.Name, err) } - } + }) } }