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

move from hyper-proxy2 to hyper-http-proxy #1502

Merged
merged 1 commit into from
May 27, 2024

Conversation

aviramha
Copy link
Contributor

No description provided.

@aviramha
Copy link
Contributor Author

Sorry about the spam @clux - first time adding an annoying dependency like this to a library ;P

@clux clux added the changelog-fix changelog fix category for prs label May 26, 2024
@clux clux added this to the 0.92.0 milestone May 26, 2024
@clux
Copy link
Member

clux commented May 26, 2024

np, appreciate the quick fixes here!

@aviramha
Copy link
Contributor Author

@clux would appreciate your help figuring out how to solve it - the hyper-proxy crate doesn't support having both rustls/openssl features used together. How should I solve it?

@clux

This comment was marked as resolved.

@clux
Copy link
Member

clux commented May 26, 2024

..actually, no that breaks all features builds. maybe we just support rustls for https proxy until hyper-proxy2 can support having both enabled?

@aviramha
Copy link
Contributor Author

Fine by me. I thought to do it as a cheap shot then I thought using the sub-feature thingy is easy, (it could've been..)

Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 74.9%. Comparing base (57320a8) to head (304fab6).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1502   +/-   ##
=====================================
  Coverage   74.9%   74.9%           
=====================================
  Files         78      78           
  Lines       6864    6864           
=====================================
  Hits        5136    5136           
  Misses      1728    1728           
Files Coverage Δ
kube-client/src/client/builder.rs 58.3% <0.0%> (ø)

... and 1 file with indirect coverage changes

@clux
Copy link
Member

clux commented May 26, 2024

possibly a legit concern from cargo deny here; hyper-proxy2 does not really update dependencies frequently leading to duplicate rustls deps and this can slow down rustls adoption speed.

i don't think the duplicate versions are super problematic atm, because we are not using the same ssl parameters for the proxy connector (so mismatching struct errors shouldn't happen). at the very least a note in deny.toml about the ignore duplicate for it would be helpful

@aviramha
Copy link
Contributor Author

aviramha commented May 26, 2024

I forked the repo and updated, sent a PR upstream and changed kube rs to use our fork for the moment.
I hate it when I have two versions of same dependency in our deptree, caused me so much pain ;(

@clux
Copy link
Member

clux commented May 26, 2024

Sounds good!

It might also be worth adding the rustls-webpki feature for the rustls fork.
EDIT: oh, wait you cannot disable the old webpki 🤔

@aviramha
Copy link
Contributor Author

Not sure about the new failures?

@clux
Copy link
Member

clux commented May 26, 2024

The errors:

0.23.8/src/crypto/mod.rs:260:14:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point

hints at aws_lc_rs lib, which we have not enabled yet. i guess hyper-proxy2 also needs to not enable that feature on rustls :/

@aviramha aviramha force-pushed the features_are_hard branch 2 times, most recently from babaac3 to e04debd Compare May 27, 2024 05:58
@aviramha
Copy link
Contributor Author

Okay I took ownership of that fork and did some refactoring there to be more friendly to kube-rs.
I can also make sure the fork is always up to date :)

change from hyper-proxy2 to new fork - hyper-http-proxy

Signed-off-by: Aviram Hassan <aviramyhassan@gmail.com>
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.

Your new upstream looks great to me. Thanks for doing this.

@clux clux merged commit bd84d65 into kube-rs:main May 27, 2024
17 checks passed
@clux clux changed the title add feature propagation to hyper-proxy2 dependency move from hyper-proxy2 to hyper-http-proxy May 27, 2024
@aviramha aviramha deleted the features_are_hard branch May 27, 2024 13:38
@clux clux mentioned this pull request Jun 10, 2024
@clux clux changed the title move from hyper-proxy2 to hyper-http-proxy move from hyper-proxy2 to hyper-http-proxy Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-fix changelog fix category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants