Fix http throwable responses #2251
Conversation
There was a problem hiding this comment.
Thank you for locking down the access of various methods; that will make further maintenance simpler, as we don't have to search as much for who is calling each one.
I have a few code quality comments that I think should be fixed before this is merged, but otherwise it looks good.
| @@ -108,7 +108,11 @@ private void checkTimeout(AsyncFiber fiber) { | |||
|
|
|||
| private void resume(AsyncFiber fiber, HttpResponse<String> response, Throwable throwable) { | |||
| if (throwable != null) { | |||
There was a problem hiding this comment.
This method now has two different tests for response being null - one in the block with throwable != null and one in the Optional.ofNullable(response) logic.
It would probably be better to merge them or otherwise simplify the logic. There are two different things being tested for null, here: throwable and response. Are all four combinations meaningful?
There was a problem hiding this comment.
This method now has two different tests for response being null - one in the block with throwable != null and one in the Optional.ofNullable(response) logic.
It would probably be better to merge them or otherwise simplify the logic. There are two different things being tested for null, here:
throwableandresponse. Are all four combinations meaningful?
The only permutation that is not meaningful is both null. Let me try to refactor this method a little.
operator/src/main/java/oracle/kubernetes/operator/http/HttpResponseStep.java
Show resolved
Hide resolved
| completeWithThrowableBeforeTimeout(nextAction, new Throwable("Test")); | ||
|
|
||
| assertThat(logRecords, containsWarning(HTTP_REQUEST_GOT_THROWABLE)); | ||
| m.revert(); |
There was a problem hiding this comment.
All mementos should be reverted in the tearDown method; among other things, that ensures that if the test exceptions out, the revert still happens, so other tests are not affected. It appears that you want to collect an additional log messages. The simplest thing would be simply to add it to the original least created during the setUp call. If for some reason you cannot do that, take advantage of the fact that the memento itself is saved as consoleMemento and just call its collectLogMessages method, here.
There was a problem hiding this comment.
I started with adding this new memento to the list in setUp method, but it did not work. Let me try consoleMemento suggestion.
The PR contains a fix for a problem in http error handling that is discovered during testing domain secure mode enabled without configuring SSL use case, plus some code cleanup based on warnings in IntelliJ.
Problem: when a HttpAsyncRequestStep's AsyncProcessing is resumed without a response object, it was always considered as a timeout although sometimes resume is called with an Exception that should be made visible to the user. One example of such exceptions is javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target.
With the fix, operator logs a warning message and fails the corresponding step.
(Note: the changes originally contained a fix for a NPE on unboxing the chosen port number for Prometheus. The code change for this has been removed after merging with the develop branch yesterday; apparently the problematic code has been changed by a PR that is merged yesterday.)
Integration test result in Jenkins before the last merge with develop: https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4250/.
Integration test run with the latest: https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4255/.
Jenkins job with review changes: https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4256/.