Add better error handling and logging for misconfigured ingress and i… (

#479)

* Add better error handling and logging for misconfigured ingress and ingress classes

* Update to use events instead of logs

internal/controller/ingress/ingress_controller.go

@@ -100,11 +100,14 @@ 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):
+		r.Recorder.Event(ingress, "Warning", "NoDefaultIngressClassFound", "No ingress class found for this controller")
+		return ctrl.Result{}, nil
 	case internalerrors.IsErrInvalidIngressSpec(err):
-		log.Info("Ingress is not valid so skipping it")
+		r.Recorder.Eventf(ingress, "Warning", "InvalidIngressSpec", "Ingress is not valid so skipping it: %v", err)
 		return ctrl.Result{}, nil
 	default:
-		log.Error(err, "Failed to get ingress from store")
+		r.Recorder.Event(ingress, "Warning", "FailedGetIngress", "Failed to get ingress from store")
 		return ctrl.Result{}, err
 	}
 

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

internal/store/driver.go

@@ -879,7 +879,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)
@@ -943,6 +942,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)
 			}
 

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
 	}

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"))
+			})
+		})
+	})
 })