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

Initial support for TLS #247

Merged
merged 34 commits into from
Oct 27, 2023
Merged

Initial support for TLS #247

merged 34 commits into from
Oct 27, 2023

Conversation

minghuaw
Copy link
Contributor

@minghuaw minghuaw commented Oct 25, 2023

An initial attempt to add TLS support for gRPC.

Motivation

#6

Solution

Two new features flags are added "rustls" and "native-tls" so that user can choose the preferred TLS implementation in the community. A new client/server example that uses "rustls" is added to showcase the usage.

Additionally, Cargo.lock is removed from the tracked files in git.

@minghuaw minghuaw requested review from a team as code owners October 25, 2023 09:42
@CLAassistant
Copy link

CLAassistant commented Oct 25, 2023

CLA assistant check
All committers have signed the CLA.

@PureWhiteWu
Copy link
Member

Wow! This is a really great pr to implement an important feature! Thanks very much for your contribution!

@PureWhiteWu PureWhiteWu added A-volo-grpc This issue concerns the `volo-grpc` crate. C-enhancement This is a PR that adds a new feature or fixes a bug. labels Oct 26, 2023
examples/src/hello_grpc_tls/grpc_tls_client.rs Outdated Show resolved Hide resolved
examples/Cargo.toml Outdated Show resolved Hide resolved
examples/src/hello_grpc_tls/grpc_tls_server.rs Outdated Show resolved Hide resolved
volo-grpc/Cargo.toml Outdated Show resolved Hide resolved
volo-grpc/src/lib.rs Outdated Show resolved Hide resolved
volo/Cargo.toml Outdated Show resolved Hide resolved
volo/src/net/dial.rs Show resolved Hide resolved
volo/src/net/dial.rs Outdated Show resolved Hide resolved
volo/src/net/dial.rs Outdated Show resolved Hide resolved
volo/src/net/dial.rs Outdated Show resolved Hide resolved
@PureWhiteWu
Copy link
Member

Furthermore, I think we should still preserve the Cargo.lock, this is useful for cooperation, because everyone maintaining this project can have the same build environment and reproduce the build.

@minghuaw
Copy link
Contributor Author

Furthermore, I think we should still preserve the Cargo.lock, this is useful for cooperation, because everyone maintaining this project can have the same build environment and reproduce the build.

I doubt this though. I personally do not remember other open source library crates doing the same thing. Plus, I have just looked through a few closed PRs, and seems like Cargo.lock appear quite frequently in the PRs. That doesn't sounds like it's "keeping the same build environment".

@minghuaw
Copy link
Contributor Author

minghuaw commented Oct 26, 2023

Which version of pilota and motore should be used? Trying to compile the examples but getting errors that afit and rpitit are not enabled in those two crates

Edit: Nvm, updated my nightly toolchain and this problem is gone. Should there be a minimum toolchain version somewhere though?

@PureWhiteWu
Copy link
Member

I doubt this though. I personally do not remember other open source library crates doing the same thing. Plus, I have just looked through a few closed PRs, and seems like Cargo.lock appear quite frequently in the PRs. That doesn't sounds like it's "keeping the same build environment".

In fact, we have tried to remove the Cargo.lock before, but we encountered some problems, such as I have updated the dependency in my dev environment, but other people didn't, so after they pull the code, they will encounter compile failure, and may lead to extra communication cost.

@PureWhiteWu
Copy link
Member

Should there be a minimum toolchain version somewhere though?

We are supposed to have a MSRV of 1.75 in the future. But for now, we recommend to use the latest nightly.

@minghuaw
Copy link
Contributor Author

I doubt this though. I personally do not remember other open source library crates doing the same thing. Plus, I have just looked through a few closed PRs, and seems like Cargo.lock appear quite frequently in the PRs. That doesn't sounds like it's "keeping the same build environment".

In fact, we have tried to remove the Cargo.lock before, but we encountered some problems, such as I have updated the dependency in my dev environment, but other people didn't, so after they pull the code, they will encounter compile failure, and may lead to extra communication cost.

I still think there are better ways to get around this, but if that's really troubling for you, I am fine with tracking Cargo.lock with git (fixed in 6b9c8d5).

Copy link
Member

@PureWhiteWu PureWhiteWu left a comment

Choose a reason for hiding this comment

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

LGTM,Thanks for your contribution!
@bobozhengsir @Millione PTAL

@PureWhiteWu PureWhiteWu merged commit 4981534 into cloudwego:main Oct 27, 2023
9 checks passed
@PureWhiteWu
Copy link
Member

Thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-volo-grpc This issue concerns the `volo-grpc` crate. C-enhancement This is a PR that adds a new feature or fixes a bug.
Development

Successfully merging this pull request may close these issues.

4 participants