Merge pull request #272 from aledbf/refactor-annotation-parsers

Fix error getting class information from Ingress annotations

Author: aledbf

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

@@ -30,6 +30,7 @@ import (
 
 	"github.com/golang/glog"
 	"github.com/mitchellh/mapstructure"
+	"github.com/spf13/pflag"
 
 	"k8s.io/kubernetes/pkg/api"
 
@@ -251,6 +252,11 @@ func (n NGINXController) Info() *ingress.BackendInfo {
 	}
 }
 
+// OverrideFlags customize NGINX controller flags
+func (n NGINXController) OverrideFlags(flags *pflag.FlagSet) {
+	flags.Set("ingress-class", "nginx")
+}
+
 // testTemplate checks if the NGINX configuration inside the byte array is valid
 // running the command "nginx -t" using a temporal file.
 func (n NGINXController) testTemplate(cfg []byte) error {

core/pkg/ingress/annotations/authreq/main.go

@@ -92,7 +92,11 @@ func (a authReq) Parse(ing *extensions.Ingress) (interface{}, error) {
 		return nil, ing_errors.NewLocationDenied("invalid url host")
 	}
 
-	m, _ := parser.GetStringAnnotation(authMethod, ing)
+	m, err := parser.GetStringAnnotation(authMethod, ing)
+	if err != nil {
+		return nil, err
+	}
+
 	if len(m) != 0 && !validMethod(m) {
 		return nil, ing_errors.NewLocationDenied("invalid HTTP method")
 	}

core/pkg/ingress/controller/controller.go

@@ -173,7 +173,7 @@ func newIngressController(config *Configuration) *GenericController {
 		DeleteFunc: func(obj interface{}) {
 			delIng := obj.(*extensions.Ingress)
 			if !IsValidClass(delIng, config.IngressClass) {
-				glog.Infof("ignoring add for ingress %v based on annotation %v", delIng.Name, ingressClassKey)
+				glog.Infof("ignoring delete for ingress %v based on annotation %v", delIng.Name, ingressClassKey)
 				return
 			}
 			ic.recorder.Eventf(delIng, api.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", delIng.Namespace, delIng.Name))
@@ -182,7 +182,7 @@ func newIngressController(config *Configuration) *GenericController {
 		UpdateFunc: func(old, cur interface{}) {
 			oldIng := old.(*extensions.Ingress)
 			curIng := cur.(*extensions.Ingress)
-			if !IsValidClass(curIng, config.IngressClass) {
+			if !IsValidClass(curIng, config.IngressClass) && !IsValidClass(oldIng, config.IngressClass) {
 				return
 			}
 
@@ -564,6 +564,10 @@ func (ic *GenericController) getBackendServers() ([]*ingress.Backend, []*ingress
 	for _, ingIf := range ings {
 		ing := ingIf.(*extensions.Ingress)
 
+		if !IsValidClass(ing, ic.cfg.IngressClass) {
+			continue
+		}
+
 		anns := ic.annotations.Extract(ing)
 
 		for _, rule := range ing.Spec.Rules {
@@ -698,6 +702,10 @@ func (ic *GenericController) createUpstreams(data []interface{}) map[string]*ing
 	for _, ingIf := range data {
 		ing := ingIf.(*extensions.Ingress)
 
+		if !IsValidClass(ing, ic.cfg.IngressClass) {
+			continue
+		}
+
 		secUpstream := ic.annotations.SecureUpstream(ing)
 		hz := ic.annotations.HealthCheck(ing)
 
@@ -840,6 +848,10 @@ func (ic *GenericController) createServers(data []interface{}, upstreams map[str
 	// initialize all the servers
 	for _, ingIf := range data {
 		ing := ingIf.(*extensions.Ingress)
+		if !IsValidClass(ing, ic.cfg.IngressClass) {
+			continue
+		}
+
 		// check if ssl passthrough is configured
 		sslpt := ic.annotations.SSLPassthrough(ing)
 
@@ -868,6 +880,9 @@ func (ic *GenericController) createServers(data []interface{}, upstreams map[str
 	// configure default location and SSL
 	for _, ingIf := range data {
 		ing := ingIf.(*extensions.Ingress)
+		if !IsValidClass(ing, ic.cfg.IngressClass) {
+			continue
+		}
 
 		for _, rule := range ing.Spec.Rules {
 			host := rule.Host

core/pkg/ingress/controller/launch.go

@@ -84,6 +84,8 @@ func NewIngressController(backend ingress.Controller) *GenericController {
 		ingress controller should update the Ingress status IP/hostname. Default is true`)
 	)
 
+	backend.OverrideFlags(flags)
+
 	flags.AddGoFlagSet(flag.CommandLine)
 	flags.Parse(os.Args)
 

core/pkg/ingress/controller/util.go

@@ -26,6 +26,7 @@ import (
 
 	"k8s.io/ingress/core/pkg/ingress"
 	"k8s.io/ingress/core/pkg/ingress/annotations/parser"
+	"k8s.io/ingress/core/pkg/ingress/errors"
 )
 
 // DeniedKeyName name of the key that contains the reason to deny a location
@@ -92,7 +93,10 @@ func IsValidClass(ing *extensions.Ingress, class string) bool {
 		return true
 	}
 
-	cc, _ := parser.GetStringAnnotation(ingressClassKey, ing)
+	cc, err := parser.GetStringAnnotation(ingressClassKey, ing)
+	if err != nil && !errors.IsMissingAnnotations(err) {
+		glog.Warningf("unexpected error reading ingress annotation: %v", err)
+	}
 	if cc == "" {
 		return true
 	}

core/pkg/ingress/controller/util_test.go

@@ -19,6 +19,8 @@ package controller
 import (
 	"testing"
 
+	"reflect"
+
 	"k8s.io/ingress/core/pkg/ingress"
 	"k8s.io/ingress/core/pkg/ingress/annotations/auth"
 	"k8s.io/ingress/core/pkg/ingress/annotations/authreq"
@@ -29,7 +31,6 @@ import (
 	"k8s.io/ingress/core/pkg/ingress/resolver"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/apis/extensions"
-	"reflect"
 )
 
 type fakeError struct{}
@@ -54,6 +55,7 @@ func TestIsValidClass(t *testing.T) {
 	data := map[string]string{}
 	data[ingressClassKey] = "custom"
 	ing.SetAnnotations(data)
+
 	b = IsValidClass(ing, "custom")
 	if !b {
 		t.Errorf("Expected valid class but %v returned", b)
@@ -62,6 +64,10 @@ func TestIsValidClass(t *testing.T) {
 	if b {
 		t.Errorf("Expected invalid class but %v returned", b)
 	}
+	b = IsValidClass(ing, "")
+	if !b {
+		t.Errorf("Expected invalid class but %v returned", b)
+	}
 }
 
 func TestIsHostValid(t *testing.T) {

core/pkg/ingress/types.go

@@ -17,6 +17,8 @@ limitations under the License.
 package ingress
 
 import (
+	"github.com/spf13/pflag"
+
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/healthz"
 
@@ -86,6 +88,8 @@ type Controller interface {
 	BackendDefaults() defaults.Backend
 	// Info returns information about the ingress controller
 	Info() *BackendInfo
+	// OverrideFlags allow the customization of the flags in the backend
+	OverrideFlags(*pflag.FlagSet)
 }
 
 // BackendInfo returns information about the backend.