Add better error handling and logging for misconfigured ingress and i… #479

alex-bezek · 2024-10-31T03:19:46Z

…ngress classes

What

Updates the operator to handle logging helpful error messages for situations it can't resolve ingress for
Resolves:

Creating an ingress object with no http rules makes the controller crash loop #428
- Catch and log a message about not supporting no rules instead of crash looping
Specific Errors and Warnings for Unsupported Ingress Configurations #176
- Catch and log a message for when a default backend is used
- Catch and log a message for when an ingress has a rule with no host
- Log a message in the driver if it detects multiple ingress with the same host and route path
Better Logs for wrong ingress class #175
- warn about using the old deprecated k8s ingress class annotation instead of the spec field. It looks like my kubectl now warns me, but included anyways.
Improve Error logging if the Ingress Class is missing #263
Includes more information about the current classes and warns if there are no classes for the controller

How

test driven dev
more validation checks
a custom error type

Validation

Got nice logs like these after recreating some of the error cases

2024-10-31T02:03:02Z	INFO	Ingress is not valid so skipping it: invalid ingress spec: [HTTP rules are required for ingress]	{"controller": "ingress", "controllerGroup": "networking.k8s.io", "controllerKind": "Ingress", "Ingress": {"name":"ingress-no-http-rules","namespace":"default"}, "namespace": "default", "name": "ingress-no-http-rules", "reconcileID": "49ecf97c-922c-403a-aa0d-a5c7cd41d12c", "ingress": {"name":"ingress-no-http-rules","namespace":"default"}}
2024-10-31T02:03:21Z	INFO	Ingress is not valid so skipping it: invalid ingress spec: [Default backends are not supported]	{"controller": "ingress", "controllerGroup": "networking.k8s.io", "controllerKind": "Ingress", "Ingress": {"name":"ingress-with-default-backend","namespace":"default"}, "namespace": "default", "name": "ingress-with-default-backend", "reconcileID": "b803f4e9-82fc-4afc-80c6-9dd6b1621024", "ingress": {"name":"ingress-with-default-backend","namespace":"default"}}
2024-10-31T02:03:35Z	INFO	Ingress is not valid so skipping it: invalid ingress spec: [A host is required to be set for each rule]	{"controller": "ingress", "controllerGroup": "networking.k8s.io", "controllerKind": "Ingress", "Ingress": {"name":"ingress-no-hostname","namespace":"default"}, "namespace": "default", "name": "ingress-no-hostname", "reconcileID": "68b80707-2434-4356-a0c6-e30248fb715a", "ingress": {"name":"ingress-no-hostname","namespace":"default"}}
2024-10-31T02:03:53Z	INFO	cache-store-driver	Deprecated annotation 'kubernetes.io/ingress.class' detected with value: ngrok
2024-10-31T02:03:53Z	INFO	cache-store-driver	Ingress class k8s.ngrok.com/ingress-controller not found	{"ingress": "ingress-deprecated-class"}
2024-10-31T02:03:53Z	INFO	Ingress is not of type ngrok so skipping it	{"controller": "ingress", "controllerGroup": "networking.k8s.io", "controllerKind": "Ingress", "Ingress": {"name":"ingress-deprecated-class","namespace":"default"}, "namespace": "default", "name": "ingress-deprecated-class", "reconcileID": "7af6cff8-8464-4f9e-8924-4e976e866b06", "ingress": {"name":"ingress-deprecated-class","namespace":"default"}}
2024-10-31T02:05:16Z	INFO	cache-store-driver	Ingress is not of type k8s.ngrok.com/ingress-controller so skipping it	{"class": "not-ngrok"}
2024-10-31T02:05:16Z	INFO	Ingress is not of type ngrok so skipping it	{"controller": "ingress", "controllerGroup": "networking.k8s.io", "controllerKind": "Ingress", "Ingress": {"name":"ingress-no-hostname","namespace":"default"}, "namespace": "default", "name": "ingress-no-hostname", "reconcileID": "e211e611-a830-4c6f-aaa1-8650a6568e1f", "ingress": {"name":"ingress-no-hostname","namespace":"default"}}

Recreated and tested with these cases

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-no-http-rules
  namespace: default
spec:
  rules:
  - host: foo.bar.com
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-with-default-backend
  namespace: default
