Skip to content

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

Merged
murgatroid99 merged 6 commits intogrpc:masterfrom
murgatroid99:grpc-js_proxy_structured_uri_fixes
Apr 21, 2020
Merged

grpc-js: Fix interactions between proxy code and new URI parsing#1375
murgatroid99 merged 6 commits intogrpc:masterfrom
murgatroid99:grpc-js_proxy_structured_uri_fixes

Conversation

@murgatroid99
Copy link
Copy Markdown
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
Copy Markdown

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

I get the following error message:
image

@murgatroid99
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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