From d305f3140b25a2f3e8e8e143348d060be94cc15d Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 28 Jul 2021 10:21:43 -0700 Subject: [PATCH] Fix NPE in Apache HttpAsyncClient instrumentation (#3692) * Fix NPE in Apache HttpAsyncClient instrumentation * Fix Apache HttpClient host + absolute uri * Add similar test for Apache HttpClient 5 * Better tests * Sync with 4.0 and 4.3 * Fix * sync * Elasticsearch twist * Remove so-called optimization path --- .../ApacheHttpAsyncClientInstrumentation.java | 3 +- .../ApacheHttpClientRequest.java | 58 +++++-- .../groovy/ApacheHttpAsyncClientTest.groovy | 155 ++++++++++++++---- .../v4_0/ApacheHttpClientRequest.java | 34 +++- .../test/groovy/ApacheHttpClientTest.groovy | 46 ++++++ .../test/groovy/ApacheHttpClientTest.groovy | 46 ++++++ 6 files changed, 280 insertions(+), 62 deletions(-) diff --git a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java index adc251f696..b7eed163f2 100644 --- a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java +++ b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java @@ -100,9 +100,10 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation @Override public HttpRequest generateRequest() throws IOException, HttpException { + HttpHost target = delegate.getTarget(); HttpRequest request = delegate.generateRequest(); - ApacheHttpClientRequest otelRequest = new ApacheHttpClientRequest(request); + ApacheHttpClientRequest otelRequest = new ApacheHttpClientRequest(target, request); if (instrumenter().shouldStart(parentContext, otelRequest)) { wrappedFutureCallback.context = instrumenter().start(parentContext, otelRequest); diff --git a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java index a8a54a6a82..b1a7ca4928 100644 --- a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java +++ b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java @@ -9,10 +9,9 @@ import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.net.URI; import java.net.URISyntaxException; import org.apache.http.Header; +import org.apache.http.HttpHost; import org.apache.http.HttpRequest; import org.apache.http.ProtocolVersion; -import org.apache.http.RequestLine; -import org.apache.http.client.methods.HttpUriRequest; import org.checkerframework.checker.nullness.qual.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -25,24 +24,13 @@ public final class ApacheHttpClientRequest { private final HttpRequest delegate; - public ApacheHttpClientRequest(HttpRequest httpRequest) { - URI calculatedUri; - if (httpRequest instanceof HttpUriRequest) { - // Note: this is essentially an optimization: HttpUriRequest allows quicker access to required - // information. The downside is that we need to load HttpUriRequest which essentially means we - // depend on httpasyncclient library depending on httpclient library. Currently this seems to - // be the case. - calculatedUri = ((HttpUriRequest) httpRequest).getURI(); + public ApacheHttpClientRequest(HttpHost httpHost, HttpRequest httpRequest) { + URI calculatedUri = getUri(httpRequest); + if (calculatedUri != null && httpHost != null) { + uri = getCalculatedUri(httpHost, calculatedUri); } else { - RequestLine requestLine = httpRequest.getRequestLine(); - try { - calculatedUri = new URI(requestLine.getUri()); - } catch (URISyntaxException e) { - logger.debug(e.getMessage(), e); - calculatedUri = null; - } + uri = calculatedUri; } - uri = calculatedUri; delegate = httpRequest; } @@ -133,4 +121,38 @@ public final class ApacheHttpClientRequest { return null; } } + + @Nullable + private static URI getUri(HttpRequest httpRequest) { + try { + // this can be relative or absolute + return new URI(httpRequest.getRequestLine().getUri()); + } catch (URISyntaxException e) { + logger.debug(e.getMessage(), e); + return null; + } + } + + @Nullable + private static URI getCalculatedUri(HttpHost httpHost, URI uri) { + try { + String path = uri.getPath(); + if (!path.startsWith("/")) { + // elasticsearch RestClient sends relative urls + // TODO(trask) add test for this and extend to Apache 4, 4.3 and 5 + path = "/" + path; + } + return new URI( + httpHost.getSchemeName(), + null, + httpHost.getHostName(), + httpHost.getPort(), + path, + uri.getQuery(), + uri.getFragment()); + } catch (URISyntaxException e) { + logger.debug(e.getMessage(), e); + return null; + } + } } diff --git a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy index 2e082c9ee9..46bd5c1c0d 100644 --- a/instrumentation/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy +++ b/instrumentation/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy @@ -9,6 +9,7 @@ import io.opentelemetry.instrumentation.test.base.HttpClientTest import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import java.util.concurrent.CancellationException +import org.apache.http.HttpHost import org.apache.http.HttpResponse import org.apache.http.client.config.RequestConfig import org.apache.http.concurrent.FutureCallback @@ -17,7 +18,7 @@ import org.apache.http.message.BasicHeader import spock.lang.AutoCleanup import spock.lang.Shared -class ApacheHttpAsyncClientTest extends HttpClientTest implements AgentTestTrait { +abstract class ApacheHttpAsyncClientTest extends HttpClientTest implements AgentTestTrait { @Shared RequestConfig requestConfig = RequestConfig.custom() @@ -32,45 +33,20 @@ class ApacheHttpAsyncClientTest extends HttpClientTest implement client.start() } - @Override - HttpUriRequest buildRequest(String method, URI uri, Map headers) { - def request = new HttpUriRequest(method, uri) - headers.entrySet().each { - request.addHeader(new BasicHeader(it.key, it.value)) - } - return request - } - - @Override - int sendRequest(HttpUriRequest request, String method, URI uri, Map headers) { - return client.execute(request, null).get().statusLine.statusCode - } - - @Override - void sendRequestWithCallback(HttpUriRequest request, String method, URI uri, Map headers, AbstractHttpClientTest.RequestResult requestResult) { - client.execute(request, new FutureCallback() { - @Override - void completed(HttpResponse httpResponse) { - requestResult.complete(httpResponse.statusLine.statusCode) - } - - @Override - void failed(Exception e) { - requestResult.complete(e) - } - - @Override - void cancelled() { - throw new CancellationException() - } - }) - } - @Override Integer responseCodeOnRedirectError() { return 302 } + @Override + HttpUriRequest buildRequest(String method, URI uri, Map headers) { + def request = createRequest(method, uri) + headers.entrySet().each { + request.setHeader(new BasicHeader(it.key, it.value)) + } + return request + } + @Override Set> httpAttributes(URI uri) { Set> extra = [ @@ -79,4 +55,113 @@ class ApacheHttpAsyncClientTest extends HttpClientTest implement ] super.httpAttributes(uri) + extra } + + // compilation fails with @Override annotation on this method (groovy quirk?) + int sendRequest(HttpUriRequest request, String method, URI uri, Map headers) { + def response = executeRequest(request, uri) + response.entity?.content?.close() // Make sure the connection is closed. + return response.statusLine.statusCode + } + + // compilation fails with @Override annotation on this method (groovy quirk?) + void sendRequestWithCallback(HttpUriRequest request, String method, URI uri, Map headers, AbstractHttpClientTest.RequestResult requestResult) { + try { + executeRequestWithCallback(request, uri, new FutureCallback() { + @Override + void completed(HttpResponse httpResponse) { + httpResponse.entity?.content?.close() // Make sure the connection is closed. + requestResult.complete(httpResponse.statusLine.statusCode) + } + + @Override + void failed(Exception e) { + requestResult.complete(e) + } + + @Override + void cancelled() { + requestResult.complete(new CancellationException()) + } + }) + } catch (Throwable throwable) { + requestResult.complete(throwable) + } + } + + abstract HttpUriRequest createRequest(String method, URI uri) + + abstract HttpResponse executeRequest(HttpUriRequest request, URI uri) + + abstract void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback callback) + + static String fullPathFromURI(URI uri) { + StringBuilder builder = new StringBuilder() + if (uri.getPath() != null) { + builder.append(uri.getPath()) + } + + if (uri.getQuery() != null) { + builder.append('?') + builder.append(uri.getQuery()) + } + + if (uri.getFragment() != null) { + builder.append('#') + builder.append(uri.getFragment()) + } + return builder.toString() + } +} + +class ApacheClientUriRequest extends ApacheHttpAsyncClientTest { + @Override + HttpUriRequest createRequest(String method, URI uri) { + return new HttpUriRequest(method, uri) + } + + @Override + HttpResponse executeRequest(HttpUriRequest request, URI uri) { + return client.execute(request, null).get() + } + + @Override + void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback callback) { + client.execute(request, callback) + } +} + +class ApacheClientHostRequest extends ApacheHttpAsyncClientTest { + @Override + HttpUriRequest createRequest(String method, URI uri) { + // also testing with absolute path below + return new HttpUriRequest(method, new URI(fullPathFromURI(uri))) + } + + @Override + HttpResponse executeRequest(HttpUriRequest request, URI uri) { + return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, null).get() + } + + @Override + void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback callback) { + client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, callback) + } +} + +class ApacheClientHostAbsoluteUriRequest extends ApacheHttpAsyncClientTest { + + @Override + HttpUriRequest createRequest(String method, URI uri) { + return new HttpUriRequest(method, new URI(uri.toString())) + } + + @Override + HttpResponse executeRequest(HttpUriRequest request, URI uri) { + return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, null).get() + } + + @Override + void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback callback) { + client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, callback) + } } diff --git a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientRequest.java b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientRequest.java index e3dd2cb1f7..8ca0ea96e2 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientRequest.java +++ b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientRequest.java @@ -26,14 +26,7 @@ public final class ApacheHttpClientRequest { private final HttpRequest delegate; public ApacheHttpClientRequest(HttpHost httpHost, HttpRequest httpRequest) { - URI calculatedUri; - try { - calculatedUri = new URI(httpHost.toURI() + httpRequest.getRequestLine().getUri()); - } catch (URISyntaxException e) { - logger.debug(e.getMessage(), e); - calculatedUri = null; - } - uri = calculatedUri; + uri = getCalculatedUri(httpHost, httpRequest); delegate = httpRequest; } @@ -121,4 +114,29 @@ public final class ApacheHttpClientRequest { return null; } } + + @Nullable + private static URI getCalculatedUri(HttpHost httpHost, HttpRequest httpRequest) { + final URI uri; + try { + // this can be relative or absolute + uri = new URI(httpRequest.getRequestLine().getUri()); + } catch (URISyntaxException e) { + logger.debug(e.getMessage(), e); + return null; + } + try { + return new URI( + httpHost.getSchemeName(), + null, + httpHost.getHostName(), + httpHost.getPort(), + uri.getPath(), + uri.getQuery(), + uri.getFragment()); + } catch (URISyntaxException e) { + logger.debug(e.getMessage(), e); + return null; + } + } } diff --git a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy index 3cbe141463..2387af50ae 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy +++ b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy @@ -94,11 +94,18 @@ abstract class ApacheHttpClientTest extends HttpClientTes } return builder.toString() } + + static String absoluteUriWithNonResolvingHost(URI uri) { + // substituting a non-resolving host and port to make sure that the host and port from this uri are not used + return new URI(uri.getScheme(), uri.getAuthority(), "nowhere", 1, uri.getPath(), uri.getQuery(), uri.getFragment()) + .toString() + } } class ApacheClientHostRequest extends ApacheHttpClientTest { @Override BasicHttpRequest createRequest(String method, URI uri) { + // also testing with absolute path below return new BasicHttpRequest(method, fullPathFromURI(uri)) } @@ -115,9 +122,29 @@ class ApacheClientHostRequest extends ApacheHttpClientTest { } } +class ApacheClientHostAbsoluteUriRequest extends ApacheHttpClientTest { + @Override + BasicHttpRequest createRequest(String method, URI uri) { + return new BasicHttpRequest(method, absoluteUriWithNonResolvingHost(uri)) + } + + @Override + HttpResponse executeRequest(BasicHttpRequest request, URI uri) { + return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request) + } + + @Override + void executeRequestWithCallback(BasicHttpRequest request, URI uri, Consumer callback) { + client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request) { + callback.accept(it) + } + } +} + class ApacheClientHostRequestContext extends ApacheHttpClientTest { @Override BasicHttpRequest createRequest(String method, URI uri) { + // also testing with absolute path below return new BasicHttpRequest(method, fullPathFromURI(uri)) } @@ -134,6 +161,25 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest { + @Override + BasicHttpRequest createRequest(String method, URI uri) { + return new BasicHttpRequest(method, absoluteUriWithNonResolvingHost(uri)) + } + + @Override + HttpResponse executeRequest(BasicHttpRequest request, URI uri) { + return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, new BasicHttpContext()) + } + + @Override + void executeRequestWithCallback(BasicHttpRequest request, URI uri, Consumer callback) { + client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, { + callback.accept(it) + }, new BasicHttpContext()) + } +} + class ApacheClientUriRequest extends ApacheHttpClientTest { @Override HttpUriRequest createRequest(String method, URI uri) { diff --git a/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy b/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy index 50aebea264..19fe50609b 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy +++ b/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy @@ -120,6 +120,29 @@ class ApacheClientHostRequest extends ApacheHttpClientTest { } } +class ApacheClientHostAbsoluteUriRequest extends ApacheHttpClientTest { + @Override + ClassicHttpRequest createRequest(String method, URI uri) { + // TODO(trask) substitute a non-resolving host and port to make sure that the host and port + // from this uri are not used (currently that causes redirect tests to fail + // because Apache HttpClient 5 appears to resolve relative redirects against this uri + // instead of against the host, which is different from Apache HttpClient 4 behavior) + return new BasicClassicHttpRequest(method, uri.toString()) + } + + @Override + ClassicHttpResponse executeRequest(ClassicHttpRequest request, URI uri) { + return client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request) + } + + @Override + void executeRequestWithCallback(ClassicHttpRequest request, URI uri, Consumer callback) { + client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request) { + callback.accept(it) + } + } +} + class ApacheClientHostRequestContext extends ApacheHttpClientTest { @Override ClassicHttpRequest createRequest(String method, URI uri) { @@ -139,6 +162,29 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest { + @Override + ClassicHttpRequest createRequest(String method, URI uri) { + // TODO(trask) substitute a non-resolving host and port to make sure that the host and port + // from this uri are not used (currently that causes redirect tests to fail + // because Apache HttpClient 5 appears to resolve relative redirects against this uri + // instead of against the host, which is different from Apache HttpClient 4 behavior) + return new BasicClassicHttpRequest(method, uri.toString()) + } + + @Override + ClassicHttpResponse executeRequest(ClassicHttpRequest request, URI uri) { + return client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request, new BasicHttpContext()) + } + + @Override + void executeRequestWithCallback(ClassicHttpRequest request, URI uri, Consumer callback) { + client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request, new BasicHttpContext()) { + callback.accept(it) + } + } +} + class ApacheClientUriRequest extends ApacheHttpClientTest { @Override ClassicHttpRequest createRequest(String method, URI uri) {