Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix find ingress for secret ns bug #326

Merged
merged 2 commits into from
Aug 1, 2018
Merged

Fix find ingress for secret ns bug #326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1a6eab
Fix namespace check for findIngressForSecret
Dean-Coakley Jul 23, 2018
38065b7
Improve TestFindIngressesForSecret
pleshakov Jul 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions nginx-controller/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -848,21 +848,25 @@ func (lbc *LoadBalancerController) findIngressesForSecret(secret string) ([]exte

items:
for _, ing := range ings.Items {
if ing.Namespace != secretNamespace {
continue
}

if !lbc.isNginxIngress(&ing) {
continue
}
if !lbc.cnf.HasIngress(&ing) {
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)
}
}
Expand Down
161 changes: 161 additions & 0 deletions nginx-controller/controller/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controller

import (
"fmt"
"net/http"
"reflect"
"testing"
"time"
Expand Down Expand Up @@ -827,3 +828,163 @@ func TestGetServicePortForIngressPort(t *testing.T) {
t.Errorf("Mismatched strings should not return port: %+v", svcPort)
}
}

func TestFindIngressesForSecret(t *testing.T) {
testCases := []struct {
secret v1.Secret
ingress extensions.Ingress
expectedToFind bool
desc string
}{
{
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-1",
},
Spec: extensions.IngressSpec{
TLS: []extensions.IngressTLS{
extensions.IngressTLS{
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: "my-ingress",
Namespace: "namespace-2",
Annotations: map[string]string{
nginx.JWTKeyAnnotation: "my-jwk-secret",
},
},
},
expectedToFind: false,
desc: "an Ingress references a JWK secret that exists in a different namespace",
},
}

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)
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,
},
}

err = cnf.AddOrUpdateIngress(ngxIngress)
if err != nil {
t.Errorf("Ingress was not added: %v", err)
}

lbc.ingLister.Add(&test.ingress)
lbc.secrLister.Add(&test.secret)

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.expectedToFind {
t.Errorf("Expected no ingresses. Got: %v/%v", 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.expectedToFind {
t.Errorf("Expected %v/%v. Got: No ingresses. %v", test.ingress.ObjectMeta.Namespace, test.ingress.ObjectMeta.Name, err)
}
})
}
}