fix: use upstream path in modify response - #269
Conversation
c12ba39 to
3b35b04
Compare
|
Hi @seanbanko can you add a concrete test case? Otherwise I'm sure this will regress |
c8ad898 to
734ea73
Compare
Great call. I've added a simple test case to I copied |
|
@squat just bumping this. Is that test case is sufficient? |
|
Just taking a look at recent commits, @simonpasquier are you able to review? |
| r.modifiers = map[string]func(*http.Response) error{ | ||
| "/api/v1/rules": modifyAPIResponse(r.filterRules), | ||
| "/api/v1/alerts": modifyAPIResponse(r.filterAlerts), | ||
| upstream.Path + "/api/v1/rules": modifyAPIResponse(r.filterRules), |
There was a problem hiding this comment.
Should we use https://pkg.go.dev/net/url#JoinPath instead?
It might be the right time to use a proper struct for modifies instead of an opaque map[string]func.
There was a problem hiding this comment.
Updated to use url.JoinPath.
I agree the map for modifies isn't ideal. Do you think it's better to have a separate PR for the refactor to keep scope limited? Glad to open a follow-up.
7935d27 to
c918521
Compare
|
Looks like the tests are failing. |
7485653 to
094808b
Compare
|
I just re-ran the tests and they are still failing. |
09c91ac to
8bf332a
Compare
8bf332a to
4c789e2
Compare
Signed-off-by: Sean Banko <sean.banko@gmail.com>
4c789e2 to
f115f09
Compare
|
Just following up here. Anything I can do to help get this merged? |
|
@simonpasquier let me know if I can do anything to help get this over the line! |
|
anything additional needed to get this merged? |
|
Hi @simonpasquier , just following up on this to see if the submission is still being considered. |
5 participants
Some open-source projects expose a Prometheus-compatible API at at a subpath, e.g.
/prometheus. We should use theupstream.Pathconfigured when matching theresp.Request.URL.PathinModifyResponseso that we can handle the case of a non-root upstream path when proxying/api/v1/rulesand/api/v1/alerts.