From d728615ffa46cc6520b0a674ea342c7b7abb9198 Mon Sep 17 00:00:00 2001 From: NicklasWallgren Date: Thu, 4 Jan 2024 16:01:59 +0100 Subject: [PATCH] Fix NPE in ApacheHttpClientApiAdapter#getHostName (#3479) * Fix NPE in ApacheHttpClientApiAdapter#getHostName. * Implement test for HttpClient#executeOpen * Update CHANGELOG.asciidoc --- CHANGELOG.asciidoc | 4 + .../AbstractApacheHttpClientAdvice.java | 5 +- .../common/ApacheHttpClientApiAdapter.java | 4 +- .../helper/ApacheHttpClient4ApiAdapter.java | 2 +- .../v5/ApacheHttpClient5Instrumentation.java | 2 +- .../helper/ApacheHttpClient5ApiAdapter.java | 22 +++++- ...pClientExecuteOpenInstrumentationTest.java | 78 +++++++++++++++++++ 7 files changed, 110 insertions(+), 7 deletions(-) create mode 100644 apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient5-plugin/src/test/java/co/elastic/apm/agent/httpclient/v5/ApacheHttpClientExecuteOpenInstrumentationTest.java diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index a0c8f7563f..0eae1db9ed 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -31,6 +31,10 @@ Use subheadings with the "=====" level for adding notes for unreleased changes: === Unreleased +[float] +===== Bug fixes +* Fixed NPE in ApacheHttpClientApiAdapter#getHostName - {pull}3479[#3479] + [[release-notes-1.x]] === Java Agent version 1.x diff --git a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/AbstractApacheHttpClientAdvice.java b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/AbstractApacheHttpClientAdvice.java index fce3555a6d..aa6c60f04d 100644 --- a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/AbstractApacheHttpClientAdvice.java +++ b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/AbstractApacheHttpClientAdvice.java @@ -27,6 +27,7 @@ import co.elastic.apm.agent.tracer.dispatch.TextHeaderGetter; import co.elastic.apm.agent.tracer.dispatch.TextHeaderSetter; +import javax.annotation.Nullable; import java.net.URISyntaxException; public abstract class AbstractApacheHttpClientAdvice { @@ -36,12 +37,12 @@ public abstract class AbstractApacheHttpClientAdvice { TextHeaderGetter> Object startSpan(final Tracer tracer, final ApacheHttpClientApiAdapter adapter, final WRAPPER request, - final HTTPHOST httpHost, + @Nullable final HTTPHOST httpHost, final HeaderAccessor headerAccessor) throws URISyntaxException { ElasticContext elasticContext = tracer.currentContext(); Span span = null; if (elasticContext.getSpan() != null) { - span = HttpClientHelper.startHttpClientSpan(elasticContext, adapter.getMethod(request), adapter.getUri(request), adapter.getHostName(httpHost)); + span = HttpClientHelper.startHttpClientSpan(elasticContext, adapter.getMethod(request), adapter.getUri(request), adapter.getHostName(httpHost, request)); if (span != null) { span.activate(); } diff --git a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/ApacheHttpClientApiAdapter.java b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/ApacheHttpClientApiAdapter.java index 4363cc3e17..a3834035cd 100644 --- a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/ApacheHttpClientApiAdapter.java +++ b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient-common/src/main/java/co/elastic/apm/agent/httpclient/common/ApacheHttpClientApiAdapter.java @@ -19,6 +19,7 @@ package co.elastic.apm.agent.httpclient.common; +import javax.annotation.Nullable; import java.net.URI; import java.net.URISyntaxException; @@ -27,7 +28,8 @@ public interface ApacheHttpClientApiAdapter { private static final ApacheHttpClient5ApiAdapter INSTANCE = new ApacheHttpClient5ApiAdapter(); + private static final Logger logger = LoggerFactory.getLogger(ApacheHttpClient5ApiAdapter.class); + private ApacheHttpClient5ApiAdapter() { } @@ -49,8 +56,19 @@ public URI getUri(ClassicHttpRequest request) throws URISyntaxException { } @Override - public CharSequence getHostName(HttpHost httpHost) { - return httpHost.getHostName(); + public CharSequence getHostName(@Nullable HttpHost httpHost, ClassicHttpRequest request) { + if (httpHost != null) { + return httpHost.getHostName(); + } + + try { + return RoutingSupport.determineHost(request) + .getHostName(); + } catch (HttpException e) { + logger.error("Exception while determining HostName", e); + + return null; + } } @Override diff --git a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient5-plugin/src/test/java/co/elastic/apm/agent/httpclient/v5/ApacheHttpClientExecuteOpenInstrumentationTest.java b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient5-plugin/src/test/java/co/elastic/apm/agent/httpclient/v5/ApacheHttpClientExecuteOpenInstrumentationTest.java new file mode 100644 index 0000000000..ef04c9f59e --- /dev/null +++ b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient5-plugin/src/test/java/co/elastic/apm/agent/httpclient/v5/ApacheHttpClientExecuteOpenInstrumentationTest.java @@ -0,0 +1,78 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package co.elastic.apm.agent.httpclient.v5; + + +import co.elastic.apm.agent.httpclient.AbstractHttpClientInstrumentationTest; +import org.apache.hc.client5.http.ClientProtocolException; +import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.HttpClients; +import org.apache.hc.core5.http.ClassicHttpResponse; +import org.apache.hc.core5.http.HttpEntity; +import org.apache.hc.core5.http.io.HttpClientResponseHandler; +import org.apache.hc.core5.http.io.entity.EntityUtils; +import org.junit.AfterClass; +import org.junit.BeforeClass; + +import java.io.IOException; + +public class ApacheHttpClientExecuteOpenInstrumentationTest extends AbstractHttpClientInstrumentationTest { + + private static CloseableHttpClient client; + + @BeforeClass + public static void setUp() { + client = HttpClients.createDefault(); + } + + @AfterClass + public static void close() throws IOException { + client.close(); + } + + /** + * RFC 7230: treat presence of userinfo in authority component in request URI as an HTTP protocol violation. + * + * Uses {@link org.apache.hc.core5.http.message.BasicHttpRequest#setUri} to fill {@link org.apache.hc.core5.net.URIAuthority} + * + * Assertions on authority in {@link org.apache.hc.client5.http.impl.classic.ProtocolExec#execute} + */ + @Override + public boolean isTestHttpCallWithUserInfoEnabled() { + return false; + } + + @Override + protected void performGet(String path) throws Exception { + HttpClientResponseHandler responseHandler = response -> { + int status = response.getCode(); + if (status >= 200 && status < 300) { + HttpEntity entity = response.getEntity(); + String res = entity != null ? EntityUtils.toString(entity) : null; + return res; + } else { + throw new ClientProtocolException("Unexpected response status: " + status); + } + }; + + ClassicHttpResponse response = client.executeOpen(null, new HttpGet(path), null); + responseHandler.handleResponse(response); + } +}