Copilot commented on code in PR #16411:
URL: https://github.com/apache/pinot/pull/16411#discussion_r2225166966
##########
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java:
##########
@@ -225,21 +240,51 @@ public static URI getDirectoryURI(String uriStr)
}
/**
- * Retrieve a URL via GET request, with an optional authorization token.
+ * Retrieves the content of the given URL using a GET request.
+ * Supports HTTPS connections, including those with self-signed certificates.
*
- * @param url target url
- * @param authToken optional auth token, or null
- * @return fetched document
- * @throws IOException on connection problems
+ * @param url The target URL to fetch.
+ * @param authToken Optional authorization token to include in the request
header, or null.
+ * @return The response body as a string.
+ * @throws IOException If an error occurs during the connection or reading
the response.
*/
- private static String fetchUrl(URL url, String authToken)
+ public static String fetchUrl(URL url, String authToken, TlsSpec tlsSpec)
throws IOException {
- URLConnection connection = url.openConnection();
+ try {
+ URLConnection connection = url.openConnection();
+
+ if (connection instanceof HttpsURLConnection) {
+ HttpsURLConnection httpsConn = (HttpsURLConnection) connection;
+
+ if (tlsSpec != null) {
+ TrustManagerFactory tmf = TlsUtils.createTrustManagerFactory(
+ tlsSpec.getTrustStorePath(),
+ tlsSpec.getTrustStorePassword(),
+ tlsSpec.getTrustStoreType());
+
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
+
+ httpsConn.setSSLSocketFactory(sslContext.getSocketFactory());
+ }
+ connection = httpsConn;
+ }
+
+ if (StringUtils.isNotBlank(authToken)) {
+ connection.setRequestProperty("Authorization", authToken);
+ }
+
+ connection.setConnectTimeout(5000);
Review Comment:
The timeout values (5000ms) are hardcoded magic numbers. Consider making
these configurable through the TlsSpec or as constants to improve
maintainability.
##########
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java:
##########
@@ -225,21 +240,51 @@ public static URI getDirectoryURI(String uriStr)
}
/**
- * Retrieve a URL via GET request, with an optional authorization token.
+ * Retrieves the content of the given URL using a GET request.
+ * Supports HTTPS connections, including those with self-signed certificates.
*
- * @param url target url
- * @param authToken optional auth token, or null
- * @return fetched document
- * @throws IOException on connection problems
+ * @param url The target URL to fetch.
+ * @param authToken Optional authorization token to include in the request
header, or null.
+ * @return The response body as a string.
+ * @throws IOException If an error occurs during the connection or reading
the response.
*/
- private static String fetchUrl(URL url, String authToken)
+ public static String fetchUrl(URL url, String authToken, TlsSpec tlsSpec)
throws IOException {
- URLConnection connection = url.openConnection();
+ try {
+ URLConnection connection = url.openConnection();
+
+ if (connection instanceof HttpsURLConnection) {
+ HttpsURLConnection httpsConn = (HttpsURLConnection) connection;
+
+ if (tlsSpec != null) {
+ TrustManagerFactory tmf = TlsUtils.createTrustManagerFactory(
+ tlsSpec.getTrustStorePath(),
+ tlsSpec.getTrustStorePassword(),
+ tlsSpec.getTrustStoreType());
+
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
+
+ httpsConn.setSSLSocketFactory(sslContext.getSocketFactory());
+ }
+ connection = httpsConn;
+ }
+
+ if (StringUtils.isNotBlank(authToken)) {
+ connection.setRequestProperty("Authorization", authToken);
+ }
+
+ connection.setConnectTimeout(5000);
+ connection.setReadTimeout(5000);
Review Comment:
The timeout values (5000ms) are hardcoded magic numbers. Consider making
these configurable through the TlsSpec or as constants to improve
maintainability.
##########
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java:
##########
@@ -225,21 +240,51 @@ public static URI getDirectoryURI(String uriStr)
}
/**
- * Retrieve a URL via GET request, with an optional authorization token.
+ * Retrieves the content of the given URL using a GET request.
+ * Supports HTTPS connections, including those with self-signed certificates.
*
- * @param url target url
- * @param authToken optional auth token, or null
- * @return fetched document
- * @throws IOException on connection problems
+ * @param url The target URL to fetch.
+ * @param authToken Optional authorization token to include in the request
header, or null.
+ * @return The response body as a string.
+ * @throws IOException If an error occurs during the connection or reading
the response.
*/
- private static String fetchUrl(URL url, String authToken)
+ public static String fetchUrl(URL url, String authToken, TlsSpec tlsSpec)
throws IOException {
- URLConnection connection = url.openConnection();
+ try {
+ URLConnection connection = url.openConnection();
+
+ if (connection instanceof HttpsURLConnection) {
+ HttpsURLConnection httpsConn = (HttpsURLConnection) connection;
+
+ if (tlsSpec != null) {
+ TrustManagerFactory tmf = TlsUtils.createTrustManagerFactory(
+ tlsSpec.getTrustStorePath(),
+ tlsSpec.getTrustStorePassword(),
+ tlsSpec.getTrustStoreType());
+
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
+
+ httpsConn.setSSLSocketFactory(sslContext.getSocketFactory());
+ }
+ connection = httpsConn;
+ }
+
+ if (StringUtils.isNotBlank(authToken)) {
+ connection.setRequestProperty("Authorization", authToken);
+ }
+
+ connection.setConnectTimeout(5000);
+ connection.setReadTimeout(5000);
+
+ if (connection instanceof HttpURLConnection) {
+ ((HttpURLConnection) connection).setRequestMethod("GET");
+ }
- if (StringUtils.isNotBlank(authToken)) {
- connection.setRequestProperty("Authorization", authToken);
+ return IOUtils.toString(connection.getInputStream(),
StandardCharsets.UTF_8);
+ } catch (Exception e) {
+ throw new IOException("Failed to fetch URL: " + url, e);
Review Comment:
Catching generic Exception is too broad and can mask unexpected errors.
Consider catching specific exceptions like SSLException,
GeneralSecurityException, and IOException separately for better error handling.
```suggestion
} catch (IOException e) {
throw new IOException("I/O error while fetching URL: " + url, e);
} catch (GeneralSecurityException e) {
throw new IOException("Security error while setting up SSL context for
URL: " + url, e);
} catch (RuntimeException e) {
throw new IOException("Unexpected error while fetching URL: " + url,
e);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
