impl(gax-internal): use universe_domain when creating gRPC/HTTP transport#5302
impl(gax-internal): use universe_domain when creating gRPC/HTTP transport#5302alvarowolfx wants to merge 15 commits intogoogleapis:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5302 +/- ##
==========================================
- Coverage 97.74% 97.73% -0.01%
==========================================
Files 216 216
Lines 47482 47601 +119
==========================================
+ Hits 46412 46525 +113
- Misses 1070 1076 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| mockall::mock! { | ||
| #[derive(Debug)] | ||
| Credentials {} |
There was a problem hiding this comment.
Aside, non-blocking: We don't need to define this more than once per crate.
And we could even put it in google-cloud-test-utils to define it once for all tests
|
Split gax changes into #5419 |
| // For non GDU environments, endpoint takes priority if provided. | ||
| // And we don't treat regional/locational endpoints specially. | ||
| return Ok((custom_origin, custom_host)); |
There was a problem hiding this comment.
is private.my-custom-ud.com a thing? Or restricted.my-custom-ud.com?
In which case we would want to use like <service>.my-custom-ud.com as the host, but this would use private.my-custom-ud.com.
Duckie found b/361611906 for me. Seems like yes?
There was a problem hiding this comment.
Don't be afraid to scrap this whole origin_and_header function. I wrote it and it was not my finest work.
(My memory is that I was trying to make it as efficient as possible, but (1) that is not so important for client-construction vs. like per request, (2) it made the code inflexible.).
…into consideration
| let (Some(prefix), Some(service)) = ( | ||
| custom_host.strip_suffix(".googleapis.com"), | ||
| default_host.strip_suffix(".googleapis.com"), | ||
| custom_host.strip_suffix(&universe_suffix), |
There was a problem hiding this comment.
I think this fails to strip the suffix on the .with_endpoint("foo.googleapis.com").with_universe_domain("my-custom.ud") case and then it returns "foo.custom.ud", when it should return "foo.googleapis.com".
Assuming I know what the caller is supplying to this function.
There was a problem hiding this comment.
yeah, that's the conflicting thing that I'm stuck on this feature. You commented that before on #5302 (comment).
I'm trying to make the code look good and pass the test cases for localhost and the scenario that you described.
#[test_case("localhost:5678", "test.googleapis.com"; "emulator")]
#[test_case("https://localhost:5678", "test.googleapis.com"; "emulator with scheme")]
#[test_case("https://test.googleapis.com", "test.googleapis.com"; "GDU endpoint with universe domain") ]
#[test_case("https://test.another-universe.com", "test.another-universe.com"; "endpoint priority") ]
fn header_universe_domain(input: &str, want: &str) -> anyhow::Result<()> {
let got = header(
Some(input),
"https://test.googleapis.com",
"my-custom-universe.com",
)?;
assert_eq!(got, want, "input={input:?}");
Ok(())
}
There was a problem hiding this comment.
FWIW, I think these tests are wrong. Not sure why I wrote them.
google-cloud-rust/src/gax-internal/src/host.rs
Lines 161 to 163 in 822e215
Gemini tells me we should use "localhost" as the host when talking to an emulator, not the default service endpoint.
There was a problem hiding this comment.
yup, I'm also finding that some previous assertions for universe domains might be incorrect, like
, it should prioritize the endpoint override, so host should betest.my-universe-domain.com
|
Take inspiration from this: The idea is just to give up if we don't recognize the universe domain as the GDU or the supplied universe domain option. It fails if you have credentials for universe A, but then you try to make a request to universe B where neither are the GDU. Is that even a use case? It seems to defeat the purpose. |
|
Split host resolution header into #5464 |
Towards #3646