Refactor ingress validation in webhook

internal/admission/controller/main.go

@@ -17,12 +17,15 @@ limitations under the License.
 package controller
 
 import (
+	"fmt"
+
 	"k8s.io/api/admission/v1beta1"
 	extensions "k8s.io/api/extensions/v1beta1"
 	networking "k8s.io/api/networking/v1beta1"
-	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 	"k8s.io/klog"
+
+	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 )
 
 // Checker must return an error if the ingress provided as argument
@@ -37,57 +40,82 @@ type IngressAdmission struct {
 	Checker Checker
 }
 
+var (
+	extensionsResource = metav1.GroupVersionResource{
+		Group:    networking.GroupName,
+		Version:  "v1beta1",
+		Resource: "ingresses",
+	}
+
+	networkingResource = metav1.GroupVersionResource{
+		Group:    extensions.GroupName,
+		Version:  "v1beta1",
+		Resource: "ingresses",
+	}
+)
+
 // HandleAdmission populates the admission Response
 // with Allowed=false if the Object is an ingress that would prevent nginx to reload the configuration
 // with Allowed=true otherwise
-func (ia *IngressAdmission) HandleAdmission(ar *v1beta1.AdmissionReview) error {
+func (ia *IngressAdmission) HandleAdmission(ar *v1beta1.AdmissionReview) {
 	if ar.Request == nil {
-		klog.Infof("rejecting nil request")
 		ar.Response = &v1beta1.AdmissionResponse{
 			Allowed: false,
 		}
-		return nil
+
+		return
 	}
-	klog.V(3).Infof("handling ingress admission webhook request for {%s}  %s in namespace %s", ar.Request.Resource.String(), ar.Request.Name, ar.Request.Namespace)
 
-	ingressResource := v1.GroupVersionResource{Group: networking.SchemeGroupVersion.Group, Version: networking.SchemeGroupVersion.Version, Resource: "ingresses"}
+	if ar.Request.Resource != extensionsResource && ar.Request.Resource != networkingResource {
-
+		err := fmt.Errorf("rejecting admission review because the request does not contains an Ingress resource but %s with name %s in namespace %s",
-	oldIngressResource := v1.GroupVersionResource{Group: extensions.SchemeGroupVersion.Group, Version: extensions.SchemeGroupVersion.Version, Resource: "ingresses"}
+			ar.Request.Resource.String(), ar.Request.Name, ar.Request.Namespace)
-
-	if ar.Request.Resource == ingressResource || ar.Request.Resource == oldIngressResource {
 		ar.Response = &v1beta1.AdmissionResponse{
 			UID:     ar.Request.UID,
 			Allowed: false,
-		}
+			Result:  &metav1.Status{Message: err.Error()},
-		ingress := networking.Ingress{}
-		deserializer := codecs.UniversalDeserializer()
-		if _, _, err := deserializer.Decode(ar.Request.Object.Raw, nil, &ingress); err != nil {
-			ar.Response.Result = &v1.Status{Message: err.Error()}
-			ar.Response.AuditAnnotations = map[string]string{
-				parser.GetAnnotationWithPrefix("error"): err.Error(),
-			}
-			klog.Errorf("failed to decode ingress %s in namespace %s: %s, refusing it", ar.Request.Name, ar.Request.Namespace, err.Error())
-			return err
 		}
 
-		err := ia.Checker.CheckIngress(&ingress)
+		return
-		if err != nil {
-			ar.Response.Result = &v1.Status{Message: err.Error()}
-			ar.Response.AuditAnnotations = map[string]string{
-				parser.GetAnnotationWithPrefix("error"): err.Error(),
-			}
-			klog.Errorf("failed to generate configuration for ingress %s in namespace %s: %s, refusing it", ar.Request.Name, ar.Request.Namespace, err.Error())
-			return err
-		}
-		ar.Response.Allowed = true
-		klog.Infof("successfully validated configuration, accepting ingress %s in namespace %s", ar.Request.Name, ar.Request.Namespace)
-		return nil
 	}
 
-	klog.Infof("accepting non ingress %s in namespace %s %s", ar.Request.Name, ar.Request.Namespace, ar.Request.Resource.String())
+	ingress := networking.Ingress{}
+	deserializer := codecs.UniversalDeserializer()
+	if _, _, err := deserializer.Decode(ar.Request.Object.Raw, nil, &ingress); err != nil {
+		klog.Errorf("failed to decode ingress %s in namespace %s: %s, refusing it",
+			ar.Request.Name, ar.Request.Namespace, err.Error())
+
+		ar.Response = &v1beta1.AdmissionResponse{
+			UID:     ar.Request.UID,
+			Allowed: false,
+
+			Result: &metav1.Status{Message: err.Error()},
+			AuditAnnotations: map[string]string{
+				parser.GetAnnotationWithPrefix("error"): err.Error(),
+			},
+		}
+
+		return
+	}
+
+	if err := ia.Checker.CheckIngress(&ingress); err != nil {
+		klog.Errorf("failed to generate configuration for ingress %s in namespace %s: %s, refusing it",
+			ar.Request.Name, ar.Request.Namespace, err.Error())
+		ar.Response = &v1beta1.AdmissionResponse{
+			UID:     ar.Request.UID,
+			Allowed: false,
+			Result:  &metav1.Status{Message: err.Error()},
+			AuditAnnotations: map[string]string{
+				parser.GetAnnotationWithPrefix("error"): err.Error(),
+			},
+		}
+
+		return
+	}
+
+	klog.Infof("successfully validated configuration, accepting ingress %s in namespace %s",
+		ar.Request.Name, ar.Request.Namespace)
 	ar.Response = &v1beta1.AdmissionResponse{
 		UID:     ar.Request.UID,
 		Allowed: true,
 	}
-	return nil
 }

internal/admission/controller/main_test.go

@@ -58,52 +58,44 @@ func TestHandleAdmission(t *testing.T) {
 			Resource: v1.GroupVersionResource{Group: "", Version: "v1", Resource: "pod"},
 		},
 	}
-	err := adm.HandleAdmission(review)
+
-	if !review.Response.Allowed {
+	adm.HandleAdmission(review)
-		t.Errorf("with a non ingress resource, the check should pass")
+	if review.Response.Allowed {
-	}
+		t.Fatalf("with a non ingress resource, the check should not pass")
-	if err != nil {
-		t.Errorf("with a non ingress resource, no error should be returned")
 	}
 
-	review.Request.Resource = v1.GroupVersionResource{Group: networking.SchemeGroupVersion.Group, Version: networking.SchemeGroupVersion.Version, Resource: "ingresses"}
+	review.Request.Resource = v1.GroupVersionResource{Group: networking.GroupName, Version: "v1beta1", Resource: "ingresses"}
 	review.Request.Object.Raw = []byte{0xff}
 
-	err = adm.HandleAdmission(review)
+	adm.HandleAdmission(review)
 	if review.Response.Allowed {
-		t.Errorf("when the request object is not decodable, the request should not be allowed")
+		t.Fatalf("when the request object is not decodable, the request should not be allowed")
-	}
-	if err == nil {
-		t.Errorf("when the request object is not decodable, an error should be returned")
 	}
 
 	raw, err := json.Marshal(networking.Ingress{ObjectMeta: v1.ObjectMeta{Name: testIngressName}})
 	if err != nil {
-		t.Errorf("failed to prepare test ingress data: %v", err.Error())
+		t.Fatalf("failed to prepare test ingress data: %v", err.Error())
 	}
+
 	review.Request.Object.Raw = raw
 
 	adm.Checker = testChecker{
 		t:   t,
 		err: fmt.Errorf("this is a test error"),
 	}
-	err = adm.HandleAdmission(review)
+
+	adm.HandleAdmission(review)
 	if review.Response.Allowed {
-		t.Errorf("when the checker returns an error, the request should not be allowed")
+		t.Fatalf("when the checker returns an error, the request should not be allowed")
-	}
-	if err == nil {
-		t.Errorf("when the checker returns an error, an error should be returned")
 	}
 
 	adm.Checker = testChecker{
 		t:   t,
 		err: nil,
 	}
-	err = adm.HandleAdmission(review)
+
+	adm.HandleAdmission(review)
 	if !review.Response.Allowed {
-		t.Errorf("when the checker returns no error, the request should be allowed")
+		t.Fatalf("when the checker returns no error, the request should be allowed")
-	}
-	if err != nil {
-		t.Errorf("when the checker returns no error, no error should be returned")
 	}
 }

internal/admission/controller/server.go

@@ -36,7 +36,7 @@ var (
 // AdmissionController checks if an object
 // is allowed in the cluster
 type AdmissionController interface {
-	HandleAdmission(*v1beta1.AdmissionReview) error
+	HandleAdmission(*v1beta1.AdmissionReview)
 }
 
 // AdmissionControllerServer implements an HTTP server
@@ -58,18 +58,16 @@ func NewAdmissionControllerServer(ac AdmissionController) *AdmissionControllerSe
 
 // ServeHTTP implements http.Server method
 func (acs *AdmissionControllerServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	klog.Infof("handling admission controller request %s", r.URL.String())
-
 	review, err := parseAdmissionReview(acs.Decoder, r.Body)
 	if err != nil {
-		klog.Error("Can't decode request", err)
+		klog.Errorf("Unexpected error decoding request: %v", err)
 		w.WriteHeader(http.StatusBadRequest)
 		return
 	}
 
 	acs.AdmissionController.HandleAdmission(review)
 	if err := writeAdmissionReview(w, review); err != nil {
-		klog.Error(err)
+		klog.Errorf("Unexpected returning admission review: %v", err)
 	}
 }
 

internal/admission/controller/server_test.go

@@ -29,12 +29,10 @@ import (
 
 type testAdmissionHandler struct{}
 
-func (testAdmissionHandler) HandleAdmission(ar *v1beta1.AdmissionReview) error {
+func (testAdmissionHandler) HandleAdmission(ar *v1beta1.AdmissionReview) {
 	ar.Response = &v1beta1.AdmissionResponse{
 		Allowed: true,
 	}
-
-	return nil
 }
 
 type errorReader struct{}

internal/ingress/controller/controller.go

@@ -199,19 +199,18 @@ func (n *NGINXController) syncIngress(interface{}) error {
 // CheckIngress returns an error in case the provided ingress, when added
 // to the current configuration, generates an invalid configuration
 func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
-
 	if ing == nil {
 		// no ingress to add, no state change
 		return nil
 	}
 
 	if !class.IsValid(ing) {
-		klog.Infof("ignoring ingress %v in %v based on annotation %v", ing.Name, ing.ObjectMeta.Namespace, class.IngressKey)
+		klog.Warningf("ignoring ingress %v in %v based on annotation %v", ing.Name, ing.ObjectMeta.Namespace, class.IngressKey)
 		return nil
 	}
 
 	if n.cfg.Namespace != "" && ing.ObjectMeta.Namespace != n.cfg.Namespace {
-		klog.Infof("ignoring ingress %v in namespace %v different from the namespace watched %s", ing.Name, ing.ObjectMeta.Namespace, n.cfg.Namespace)
+		klog.Warningf("ignoring ingress %v in namespace %v different from the namespace watched %s", ing.Name, ing.ObjectMeta.Namespace, n.cfg.Namespace)
 		return nil
 	}
 
@@ -220,7 +219,7 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 			toCheck.ObjectMeta.Name == ing.ObjectMeta.Name
 	}
 
-	k8s.SetDefaultPathTypeIfEmpty(ing)
+	k8s.SetDefaultNGINXPathType(ing)
 
 	ings := n.store.ListIngresses(filter)
 	ings = append(ings, &ingress.Ingress{

internal/ingress/controller/store/store.go

@@ -662,20 +662,11 @@ func (s *k8sStore) syncIngress(ing *networkingv1beta1.Ingress) {
 			if path.Path == "" {
 				copyIng.Spec.Rules[ri].HTTP.Paths[pi].Path = "/"
 			}
-
-			if path.PathType == nil {
-				copyIng.Spec.Rules[ri].HTTP.Paths[pi].PathType = &defaultPathType
-				continue
-			}
-
-			// PathType ImplementationSpecific is not supported.
-			// Set type to PathTypePrefix.
-			if *path.PathType == networkingv1beta1.PathTypeImplementationSpecific {
-				copyIng.Spec.Rules[ri].HTTP.Paths[pi].PathType = &defaultPathType
-			}
 		}
 	}
 
+	k8s.SetDefaultNGINXPathType(copyIng)
+
 	err := s.listers.IngressWithAnnotation.Update(&ingress.Ingress{
 		Ingress:           *copyIng,
 		ParsedAnnotations: s.annotations.Extract(ing),
@@ -963,12 +954,12 @@ func toIngress(obj interface{}) (*networkingv1beta1.Ingress, bool) {
 			return nil, false
 		}
 
-		k8s.SetDefaultPathTypeIfEmpty(ing)
+		k8s.SetDefaultNGINXPathType(ing)
 		return ing, true
 	}
 
 	if ing, ok := obj.(*networkingv1beta1.Ingress); ok {
-		k8s.SetDefaultPathTypeIfEmpty(ing)
+		k8s.SetDefaultNGINXPathType(ing)
 		return ing, true
 	}
 

internal/k8s/main.go

@@ -150,11 +150,11 @@ func NetworkingIngressAvailable(client clientset.Interface) (bool, bool) {
 	return runningVersion.AtLeast(version114), runningVersion.AtLeast(version118)
 }
 
-// Default path type is Prefix to not break existing definitions
+// default path type is Prefix to not break existing definitions
 var defaultPathType = networkingv1beta1.PathTypePrefix
 
-// SetDefaultPathTypeIfEmpty sets a default PathType when is not defined.
+// SetDefaultNGINXPathType sets a default PathType when is not defined.
-func SetDefaultPathTypeIfEmpty(ing *networkingv1beta1.Ingress) {
+func SetDefaultNGINXPathType(ing *networkingv1beta1.Ingress) {
 	for _, rule := range ing.Spec.Rules {
 		if rule.IngressRuleValue.HTTP == nil {
 			continue
@@ -165,6 +165,10 @@ func SetDefaultPathTypeIfEmpty(ing *networkingv1beta1.Ingress) {
 			if p.PathType == nil {
 				p.PathType = &defaultPathType
 			}
+
+			if *p.PathType == networkingv1beta1.PathTypeImplementationSpecific {
+				p.PathType = &defaultPathType
+			}
 		}
 	}
 }