helm release 4.5.0 broke regex in path

[gecube]

Hi!

It is just more reminder to me. 4 hours ago we faced the issue with our k8s cluster - the application stopped to work. I found that the fluxcd automatically updated the chart of nginx ingress to the latest version, 4.5.0. The issue is that we have a lot of ingresses and some of them stopped working. The values for helm was the same. I will investigate what happened and probably return with more data. The revert to helm chart 4.4.2 helped. The issue was reproducible at least on two clusters (our dev and stage).

The values looked like:

...
  controller:
    service:
      annotations:
        service.beta.kubernetes.io/aws-load-balancer-backend-protocol: tcp
        service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout: '60'
        service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: 'true'
        service.beta.kubernetes.io/aws-load-balancer-type: nlb

Others were by default.

The ingresses are typical like:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    cert-manager.io/cluster-issuer: letsencrypt-http01-nginx-issuer
    nginx.ingress.kubernetes.io/rewrite-target: /$2
  name: source-ingress
  namespace: source
spec:
  ingressClassName: nginx
  rules:
  - host: blablbla.host.com
    http:
      paths:
      - path: /api/source(/|$)(.*)
        backend:
          service:
            name: source-service
            port:
              number: 3000
        pathType: ImplementationSpecific
  tls:
  - hosts:
    - blablbla.host.com
    secretName: source-tls

I will try to find where the behaviour changed.

[tao12345666333]

the application stopped to work

could you please add more details about this? I don't understand exactly what problem you're having

[gecube]

@tao12345666333 no problem, I will try to be precise as possible.

So we have bunch of ingresses inside of our k8s cluster

kubectl get ing -A | wc -l
      27

On the version helm nginx 4.4.2 if I open https://***.***.***.ai/api/source/graphql it will open the GraphQL console.
If I change the version to 4.5.0 with the same set of ingress objects - I am getting to the home page of application.
So it is breaks the application.
I may guess that it is not the behaviour I must observe because all minor updates of nginx ingress before didn't lead to it.
Again - if I revert back to helm nginx 4.4.2 - everything works well.

What could give some additional clues:

1. We are using pathType: ImplementationSpecific I don't remember why we chosen it.
2. We are using annotations extensively like nginx.ingress.kubernetes.io/enable-cors: "true" and nginx.ingress.kubernetes.io/rewrite-target: /$2. But without dangerous stuff like configuration-snippet
3. we are using regexps and capture groups in path: like /api/events(/|$)(.*)
4. all settings (as I mentioned before) were on defaults, no changes to cm of nginx ingress and other custom settings.

[cinesia]

Hi,
I had the same problem.
The issue is related to the regex you used to specify the path /api/events(/|$)(.*), which is not recognized anymore by the Ingress Nginx Controller. To verify, you can restart the Ingress Nginx pod and look closely at the initial logs, it will say that the path is invalid.
My solution was to roll back to version 4.4.2.

