impl(gax-internal): host resolution for custom universe domains#5464
impl(gax-internal): host resolution for custom universe domains#5464alvarowolfx wants to merge 1 commit intogoogleapis:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5464 +/- ##
==========================================
- Coverage 97.73% 97.73% -0.01%
==========================================
Files 217 217
Lines 48186 48243 +57
==========================================
+ Hits 47095 47149 +54
- Misses 1091 1094 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
Can you try skipping this substitution? I think it will simplify the logic.
| universe_domain: &str, | ||
| ) -> Result<(Uri, String), HostError> { | ||
| let default_origin = Uri::from_str(default_endpoint).map_err(HostError::Uri)?; | ||
| let service_endpoint = default_endpoint.replace(DEFAULT_UNIVERSE_DOMAIN, universe_domain); |
There was a problem hiding this comment.
we should determine the service before doing this substitution.
(Can/should we provide the service to this function instea of the default_endpoint? I don't remember what the callers look like)
| let universe_suffix = format!(".{universe_domain}"); | ||
|
|
||
| let service = default_host | ||
| .strip_suffix(&universe_suffix) |
| default_host.strip_suffix(".googleapis.com"), | ||
| ) else { | ||
|
|
||
| let default_suffix = format!(".{DEFAULT_UNIVERSE_DOMAIN}"); |
There was a problem hiding this comment.
nit: this allocation is unnecessary. I'd rather spell out ".googleapis.com" than reuse the const and allocate an extra string when this is called.
| // This is a global or regional endpoint. It should be used as the host. | ||
| // `{service}.{region}.rep.googleapis.com` | ||
| [s, _, "rep"] if *s == service => Ok((custom_origin, custom_host)), | ||
| [s] | [s, _, "rep"] if *s == service => Ok((custom_origin, custom_host)), |
There was a problem hiding this comment.
comment: hmm, so this is needed now for the .with_endpoint("<service>.googleais.com").with_universe_domain("my-ud.com") case.
Otherwise we would fallback to the default host for the custom universe.
There was a problem hiding this comment.
This seems too complicated. I think it is a result of substituting the universe domain in the default endpoint. Can we stop doing that?
We will also mess up theoretical1 cases like .with_endpoint("private.googleapis.com").with_universe_domain("my-ud.com"). I think it should use "<service>.googleapis.com" as the host, not "<service>.my-ud.com".
Footnotes
-
While I don't think it is a real example, it seems more consistent to enforce this behavior. ↩
| #[test_case("https://us-central1-wrong.gooogleapis.com", "test.googleapis.com"; "locational but with bad service")] | ||
| #[test_case("https://us-central1test.gooogleapis.com", "test.googleapis.com"; "maybe locational with missing dash")] | ||
| #[test_case("https://-test.gooogleapis.com", "test.googleapis.com"; "maybe locational with missing location")] |
| ) else { | ||
|
|
||
| let default_suffix = format!(".{DEFAULT_UNIVERSE_DOMAIN}"); | ||
| let universe_suffix = format!(".{universe_domain}"); |
There was a problem hiding this comment.
nit: we could avoid this allocation if we pass in the universe suffix. Your call on if that hurts readability too much.
2 participants
Towards #3646