gnodet-bot commented on code in PR #29235:
URL: https://github.com/apache/camel/pull/29235#discussion_r4480003219
##########
components/camel-hashicorp-vault/src/main/java/org/apache/camel/component/hashicorp/vault/HashicorpVaultPropertiesFunction.java:
##########
@@ -78,19 +89,11 @@ public class HashicorpVaultPropertiesFunction extends
ServiceSupport implements
private static final Logger LOG =
LoggerFactory.getLogger(HashicorpVaultPropertiesFunction.class);
- private static final String CAMEL_HASHICORP_VAULT_TOKEN_ENV =
"CAMEL_HASHICORP_VAULT_TOKEN";
- private static final String CAMEL_HASHICORP_VAULT_HOST_ENV =
"CAMEL_HASHICORP_VAULT_HOST";
- private static final String CAMEL_HASHICORP_VAULT_PORT_ENV
- = "CAMEL_HASHICORP_VAULT_PORT";
- private static final String CAMEL_HASHICORP_VAULT_SCHEME_ENV
- = "CAMEL_HASHICORP_VAULT_SCHEME";
- private static final String CAMEL_HASHICORP_VAULT_CLOUD_ENV
- = "CAMEL_HASHICORP_VAULT_CLOUD";
- private static final String CAMEL_HASHICORP_VAULT_NAMESPACE_ENV
- = "CAMEL_HASHICORP_VAULT_NAMESPACE";
private static final String DEFAULT_ENGINE = "secret";
private CamelContext camelContext;
private VaultTemplate client;
+ // the client the function created, which it closes on stop (a client
given to the function is not closed)
Review Comment:
⚠️ **Missing `volatile`:** `HashicorpVaultPropertiesFunction.client` is not
declared `volatile`, but `doStop()` (added in this PR) nulls it from the
lifecycle thread. Without `volatile`, that null assignment may not be visible
to threads concurrently reading `client`. The companion `ReloadTriggerTask`
correctly upgrades to `private volatile VaultTemplate client` — the same fix is
needed here.
##########
components/camel-hashicorp-vault/src/main/java/org/apache/camel/component/hashicorp/vault/HashicorpVaultClient.java:
##########
@@ -0,0 +1,245 @@
+/*
+ * 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.camel.component.hashicorp.vault;
+
+import java.io.Closeable;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Supplier;
+
+import org.apache.camel.CamelContext;
+import org.apache.camel.spi.ExecutorServiceManager;
+import org.apache.camel.util.ObjectHelper;
+import org.springframework.http.client.SimpleClientHttpRequestFactory;
+import org.springframework.scheduling.concurrent.ConcurrentTaskScheduler;
+import org.springframework.vault.authentication.AppRoleAuthentication;
+import org.springframework.vault.authentication.AppRoleAuthenticationOptions;
+import
org.springframework.vault.authentication.AppRoleAuthenticationOptions.RoleId;
+import
org.springframework.vault.authentication.AppRoleAuthenticationOptions.SecretId;
+import org.springframework.vault.authentication.ClientAuthentication;
+import org.springframework.vault.authentication.JwtAuthentication;
+import org.springframework.vault.authentication.JwtAuthenticationOptions;
+import org.springframework.vault.authentication.KubernetesAuthentication;
+import
org.springframework.vault.authentication.KubernetesAuthenticationOptions;
+import
org.springframework.vault.authentication.KubernetesServiceAccountTokenFile;
+import org.springframework.vault.authentication.LifecycleAwareSessionManager;
+import org.springframework.vault.authentication.ResourceCredentialSupplier;
+import org.springframework.vault.authentication.TokenAuthentication;
+import org.springframework.vault.client.VaultClient;
+import org.springframework.vault.client.VaultEndpoint;
+import org.springframework.vault.core.VaultTemplate;
+import org.springframework.vault.support.ClientOptions;
+
+/**
+ * A client of HashiCorp Vault that logs in as its {@link
HashicorpVaultClientSettings} say, and keeps its token valid:
+ * it renews the token before the token expires, and logs in again when the
token can no longer be renewed. Closing the
+ * client stops the renewal and revokes the token it logged in with. A token
given for the token login is renewed too,
+ * but never revoked.
+ * <p/>
+ * The endpoint, the properties function and the secret refresh task each
create their client with
+ * {@link #create(CamelContext, Object, HashicorpVaultClientSettings)}, and
close it when they stop.
+ */
+public final class HashicorpVaultClient implements Closeable {
+
+ private final VaultTemplate vaultTemplate;
+ private final LifecycleAwareSessionManager sessionManager;
+ private final ExecutorServiceManager executorServiceManager;
+ private final ScheduledExecutorService renewal;
+
+ private HashicorpVaultClient(VaultTemplate vaultTemplate,
LifecycleAwareSessionManager sessionManager,
+ ExecutorServiceManager
executorServiceManager, ScheduledExecutorService renewal) {
+ this.vaultTemplate = vaultTemplate;
+ this.sessionManager = sessionManager;
+ this.executorServiceManager = executorServiceManager;
+ this.renewal = renewal;
+ }
+
+ /**
+ * Creates a client. It logs in at its first request to Vault.
+ *
+ * @param camelContext the context, whose executor service
manager runs the renewal of the token
+ * @param source the owner of the client, which names
the thread that renews the token
+ * @param settings where Vault is and how to log in to it
+ * @return the client, to close when it is no
longer used
+ * @throws IllegalArgumentException when the settings are incomplete or
not valid
+ */
+ public static HashicorpVaultClient create(CamelContext camelContext,
Object source, HashicorpVaultClientSettings settings) {
+ validate(settings);
+ VaultEndpoint endpoint = new VaultEndpoint();
+ endpoint.setHost(settings.getHost());
+ endpoint.setPort(Integer.parseInt(settings.getPort()));
+ endpoint.setScheme(settings.getScheme());
+
+ VaultClient client = vaultClient(endpoint, null);
+ // on HashiCorp Cloud the login methods are in the namespace, while
Camel reads the secrets with the namespace in
+ // their path: so only the login, and the renewal and revocation of
its token, are sent to the namespace
+ String namespace = settings.getCloudNamespace();
+ VaultClient loginClient = namespace != null ? vaultClient(endpoint,
namespace) : client;
+ ClientAuthentication authentication = createAuthentication(settings,
loginClient);
+
+ ExecutorServiceManager executorServiceManager =
camelContext.getExecutorServiceManager();
+ ScheduledExecutorService renewal
+ =
executorServiceManager.newSingleThreadScheduledExecutor(source,
"HashicorpVaultTokenRenewal");
Review Comment:
⚠️ **Executor leak on constructor failure:** `renewal` (the
`ScheduledExecutorService`) is created at this line, but
`LifecycleAwareSessionManager` and `VaultTemplate` are constructed afterwards
without a `try/catch`. If either Spring constructor throws, `renewal` is leaked
— its thread pool keeps running. Consider:
```java
try {
LifecycleAwareSessionManager sessionManager = ...
return new HashicorpVaultClient(...);
} catch (Exception e) {
executorServiceManager.shutdownGraceful(renewal);
throw 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]
