Expands the HTTP interceptor API to include a call back for failed connection attempts #6144

SamBarker · 2024-07-17T05:31:46Z

Description

Expands the HTTP interceptor API to include a call back for failed connection attempts as discussed in #6143

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Depends on the definition of breaking change as it adds a new default method to an interface which could be seen as a breaking change for any implementation that already has that method defined but practically speaking this is backwards compatible.

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

SamBarker · 2024-07-22T02:25:34Z

...lient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyAsyncResponseListener.java

@@ -103,6 +103,11 @@ public void onContent(Response response, ByteBuffer content, Callback callback)
    }
  }

+  @Override
+  public void onFailure(Response response, Throwable failure) {
+    asyncResponse.completeExceptionally(failure);


This is the part of the PR I'm most un-sure of as I think it implies that the jetty implementation never completed futures on connection failure. Instead it relied on timeouts to handle those failures, but I could be very wrong about that.

I can't remember, but I do recall using the Adapter to avoid implementing all methods.
It's certainly strange that the onFailure wasn't implemented before. However, the HttpClient behavior did change a little after the original implementation (especially the retries part).

Anyway, it does seem OK to have this.

It looks like overriding onComplete ensured timely completion - but does not have appropriate exception handling.

What we are probably looking for here instead is using onComplete Result.getRequestFailure - that maps most closely to these types of exceptions. There is also Result.getResponseFailure which should be the same as what is passed to onFailure.

Oh this gets fun 😅

Updating onComplete to respect Result.isSucceeded() or Result.isFailed() causes the expectContinue test to fail, because Jetty considers a 404 in response to a request with Expect=100-continue to be a failure. So i've tried adding server.expect().post().withPath("/post-expect-continue").andReply(100, request -> "").always(); which causes the server to reply appropriately to the POST request. However that breaks the expectContinue test for jetty, OkHttp and VertX (there doesn't seem to be a JDK version 👀 ) clients tests 😢 .

In the jetty case the test now fails because the response is considered pending and there is no further demand so the request future never completes. I haven't debugged the others to work out whats going wrong but I suspect its something similar.

Reading MDN on 100-continue suggests the jetty implementation is wrong as it attaches the body to the POST request at the same time as the Expect=100-continue header. Whereas MDN suggests that it should send Expect=100-continue and wait for the response to that before continuing by sending the body.

I'm nothing like close enough to the details of the Kube API server to understand if 100-continue is commonly used or if it has a problem with including the POST body on the initial request. However this is starting to feel like a rabbit hole and not really in scope for this PR and thus what the implications of this are in practice.

Thoughts @manusa, @shawkins ?

Ahh the JDK client explicitly disables the expectContinue test.

Reading MDN on 100-continue suggests the jetty implementation is wrong as it attaches the body to the POST request at the same time as the Expect=100-continue header. Whereas MDN suggests that it should send Expect=100-continue and wait for the response to that before continuing by sending the body.

After further debugging the jetty Impl is right, jetty handles delaying the body if the Expect header is set but I'm still not seeing the request complete.

Hmm turns out I was very wrong about what was going on after much debugging I have
https://github.com/fabric8io/kubernetes-client/pull/6197/files

Which changes the default socketPolicy for the mockWebserver to EXPECT_CONTINUE from KEEP_OPEN. I haven't found a way to make it conditional on the headers as its too late by the time dispatch is called. I'm not at all sure what implications that change has.

I'm happy to merge that PR into this one if preferred but I think they are really independent. I'm happy to clean up that PR and its description and or raise an issue to cover it if the consensus is to keep it independent.

Nice work, I think it's better to keep them separate.

SamBarker · 2024-07-22T02:55:38Z

...etes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java

@@ -170,6 +172,28 @@ public CompletableFuture<Boolean> afterFailure(BasicBuilder builder, HttpRespons
    }
  }

+  @Test
+  @DisplayName("afterFailure (HTTP), invoked when remote server offline")


This commit could arguably be dropped but the test evolves in the next commit with the fix.

SamBarker · 2024-07-22T04:29:12Z

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/StandardHttpClient.java

+      if (throwable instanceof CompletionException) {
+        throw (CompletionException) throwable;
+      }


sonar is complaining this branch is untested, but I can't work out a way to inject a CompletionException. Any pointers gratefully received.

Came up with SamBarker@43f88fa which does trigger the branch using the Jetty client.

apparently not according to sonar. It seemed to work in the debugger locally.

