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

NewClient functions behaviour is incompatible with secure forward-proxies #7556

Open
puneet-traceable opened this issue Aug 23, 2024 · 8 comments
Assignees
Labels
Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. Type: Bug

Comments

@puneet-traceable
Copy link

puneet-traceable commented Aug 23, 2024

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.

  1. setup a squid proxy
  2. setup grpc client(examples/helloworld/greeter_client) and grpc server(examples/helloworld/greeter_server) to use tls
  3. run grpc server
  4. on client shell set env var for using proxy(export https_proxy="http://<proxy_host:port")
  5. start tcpdump

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
Screenshot 2024-08-23 at 7 50 34 PM

tcpdump for curl
Screenshot 2024-08-23 at 9 48 22 PM

@dfawley
Copy link
Member

dfawley commented Aug 23, 2024

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.

@puneet-traceable
Copy link
Author

@dfawley I get that this would work in most of the cases.
But there are more issues with this change.
For example: if dns lookups are disabled on app instances and it's only the proxy that can resolve the dns, current grpc client built with NewClient does not work. curl for http2 does not face this problem.

@dfawley
Copy link
Member

dfawley commented Aug 27, 2024

But there are more issues with this change.

Are you saying the workaround of using passthrough:///<hostname>:<port> as the target string isn't working for you in these situations? If so, can you provide some more information and debugging logs of that?

@puneet-traceable
Copy link
Author

target

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.

@dfawley
Copy link
Member

dfawley commented Aug 28, 2024

Thanks for confirming. Yes, this should be treated as a bug and the workaround was not suggested to avoid fixing it.

@dfawley
Copy link
Member

dfawley commented Aug 30, 2024

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.

@ejona86
Copy link
Member

ejona86 commented Aug 31, 2024

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.

puneet-traceable added a commit to hypertrace/opentelemetry-collector that referenced this issue Sep 17, 2024
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
puneet-traceable added a commit to hypertrace/opentelemetry-collector that referenced this issue Sep 17, 2024
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
puneet-traceable added a commit to hypertrace/opentelemetry-collector that referenced this issue Sep 18, 2024
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
puneet-traceable added a commit to hypertrace/opentelemetry-collector that referenced this issue Sep 18, 2024
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
puneet-traceable added a commit to hypertrace/opentelemetry-collector that referenced this issue Sep 18, 2024
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
puneet-traceable added a commit to hypertrace/opentelemetry-collector that referenced this issue Sep 18, 2024
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
@arjan-bal
Copy link
Contributor

Keeping this issue open to track the fix.

@arjan-bal arjan-bal reopened this Sep 19, 2024
@purnesh42H purnesh42H added the Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. label Sep 23, 2024
tim-mwangi pushed a commit to hypertrace/opentelemetry-collector that referenced this issue Oct 3, 2024
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
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. Type: Bug
Projects
None yet
6 participants