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

Author: Fernando Diaz

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