-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fixes the issue of missing root cause in container launch TimeoutException (e.g. SSLHandshakeException) #5778
Conversation
…ecially SSL connection check errors when applying a HTTPS WaitStrategy (javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target). The exception was not propagated to the ContainerLaunchException, therefore not visible in the logs / final stacktrace.
Thanks for the PR @cdanger, a really good addition for the DX. Could you please look into adding a test for this behavior? Maybe it is also fine to add it to the existing |
… cause of a HttpWaitStrategy failure in a ContainerLaunchException / TimeoutException, such as HTTPs check / certificate validation error): HttpWaitStrategyTest#testWaitUntilReadyWithTimeoutCausedBySslHandshakeError()
OK I added a new commit with a new test for this in the |
Waiting on code owner review... |
core/src/test/java/org/testcontainers/junit/wait/strategy/HttpWaitStrategyTest.java
Outdated
Show resolved
Hide resolved
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.
Hey @cdanger, sorry for keeping the PR hanging. See my comment with a suggestion of simplifying the test.
core/src/test/java/org/testcontainers/junit/wait/strategy/HttpWaitStrategyTest.java
Outdated
Show resolved
Hide resolved
@cdanger Do you want to finish up this PR regarding our suggestions, or should we take care of it? |
…WaitStrategyTest.java Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
…aceContaining("javax.net.ssl.SSLHandshakeException")` and remove all the custom code in `AbstractWaitStrategy` class.
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.
@kiview I've applied your suggestion and removed all custom changes to AbstractWaitStrategy , so it's good on my side.
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.
Great, thanks for your collaboration @cdanger 👍
@cdanger CI is failing because of spotless, can you please run |
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.
OK I did the spotlessApply and committed again.
Thanks @cdanger, merged 👍 |
Fixes the issue of root causes not propagated to container launch TimeoutException; e.g. when applying a HttpWaitStrategy with TLS enabled (HTTPS), if HTTPS connection check fails because of certificate validation issue (e.g.
javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
), this exception is not propagated, therefor missing from the logs and the TimeoutException thrown at the end. The only wait to know about it is by using a debugger. The fix just adds the causing exception to the ContainerLaunchException, so that it's part of the TimeoutException stacktrace later on.