From 1bccb05fe064302e7459c7b29e660b3e5a48e362 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Date: Fri, 23 Mar 2018 15:02:52 -0500 Subject: [PATCH] Handle annotations and conflicting paths for MergeableTypes Adds new rules for the MergeableTypes. Minions will not be able to have conflicting locations, and can only have service level annotations. Masters will only be able to have host level annotations. Fixes #264 --- examples/mergable-ingress-types/README.md | 27 +++++- nginx-controller/controller/controller.go | 23 +++++ .../controller/controller_test.go | 59 +++++++++++++ nginx-controller/nginx/configurator.go | 71 ++++++++++++++-- nginx-controller/nginx/configurator_test.go | 84 +++++++++++++++++++ nginx-controller/nginx/ingress.go | 26 +++++- 6 files changed, 280 insertions(+), 10 deletions(-) diff --git a/examples/mergable-ingress-types/README.md b/examples/mergable-ingress-types/README.md index ce5985cf00..5859af0e0b 100644 --- a/examples/mergable-ingress-types/README.md +++ b/examples/mergable-ingress-types/README.md @@ -10,9 +10,32 @@ host level, which includes the TLS configuration, and any annotations which will can only be one ingress resource on a unique host that contains the master value. Paths cannot be part of the ingress resource. +Masters cannot contain the following annotations: +* nginx.org/rewrites +* nginx.org/ssl-services +* nginx.org/websocket-services +* nginx.com/sticky-cookie-services + A Minion is declared using `nginx.org/mergible-ingress-type: minion`. A Minion will be used to append different -locations to an ingress resource with the Master value. The annotations of minions are replaced with the annotations of -their master. TLS configurations are not allowed. There can be multiple minions which must have the same host as the master. +locations to an ingress resource with the Master value. TLS configurations are not allowed. Multiple minions can be +applied per master as long as they do not have conflicting paths. If a conflicting path is present then the path defined +on the oldest minion will be used. + +Minions cannot contain the following annotations: +* nginx.org/proxy-hide-headers +* nginx.org/proxy-pass-headers +* nginx.org/redirect-to-https +* ingress.kubernetes.io/ssl-redirect +* nginx.org/hsts +* nginx.org/hsts-max-age +* nginx.org/hsts-include-subdomains +* nginx.org/server-tokens +* nginx.org/listen-ports +* nginx.org/listen-ports-ssl +* nginx.com/jwt-key +* nginx.com/jwt-realm +* nginx.com/jwt-token +* nginx.com/jwt-login-url Note: Ingress Resources with more than one host cannot be used. diff --git a/nginx-controller/controller/controller.go b/nginx-controller/controller/controller.go index c4b32e4dfe..105e6d9c5a 100644 --- a/nginx-controller/controller/controller.go +++ b/nginx-controller/controller/controller.go @@ -37,6 +37,7 @@ import ( api_v1 "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sort" ) const ( @@ -1203,7 +1204,14 @@ func (lbc *LoadBalancerController) getMinionsForMaster(master *nginx.IngressEx) return []*nginx.IngressEx{}, err } + // ingresses are sorted by creation time + sort.Slice(ings.Items[:], func(i, j int) bool { + return ings.Items[i].CreationTimestamp.Time.UnixNano() < ings.Items[j].CreationTimestamp.Time.UnixNano() + }) + var minions []*nginx.IngressEx + var minionPaths = make(map[string]*extensions.Ingress) + for i, _ := range ings.Items { if !lbc.isNginxIngress(&ings.Items[i]) { continue @@ -1222,6 +1230,20 @@ func (lbc *LoadBalancerController) getMinionsForMaster(master *nginx.IngressEx) glog.Errorf("Ingress Resource %v/%v with the 'nginx.org/mergible-ingress-type' annotation set to 'minion' must contain a Path", ings.Items[i].Namespace, ings.Items[i].Name) continue } + + uniquePaths := []extensions.HTTPIngressPath{} + for _, path := range ings.Items[i].Spec.Rules[0].HTTP.Paths { + if val, ok := minionPaths[path.Path]; ok { + glog.Errorf("Ingress Resource %v/%v with the 'nginx.org/mergible-ingress-type' annotation set to 'minion' cannot contain the same path as another ingress resource, %v/%v.", + ings.Items[i].Namespace, ings.Items[i].Name, val.Namespace, val.Name) + glog.Errorf("Path %s for Ingress Resource %v/%v will be ignored", path.Path, val.Namespace, val.Name) + } else { + minionPaths[path.Path] = &ings.Items[i] + uniquePaths = append(uniquePaths, path) + } + } + ings.Items[i].Spec.Rules[0].HTTP.Paths = uniquePaths + ingEx, err := lbc.createIngress(&ings.Items[i]) if err != nil { glog.Errorf("Error creating ingress resource %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err) @@ -1233,6 +1255,7 @@ func (lbc *LoadBalancerController) getMinionsForMaster(master *nginx.IngressEx) } minions = append(minions, ingEx) } + return minions, nil } diff --git a/nginx-controller/controller/controller_test.go b/nginx-controller/controller/controller_test.go index c6979e532b..b7fadc9641 100644 --- a/nginx-controller/controller/controller_test.go +++ b/nginx-controller/controller/controller_test.go @@ -401,6 +401,65 @@ func TestGetMinionsForMasterInvalidMinion(t *testing.T) { } } +func TestGetMinionsForMasterConflictingPaths(t *testing.T) { + cafeMaster, coffeeMinion, teaMinion, lbc := getMergableDefaults() + + // Makes sure there is an empty path assigned to a master, to allow for lbc.createIngress() to pass + cafeMaster.Spec.Rules[0].HTTP = &extensions.HTTPIngressRuleValue{ + Paths: []extensions.HTTPIngressPath{}, + } + + coffeeMinion.Spec.Rules[0].HTTP.Paths = append(coffeeMinion.Spec.Rules[0].HTTP.Paths, extensions.HTTPIngressPath{ + Path: "/tea", + Backend: extensions.IngressBackend{ + ServiceName: "tea-svc", + ServicePort: intstr.IntOrString{ + StrVal: "80", + }, + }, + }) + + lbc.ingLister.Add(&cafeMaster) + lbc.ingLister.Add(&coffeeMinion) + lbc.ingLister.Add(&teaMinion) + + cafeMasterIngEx, err := lbc.createIngress(&cafeMaster) + if err != nil { + t.Errorf("Error creating %s(Master): %v", cafeMaster.Name, err) + } + + minions, err := lbc.getMinionsForMaster(cafeMasterIngEx) + if err != nil { + t.Errorf("Error getting Minions for %s(Master): %v", cafeMaster.Name, err) + } + + if len(minions) != 2 { + t.Errorf("Invalid amount of minions: %+v", minions) + } + + coffeePathCount := 0 + teaPathCount := 0 + for _, minion := range minions { + for _, path := range minion.Ingress.Spec.Rules[0].HTTP.Paths { + if path.Path == "/coffee" { + coffeePathCount++ + } else if path.Path == "/tea" { + teaPathCount++ + } else { + t.Errorf("Invalid Path %s exists", path.Path) + } + } + } + + if coffeePathCount != 1 { + t.Errorf("Invalid amount of coffee paths, amount %d", coffeePathCount) + } + + if teaPathCount != 1 { + t.Errorf("Invalid amount of tea paths, amount %d", teaPathCount) + } +} + func getMergableDefaults() (cafeMaster, coffeeMinion, teaMinion extensions.Ingress, lbc LoadBalancerController) { cafeMaster = extensions.Ingress{ TypeMeta: meta_v1.TypeMeta{}, diff --git a/nginx-controller/nginx/configurator.go b/nginx-controller/nginx/configurator.go index 566b63b0c1..b3e1b4fc88 100644 --- a/nginx-controller/nginx/configurator.go +++ b/nginx-controller/nginx/configurator.go @@ -83,6 +83,13 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr var locations []Location var upstreams []Upstream var keepalive string + var removedAnnotations []string + + removedAnnotations = filterMasterAnnotations(mergeableIngs.Master.Ingress.Annotations) + if len(removedAnnotations) != 0 { + glog.Errorf("Ingress Resource %v/%v with the annotation 'nginx.com/mergeable-ingress-type' set to 'master' cannot contain the '%v' annotation(s). They will be ignored", + mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, strings.Join(removedAnnotations, ",")) + } pems, jwtKeyFileName := cnf.updateSecrets(mergeableIngs.Master) masterNginxCfg := cnf.generateNginxCfg(mergeableIngs.Master, pems, jwtKeyFileName) @@ -101,20 +108,28 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr minions := mergeableIngs.Minions for _, minion := range minions { + // Remove the default backend so that "/" will not be generated + minion.Ingress.Spec.Backend = nil + + // Add acceptable master annotations to minion + mergeMasterAnnotationsIntoMinion(minion.Ingress.Annotations, mergeableIngs.Master.Ingress.Annotations) + + removedAnnotations = filterMinionAnnotations(minion.Ingress.Annotations) + if len(removedAnnotations) != 0 { + glog.Errorf("Ingress Resource %v/%v with the annotation 'nginx.com/mergeable-ingress-type' set to 'minion' cannot contain the %v annotation(s). They will be ignored", + minion.Ingress.Namespace, minion.Ingress.Name, strings.Join(removedAnnotations, ",")) + } + pems, jwtKeyFileName := cnf.updateSecrets(minion) nginxCfg := cnf.generateNginxCfg(minion, pems, jwtKeyFileName) - // Replace all minion annotations with master annotations - minion.Ingress.Annotations = mergeableIngs.Master.Ingress.Annotations - for _, server := range nginxCfg.Servers { for _, loc := range server.Locations { - if loc.Path != "/" { - loc.IngressResource = objectMetaToFileName(&minion.Ingress.ObjectMeta) - locations = append(locations, loc) - } + loc.IngressResource = objectMetaToFileName(&minion.Ingress.ObjectMeta) + locations = append(locations, loc) } } + for _, val := range nginxCfg.Upstreams { upstreams = append(upstreams, val) } @@ -766,6 +781,48 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error { return nil } +func filterMasterAnnotations(annotations map[string]string) []string { + var removedAnnotations []string + + for _, blacklistAnn := range masterBlacklist { + if _, ok := annotations[blacklistAnn]; ok { + removedAnnotations = append(removedAnnotations, blacklistAnn) + delete(annotations, blacklistAnn) + } + } + + return removedAnnotations +} + +func filterMinionAnnotations(annotations map[string]string) []string { + var removedAnnotations []string + + for _, blacklistAnn := range minionBlacklist { + if _, ok := annotations[blacklistAnn]; ok { + removedAnnotations = append(removedAnnotations, blacklistAnn) + delete(annotations, blacklistAnn) + } + } + + return removedAnnotations +} + +func mergeMasterAnnotationsIntoMinion(minionAnnotations map[string]string, masterAnnotations map[string]string) { + for key, val := range masterAnnotations { + isBlacklisted := false + if _, ok := minionAnnotations[key]; !ok { + for _, blacklistAnn := range minionBlacklist { + if blacklistAnn == key { + isBlacklisted = true + } + } + if !isBlacklisted { + minionAnnotations[key] = val + } + } + } +} + // UpdateConfig updates NGINX Configuration parameters func (cnf *Configurator) UpdateConfig(config *Config, ingExes []*IngressEx, mergeableIngs map[string]*MergeableIngresses) error { cnf.config = config diff --git a/nginx-controller/nginx/configurator_test.go b/nginx-controller/nginx/configurator_test.go index 1a86dd379d..dfcd2c54b9 100644 --- a/nginx-controller/nginx/configurator_test.go +++ b/nginx-controller/nginx/configurator_test.go @@ -1,6 +1,7 @@ package nginx import ( + "reflect" "testing" ) @@ -61,3 +62,86 @@ func TestParseStickyServiceInvalidFormat(t *testing.T) { t.Errorf("parseStickyService(%s) should return error, got nil", stickyService) } } + +func TestFilterMasterAnnotations(t *testing.T) { + masterAnnotations := map[string]string{ + "nginx.org/rewrites": "serviceName=service1 rewrite=rewrite1", + "nginx.org/ssl-services": "service1", + "nginx.org/hsts": "True", + "nginx.org/hsts-max-age": "2700000", + "nginx.org/hsts-include-subdomains": "True", + } + removedAnnotations := filterMasterAnnotations(masterAnnotations) + + expectedfilteredMasterAnnotations := map[string]string{ + "nginx.org/hsts": "True", + "nginx.org/hsts-max-age": "2700000", + "nginx.org/hsts-include-subdomains": "True", + } + expectedRemovedAnnotations := []string{ + "nginx.org/rewrites", + "nginx.org/ssl-services", + } + + if !reflect.DeepEqual(expectedfilteredMasterAnnotations, masterAnnotations) { + t.Errorf("filterMasterAnnotations returned %v, but expected %v", masterAnnotations, expectedfilteredMasterAnnotations) + } + if !reflect.DeepEqual(expectedRemovedAnnotations, removedAnnotations) { + t.Errorf("filterMasterAnnotations returned %v, but expected %v", removedAnnotations, expectedRemovedAnnotations) + } +} + +func TestFilterMinionAnnotations(t *testing.T) { + minionAnnotations := map[string]string{ + "nginx.org/rewrites": "serviceName=service1 rewrite=rewrite1", + "nginx.org/ssl-services": "service1", + "nginx.org/hsts": "True", + "nginx.org/hsts-max-age": "2700000", + "nginx.org/hsts-include-subdomains": "True", + } + removedAnnotations := filterMinionAnnotations(minionAnnotations) + + expectedfilteredMinionAnnotations := map[string]string{ + "nginx.org/rewrites": "serviceName=service1 rewrite=rewrite1", + "nginx.org/ssl-services": "service1", + } + expectedRemovedAnnotations := []string{ + "nginx.org/hsts", + "nginx.org/hsts-max-age", + "nginx.org/hsts-include-subdomains", + } + + if !reflect.DeepEqual(expectedfilteredMinionAnnotations, minionAnnotations) { + t.Errorf("filterMinionAnnotations returned %v, but expected %v", minionAnnotations, expectedfilteredMinionAnnotations) + } + if !reflect.DeepEqual(expectedRemovedAnnotations, removedAnnotations) { + t.Errorf("filterMinionAnnotations returned %v, but expected %v", removedAnnotations, expectedRemovedAnnotations) + } +} + +func TestMergeMasterAnnotationsIntoMinion(t *testing.T) { + masterAnnotations := map[string]string{ + "nginx.org/proxy-buffering": "True", + "nginx.org/proxy-buffers": "2", + "nginx.org/proxy-buffer-size": "8k", + "nginx.org/hsts": "True", + "nginx.org/hsts-max-age": "2700000", + "nginx.org/proxy-connect-timeout": "50s", + } + minionAnnotations := map[string]string{ + "nginx.org/client-max-body-size": "2m", + "nginx.org/proxy-connect-timeout": "20s", + } + mergeMasterAnnotationsIntoMinion(minionAnnotations, masterAnnotations) + + expectedMergedAnnotations := map[string]string{ + "nginx.org/proxy-buffering": "True", + "nginx.org/proxy-buffers": "2", + "nginx.org/proxy-buffer-size": "8k", + "nginx.org/client-max-body-size": "2m", + "nginx.org/proxy-connect-timeout": "20s", + } + if !reflect.DeepEqual(expectedMergedAnnotations, minionAnnotations) { + t.Errorf("mergeMasterAnnotationsIntoMinion returned %v, but expected %v", minionAnnotations, expectedMergedAnnotations) + } +} diff --git a/nginx-controller/nginx/ingress.go b/nginx-controller/nginx/ingress.go index 325e26cfd1..827c7cbdbd 100644 --- a/nginx-controller/nginx/ingress.go +++ b/nginx-controller/nginx/ingress.go @@ -15,6 +15,30 @@ type IngressEx struct { } type MergeableIngresses struct { - Master *IngressEx + Master *IngressEx Minions []*IngressEx } + +var masterBlacklist = []string{ + "nginx.org/rewrites", + "nginx.org/ssl-services", + "nginx.org/websocket-services", + "nginx.com/sticky-cookie-services", +} + +var minionBlacklist = []string{ + "nginx.org/proxy-hide-headers", + "nginx.org/proxy-pass-headers", + "nginx.org/redirect-to-https", + "ingress.kubernetes.io/ssl-redirect", + "nginx.org/hsts", + "nginx.org/hsts-max-age", + "nginx.org/hsts-include-subdomains", + "nginx.org/server-tokens", + "nginx.org/listen-ports", + "nginx.org/listen-ports-ssl", + "nginx.com/jwt-key", + "nginx.com/jwt-realm", + "nginx.com/jwt-token", + "nginx.com/jwt-login-url", +}