Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

conditionally install aws-lc-rs #1617

Merged
merged 2 commits into from
Oct 25, 2024
Merged

conditionally install aws-lc-rs #1617

merged 2 commits into from
Oct 25, 2024

Conversation

goenning
Copy link
Contributor

@goenning goenning commented Oct 25, 2024

Motivation

There was a recent panic on our app ( aptakube/aptakube#319 ) and it was caused by this unwrap

Solution

I suppose we should always check if there's a CryptoProvider already installed before installing another one. That's how it's done here

#[cfg(all(feature = "aws-lc-rs", feature = "rustls-tls"))]
{
if rustls::crypto::CryptoProvider::get_default().is_none() {
// the only error here is if it's been initialized in between: we can ignore it
// since our semantic is only to set the default value if it does not exist.
let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();
}
}
but for some reason the oidc.rs did not

I'm also explicitly installing aws_lc_rs (with a fallback to ring) on my app, so I would expect kube to just skip it as it's already installed

cc @mcluseau

Signed-off-by: goenning <me@goenning.net>
@goenning goenning changed the title conditionally install aws conditionally install aws-lc-rs Oct 25, 2024
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.3%. Comparing base (ecbdafc) to head (ac24670).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1617     +/-   ##
=======================================
+ Coverage   75.3%   75.3%   +0.1%     
=======================================
  Files         82      82             
  Lines       7343    7344      +1     
=======================================
+ Hits        5527    5528      +1     
  Misses      1816    1816             
Files with missing lines Coverage Δ
kube-client/src/client/auth/oidc.rs 56.4% <100.0%> (+0.3%) ⬆️

@mcluseau
Copy link
Contributor

Hi @goenning , I agree. It triggers some "should we factorize this" questions in my mind, but that's a question for another day :)
/lgtm

@clux clux added the changelog-fix changelog fix category for prs label Oct 25, 2024
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Thanks for this. I see (after initially being wrong and deleting my first comment) that we do this already in one other place, and bubble up an error in oauth.

I guess the oauth one is coming through tame-oauth so it makes sense it's different.

@clux clux added this to the 0.97.0 milestone Oct 25, 2024
@clux clux enabled auto-merge (squash) October 25, 2024 09:17
@clux clux merged commit 140c9bd into kube-rs:main Oct 25, 2024
16 checks passed
@clux clux mentioned this pull request Nov 19, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
changelog-fix changelog fix category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants