Use pointer receiver

Add validatior to admitter

Export skipper ingress annotations

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>

cmd/webhook/admission/admission_test.go

@@ -96,7 +96,7 @@ func TestRouteGroupAdmitter(t *testing.T) {
 		{
 			name:      "not allowed",
 			inputFile: "invalid-rg.json",
-			message:   "Could not validate RouteGroup: error in route group n1/rg1: route group without spec",
+			message:   "error in route group n1/rg1: route group without spec",
 		},
 		{
 			name:      "valid eskip filters",
@@ -105,7 +105,7 @@ func TestRouteGroupAdmitter(t *testing.T) {
 		{
 			name:      "invalid eskip filters",
 			inputFile: "rg-with-invalid-eskip-filters.json",
-			message:   "Could not validate RouteGroup: parse failed after token status, last route id: , position 11: syntax error",
+			message:   "parse failed after token status, last route id: , position 11: syntax error",
 		},
 		{
 			name:      "valid eskip predicates",
@@ -114,12 +114,12 @@ func TestRouteGroupAdmitter(t *testing.T) {
 		{
 			name:      "invalid eskip predicates",
 			inputFile: "rg-with-invalid-eskip-predicates.json",
-			message:   "Could not validate RouteGroup: parse failed after token Method, last route id: Method, position 6: syntax error",
+			message:   "parse failed after token Method, last route id: Method, position 6: syntax error",
 		},
 		{
 			name:      "invalid eskip filters and predicates",
 			inputFile: "rg-with-invalid-eskip-filters-and-predicates.json",
-			message:   "Could not validate RouteGroup: parse failed after token status, last route id: , position 11: syntax error\\nparse failed after token Method, last route id: Method, position 6: syntax error",
+			message:   "parse failed after token status, last route id: , position 11: syntax error\\nparse failed after token Method, last route id: Method, position 6: syntax error",
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -135,7 +135,7 @@ func TestRouteGroupAdmitter(t *testing.T) {
 			req.Header.Set("Content-Type", "application/json")
 
 			w := httptest.NewRecorder()
-			rgAdm := RouteGroupAdmitter{}
+			rgAdm := &RouteGroupAdmitter{}
 
 			h := Handler(rgAdm)
 			h(w, req)
@@ -168,22 +168,22 @@ func TestIngressAdmitter(t *testing.T) {
 		{
 			name:      "invalid eskip filters",
 			inputFile: "invalid-filters.json",
-			message:   `Ingress validation failed: parsing \"zalando.org/skipper-filter\" annotation failed: parse failed after token this, last route id: , position 9: syntax error`,
+			message:   `invalid \"zalando.org/skipper-filter\" annotation: parse failed after token this, last route id: , position 9: syntax error`,
 		},
 		{
 			name:      "invalid eskip predicates",
 			inputFile: "invalid-predicates.json",
-			message:   `Ingress validation failed: parsing \"zalando.org/skipper-predicate\" annotation failed: parse failed after token ), last route id: , position 15: syntax error`,
+			message:   `invalid \"zalando.org/skipper-predicate\" annotation: parse failed after token ), last route id: , position 15: syntax error`,
 		},
 		{
 			name:      "invalid eskip routes",
 			inputFile: "invalid-routes.json",
-			message:   `Ingress validation failed: parsing \"zalando.org/skipper-routes\" annotation failed: invalid predicate count arg`,
+			message:   `invalid \"zalando.org/skipper-routes\" annotation: invalid predicate count arg`,
 		},
 		{
 			name:      "invalid eskip filters and predicates",
 			inputFile: "invalid-filters-and-predicates.json",
-			message:   `Ingress validation failed: parsing \"zalando.org/skipper-filter\" annotation failed: parse failed after token this, last route id: , position 9: syntax error\nparsing \"zalando.org/skipper-predicate\" annotation failed: parse failed after token ), last route id: , position 15: syntax error`,
+			message:   `invalid \"zalando.org/skipper-filter\" annotation: parse failed after token this, last route id: , position 9: syntax error\ninvalid \"zalando.org/skipper-predicate\" annotation: parse failed after token ), last route id: , position 15: syntax error`,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -199,7 +199,7 @@ func TestIngressAdmitter(t *testing.T) {
 			req.Header.Set("Content-Type", "application/json")
 
 			w := httptest.NewRecorder()
-			ingressAdm := IngressAdmitter{}
+			ingressAdm := &IngressAdmitter{}
 
 			h := Handler(ingressAdm)
 			h(w, req)

cmd/webhook/admission/ingressadmission.go → cmd/webhook/admission/ingress.go

@@ -2,45 +2,46 @@ package admission
 
 import (
 	"encoding/json"
-	"fmt"
 
-	log "github.com/sirupsen/logrus"
 	"github.com/zalando/skipper/dataclients/kubernetes/definitions"
 )
 
-type IngressAdmitter struct{}
+type IngressAdmitter struct {
+	IngressValidator *definitions.IngressV1Validator
+}
 
-func (IngressAdmitter) name() string {
+func (iga *IngressAdmitter) name() string {
 	return "ingress"
 }
 
-func (IngressAdmitter) admit(req *admissionRequest) (*admissionResponse, error) {
+func (iga *IngressAdmitter) admit(req *admissionRequest) (*admissionResponse, error) {
+
+	// Serve as default validator if not set
+	if iga.IngressValidator == nil {
+		iga.IngressValidator = &definitions.IngressV1Validator{}
+	}
+
 	ingressItem := definitions.IngressV1Item{}
 	err := json.Unmarshal(req.Object, &ingressItem)
 	if err != nil {
-		emsg := fmt.Sprintf("Could not parse Ingress: %v", err)
-		log.Error(emsg)
 		return &admissionResponse{
 			UID:     req.UID,
 			Allowed: false,
 			Result: &status{
-				Message: emsg,
+				Message: err.Error(),
 			},
-		}, nil
+		}, err
 	}
 
-	ingressValidatore := definitions.IngressV1Validator{}
-	err = ingressValidatore.Validate(&ingressItem)
+	err = iga.IngressValidator.Validate(&ingressItem)
 	if err != nil {
-		emsg := fmt.Sprintf("Ingress validation failed: %v", err)
-		log.Error(emsg)
 		return &admissionResponse{
 			UID:     req.UID,
 			Allowed: false,
 			Result: &status{
-				Message: emsg,
+				Message: err.Error(),
 			},
-		}, nil
+		}, err
 	}
 
 	return &admissionResponse{

cmd/webhook/admission/rgadmission.go → cmd/webhook/admission/routegroup.go

@@ -2,45 +2,46 @@ package admission
 
 import (
 	"encoding/json"
-	"fmt"
 
-	log "github.com/sirupsen/logrus"
 	"github.com/zalando/skipper/dataclients/kubernetes/definitions"
 )
 
 type RouteGroupAdmitter struct {
+	RouteGroupValidator *definitions.RouteGroupValidator
 }
 
-func (RouteGroupAdmitter) name() string {
+func (rga *RouteGroupAdmitter) name() string {
 	return "routegroup"
 }
 
-func (RouteGroupAdmitter) admit(req *admissionRequest) (*admissionResponse, error) {
+func (rga *RouteGroupAdmitter) admit(req *admissionRequest) (*admissionResponse, error) {
+
+	// Serve as default validator if not set
+	if rga.RouteGroupValidator == nil {
+		rga.RouteGroupValidator = &definitions.RouteGroupValidator{}
+	}
+
 	rgItem := definitions.RouteGroupItem{}
 	err := json.Unmarshal(req.Object, &rgItem)
 	if err != nil {
-		emsg := fmt.Sprintf("Could not parse RouteGroup: %v", err)
-		log.Error(emsg)
 		return &admissionResponse{
 			UID:     req.UID,
 			Allowed: false,
 			Result: &status{
-				Message: emsg,
+				Message: err.Error(),
 			},
-		}, nil
+		}, err
 	}
 
-	err = definitions.ValidateRouteGroup(&rgItem)
+	err = rga.RouteGroupValidator.Validate(&rgItem)
 	if err != nil {
-		emsg := fmt.Sprintf("Could not validate RouteGroup: %v", err)
-		log.Error(emsg)
 		return &admissionResponse{
 			UID:     req.UID,
 			Allowed: false,
 			Result: &status{
-				Message: emsg,
+				Message: err.Error(),
 			},
-		}, nil
+		}, err
 	}
 
 	return &admissionResponse{

cmd/webhook/admission/testdata/ingress/valid-ingress-with-annotations.json

@@ -8,9 +8,9 @@
                 "name": "ing1",
                 "namespace": "ing1",
                 "annotations": {
-                    "zalando.org/skipper-filter": "status(10) -> inlineContent(\"This should work\")",
+                    "zalando.org/skipper-filter": "status(200) -> inlineContent(\"This should work\")",
                     "zalando.org/skipper-predicate": "Header(\"test\", \"test\") && Path(\"/login\")",
-                    "zalando.org/skipper-routes": "r1: Header(\"test2\", \"test2\") && Path(\"/login\") -> status(200) -> \"shunt\""
+                    "zalando.org/skipper-routes": "r1: Header(\"test2\", \"test2\") && Path(\"/login\") -> status(200) -> \"http://foo.test\""
                 }
             },
             "spec": {

cmd/webhook/main.go

@@ -12,6 +12,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	log "github.com/sirupsen/logrus"
 	"github.com/zalando/skipper/cmd/webhook/admission"
+	"github.com/zalando/skipper/dataclients/kubernetes/definitions"
 )
 
 const (
@@ -52,8 +53,8 @@ func main() {
 	var cfg = &config{}
 	cfg.parse()
 
-	rgAdmitter := admission.RouteGroupAdmitter{}
-	ingressAdmitter := admission.IngressAdmitter{}
+	rgAdmitter := &admission.RouteGroupAdmitter{RouteGroupValidator: &definitions.RouteGroupValidator{}}
+	ingressAdmitter := &admission.IngressAdmitter{IngressValidator: &definitions.IngressV1Validator{}}
 	handler := http.NewServeMux()
 	handler.Handle("/routegroups", admission.Handler(rgAdmitter))
 	handler.Handle("/ingresses", admission.Handler(ingressAdmitter))

dataclients/kubernetes/clusterclient.go

@@ -358,10 +358,11 @@ func (c *clusterClient) LoadRouteGroups() ([]*definitions.RouteGroupItem, error)
 		return nil, err
 	}
 
+	routeGroupValidator := &definitions.RouteGroupValidator{}
 	rgs := make([]*definitions.RouteGroupItem, 0, len(rgl.Items))
 	for _, i := range rgl.Items {
 		// Validate RouteGroup item.
-		if err := definitions.ValidateRouteGroup(i); err != nil {
+		if err := routeGroupValidator.Validate(i); err != nil {
 			log.Errorf("[routegroup] %v", err)
 			continue
 		}

dataclients/kubernetes/definitions/ingressv1.go

@@ -6,6 +6,12 @@ import (
 	"strconv"
 )
 
+const (
+	SkipperfilterAnnotationKey    = "zalando.org/skipper-filter"
+	SkipperpredicateAnnotationKey = "zalando.org/skipper-predicate"
+	SkipperRoutesAnnotationKey    = "zalando.org/skipper-routes"
+)
+
 var errInvalidPortType = errors.New("invalid port type")
 
 type IngressV1List struct {

dataclients/kubernetes/definitions/ingressvalidator.go

@@ -6,12 +6,6 @@ import (
 	"github.com/zalando/skipper/eskip"
 )
 
-const (
-	skipperfilterAnnotationKey    = "zalando.org/skipper-filter"
-	skipperpredicateAnnotationKey = "zalando.org/skipper-predicate"
-	skipperRoutesAnnotationKey    = "zalando.org/skipper-routes"
-)
-
 type IngressV1Validator struct{}
 
 func (igv *IngressV1Validator) Validate(item *IngressV1Item) error {
@@ -25,32 +19,32 @@ func (igv *IngressV1Validator) Validate(item *IngressV1Item) error {
 }
 
 func (igv *IngressV1Validator) validateFilterAnnotation(annotations map[string]string) error {
-	if filters, ok := annotations[skipperfilterAnnotationKey]; ok {
+	if filters, ok := annotations[SkipperfilterAnnotationKey]; ok {
 		_, err := eskip.ParseFilters(filters)
 		if err != nil {
-			err = fmt.Errorf("parsing \"%s\" annotation failed: %w", skipperfilterAnnotationKey, err)
+			err = fmt.Errorf("invalid \"%s\" annotation: %w", SkipperfilterAnnotationKey, err)
 		}
 		return err
 	}
 	return nil
 }
 
 func (igv *IngressV1Validator) validatePredicateAnnotation(annotations map[string]string) error {
-	if predicates, ok := annotations[skipperpredicateAnnotationKey]; ok {
+	if predicates, ok := annotations[SkipperpredicateAnnotationKey]; ok {
 		_, err := eskip.ParsePredicates(predicates)
 		if err != nil {
-			err = fmt.Errorf("parsing \"%s\" annotation failed: %w", skipperpredicateAnnotationKey, err)
+			err = fmt.Errorf("invalid \"%s\" annotation: %w", SkipperpredicateAnnotationKey, err)
 		}
 		return err
 	}
 	return nil
 }
 
 func (igv *IngressV1Validator) validateRoutesAnnotation(annotations map[string]string) error {
-	if routes, ok := annotations[skipperRoutesAnnotationKey]; ok {
+	if routes, ok := annotations[SkipperRoutesAnnotationKey]; ok {
 		_, err := eskip.Parse(routes)
 		if err != nil {
-			err = fmt.Errorf("parsing \"%s\" annotation failed: %w", skipperRoutesAnnotationKey, err)
+			err = fmt.Errorf("invalid \"%s\" annotation: %w", SkipperRoutesAnnotationKey, err)
 		}
 		return err
 	}

dataclients/kubernetes/ingress.go

@@ -20,9 +20,6 @@ const (
 	ingressRouteIDPrefix                = "kube"
 	backendWeightsAnnotationKey         = "zalando.org/backend-weights"
 	ratelimitAnnotationKey              = "zalando.org/ratelimit"
-	skipperfilterAnnotationKey          = "zalando.org/skipper-filter"
-	skipperpredicateAnnotationKey       = "zalando.org/skipper-predicate"
-	skipperRoutesAnnotationKey          = "zalando.org/skipper-routes"
 	skipperLoadBalancerAnnotationKey    = "zalando.org/skipper-loadbalancer"
 	skipperBackendProtocolAnnotationKey = "zalando.org/skipper-backend-protocol"
 	pathModeAnnotationKey               = "zalando.org/skipper-ingress-path-mode"
@@ -229,7 +226,7 @@ func annotationFilter(m *definitions.Metadata, logger *logger) []*eskip.Filter {
 	if ratelimitAnnotationValue, ok := m.Annotations[ratelimitAnnotationKey]; ok {
 		annotationFilter = ratelimitAnnotationValue
 	}
-	if val, ok := m.Annotations[skipperfilterAnnotationKey]; ok {
+	if val, ok := m.Annotations[definitions.SkipperfilterAnnotationKey]; ok {
 		if annotationFilter != "" {
 			annotationFilter += " -> "
 		}
@@ -249,7 +246,7 @@ func annotationFilter(m *definitions.Metadata, logger *logger) []*eskip.Filter {
 // parse predicate annotation
 func annotationPredicate(m *definitions.Metadata) string {
 	var annotationPredicate string
-	if val, ok := m.Annotations[skipperpredicateAnnotationKey]; ok {
+	if val, ok := m.Annotations[definitions.SkipperpredicateAnnotationKey]; ok {
 		annotationPredicate = val
 	}
 	return annotationPredicate
@@ -258,12 +255,12 @@ func annotationPredicate(m *definitions.Metadata) string {
 // parse routes annotation
 func extraRoutes(m *definitions.Metadata, logger *logger) []*eskip.Route {
 	var extraRoutes []*eskip.Route
-	annotationRoutes := m.Annotations[skipperRoutesAnnotationKey]
+	annotationRoutes := m.Annotations[definitions.SkipperRoutesAnnotationKey]
 	if annotationRoutes != "" {
 		var err error
 		extraRoutes, err = eskip.Parse(annotationRoutes)
 		if err != nil {
-			logger.Errorf("Failed to parse routes from %s, skipping: %v", skipperRoutesAnnotationKey, err)
+			logger.Errorf("Failed to parse routes from %s, skipping: %v", definitions.SkipperRoutesAnnotationKey, err)
 		}
 	}
 	return extraRoutes

dataclients/kubernetes/kube_test.go

@@ -201,13 +201,13 @@ func testIngress(ns, name, defaultService, ratelimitCfg, filterString, predicate
 		setAnnotation(i, ratelimitAnnotationKey, ratelimitCfg)
 	}
 	if filterString != "" {
-		setAnnotation(i, skipperfilterAnnotationKey, filterString)
+		setAnnotation(i, definitions.SkipperfilterAnnotationKey, filterString)
 	}
 	if predicateString != "" {
-		setAnnotation(i, skipperpredicateAnnotationKey, predicateString)
+		setAnnotation(i, definitions.SkipperpredicateAnnotationKey, predicateString)
 	}
 	if routesString != "" {
-		setAnnotation(i, skipperRoutesAnnotationKey, routesString)
+		setAnnotation(i, definitions.SkipperRoutesAnnotationKey, routesString)
 	}
 	if pathModeString != "" {
 		setAnnotation(i, pathModeAnnotationKey, pathModeString)

dataclients/kubernetes/path_test.go

@@ -86,7 +86,7 @@ func TestPathMatchingModes(t *testing.T) {
 
 		annotation := strings.Join(annotations, " && ")
 		if len(annotations) > 0 {
-			i.Metadata.Annotations[skipperpredicateAnnotationKey] = annotation
+			i.Metadata.Annotations[definitions.SkipperpredicateAnnotationKey] = annotation
 		}
 
 		api.ingresses.Items = []*definitions.IngressV1Item{i}

dataclients/kubernetes/redirect.go

@@ -37,7 +37,7 @@ func createRedirectInfo(defaultEnabled bool, defaultCode int) *redirectInfo {
 func (r *redirectInfo) initCurrent(m *definitions.Metadata) {
 	r.enable = m.Annotations[redirectAnnotationKey] == "true"
 	r.disable = m.Annotations[redirectAnnotationKey] == "false"
-	r.ignore = strings.Contains(m.Annotations[skipperpredicateAnnotationKey], `Header("X-Forwarded-Proto"`) || strings.Contains(m.Annotations[skipperRoutesAnnotationKey], `Header("X-Forwarded-Proto"`)
+	r.ignore = strings.Contains(m.Annotations[definitions.SkipperpredicateAnnotationKey], `Header("X-Forwarded-Proto"`) || strings.Contains(m.Annotations[definitions.SkipperRoutesAnnotationKey], `Header("X-Forwarded-Proto"`)
 
 	r.code = r.defaultCode
 	if annotationCode, ok := m.Annotations[redirectCodeAnnotationKey]; ok {