From 2ddba72baa451abffd036de4306a91062e82a146 Mon Sep 17 00:00:00 2001 From: Giancarlo Rubio Date: Thu, 2 Mar 2017 16:50:31 +0100 Subject: [PATCH] Fix ingress class --- controllers/nginx/pkg/cmd/controller/nginx.go | 14 +++++-- core/pkg/ingress/controller/controller.go | 15 ++++---- core/pkg/ingress/controller/launch.go | 1 + core/pkg/ingress/controller/util.go | 18 ++++++--- core/pkg/ingress/controller/util_test.go | 38 ++++++++++++++++--- core/pkg/ingress/types.go | 2 + 6 files changed, 65 insertions(+), 23 deletions(-) diff --git a/controllers/nginx/pkg/cmd/controller/nginx.go b/controllers/nginx/pkg/cmd/controller/nginx.go index 458f9f6c3..35ebe1823 100644 --- a/controllers/nginx/pkg/cmd/controller/nginx.go +++ b/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. @@ -256,7 +257,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 diff --git a/core/pkg/ingress/controller/controller.go b/core/pkg/ingress/controller/controller.go index b0ec1b9fa..04d69e305 100644 --- a/core/pkg/ingress/controller/controller.go +++ b/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 } diff --git a/core/pkg/ingress/controller/launch.go b/core/pkg/ingress/controller/launch.go index 216682bdd..18d3ab378 100644 --- a/core/pkg/ingress/controller/launch.go +++ b/core/pkg/ingress/controller/launch.go @@ -144,6 +144,7 @@ func NewIngressController(backend ingress.Controller) *GenericController { ResyncPeriod: *resyncPeriod, DefaultService: *defaultSvc, IngressClass: *ingressClass, + DefaultIngressClass: backend.DefaultIngressClass(), Namespace: *watchNamespace, ConfigMapName: *configMap, TCPConfigMapName: *tcpConfigMapName, diff --git a/core/pkg/ingress/controller/util.go b/core/pkg/ingress/controller/util.go index 7ff154d8f..99f92eac0 100644 --- a/core/pkg/ingress/controller/util.go +++ b/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{}) { diff --git a/core/pkg/ingress/controller/util_test.go b/core/pkg/ingress/controller/util_test.go index 96d46000d..f52558cc6 100644 --- a/core/pkg/ingress/controller/util_test.go +++ b/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) { diff --git a/core/pkg/ingress/types.go b/core/pkg/ingress/types.go index 82ddcf0d3..8121abe4d 100644 --- a/core/pkg/ingress/types.go +++ b/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,