- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 333
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
Allow removing hyper-rustls/ring feature #1717
Conversation
066cc7a
to
6f04610
Compare
6f04610
to
467eb1a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1717 +/- ##
=====================================
Coverage 76.1% 76.1%
=====================================
Files 84 84
Lines 7859 7859
=====================================
Hits 5976 5976
Misses 1883 1883
🚀 New features to boost your workflow:
|
kube-client/src/client/auth/oauth.rs
Outdated
#[cfg(not(any( | ||
all( | ||
feature = "rustls-tls", | ||
any(feature = "ring", feature = "aws-lc-rs", feature = "webpki-roots") |
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.
i think we need (when using rustls);
- one of
aws-lc-rs
orring
webpki-roots
optional, but orthogonal to this choice (so probably does not need to be checked here)
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.
see my previous comment.
let's try adding cfg-if to make it a bit clearer
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.
i think we can avoid this extra change and dependency, and keep all logic as we had it before by introducing the new compile error up to top level (257 in oidc.rs).
#[cfg(all(feature = "rustls-tls", not(any(feature = "ring", feature = "aws-lc-rs"))))]
compile_error!("At least one of ring or aws-lc-rs feature must be enabled to use rustls-tls feature");
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.
mostly because the way it's indented now is also quite big and hard to extend, and we want to just bail out as soon as possible anyway.
besides, oidc.rs isn't even the client that is used for most things (that one comes from config_ext.rs + builder.rs and does the selection in tls.rs (which does not do this same if-else on providers).
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.
done.
btw, are you fine with removing the auto-installation of the aws-lc-rs provider if aws-lc-rs feature was set?
theoretically both ring and aws-lc-rs might be enabled, and we should leave it to the user (or let rustls install if only one of them was selected)
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.
removing auto-installation
not super comfortable touching this yet. removing it outright will feel breaking, and it's a very low level api people will have to familiarise themselves with to do basic things when using aws-lc-rs. if aws-lc-rs becomes the default, i would want it to be frictionless, and this feels ok to me as a 'helping hand'; install if it hasn't been installed already. nothing stops people from installing another default provider else earlier on.
this might turn into a bigger discussion though. maybe we need to do the same thing we did for openssl vs rustls; always speculatively install one provider, tilting towards our default. that way we can change our default later on (like we did for openssl -> rustls).
467eb1a
to
e0f4a9a
Compare
e0f4a9a
to
375899d
Compare
no reason to include the "ring" crate if aws-lc-rs is being used. add a new "ring" feature (enabled by default, to avoid breaking existing users), that will enable hyper-rustls/ring only if needed. Signed-off-by: Eliad Peller <eliad.peller@wiz.io>
375899d
to
bba11ca
Compare
@@ -12,10 +12,11 @@ keywords = ["kubernetes", "client",] | |||
categories = ["web-programming::http-client", "network-programming", "api-bindings"] | |||
|
|||
[features] | |||
default = ["client"] | |||
default = ["client", "ring"] |
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.
default = ["client", "ring"] | |
default = ["client", "rustls-tls", "ring"] |
possibly worth listing this explicitly. EDIT: ah, but it's explicit in kube/Cargo.toml
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.
Thanks a lot. Sorry about all the back and forths. I'll get this in before the release.
minor follow-up #1720 |
Motivation
no reason to include the "ring" crate if aws-lc-rs is being used.
Solution
features are only additive, so they can't be removed once it's added (by any sub-crate).
avoid adding extra features that might not be used (e.g. in case of aws-lc-rs).
in order to make it optional we can add a new "ring" feature to the default features, so it can be avoided with
default-features = false
. however, i think letting the configure it as needed is a better approach.please update if you do prefer to add a new default feature-flag (or some other approach) and i'll adjust the patch.