Revert Kube-Lego Edge Case Fix and Enforce Regex on All Locations

[zrdaley]

What this PR does / why we need it:

1. Reverts Fix Rewrite-Target Annotation Edge Case #3078
2. Sort the locations by path length in descending order in the NGINX template
3. Enforce the ~* location modifier on ALL paths

The goal of this is to allow for longest path matching without having to prioritize either regex or non-regex paths. In addition, this would allow a user to use regex on non-rewrite annotated paths.

How this works:

NGINX regex matching uses first match not longest match. Ordering by descending length will help resolve this problem.

Which issue this PR fixes *:

fixes #1415

Special notes for your reviewer:

Due to the nature of how NGINX does regex matching, there will still be edge cases. However they are far less blocking than what currently exists:

Example edge case: long regex

Let two paths be defined on an ingress:

• /foo/bar
• /foo/[A-Z0-9]{32}

A request to foo/bar will hit /foo/[A-Z0-9]{32}

@ElvinEfendi @jmreid @sbfaulkner @wayt

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zrdaley
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: aledbf

If they are not already assigned, you can assign the PR to them by writing /assign @aledbf in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aledbf]

We already sort the locations

ingress-nginx/internal/ingress/controller/controller.go

Line 606 in dfa4f8a

sort.SliceStable(value.Locations, func(i, j int) bool {

(at this point in the code the list should be already ordered)

[zrdaley]

For my implementation they need to be ordered in descending length. Would wrapping this sort in sort.Reverse have any negative impact? I.e. is there a reason why they're already being sorted?

[aledbf]

is there a reason why they're already being sorted?

Avoid unnecessarily reloads because of random order and to ensure / is the last location

[zrdaley]

Oh okay great, so it is already sorting in descending order.

[ElvinEfendi]

So all the locations generated will use regular expression regardless whether there's rewrite annotation or not?

Can you given an example to show an issue with the current state?

[zrdaley]

@ElvinEfendi Yes. And the ordering will be used to help mitigate the first match problem.

Consider the case where 3 locations are being built under the existing logic:

• ^~ /blog/topics
• ^~ /blog/topics/
• ~* ^/blog/topics/.*\/?(?<baseuri>.*) (this ingress uses rewrite annotation)

The URI /blog/topics/announcements would match ^~ /blog/topics/ instead of ~* ^/blog/topics/.*\/?(?<baseuri>.*)

With the new logic, the locations and ordering within the config would be:

• ~* ^/blog/topics/.+\/?(?<baseuri>.*) (note that .* has to be replaced with .+ to prevent a catch all)
• ~* ^/blog/topics/
• ~* ^/blog/topics

/blog/topics/announcements would match ~* ^/blog/topics/.+\/?(?<baseuri>.*)
/blog/topics/ would match ^~ /blog/topics/
/blog/topics would match ^~ /blog/topics

There are still edge cases but I've looked at every example ingress that users wanting this feature have listed (see everything below this comment) and they will all work under this logic. Additionally, this resolves the kube-lego problem.

[ElvinEfendi]

@zrdaley I don't know how to feel about using regular expression location modifier for everything always. I like the approach more at #1415 where you enable it using an annotation just for the paths defined in the given ingress. But I also don't like giving too much control using that annotation, I'd rather have an annotation where you can say enable regular expression paths. But then kube-lego issue will stay if you don't enable the annotation.

What about we revert #3078, fix kube-lego issue in a sparate PR by focusing specifically on kube-lego ACME path, in other words apply the exact match location modifier to only kube-lego ACME path and only when rewrite annotation is given. Then we can come back to this PR.

---

@aledbf since you have more context on this I'd love to hear your opinion.

[aledbf]

@ElvinEfendi @zrdaley I am not sure using regular expressions in all the locations makes sense if the paths don't contains a regex. Detecting a regex in the path is not the way so what about an annotation like use-regex: "true" to enable this? If one server contains at least one path with this annotation, we switched all the locations in this server to use a regex in the location.

[aledbf]

@zrdaley thank you for working on this and 👍 for the docs :)

[ElvinEfendi]

Detecting a regex in the path is not the way so what about an annotation like use-regex: "true" to enable this? If one server contains at least one path with this annotation, we switched all the locations in this server to use a regex in the location.

this is exactly what I'm suggesting above as well 👍

[zrdaley]

I don't like the idea of mixing regex and non-regex paths on a server unless the user is given the full ability to use any location modifier. I think that using a boolean annotation for regex could work if it is implemented in the way @aledbf described.

And no problem :)

[wayt]

Detecting a regex in the path is not the way so what about an annotation like use-regex: "true" to enable this? If one server contains at least one path with this annotation, we switched all the locations in this server to use a regex in the location.

I agree with you both on this, this ensures backward compatibility.