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

add valid_until to Client #1676

Closed
wants to merge 1 commit into from

Conversation

goenning
Copy link
Contributor

@goenning goenning commented Jan 2, 2025

fixes #1675

I found 3 different ways of doing this, but I'm not sure which one is the best.

This PR implements Option 1, but I'm fine with other options too (except maybe 3, my least favorite)

  • Option 1: Fetch the identity_pem during the make_generic_builder and pass that onto both rustls and openssl HttpsConnector functions as an arg. This is a breaking change for those who are NOT using the make_generic_builder function.
  • Option 2: Keep the exec_identity_pem call in both rustls and openssl connector builder as an arg, and bubble up the expiration datetime to the make_generic_builder function. This is also a breaking change for those who are using the make_generic_builder function as the return type for the HttpsConnector functions will change.
  • Option 3: Call exec_identity_pem twice, once in the make_generic_builder function just to get the expiration date, and again inside the rustls and openssl HttpsConnector functions (as it is currently). This is NOT a breaking change, but it is inefficient as we are calling the exec_identity_pem function twice as this is a blocking system call.

@goenning
Copy link
Contributor Author

goenning commented Jan 2, 2025

will work on fixing tests soon, just wanted to get your thoughts on the options

@goenning
Copy link
Contributor Author

Hey @clux just wondering if you had a chance to look at this PR. I’m happy to make any API that you think might be better.

@clux
Copy link
Member

clux commented Feb 21, 2025

Hey! Sorry, I've been slowly working my way through the stack of PRs. From a quick look though, one thing does stand out to me though;

Breaking Client::new for this is not great. It's the main interface for creating any custom_client, and it inherently feels awkward to me to force this out in the public unless it's asked for. Same about ConfigExt if we can help it.

Option 2, you describe breaking make_generic_builder, but this is not public? Am I misunderstanding the concern?

AFAIKT, option 3, the inefficient double call might not be too bad? The functions it all call just seem to be re-juggling structs in memory a little bit? Maybe it's a sensible thing to start with. Sounds like maybe the date can be exposed through rustls more effeciently at a later date?

@goenning
Copy link
Contributor Author

Option 2, you describe breaking make_generic_builder, but this is not public? Am I misunderstanding the concern?

That was a typo, I meant to say it's a breaking change for those NOT using make_generic_builder, the breaking change would be on rustls_https_connector_with_connector and openssl_https_connector_with_connector

AFAIKT, option 3, the inefficient double call might not be too bad? The functions it all call just seem to be re-juggling structs in memory a little bit? Maybe it's a sensible thing to start with. Sounds like maybe the date can be exposed through rustls more effeciently at a later date?

I've created #1707 as an implementation of option 3, let me know what you think!

@clux
Copy link
Member

clux commented Mar 11, 2025

replaced by #1707

@clux clux closed this Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kube ignores expirationTimestamp when exec returns a client certificate
2 participants