Skip to content

Commit 7596444

Browse files
pleshakovpleshakov
committed
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).
1 parent ebb2a51 commit 7596444

File tree

11 files changed

+487
-185
lines changed

11 files changed

+487
-185
lines changed

‎internal/controller/controller.go

Lines changed: 164 additions & 127 deletions
Large diffs are not rendered by default.

‎internal/controller/controller_test.go

Lines changed: 197 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -955,7 +955,7 @@ func TestFindIngressesForSecret(t *testing.T) {
955955
ngxIngress := &nginx.IngressEx{
956956
Ingress: &test.ingress,
957957
TLSSecrets: map[string]*v1.Secret{
958-
test.secret.ObjectMeta.Name: &test.secret,
958+
test.secret.Name: &test.secret,
959959
},
960960
}
961961

@@ -967,28 +967,214 @@ func TestFindIngressesForSecret(t *testing.T) {
967967
lbc.ingressLister.Add(&test.ingress)
968968
lbc.secretLister.Add(&test.secret)
969969

970-
nonMinions, minions, err := lbc.findIngressesForSecret(test.secret.ObjectMeta.Namespace, test.secret.ObjectMeta.Name)
970+
ings, err := lbc.findIngressesForSecret(test.secret.Namespace, test.secret.Name)
971971
if err != nil {
972972
t.Fatalf("Couldn't find Ingress resource: %v", err)
973973
}
974974

975-
if len(minions) > 0 {
976-
t.Fatalf("Expected 0 minions. Got: %v", len(minions))
975+
if len(ings) > 0 {
976+
if !test.expectedToFind {
977+
t.Fatalf("Expected 0 ingresses. Got: %v", len(ings))
978+
}
979+
if len(ings) != 1 {
980+
t.Fatalf("Expected 1 ingress. Got: %v", len(ings))
981+
}
982+
if ings[0].Name != test.ingress.Name || ings[0].Namespace != test.ingress.Namespace {
983+
t.Fatalf("Expected: %v/%v. Got: %v/%v.", test.ingress.Namespace, test.ingress.Name, ings[0].Namespace, ings[0].Name)
984+
}
985+
} else if test.expectedToFind {
986+
t.Fatal("Expected 1 ingress. Got: 0")
977987
}
988+
})
989+
}
990+
}
991+
992+
func TestFindIngressesForSecretWithMinions(t *testing.T) {
993+
testCases := []struct {
994+
secret v1.Secret
995+
ingress extensions.Ingress
996+
expectedToFind bool
997+
desc string
998+
}{
999+
{
1000+
secret: v1.Secret{
1001+
ObjectMeta: meta_v1.ObjectMeta{
1002+
Name: "my-jwk-secret",
1003+
Namespace: "default",
1004+
},
1005+
},
1006+
ingress: extensions.Ingress{
1007+
ObjectMeta: meta_v1.ObjectMeta{
1008+
Name: "cafe-ingress-tea-minion",
1009+
Namespace: "default",
1010+
Annotations: map[string]string{
1011+
"kubernetes.io/ingress.class": "nginx",
1012+
"nginx.org/mergeable-ingress-type": "minion",
1013+
nginx.JWTKeyAnnotation: "my-jwk-secret",
1014+
},
1015+
},
1016+
Spec: extensions.IngressSpec{
1017+
Rules: []extensions.IngressRule{
1018+
extensions.IngressRule{
1019+
Host: "cafe.example.com",
1020+
IngressRuleValue: extensions.IngressRuleValue{
1021+
HTTP: &extensions.HTTPIngressRuleValue{
1022+
Paths: []extensions.HTTPIngressPath{
1023+
extensions.HTTPIngressPath{
1024+
Path: "/tea",
1025+
Backend: extensions.IngressBackend{
1026+
ServiceName: "tea-svc",
1027+
ServicePort: intstr.FromString("80"),
1028+
},
1029+
},
1030+
},
1031+
},
1032+
},
1033+
},
1034+
},
1035+
},
1036+
},
1037+
expectedToFind: true,
1038+
desc: "a minion Ingress references a JWK Secret that exists in the Ingress namespace",
1039+
},
1040+
{
1041+
secret: v1.Secret{
1042+
ObjectMeta: meta_v1.ObjectMeta{
1043+
Name: "my-jwk-secret",
1044+
Namespace: "namespace-1",
1045+
},
1046+
},
1047+
ingress: extensions.Ingress{
1048+
ObjectMeta: meta_v1.ObjectMeta{
1049+
Name: "cafe-ingress-tea-minion",
1050+
Namespace: "default",
1051+
Annotations: map[string]string{
1052+
"kubernetes.io/ingress.class": "nginx",
1053+
"nginx.org/mergeable-ingress-type": "minion",
1054+
nginx.JWTKeyAnnotation: "my-jwk-secret",
1055+
},
1056+
},
1057+
Spec: extensions.IngressSpec{
1058+
Rules: []extensions.IngressRule{
1059+
extensions.IngressRule{
1060+
Host: "cafe.example.com",
1061+
IngressRuleValue: extensions.IngressRuleValue{
1062+
HTTP: &extensions.HTTPIngressRuleValue{
1063+
Paths: []extensions.HTTPIngressPath{
1064+
extensions.HTTPIngressPath{
1065+
Path: "/tea",
1066+
Backend: extensions.IngressBackend{
1067+
ServiceName: "tea-svc",
1068+
ServicePort: intstr.FromString("80"),
1069+
},
1070+
},
1071+
},
1072+
},
1073+
},
1074+
},
1075+
},
1076+
},
1077+
},
1078+
expectedToFind: false,
1079+
desc: "a Minion references a JWK secret that exists in a different namespace",
1080+
},
1081+
}
9781082

979-
if len(nonMinions) > 0 {
1083+
master := extensions.Ingress{
1084+
ObjectMeta: meta_v1.ObjectMeta{
1085+
Name: "cafe-ingress-master",
1086+
Namespace: "default",
1087+
Annotations: map[string]string{
1088+
"kubernetes.io/ingress.class": "nginx",
1089+
"nginx.org/mergeable-ingress-type": "master",
1090+
},
1091+
},
1092+
Spec: extensions.IngressSpec{
1093+
Rules: []extensions.IngressRule{
1094+
extensions.IngressRule{
1095+
Host: "cafe.example.com",
1096+
IngressRuleValue: extensions.IngressRuleValue{
1097+
HTTP: &extensions.HTTPIngressRuleValue{ // HTTP must not be nil for Master
1098+
Paths: []extensions.HTTPIngressPath{},
1099+
},
1100+
},
1101+
},
1102+
},
1103+
},
1104+
}
1105+
1106+
for _, test := range testCases {
1107+
t.Run(test.desc, func(t *testing.T) {
1108+
fakeClient := fake.NewSimpleClientset()
1109+
1110+
templateExecutor, err := nginx.NewTemplateExecutor("../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl", true, true, []string{"127.0.0.1"}, 8080)
1111+
if err != nil {
1112+
t.Fatalf("templateExecuter could not start: %v", err)
1113+
}
1114+
ngxc := nginx.NewNginxController("/etc/nginx", true)
1115+
apiCtrl, err := plus.NewNginxAPIController(&http.Client{}, "", true)
1116+
if err != nil {
1117+
t.Fatalf("NGINX API Controller could not start: %v", err)
1118+
}
1119+
1120+
cnf := nginx.NewConfigurator(ngxc, &nginx.Config{}, apiCtrl, templateExecutor)
1121+
lbc := LoadBalancerController{
1122+
client: fakeClient,
1123+
ingressClass: "nginx",
1124+
configurator: cnf,
1125+
isNginxPlus: true,
1126+
}
1127+
1128+
lbc.ingressLister.Store, _ = cache.NewInformer(
1129+
cache.NewListWatchFromClient(lbc.client.ExtensionsV1beta1().RESTClient(), "ingresses", "default", fields.Everything()),
1130+
&extensions.Ingress{}, time.Duration(1), nil)
1131+
1132+
lbc.secretLister.Store, lbc.secretController = cache.NewInformer(
1133+
cache.NewListWatchFromClient(lbc.client.Core().RESTClient(), "secrets", "default", fields.Everything()),
1134+
&v1.Secret{}, time.Duration(1), nil)
1135+
1136+
mergeable := &nginx.MergeableIngresses{
1137+
Master: &nginx.IngressEx{
1138+
Ingress: &master,
1139+
},
1140+
Minions: []*nginx.IngressEx{
1141+
&nginx.IngressEx{
1142+
Ingress: &test.ingress,
1143+
JWTKey: nginx.JWTKey{
1144+
Name: test.secret.Name,
1145+
},
1146+
},
1147+
},
1148+
}
1149+
1150+
err = cnf.AddOrUpdateMergeableIngress(mergeable)
1151+
if err != nil {
1152+
t.Fatalf("Ingress was not added: %v", err)
1153+
}
1154+
1155+
lbc.ingressLister.Add(&master)
1156+
lbc.ingressLister.Add(&test.ingress)
1157+
lbc.secretLister.Add(&test.secret)
1158+
1159+
ings, err := lbc.findIngressesForSecret(test.secret.Namespace, test.secret.Name)
1160+
if err != nil {
1161+
t.Fatalf("Couldn't find Ingress resource: %v", err)
1162+
}
1163+
1164+
if len(ings) > 0 {
9801165
if !test.expectedToFind {
981-
t.Fatalf("Expected 0 non-Minions. Got: %v", len(nonMinions))
1166+
t.Fatalf("Expected 0 ingresses. Got: %v", len(ings))
9821167
}
983-
if len(nonMinions) != 1 {
984-
t.Fatalf("Expected 1 non-Minion. Got: %v", len(nonMinions))
1168+
if len(ings) != 1 {
1169+
t.Fatalf("Expected 1 ingress. Got: %v", len(ings))
9851170
}
986-
if nonMinions[0].Name != test.ingress.ObjectMeta.Name || nonMinions[0].Namespace != test.ingress.ObjectMeta.Namespace {
987-
t.Fatalf("Expected: %v/%v. Got: %v/%v.", test.ingress.ObjectMeta.Namespace, test.ingress.ObjectMeta.Name, nonMinions[0].Namespace, nonMinions[0].Name)
1171+
if ings[0].Name != test.ingress.Name || ings[0].Namespace != test.ingress.Namespace {
1172+
t.Fatalf("Expected: %v/%v. Got: %v/%v.", test.ingress.Namespace, test.ingress.Name, ings[0].Namespace, ings[0].Name)
9881173
}
9891174
} else if test.expectedToFind {
990-
t.Fatal("Expected 1 non-Minion. Got: 0")
1175+
t.Fatal("Expected 1 ingress. Got: 0")
9911176
}
9921177
})
9931178
}
1179+
9941180
}

‎internal/handlers/secret.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,8 @@ func CreateSecretHandlers(lbc *controller.LoadBalancerController) cache.Resource
1717
if err := lbc.ValidateSecret(secret); err != nil {
1818
return
1919
}
20-
nsname := secret.Namespace + "/" + secret.Name
21-
if nsname == lbc.GetDefaultServerSecret() {
22-
glog.V(3).Infof("Adding default server Secret: %v", secret.Name)
23-
lbc.AddSyncQueue(obj)
24-
}
20+
glog.V(3).Infof("Adding Secret: %v", secret.Name)
21+
lbc.AddSyncQueue(obj)
2522
},
2623
DeleteFunc: func(obj interface{}) {
2724
secret, isSecr := obj.(*api_v1.Secret)

0 commit comments

Comments
 (0)