From 7596444dccb0f28808127ec9b91a52ea19c46687 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 26 Sep 2018 19:14:33 +0100 Subject: [PATCH] Improve secret handling - Simplify the secret handling logic. - Change the secret handling logic. See below. Changes: 1. An Ingress includes TLS termination, but the referenced TLS Secret is missing in Kubernetes/invalid or goes missing/ becomes invalid. An Ingress can be a regular Ingress or a mergeable Master. Before: the Ingress resource was rejected. Now: the Ingress resource is not rejected. Instead, the generated config for that Ingress resource now includes the ssl_ciphers directive set to "NULL", which makes NGINX break any attempt to establish a TLS connection with the corresponding Ingress host. 2. An Ingress includes JWT auth, but the referenced JWK is missing in Kubernetes/invalid or goes missing/becomes invalid. An Ingress can be a regular Ingress, a mergeable Master or a mergeable Minion. Before: the Ingress resource was rejected. Now. the Ingress resource is not rejected. However, the generated config for that Ingress still references the JWK on the file system, which does not exist. This makes NGINX Plus return a 500 response for a request to the corresponding Ingress host (or the path of the host for mergeable minions). --- internal/controller/controller.go | 291 ++++++++++-------- internal/controller/controller_test.go | 208 ++++++++++++- internal/handlers/secret.go | 7 +- internal/nginx/configurator.go | 93 ++++-- internal/nginx/configurator_test.go | 53 +++- internal/nginx/ingress.go | 8 +- internal/nginx/nginx.go | 1 + internal/nginx/secret.go | 4 +- .../nginx/templates/nginx-plus.ingress.tmpl | 3 + internal/nginx/templates/nginx.ingress.tmpl | 3 + internal/nginx/templates/templates_test.go | 1 + 11 files changed, 487 insertions(+), 185 deletions(-) diff --git a/internal/controller/controller.go b/internal/controller/controller.go index e4d550933d..94a50a0ffe 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -513,6 +513,9 @@ func (lbc *LoadBalancerController) syncIng(task queue.Task) { if utils.IsMaster(ing) { mergeableIngExs, err := lbc.createMergableIngresses(ing) if err != nil { + // we need to requeue because an error can occur even if the master is valid + // otherwise, we will not be able to generate the config until there is change + // in the master or minions. lbc.syncQueue.RequeueAfter(task, err, 5*time.Second) lbc.recorder.Eventf(ing, api_v1.EventTypeWarning, "Rejected", "%v was rejected: %v", key, err) if lbc.reportStatusEnabled() { @@ -550,7 +553,6 @@ func (lbc *LoadBalancerController) syncIng(task queue.Task) { } ingEx, err := lbc.createIngress(ing) if err != nil { - lbc.syncQueue.RequeueAfter(task, err, 5*time.Second) lbc.recorder.Eventf(ing, api_v1.EventTypeWarning, "Rejected", "%v was rejected: %v", key, err) if lbc.reportStatusEnabled() { err = lbc.statusUpdater.ClearIngressStatus(*ing) @@ -631,150 +633,177 @@ func (lbc *LoadBalancerController) syncSecret(task queue.Task) { return } - nonMinions, minions, err := lbc.findIngressesForSecret(namespace, 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) } - glog.V(2).Infof("Found %v Non-Minion and %v Minion Ingress resources with Secret %v", len(nonMinions), len(minions), key) + glog.V(2).Infof("Found %v Ingresses with Secret %v", len(ings), key) if !secrExists { glog.V(2).Infof("Deleting Secret: %v\n", key) - for _, minion := range minions { - master, err := lbc.FindMasterForMinion(&minion) - if err != nil { - glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err) - continue - } - mergeableIngress, err := lbc.createMergableIngresses(master) - if err != nil { - glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err) - continue - } - err = lbc.configurator.AddOrUpdateMergeableIngress(mergeableIngress) - if err != nil { - glog.Errorf("Failed to update Ingress %v(Master) of %v(Minion): %v", master.Name, minion.Name, err) - } - lbc.recorder.Eventf(&minion, api_v1.EventTypeWarning, "Rejected", "%v/%v was rejected due to deleted Secret %v: %v", minion.Namespace, minion.Name, key) - lbc.recorder.Eventf(master, api_v1.EventTypeWarning, "Rejected", "Minion %v/%v was rejected due to deleted Secret %v: %v", minion.Namespace, minion.Name, key) - lbc.syncQueue.Enqueue(&minion) - } - - if err := lbc.configurator.DeleteSecret(key, nonMinions); err != nil { - glog.Errorf("Error when deleting Secret: %v: %v", key, err) - } - - for _, ing := range nonMinions { - lbc.syncQueue.Enqueue(&ing) - lbc.recorder.Eventf(&ing, api_v1.EventTypeWarning, "Rejected", "%v/%v was rejected due to deleted Secret %v", ing.Namespace, ing.Name, key) - } + lbc.handleSecretDeletion(key, ings) if key == lbc.defaultServerSecret { glog.Warningf("The default server Secret %v was removed. Retaining the Secret.", key) } - } else { - glog.V(2).Infof("Adding / Updating Secret: %v\n", key) + return + } - secret := obj.(*api_v1.Secret) + glog.V(2).Infof("Adding / Updating Secret: %v\n", key) - if key == lbc.defaultServerSecret { - err := nginx.ValidateTLSSecret(secret) - if err != nil { - glog.Errorf("Couldn't validate the default server Secret %v: %v", key, err) - lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "Rejected", "the default server Secret %v was rejected, using the previous version: %v", key, err) - } else { - err := lbc.configurator.AddOrUpdateDefaultServerTLSSecret(secret) - if err != nil { - glog.Errorf("Error when updating the default server Secret %v: %v", key, err) - lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "UpdatedWithError", "the default server Secret %v was updated, but not applied: %v", key, err) + secret := obj.(*api_v1.Secret) - } else { - lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "the default server Secret %v was updated", key) - } + if key == lbc.defaultServerSecret { + lbc.handleDefaultSecretUpdate(secret) + // we don't return here in case the default secret is also used in Ingress resources + } + + if len(ings) > 0 { + lbc.handleSecretUpdate(secret, ings) + } +} + +func (lbc *LoadBalancerController) handleSecretDeletion(key string, ings []extensions.Ingress) { + eventType := api_v1.EventTypeWarning + title := "Missing Secret" + message := fmt.Sprintf("Secret %v was removed", key) + + lbc.emitEventForIngresses(eventType, title, message, ings) + + regular, mergeable := lbc.createIngresses(ings) + + eventType = api_v1.EventTypeNormal + title = "Updated" + message = fmt.Sprintf("Configuration was updated due to removed secret %v", key) + + if err := lbc.configurator.DeleteSecret(key, regular, mergeable); err != nil { + glog.Errorf("Error when deleting Secret: %v: %v", key, err) + + eventType = api_v1.EventTypeWarning + title = "UpdatedWithError" + message = fmt.Sprintf("Configuration was updated due to removed secret %v, but not applied: %v", key, err) + } + + lbc.emitEventForIngresses(eventType, title, message, ings) +} + +func (lbc *LoadBalancerController) handleSecretUpdate(secret *api_v1.Secret, ings []extensions.Ingress) { + secretNsName := secret.Namespace + "/" + secret.Name + + err := lbc.ValidateSecret(secret) + if err != nil { + // Secret becomes Invalid + glog.Errorf("Couldn't validate secret %v: %v", secretNsName, err) + glog.Errorf("Removing invalid secret %v", secretNsName) + + lbc.handleSecretDeletion(secretNsName, ings) + + lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "Rejected", "%v was rejected: %v", secretNsName, err) + return + } + + eventType := api_v1.EventTypeNormal + title := "Updated" + message := fmt.Sprintf("Configuration was updated due to updated secret %v", secretNsName) + + regular, mergeable := lbc.createIngresses(ings) + + if err := lbc.configurator.AddOrUpdateSecret(secret, regular, mergeable); err != nil { + glog.Errorf("Error when updating Secret %v: %v", secretNsName, err) + lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "UpdatedWithError", "%v was updated, but not applied: %v", secretNsName, err) + + eventType = api_v1.EventTypeWarning + title = "UpdatedWithError" + message = fmt.Sprintf("Configuration was updated due to updated secret %v, but not applied: %v", secretNsName, err) + } + + lbc.emitEventForIngresses(eventType, title, message, ings) +} + +func (lbc *LoadBalancerController) handleDefaultSecretUpdate(secret *api_v1.Secret) { + secretNsName := secret.Namespace + "/" + secret.Name + + err := nginx.ValidateTLSSecret(secret) + if err != nil { + glog.Errorf("Couldn't validate the default server Secret %v: %v", secretNsName, err) + lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "Rejected", "the default server Secret %v was rejected, using the previous version: %v", secretNsName, err) + return + } + + err = lbc.configurator.AddOrUpdateDefaultServerTLSSecret(secret) + if err != nil { + glog.Errorf("Error when updating the default server Secret %v: %v", secretNsName, err) + lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "UpdatedWithError", "the default server Secret %v was updated, but not applied: %v", secretNsName, err) + return + } + + lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "the default server Secret %v was updated", secretNsName) +} + +func (lbc *LoadBalancerController) emitEventForIngresses(eventType string, title string, message string, ings []extensions.Ingress) { + for _, ing := range ings { + lbc.recorder.Eventf(&ing, eventType, title, message) + if utils.IsMinion(&ing) { + master, err := lbc.FindMasterForMinion(&ing) + if err != nil { + glog.Errorf("Ignoring Ingress %v(Minion): %v", ing.Name, err) + continue } + masterMsg := fmt.Sprintf("%v for Minion %v/%v", message, ing.Namespace, ing.Name) + lbc.recorder.Eventf(master, eventType, title, masterMsg) } + } +} - if len(nonMinions) > 0 || len(minions) > 0 { - err := lbc.ValidateSecret(secret) +func (lbc *LoadBalancerController) createIngresses(ings []extensions.Ingress) (regular []nginx.IngressEx, mergeable []nginx.MergeableIngresses) { + for i := range ings { + if utils.IsMaster(&ings[i]) { + mergeableIng, err := lbc.createMergableIngresses(&ings[i]) if err != nil { - // Secret becomes Invalid - glog.Errorf("Couldn't validate secret %v: %v", key, err) - - for _, minion := range minions { - master, err := lbc.FindMasterForMinion(&minion) - if err != nil { - glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err) - continue - } - mergeableIngress, err := lbc.createMergableIngresses(master) - if err != nil { - glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err) - continue - } - err = lbc.configurator.AddOrUpdateMergeableIngress(mergeableIngress) - if err != nil { - glog.Errorf("Failed to update Ingress %v(Master) of %v(Minion): %v", master.Name, minion.Name, err) - } - lbc.recorder.Eventf(&minion, api_v1.EventTypeWarning, "Rejected", "%v/%v was rejected due to invalid Secret %v: %v", minion.Namespace, minion.Name, key, err) - lbc.recorder.Eventf(master, api_v1.EventTypeWarning, "Rejected", "Minion %v/%v was rejected due to invalid Secret %v: %v", minion.Namespace, minion.Name, key, err) - lbc.syncQueue.Enqueue(&minion) - } - - if err := lbc.configurator.DeleteSecret(key, nonMinions); err != nil { - glog.Errorf("Error when deleting Secret: %v: %v", key, err) - } - for _, ing := range nonMinions { - lbc.syncQueue.Enqueue(&ing) - lbc.recorder.Eventf(&ing, api_v1.EventTypeWarning, "Rejected", "%v/%v was rejected due to invalid Secret %v: %v", ing.Namespace, ing.Name, key, err) - } - lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "Rejected", "%v was rejected: %v", key, err) - return + glog.Errorf("Ignoring Ingress %v(Master): %v", ings[i].Name, err) + continue } + mergeable = append(mergeable, *mergeableIng) + continue + } - if err := lbc.configurator.AddOrUpdateSecret(secret); err != nil { - glog.Errorf("Error when updating Secret %v: %v", key, err) - lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "UpdatedWithError", "%v was updated, but not applied: %v", key, err) - for _, ing := range nonMinions { - lbc.recorder.Eventf(&ing, api_v1.EventTypeWarning, "UpdatedWithError", "Configuration for %v/%v was updated, but not applied: %v", ing.Namespace, ing.Name, err) - } - for _, minion := range minions { - master, err := lbc.FindMasterForMinion(&minion) - if err != nil { - glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err) - continue - } - lbc.recorder.Eventf(master, api_v1.EventTypeWarning, "UpdatedWithError", "Configuration for minion %v/%v was updated, but not applied: %v", minion.Namespace, minion.Name, err) - lbc.recorder.Eventf(&minion, api_v1.EventTypeWarning, "UpdatedWithError", "Configuration for %v/%v was updated, but not applied: %v", minion.Namespace, minion.Name, err) - } - } else { - lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "%v was updated", key) - for _, ing := range nonMinions { - lbc.recorder.Eventf(&ing, api_v1.EventTypeNormal, "Updated", "Configuration for %v/%v was updated", ing.Namespace, ing.Name) - } - for _, minion := range minions { - master, err := lbc.FindMasterForMinion(&minion) - if err != nil { - glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err) - continue - } - lbc.recorder.Eventf(master, api_v1.EventTypeNormal, "Updated", "Configuration for minion %v/%v was updated", minion.Namespace, minion.Name) - lbc.recorder.Eventf(&minion, api_v1.EventTypeNormal, "Updated", "Configuration for %v/%v was updated", minion.Namespace, minion.Name) - } + if utils.IsMinion(&ings[i]) { + master, err := lbc.FindMasterForMinion(&ings[i]) + if err != nil { + glog.Errorf("Ignoring Ingress %v(Minion): %v", ings[i].Name, err) + continue } + mergeableIng, err := lbc.createMergableIngresses(master) + if err != nil { + glog.Errorf("Ignoring Ingress %v(Master): %v", master, err) + continue + } + + mergeable = append(mergeable, *mergeableIng) + continue } + + ingEx, err := lbc.createIngress(&ings[i]) + if err != nil { + glog.Errorf("Ignoring Ingress %v/%v: $%v", ings[i].Namespace, ings[i].Name, err) + } + regular = append(regular, *ingEx) } + + return regular, mergeable } -func (lbc *LoadBalancerController) findIngressesForSecret(secretNamespace string, secretName string) (nonMinions []extensions.Ingress, minions []extensions.Ingress, error error) { - ings, err := lbc.ingressLister.List() +func (lbc *LoadBalancerController) findIngressesForSecret(secretNamespace string, secretName string) (ings []extensions.Ingress, error error) { + allIngs, err := lbc.ingressLister.List() if err != nil { - return nil, nil, fmt.Errorf("Couldn't get the list of Ingress resources: %v", err) + return nil, fmt.Errorf("Couldn't get the list of Ingress resources: %v", err) } items: - for _, ing := range ings.Items { + for _, ing := range allIngs.Items { if ing.Namespace != secretNamespace { continue } @@ -789,14 +818,14 @@ items: } for _, tls := range ing.Spec.TLS { if tls.SecretName == secretName { - nonMinions = append(nonMinions, ing) + ings = append(ings, ing) continue items } } if lbc.isNginxPlus { if jwtKey, exists := ing.Annotations[nginx.JWTKeyAnnotation]; exists { if jwtKey == secretName { - nonMinions = append(nonMinions, ing) + ings = append(ings, ing) } } } @@ -818,13 +847,13 @@ items: if jwtKey, exists := ing.Annotations[nginx.JWTKeyAnnotation]; exists { if jwtKey == secretName { - minions = append(minions, ing) + ings = append(ings, ing) } } } } - return nonMinions, minions, nil + return ings, nil } // EnqueueIngressForService enqueues the ingress for the given service @@ -886,16 +915,19 @@ func (lbc *LoadBalancerController) createIngress(ing *extensions.Ingress) (*ngin secretObject, secretExists, err := lbc.secretLister.GetByKey(secretKey) if err != nil { - return nil, fmt.Errorf("error getting secret %v from store: %v", secretKey, err) + glog.Warningf("Error retrieving secret %v for Ingress %v: %v", secretName, ing.Name, err) + continue } if !secretExists { - return nil, fmt.Errorf("secret %v not found for Ingress %v", secretKey, ing.Name) + glog.Warningf("secret %v not found for Ingress %v", secretKey, ing.Name) + continue } secret := secretObject.(*api_v1.Secret) err = nginx.ValidateTLSSecret(secret) if err != nil { - return nil, fmt.Errorf("Error validating secret %v for Ingress %v: %v", secretName, ing.Name, err) + glog.Warningf("Error validating secret %v for Ingress %v: %v", secretName, ing.Name, err) + continue } ingEx.TLSSecrets[secretName] = secret } @@ -906,15 +938,20 @@ func (lbc *LoadBalancerController) createIngress(ing *extensions.Ingress) (*ngin secret, err := lbc.client.Core().Secrets(ing.Namespace).Get(secretName, meta_v1.GetOptions{}) if err != nil { - return nil, fmt.Errorf("Error retrieving secret %v for Ingress %v: %v", secretName, ing.Name, err) + glog.Warningf("Error retrieving secret %v for Ingress %v: %v", secretName, ing.Name, err) + secret = nil + } else { + err = nginx.ValidateJWKSecret(secret) + if err != nil { + glog.Warningf("Error validating secret %v for Ingress %v: %v", secretName, ing.Name, err) + secret = nil + } } - err = nginx.ValidateJWKSecret(secret) - if err != nil { - return nil, fmt.Errorf("Error validating secret %v for Ingress %v: %v", secretName, ing.Name, err) + ingEx.JWTKey = nginx.JWTKey{ + Name: jwtKey, + Secret: secret, } - - ingEx.JWTKey = secret } } diff --git a/internal/controller/controller_test.go b/internal/controller/controller_test.go index f69cd75aa6..06205451c8 100644 --- a/internal/controller/controller_test.go +++ b/internal/controller/controller_test.go @@ -955,7 +955,7 @@ func TestFindIngressesForSecret(t *testing.T) { ngxIngress := &nginx.IngressEx{ Ingress: &test.ingress, TLSSecrets: map[string]*v1.Secret{ - test.secret.ObjectMeta.Name: &test.secret, + test.secret.Name: &test.secret, }, } @@ -967,28 +967,214 @@ func TestFindIngressesForSecret(t *testing.T) { lbc.ingressLister.Add(&test.ingress) lbc.secretLister.Add(&test.secret) - nonMinions, minions, err := lbc.findIngressesForSecret(test.secret.ObjectMeta.Namespace, test.secret.ObjectMeta.Name) + ings, err := lbc.findIngressesForSecret(test.secret.Namespace, test.secret.Name) if err != nil { t.Fatalf("Couldn't find Ingress resource: %v", err) } - if len(minions) > 0 { - t.Fatalf("Expected 0 minions. Got: %v", len(minions)) + if len(ings) > 0 { + if !test.expectedToFind { + t.Fatalf("Expected 0 ingresses. Got: %v", len(ings)) + } + if len(ings) != 1 { + t.Fatalf("Expected 1 ingress. Got: %v", len(ings)) + } + if ings[0].Name != test.ingress.Name || ings[0].Namespace != test.ingress.Namespace { + t.Fatalf("Expected: %v/%v. Got: %v/%v.", test.ingress.Namespace, test.ingress.Name, ings[0].Namespace, ings[0].Name) + } + } else if test.expectedToFind { + t.Fatal("Expected 1 ingress. Got: 0") } + }) + } +} + +func TestFindIngressesForSecretWithMinions(t *testing.T) { + testCases := []struct { + secret v1.Secret + ingress extensions.Ingress + expectedToFind bool + desc string + }{ + { + secret: v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "my-jwk-secret", + Namespace: "default", + }, + }, + ingress: extensions.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "cafe-ingress-tea-minion", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + "nginx.org/mergeable-ingress-type": "minion", + nginx.JWTKeyAnnotation: "my-jwk-secret", + }, + }, + Spec: extensions.IngressSpec{ + Rules: []extensions.IngressRule{ + extensions.IngressRule{ + Host: "cafe.example.com", + IngressRuleValue: extensions.IngressRuleValue{ + HTTP: &extensions.HTTPIngressRuleValue{ + Paths: []extensions.HTTPIngressPath{ + extensions.HTTPIngressPath{ + Path: "/tea", + Backend: extensions.IngressBackend{ + ServiceName: "tea-svc", + ServicePort: intstr.FromString("80"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedToFind: true, + desc: "a minion 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: "cafe-ingress-tea-minion", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + "nginx.org/mergeable-ingress-type": "minion", + nginx.JWTKeyAnnotation: "my-jwk-secret", + }, + }, + Spec: extensions.IngressSpec{ + Rules: []extensions.IngressRule{ + extensions.IngressRule{ + Host: "cafe.example.com", + IngressRuleValue: extensions.IngressRuleValue{ + HTTP: &extensions.HTTPIngressRuleValue{ + Paths: []extensions.HTTPIngressPath{ + extensions.HTTPIngressPath{ + Path: "/tea", + Backend: extensions.IngressBackend{ + ServiceName: "tea-svc", + ServicePort: intstr.FromString("80"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedToFind: false, + desc: "a Minion references a JWK secret that exists in a different namespace", + }, + } - if len(nonMinions) > 0 { + master := extensions.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "cafe-ingress-master", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + "nginx.org/mergeable-ingress-type": "master", + }, + }, + Spec: extensions.IngressSpec{ + Rules: []extensions.IngressRule{ + extensions.IngressRule{ + Host: "cafe.example.com", + IngressRuleValue: extensions.IngressRuleValue{ + HTTP: &extensions.HTTPIngressRuleValue{ // HTTP must not be nil for Master + Paths: []extensions.HTTPIngressPath{}, + }, + }, + }, + }, + }, + } + + for _, test := range testCases { + t.Run(test.desc, func(t *testing.T) { + fakeClient := fake.NewSimpleClientset() + + templateExecutor, err := nginx.NewTemplateExecutor("../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl", true, true, []string{"127.0.0.1"}, 8080) + if err != nil { + t.Fatalf("templateExecuter could not start: %v", err) + } + ngxc := nginx.NewNginxController("/etc/nginx", true) + apiCtrl, err := plus.NewNginxAPIController(&http.Client{}, "", true) + if err != nil { + t.Fatalf("NGINX API Controller could not start: %v", err) + } + + cnf := nginx.NewConfigurator(ngxc, &nginx.Config{}, apiCtrl, templateExecutor) + lbc := LoadBalancerController{ + client: fakeClient, + ingressClass: "nginx", + configurator: cnf, + isNginxPlus: true, + } + + lbc.ingressLister.Store, _ = cache.NewInformer( + cache.NewListWatchFromClient(lbc.client.ExtensionsV1beta1().RESTClient(), "ingresses", "default", fields.Everything()), + &extensions.Ingress{}, time.Duration(1), nil) + + lbc.secretLister.Store, lbc.secretController = cache.NewInformer( + cache.NewListWatchFromClient(lbc.client.Core().RESTClient(), "secrets", "default", fields.Everything()), + &v1.Secret{}, time.Duration(1), nil) + + mergeable := &nginx.MergeableIngresses{ + Master: &nginx.IngressEx{ + Ingress: &master, + }, + Minions: []*nginx.IngressEx{ + &nginx.IngressEx{ + Ingress: &test.ingress, + JWTKey: nginx.JWTKey{ + Name: test.secret.Name, + }, + }, + }, + } + + err = cnf.AddOrUpdateMergeableIngress(mergeable) + if err != nil { + t.Fatalf("Ingress was not added: %v", err) + } + + lbc.ingressLister.Add(&master) + lbc.ingressLister.Add(&test.ingress) + lbc.secretLister.Add(&test.secret) + + ings, err := lbc.findIngressesForSecret(test.secret.Namespace, test.secret.Name) + if err != nil { + t.Fatalf("Couldn't find Ingress resource: %v", err) + } + + if len(ings) > 0 { if !test.expectedToFind { - t.Fatalf("Expected 0 non-Minions. Got: %v", len(nonMinions)) + t.Fatalf("Expected 0 ingresses. Got: %v", len(ings)) } - if len(nonMinions) != 1 { - t.Fatalf("Expected 1 non-Minion. Got: %v", len(nonMinions)) + if len(ings) != 1 { + t.Fatalf("Expected 1 ingress. Got: %v", len(ings)) } - if nonMinions[0].Name != test.ingress.ObjectMeta.Name || nonMinions[0].Namespace != test.ingress.ObjectMeta.Namespace { - t.Fatalf("Expected: %v/%v. Got: %v/%v.", test.ingress.ObjectMeta.Namespace, test.ingress.ObjectMeta.Name, nonMinions[0].Namespace, nonMinions[0].Name) + if ings[0].Name != test.ingress.Name || ings[0].Namespace != test.ingress.Namespace { + t.Fatalf("Expected: %v/%v. Got: %v/%v.", test.ingress.Namespace, test.ingress.Name, ings[0].Namespace, ings[0].Name) } } else if test.expectedToFind { - t.Fatal("Expected 1 non-Minion. Got: 0") + t.Fatal("Expected 1 ingress. Got: 0") } }) } + } diff --git a/internal/handlers/secret.go b/internal/handlers/secret.go index b0972ff727..74d0f72012 100644 --- a/internal/handlers/secret.go +++ b/internal/handlers/secret.go @@ -17,11 +17,8 @@ func CreateSecretHandlers(lbc *controller.LoadBalancerController) cache.Resource if err := lbc.ValidateSecret(secret); err != nil { return } - nsname := secret.Namespace + "/" + secret.Name - if nsname == lbc.GetDefaultServerSecret() { - glog.V(3).Infof("Adding default server Secret: %v", secret.Name) - lbc.AddSyncQueue(obj) - } + glog.V(3).Infof("Adding Secret: %v", secret.Name) + lbc.AddSyncQueue(obj) }, DeleteFunc: func(obj interface{}) { secret, isSecr := obj.(*api_v1.Secret) diff --git a/internal/nginx/configurator.go b/internal/nginx/configurator.go index d425cb8a15..f3a8e44932 100644 --- a/internal/nginx/configurator.go +++ b/internal/nginx/configurator.go @@ -17,11 +17,13 @@ import ( const emptyHost = "" +const pemFileNameForMissingTLSSecret = "/etc/nginx/secrets/default" + // DefaultServerSecretName is the filename of the Secret with a TLS cert and a key for the default server const DefaultServerSecretName = "default" -// JWTKey is the key of the data field of a Secret where the JWK must be stored. -const JWTKey = "jwk" +// JWTKeyKey is the key of the data field of a Secret where the JWK must be stored. +const JWTKeyKey = "jwk" // JWTKeyAnnotation is the annotation where the Secret with a JWK is specified. const JWTKeyAnnotation = "nginx.com/jwt-key" @@ -67,9 +69,10 @@ func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) error { } func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) error { - pems, jwtKeyFileName := cnf.updateSecrets(ingEx) + pems := cnf.updateTLSSecrets(ingEx) + cnf.updateJWTSecret(ingEx.JWTKey) isMinion := false - nginxCfg := cnf.generateNginxCfg(ingEx, pems, jwtKeyFileName, isMinion) + nginxCfg := cnf.generateNginxCfg(ingEx, pems, isMinion) name := objectMetaToFileName(&ingEx.Ingress.ObjectMeta) content, err := cnf.templateExecutor.ExecuteIngressConfigTemplate(&nginxCfg) if err != nil { @@ -122,9 +125,11 @@ func (cnf *Configurator) generateNginxCfgForMergeableIngresses(mergeableIngs *Me mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, strings.Join(removedAnnotations, ",")) } - pems, jwtKeyFileName := cnf.updateSecrets(mergeableIngs.Master) + pems := cnf.updateTLSSecrets(mergeableIngs.Master) + cnf.updateJWTSecret(mergeableIngs.Master.JWTKey) + isMinion := false - masterNginxCfg := cnf.generateNginxCfg(mergeableIngs.Master, pems, jwtKeyFileName, isMinion) + masterNginxCfg := cnf.generateNginxCfg(mergeableIngs.Master, pems, isMinion) masterServer = masterNginxCfg.Servers[0] masterServer.Locations = []Location{} @@ -150,9 +155,10 @@ func (cnf *Configurator) generateNginxCfgForMergeableIngresses(mergeableIngs *Me minion.Ingress.Namespace, minion.Ingress.Name, strings.Join(removedAnnotations, ",")) } - pems, jwtKeyFileName := cnf.updateSecrets(minion) + pems := cnf.updateTLSSecrets(minion) + cnf.updateJWTSecret(minion.JWTKey) isMinion := true - nginxCfg := cnf.generateNginxCfg(minion, pems, jwtKeyFileName, isMinion) + nginxCfg := cnf.generateNginxCfg(minion, pems, isMinion) for _, server := range nginxCfg.Servers { for _, loc := range server.Locations { @@ -181,13 +187,16 @@ func (cnf *Configurator) generateNginxCfgForMergeableIngresses(mergeableIngs *Me } } -func (cnf *Configurator) updateSecrets(ingEx *IngressEx) (map[string]string, string) { +func (cnf *Configurator) updateTLSSecrets(ingEx *IngressEx) map[string]string { pems := make(map[string]string) for _, tls := range ingEx.Ingress.Spec.TLS { secretName := tls.SecretName - pemFileName := cnf.addOrUpdateSecret(ingEx.TLSSecrets[secretName]) + pemFileName := pemFileNameForMissingTLSSecret + if secret, exists := ingEx.TLSSecrets[secretName]; exists { + pemFileName = cnf.addOrUpdateSecret(secret) + } for _, host := range tls.Hosts { pems[host] = pemFileName @@ -197,16 +206,16 @@ func (cnf *Configurator) updateSecrets(ingEx *IngressEx) (map[string]string, str } } - jwtKeyFileName := "" + return pems +} - if cnf.isPlus() && ingEx.JWTKey != nil { - jwtKeyFileName = cnf.addOrUpdateSecret(ingEx.JWTKey) +func (cnf *Configurator) updateJWTSecret(jwtKey JWTKey) { + if cnf.isPlus() && jwtKey.Secret != nil { + cnf.addOrUpdateSecret(jwtKey.Secret) } - - return pems, jwtKeyFileName } -func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]string, jwtKeyFileName string, isMinion bool) IngressNginxConfig { +func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]string, isMinion bool) IngressNginxConfig { ingCfg := cnf.createConfig(ingEx) upstreams := make(map[string]Upstream) @@ -272,9 +281,14 @@ func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]stri server.SSL = true server.SSLCertificate = pemFile server.SSLCertificateKey = pemFile + if pemFile == pemFileNameForMissingTLSSecret { + server.SSLCiphers = "NULL" + } } - if !isMinion && jwtKeyFileName != "" { + if !isMinion && ingEx.JWTKey.Name != "" { + jwtKeyFileName := cnf.nginx.getSecretFileName(ingEx.Ingress.Namespace + "-" + ingEx.JWTKey.Name) + server.JWTAuth = &JWTAuth{ Key: jwtKeyFileName, Realm: ingCfg.JWTRealm, @@ -323,7 +337,9 @@ func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]stri loc := createLocation(pathOrDefault(path.Path), upstreams[upsName], &ingCfg, wsServices[path.Backend.ServiceName], rewrites[path.Backend.ServiceName], sslServices[path.Backend.ServiceName], grpcServices[path.Backend.ServiceName]) - if isMinion && jwtKeyFileName != "" { + if isMinion && ingEx.JWTKey.Name != "" { + jwtKeyFileName := cnf.nginx.getSecretFileName(ingEx.Ingress.Namespace + "-" + ingEx.JWTKey.Name) + loc.JWTAuth = &JWTAuth{ Key: jwtKeyFileName, Realm: ingCfg.JWTRealm, @@ -881,7 +897,7 @@ func upstreamMapToSlice(upstreams map[string]Upstream) []Upstream { } // AddOrUpdateSecret creates or updates a file with the content of the secret -func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) error { +func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret, ingExes []IngressEx, mergeableIngresses []MergeableIngresses) error { cnf.addOrUpdateSecret(secret) kind, _ := GetSecretKind(secret) @@ -889,6 +905,20 @@ func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) error { return nil } + for i := range ingExes { + err := cnf.addOrUpdateIngress(&ingExes[i]) + if err != nil { + return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingExes[i].Ingress.Namespace, ingExes[i].Ingress.Name, err) + } + } + + for i := range mergeableIngresses { + err := cnf.addOrUpdateMergeableIngress(&mergeableIngresses[i]) + if err != nil { + return fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", mergeableIngresses[i].Master.Ingress.Namespace, mergeableIngresses[i].Master.Ingress.Name, err) + } + } + if err := cnf.nginx.Reload(); err != nil { return fmt.Errorf("Error when reloading NGINX when updating Secret: %v", err) } @@ -904,7 +934,7 @@ func (cnf *Configurator) addOrUpdateSecret(secret *api_v1.Secret) string { kind, _ := GetSecretKind(secret) if cnf.isPlus() && kind == JWK { mode = jwkSecretFileMode - data = []byte(secret.Data[JWTKey]) + data = []byte(secret.Data[JWTKeyKey]) } else { mode = TLSSecretFileMode data = GenerateCertAndKeyFileContent(secret) @@ -935,17 +965,24 @@ func GenerateCertAndKeyFileContent(secret *api_v1.Secret) []byte { } // DeleteSecret deletes the file associated with the secret and the configuration files for the Ingress resources. NGINX is reloaded only when len(ings) > 0 -func (cnf *Configurator) DeleteSecret(key string, ings []extensions.Ingress) error { - for _, ing := range ings { - name := objectMetaToFileName(&ing.ObjectMeta) - cnf.nginx.DeleteIngress(name) - delete(cnf.ingresses, name) - delete(cnf.minions, name) +func (cnf *Configurator) DeleteSecret(key string, ingExes []IngressEx, mergeableIngresses []MergeableIngresses) error { + cnf.nginx.DeleteSecretFile(keyToFileName(key)) + + for i := range ingExes { + err := cnf.addOrUpdateIngress(&ingExes[i]) + if err != nil { + return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingExes[i].Ingress.Namespace, ingExes[i].Ingress.Name, err) + } } - cnf.nginx.DeleteSecretFile(keyToFileName(key)) + for i := range mergeableIngresses { + err := cnf.addOrUpdateMergeableIngress(&mergeableIngresses[i]) + if err != nil { + return fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", mergeableIngresses[i].Master.Ingress.Namespace, mergeableIngresses[i].Master.Ingress.Name, err) + } + } - if len(ings) > 0 { + if len(ingExes)+len(mergeableIngresses) > 0 { if err := cnf.nginx.Reload(); err != nil { return fmt.Errorf("Error when reloading NGINX when deleting Secret %v: %v", key, err) } diff --git a/internal/nginx/configurator_test.go b/internal/nginx/configurator_test.go index 26b8ad04bd..ef2cf877b6 100644 --- a/internal/nginx/configurator_test.go +++ b/internal/nginx/configurator_test.go @@ -562,7 +562,7 @@ func TestGenerateNginxCfg(t *testing.T) { "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", } - result := cnf.generateNginxCfg(&cafeIngressEx, pems, "", false) + result := cnf.generateNginxCfg(&cafeIngressEx, pems, false) if !reflect.DeepEqual(result, expected) { t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", result, expected) @@ -576,7 +576,12 @@ func TestGenerateNginxCfgForJWT(t *testing.T) { cafeIngressEx.Ingress.Annotations["nginx.com/jwt-realm"] = "Cafe App" cafeIngressEx.Ingress.Annotations["nginx.com/jwt-token"] = "$cookie_auth_token" cafeIngressEx.Ingress.Annotations["nginx.com/jwt-login-url"] = "https://login.example.com" - cafeIngressEx.JWTKey = &api_v1.Secret{} + cafeIngressEx.JWTKey = JWTKey{"cafe-jwk", &api_v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "cafe-jwk", + Namespace: "default", + }, + }} cnf, err := createTestConfigurator() if err != nil { @@ -600,7 +605,7 @@ func TestGenerateNginxCfgForJWT(t *testing.T) { "cafe.example.com": "/etc/nginx/secrets/default-cafe-secret", } - result := cnf.generateNginxCfg(&cafeIngressEx, pems, "/etc/nginx/secrets/default-cafe-jwk", false) + result := cnf.generateNginxCfg(&cafeIngressEx, pems, false) if !reflect.DeepEqual(result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) { t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) @@ -632,10 +637,13 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { mergeableIngresses.Master.Ingress.Annotations["nginx.com/jwt-realm"] = "Cafe" mergeableIngresses.Master.Ingress.Annotations["nginx.com/jwt-token"] = "$cookie_auth_token" mergeableIngresses.Master.Ingress.Annotations["nginx.com/jwt-login-url"] = "https://login.example.com" - mergeableIngresses.Master.JWTKey = &api_v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "cafe-jwk", - Namespace: "default", + mergeableIngresses.Master.JWTKey = JWTKey{ + "cafe-jwk", + &api_v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "cafe-jwk", + Namespace: "default", + }, }, } @@ -643,10 +651,13 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { mergeableIngresses.Minions[0].Ingress.Annotations["nginx.com/jwt-realm"] = "Coffee" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.com/jwt-token"] = "$cookie_auth_token_coffee" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.com/jwt-login-url"] = "https://login.cofee.example.com" - mergeableIngresses.Minions[0].JWTKey = &api_v1.Secret{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "coffee-jwk", - Namespace: "default", + mergeableIngresses.Minions[0].JWTKey = JWTKey{ + "coffee-jwk", + &api_v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "coffee-jwk", + Namespace: "default", + }, }, } @@ -692,6 +703,26 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { } } +func TestGenerateNginxCfgWithMissingTLSSecret(t *testing.T) { + cafeIngressEx := createCafeIngressEx() + cnf, err := createTestConfigurator() + if err != nil { + t.Errorf("Failed to create a test configurator: %v", err) + } + + pems := map[string]string{ + "cafe.example.com": pemFileNameForMissingTLSSecret, + } + + result := cnf.generateNginxCfg(&cafeIngressEx, pems, false) + + expectedCiphers := "NULL" + resultCiphers := result.Servers[0].SSLCiphers + if !reflect.DeepEqual(resultCiphers, expectedCiphers) { + t.Errorf("generateNginxCfg returned SSLCiphers %v, but expected %v", resultCiphers, expectedCiphers) + } +} + func TestAddOrUpdateIngress(t *testing.T) { cnf, err := createTestConfigurator() if err != nil { diff --git a/internal/nginx/ingress.go b/internal/nginx/ingress.go index ea188b3759..72ca569c94 100644 --- a/internal/nginx/ingress.go +++ b/internal/nginx/ingress.go @@ -12,7 +12,7 @@ import ( type IngressEx struct { Ingress *extensions.Ingress TLSSecrets map[string]*api_v1.Secret - JWTKey *api_v1.Secret + JWTKey JWTKey Endpoints map[string][]string HealthChecks map[string]*api_v1.Probe } @@ -23,6 +23,12 @@ type MergeableIngresses struct { Minions []*IngressEx } +// JWTKey represents a secret that holds JSON Web Key +type JWTKey struct { + Name string + Secret *api_v1.Secret +} + var masterBlacklist = map[string]bool{ "nginx.org/rewrites": true, "nginx.org/ssl-services": true, diff --git a/internal/nginx/nginx.go b/internal/nginx/nginx.go index 428ae0444d..304fc1d955 100644 --- a/internal/nginx/nginx.go +++ b/internal/nginx/nginx.go @@ -84,6 +84,7 @@ type Server struct { SSL bool SSLCertificate string SSLCertificateKey string + SSLCiphers string GRPCOnly bool StatusZone string HTTP2 bool diff --git a/internal/nginx/secret.go b/internal/nginx/secret.go index 4e1ac76e13..1329eeac1d 100644 --- a/internal/nginx/secret.go +++ b/internal/nginx/secret.go @@ -28,8 +28,8 @@ func ValidateTLSSecret(secret *api_v1.Secret) error { // ValidateJWKSecret validates the secret. If it is valid, the function returns nil. func ValidateJWKSecret(secret *api_v1.Secret) error { - if _, exists := secret.Data[JWTKey]; !exists { - return fmt.Errorf("Secret doesn't have %v", JWTKey) + if _, exists := secret.Data[JWTKeyKey]; !exists { + return fmt.Errorf("Secret doesn't have %v", JWTKeyKey) } return nil diff --git a/internal/nginx/templates/nginx-plus.ingress.tmpl b/internal/nginx/templates/nginx-plus.ingress.tmpl index 599abbca2b..b769df7e76 100644 --- a/internal/nginx/templates/nginx-plus.ingress.tmpl +++ b/internal/nginx/templates/nginx-plus.ingress.tmpl @@ -31,6 +31,9 @@ server { {{- end}} ssl_certificate {{$server.SSLCertificate}}; ssl_certificate_key {{$server.SSLCertificateKey}}; + {{if $server.SSLCiphers}} + ssl_ciphers {{$server.SSLCiphers}}; + {{end}} {{end}} {{range $setRealIPFrom := $server.SetRealIPFrom}} set_real_ip_from {{$setRealIPFrom}};{{end}} diff --git a/internal/nginx/templates/nginx.ingress.tmpl b/internal/nginx/templates/nginx.ingress.tmpl index 7a0c8c8cf8..cf0ebffae8 100644 --- a/internal/nginx/templates/nginx.ingress.tmpl +++ b/internal/nginx/templates/nginx.ingress.tmpl @@ -20,6 +20,9 @@ server { {{- end}} ssl_certificate {{$server.SSLCertificate}}; ssl_certificate_key {{$server.SSLCertificateKey}}; + {{if $server.SSLCiphers}} + ssl_ciphers {{$server.SSLCiphers}}; + {{end}} {{end}} {{range $setRealIPFrom := $server.SetRealIPFrom}} set_real_ip_from {{$setRealIPFrom}};{{end}} diff --git a/internal/nginx/templates/templates_test.go b/internal/nginx/templates/templates_test.go index 6da5c310c0..4d3dcaa7d7 100644 --- a/internal/nginx/templates/templates_test.go +++ b/internal/nginx/templates/templates_test.go @@ -51,6 +51,7 @@ var ingCfg = nginx.IngressNginxConfig{ SSL: true, SSLCertificate: "secret.pem", SSLCertificateKey: "secret.pem", + SSLCiphers: "NULL", SSLPorts: []int{443}, SSLRedirect: true, Locations: []nginx.Location{