Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

fix: Enforce http2, fix dfx local server not supported by ic-rosetta-api #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Enforce http2, fix dfx local server not supported by ic-rosetta-api #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,9 @@ async fn forward_api(
) -> Result<Response<Body>, Box<dyn Error>> {
let proxied_request = create_proxied_request(ip_addr, replica_url, request)?;

let client = Client::builder().build(hyper_tls::HttpsConnector::new());
let client = Client::builder()
.http2_only(true)
.build(hyper_tls::HttpsConnector::new());
let response = client.request(proxied_request).await?;
Ok(response)
}
Expand Down Expand Up @@ -555,7 +557,7 @@ fn main() -> Result<(), Box<dyn Error>> {
.enable_all()
.build()?;
runtime.block_on(async {
let server = Server::bind(&opts.address).serve(service);
let server = Server::bind(&opts.address).http2_only(true).serve(service);
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Contributor

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)?

Copy link
Member

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.

server.await?;
Ok(())
})
Expand Down