From d9f75145704b3ca7edff7dc7d762c559cea8fe71 Mon Sep 17 00:00:00 2001 From: Alex Bezek Date: Wed, 30 Oct 2024 22:21:46 -0500 Subject: [PATCH] Add better error handling and logging for misconfigured ingress and ingress classes --- .../controller/ingress/ingress_controller.go | 5 +- internal/errors/errors.go | 16 ++ internal/store/driver.go | 10 +- internal/store/store.go | 59 +++++-- internal/store/store_test.go | 147 ++++++++++++++++++ 5 files changed, 220 insertions(+), 17 deletions(-) diff --git a/internal/controller/ingress/ingress_controller.go b/internal/controller/ingress/ingress_controller.go index 4d652e15..8ff8a05e 100644 --- a/internal/controller/ingress/ingress_controller.go +++ b/internal/controller/ingress/ingress_controller.go @@ -2,6 +2,7 @@ package ingress import ( "context" + "fmt" "github.com/go-logr/logr" ingressv1alpha1 "github.com/ngrok/ngrok-operator/api/ingress/v1alpha1" @@ -100,8 +101,10 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct case internalerrors.IsErrDifferentIngressClass(err): log.Info("Ingress is not of type ngrok so skipping it") return ctrl.Result{}, nil + case internalerrors.IsErrorNoDefaultIngressClassFound(err): + log.Info("No ingress class found for the controller") case internalerrors.IsErrInvalidIngressSpec(err): - log.Info("Ingress is not valid so skipping it") + log.Info(fmt.Sprintf("Ingress is not valid so skipping it: %v", err)) return ctrl.Result{}, nil default: log.Error(err, "Failed to get ingress from store") diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 1d5b0c7a..0cc0f834 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -87,6 +87,22 @@ func IsErrDifferentIngressClass(err error) bool { return ok } +type ErrorNoDefaultIngressClassFound struct{} + +// NoDefaultIngressClassFound is meant to be used when no default ingress class is found +func NewNoDefaultIngressClassFound() error { + return ErrorNoDefaultIngressClassFound{} +} + +func (e ErrorNoDefaultIngressClassFound) Error() string { + return "no default ingress class found" +} + +func IsErrorNoDefaultIngressClassFound(err error) bool { + _, ok := err.(ErrorNoDefaultIngressClassFound) + return ok +} + // ErrInvalidIngressSpec is meant to be used when an ingress object has an invalid spec type ErrInvalidIngressSpec struct { errors []string diff --git a/internal/store/driver.go b/internal/store/driver.go index a67a38c0..6fdd65de 100644 --- a/internal/store/driver.go +++ b/internal/store/driver.go @@ -884,7 +884,6 @@ func (d *Driver) calculateHTTPSEdgesFromIngress(edgeMap map[string]ingressv1alph } for _, rule := range ingress.Spec.Rules { - // TODO: Handle routes without hosts that then apply to all edges edge, ok := edgeMap[rule.Host] if !ok { d.log.Error(err, "could not find edge associated with rule", "host", rule.Host) @@ -948,6 +947,15 @@ func (d *Driver) calculateHTTPSEdgesFromIngress(edgeMap map[string]ingressv1alph } route.Metadata = d.ingressNgrokMetadata + // Loop through existing routes and check if any match the path and match type + // If they do, warn about it and continue replacing it + for _, existingRoute := range edge.Spec.Routes { + if existingRoute.Match == route.Match && existingRoute.MatchType == route.MatchType { + d.log.Info("replacing existing route", "route", existingRoute.Match, "newRoute", route.Match) + continue + } + } + edge.Spec.Routes = append(edge.Spec.Routes, route) } diff --git a/internal/store/store.go b/internal/store/store.go index 34d7d058..fdf7b96d 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -370,7 +370,7 @@ func (s Store) ListNgrokModuleSetsV1() []*ingressv1alpha1.NgrokModuleSet { for _, item := range s.stores.NgrokModuleV1.List() { module, ok := item.(*ingressv1alpha1.NgrokModuleSet) if !ok { - s.log.Info("listNgrokModulesV1: dropping object of unexpected type: %#v", item) + s.log.Info(fmt.Sprintf("listNgrokModulesV1: dropping object of unexpected type: %#v", item)) continue } modules = append(modules, module) @@ -384,7 +384,14 @@ func (s Store) ListNgrokModuleSetsV1() []*ingressv1alpha1.NgrokModuleSet { return modules } +// shouldHandleIngress checks if the ingress object is valid and belongs to the correct class. func (s Store) shouldHandleIngress(ing *netv1.Ingress) (bool, error) { + if ing.Annotations != nil { + if deprecatedClass, ok := ing.Annotations["kubernetes.io/ingress.class"]; ok { + s.log.Info(fmt.Sprintf("Deprecated annotation 'kubernetes.io/ingress.class' detected with value: %s", deprecatedClass)) + } + } + ok, err := s.shouldHandleIngressIsValid(ing) if err != nil { return ok, err @@ -395,39 +402,61 @@ func (s Store) shouldHandleIngress(ing *netv1.Ingress) (bool, error) { // shouldHandleIngressCheckClass checks if the ingress should be handled by the controller based on the ingress class func (s Store) shouldHandleIngressCheckClass(ing *netv1.Ingress) (bool, error) { ngrokClasses := s.ListNgrokIngressClassesV1() + if len(ngrokClasses) == 0 { + return false, errors.NewNoDefaultIngressClassFound() + } if ing.Spec.IngressClassName != nil { for _, class := range ngrokClasses { if *ing.Spec.IngressClassName == class.Name { return true, nil } } - } else { - for _, class := range ngrokClasses { - if class.Annotations["ingressclass.kubernetes.io/is-default-class"] == "true" { - return true, nil - } + // Log a specific warning message for unmatched ingress class + s.log.Info(fmt.Sprintf("Ingress is not of type %s so skipping it", s.controllerName), "class", *ing.Spec.IngressClassName) + return false, errors.NewErrDifferentIngressClass(s.ListNgrokIngressClassesV1(), ing.Spec.IngressClassName) + } + + // Check if any class is marked as default + for _, class := range ngrokClasses { + if class.Annotations["ingressclass.kubernetes.io/is-default-class"] == "true" { + return true, nil } } - return false, errors.NewErrDifferentIngressClass(s.ListNgrokIngressClassesV1(), ing.Spec.IngressClassName) + + // Log if no default class found and no specific class match + s.log.Info(fmt.Sprintf("Matching ingress class %s not found and no suitable default", s.controllerName), "ingress", ing.Name) + return false, errors.NewErrDifferentIngressClass(ngrokClasses, ing.Spec.IngressClassName) } -// shouldHandleIngressIsValid checks if the ingress should be handled by the controller based on the ingress spec +// shouldHandleIngressIsValid checks if the ingress spec meets controller requirements. func (s Store) shouldHandleIngressIsValid(ing *netv1.Ingress) (bool, error) { errs := errors.NewErrInvalidIngressSpec() if len(ing.Spec.Rules) == 0 { errs.AddError("At least one rule is required to be set") } else { - if ing.Spec.Rules[0].Host == "" { - errs.AddError("A host is required to be set") - } - - for _, path := range ing.Spec.Rules[0].HTTP.Paths { - if path.Backend.Resource != nil { - errs.AddError("Resource backends are not supported") + for _, rule := range ing.Spec.Rules { + if rule.Host == "" { + errs.AddError("A host is required to be set for each rule") + } + if rule.HTTP != nil { + for _, path := range rule.HTTP.Paths { + if path.Backend.Resource != nil { + errs.AddError("Resource backends are not supported") + } + if path.Backend.Service == nil { + errs.AddError("Service backends are required for this ingress") + } + } + } else { + errs.AddError("HTTP rules are required for ingress") } } } + if ing.Spec.DefaultBackend != nil { + errs.AddError("Default backends are not supported") + } + if errs.HasErrors() { return false, errs } diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 5e8692a9..a5f803d2 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -337,4 +337,151 @@ var _ = Describe("Store", func() { }) }) }) + + var _ = Describe("Store Validation", func() { + var store Store + var logger logr.Logger + + BeforeEach(func() { + // Setup the Store directly instead of through the Storer interface + logger = logr.New(logr.Discard().GetSink()) + cacheStores := NewCacheStores(logger) + store = Store{ + stores: cacheStores, + controllerName: defaultControllerName, + log: logger, + } + ngrokClass := NewTestIngressClass("ngrok", true, true) + Expect(store.Add(&ngrokClass)).To(BeNil()) + }) + + Context("when ingress has missing HTTP rules", func() { + It("returns an error without crashing", func() { + ing := NewTestIngressV1("ingress-no-rules", "test-namespace") + ing.Spec.Rules = []netv1.IngressRule{{ + Host: "test.com", + }} + ok, err := store.shouldHandleIngressIsValid(&ing) + Expect(ok).To(BeFalse()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("HTTP rules are required for ingress")) + }) + }) + + Context("when ingress has unsupported default backend", func() { + It("ignores the ingress with default backend and returns an error", func() { + ing := NewTestIngressV1("ingress-default-backend", "test-namespace") + ing.Spec.DefaultBackend = &netv1.IngressBackend{ + Service: &netv1.IngressServiceBackend{ + Name: "default-service", + Port: netv1.ServiceBackendPort{Number: 80}, + }, + } + ok, err := store.shouldHandleIngressIsValid(&ing) + Expect(ok).To(BeFalse()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Default backends are not supported")) + }) + }) + + Context("when ingress rule is missing hostname", func() { + It("flags the ingress as invalid", func() { + ing := NewTestIngressV1("ingress-no-host", "test-namespace") + ing.Spec.Rules = []netv1.IngressRule{ + { + Host: "a-hostname.com", + IngressRuleValue: netv1.IngressRuleValue{ + HTTP: &netv1.HTTPIngressRuleValue{ + Paths: []netv1.HTTPIngressPath{ + { + Path: "/", + Backend: netv1.IngressBackend{ + Service: &netv1.IngressServiceBackend{ + Name: "test-service", + Port: netv1.ServiceBackendPort{Number: 80}, + }, + }, + }, + }, + }, + }, + }, + { + Host: "", // Missing hostname + IngressRuleValue: netv1.IngressRuleValue{ + HTTP: &netv1.HTTPIngressRuleValue{ + Paths: []netv1.HTTPIngressPath{ + { + Path: "/", + Backend: netv1.IngressBackend{ + Service: &netv1.IngressServiceBackend{ + Name: "test-service", + Port: netv1.ServiceBackendPort{Number: 80}, + }, + }, + }, + }, + }, + }, + }, + } + ok, err := store.shouldHandleIngressIsValid(&ing) + Expect(ok).To(BeFalse()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("A host is required to be set")) + }) + }) + + Context("when ingress uses deprecated ingress annotation", func() { + It("logs a warning about the deprecated annotation", func() { + ing := NewTestIngressV1("ingress-deprecated-annotation", "test-namespace") + ingressClassName := "not-ngrok" + ing.Spec.IngressClassName = &ingressClassName + ing.Annotations = map[string]string{ + "kubernetes.io/ingress.class": "ngrok", + } + ok, err := store.shouldHandleIngress(&ing) + Expect(ok).To(BeFalse()) + Expect(err).ToNot(BeNil()) + }) + }) + + Context("when ingress class does not match", func() { + It("returns an error message showing the ingress class name", func() { + ing := NewTestIngressV1WithClass("ingress-wrong-class", "test-namespace", "not-ngrok") + ok, err := store.shouldHandleIngressCheckClass(&ing) + Expect(ok).To(BeFalse()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("ingress class mismatching")) + Expect(err.Error()).To(ContainSubstring("not-ngrok")) + }) + }) + + Context("when no matching ingress classes are configured", func() { + It("lists known ingress classes in the error message", func() { + ing := NewTestIngressV1WithClass("ingress-no-match", "test-namespace", "no-match-class") + ok, err := store.shouldHandleIngressCheckClass(&ing) + Expect(ok).To(BeFalse()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("no-match-class")) + Expect(err.Error()).To(ContainSubstring("ngrok")) + }) + }) + + Context("when configured ingress class cannot be found", func() { + BeforeEach(func() { + // Delete the ngrok ingress class to simulate missing configuration + ngrokClass := NewTestIngressClass("ngrok", true, true) + Expect(store.Delete(&ngrokClass)).To(BeNil()) + }) + + It("emits a warning or event about the missing class", func() { + ing := NewTestIngressV1WithClass("ingress-missing-class", "test-namespace", "ngrok") + ok, err := store.shouldHandleIngressCheckClass(&ing) + Expect(ok).To(BeFalse()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("no default ingress class found")) + }) + }) + }) })