spec:
  ingressClassName: "ngrok"
  defaultBackend:
    service:
      name: default-backend-service
      port:
        number: 80
  rules:
  - host: "test-alex-bezek.ngrok.io"
    http:
      paths:
      - path: /
        pathType: ImplementationSpecific
        backend:
          service:
            name: example-service
            port:
              number: 80
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-no-hostname
  namespace: default
spec:
  ingressClassName: "ngrok"
  rules:
  - http:
      paths:
      - path: /
        pathType: ImplementationSpecific
        backend:
          service:
            name: no-host-service
            port:
              number: 80
# Dummy conflict-service-1 and conflict-service-2 services are created in the same namespace.
apiVersion: v1
kind: Service
metadata:
  name: conflict-service-1
  namespace: default
spec:
  selector:
    app: conflict-app-1
  ports:
  - protocol: TCP
    port: 80
    targetPort: 80
---
apiVersion: v1
kind: Service
metadata:
  name: conflict-service-2
  namespace: default
spec:
  selector:
    app: conflict-app-2
  ports:
  - protocol: TCP
    port: 80
    targetPort: 80
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-conflict-1
  namespace: default
spec:
  ingressClassName: "ngrok"
  rules:
  - host: "conflict-test-alex-bezek.ngrok.io"
    http:
      paths:
      - path: /conflict2
        pathType: ImplementationSpecific
        backend:
          service:
            name: conflict-service-1
            port:
              number: 80
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-conflict-2
  namespace: default
spec:
  ingressClassName: "ngrok"
  rules:
  - host: "conflict-test-alex-bezek.ngrok.io"
    http:
      paths:
      - path: /conflict2
        pathType: ImplementationSpecific
        backend:
          service:
            name: conflict-service-2
            port:
              number: 80
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-deprecated-class
  namespace: default
  annotations:
    kubernetes.io/ingress.class: "ngrok"
spec:
  rules:
  - host: "deprecated-test-alex-bezek.ngrok.io"
    http:
      paths:
      - path: /
        pathType: ImplementationSpecific
        backend:
          service:
            name: deprecated-service
            port:
              number: 80
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-no-hostname
  namespace: default
spec:
  ingressClassName: "not-ngrok"
  rules:
  - host: "non-ngrok-test-alex-bezek.ngrok.io"
    http:
      paths:
      - path: /
        pathType: ImplementationSpecific
        backend:
          service:
            name: no-host-service
            port:
              number: 80

…ngress classes

hjkatz · 2024-10-31T14:37:13Z

internal/controller/ingress/ingress_controller.go

 	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))


Q. Do we emit an event onto the Ingress resouce too so users can see this problem with their Ingress definition?

It doesn't look like we do. I can update this function to record events instead of log statements so we get those. Good idea

I generally like both, that way we as devs can see the logs and users can see the formatted events. Up to you though :)

well any event recorded automatically gets added as a log statement, so we can have both

