Ingress proposal: path rewriting

[l0rd]

No description provided.

[danehans]

Does rewrite need to support regex path matching?

[danehans]

Should the Route, Service or both support the rewrite annotation?

[danehans]

This proposal should live in enhancements/enhancements/ingress.

[danehans]

It would be helpful to provide implementation details. For example, incorporate route specific annotations, provide rewrite examples similar to the haproxy ingress controller, and highlight code changes, testing ,etc ..

[Miciah]

Does rewrite need to support regex path matching?

We don't support regexps for spec.path (and doing so would be a misfeature IMO).

Should the Route, Service or both support the rewrite annotation?

Route would be easiest to implement and most consistent with third-party annotations on Ingress.

It would be helpful to provide implementation details. For example, incorporate route specific annotations, provide rewrite examples similar to the haproxy ingress controller, and highlight code changes, testing ,etc ..

If it's any help, here is a proof-of-concept implementation, minimally tested, based on https://haproxy-ingress.github.io/docs/configuration/keys/#rewrite-target: Miciah/router@010ce49

Let's hammer out the details of the annotation, fill out the enhancement proposal, and then build consensus around the API.

[l0rd]

Thank you @Miciah and @danehans for your review. I have moved the proposal to enhancements/ingress and @benoitf will update it with the required informations. We will let you know as soon as the proposal is ready for (a second) review.

[Miciah]

Do you need capture? The Route API's spec.path value is not a regexp.

[benoitf]

Hello @Miciah I've added a paragraph about that we don't need capture.
But it was an example with nginx annotation. I've added example for the openshift annotation and added as well an example with Traefik

[benoitf]

@danehans

It would be helpful to provide implementation details. For example, incorporate route specific
annotations, provide rewrite examples similar to the haproxy ingress controller, and highlight
code changes, testing ,etc ..

I've opened
openshift/router#129
and
openshift/openshift-docs#22021

to show implementation details/documentation, etc

[elafontaine]

I wonder about the meaning of this line; is it that the rule won't be matched due to the requested path not being with the / ? This is my understanding.... but I would like to be sure I understand for my expectation in the future.

[benoitf]

it's basically that the router will not redirect to the backend (so it means the user will have the 404 error from the frontend)
The 'Application is not available' page

[elafontaine]

Shouldn't this line be an output of /baz ? If I'm wrong, where does /bar comes from?

[elafontaine]

Some applications deals differently when there is a trailing / in the path at the end.

Are we saying we're going to remove the trailing / from now on?

(BTW, I've got no ill intention, I'm just asking as I'm going to be a user, please don't hate me XD)

[elafontaine]

(It seems inconsistent with line 91)

[benoitf]

Thanks for the catch. I did not recopy the right stuff.
Now I will remove the content from there and link it to openshift/openshift-docs#22021

[elafontaine]

Thanks for coming back to me :) I really appreciate.

[elafontaine]

@benoitf Really appreciate someone taking the time to do it at Red Hat :). I've been asking for this for the last 2 months. Keep up the good work :)

[benoitf]

@elafontaine thanks for your feedback

I've now linked the documentation of this enhancement to a WIP Pull Request on openshift-docs repository.
openshift/openshift-docs#22021

[elafontaine]

I guess this is already clear, but I'm stating it for evidence; the list in the proposal should probably be some test cases somewhere for ensuring the rewrite-path behavior. Then we could have the / special case to be handled in there if we wanted to ensure it's not put in place. Honestly though, if it were me, I wouldn't handle / as a special case.

(Opinion alert!)
It's not special enough that we should interpret the path being given to us;
1- If somebody specify "//" than it's their problem... (searching for trouble)
2- It may start to complicate regex matching and cause some undesired side-effect
3- It is something that can be fixed by the client automation side (instead of server trying to resolve clients problems).
4- We can leave the server side handle the "//" (many library do support it, some other don't but then it only highlight the underlying problems)
5- Others want to get the "/" at the end of the path while others want to remove it, this is really the server side that knows what it wants, so getting the regex correctly in place should be given to the ingress resource owner, not haproxy side-effect.

I'm saying this following some massive troubleshooting with the nginx-ingress in which I've learned they've had complexify their rewrite path like you are about to do because of the path rewrite. I understand this is a really complex subject, but I believe the simpler we keep it, the better the user experience and flexibility to your users :) (Disclamer, I'm a customer)

[benoitf]

Hi @elafontaine

I implemented using the same rules than https://haproxy-ingress.github.io/ as it may be easier for people using this annotation for the same underlying engine (haproxy)

[elafontaine]

Just read this documentation of haproxy. I believe it would be ideal if it was straight forward :) . I was worried about the PR that seems to implement it in the router project (openshift/router#129) as it didn't seem to respect the upstream documentation; https://haproxy-ingress.github.io/docs/configuration/keys/#rewrite-target

I honestly want this to be so straight forward so, thanks for instructing me :).

Thanks again and let me know when it's in/want a review/want a user :).

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: l0rd
To complete the pull request process, please assign jwforres
You can assign the PR to them by writing /assign @jwforres in a comment when ready.

The full list of commands accepted by this bot can be found 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

[l0rd]

Closing this PR as the enhancement has been implemented and merged. OCP 4.6 routes will support path rewriting.