New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(route): Implements rewrite-target for ha-proxy #129
feat(route): Implements rewrite-target for ha-proxy #129
Conversation
Hi @benoitf. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@@ -439,6 +442,15 @@ backend {{genBackendNamePrefix $cfg.TLSTermination}}:{{$cfgIdx}} | |||
http-request add-header Forwarded for=%[src];host=%[req.hdr(host)];proto=%[req.hdr(X-Forwarded-Proto)] | |||
{{- end }} | |||
|
|||
{{- with $pathRewriteTarget := firstMatch $pathRewriteTargetPattern (index $cfg.Annotations "haproxy.router.openshift.io/rewrite-target") }} | |||
# Path rewrite target | |||
{{- if eq $pathRewriteTarget "/" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this if required?
If we could leave it to a simple pattern without side effect, I believe it should help in the long term.
I may not be understanding why "/" is a special case though.
(Thanks for looking into this :) ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it that we're trying to allow /$1 and / to do the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
it's to avoid the missing /
or the double /
in the final request to the backend server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://haproxy-ingress.github.io/docs/configuration/keys/#rewrite-target doesn't seems to consider this a special case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just understood what it meant... Line 2 and line 5 in the haproxy documentation example seems to consider it a special case to be handle only when you get "//" at the beginning so that the output is "/" at the begining. Those special case would be handled by the defautl behaviour of haproxy (point 1)
In here, we only handle the case where the rewrite path annotation would be "/" so that all traffic to the host only (i.e. http://endpoint) would still be routed correctly to the backend service (i.e. http://endpoint/). is that it? (Point2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm still wondering about Point2 being right or not for my own understanding XD. Awaiting your answer anxiously :) )
{{- with $pathRewriteTarget := firstMatch $pathRewriteTargetPattern (index $cfg.Annotations "haproxy.router.openshift.io/rewrite-target") }} | ||
# Path rewrite target | ||
{{- if eq $pathRewriteTarget "/" }} | ||
http-request replace-path ^{{ $cfg.Path }}/?(.*)$ {{ $pathRewriteTarget }}\1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern looks suspicious. Suppose $cfg.Path
were /foo
, $pathRewriteTarget
were /bar
, and a client requested /foobar
; would this rule rewrite the request to /barbar
? I would expect the pattern to be something like this: ^{{ $cfg.Path }}(?:/(.*)|$)$
. Similar applies to the pattern below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^{{ $cfg.Path }}(?:/(.*)|$)$
Could probably be simplified to the following: ^{{ $cfg.Path }}(?:/(.*))?$
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Miciah about your first remark, if user has $pathRewriteTarget
set to /bar
it doesn't go in this if cause but in the else cause
Besides, in your example if client request /foobar
it will not even reach the backend so we will not use at all this stuff.
it's because os_http_be.map
file will contain something like
``^mydomain.com(:[0-9]+)?/foo(/.*)?$ be_http:default:example`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is surprising to me as i would have expected the behaviour described by @Miciah to be true ;
Looking at the documentation provided by HAproxy regarding this annotation, one could expect that /foobar
be replaced with /barbar
when the annotation is set to /bar
and the ingress path is /foo
. (ref. line 5 in the table https://haproxy-ingress.github.io/docs/configuration/keys/#rewrite-target)
Ingress path | Request path | Rewrite target | Output
/foo | /foobar | /bar | /barbar
I wouldn't expect /
to be treated differently in the frontend as it's part of the "path" string...
Now, I'm just giving my opinion from a user perspective... please don't mind it if this is something that should be addressed in the haproxy project... (I wouldn't know where to go to refer this as a potential design issue...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elafontaine : well if you specify route path being /foobar
then you can play with annotation on it but if host of the route is only /foo
then you can only play with /foo
and /foo/something
another app could register a backend service with /foobar
host
about /
being handled differently, it's to act like nginx, haproxy and traefik rewrite-target annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I wouldn't be able to have /foobar
still be redirected to /barbar
because it wouldn't even be catched by the ingress resource as being part of /foo
ingress path. Is my understanding right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it would make sense when you think of them as directory/file endpoint, but that's not how I was seeing it before :) )
How would I be able to do this without annotation in the Route resource? |
We need this feature today and looking for ideas on how to do this in a cluster we are using OCP 4.4 on IBM Cloud managed. |
This is for a customer that has a large application running on DIY kubernetes and using an ingress based on Nginx, and now moving to use OpenShift and trying to port the Ingress config to OCP Routes/haproxy |
@benoitf, are you able to rebase the PR? |
There isn't a supported way. The only workaround I can think of at the moment would be to deploy the router using a custom deployment rather than using the operator. |
Annotation is haproxy.router.openshift.io/rewrite-target
@Miciah rebased |
Many thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benoitf, Miciah 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:
Approvers can indicate their approval by writing |
/retest |
@benoitf: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
Weird, the PR is already approved and lgtm'd, but... |
/retest Please review the full test history for this PR and help us cut down flakes. |
19 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/test prow/images |
@Miciah: The specified target(s) for
Use
In response to this:
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. |
/test images |
1 similar comment
/test images |
/test e2e-upgrade |
Last e2e failure looks related to BZ1857928 – [sig-operator] an end user use OLM can subscribe to the cockroachdb operator. |
You could try using another Nginx as a reverse-proxy and write your own nginx.conf to do this. I am also surprised that the latest Openshift on IBM Cloud could not support rewrite-path by annotation. |
This feature is available from which version of OpenShift? |
It is in OpenShift 4.6. |
Annotation is haproxy.router.openshift.io/rewrite-target
Fixes openshift/origin#20474
documentation: openshift/openshift-docs#22021
This feature is also related to this enhancement: openshift/enhancements#287