Upgrade tonic dependencies to 0.13.0 version #7377
Conversation
github-actions
bot
added
arrow
|
Thank you @rmn-boiko -- looks like there are a few CI failures yet to fix, but otherwise 💯 |
|
Have to hold this PR until prost will support latest tonic version |
rmn-boiko
force-pushed
the
upgrade-tonic-version
branch
from
047326a to
5b6c22d
Compare
|
I've updated PR with the latest codegen changes. Will wait for comments |
There was a problem hiding this comment.
Thanks @rmn-boiko -- this looks good to me. It would be great to add a short doc string about the new router feature though
| [Apache Arrow FlightSQL], a protocol for interacting with SQL databases. | ||
|
|
||
| - `tls`: Enables `tls` on `tonic` | ||
| - `tls`: Enables `_tls-any` on `tonic` |
There was a problem hiding this comment.
Can you please also add dos about the "router" feature?
There was a problem hiding this comment.
As mentioned in previous comments, we should encourage users to enable correct tonic's feature instead.
Or we can re-export the same feature flags.
alamb
added
the
next-major-release
|
Since this updates a dependency we need to wait for the next major arrow release before merging it |
|
|
||
| // Set dafault crypto provider to use | ||
| // See: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html#using-the-per-process-default-cryptoprovider | ||
| rustls::crypto::ring::default_provider(); |
There was a problem hiding this comment.
maybe you need to call
let _ = rustls::crypto::ring::default_provider().install_default();
There was a problem hiding this comment.
As explained in previous comments.
| default = [] | ||
| flight-sql-experimental = ["dep:arrow-arith", "dep:arrow-data", "dep:arrow-ord", "dep:arrow-row", "dep:arrow-select", "dep:arrow-string", "dep:once_cell", "dep:paste"] | ||
| tls = ["tonic/tls"] | ||
| tls = ["tonic/_tls-any"] |
There was a problem hiding this comment.
Maybe we can simply remove this feature and let users enable it directly on tonic. Since it doesn't work even when users enable this feature, they still need to enable ring or aws-lc directly on tonic anyway.
We should document this behavior change.
| http-body = "1.0.0" | ||
| hyper-util = "0.1" | ||
| pin-project-lite = "0.2" | ||
| rustls = { version = "0.23", default-features = false, features = ["ring"] } |
There was a problem hiding this comment.
We should enable it using tonic = { .., features = ["transport", "codegen", "prost", "tls-ring"]} instead of relying directly on rustls.
Tricks happened at
https://docs.rs/tonic/latest/src/tonic/transport/channel/service/tls.rs.html#48-57
#[allow(unreachable_patterns)]
let builder = match crypto::CryptoProvider::get_default() {
Some(provider) => with_provider(provider.clone()),
#[cfg(feature = "tls-ring")]
None => with_provider(Arc::new(crypto::ring::default_provider())),
#[cfg(feature = "tls-aws-lc")]
None => with_provider(Arc::new(crypto::aws_lc_rs::default_provider())),
// somehow tls is enabled, but neither of the crypto features are enabled.
_ => ClientConfig::builder(),
};There was a problem hiding this comment.
I don't really have any knowledge / preference -- but it would be nice to get this PR ready to merge
@MichaelScofield has a slightly different proposal here:
| [Apache Arrow FlightSQL], a protocol for interacting with SQL databases. | ||
|
|
||
| - `tls`: Enables `tls` on `tonic` | ||
| - `tls`: Enables `_tls-any` on `tonic` |
There was a problem hiding this comment.
As mentioned in previous comments, we should encourage users to enable correct tonic's feature instead.
Or we can re-export the same feature flags.
|
|
||
| // Set dafault crypto provider to use | ||
| // See: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html#using-the-per-process-default-cryptoprovider | ||
| rustls::crypto::ring::default_provider(); |
There was a problem hiding this comment.
As explained in previous comments.
|
It's open season on breaking changes, is this one ready to go (mod merge conflicts)? |
|
I think so -- I'll try and fix the conflicts and get it going again |
|
Update: given the unresolved comments in this PR, I felt starting a new one might be more productive. Here is an alternative proposal: |
|
Given this PR seems to have unanswered questions and seems to have gone silent, so marking as draft |
5 participants
Which issue does this PR close?
Upgrade version of tonic used in dependencies from 0.12.3 to 0.13.0, which includes a batch of breaking changes.
Rationale for this change
Using the latest version of crates will prevent any potential issues and vulnerabilities. And will allow users to use latest tonic with arrow crates without keeping different versions
What changes are included in this PR?
Updated the new interface names.
Changed feature names used in the latest tonic.
Updated GitHub workflows to run tests correctly with the required tonic features.