Skip to content

Feature http transport #296

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.

Sign up for GitHub

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

Open
ii64 wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Feature http transport #296

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3c5be1f
chore: update deps
ii64 Dec 23, 2023
b468f0e
feat: implement http transport
ii64 Dec 23, 2023
c777b22
feat: match handler http transport related grpc
ii64 Dec 23, 2023
b1adc65
feat: match handler http transport related thrift
ii64 Dec 23, 2023
0bdd28c
chore: lint
ii64 Dec 23, 2023
07bef72
chore: lint
ii64 Dec 23, 2023
a39d228
chore: hyper update, reqwest header adapter
ii64 Dec 23, 2023
13b331d
fix: builder header on multiplex
ii64 Dec 23, 2023
2ce7eec
fix: unhandled match arm for http makeincoming
ii64 Dec 23, 2023
887c81e
fix: pass favor_dual_stack for http
ii64 Dec 23, 2023
433b695
chore: downgrade rustls
ii64 Dec 24, 2023
54b7a1e
fix: install pkg-config, libssl-dev on CI
ii64 Dec 24, 2023
7466afc
fix: reuse buffered write duplex transport
ii64 Dec 25, 2023
cc11eb0
chore: remove unused commented code
ii64 Dec 27, 2023
ac00381
feat: vendored flag for volo and CI check
ii64 Dec 28, 2023
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: 6 additions & 0 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ jobs:
# - uses: Swatinem/rust-cache@v1
- name: Run tests
run: |
apt install -y pkg-config libssl-dev
Copy link
Copy Markdown
Contributor

@yukiiiteru yukiiiteru Dec 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using vendored for openssl-sys?

Copy link
Copy Markdown
Member Author

@ii64 ii64 Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not really sure how we can set a transitive dependency features, it's a dependency of tokio-native-tls

Copy link
Copy Markdown
Contributor

@yukiiiteru yukiiiteru Dec 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. tokio-native-tls has a feature vendored for enabling native-tls/vendored
  2. native-tls has a feature vendored for enabling openssl/vendored
  3. openssl has a feature vendored for enabling ffi/vendored, and note that the ffi is openssl-sys in the same repository
  4. openssl-sys has a feature vendored through which openssl can use its own openssl implementation instead of libssl-dev

It's complicated and I don't like them 🥲 but it works.

Copy link
Copy Markdown
Member Author

@ii64 ii64 Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But actually with that, we can let the user decide if they want to use vendored (and statically link the library) or just go by the system package by adding our:

[features]
vendored = ["tokio-native-tls/vendored"]

What do you think?

Copy link
Copy Markdown
Contributor

@yukiiiteru yukiiiteru Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a feature vendored to enable tokio-native-tls/vendored is the best way, users can decide by themselves, and we can use vendored in CI.

Copy link
Copy Markdown
Member Author

@ii64 ii64 Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, ref for that change @ ac00381

Copy link
Copy Markdown
Member Author

@ii64 ii64 Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I'll open a new PR for that

cargo check --features multiplex
cargo check --features rustls,native-tls
cargo test

test-linux-aarch64:
Expand All @@ -47,7 +49,9 @@ jobs:
# - uses: Swatinem/rust-cache@v1
- name: Run tests
run: |
apt install -y pkg-config libssl-dev
cargo check --features multiplex
cargo check --features rustls,native-tls
cargo test

test-macos:
Expand All @@ -62,6 +66,7 @@ jobs:
- name: Run tests
run: |
cargo check --features multiplex
cargo check --features rustls,native-tls
cargo test

test-windows:
Expand All @@ -76,6 +81,7 @@ jobs:
- name: Run tests
run: |
cargo check --features multiplex
cargo check --features rustls,native-tls
cargo test

lint:
Expand Down
128 changes: 49 additions & 79 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pilota-thrift-parser = "0.10"
motore = "0.4"
# motore = { git = "https://github.com/cloudwego/motore", branch = "main" }

metainfo = "0.7"
metainfo = "0.7.7"

anyhow = "1"
async-broadcast = "0.6"
Expand Down Expand Up @@ -62,6 +62,7 @@ http-body-util = "0.1"
hyper = "1"
hyper-timeout = "0.5"
hyper-util = "0.1.2"
reqwest = { version = "0.11", features = ["json", "stream"] }
itertools = "0"
lazy_static = "1"
libc = "0.2"
Expand Down Expand Up @@ -110,9 +111,9 @@ url_path = "0.1"
walkdir = "2"

# Optional dependencies
librustls = { package = "rustls", version = "0.22" }
rustls-pemfile = "2"
tokio-rustls = "0.25"
librustls = { package = "rustls", version = "0.21" }
rustls-pemfile = "1"
tokio-rustls = "0.24"
Comment thread
ii64 marked this conversation as resolved.
Outdated
tokio-native-tls = "0.3"

[profile.release]
Expand Down
4 changes: 4 additions & 0 deletions volo-grpc/src/transport/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ impl<U> ClientTransport<U> {
rpc_config.connect_timeout,
rpc_config.read_timeout,
rpc_config.write_timeout,
None,
);
let mut http_client = http2::Builder::new(TokioExecutor::new());
http_client
Expand Down Expand Up @@ -79,6 +80,7 @@ impl<U> ClientTransport<U> {
rpc_config.connect_timeout,
rpc_config.read_timeout,
rpc_config.write_timeout,
None,
);
let mut http_client = http2::Builder::new(TokioExecutor::new());
http_client
Expand Down Expand Up @@ -231,6 +233,8 @@ fn build_uri(addr: Address, path: &str) -> hyper::Uri {
.path_and_query(path)
.build()
.expect("fail to build unix uri"),
Address::Http(url) => hyper::Uri::try_from(url.to_string())
.expect("fail to build http uri"),
}
}

Expand Down
2 changes: 1 addition & 1 deletion volo-grpc/src/transport/connect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ impl Connector {

#[cfg(any(feature = "rustls", feature = "native-tls"))]
pub fn new_with_tls(cfg: Option<Config>, tls_config: ClientTlsConfig) -> Self {
let mut mt = TlsMakeTransport::new(cfg.unwrap_or_default(), tls_config);
let mut mt = TlsMakeTransport::new(cfg.clone().unwrap_or_default(), tls_config);
if let Some(cfg) = cfg {
mt.set_connect_timeout(cfg.connect_timeout);
mt.set_read_timeout(cfg.read_timeout);
Expand Down
1 change: 1 addition & 0 deletions volo-thrift/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ bytes.workspace = true
chrono.workspace = true
futures.workspace = true
fxhash.workspace = true
http.workspace = true
lazy_static.workspace = true
linkedbytes.workspace = true
linked-hash-map.workspace = true
Expand Down
Loading