Merge pull request #362 from gianrubio/fix-ingress-class

Fix ingress class

controllers/nginx/pkg/cmd/controller/nginx.go

@@ -48,9 +48,10 @@ const (
 )
 
 var (
-	tmplPath = "/etc/nginx/template/nginx.tmpl"
-	cfgPath  = "/etc/nginx/nginx.conf"
-	binary   = "/usr/sbin/nginx"
+	tmplPath        = "/etc/nginx/template/nginx.tmpl"
+	cfgPath         = "/etc/nginx/nginx.conf"
+	binary          = "/usr/sbin/nginx"
+	defIngressClass = "nginx"
 )
 
 // newNGINXController creates a new NGINX Ingress controller.
@@ -259,7 +260,12 @@ func (n NGINXController) Info() *ingress.BackendInfo {
 
 // OverrideFlags customize NGINX controller flags
 func (n NGINXController) OverrideFlags(flags *pflag.FlagSet) {
-	flags.Set("ingress-class", "nginx")
+	flags.Set("ingress-class", defIngressClass)
+}
+
+// DefaultIngressClass just return the default ingress class
+func (n NGINXController) DefaultIngressClass() string {
+	return defIngressClass
 }
 
 // testTemplate checks if the NGINX configuration inside the byte array is valid

core/pkg/ingress/controller/controller.go

@@ -127,6 +127,7 @@ type Configuration struct {
 	UDPConfigMapName      string
 	DefaultSSLCertificate string
 	DefaultHealthzURL     string
+	DefaultIngressClass   string
 	// optional
 	PublishService string
 	// Backend is the particular implementation to be used.
@@ -166,7 +167,7 @@ func newIngressController(config *Configuration) *GenericController {
 	ingEventHandler := cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			addIng := obj.(*extensions.Ingress)
-			if !IsValidClass(addIng, config.IngressClass) {
+			if !IsValidClass(addIng, config) {
 				glog.Infof("ignoring add for ingress %v based on annotation %v", addIng.Name, ingressClassKey)
 				return
 			}
@@ -175,7 +176,7 @@ func newIngressController(config *Configuration) *GenericController {
 		},
 		DeleteFunc: func(obj interface{}) {
 			delIng := obj.(*extensions.Ingress)
-			if !IsValidClass(delIng, config.IngressClass) {
+			if !IsValidClass(delIng, config) {
 				glog.Infof("ignoring delete for ingress %v based on annotation %v", delIng.Name, ingressClassKey)
 				return
 			}
@@ -185,7 +186,7 @@ func newIngressController(config *Configuration) *GenericController {
 		UpdateFunc: func(old, cur interface{}) {
 			oldIng := old.(*extensions.Ingress)
 			curIng := cur.(*extensions.Ingress)
-			if !IsValidClass(curIng, config.IngressClass) && !IsValidClass(oldIng, config.IngressClass) {
+			if !IsValidClass(curIng, config) && !IsValidClass(oldIng, config) {
 				return
 			}
 
@@ -588,7 +589,7 @@ func (ic *GenericController) getBackendServers() ([]*ingress.Backend, []*ingress
 	for _, ingIf := range ings {
 		ing := ingIf.(*extensions.Ingress)
 
-		if !IsValidClass(ing, ic.cfg.IngressClass) {
+		if !IsValidClass(ing, ic.cfg) {
 			continue
 		}
 
@@ -711,7 +712,7 @@ func (ic *GenericController) createUpstreams(data []interface{}) map[string]*ing
 	for _, ingIf := range data {
 		ing := ingIf.(*extensions.Ingress)
 
-		if !IsValidClass(ing, ic.cfg.IngressClass) {
+		if !IsValidClass(ing, ic.cfg) {
 			continue
 		}
 
@@ -872,7 +873,7 @@ func (ic *GenericController) createServers(data []interface{},
 	// initialize all the servers
 	for _, ingIf := range data {
 		ing := ingIf.(*extensions.Ingress)
-		if !IsValidClass(ing, ic.cfg.IngressClass) {
+		if !IsValidClass(ing, ic.cfg) {
 			continue
 		}
 
@@ -912,7 +913,7 @@ func (ic *GenericController) createServers(data []interface{},
 	// configure default location and SSL
 	for _, ingIf := range data {
 		ing := ingIf.(*extensions.Ingress)
-		if !IsValidClass(ing, ic.cfg.IngressClass) {
+		if !IsValidClass(ing, ic.cfg) {
 			continue
 		}
 

core/pkg/ingress/controller/launch.go

@@ -147,6 +147,7 @@ func NewIngressController(backend ingress.Controller) *GenericController {
 		ResyncPeriod:          *resyncPeriod,
 		DefaultService:        *defaultSvc,
 		IngressClass:          *ingressClass,
+		DefaultIngressClass:   backend.DefaultIngressClass(),
 		Namespace:             *watchNamespace,
 		ConfigMapName:         *configMap,
 		TCPConfigMapName:      *tcpConfigMapName,

core/pkg/ingress/controller/util.go

@@ -88,20 +88,26 @@ func matchHostnames(pattern, host string) bool {
 // IsValidClass returns true if the given Ingress either doesn't specify
 // the ingress.class annotation, or it's set to the configured in the
 // ingress controller.
-func IsValidClass(ing *extensions.Ingress, class string) bool {
-	if class == "" {
-		return true
-	}
+func IsValidClass(ing *extensions.Ingress, config *Configuration) bool {
+	currentIngClass := config.IngressClass
 
 	cc, err := parser.GetStringAnnotation(ingressClassKey, ing)
 	if err != nil && !errors.IsMissingAnnotations(err) {
 		glog.Warningf("unexpected error reading ingress annotation: %v", err)
 	}
-	if cc == "" {
+
+	// we have 2 valid combinations
+	// 1 - ingress with default class | blank annotation on ingress
+	// 2 - ingress with specific class | same annotation on ingress
+	//
+	// and 2 invalid combinations
+	// 3 - ingress with default class | fixed annotation on ingress
+	// 4 - ingress with specific class | different annotation on ingress
+	if (cc == "" && currentIngClass == "") || (currentIngClass == config.DefaultIngressClass) {
 		return true
 	}
 
-	return cc == class
+	return cc == currentIngClass
 }
 
 func mergeLocationAnnotations(loc *ingress.Location, anns map[string]interface{}) {

core/pkg/ingress/controller/util_test.go

@@ -39,6 +39,13 @@ func (fe *fakeError) Error() string {
 	return "fakeError"
 }
 
+// just 2 combinations are valid
+// 1 - ingress with default class (or no args) | blank annotation on ingress | valid
+// 2 - ingress with specified class | same annotation on ingress | valid
+//
+// this combinations are invalid
+// 3 - ingress with default class (or no args) | fixed annotation on ingress | invalid
+// 4 - ingress with specified class | different annotation on ingress | invalid
 func TestIsValidClass(t *testing.T) {
 	ing := &extensions.Ingress{
 		ObjectMeta: api.ObjectMeta{
@@ -47,27 +54,46 @@ func TestIsValidClass(t *testing.T) {
 		},
 	}
 
-	b := IsValidClass(ing, "")
+	config := &Configuration{DefaultIngressClass: "nginx", IngressClass: ""}
+	b := IsValidClass(ing, config)
 	if !b {
 		t.Error("Expected a valid class (missing annotation)")
 	}
 
+	config.IngressClass = "custom"
+	b = IsValidClass(ing, config)
+	if b {
+		t.Error("Expected a invalid class (missing annotation)")
+	}
+
 	data := map[string]string{}
 	data[ingressClassKey] = "custom"
 	ing.SetAnnotations(data)
-
-	b = IsValidClass(ing, "custom")
+	b = IsValidClass(ing, config)
 	if !b {
 		t.Errorf("Expected valid class but %v returned", b)
 	}
-	b = IsValidClass(ing, "nginx")
+
+	config.IngressClass = "killer"
+	b = IsValidClass(ing, config)
 	if b {
 		t.Errorf("Expected invalid class but %v returned", b)
 	}
-	b = IsValidClass(ing, "")
-	if !b {
+
+	data[ingressClassKey] = ""
+	ing.SetAnnotations(data)
+	config.IngressClass = "killer"
+	b = IsValidClass(ing, config)
+	if b {
 		t.Errorf("Expected invalid class but %v returned", b)
 	}
+
+	config.IngressClass = ""
+	b = IsValidClass(ing, config)
+	if !b {
+		t.Errorf("Expected valid class but %v returned", b)
+	}
+
 }
 
 func TestIsHostValid(t *testing.T) {

core/pkg/ingress/types.go

@@ -96,6 +96,8 @@ type Controller interface {
 	Info() *BackendInfo
 	// OverrideFlags allow the customization of the flags in the backend
 	OverrideFlags(*pflag.FlagSet)
+	// DefaultIngressClass just return the default ingress class
+	DefaultIngressClass() string
 }
 
 // StoreLister returns the configured stores for ingresses, services,