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

Fixes the issue of missing root cause in container launch TimeoutException (e.g. SSLHandshakeException) #5778

Merged
merged 9 commits into from Nov 28, 2022

Conversation

cdanger
Copy link
Contributor

@cdanger cdanger commented Aug 27, 2022

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.

…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.
@cdanger cdanger requested a review from a team as a code owner August 27, 2022 20:27
@cdanger cdanger changed the title Fixes causes of a TimeoutException (e.g. SSLHandshakeException) missing from logs / final stacktrace Fixes the issue of missing root cause in container launch TimeoutException (e.g. SSLHandshakeException) Aug 27, 2022
@kiview kiview self-assigned this Aug 29, 2022
@kiview
Copy link
Member

kiview commented Aug 29, 2022

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 waitUntilReadyAndTimeout check, or maybe an additional test needs to be added.

cdanger and others added 2 commits September 1, 2022 01:03
… cause of a HttpWaitStrategy failure in a ContainerLaunchException / TimeoutException, such as HTTPs check / certificate validation error): HttpWaitStrategyTest#testWaitUntilReadyWithTimeoutCausedBySslHandshakeError()
@cdanger
Copy link
Contributor Author

cdanger commented Aug 31, 2022

Could you please look into adding a test for this behavior? Maybe it is also fine to add it to the existing waitUntilReadyAndTimeout check, or maybe an additional test needs to be added.

OK I added a new commit with a new test for this in the HttpWaitStrategyTest class: testWaitUntilReadyWithTimeoutCausedBySslHandshakeError() .

@cdanger
Copy link
Contributor Author

cdanger commented Oct 8, 2022

Waiting on code owner review...

Copy link
Member

@kiview kiview left a 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.

@kiview
Copy link
Member

kiview commented Nov 21, 2022

@cdanger Do you want to finish up this PR regarding our suggestions, or should we take care of it?

cdanger and others added 3 commits November 22, 2022 21:41
…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.
Copy link
Contributor Author

@cdanger cdanger left a 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.

kiview
kiview previously approved these changes Nov 23, 2022
Copy link
Member

@kiview kiview left a 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 👍

@kiview kiview added this to the next milestone Nov 23, 2022
@kiview
Copy link
Member

kiview commented Nov 23, 2022

@cdanger CI is failing because of spotless, can you please run ./gradlew :testcontainers:spotlessApply and commit the changes? 🙏

Copy link
Contributor Author

@cdanger cdanger left a 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.

@kiview kiview merged commit f5ec43d into testcontainers:main Nov 28, 2022
@kiview
Copy link
Member

kiview commented Nov 28, 2022

Thanks @cdanger, merged 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants