Skip to content

Conversation

@wiktor-k
Copy link
Contributor

Usually, the container's HTTPS ports use self-signed certificates instead of real ones. This makes the HTTPS health check always fail and be retried.

This PR allows self-signed certificates for health checks.

I think for health-checks it's an acceptable trade-off but I'm open for getting even better ideas that solve the underlying problem (HTTPS health-checks being almost useless).

Thanks for your time! 👋

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Dec 18, 2024

It seems there are some issues relevant to newer Rust compiler. If this looks 👌 I'm happy to submit another PR solving the Rust lints and rebase this one on top of the other.

Edit: Fixed issues in #41.

@ilaborie
Copy link
Contributor

the #41 PR is merged, you can rebase on top of main

Copy link
Contributor

@ilaborie ilaborie left a comment

Choose a reason for hiding this comment

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

👏 Very good suggestion

Just update the PR with these small changes to let the user opt-in to validate certificate:

  • Add the require_valid_certs: bool attribute in the WaitStrategy::HttpSuccess variant.
  • use this field here
  • document to explain that for testing purpose the default value is allowing invalid certificate.

);
let Ok(response) = reqwest::get(&url).await else {
let Ok(client) = reqwest::ClientBuilder::new()
.danger_accept_invalid_certs(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's too dangerous, I understand the use case, so I think we should allow the use to choose if this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent feedback - exactly what I was looking for!

document to explain that for testing purpose the default value is allowing invalid certificate.

If the option is there I'm okay with having require_valid_certs: true as the default (I'd just set it to false in my code).

I assume you're talking about WaitStrategy::https function?

Usually, the container's HTTPS ports use self-signed certificates
instead of real ones. This makes the HTTPS health check always fail
and be retried.

This PR allows self-signed certificates for health checks by adding
the `require_valid_certs` configuration field to `WaitStrategy`.

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@ilaborie ilaborie merged commit b8824a6 into wefoxplatform:main Dec 20, 2024
9 checks passed
@wiktor-k wiktor-k deleted the wiktor-k/allow-using-self-signed-certs branch December 20, 2024 09:11
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.

2 participants