Respect the path of --remote-url instead of always appending api/v1/write - #196
Respect the path of --remote-url instead of always appending api/v1/write#196jmichalek132 wants to merge 4 commits into
Conversation
…rite The migration to client_golang's exp/api/remote client made remote.NewAPI join its default path (api/v1/write) onto whatever path --remote-url carries, so non-default endpoints such as Thanos Receive's /api/v1/receive became unreachable. Pass an empty API path when the URL has one, keeping the previous default for host-only URLs. Fixes prometheus-community#173 Signed-off-by: Juraj Michalek <juraj.michalek@grafana.com>
Signed-off-by: Juraj Michalek <juraj.michalek@grafana.com>
c988858 to
9cbc239
Compare
| if cfg.URL.Path != "" && cfg.URL.Path != "/" { | ||
| // remote.NewAPI path.Join-s its path option onto the URL's path; | ||
| // an empty path option keeps the URL's path as-is. | ||
| opts = append(opts, remote.WithAPIPath("")) |
There was a problem hiding this comment.
Yeah this option is sort of tricky to use.
I think originally we thought clients would dedicatedly build paths this way so, you could pass in URL with path of metrics/v1/tenant and call WithAPIPath("api/v1/receive") or so.
I think a cleaner option here, might be to parse the URL from the flag, and split url and path when calling remote.NewAPI and WithAPIPath?
Or we can think about changing behaviour on the exp module, so that if it detects path, it won't append default
There was a problem hiding this comment.
Thanks, took your first suggestion: 3baa985 splits the flag's URL, base to remote.NewAPI and path to WithAPIPath. Host-only URLs and a bare / still get the api/v1/write default.
One wrinkle it introduces: NewAPI joins into url.URL.Path and leaves RawPath alone, so the endpoint sees the decoded path. Harmless for unreserved characters (/ten%61nt posts to /tenant), but %2F exists to stop a slash being a segment boundary, so /tenant%2Freceive would quietly post to /tenant/receive. 5099ae5 rejects that instead of rewriting it.
If you'd rather not carry that validation, your second idea solves it properly: an exp option meaning "this URL is the exact endpoint", skipping the default and leaving the parsed URL untouched, would preserve RawPath. Happy to open that PR.
remote.NewAPI takes the base URL and the API path separately, then
path.Join-s the latter onto the former. Passing the flag's whole URL as
the base and neutralising the join with WithAPIPath("") worked, but
leaned on path.Join dropping empty elements rather than on the client's
documented seam.
Split the URL along that seam instead: everything but the path becomes
the base, the path becomes the API path. Host-only URLs and a bare "/"
still fall through to the client's api/v1/write default.
One behavioural consequence: NewAPI assigns the joined path to
url.URL.Path without updating RawPath, so a percent-encoded segment now
reaches the endpoint decoded (/tenant%2Freceive posts to
/tenant/receive). The test case is updated to document that.
Signed-off-by: Juraj Michalek <juraj.michalek@grafana.com>
Passing the path separately from the base URL means remote.NewAPI joins it into url.URL.Path and leaves RawPath behind, so the endpoint sees the decoded path. For unreserved characters that is plain normalization: %61 is just "a". An encoded separator is different. %2F exists precisely to stop a slash being a segment boundary, so decoding it turns one segment into two and posts the samples to a path the user did not ask for, possibly another tenant's. Reject that input instead of rewriting it silently. Slashes are the only delimiter at risk, because url.URL escapes the others when it re-encodes a path, so a slash-count comparison between Path and RawPath detects exactly the lossy case and leaves harmless escapes alone. Signed-off-by: Juraj Michalek <juraj.michalek@grafana.com>
2 participants
Fixes #173
Since the migration to client_golang's
exp/api/remoteclient,remote.NewAPIjoins its default path (api/v1/write) onto whatever path--remote-urlcarries. Non-default endpoints such as Thanos Receive's/api/v1/receivebecome/api/v1/receive/api/v1/writeand 404; the only workaround was pinning image v0.6.0.With this change, a
--remote-urlpath is respected, subject to the client's path cleaning (trailing and duplicate slashes are dropped; query strings survive). Percent-encoded unreserved characters are normalized to their decoded form, and a path with an escaped separator (%2F) is rejected rather than silently rewritten. Host-only URLs keep theapi/v1/writedefault. A bare/also gets the default: posting samples to/is almost never intended, so this deliberately deviates from the pre-0.7.0 behavior for that one case. Flag help updated to match.Compatibility note: URLs that relied on the current prefix behavior (
http://host/prometheusresolving to/prometheus/api/v1/write) now post to/prometheusdirectly. This restores the pre-exp-client (<= v0.6.0) behavior where the URL path was respected. If you would rather keep prefix semantics, an alternative is a dedicated path-override flag; happy to rework in that direction. The CHANGELOG entry calls this out as well.Tested with a table test covering host-only, root-path, custom-path, trailing-slash, prefix-path, query-string, escaped-character, escaped-separator, and double-slash URLs, for both v1 and v2 message types, against an httptest receiver.