Skip to content
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

expose the WithDefaultScheme DialOption #8008

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dprotaso
Copy link

@dprotaso dprotaso commented Jan 13, 2025

This exposes the DialOption WithDefaultScheme

Note: I was hoping to expose this so I could set passthrough scheme on the client without manipulating URLs or setting it globally. Let me know if you think it makes sense to add an option like WithDefaultPassthroughScheme() or something.

I also didn't see a passthrough scheme constant in the code base - should we add one?

RELEASE NOTES: WithDefaultScheme DialOption allows you to configure the scheme for a client

Copy link

codecov bot commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.03%. Comparing base (f35fb34) to head (bdec047).
Report is 44 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8008      +/-   ##
==========================================
- Coverage   82.17%   82.03%   -0.14%     
==========================================
  Files         381      381              
  Lines       38546    38546              
==========================================
- Hits        31676    31623      -53     
- Misses       5560     5602      +42     
- Partials     1310     1321      +11     
Files with missing lines Coverage Δ
clientconn.go 92.14% <100.00%> (-0.64%) ⬇️
dialoptions.go 88.97% <100.00%> (ø)

... and 25 files with indirect coverage changes

@purnesh42H
Copy link
Contributor

@dprotaso Name resolver is picked based on the scheme in the target string. See here https://github.com/grpc/grpc-go/tree/master/examples/features/name_resolving.

Could you clarify more how using dial option is more preferred for your use case than modifying the url? With grpc.NewClient the default scheme is dns which means resolution will happen at client. grpc.Dial (deprecated now) uses passthrough as default scheme which means resolution will not happen at client.

@purnesh42H purnesh42H added Status: Requires Reporter Clarification Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. Type: Feature New features or improvements in behavior labels Jan 15, 2025
@purnesh42H
Copy link
Contributor

@dprotaso gentle ping on your use case

@dprotaso
Copy link
Author

dprotaso commented Jan 20, 2025 via email

@purnesh42H
Copy link
Contributor

purnesh42H commented Jan 21, 2025

You can modify the url in the custom dialer that you are passing to grpc.WithContextDialer(), e.g. "passthrough:target" and return the net.Conn accordingly.

EDIT: Ignore this. This won't work as resolution would have already happened. I will check with other maintainers if there was a reason why withDefaultScheme was not exported

@purnesh42H
Copy link
Contributor

If you are using proxy, then there is an existing bug that we are working on fixing #7556

@purnesh42H
Copy link
Contributor

purnesh42H commented Jan 27, 2025

@dprotaso After discussing with the maintainers, we've concluded that a custom dialer option to skip the default resolver isn't something we anticipate users needing frequently. The passthrough scheme, while still present for backward compatibility, is generally not recommended. So, the alternatives are to prefix passthrough to target URL or use grpc.Dial.

However, we're very interested in understanding the specific requirement that's leading you to use a custom dialer and bypass the default resolver. We want to see if that could potentially be addressed in a more standard and maintainable way.

Could you elaborate on the following:

  • What problem are you trying to solve by bypassing the default resolver and using custom dialer? Please describe the specific use case and why the standard resolution mechanisms aren't suitable.
  • Are you aware that this workaround might not be portable to other gRPC languages?
  • Have you considered implementing a custom resolver? Although it is experimental and should not be the first choice over default grpc resolvers, it is still a more integrated way to achieve your goal within the gRPC framework.

@dprotaso
Copy link
Author

dprotaso commented Jan 27, 2025 via email

@purnesh42H
Copy link
Contributor

purnesh42H commented Jan 29, 2025

You’ve marked grpc.Dial as deprecated - if you recommend people to use it
then you should remove the deprecated annotation.

yeah its not recommended. It exists only for backward compatibility.

We’re using a custom dialer in testing where external DNS isn’t setup - nor
should it be.

You can either have a custom resolver

or use our manual resolver https://github.com/grpc/grpc-go/blob/master/resolver/manual/manual.go#L19. It allows you to manually send resolved addresses to ClientConn. It is used in many tests in grpc-go repo. Just to callout this should be used only for testing. Here is one example test using manual resolver

