-
Notifications
You must be signed in to change notification settings - Fork 523
Preserve pathname prefix in re uri #10409
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
Preserve pathname prefix in re uri #10409
Conversation
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.
Hi, thanks for trying to address this! Overall, this needs some more work.
We've got many places in the code that are currently using re_uri::Origin
directly instead of a full ProxyUri
. That means they wouldn't handle path prefixes correctly. We should instead change Origin
to handle path prefixes.
We also need to make sure that we reserve all paths which don't end in one of our endpoints, that way we can continue to add them in the future. For example, rerun+http://127.0.0.1:9876/rerun/something
should be rejected completely, as neither rerun/something
nor something
is a valid endpoint.
Lastly, we'll need some updated docs. We need to be clear about:
- Endpoint paths may contain an arbitrary amount of prefix path segments
- We reserve the right to add other endpoints in the future, and will reject anything that isn't currently a valid endpoint.
This makes me think we should also preserve query params which we aren't using... 🤔 But you don't need to worry about that, it can be addressed another time.
Again, thanks for taking the time to look at this!
crates/utils/re_uri/src/redap_uri.rs
Outdated
.filter(|s| !s.is_empty()) // handle trailing slashes | ||
.collect::<Vec<_>>(); | ||
|
||
match segments.as_slice() { | ||
["proxy"] => Ok(Self::Proxy(ProxyUri::new(origin))), | ||
|
||
// Handle proxy endpoint with pathname prefix - fix for issue #10373 | ||
segments if segments.last() == Some(&"proxy") => { |
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 general approach is sound, but it should apply to all endpoints, not just /proxy
. One possible solution is to check the last N segments where N is the length of the endpoint, including any possible dynamic path segments.
For example, for /proxy
we just need to check the last path segment. But for /dataset/{dataset_id}
, we need to check the last 2.
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 now handles the other endpoints as well
This still won't work as-is. If I add a log to tell me the actual HTTP URL which the gRPC client will attempt to connect to, and I run
Which is missing the path prefix. This needs to go through #[test]
fn resolved_endpoints_with_prefix() {
let cases = [
// HTTP
(
"rerun+http://localhost/a/b/proxy",
"http://localhost:9876/a/b",
),
(
"rerun+http://localhost/a/b/catalog",
"http://localhost:51234/a/b",
),
(
"rerun+http://localhost/a/b/entry/1830B33B45B963E7774455beb91701ae",
"http://localhost:51234/a/b",
),
(
"rerun+http://localhost/a/b/dataset/1830B33B45B963E7774455beb91701ae/data?partition_id=pid",
"http://localhost:51234/a/b",
),
// HTTPS
(
"rerun://example.com/foo/bar/proxy",
"https://example.com:443/foo/bar",
),
(
"rerun://example.com/foo/bar/catalog",
"https://example.com:443/foo/bar",
),
(
"rerun://example.com/foo/bar/entry/1830B33B45B963E7774455beb91701ae",
"https://example.com:443/foo/bar",
),
(
"rerun://example.com/foo/bar/dataset/1830B33B45B963E7774455beb91701ae/data?partition_id=pid",
"https://example.com:443/foo/bar",
),
];
for (url, expected) in cases {
let result: RedapUri = url.parse().expect("failed to parse proxy URL");
assert_eq!(
expected,
result.origin().as_url(),
"failed to resolve {url}"
);
}
} |
It's also important that you actually test this for your own use case, a locally-running |
looks abandoned. Closing for book-keeping purposes. Please re-open if there's new movement here |
Related
re_uri
#10373What
This PR implements support for parsing proxy endpoints with arbitrary pathname prefixes in the RedapUri parser, addressing the issue where URLs like rerun+http://host/prefix/proxy would fail to parse