VertxHttpClientHTTPConduit redirects the request body twice when the body was passed through a single buffer

[ppalaga]

Originally posted in #1609 (comment) by @dcheng1248

I just did a test and unfortunately there still seems to be an issue at the unit test level. I'm actually not sure if this is related to the request data caching vs offloading you mentioned, so any advice would be appreciated. Our request body is very small FWIW, < 10 lines of XML. If my issue is not related to the cache vs offloading, I think this reflects different behaviours of the two HTTP Conduits.

I will give the information I can but need to redact/change some things.

Unit test mocks:

private void mockRedirect(...) throws IOException {
    wireMockServer.stubFor(
            WireMock.post(urlPathEqualTo("/normal/test/path"))
                    .withBasicAuth(VALID_USERNAME, String.valueOf(VALID_PASSWORD))
                    .withHeader(...)
                    .withHeader(...)
                    .withRequestBody(matchingXPath(...))
                    .atPriority(1)
                    .willReturn(
                            aResponse()
                                    .withStatus(302)
                                    .withHeader("location", wireMockServer.baseUrl() + "/redirect-test")));
  }
private void mockRedirectFinal(...) throws IOException {
    wireMockServer.stubFor(
            WireMock.post(urlPathEqualTo("/redirect-test"))
                    .withBasicAuth(VALID_USERNAME, String.valueOf(VALID_PASSWORD))
                    .withHeader(...)
                    .withHeader(...)
                    .withRequestBody(matchingXPath(...))
                    .willReturn(
                            aResponse()
                                    .withStatus(200)
                                    .withHeader("content-type", "application/xml")
                                    .withBodyFile("sample-file.xml")));
  }

Tests done:
quarkus 3.15, 3.16 and 3.17 with URLConnectionHTTPConduit and VertxHTTPConduit

Results:
3.15:
I only tested the default URLConectionHTTPConduit here. Test passes as expected.

3.16:
The default VertxHTTP fails with a 302 Error as expected.
The URLConnectionHTTP passes as expected.

3.17:
The URLConnectionHTTP passes.
The VertxHTTP fails with a 404 Error. Stack trace:

Caused by: org.apache.cxf.transport.http.HTTPException: HTTP response '404: Not Found' when communicating with http://localhost:50575/redirect-test
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$ResponseHandler.doProcessResponseCode(VertxHttpClientHTTPConduit.java:1272)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$ResponseHandler.handle(VertxHttpClientHTTPConduit.java:1176)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$ResponseHandler.handle(VertxHttpClientHTTPConduit.java:1154)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyHandler$Mode$Sync.awaitResponse(VertxHttpClientHTTPConduit.java:1044)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyHandler.handle(VertxHttpClientHTTPConduit.java:564)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyHandler.handle(VertxHttpClientHTTPConduit.java:425)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyOutputStream.close(VertxHttpClientHTTPConduit.java:420)
	at org.apache.cxf.ext.logging.LoggingOutputStream.postClose(LoggingOutputStream.java:53)
	at org.apache.cxf.io.CachedOutputStream.close(CachedOutputStream.java:228)
	at org.apache.cxf.transport.AbstractConduit.close(AbstractConduit.java:56)
	at org.apache.cxf.transport.http.HTTPConduit.close(HTTPConduit.java:717)
	at org.apache.cxf.interceptor.MessageSenderInterceptor$MessageSenderEndingInterceptor.handleMessage(MessageSenderInterceptor.java:63)
	... 103 more

Upon further investigation, removal of .withRequestBody(matchingXPath(...)) in the stub of RequestFinal() made the test pass. So it seems like it did redirect the request, but the body is not included in the redirected request? I tried to look through the source code but could not find the reason quickly.

PS: One tiny bug I did notice is that in VertxHttpClientHTTPConduit.redirectRetransmit, both the URLs in the debug log refer to the redirection URL (in the previous method the new URL is already added to the list, so list.size()-1 is the new URL, not the old one).

PPS: If it's not too difficult to implement, being able to switch on logging the redirection responses and requests might also help with debugging.

Thanks a lot for your help again.

[ppalaga]

@dcheng1248 thanks for the report! It would be great to have a minimal reproducer. From what you posted, the only difference against our tests (that are passing) I can see is the basic auth. But as you say, it works when you remove the body condition, so the auth headers seem to get redirected properly...

[dcheng1248]

@ppalaga thanks for the quick response. See here for minimal reproduction.
Running the unit test would produce the error.
Uncommenting #quarkus.cxf.http-conduit-factory=URLConnectionHTTPConduitFactory would let the test pass.

Please let me know if you need more information.

[ppalaga]

Thanks for the reproducer, @dcheng1248! The issue was that the body was cached twice if there was just a single buffer used to pass the original body. See the fix in #1631 . It is actually strange that our CXF test service happily ignored the second XML document in the request body. I had to adapt the test to run against a plain dummy vert.x endpoint to be able to reproduce it.

[ppalaga]

It is actually strange that our CXF test service happily ignored the second XML document in the request body.

The service rejects the request with two XML root elements only if quarkus.cxf.endpoint."/hello".schema-validation.enabled-for = request is set.

[dcheng1248]

@ppalaga many thanks for the explanations and the quick fix. You guys do awesome work for the community. Looking forward to trying out the fix!

[ppalaga]

I released Quarkus CXF 3.17.1 you can try it like this

Not sure whether Quarkus team will want to release Quarkus Platform 3.17.3 because of this. You may want to watch this PR.

[dcheng1248]

@ppalaga a colleague tested the fix today on our system and got a Redirect loop detected on Conduit error. Looking at the source I would guess setting the http.redirect.max.same.uri.count property could probably fix it. I haven't had the time to look into it deeper or test it myself, but it seems there is no quarkus cxf property that sets this property? Is this something you might consider adding in the future?

[ppalaga]

Yeah, there is no Quarkus property for that, but starting the app with -Dhttp.redirect.max.same.uri.count=42 should do what you need. Adding a new Quarkus property is doable - feel free to file a new issue for that.

The bottom line is that when implementing the redirects, I wondered a lot in which situations would multiple hits of a single URI in the redirection chain make any sense. Could you please explain? Is it perhaps some sort of load ballancer that redirects to itself multiple times until it is able to get a free worker node?

[dcheng1248]

@ppalaga thanks for the information! I created a new issue #1639 for that.

Regarding your question, honestly I'm not 100% sure. In our case this is towards a system we don't manage - I wasn't even aware redirection was occurring until I got that 302 Error on quarkus 3.16. That said, I can think of two possible use cases in general.

1. Session management. It can be through cookies or just internal routing of the load balancer, but afaik a 302 to the same URL is possible in both cases.
2. As you said, resource management/ health check that results in load balancer redirection.

[ppalaga]

Hm... now when thinking bout the above, where you mention Session management, I wonder whether the lack of auth retransmits (a.k.a. #1616 ) is not what you are really missing?

[dcheng1248]

In our use case probably not. Auth retransmit deals with 401 or 407, while we get a redirection loop error (not a auth redirection loop) that stems from the same url according to the stack trace.

Since the apache implementation also doesn't seems to deal with session cookie headers from 302 (from my quick scan of the source, please correct if I'm wrong), I doubt that's the issue. I would more guess in our case it's either an LB port/server routing through IPHashing etc. or a LB resource management that causes the redirection to the same URL.