[GitHub] [beam] egalpin commented on a diff in pull request #17297: [BEAM-14000] Fixes Elastic search IO doesnot work when both password and keystore are used

[GitBox <gi...@apache.org>]

egalpin commented on code in PR #17297:
URL: https://github.com/apache/beam/pull/17297#discussion_r844232434


##########
sdks/java/io/elasticsearch/src/main/java/org/apache/beam/sdk/io/elasticsearch/ElasticsearchIO.java:
##########
@@ -637,13 +638,16 @@ RestClient createClient() throws IOException {
           final SSLContext sslContext =
               SSLContexts.custom().loadTrustMaterial(keyStore, trustStrategy).build();
           final SSLIOSessionStrategy sessionStrategy = new SSLIOSessionStrategy(sslContext);
-          restClientBuilder.setHttpClientConfigCallback(
-              httpClientBuilder ->
-                  httpClientBuilder.setSSLContext(sslContext).setSSLStrategy(sessionStrategy));
+          httpAsyncClientBuilder.setSSLContext(sslContext).setSSLStrategy(sessionStrategy);
         } catch (Exception e) {
           throw new IOException("Can't load the client certificate from the keystore", e);
         }
       }
+
+      if (getUsername() != null || (getKeystorePath() != null && !getKeystorePath().isEmpty())) {
+        restClientBuilder.setHttpClientConfigCallback(httpClientBuilder -> httpAsyncClientBuilder);

Review Comment:
   If I'm not mistaken, I _believe_ this will completely replace the instance of `HttpAsyncClientBuilder` created in `RestClientBuilder`[1].  With the previous implementation, some settings like `setSSLContext` would be overwritten but all others left left intact.  We'll need to find a way to perform this additive application of settings on the builder across both username and SSL without outright replacement of the default `HttpAsyncClientBuilder` in `RestClientBuilder`.  This can be achieved using a similar technique as what's on lines 647-667 (just below this) in this file. Ex. (not tested code)
   
   ```java
         restClientBuilder.setHttpClientConfigCallback(
             httpClientBuilder -> {
               if (getUsername() != null) {
                 final CredentialsProvider credentialsProvider = new BasicCredentialsProvider();
                 credentialsProvider.setCredentials(
                     AuthScope.ANY, new UsernamePasswordCredentials(getUsername(), getPassword()));
                 httpClientBuilder.setDefaultCredentialsProvider(credentialsProvider);
               }
               if (getKeystorePath() != null && !getKeystorePath().isEmpty()) {
                 KeyStore keyStore = null;
                 keyStore = KeyStore.getInstance("jks");
                 try (InputStream is = new FileInputStream(new File(getKeystorePath()))) {
                   String keystorePassword = getKeystorePassword();
                   keyStore.load(is, (keystorePassword == null) ? null : keystorePassword.toCharArray());
                 }
                 final TrustStrategy trustStrategy =
                     isTrustSelfSignedCerts() ? new TrustSelfSignedStrategy() : null;
                 final SSLContext sslContext =
                     SSLContexts.custom().loadTrustMaterial(keyStore, trustStrategy).build();
                 final SSLIOSessionStrategy sessionStrategy = new SSLIOSessionStrategy(sslContext);
                 httpClientBuilder.setSSLContext(sslContext).setSSLStrategy(sessionStrategy);
               }
               return httpClientBuilder;
             });
   ```
   
   [1] https://github.com/elastic/elasticsearch/blob/v7.8.0/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java#L209-L213



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org