UPDATE:
There was already a hint about the problem here (#9511 (comment)).
Based on what I got from the discussions and the PRs (#9511 #9607), it should be related to some sort of path validation.

[gecube]

@cinesia Thanks! I knew that somebody also will be hit but the same issue. It is very pity that regexs like /api/events(/|$)(.*) are not working anymore. At least I may wait still the solution will be developed or some advices given how to rework regexs.

[arisro]

Because of the revert for the more complex path validation, it ended up with this hardcoded implementation:

https://github.com/kubernetes/ingress-nginx/blob/main/pkg/util/ingress/ingress.go#L35

Which is missing, for whatever reason, ? and |.

[cinesia]

Because of the revert for the more complex path validation, we ended up with this hardcoded implementation:

https://github.com/kubernetes/ingress-nginx/blob/main/pkg/util/ingress/ingress.go#L35

Which is missing, for whatever reason, ? and |.

Do you plan to release a hotfix? It's kind of a big breaking change, which I would not expect moving from 4.4.2 to 4.5.0 according to semantic versioning.

[arisro]

Not a maintainer - we simply downgraded the install to 4.4.2 (and also pinned it 👀 ).

[tao12345666333]

Thanks for your feedback, We will verify and fix it quickly

[eugene-romero-capgemini]

This is indeed a breaking change, we are seeing the same issue as @cinesia. Some documentation on the expected new format could be good to have (unless it is a bug? was the old format deprecated at some point?)

[strongjz]

/triage accepted
/priority critical-urgent
/kind bug
/assign @strongjz

[JVMartin]

Is this a repeat of #9464 ?

[tao12345666333]

We just released a new version, can you test it with the new version?

[jeffcasavant]

The new version does not have the issue on my cluster.

[gecube]

I can confirm that 4.5.2 helm is working
BTW, 4.5.1 chart is not accesible:

ingress-nginx    	helmchart/ingress-nginx-ingress-nginx      	4.4.2   	False    	False	invalid chart reference: failed to get chart version for remote reference: no 'ingress-nginx' chart with version matching '4.5.1' found

[tao12345666333]

BTW, 4.5.1 chart is not accesible:

Yes, 4.5.1 doesn't actually create a release by CI job, so it doesn't exist.

[fcuello-fudo]

I think the path validation was fine and there is a missing annotation in the config above: nginx.ingress.kubernetes.io/use-regex: "true". Helm chart version 4.5.0 works for me after adding that annotation where it was missing. You can also see that in the logs.

[arisro]

We have that annotation set.:) everything was working fine in 4.4.2. Is pretty clear from the code that "*" and "|" are missing from the regex validation charset.

Maybe you are not using those - if you do, create a similar Ingress (so you don't brick the existing one) and see if it gets picked up by the controller. 🙃

[gecube]

@fcuello-fudo I don't want to argue, because it will be contrproductive.
I have no reasons for using nginx.ingress.kubernetes.io/use-regex: "true" annotations, and everything worked before. So it was a breaking change of behaviour. Also the question of should one use this annotation or not is not well documented.

The fact is that "regex" like path: /api/source(/|$)(.*) worked without nginx.ingress.kubernetes.io/use-regex: "true" annotation

[fcuello-fudo]

NAME            NAMESPACE       REVISION        UPDATED                                 STATUS          CHART                   APP VERSION
ingress-nginx   ingress-nginx   70              2023-02-14 10:11:44.951722068 +0100 CET deployed        ingress-nginx-4.5.0     1.6.3      

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
   ...
    nginx.ingress.kubernetes.io/rewrite-target: /$1
    nginx.ingress.kubernetes.io/use-regex: "true"
spec:
  ingressClassName: nginx
  rules:
  - host: host
    http:
      paths:
      - backend:
          service:
            name: prometheus-kube-prometheus-alertmanager
            port:
              number: 9093
        path: /alertmanager/(.*)
        pathType: Prefix

This is working fine and doesn't work without nginx.ingress.kubernetes.io/use-regex: "true" 🤷

[fcuello-fudo]

@gecube sure, I'm just pointing out that the doc says that you should use nginx.ingress.kubernetes.io/use-regex: "true" to enable regex (otherwise there is no point on having that annotation) and when I added it it fixed the problem for me.

[IgorMilavec]

Using nginx.ingress.kubernetes.io/use-regex: "true" and it still fails for me with 4.5. Will try 4.5.2.

[strongjz]

I can confirm that 4.5.2 helm is working BTW, 4.5.1 chart is not accesible:

ingress-nginx    	helmchart/ingress-nginx-ingress-nginx      	4.4.2   	False    	False	invalid chart reference: failed to get chart version for remote reference: no 'ingress-nginx' chart with version matching '4.5.1' found

We are do not why 4.5.1 was not released in the PR with 1.6.4, so we bumped the chart to 4.5.2.

[strongjz]

1.6.4 was released with helm 4.5.2 please use those now. All of the path validation was removed from the ingress and the admission controller.

If you have an issue with 1.6.4 or helm 4.5.2 please open a new issue.

thank you,
James

/close

[k8s-ci-robot]

@strongjz: Closing this issue.

In response to this:

1.6.4 was released with helm 4.5.2 please use those now. All of the path validation was removed from the ingress and the admission controller.

If you have an issue with 1.6.4 or helm 4.5.2 please open a new issue.

thank you,
James

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.