Copilot commented on code in PR #13641:
URL: https://github.com/apache/skywalking/pull/13641#discussion_r2659696838
##########
oap-server/server-starter/src/main/resources/application.yml:
##########
@@ -160,6 +160,8 @@ storage:
password: ${SW_ES_PASSWORD:""}
trustStorePath: ${SW_STORAGE_ES_SSL_JKS_PATH:""}
trustStorePass: ${SW_STORAGE_ES_SSL_JKS_PASS:""}
+ keyStorePath: ${SW_STORAGE_ES_SSL_KEY_STORE_PATH:""} # Path to client
certificate keystore for mutual TLS (OpenSearch/Elasticsearch client cert
auth). Supports PKCS12 (.p12, .pfx) and JKS (.jks) formats.
+ keyStorePass: ${SW_STORAGE_ES_SSL_KEY_STORE_PASS:""} # Password for the
client certificate keystore. Can be managed via secretsManagementFile.
secretsManagementFile: ${SW_ES_SECRETS_MANAGEMENT_FILE:""} # Secrets
management file in the properties format includes the username, password, which
are managed by 3rd party tool.
Review Comment:
The new keyStorePath and keyStorePass configuration options for client
certificate authentication are not documented. The existing elasticsearch.md
documentation covers trustStorePath but does not include the new mutual TLS
configuration options. Users need documentation explaining how to configure
client certificate authentication for OpenSearch/Elasticsearch.
```suggestion
trustStorePath: ${SW_STORAGE_ES_SSL_JKS_PATH:""} # Path to the
truststore that contains CA certificates used to verify the
OpenSearch/Elasticsearch server certificate.
trustStorePass: ${SW_STORAGE_ES_SSL_JKS_PASS:""} # Password for the
truststore defined in trustStorePath.
keyStorePath: ${SW_STORAGE_ES_SSL_KEY_STORE_PATH:""} # Path to the
client certificate keystore for mutual TLS (OpenSearch/Elasticsearch client
certificate authentication). Supports PKCS12 (.p12, .pfx) and JKS (.jks)
formats.
keyStorePass: ${SW_STORAGE_ES_SSL_KEY_STORE_PASS:""} # Password for the
client certificate keystore defined in keyStorePath. This can be externalized
via secretsManagementFile.
secretsManagementFile: ${SW_ES_SECRETS_MANAGEMENT_FILE:""} # Secrets
management file in properties format (e.g. username, password, keystore
passwords) managed by a 3rd party tool.
```
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/ElasticSearchBuilder.java:
##########
@@ -116,6 +126,18 @@ public ElasticSearchBuilder trustStorePass(String
trustStorePass) {
return this;
}
+ public ElasticSearchBuilder keyStorePath(String keyStorePath) {
+ requireNonNull(keyStorePath, "keyStorePath");
+ this.keyStorePath = keyStorePath;
+ return this;
+ }
+
+ public ElasticSearchBuilder keyStorePass(String keyStorePass) {
+ requireNonNull(keyStorePass, "keyStorePass");
+ this.keyStorePass = keyStorePass;
+ return this;
+ }
Review Comment:
The keyStorePass and trustStorePass builder methods use requireNonNull which
prevents passing null values. However, in ElasticSearchClient.connect(), the
code checks if these values are null before calling the builder methods. This
inconsistency could cause issues if someone tries to set an empty password. The
builder should allow null values and handle password-less keystores, or the
validation logic should be consistent throughout.
##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/ElasticSearchBuilder.java:
##########
@@ -157,19 +179,62 @@ public ElasticSearch build() {
.connectTimeout(connectTimeout)
.idleTimeout(socketTimeout)
.useHttp2Preface(false)
- .workerGroup(numHttpClientThread > 0 ?
numHttpClientThread : NUM_PROC);
-
- if (StringUtil.isNotBlank(trustStorePath)) {
- final TrustManagerFactory trustManagerFactory =
-
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
- final KeyStore truststore = KeyStore.getInstance("jks");
- try (final InputStream is =
Files.newInputStream(Paths.get(trustStorePath))) {
- truststore.load(is, trustStorePass.toCharArray());
- }
- trustManagerFactory.init(truststore);
-
- factoryBuilder.tlsCustomizer(
- sslContextBuilder ->
sslContextBuilder.trustManager(trustManagerFactory));
+ .workerGroup(new
NioEventLoopGroup(numHttpClientThread > 0 ? numHttpClientThread : NUM_PROC),
true);
Review Comment:
The workerGroup is now instantiated with a new NioEventLoopGroup and set to
shutdown on close (second parameter true). However, the previous implementation
used the integer value directly. This change means that every ElasticSearch
client instance will create its own event loop group, which could lead to
resource exhaustion if multiple clients are created. Consider whether this
EventLoopGroup should be shared across clients or if there's proper lifecycle
management to prevent resource leaks.
##########
test/e2e-v2/cases/storage/opensearch/docker-compose.yml:
##########
@@ -42,15 +107,24 @@ services:
SW_STORAGE: elasticsearch
SW_STORAGE_ES_CLUSTER_NODES: opensearch:9200
SW_ES_USER: admin
- SW_ES_PASSWORD: admin
+ SW_ES_PASSWORD: SecurePass@2024!
+ SW_STORAGE_ES_HTTP_PROTOCOL: https
+ SW_STORAGE_ES_SSL_JKS_PATH: /etc/skywalking/certs/truststore.jks
+ SW_STORAGE_ES_SSL_JKS_PASS: changeit
+ SW_STORAGE_ES_SSL_KEY_STORE_PATH: /etc/skywalking/certs/client.p12
+ SW_STORAGE_ES_SSL_KEY_STORE_PASS: changeit
+ JAVA_OPTS: -Dcom.linecorp.armeria.useEpoll=false # TODO: fix this
Review Comment:
This TODO comment indicates an incomplete fix for an Armeria epoll issue.
While TODOs in test configuration are acceptable if tracked, this should
reference a corresponding issue number or have a plan for resolution to ensure
it doesn't remain indefinitely.
```suggestion
JAVA_OPTS: -Dcom.linecorp.armeria.useEpoll=false # Epoll disabled in
e2e tests due to known instability on some hosts
```
--
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]
