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 8246bd230f..2128dad337 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 ( @@ -1213,7 +1214,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 @@ -1232,6 +1240,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) @@ -1243,6 +1265,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 d39131a391..edb2a1cc0e 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) } @@ -789,6 +804,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", +}