From af910a16d43f0e5e58b0491de5dcbe020b26ebdc Mon Sep 17 00:00:00 2001 From: Manuel Alejandro de Brito Fontes Date: Tue, 28 Apr 2020 11:14:27 -0400 Subject: [PATCH] Refactor ingress validation in webhook --- internal/admission/controller/main.go | 98 +++++++++++++------- internal/admission/controller/main_test.go | 38 +++----- internal/admission/controller/server.go | 8 +- internal/admission/controller/server_test.go | 4 +- internal/ingress/controller/controller.go | 7 +- internal/ingress/controller/store/store.go | 17 +--- internal/k8s/main.go | 10 +- 7 files changed, 96 insertions(+), 86 deletions(-) diff --git a/internal/admission/controller/main.go b/internal/admission/controller/main.go index fed650a4e..4e303c538 100644 --- a/internal/admission/controller/main.go +++ b/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" - "k8s.io/ingress-nginx/internal/ingress/annotations/parser" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "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"} - - oldIngressResource := v1.GroupVersionResource{Group: extensions.SchemeGroupVersion.Group, Version: extensions.SchemeGroupVersion.Version, Resource: "ingresses"} - - if ar.Request.Resource == ingressResource || ar.Request.Resource == oldIngressResource { + 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", + ar.Request.Resource.String(), ar.Request.Name, ar.Request.Namespace) ar.Response = &v1beta1.AdmissionResponse{ UID: ar.Request.UID, Allowed: false, - } - 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 + Result: &metav1.Status{Message: err.Error()}, } - err := ia.Checker.CheckIngress(&ingress) - 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 + return } - 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 } diff --git a/internal/admission/controller/main_test.go b/internal/admission/controller/main_test.go index 435eb4334..3d494e950 100644 --- a/internal/admission/controller/main_test.go +++ b/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 { - t.Errorf("with a non ingress resource, the check should pass") - } - if err != nil { - t.Errorf("with a non ingress resource, no error should be returned") + + adm.HandleAdmission(review) + if review.Response.Allowed { + t.Fatalf("with a non ingress resource, the check should not pass") } - 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") - } - if err == nil { - t.Errorf("when the request object is not decodable, an error should be returned") + t.Fatalf("when the request object is not decodable, the request should not be allowed") } 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") - } - if err == nil { - t.Errorf("when the checker returns an error, an error should be returned") + t.Fatalf("when the checker returns an error, the request should not be allowed") } 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") - } - if err != nil { - t.Errorf("when the checker returns no error, no error should be returned") + t.Fatalf("when the checker returns no error, the request should be allowed") } } diff --git a/internal/admission/controller/server.go b/internal/admission/controller/server.go index 5031dca58..cecfc9432 100644 --- a/internal/admission/controller/server.go +++ b/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) } } diff --git a/internal/admission/controller/server_test.go b/internal/admission/controller/server_test.go index 44a28090e..7e9f1dd52 100644 --- a/internal/admission/controller/server_test.go +++ b/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{} diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 57edd9d40..b46b95c92 100644 --- a/internal/ingress/controller/controller.go +++ b/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{ diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 74dc619cd..5ec8b733d 100644 --- a/internal/ingress/controller/store/store.go +++ b/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 } diff --git a/internal/k8s/main.go b/internal/k8s/main.go index 612a60dc2..3e37f8982 100644 --- a/internal/k8s/main.go +++ b/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. -func SetDefaultPathTypeIfEmpty(ing *networkingv1beta1.Ingress) { +// SetDefaultNGINXPathType sets a default PathType when is not defined. +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 + } } } }