Note that the sonar coverage is per-module. I'm not sure if this might affect the way coverage is computed since the Abstract test class is hosted in a different module.

Urgh yeah I hadn't thought about the multiple modules pushing in different directions. Sonar only reports a single number on the StandardHttpClient so a bit hard to know. I suspect on balance its probably better to have the test than not but I only added it to try and pass the coverage stats for the PR so not wedded to it as the core flows are tested.

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java

manusa

Besides the Javadoc issue I think everything else looks OK.

@shawkins any comments?

BTW, even though this is adding a feature, I'm going to consider it as a bug fix since it's addressing the lack of feature parity of Fabric8-OkHttp interceptors.

This way we can include it as part of #6097 and release it in 6.13.2

shawkins

Overall looks good, but we may need more refinement of Jetty exception handling.

shawkins · 2024-07-24T13:54:35Z

...lient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyAsyncResponseListener.java

@@ -103,6 +103,11 @@ public void onContent(Response response, ByteBuffer content, Callback callback)
    }
  }

+  @Override
+  public void onFailure(Response response, Throwable failure) {
+    asyncResponse.completeExceptionally(failure);


It looks like overriding onComplete ensured timely completion - but does not have appropriate exception handling.

What we are probably looking for here instead is using onComplete Result.getRequestFailure - that maps most closely to these types of exceptions. There is also Result.getResponseFailure which should be the same as what is passed to onFailure.

SamBarker · 2024-07-24T22:25:59Z

BTW, even though this is adding a feature, I'm going to consider it as a bug fix since it's addressing the lack of feature parity of Fabric8-OkHttp interceptors.
This way we can include it as part of #6097 and release it in 6.13.2

Sounds great from my POV

SamBarker · 2024-07-31T08:54:16Z

I plan to clean this up once #6197 is merged, but I think the coverage issues will go away once thats merged.

manusa · 2024-08-08T05:33:17Z

Hi @SamBarker,
#6197 is finally in. Is there something else left to do on this PR? Shall we merge it too?

Fixes fabric8io#6143

…re consistent.

…pletionException. This happens in the Jetty implementation but gets wrapped in an IOException by the OkHttp impl but this should be enough for the coverage checker.

… so its invoked when an established websocket connection fails

…ed up tests. Rather than waiting ~20s to give up retrying.

Really just making the coverage checker happy.

…better covered elsewhere.

sonarqubecloud · 2024-08-09T00:56:28Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

SamBarker · 2024-08-09T01:29:34Z

Hi @SamBarker,

#6197 is finally in. Is there something else left to do on this PR? Shall we merge it too?

I've added an extra commit to get the sonar coverage report passing so yes! Let's get this merged.

Any info on a timeline for the next 6.13 release?

manusa · 2024-08-09T05:00:00Z

Any info on a timeline for the next 6.13 release?

I was just waiting for this one (#6097), so probably in a couple of hours or early Monday.

manusa

LGTM, thx!

SamBarker changed the title ~~Failing test demonstrating https://github.com/fabric8io/kubernetes-client/issues/6143~~ Failing test demonstrating #6143 Jul 17, 2024

SamBarker force-pushed the afterFailureInvocation branch 2 times, most recently from bb6746c to c48e929 Compare July 21, 2024 23:59

SamBarker changed the title ~~Failing test demonstrating #6143~~ Expands the HTTP interceptor API to include a call back for failed connection attempts Jul 22, 2024

SamBarker force-pushed the afterFailureInvocation branch 2 times, most recently from 0e21fe0 to cbee038 Compare July 22, 2024 02:45

SamBarker commented Jul 22, 2024

View reviewed changes

SamBarker force-pushed the afterFailureInvocation branch 2 times, most recently from 43f88fa to 99cacc3 Compare July 22, 2024 21:29

SamBarker marked this pull request as ready for review July 22, 2024 21:31

SamBarker requested review from manusa, oscerd, rohanKanojia and shawkins as code owners July 22, 2024 21:31

manusa reviewed Jul 23, 2024

View reviewed changes

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java Outdated Show resolved Hide resolved

SamBarker force-pushed the afterFailureInvocation branch from 21db115 to 8d522ac Compare July 24, 2024 09:46

manusa reviewed Jul 24, 2024

View reviewed changes

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java Outdated Show resolved Hide resolved

manusa reviewed Jul 24, 2024

View reviewed changes

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java Outdated Show resolved Hide resolved

manusa requested changes Jul 24, 2024

View reviewed changes

manusa mentioned this pull request Jul 24, 2024

Release Fabric8 Kubernetes Client 6.13.2 #6097

Closed

18 tasks

shawkins reviewed Jul 24, 2024

View reviewed changes

SamBarker force-pushed the afterFailureInvocation branch from 95e325f to f9f1042 Compare July 24, 2024 22:28

SamBarker added 4 commits August 9, 2024 09:22

Add test which demonstrates what I expect to work

d94b383

Introduce afterConnectionFailure to complement after & afterFailure.

53f5464

Fixes fabric8io#6143

Add a bit more explanatory Javadoc to Interceptor

dbc9583

Extract private method to ensure changes to DefaultMockServer usage a…

e2638bd

…re consistent.

SamBarker added 3 commits August 9, 2024 09:25

Add test which covers future being completed exceptionally with a Com…

f854fc9

…pletionException. This happens in the Jetty implementation but gets wrapped in an IOException by the OkHttp impl but this should be enough for the coverage checker.

Move afterConnectionFailure callback to retryWithExponentialBackoff…

4d0d78f

… so its invoked when an established websocket connection fails

restart the mock server after a connection failure is detected to spe…

ff3accd

…ed up tests. Rather than waiting ~20s to give up retrying.

SamBarker force-pushed the afterFailureInvocation branch from f9f1042 to 13070a6 Compare August 8, 2024 21:26

SamBarker added 3 commits August 9, 2024 09:31

Add changelog entry

83dfcba

Clarify javadoc

a7c6962

@see -> @link

dc27f5e

SamBarker force-pushed the afterFailureInvocation branch from 13070a6 to dc27f5e Compare August 8, 2024 21:31

Add unit test to ensure we unwrap CompletionExceptions.

e9e4b2c

Really just making the coverage checker happy.

SamBarker force-pushed the afterFailureInvocation branch from 0063e1e to e9e4b2c Compare August 8, 2024 23:44

Drop exception handling test from AbstractInterceptorTest as its now …

b8b79d4

…better covered elsewhere.

SamBarker force-pushed the afterFailureInvocation branch from c49c1c5 to b8b79d4 Compare August 9, 2024 00:24

rohanKanojia approved these changes Aug 9, 2024

View reviewed changes

SamBarker requested a review from manusa August 9, 2024 04:21

manusa approved these changes Aug 9, 2024

View reviewed changes

manusa added this to the 7.0.0 milestone Aug 9, 2024 — with automated-tasks

manusa merged commit 8147542 into fabric8io:main Aug 9, 2024
21 checks passed

SamBarker deleted the afterFailureInvocation branch August 11, 2024 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expands the HTTP interceptor API to include a call back for failed connection attempts #6144

Expands the HTTP interceptor API to include a call back for failed connection attempts #6144

SamBarker commented Jul 17, 2024 •

edited by manusa

Loading

SamBarker Jul 22, 2024

manusa Jul 23, 2024

shawkins Jul 24, 2024

SamBarker Jul 24, 2024 •

edited

Loading

SamBarker Jul 25, 2024

SamBarker Jul 25, 2024

SamBarker Jul 25, 2024

manusa Jul 25, 2024

SamBarker Jul 22, 2024

SamBarker Jul 22, 2024

SamBarker Jul 22, 2024

SamBarker Jul 22, 2024

manusa Jul 23, 2024

SamBarker Jul 23, 2024

manusa left a comment

shawkins left a comment

shawkins Jul 24, 2024

SamBarker commented Jul 24, 2024

SamBarker commented Jul 31, 2024

manusa commented Aug 8, 2024

sonarqubecloud bot commented Aug 9, 2024

SamBarker commented Aug 9, 2024

manusa commented Aug 9, 2024

manusa left a comment

Expands the HTTP interceptor API to include a call back for failed connection attempts #6144

Expands the HTTP interceptor API to include a call back for failed connection attempts #6144

Conversation

SamBarker commented Jul 17, 2024 • edited by manusa Loading

Description

Type of change

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamBarker Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manusa left a comment

Choose a reason for hiding this comment

shawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamBarker commented Jul 24, 2024

SamBarker commented Jul 31, 2024

manusa commented Aug 8, 2024

sonarqubecloud bot commented Aug 9, 2024

Quality Gate passed

SamBarker commented Aug 9, 2024

manusa commented Aug 9, 2024

manusa left a comment

Choose a reason for hiding this comment

SamBarker commented Jul 17, 2024 •

edited by manusa

Loading

SamBarker Jul 24, 2024 •

edited

Loading