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

grpc-js: Fix interactions between proxy code and new URI parsing #1375

Merged

Conversation

murgatroid99
Copy link
Member

There were multiple issues here:

  • We expect a proxy URL in the form http://host:port, but this does not actually fit with our naming scheme, so the URI parser parses it wrong. The solution is to revert to a previous version of getProxyInfo that parses the proxy address using the URL class.

  • If the target had no scheme and was parsed weirdly into the GrpcUri structure, the resolver knew how to deal with that, but the proxy didn't, and it was making a bad request to the proxy. The solution is to stop using a default resolver, and instead modify the target at channel construction time so that it has a known scheme.

  • The channel constructor was just overwriting the target after it got modified by the proxy.

  • The proxy map result target didn't have an explicit scheme, even though it would definitely need to be interpreted as a DNS name. Added a scheme to fix that.

@murgatroid99 murgatroid99 merged commit ec82d9c into grpc:master Apr 21, 2020
@ThWoywod
Copy link

It looks like like, that this fix do not really work.

I get the following error message:
image

@murgatroid99
Copy link
Member Author

@ThWoywod Can you run your code with the environment variables GRPC_TRACE=all and GRPC_VERBOSITY=DEBUG, and share the log output?

@murgatroid99
Copy link
Member Author

Never mind on the trace information. I believe I solved the problem in #1381. This time I managed to test it and I did successfully traverse a proxy and get a response from the server.

@ThWoywod
Copy link

Yeah thanks, with v 1.0.2 it works pretty well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants