Support async client in AwsRequestSigningApacheV5Interceptor

[SnuK87]

Is it possible to use this interceptor with the CloseableHttpAsyncClient? I always get a 403 response but using the CloseableHttpClient works fine.

My approach:

    public void test() {
        val provider = new MyCredsProvider();
        HttpRequestInterceptor interceptor = new AwsRequestSigningApacheV5Interceptor("execute-api", Aws4Signer.create(), provider, Region.XXX);

        SimpleHttpRequest postAsync = SimpleRequestBuilder.post()
            .setUri("https://...")
            .addHeader("content-type", "application/json")
            .setBody("{}", ContentType.APPLICATION_JSON)
            .build();

        CloseableHttpAsyncClient client = HttpAsyncClients.custom()
            .addRequestInterceptorLast(interceptor)
            .setVersionPolicy(HttpVersionPolicy.FORCE_HTTP_1)
            .build();

        client.start();

        client.execute(postAsync, new FutureCallback<SimpleHttpResponse>() {
            @Override
            public void completed(SimpleHttpResponse simpleHttpResponse) {
                System.out.println(simpleHttpResponse.getCode());
            }

            @Override
            public void failed(Exception e) {
                e.printStackTrace();
            }

            @Override
            public void cancelled() {
                System.out.println("cancelled");
            }
        });
    }

[dblock]

Do you think you could turn your example with both CloseableHttpClient and CloseableHttpAsyncClient into 2 unit tests? I see us showing how to use CloseableHttpClient in a sample, but we don't have an integration test that uses it, which we should. Expecting that one will pass and the other fail, and we can take a look from there?

Enable debug logging. I would look at the headers being sent and compare between the two.

[SnuK87]

I was able to identify the problem but I'm struggling to come up with a fix.

The problem is that this statement is always false because the CloseableHttpAsyncClient uses SimpleHttpRequest instead of ClassicHttpRequest.

So I tried to create a new Interceptor that works with SimpleHttpRequest but I'm only able to cast the HttpRequest to BasicHttpRequest in the process() method. The BasicHttpRequest doesn't have a body and the cast to SimpleHttpRequest (which has the body) fails.

I'll set up a pull request later to make the problem clear. Maybe we can come up with a solution.

[dblock]

Why does the cast to SimpleHttpRequest fail? What's the actual type of the instance of that request?

Maybe start by writing a unit test, so we know we've fixed it, and we can see different solutions.

There should really be a more generic way to get/set the entity out/in of the request. The entityDetails tells us whether there's one, but doesn't let you get the stream.

[SnuK87]

The actual type of the HttpRequest is org.apache.hc.core5.http.message.BasicHttpRequest. The cast to SimpleHttpRequest fails with:

java.lang.ClassCastException: class org.apache.hc.core5.http.message.BasicHttpRequest cannot be cast to class org.apache.hc.client5.http.async.methods.SimpleHttpRequest (org.apache.hc.core5.http.message.BasicHttpRequest and org.apache.hc.client5.http.async.methods.SimpleHttpRequest are in unnamed module of loader 'app')

I've tried to add a unit test for a SimpleHttpRequest here #102

[acm19]

The v5 client seems pretty rigid when it comes to this. I took a quick look and it doesn't seem to be a way around having different implementations for each type of request. I'll try to take a closer look later this week if you haven't come up with something before.

[dblock]

I think multiple implementations isn't the end of the world!

[acm19]

I think multiple implementations isn't the end of the world!

It does look like a code smell though.

Anyway, I finally had the time to take a closer look at this and I think we're not doing in the expected way. We're using a request interceptor that is not expected to deal with the enclosed entity, that's why we need to do all the casting and hacks. Casting won't work for the async request because you receive a BasicHttpRequest rather than a SimpleHttpRequest in the interceptor.

The proper way would be to remove the request interceptors and use exec interceptor as in this example. I'll see if I can find to try it out with the async request at least. I do have improvements to the current interceptor I'll push from my looking around.

[lbenedetto]

This feature is important to me because org.opensearch.client.RestClient uses ClosableHttpAsyncClient under the hood.

[dblock]

Didn't https://github.com/acm19/aws-request-signing-apache-interceptor/pull/104 do this? @lbenedetto try with the latest version?

        HttpRequestInterceptor interceptor = new AwsRequestSigningApacheV5Interceptor(
                service,
                Aws4Signer.create(),
                DefaultCredentialsProvider.create(),
                region);

        return HttpAsyncClients.custom()
                .addRequestInterceptorLast(interceptor)
                .setConnectionManager(PoolingAsyncClientConnectionManagerBuilder.create()
                        .setDefaultTlsConfig(TlsConfig.custom()
                                .setVersionPolicy(HttpVersionPolicy.FORCE_HTTP_1)
                                .build())
                        .build())
                .build();
    }

[lbenedetto]

I'm using the latest, 2.3.1.

@acm19 said in a comment on #104 "I didn't add async support, I barely added the sample to test it once we add it. "

[lbenedetto]

I was mistaken, it actually works fine. I was just missing STS on my classpath.

I think this issue can be closed.

[acm19]

I'll re-open this so we either try to solve it or document that it's a limitation of the client.