Breaking change in helm chart 4.4.1 / controller 1.5.2

JVMartin commented 1 year ago

As soon as we automatically upgraded to 4.4.1, many of our services were inaccessible. Looked for common denominator, and discovered that Ingress definitions that use multiple paths/prefixes broke in strange and bizarre ways.

They would serve fake cert instead of cert-manager brokered cert, and if we bypassed the fake cert with e.g. curl --insecure, they would nginx 404 on all paths/prefixes.

Here is a sample broken Ingress (identifying information redacted):

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: test-api
  namespace: test-production
  annotations:
    kubernetes.io/ingress.class: nginx
    cert-manager.io/cluster-issuer: letsencrypt-prod
    nginx.ingress.kubernetes.io/rewrite-target: /$1
spec:
  tls:
  - hosts:
    - test.com
    secretName: test-cert
  rules:
  - host: test.com
    http:
      paths:
      - path: /?(.*)
        pathType: Prefix
        backend:
          service:
            name: mirror
            port:
              number: 80
      - path: /v1/?(.*)
        pathType: Prefix
        backend:
          service:
            name: test-api
            port:
              number: 80

This is on DOKS v1.24.4.

This caused major outages for us across the board, and was extremely insidious to debug.

There were no errors in the ingress controller pods. No errors in the ingress descriptions (k describe ingress). No issue modifying the ingress definitions (k apply -f). No issues in the compiled nginx configs in the ingress controller pods. Because it was so insidious to debug this issue, we had a long downtime and huge impact.

Reverting to 4.4.0 immediately solved the issues.

rikatz commented 1 year ago

Reason: https://github.com/kubernetes/ingress-nginx/pull/9309/files

We had to do some proper validation on paths, and "?" character was left off. I will check with @strongjz and do a quick patch

strongjz commented 1 year ago

/priority critical-urgent /triage accepted

strongjz commented 1 year ago

https://github.com/kubernetes/ingress-nginx/pull/9465

JVMartin commented 1 year ago

Thanks so much for the quick turnaround on this @rikatz @strongjz

I'm out of my depth, but just wanted to voice - even if this is a bug with e.g. the Ingress yaml/config parser thinking ? isn't a valid character in the regexp of a Prefix path definition, shouldn't we still have seen errors from somewhere when we were debugging? (Something like kubectl describe ingress saying "invalid path", or an error from kubectl logs on the ingress controller pods)?

Perhaps a separate issue would be to increase visibility into any potential similar bugs to this?

Or perhaps the "validation" part of the code thought it was okay, but some sanity check post-validation (deeper in the call stack) didn't and so there was just no visibility whatsoever into the underlying issue to me as a user, and so all that is needed is to fix the bug bc it isn't really possible to surface this kind of error to the user?

rikatz commented 1 year ago

yes, this is actually a bigger problem we have on ingress, the feedback we provide for users is "poor" :)

we had some plans to add this feedback on events (describe wont show anything as we don't have status fields on Ingress).

The validation we could also do is on webhooks, but in your case the ingress object was already on apiserver, so it is "after" anything we could do.

It is still our plan to provide some better feedbacks to users on why some Ingress couldn't be reconciled, add some pre validation before entering the sync loop.

Thanks for raising this bug :D We are rolling back and re-planning how to release this, and also we will have some minor release soon allowing regex to be part only of "ImplementationSpecific" types

strongjz commented 1 year ago

1.5.2 Github release was removed, and the helm chart has rolled forward to 4.4.2 with controller version 1.5.1

JVMartin commented 1 year ago

@rikatz Thanks for that insight.

I think the validations on the webhooks you describe must not exist (or otherwise help during this bug), because while we had 4.4.1 deployed, we were making small tweaks to the Ingress and applying them (kubectl apply -f ingress.yaml) and the apiserver was happily accepting them with no errors (though we couldn't get anything to actually work with regexp paths until we discovered the helm release and rolled back to 4.4.0).

Are you saying that the nginx ingress controller is no longer going to support regexp in Prefix path types? I'll have to research ImplementationSpecific type, thanks.

JVMartin commented 1 year ago

Ohh sorry I misread - you said the validations we could also do, as in they don't exist yet. Got it.

mycrEEpy commented 1 year ago

Don't know if it's a good idea but maybe you should also remove the container image for v1.5.2? Automation systems like Renovate will still pick up this new version and suggest the update to the user or worse, automatically roll it out due to being a patch version.