Handle annotations and conflicting paths for MergeableTypes

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.
 

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
 }
 

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

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

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

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",
+}