richardstartin commented on a change in pull request #8329: URL: https://github.com/apache/pinot/pull/8329#discussion_r829533085
########## File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TlsUtils.java ########## @@ -255,6 +257,14 @@ public static URL makeKeyStoreUrl(String storePath) return inputUri.toURL(); } + public static SSLContext getSslContext() { + return _sslContext != null ? _sslContext : SSLContexts.createDefault(); Review comment: I suggest putting it in a holder class: ```java private static final AtomicReference<SSLContext> OVERRIDE = new AtomicReference<>(); public static void registerOverride(SSLContext override) { if (!OVERRIDE.compareAndSet(null, override)) { // warn that something else beat the caller here } } private static final class Holder { static final SSLContext SSL_CONTEXT = OVERRIDE.get() == null ? SSLContexts.createDefault() : OVERRIDE.get(); } ``` And then reference it via `Holder.SSL_CONTEXT` everywhere. This has a few advantages: * registration is thread safe * mutable until first use * synchronization, at most once initialisation guaranteed by the classloader * after initialisation, the `SSLContext` is constant which can drive optimisations like constant folding, which may or may not be important ########## File path: pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java ########## @@ -0,0 +1,364 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.pinot.common.utils.http; + +import java.io.IOException; +import java.net.URI; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import javax.annotation.Nullable; +import javax.net.ssl.SSLContext; +import org.apache.commons.collections.MapUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.http.Header; +import org.apache.http.HttpEntity; +import org.apache.http.HttpHeaders; +import org.apache.http.HttpVersion; +import org.apache.http.NameValuePair; +import org.apache.http.StatusLine; +import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.client.methods.HttpPut; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.client.methods.RequestBuilder; +import org.apache.http.conn.ssl.NoopHostnameVerifier; +import org.apache.http.conn.ssl.SSLConnectionSocketFactory; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; +import org.apache.http.entity.mime.MultipartEntityBuilder; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClients; +import org.apache.http.message.BasicHeader; +import org.apache.http.util.EntityUtils; +import org.apache.pinot.common.exception.HttpErrorStatusException; +import org.apache.pinot.common.utils.SimpleHttpErrorInfo; +import org.apache.pinot.common.utils.SimpleHttpResponse; +import org.apache.pinot.common.utils.TlsUtils; +import org.apache.pinot.spi.utils.CommonConstants; +import org.apache.pinot.spi.utils.JsonUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * The {@code HTTPClient} wraps around a {@link CloseableHttpClient} to provide a reusable client for making + * HTTP requests. + */ +public class HttpClient implements AutoCloseable { + private static final Logger LOGGER = LoggerFactory.getLogger(HttpClient.class); + + public static final int DEFAULT_SOCKET_TIMEOUT_MS = 600 * 1000; // 10 minutes + public static final int GET_REQUEST_SOCKET_TIMEOUT_MS = 5 * 1000; // 5 seconds + public static final int DELETE_REQUEST_SOCKET_TIMEOUT_MS = 10 * 1000; // 10 seconds + public static final String AUTH_HTTP_HEADER = "Authorization"; + public static final String JSON_CONTENT_TYPE = "application/json"; + + private final CloseableHttpClient _httpClient; + + public HttpClient() { + this(null); + } + + public HttpClient(@Nullable SSLContext sslContext) { + SSLContext context = sslContext != null ? sslContext : TlsUtils.getSslContext(); + // Set NoopHostnameVerifier to skip validating hostname when uploading/downloading segments. + SSLConnectionSocketFactory csf = new SSLConnectionSocketFactory(context, NoopHostnameVerifier.INSTANCE); + _httpClient = HttpClients.custom().setSSLSocketFactory(csf).build(); + } + + // -------------------------------------------------------------------------- + // Generic HTTP Request APIs + // -------------------------------------------------------------------------- + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendGetRequest(URI, String) + */ + public SimpleHttpResponse sendGetRequest(URI uri) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.get(uri).setVersion(HttpVersion.HTTP_1_1); + setTimeout(requestBuilder, GET_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + public SimpleHttpResponse sendGetRequest(URI uri, @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.get(uri).setVersion(HttpVersion.HTTP_1_1); + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + setTimeout(requestBuilder, GET_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendDeleteRequest(URI, String) + */ + public SimpleHttpResponse sendDeleteRequest(URI uri) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.delete(uri).setVersion(HttpVersion.HTTP_1_1); + setTimeout(requestBuilder, DELETE_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + public SimpleHttpResponse sendDeleteRequest(URI uri, @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.delete(uri).setVersion(HttpVersion.HTTP_1_1); + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + setTimeout(requestBuilder, DELETE_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendPostRequest(URI, HttpEntity, Map, String) + */ + public SimpleHttpResponse sendPostRequest(URI uri, HttpEntity payload, Map<String, String> headers) + throws IOException { + return sendPostRequest(uri, payload, headers, null); + } + + public SimpleHttpResponse sendPostRequest(URI uri, HttpEntity payload, Map<String, String> headers, + @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.post(uri).setVersion(HttpVersion.HTTP_1_1); + if (payload != null) { + requestBuilder.setEntity(payload); + } + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + if (MapUtils.isNotEmpty(headers)) { + for (Map.Entry<String, String> header : headers.entrySet()) { + requestBuilder.addHeader(header.getKey(), header.getValue()); + } + } + setTimeout(requestBuilder, DEFAULT_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendPutRequest(URI, HttpEntity, Map, String) + */ + public SimpleHttpResponse sendPutRequest(URI uri, HttpEntity payload, Map<String, String> headers) + throws IOException { + return sendPutRequest(uri, payload, headers, null); + } + + public SimpleHttpResponse sendPutRequest(URI uri, HttpEntity payload, Map<String, String> headers, + @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.put(uri).setVersion(HttpVersion.HTTP_1_1); + if (payload != null) { + requestBuilder.setEntity(payload); + } + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + setTimeout(requestBuilder, DELETE_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + // -------------------------------------------------------------------------- + // JSON post/put utility APIs + // -------------------------------------------------------------------------- + + public SimpleHttpResponse postJsonRequest(URI uri, @Nullable String jsonRequestBody) + throws IOException { + return postJsonRequest(uri, jsonRequestBody, null); + } + + public SimpleHttpResponse postJsonRequest(URI uri, @Nullable String requestBody, + @Nullable Map<String, String> headers) + throws IOException { + if (MapUtils.isEmpty(headers)) { + headers = new HashMap<>(); + } + headers.put(HttpHeaders.CONTENT_TYPE, JSON_CONTENT_TYPE); + HttpEntity entity = requestBody == null ? null : new StringEntity(requestBody, ContentType.APPLICATION_JSON); + return sendPostRequest(uri, entity, headers, null); + } + + public SimpleHttpResponse putJsonRequest(URI uri, @Nullable String jsonRequestBody) + throws IOException { + return putJsonRequest(uri, jsonRequestBody, null); + } + + public SimpleHttpResponse putJsonRequest(URI uri, @Nullable String requestBody, + @Nullable Map<String, String> headers) + throws IOException { + if (MapUtils.isEmpty(headers)) { + headers = new HashMap<>(); + } + headers.put(HttpHeaders.CONTENT_TYPE, JSON_CONTENT_TYPE); + HttpEntity entity = requestBody == null ? null : new StringEntity(requestBody, ContentType.APPLICATION_JSON); + return sendPutRequest(uri, entity, headers, null); + } + + // -------------------------------------------------------------------------- + // Lower-level request/execute APIs. + // -------------------------------------------------------------------------- + + public SimpleHttpResponse sendRequest(HttpUriRequest request) + throws IOException { + try (CloseableHttpResponse response = _httpClient.execute(request)) { + if (response.containsHeader(CommonConstants.Controller.HOST_HTTP_HEADER)) { + String controllerHost = response.getFirstHeader(CommonConstants.Controller.HOST_HTTP_HEADER).getValue(); + String controllerVersion = response.getFirstHeader(CommonConstants.Controller.VERSION_HTTP_HEADER).getValue(); + LOGGER.info(String + .format("Sending request: %s to controller: %s, version: %s", request.getURI(), controllerHost, Review comment: Yes, please do not use `String.format` - it's ridiculously expensive. Most of the time we use it, it is when something has gone wrong so I suppose it is ok but it should never be used on the happy path. ########## File path: pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java ########## @@ -0,0 +1,364 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.pinot.common.utils.http; + +import java.io.IOException; +import java.net.URI; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import javax.annotation.Nullable; +import javax.net.ssl.SSLContext; +import org.apache.commons.collections.MapUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.http.Header; +import org.apache.http.HttpEntity; +import org.apache.http.HttpHeaders; +import org.apache.http.HttpVersion; +import org.apache.http.NameValuePair; +import org.apache.http.StatusLine; +import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.client.methods.HttpPut; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.client.methods.RequestBuilder; +import org.apache.http.conn.ssl.NoopHostnameVerifier; +import org.apache.http.conn.ssl.SSLConnectionSocketFactory; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; +import org.apache.http.entity.mime.MultipartEntityBuilder; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClients; +import org.apache.http.message.BasicHeader; +import org.apache.http.util.EntityUtils; +import org.apache.pinot.common.exception.HttpErrorStatusException; +import org.apache.pinot.common.utils.SimpleHttpErrorInfo; +import org.apache.pinot.common.utils.SimpleHttpResponse; +import org.apache.pinot.common.utils.TlsUtils; +import org.apache.pinot.spi.utils.CommonConstants; +import org.apache.pinot.spi.utils.JsonUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * The {@code HTTPClient} wraps around a {@link CloseableHttpClient} to provide a reusable client for making + * HTTP requests. + */ +public class HttpClient implements AutoCloseable { + private static final Logger LOGGER = LoggerFactory.getLogger(HttpClient.class); + + public static final int DEFAULT_SOCKET_TIMEOUT_MS = 600 * 1000; // 10 minutes + public static final int GET_REQUEST_SOCKET_TIMEOUT_MS = 5 * 1000; // 5 seconds + public static final int DELETE_REQUEST_SOCKET_TIMEOUT_MS = 10 * 1000; // 10 seconds + public static final String AUTH_HTTP_HEADER = "Authorization"; + public static final String JSON_CONTENT_TYPE = "application/json"; + + private final CloseableHttpClient _httpClient; + + public HttpClient() { + this(null); + } + + public HttpClient(@Nullable SSLContext sslContext) { + SSLContext context = sslContext != null ? sslContext : TlsUtils.getSslContext(); + // Set NoopHostnameVerifier to skip validating hostname when uploading/downloading segments. + SSLConnectionSocketFactory csf = new SSLConnectionSocketFactory(context, NoopHostnameVerifier.INSTANCE); + _httpClient = HttpClients.custom().setSSLSocketFactory(csf).build(); + } + + // -------------------------------------------------------------------------- + // Generic HTTP Request APIs + // -------------------------------------------------------------------------- + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendGetRequest(URI, String) + */ + public SimpleHttpResponse sendGetRequest(URI uri) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.get(uri).setVersion(HttpVersion.HTTP_1_1); + setTimeout(requestBuilder, GET_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + public SimpleHttpResponse sendGetRequest(URI uri, @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.get(uri).setVersion(HttpVersion.HTTP_1_1); + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + setTimeout(requestBuilder, GET_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendDeleteRequest(URI, String) + */ + public SimpleHttpResponse sendDeleteRequest(URI uri) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.delete(uri).setVersion(HttpVersion.HTTP_1_1); + setTimeout(requestBuilder, DELETE_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + public SimpleHttpResponse sendDeleteRequest(URI uri, @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.delete(uri).setVersion(HttpVersion.HTTP_1_1); + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + setTimeout(requestBuilder, DELETE_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendPostRequest(URI, HttpEntity, Map, String) + */ + public SimpleHttpResponse sendPostRequest(URI uri, HttpEntity payload, Map<String, String> headers) + throws IOException { + return sendPostRequest(uri, payload, headers, null); + } + + public SimpleHttpResponse sendPostRequest(URI uri, HttpEntity payload, Map<String, String> headers, + @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.post(uri).setVersion(HttpVersion.HTTP_1_1); + if (payload != null) { + requestBuilder.setEntity(payload); + } + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + if (MapUtils.isNotEmpty(headers)) { + for (Map.Entry<String, String> header : headers.entrySet()) { + requestBuilder.addHeader(header.getKey(), header.getValue()); + } + } + setTimeout(requestBuilder, DEFAULT_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + /** + * Deprecated due to lack of auth header support. May break for deployments with auth enabled + * + * @see #sendPutRequest(URI, HttpEntity, Map, String) + */ + public SimpleHttpResponse sendPutRequest(URI uri, HttpEntity payload, Map<String, String> headers) + throws IOException { + return sendPutRequest(uri, payload, headers, null); + } + + public SimpleHttpResponse sendPutRequest(URI uri, HttpEntity payload, Map<String, String> headers, + @Nullable String authToken) + throws IOException { + RequestBuilder requestBuilder = RequestBuilder.put(uri).setVersion(HttpVersion.HTTP_1_1); + if (payload != null) { + requestBuilder.setEntity(payload); + } + if (StringUtils.isNotBlank(authToken)) { + requestBuilder.addHeader(AUTH_HTTP_HEADER, authToken); + } + setTimeout(requestBuilder, DELETE_REQUEST_SOCKET_TIMEOUT_MS); + return sendRequest(requestBuilder.build()); + } + + // -------------------------------------------------------------------------- + // JSON post/put utility APIs + // -------------------------------------------------------------------------- + + public SimpleHttpResponse postJsonRequest(URI uri, @Nullable String jsonRequestBody) + throws IOException { + return postJsonRequest(uri, jsonRequestBody, null); + } + + public SimpleHttpResponse postJsonRequest(URI uri, @Nullable String requestBody, + @Nullable Map<String, String> headers) + throws IOException { + if (MapUtils.isEmpty(headers)) { + headers = new HashMap<>(); + } + headers.put(HttpHeaders.CONTENT_TYPE, JSON_CONTENT_TYPE); + HttpEntity entity = requestBody == null ? null : new StringEntity(requestBody, ContentType.APPLICATION_JSON); + return sendPostRequest(uri, entity, headers, null); + } + + public SimpleHttpResponse putJsonRequest(URI uri, @Nullable String jsonRequestBody) + throws IOException { + return putJsonRequest(uri, jsonRequestBody, null); + } + + public SimpleHttpResponse putJsonRequest(URI uri, @Nullable String requestBody, + @Nullable Map<String, String> headers) + throws IOException { + if (MapUtils.isEmpty(headers)) { + headers = new HashMap<>(); + } + headers.put(HttpHeaders.CONTENT_TYPE, JSON_CONTENT_TYPE); + HttpEntity entity = requestBody == null ? null : new StringEntity(requestBody, ContentType.APPLICATION_JSON); + return sendPutRequest(uri, entity, headers, null); + } + + // -------------------------------------------------------------------------- + // Lower-level request/execute APIs. + // -------------------------------------------------------------------------- + + public SimpleHttpResponse sendRequest(HttpUriRequest request) + throws IOException { + try (CloseableHttpResponse response = _httpClient.execute(request)) { + if (response.containsHeader(CommonConstants.Controller.HOST_HTTP_HEADER)) { + String controllerHost = response.getFirstHeader(CommonConstants.Controller.HOST_HTTP_HEADER).getValue(); + String controllerVersion = response.getFirstHeader(CommonConstants.Controller.VERSION_HTTP_HEADER).getValue(); + LOGGER.info(String + .format("Sending request: %s to controller: %s, version: %s", request.getURI(), controllerHost, Review comment: In this particular case, since there are 3 parameters a varargs array will be allocated for every log statement which will always be logged since it's at INFO, so it would be better just to concatenate the strings to minimise logging overhead. ########## File path: pinot-common/src/main/java/org/apache/pinot/common/utils/TlsUtils.java ########## @@ -255,6 +262,38 @@ public static URL makeKeyStoreUrl(String storePath) return inputUri.toURL(); } + public static SSLContext getSslContext() { + return SSLContextHolder.SSL_CONTEXT; + } + + public static void setSslContext(SSLContext sslContext) { + registerOverride(sslContext); + } + + private static void registerOverride(SSLContext override) { + if (!SSL_CONTEXT_REF.compareAndSet(null, override)) { + LOGGER.warn("SSL Context has already been set."); + // warn that something else beat the caller here + } + } Review comment: oh I'd just inline this into `setSslContext`, `registerOverride` was just a name -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org