delegatingresolver: add default port to addresses #8613
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8613 +/- ##
==========================================
- Coverage 82.00% 81.97% -0.03%
==========================================
Files 415 415
Lines 40697 40719 +22
==========================================
+ Hits 33372 33380 +8
- Misses 5937 5970 +33
+ Partials 1388 1369 -19
🚀 New features to boost your workflow:
|
| addr, err := parseTarget(target.Endpoint()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) | ||
| } |
There was a problem hiding this comment.
I think we should guard this call to parseTarget behind the feature flag since it can potentially lead to new/different failures after this PR is merged.
There was a problem hiding this comment.
We can have only one block guarded by the feature flag if we instead do the following:
addr := target.Endpoint()
if target.URL.Scheme == "dns" && !targetResolutionEnabled && envconfig.AddDefaultPort {
addr, err = parseTarget(target.Endpoint(), defaultPort)
if err != nil {
// return some error
}
}This way we can use the parseTarget function from the dns resolver without any modifications.
There was a problem hiding this comment.
My thinking was we should do the basic check and add localhost for all addresses if no host is present irrespective of the resolver being used, but adding default port 443 is only for DNS resolver with target resolution disabled. And the first part should not be behind the flag , but only 2nd part should. That is my understanding, let me know if I am missing something.
There was a problem hiding this comment.
From my understanding, only the DNS resolver was defaulting the hostname to localhost. Applying this logic to all resolvers would be a behaviour change which could potentially break some custom resolver that isn't expecting this. I would recommend avoiding such a change to be safe.
Also, since we're providing a env variable flag, we should ensure the flag can revert all (if not most) changes in this PR.
internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
Outdated
Show resolved
Hide resolved
| name string | ||
| target string | ||
| wantConnectAddress string | ||
| wantErrorSubstring string |
There was a problem hiding this comment.
Can you add another test param for the env variable and tests cases for the env variable being disabled?
There was a problem hiding this comment.
I already have a seperate test for it : TestDelegatingResolverEnvVarForDefaultPortDisable
There was a problem hiding this comment.
Can we combine the tests using an additional param? That should help reduce duplicate code.
internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
Outdated
Show resolved
Hide resolved
internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
Outdated
Show resolved
Hide resolved
| disableDefaultPortEnvVar bool | ||
| }{ | ||
| { | ||
| name: "no port in target", |
There was a problem hiding this comment.
Subtest names still have spaces which make them hard to search for when you get here from a failing test run.
There was a problem hiding this comment.
Ohhh... I misunderstood some parts. Got it!
internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
Outdated
Show resolved
Hide resolved
| } | ||
| overrideTestHTTPSProxy(t, envProxyAddr) | ||
|
|
||
| targetResolver := manual.NewBuilderWithScheme("dns") |
There was a problem hiding this comment.
Don't you have to unregister the original dns resolver and re-register it at the end of the test, for this test to not affect other tests?
| targetResolver := manual.NewBuilderWithScheme("dns") | ||
| target := targetResolver.Scheme() + ":///" + test.target | ||
| // Set up a manual DNS resolver to control the proxy address resolution. | ||
| proxyResolver, proxyResolverBuilt := setupDNS(t) |
There was a problem hiding this comment.
Oh I see that this helper does re-register the original dns resolver. But maybe this section can still be a little more explicit/readable etc. I'm not able to think of the exact way to make this better right now.
There was a problem hiding this comment.
This is what we had earlier too..in the tests.
internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
Outdated
Show resolved
Hide resolved
| // | ||
| // An error is returned for empty targets or targets with a trailing colon | ||
| // but no port (e.g., "host:"). | ||
| func parseTarget(target string) (string, error) { |
There was a problem hiding this comment.
I don't know if you have the appetite to change this to follow the more idiomatic way in Go, which is to check for errors (instead of checking for the absence of them).
Something like this seems much more readable to me and more idiomatic:
func parseTarget(target string) (string, error) {
if target == "" {
return "", fmt.Errorf("missing address")
}
host, port, err := net.SplitHostPort(target)
if err != nil {
// If SplitHostPort fails, it's likely because the port is missing.
// We append the default port and return the result.
return net.JoinHostPort(target, defaultPort), nil
}
// If SplitHostPort succeeds, we check for edge cases.
if port == "" {
// A success with an empty port means the target had a trailing colon,
// e.g., "host:", which is an error.
return "", fmt.Errorf("missing port after port-separator colon")
}
if host == "" {
// A success with an empty host means the target was like ":80".
// We default the host to "localhost".
host = "localhost"
}
return net.JoinHostPort(host, port), nil
}| defaultPortEnvVar: true, | ||
| }, | ||
| { | ||
| name: "no port with default port env variable diabled", |
Fixes: #8607
RELEASE NOTES:
GRPC_EXPERIMENTAL_RESOLVER_ADD_DEFAULT_PORTfor adding a default port to addresses being sent to proxy which is set by default.