gd ───────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── │ STDIN ───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 1 │ diff --git a/internal/controller/ingress/domain_controller.go b/internal/controller/ingress/domain_controller.go 2 │ index c1ffe51..71ba3a1 100644 3 │ --- a/internal/controller/ingress/domain_controller.go 4 │ +++ b/internal/controller/ingress/domain_controller.go 5 │ @@ -106,6 +106,7 @@ func (r *DomainReconciler) SetupWithManager(mgr ctrl.Manager) error { 6 │ // For more details, check Reconcile and its Result here: 7 │ // - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile 8 │ func (r *DomainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { 9 │ + r.Recorder.Event(new(ingressv1alpha1.Domain), v1.EventTypeNormal, "TEST EVENT RECORDER FROM EVENT", fmt.Sprintf("Reconciling Domain %s", req.Name)) 10 │ return r.controller.Reconcile(ctx, req, new(ingressv1alpha1.Domain)) 11 │ } 12 │ ───────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── ❯ k get events --sort-by='.lastTimestamp' -A | grep 'TEST EVENT RECORDER' default 97s Normal TEST EVENT RECORDER FROM EVENT domain Reconciling Domain bezek-different-142345232-ngrok-io default 97s Normal TEST EVENT RECORDER FROM EVENT domain Reconciling Domain my-operator-based-cloud-endpoint-alexbezek-different.ngrok.io default 97s Normal TEST EVENT RECORDER FROM EVENT domain Reconciling Domain test-alex-bezek-ngrok-io default 97s Normal TEST EVENT RECORDER FROM EVENT domain Reconciling Domain bezek-different-1423450293-ngrok-io default 97s Normal TEST EVENT RECORDER FROM EVENT domain Reconciling Domain my-operator-based-cloud-endpoint-alexbezek-different-ngrok-io default 96s Normal TEST EVENT RECORDER FROM EVENT domain Reconciling Domain conflict-test-alex-bezek-ngrok-io default 96s Normal TEST EVENT RECORDER FROM EVENT domain Reconciling Domain example-com ❯ k logs ngrok-operator-manager-7d79944c7d-z2p7c | grep 'TEST EVENT RECORDER' 2024-10-31T15:03:58Z DEBUG events Reconciling Domain test-alex-bezek-ngrok-io {"type": "Normal", "object": {"kind":"Domain","apiVersion":"ingress.k8s.ngrok.com/v1alpha1"}, "reason": "TEST EVENT RECORDER FROM EVENT"} 2024-10-31T15:03:58Z DEBUG events Reconciling Domain bezek-different-1423450293-ngrok-io {"type": "Normal", "object": {"kind":"Domain","apiVersion":"ingress.k8s.ngrok.com/v1alpha1"}, "reason": "TEST EVENT RECORDER FROM EVENT"} 2024-10-31T15:03:58Z DEBUG events Reconciling Domain bezek-different-142345232-ngrok-io {"type": "Normal", "object": {"kind":"Domain","apiVersion":"ingress.k8s.ngrok.com/v1alpha1"}, "reason": "TEST EVENT RECORDER FROM EVENT"} 2024-10-31T15:03:58Z DEBUG events Reconciling Domain my-operator-based-cloud-endpoint-alexbezek-different-ngrok-io {"type": "Normal", "object": {"kind":"Domain","apiVersion":"ingress.k8s.ngrok.com/v1alpha1"}, "reason": "TEST EVENT RECORDER FROM EVENT"} 2024-10-31T15:03:58Z DEBUG events Reconciling Domain my-operator-based-cloud-endpoint-alexbezek-different.ngrok.io {"type": "Normal", "object": {"kind":"Domain","apiVersion":"ingress.k8s.ngrok.com/v1alpha1"}, "reason": "TEST EVENT RECORDER FROM EVENT"} 2024-10-31T15:03:59Z DEBUG events Reconciling Domain conflict-test-alex-bezek-ngrok-io {"type": "Normal", "object": {"kind":"Domain","apiVersion":"ingress.k8s.ngrok.com/v1alpha1"}, "reason": "TEST EVENT RECORDER FROM EVENT"} 2024-10-31T15:03:59Z DEBUG events Reconciling Domain example-com {"type": "Normal", "object": {"kind":"Domain","apiVersion":"ingress.k8s.ngrok.com/v1alpha1"}, "reason": "TEST EVENT RECORDER FROM EVENT"}

alex-bezek requested a review from a team as a code owner October 31, 2024 03:19

github-actions bot added the area/controller Issues dealing with the controller label Oct 31, 2024

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

d9f7514

…ngress classes

alex-bezek force-pushed the alex/better-ingress-error-handling branch from 88f5d09 to d9f7514 Compare October 31, 2024 03:22

hjkatz approved these changes Oct 31, 2024

View reviewed changes

Update to use events instead of logs

bac321b

alex-bezek requested a review from jonstacks October 31, 2024 20:57

alex-bezek enabled auto-merge (squash) November 2, 2024 03:32

Merge branch 'main' into alex/better-ingress-error-handling

390b133

alex-bezek merged commit 296b21f into main Nov 2, 2024
7 checks passed

alex-bezek deleted the alex/better-ingress-error-handling branch November 2, 2024 03:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add better error handling and logging for misconfigured ingress and i… #479

Add better error handling and logging for misconfigured ingress and i… #479

alex-bezek commented Oct 31, 2024

hjkatz Oct 31, 2024

alex-bezek Oct 31, 2024

hjkatz Oct 31, 2024

alex-bezek Oct 31, 2024

Add better error handling and logging for misconfigured ingress and i… #479

Add better error handling and logging for misconfigured ingress and i… #479

Conversation

alex-bezek commented Oct 31, 2024

What

How

Validation

hjkatz Oct 31, 2024

Choose a reason for hiding this comment

alex-bezek Oct 31, 2024

Choose a reason for hiding this comment

hjkatz Oct 31, 2024

Choose a reason for hiding this comment

alex-bezek Oct 31, 2024

Choose a reason for hiding this comment