-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
NewClient functions behaviour is incompatible with secure forward-proxies #7556
Comments
If you need a short-term workaround to keep things working as they were before, you should be able to use the passthrough resolver: client, err := grpc.NewClient("passthrough:///<hostname>:<port>", ...) That probably should not be required, though. |
@dfawley I get that this would work in most of the cases. |
Are you saying the workaround of using |
No, I didn't mean passthrough won't work. But I think NewClient api should have greater flexibility to be able to configure dns versus passthrough. Since we use opentelemetry, the real grpc-go integration is way down the stack and it's hard to configure the url this way as the same url gets used at multiple places. |
Thanks for confirming. Yes, this should be treated as a bug and the workaround was not suggested to avoid fixing it. |
For reference, this gRFC comes into play here: https://github.com/grpc/proposal/blob/master/A1-http-connect-proxy-support.md But note that Java did not implement this gRFC, and we may or may not want to do things this way. |
Java added support for "Use Case 1". But it did not use the gRFC's design. grpc/grpc-java#10022 tracks implementing Use Case 2 in Java. |
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead DialContext, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. can be changed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead DialContext, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. Passthrough scheme was the prior default and prevents resolution to happen beforehand. This change can be removed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead DialContext, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. can be changed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead Dial, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. This can be changed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead Dial, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. This can be changed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead Dial, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. This can be changed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
Keeping this issue open to track the fix. |
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead Dial, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. This can be changed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
We also ran into this issue. It's very easy to reproduce. Just use https://github.com/grpc/grpc-go/tree/master/examples/helloworld/greeter_client and run nc to mock the proxy:
If using 1.58.x, you will see the nc outputs: If using 1.66.x, you will see: |
https://github.com/grpc/grpc-go/blob/master/internal/transport/http2_client.go#L181 |
Hey @guyni , I think the using |
@eshitachandwani If you look at the usecase no. 1 here, it mentions that the external address should be resolved at the proxy. That means the hostname should be supplied in |
Hi @puneet-traceable, currently, gRPC-Go supports only use case 2, and I’m actively working on adding support for use case 1. Apologies for not mentioning this in my previous comment. |
This happens because in v1.58.x we were using |
With NewClient API usage, we are facing issues at few customers who have intermediate proxies between collector and platform. With NewClient API instead Dial, DNS resolution happens on the client side while it should happen on proxy. Also, with SGProxy client does not get the correct certificate. This can be changed once grpc fixes grpc/grpc-go#7556 and otel collector picks the fix
Thanks @eshitachandwani for handing this issue. Didn't find if it was already mentioned but this issue impacts consumers of the GCP Go SDK (https://github.com/googleapis/google-cloud-go), can we make sure any solution applies for those consumers as well (e.g. if the solution is by adding a new option, that consumers of the GCP Go SDK can enable the option)? |
@erezrokah : Could you please elaborate on your last comment? How does the GCP Go SDK currently use the proxy feature? Are they currently affected by using Thanks. |
They are currently affected due to this change googleapis/google-cloud-go@be2d56d#diff-215847f913454f2311866edbdbf41d7a3cd3879e0ed0e7aa26f4ad771dd6a1dcR296. We're experiencing the same issue as this one only when using the Google Cloud Go SDK. I think https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#the-wrong-way-grpcdial should say both Edit |
I can think of a workaround which doesn't involve a code change in the googleapis client library: import (
"google.golang.org/grpc/resolver"
"google.golang.org/grpc"
"google.golang.org/api/option"
)
// A resolver that registers the passthrough resolver under the "dns" scheme
// to indirectly set the default resolver used by NewClient as the passthrough
// resolver.
// Note: This will shadow the dns resolver.
type customPassthroughResolver struct{}
func (r *customPassthroughResolver) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) {
passthrough := resolver.Get("passthrough")
if passthrough == nil {
panic("Passthrough resolver not registered!")
}
return passthrough.Build(target, cc, opts)
}
func (r *customPassthroughResolver) Scheme() string {
return "dns"
}
// Use a grpc client option to avoid modifying the global grpc resolver registry.
clientOption := option.WithGRPCDialOption(grpc.WithResolvers(&customPassthroughResolver{}))
// Use the client option while creating the googleapis client. |
Regarding your workaround above, @erezrokah asked (on googleapis/google-cloud-go#11089): Won't this workaround disable DNS resolving altogether even when explicitly set by the scheme? |
@quartzmo, I'm not suggesting that the Go client library uses this workaround. This workaround is hacky. The Go client library should probably revert back to using
Yes, it will replace the DNS resolver with the passthrough resolver, even if the target URI uses dns as the scheme. Since the workaround uses a call option instead of registering a balancer globally, a user should add the call option only when they don't want to use the dns scheme. A less hacky fix would be to use the DNS resolution happens when using both
The passthrough resolver exists for backward compatibility. |
@erezrokah the fact that the |
We plan to update the release notes for recent releases. |
There have been a couple of reports lately around some subtle changes in behaviour of using the new recommend method. There is also a known bug, see grpc/grpc-go#7556. Until the bug is fixed and we have a way forward we will revert to calling the deprecated Dial function. Fixes: googleapis#7556
Thanks for the additional context I thought this was intended behavior. Updating the release notes and having a fixed merged would be great as currently the deprecation of |
Our understanding of the problem : The proposal to resolve the issue:
It is based on the gRFC A1 |
@eshitachandwani I'm concerned about this: "Remove environment variable detection from the transport layer. Instead, introduce a per-address attribute to specify the proxy CONNECT string. This change aligns with the xDS design and provides more control over proxy behavior." Some users have extensive use of the Adding a new, more flexible, feature is surely harmless, but if the rug is pulled out under |
Hey @bushnell-deshaw , sorry for the confusion. We are not pulling the plug on use of |
What version of gRPC are you using?
1.64.0 and v1.67.0-dev
What version of Go are you using (
go version
)?1.22
What operating system (Linux, Windows, …) and version?
Linux
What did you do?
If possible, provide a recipe for reproducing the error.
What did you expect to see?
the target should be hostname while it's sent to proxy and dns resolution for target should happen on proxy
What did you see instead?
dns is resolved on the client and only ip is sent.
Attaching tcpdump screenshot with difference
tcpdump for curl
The text was updated successfully, but these errors were encountered: