-
Notifications
You must be signed in to change notification settings - Fork 13
fix: Enforce http2, fix dfx local server not supported by ic-rosetta-api #7
base: main
Are you sure you want to change the base?
Conversation
| .build()?; | ||
| runtime.block_on(async { | ||
| let server = Server::bind(&opts.address).serve(service); | ||
| let server = Server::bind(&opts.address).http2_only(true).serve(service); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, the doc claims that http2 is supported by default:
A listening HTTP server that accepts connections in both HTTP1 and HTTP2 by default.
-- https://docs.rs/hyper/0.14.13/hyper/server/struct.Server.html
I think strictly requiring HTTP2 here might break quite a few clients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of directly setting this to true, can you add a new field to Opts, which defaults to false to keep the current default behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is icx-proxy is often invoked indirectly by dfx, so we don't get to control the command line options passed to it. Would you accept an environment variable configuration to turn this on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just change all the places where dfx runs icx-proxy (i.e. add the flag to those calls)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a command-line option to icx-proxy, and adding parameters to dfx start and dfx bootstrap to control it, sounds like a better option.
|
@TerrorJack, I believe you need to change the PR title to conform to the guideline ( |
6b5ac96 to
5e94d44
Compare
When
dfxstarts a local replica, it automatically startsicx-proxyto reroute the incoming requests. It useshyperfor http logic, andhyperdoesn't properly support http1/http2 dual stack. The mainicrepo assumes http2 in a lot of places, one consequence of that isic-rosetta-apibeing unable to connect to the local replica started bydfx. This patch fixes that problem.See ticket https://dfinity.atlassian.net/browse/ROSETTA1-162 for more context.