passthrough: return error if endpoint is empty and opt.Dialer is nil when building resolver #5732

huangchong94 · 2022-10-19T07:08:15Z

Empty string is clearly not a valid target when default scheme is passthrough and no custom dialer set. But grpc.Dial will not return error in this case until client starts making RPCs, user will get errors saying transport: Error while dialing dial tcp: missing address.

It would be better fail fast and notify user empty target is invalid before RPC calls.

This PR fixes this issue by checking 1. is endpoint empty 2. is there a custom dialer in passthroughBuilder.Build, if endpoint is empty and there is no custom dialer, an error will be returned.

RELEASE NOTES:

client: return an error from Dial if an empty target is passed and no custom dialer is present; the ClientConn would otherwise be unable to connect and perform RPCs

…when building passthrough resolver

dfawley

LGTM except for the colon character issue.

dfawley · 2022-10-19T16:54:31Z

internal/resolver/passthrough/passthrough.go


 const scheme = "passthrough"

 type passthroughBuilder struct{}

 func (*passthroughBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) {
+	if target.Endpoint == "" && opts.Dialer == nil {
+		return nil, errors.New("passthrough resolver：missing address")


The ":" character looks wrong. Please make sure you are using the proper ASCII character (0x3A), and there should be a space after it.

dfawley · 2022-10-19T16:57:01Z

internal/resolver/passthrough/passthrough.go


 const scheme = "passthrough"

 type passthroughBuilder struct{}

 func (*passthroughBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) {
+	if target.Endpoint == "" && opts.Dialer == nil {


I feel like we could do even stronger validation here (verify it could work when sent to net.Dial, e.g. split host/port & parse IP), but this is an improvement anyway.

dfawley · 2022-10-19T17:51:22Z

Somehow this change still makes me nervous. I have a feeling we'll break someone that is doing something unexpected, like using interceptors to circumvent the underlying connection. Maybe this isn't worth doing.

huangchong94 · 2022-10-20T11:30:12Z

Somehow this change still makes me nervous. I have a feeling we'll break someone that is doing something unexpected, like using interceptors to circumvent the underlying connection. Maybe this isn't worth doing.

If we consider empty string is not a valid passthrough target without custom dialer, I think we should return error early in grpc.Dial, even if someone is using interceptors to circumvent the underlying connection, maybe we don't need maintain compatibility for this kind of usage?

Just like if someone is using an invalid dns target eg. dns:///foo:, grpc.Dial will return error, no matter what interceptor he is using.

huangchong94 · 2022-10-20T12:38:46Z

To provide more context: We're using grpc-go building microservices, and we usually initialize rpc clients during service startup, if initialization fail, server process will exit immediately.

When we deployed a new feature someone forgot to fill out some config struct in some config file, which caused target become empty string, grpc.Dial didn't return error and service started successfully, everything looked fine, but as new feature rolling out, traffic coming in, we began to receive alert messages saying transport: Error while dialing dial tcp: missing address. In this case, it would be better fail early and fail fast during startup time than getting errors during running time.

Certainly we will add more validation logic in our services, but if this validation is done by grpc-go itself, many others will benefit from it too.

dfawley · 2022-10-21T15:53:11Z

Don't get me wrong, I agree with the changes here from a theoretical standpoint. However, we need to be very careful to not break any reasonable use cases. Maybe this should wait until #1786.

Consider also that, instead of an empty address, your engineer had a typo in the hostname in the target string. In that case, we wouldn't error out early, either. The system you're building needs to be tolerant to these types of situations anyway, so I don't think this is a high priority thing to change.

huangchong94 · 2022-10-26T12:45:17Z

Don't get me wrong, I agree with the changes here from a theoretical standpoint. However, we need to be very careful to not break any reasonable use cases. Maybe this should wait until #1786.

Consider also that, instead of an empty address, your engineer had a typo in the hostname in the target string. In that case, we wouldn't error out early, either. The system you're building needs to be tolerant to these types of situations anyway, so I don't think this is a high priority thing to change.

OK. I understand your concern about compatibility issue. If this PR should wait for #1786, you can close it or if you need me add more validation logic in build, I will find time to implement it.

dfawley · 2022-11-15T17:32:28Z

I talked this over with the other language leads and it looks like in C, it's impossible for the channel to fail at creation time, but in Java the channel can fail to be created, and it fails in the case where the target string is empty. The reason it fails is a bit different, however: it can't determine the authority to set for connections. So, I think we can pretty safely accept this without risk of breaking any realistic use cases. It's possible someone is using a channel with interceptors and not ever actually using the channel itself, but this isn't a supported use case, and there would be workarounds, too.

dfawley · 2022-11-15T17:33:32Z

@easwars, could you please make a second review?

easwars · 2022-11-15T21:44:17Z

internal/resolver/passthrough/passthrough.go


 const scheme = "passthrough"

 type passthroughBuilder struct{}

 func (*passthroughBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) {
+	if target.Endpoint == "" && opts.Dialer == nil {
+		return nil, errors.New("passthrough resolver: missing address")


address here can lead to confusion because an address in this context usually refers to addresses returned by the resolver. Could you please change the error message to be passthrough: received empty target in Build().

…when building resolver (grpc#5732)

yurishkuro · 2023-01-11T04:29:59Z

@dfawley tend to agree with your reasons for not changing the existing behavior, especially given the example with invalid host name. The existing behavior was actually useful for testing error paths, those tests broke after this change. I was able to fix it with invalid host name instead (jaegertracing/jaeger#4149).

With grpc/grpc-go#5732 the grpc library validates the presence of the endpoint information: > client: return an error from Dial if an empty target is passed and no custom > dialer is present; the ClientConn would otherwise be unable to connect and perform RPCs

taylow · 2023-01-11T13:33:34Z

As @huangchong94 mentioned, a missing config now causes this to error much earlier in the process, as we have just found out. I think this is much clearer/more reliable, though the error message wasn't clear until doing a bit of poking around 😅 I understand the reason for not using the term address, and that the parameter for Dial is called target, but it did require us to search for the error to figure out why our services weren't starting.

With grpc/grpc-go#5732 the grpc library validates the presence of the endpoint information: > client: return an error from Dial if an empty target is passed and no custom > dialer is present; the ClientConn would otherwise be unable to connect and perform RPCs

* fix(deps): update module google.golang.org/grpc to v1.52.0 * `make package-specs` * Provide fake endoint in tests With grpc/grpc-go#5732 the grpc library validates the presence of the endpoint information: > client: return an error from Dial if an empty target is passed and no custom > dialer is present; the ClientConn would otherwise be unable to connect and perform RPCs Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Silvestre Zabala <silvestre.zabala@sap.com>

yurishkuro · 2023-01-11T18:46:12Z

@taylow may be more useful for that specific use case, but the point is that (a) it changed the existing behavior, thus breaking other stuff, and (b) it's inconsistent since "missing config" fails immediately, but "config with typo" still behaves as before and only fails on attempts to send.

marcelosousa · 2023-01-16T18:06:20Z

As with @yurishkuro, I found that the previous behaviour was useful for testing purposes.
This change does breaks the tests in the Go library I'm working on (reviewpad/reviewpad#610) and I find it quite hackish to introduce dummy host names.

- Remove unused BOSH property and associated config field from traffic controller - Pass GRPCAddress rather than UDPAddress when creating a test server. gRPC-Go v1.52.0+ will error if the provided endpoint is empty: grpc/grpc-go#5732

- Remove unused BOSH property and associated config field from traffic controller - Pass GRPCAddress rather than UDPAddress when creating a test server. gRPC-Go v1.52.0+ will error if the provided endpoint is empty: grpc/grpc-go#5732 (cherry picked from commit 63cf49e)

huangchong94 changed the title ~~passthrough: return error if endpoint is empty and opt.Dialer is nil when building passthrough resolver~~ passthrough: return error if endpoint is empty and opt.Dialer is nil when building resolver Oct 19, 2022

passthrough: return error if endpoint is empty and opt.Dialer is nil …

d01bd83

…when building passthrough resolver

huangchong94 force-pushed the dial-empty-target branch from cb62a9e to d01bd83 Compare October 19, 2022 07:39

dfawley reviewed Oct 19, 2022

View reviewed changes

dfawley assigned huangchong94 Oct 19, 2022

dfawley added the Status: Requires Reporter Clarification label Oct 19, 2022

dfawley added this to the 1.51 Release milestone Oct 19, 2022

dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Oct 19, 2022

passthrough: fix non ASCII error string

3260f78

dfawley removed the Status: Requires Reporter Clarification label Oct 25, 2022

dfawley assigned dfawley and unassigned huangchong94 Oct 25, 2022

zasweq modified the milestones: 1.51 Release, 1.52 Release Nov 8, 2022

dfawley approved these changes Nov 15, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley Nov 15, 2022

dfawley requested a review from easwars November 15, 2022 17:33

easwars reviewed Nov 15, 2022

View reviewed changes

easwars assigned huangchong94 and unassigned easwars Nov 15, 2022

easwars added the Status: Requires Reporter Clarification label Nov 15, 2022

passthrough: update error message in build

73b40cf

easwars approved these changes Nov 16, 2022

View reviewed changes

easwars removed the Status: Requires Reporter Clarification label Nov 16, 2022

easwars merged commit 817c1e8 into grpc:master Nov 16, 2022

jronak pushed a commit to jronak/grpc-go that referenced this pull request Nov 21, 2022

passthrough: return error if endpoint is empty and opt.Dialer is nil …

f7ba15d

…when building resolver (grpc#5732)

acrmp mentioned this pull request Feb 27, 2023

Remove unused metron_endpoint.dropsonde_port property cloudfoundry/loggregator-release#534

Merged

10 tasks

acrmp mentioned this pull request Feb 28, 2023

Remove unused metron_endpoint.dropsonde_port property cloudfoundry/loggregator-release#537

Merged

10 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

passthrough: return error if endpoint is empty and opt.Dialer is nil when building resolver #5732

passthrough: return error if endpoint is empty and opt.Dialer is nil when building resolver #5732

huangchong94 commented Oct 19, 2022 •

edited by easwars

Loading

dfawley left a comment

dfawley Oct 19, 2022

dfawley Oct 19, 2022

dfawley commented Oct 19, 2022

huangchong94 commented Oct 20, 2022 •

edited

Loading

huangchong94 commented Oct 20, 2022 •

edited

Loading

dfawley commented Oct 21, 2022

huangchong94 commented Oct 26, 2022

dfawley commented Nov 15, 2022

dfawley commented Nov 15, 2022

easwars Nov 15, 2022

yurishkuro commented Jan 11, 2023

taylow commented Jan 11, 2023

yurishkuro commented Jan 11, 2023

marcelosousa commented Jan 16, 2023

passthrough: return error if endpoint is empty and opt.Dialer is nil when building resolver #5732

passthrough: return error if endpoint is empty and opt.Dialer is nil when building resolver #5732

Conversation

huangchong94 commented Oct 19, 2022 • edited by easwars Loading

dfawley left a comment

Choose a reason for hiding this comment

dfawley Oct 19, 2022

Choose a reason for hiding this comment

dfawley Oct 19, 2022

Choose a reason for hiding this comment

dfawley commented Oct 19, 2022

huangchong94 commented Oct 20, 2022 • edited Loading

huangchong94 commented Oct 20, 2022 • edited Loading

dfawley commented Oct 21, 2022

huangchong94 commented Oct 26, 2022

dfawley commented Nov 15, 2022

dfawley commented Nov 15, 2022

easwars Nov 15, 2022

Choose a reason for hiding this comment

yurishkuro commented Jan 11, 2023

taylow commented Jan 11, 2023

yurishkuro commented Jan 11, 2023

marcelosousa commented Jan 16, 2023

huangchong94 commented Oct 19, 2022 •

edited by easwars

Loading

huangchong94 commented Oct 20, 2022 •

edited

Loading

huangchong94 commented Oct 20, 2022 •

edited

Loading