Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to Apache HttpClient / Core 5.x #281

Merged
merged 2 commits into from
Oct 14, 2022
Merged

Migrate to Apache HttpClient / Core 5.x #281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
625743a
Migrate to Apache HttpClient / Core 5.x
sbcd90 Oct 13, 2022
5a0b10d
Migrate to Apache HttpClient / Core 5.x
sbcd90 Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main/java/org/opensearch/commons/authuser/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
import java.util.Map;
import java.util.Objects;

import org.apache.http.util.EntityUtils;
import org.apache.hc.core5.http.ParseException;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.opensearch.client.Response;
import org.opensearch.common.Nullable;
import org.opensearch.common.Strings;
Expand Down Expand Up @@ -83,7 +84,7 @@ public User(
* @param response
* @throws IOException
*/
public User(final Response response) throws IOException {
public User(final Response response) throws IOException, ParseException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid throwing this exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot @eirsep for review.
i think we cannot avoid it as it is thrown by the library method itself unless we want to transform it to IOException which may be misinterpreted. looks like core changes also throw the error https://github.com/opensearch-project/OpenSearch/blob/4833e080cc506237fee5aa90767d6f49d952bec3/client/rest-high-level/src/test/java/org/opensearch/client/OpenSearchRestHighLevelClientTestCase.java#L329

this(EntityUtils.toString(response.getEntity()));
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/commons/destination/message/LegacyBaseMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import java.net.URISyntaxException;
import java.util.Map;

import org.apache.http.client.utils.URIBuilder;
import org.apache.hc.core5.net.URIBuilder;
import org.opensearch.common.Strings;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/opensearch/commons/destination/message/LegacyCustomWebhookMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
import java.net.URI;
import java.util.Map;

import org.apache.http.client.methods.HttpPatch;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
import org.apache.hc.client5.http.classic.methods.HttpPatch;
import org.apache.hc.client5.http.classic.methods.HttpPost;
import org.apache.hc.client5.http.classic.methods.HttpPut;
import org.opensearch.common.Strings;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
Expand Down
44 changes: 27 additions & 17 deletions src/main/java/org/opensearch/commons/rest/SecureRestClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,20 @@

import javax.net.ssl.SSLContext;

import org.apache.http.HttpHost;
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.UsernamePasswordCredentials;
import org.apache.http.client.CredentialsProvider;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.conn.ssl.TrustSelfSignedStrategy;
import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.http.impl.nio.client.HttpAsyncClientBuilder;
import org.apache.http.ssl.SSLContextBuilder;
import org.apache.hc.client5.http.auth.AuthScope;
import org.apache.hc.client5.http.auth.CredentialsProvider;
import org.apache.hc.client5.http.auth.UsernamePasswordCredentials;
import org.apache.hc.client5.http.config.RequestConfig;
import org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder;
import org.apache.hc.client5.http.impl.auth.BasicCredentialsProvider;
import org.apache.hc.client5.http.impl.nio.PoolingAsyncClientConnectionManager;
import org.apache.hc.client5.http.impl.nio.PoolingAsyncClientConnectionManagerBuilder;
import org.apache.hc.client5.http.ssl.ClientTlsStrategyBuilder;
import org.apache.hc.client5.http.ssl.TrustSelfSignedStrategy;
import org.apache.hc.core5.http.HttpHost;
import org.apache.hc.core5.http.nio.ssl.TlsStrategy;
import org.apache.hc.core5.ssl.SSLContextBuilder;
import org.apache.hc.core5.util.Timeout;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.OpenSearchException;
Expand Down Expand Up @@ -96,7 +101,7 @@ public SecureRestClientBuilder(
this.passwd = passWord;
this.settings = Settings.EMPTY;
this.configPath = null;
hosts.add(new HttpHost(host, port, httpSSLEnabled ? ConfigConstants.HTTPS : ConfigConstants.HTTP));
hosts.add(new HttpHost(httpSSLEnabled ? ConfigConstants.HTTPS : ConfigConstants.HTTP, host, port));
}

/**
Expand Down Expand Up @@ -129,7 +134,7 @@ public SecureRestClientBuilder(Settings settings, Path configPath) {
this.passwd = null;
String host = ConfigConstants.HOST_DEFAULT;
int port = settings.getAsInt(ConfigConstants.HTTP_PORT, ConfigConstants.HTTP_PORT_DEFAULT);
hosts.add(new HttpHost(host, port, httpSSLEnabled ? ConfigConstants.HTTPS : ConfigConstants.HTTP));
hosts.add(new HttpHost(httpSSLEnabled ? ConfigConstants.HTTPS : ConfigConstants.HTTP, host, port));
}

/**
Expand Down Expand Up @@ -172,9 +177,9 @@ private RestClientBuilder createRestClientBuilder() throws IOException {
@Override
public RequestConfig.Builder customizeRequestConfig(RequestConfig.Builder requestConfigBuilder) {
return requestConfigBuilder
.setConnectTimeout(defaultConnectTimeOutMSecs)
.setSocketTimeout(defaultSoTimeoutMSecs)
.setConnectionRequestTimeout(defaultConnRequestTimeoutMSecs);
.setConnectTimeout(Timeout.ofMilliseconds(defaultConnectTimeOutMSecs))
.setResponseTimeout(Timeout.ofMilliseconds(defaultSoTimeoutMSecs))
.setConnectionRequestTimeout(Timeout.ofMilliseconds(defaultConnRequestTimeoutMSecs));
}
});

Expand All @@ -189,7 +194,12 @@ public RequestConfig.Builder customizeRequestConfig(RequestConfig.Builder reques
@Override
public HttpAsyncClientBuilder customizeHttpClient(HttpAsyncClientBuilder httpClientBuilder) {
if (sslContext != null) {
httpClientBuilder.setSSLContext(sslContext);
TlsStrategy tlsStrategy = ClientTlsStrategyBuilder.create().setSslContext(sslContext).build();
PoolingAsyncClientConnectionManager connectionManager = PoolingAsyncClientConnectionManagerBuilder
.create()
.setTlsStrategy(tlsStrategy)
.build();
httpClientBuilder.setConnectionManager(connectionManager);
}
if (credentialsProvider != null) {
httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider);
Expand Down Expand Up @@ -227,8 +237,8 @@ private CredentialsProvider createCredsProvider() {
if (Strings.isNullOrEmpty(user) || Strings.isNullOrEmpty(passwd))
return null;

final CredentialsProvider credentialsProvider = new BasicCredentialsProvider();
credentialsProvider.setCredentials(AuthScope.ANY, new UsernamePasswordCredentials(user, passwd));
final BasicCredentialsProvider credentialsProvider = new BasicCredentialsProvider();
credentialsProvider.setCredentials(new AuthScope(null, -1), new UsernamePasswordCredentials(user, passwd.toCharArray()));
return credentialsProvider;
}

Expand Down
24 changes: 17 additions & 7 deletions src/main/kotlin/org/opensearch/commons/alerting/model/ClusterMetricsInput.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.opensearch.commons.alerting.model

import org.apache.commons.validator.routines.UrlValidator
import org.apache.http.client.utils.URIBuilder
import org.apache.hc.core5.net.URIBuilder
import org.opensearch.common.CheckedFunction
import org.opensearch.common.ParseField
import org.opensearch.common.io.stream.StreamInput
Expand All @@ -13,6 +13,7 @@ import org.opensearch.common.xcontent.XContentParser
import org.opensearch.common.xcontent.XContentParserUtils
import java.io.IOException
import java.net.URI
import java.net.URISyntaxException

val ILLEGAL_PATH_PARAMETER_CHARACTERS = arrayOf(':', '"', '+', '\\', '|', '?', '#', '>', '<', ' ')

Expand Down Expand Up @@ -200,12 +201,21 @@ data class ClusterMetricsInput(
* @return The constructed [URI].
*/
private fun constructUrlFromInputs(): URI {
val uriBuilder = URIBuilder()
.setScheme(SUPPORTED_SCHEME)
.setHost(SUPPORTED_HOST)
.setPort(SUPPORTED_PORT)
.setPath(path + pathParams)
return uriBuilder.build()
/**
* this try-catch block is required due to a httpcomponents 5.1.x library issue
* it auto encodes path params in the url.
*/
return try {
val uriBuilder = URIBuilder("$SUPPORTED_SCHEME://$SUPPORTED_HOST:$SUPPORTED_PORT$path$pathParams")
uriBuilder.build()
} catch (ex: URISyntaxException) {
val uriBuilder = URIBuilder()
.setScheme(SUPPORTED_SCHEME)
.setHost(SUPPORTED_HOST)
.setPort(SUPPORTED_PORT)
.setPath(path + pathParams)
uriBuilder.build()
}
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/org/opensearch/commons/destination/message/LegacyCustomWebhookMessageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
import java.util.HashMap;
import java.util.Map;

import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpPost;
import org.apache.hc.client5.http.classic.methods.HttpGet;
import org.apache.hc.client5.http.classic.methods.HttpPost;
import org.junit.jupiter.api.Test;
import org.opensearch.common.io.stream.BytesStreamOutput;
import org.opensearch.common.io.stream.StreamInput;
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/opensearch/commons/rest/IntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import java.nio.file.Path;
import java.nio.file.Paths;

import org.apache.http.util.EntityUtils;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.opensearch.client.Request;
Expand Down
4 changes: 2 additions & 2 deletions src/test/kotlin/org/opensearch/commons/alerting/TestHelpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ package org.opensearch.commons.alerting
import com.carrotsearch.randomizedtesting.generators.RandomNumbers
import com.carrotsearch.randomizedtesting.generators.RandomStrings
import junit.framework.TestCase.assertNull
import org.apache.http.Header
import org.apache.http.HttpEntity
import org.apache.hc.core5.http.Header
import org.apache.hc.core5.http.HttpEntity
import org.opensearch.client.Request
import org.opensearch.client.RequestOptions
import org.opensearch.client.Response
Expand Down