func (s) TestChannelIdleness_Enabled_NoActivity(t *testing.T) {
.

Basically, you Initialize the manual resolver with a unique scheme (e.g., "test"). The scheme is arbitrary but must match the one used in your target uri

r := manual.NewBuilderWithScheme("test")

If you already know the address, you can set the addresses you want the client to use.

r.InitialState(resolver.State{
    Addresses: []resolver.Address{
        {Addr: "127.0.0.1:50051"}, // Directly specify IP:port
    },
})

You can then create the grpc client using manual resolver

cc, err := grpc.NewClient(r.Scheme()+":///test.server", ....., grpc.WithResolvers(r))

Anytime if you want, you can start another test backend and push an address update via the resolver.

 lis, err := net.Listen("tcp", ":0") // Random port
 ...
 srv := grpc.NewServer()
 go srv.Serve(lis)
 defer srv.Stop()
 ....
r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr.().String()}}})

@dprotaso
Copy link
Author

You still have to manipulate the URL. It seems like it would still be beneficial to have a WithDefaultScheme option here

cc, err := grpc.NewClient(r.Scheme()+":///test.server", ....., grpc.WithResolvers(r))

@dfawley
Copy link
Member

dfawley commented Jan 29, 2025

@dprotaso it would be helpful to start with your use case and why you are using the passthrough resolver at all.
"Passthrough" only exists because gRPC-Go made some mistakes with its v1 implementation, and it was how we chose to bridge the gap for backward compatibility and also match up with the cross-language gRPC design. There is no passthrough resolvers in other languages, so we cannot recommend using it. I also would try to avoid using the manual name resolver (it was only meant for testing), and, as of now, custom resolvers are experimental, as we intend to make breaking changes in it.

@dprotaso
Copy link
Author

dprotaso commented Jan 29, 2025

it would be helpful to start with your use case and why you are using the passthrough resolver at all.

We are performing e2e tests in Knative - we test grpc workloads that go through a proxy (envoy). We require a hostname in order for the proxy to route to the correct grpc backend.

The host name isn't resolvable because we don't want external dns to be involved in our e2e testing. Hence why we were using Dial before it was deprecated.

Copy link

github-actions bot commented Feb 4, 2025

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Feb 4, 2025
@dprotaso
Copy link
Author

dprotaso commented Feb 4, 2025

Do you need any further clarification?

@github-actions github-actions bot removed the stale label Feb 4, 2025
@dfawley
Copy link
Member

dfawley commented Feb 4, 2025

For stubbing out things for testing, and for ensuring the target string is directly sent to a custom Dialer, which I assume you're also using, passthrough is reasonable. But then what's bad about:

cc, err := grpc.NewClient("passthrough:///test.server", ...)
// or
cc, err := grpc.NewClient("passthrough:///"+hostname, ...)

You say you don't want to rewrite URLs, but I don't really see a simple string concatenation as "rewriting URLs".

@dprotaso
Copy link
Author

dprotaso commented Feb 4, 2025

Circling back to an earlier point someone made

After discussing with the maintainers, we've concluded that a custom dialer option to skip the default resolver isn't something we anticipate users needing frequently. The passthrough scheme, while still present for backward compatibility, is generally not recommended. So, the alternatives are to prefix passthrough to target URL or use grpc.Dial.

How do other languages skip name resolution?

@dfawley
Copy link
Member

dfawley commented Feb 4, 2025

How do other languages skip name resolution?

Parts of your use case are still unclear to me. What is your dialer connecting to / how does it work? It's e2e and you mentioned an envoy proxy, so I assume it's not staying within the same binary?

Java and C++ both offer an "in-memory" transport that could be used within the same process. I don't believe either of them offer anything like the custom dialer in Go, if you are needing to do something unusual to actually create the connection.

Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Feb 11, 2025
@dprotaso
Copy link
Author

Parts of your use case are still unclear to me. What is your dialer connecting to / how does it work?

We're spinning up a workload on a Kubernetes cluster. This workload is behind an envoy proxy ingress.

It's e2e and you mentioned an envoy proxy, so I assume it's not staying within the same binary?

No our test runner is on a separate cluster. We don't want to perform DNS resolution so we dial directory to the LB IP and use Host name headers/authority to indicate the destination of the traffic.

@github-actions github-actions bot removed the stale label Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. Status: Requires Reporter Clarification Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants