You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by wa...@apache.org on 2026/08/26 03:57:25 UTC
(skywalking) branch banyandb-client-dynamic created (now 9db8e56c3a)
This is an automated email from the ASF dual-hosted git repository.
wankai123 pushed a change to branch banyandb-client-dynamic
in repository https://gitbox.apache.org/repos/asf/skywalking.git
at 9db8e56c3a Address review on BanyanDB credential and TLS trust CA hot-reload
This branch includes the following new commits:
new 9db8e56c3a Address review on BanyanDB credential and TLS trust CA hot-reload
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(skywalking) 01/01: Address review on BanyanDB credential and TLS trust CA hot-reload
Posted by wa...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
wankai123 pushed a commit to branch banyandb-client-dynamic
in repository https://gitbox.apache.org/repos/asf/skywalking.git
commit 9db8e56c3a5442a9d41569dbf1da04dd11a6bfe9
Author: wankai123 <wa...@foxmail.com>
AuthorDate: Wed Aug 26 11:57:14 2026 +0800
Address review on BanyanDB credential and TLS trust CA hot-reload
close() returned from inside the connection lock whenever the channel was
not a ManagedChannel, which is now always the case because the auth
interceptor wraps it. The lock was therefore never released and any later
connect() or close() from another thread blocked forever. It now runs
entirely under try/finally and shuts down the retained ChannelManager,
which owns the transport and the refresh scheduler.
ChannelManager.rebuild() is refused once shutdown has run, and the
shutdown paths are synchronized with it, so a CA rotation racing a
shutdown can no longer publish a live transport after the executor has
stopped.
The secrets management file is now read synchronously before the storage
client is created, and the watchers register only afterwards; the client
field is volatile. Previously a rotation landing inside that window was
applied to the config, observed a null client, and was never delivered
again, leaving the client on the credentials the constructor snapshotted.
The auth interceptor is seeded from Options in connect() rather than in
the constructor, restoring the behaviour that mutating the supplied
Options before connecting takes effect. updateCredentials() writes Options
too, so a rotation that arrives first is not overwritten by that seeding.
MultipleFilesChangeMonitor throttles on System.nanoTime() instead of the
wall clock. A backward clock step would otherwise make the elapsed delta
negative and suppress every secret, keystore and certificate watch until
real time caught up.
A secrets file carrying only one half of the pair is normalised to no
credentials, so the boot path and the reload path treat it the same way;
BanyanDBStorageClient refuses to be constructed from a half-populated
pair, so a file that is momentarily incomplete during a rotation could
otherwise stop the OAP from starting.
The storage documentation no longer claims a CA rotation is always
interruption-free: a gRPC channel connects lazily, so the replacement is
not health-checked before the swap, and material that does not validate
the server interrupts traffic until valid material is written back. The
old+new CA overlap procedure is documented instead.
---
docs/en/changes/changes.md | 2 +-
docs/en/setup/backend/storages/banyandb.md | 21 +++--
.../library/banyandb/v1/client/BanyanDBClient.java | 44 +++++++---
.../v1/client/grpc/channel/ChannelManager.java | 16 +++-
.../v1/client/BanyanDBClientCloseTest.java | 60 +++++++++++++
.../v1/client/grpc/channel/ChannelManagerTest.java | 33 ++++++++
.../library/util/MultipleFilesChangeMonitor.java | 16 ++--
.../plugin/banyandb/BanyanDBStorageProvider.java | 98 +++++++++++++++-------
.../plugin/banyandb/BanyanDBSecretsConfigTest.java | 43 ++++++++++
9 files changed, 276 insertions(+), 57 deletions(-)
diff --git a/docs/en/changes/changes.md b/docs/en/changes/changes.md
index 730ea5163e..afd15f9713 100644
--- a/docs/en/changes/changes.md
+++ b/docs/en/changes/changes.md
@@ -5,7 +5,7 @@
#### OAP Server
* Add customizable LLM-as-judge support for AI evaluation, with OpenAI-compatible endpoint / model / API key configuration, and persist the evaluation result as queryable `GenAIEvaluationRecord` rows for later inspection.
* Support hot-reloading the BanyanDB credentials from a new `secretsManagementFile` setting in `bydb.yml` (`SW_STORAGE_BANYANDB_SECRETS_MANAGEMENT_FILE`), matching the ElasticSearch storage plugin. The properties file carrying `user`/`password` is watched, so a rotation performed by a 3rd party tool such as Vault is applied without restarting the OAP. The credentials are read per RPC by the gRPC auth interceptor, so they are swapped without re-establishing the channel and without interru [...]
-* Support hot-reloading the BanyanDB TLS trust CA. The file at `sslTrustCAPath` is now watched, and a change rebuilds the gRPC channel so the new CA takes effect without restarting the OAP. Previously the CA was only re-read after the certificate in use had already caused requests to fail, which meant a rotation was paid for with an outage. The replacement channel is created before the old one is released, so a failed rebuild leaves the current channel serving; requests already in flight [...]
+* Support hot-reloading the BanyanDB TLS trust CA. The file at `sslTrustCAPath` is now watched, and a change rebuilds the gRPC channel so the new CA takes effect without restarting the OAP. Previously the CA was only re-read after the certificate in use had already caused requests to fail, which meant a rotation was paid for with an outage. The replacement channel is created before the old one is released, so a failed rebuild leaves the current channel serving; requests already in flight [...]
* Fix `MultipleFilesChangeMonitor` being able to silently disable every file watch in the OAP. Its registry of monitors was a plain `ArrayList` that `scanChanges()` iterated from the scheduler thread without holding the lock that `start()` / `stop()` take, so starting a monitor while a scan was in flight could raise a `ConcurrentModificationException` from the iterator. That exception escapes past the per-monitor catch, and an uncaught exception cancels a `scheduleAtFixedRate` task perma [...]
* Fix `MultipleFilesChangeMonitor` never honouring its watching period. `lastCheckTimestamp` was declared and compared against, but never assigned, so the guard always measured against `0` and passed — every registered monitor re-stat'd its watched files on each 200ms tick of the shared scheduler thread, and the `watchingPeriodInSec` constructor argument had no effect at all. This affects every file watch in the OAP: the ElasticSearch storage secrets / truststore / keystore watch, the Ba [...]
diff --git a/docs/en/setup/backend/storages/banyandb.md b/docs/en/setup/backend/storages/banyandb.md
index a916a968e9..5ddf418818 100644
--- a/docs/en/setup/backend/storages/banyandb.md
+++ b/docs/en/setup/backend/storages/banyandb.md
@@ -316,9 +316,12 @@ Whatever the file contains is applied, including a file that carries only one of
deliberate: a mistake in the file then shows up at the next request instead of being quietly refused, which would
leave the previous credentials working and the mistake unnoticed until the OAP is next restarted.
-A username and a password are only ever sent together, so an incomplete file means requests go out with no
-credentials at all and BanyanDB answers them with `UNAUTHENTICATED` — it never authenticates as the named user with
-the wrong password. The OAP logs an error naming the file when it applies an incomplete pair:
+A username and a password are only ever sent together, so an incomplete file is applied as no credentials at all:
+requests go out unauthenticated and BanyanDB answers them with `UNAUTHENTICATED`, and it never authenticates as the
+named user with the wrong password. Both halves are cleared together, so a file that is only briefly incomplete
+while a 3rd party tool rewrites it cannot stop the OAP from starting — it behaves the same whether it is read at
+boot or while the OAP is running. Note this also clears any `user`/`password` set in `bydb.yml`, since the file
+overrides them. The OAP logs an error naming the file when it applies an incomplete pair:
```text
Applied incomplete credentials from /etc/skywalking/bydb-secrets.properties: requests are now sent without
@@ -341,8 +344,16 @@ built rather than per request. Rebuilding the channel is therefore what a rotati
- The change is detected within 10 seconds, and the replacement channel is created before the old one is released,
so a failure to build it leaves the current channel serving with the previous CA. That case is logged as an error.
-- Requests already in flight finish on the old channel; new requests go to the replacement. Queries and writes are
- not interrupted.
+- Requests already in flight finish on the old channel; new requests go to the replacement. The replacement is
+ **not** verified before it is swapped in: a gRPC channel connects lazily, so the TLS handshake only happens on
+ the first request that uses it. A file that parses but does not validate the server therefore replaces a healthy
+ connection, and queries and writes start failing until material that does validate is written back. Rotating to
+ a CA the server's certificate chains to is interruption-free; rotating to the wrong one is not, and recovers
+ only on the next rotation.
+
+ To rotate with no interruption at all, write a PEM holding both the current and the new CA, confirm the OAP is
+ healthy, and only then let the server switch and drop the old CA at a later rotation. A certificate collection
+ in one PEM file is supported.
- The replacement picks a target from `targets` again, so with more than one BanyanDB address configured the OAP may
end up connected to a different node after a rotation. This is harmless, but worth knowing when correlating a
rotation with a change of peer in the logs.
diff --git a/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/BanyanDBClient.java b/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/BanyanDBClient.java
index 73b7a3354f..585450c2a8 100644
--- a/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/BanyanDBClient.java
+++ b/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/BanyanDBClient.java
@@ -160,7 +160,7 @@ public class BanyanDBClient implements Closeable {
* Created up-front rather than in {@link #connect()} so that {@link #updateCredentials} works
* before the first connection and survives every channel swap {@code ChannelManager} performs.
*/
- private final AuthInterceptor authInterceptor;
+ private final AuthInterceptor authInterceptor = new AuthInterceptor(null, null);
/**
* The manager behind {@link #channel}, kept so {@link #rebuildChannel()} can reload the TLS material.
* Null until {@link #connect()} runs, and for the test-only channel injection.
@@ -190,7 +190,6 @@ public class BanyanDBClient implements Closeable {
this.targets = tt;
this.options = options;
this.connectionEstablishLock = new ReentrantLock();
- this.authInterceptor = new AuthInterceptor(options.getUsername(), options.getPassword());
}
/**
@@ -209,6 +208,10 @@ public class BanyanDBClient implements Closeable {
ChannelManager rawChannel = ChannelManager.create(this.options.buildChannelManagerSettings(),
new DefaultChannelFactory(addresses, this.options));
this.channelManager = rawChannel;
+ // Seeded here rather than in the constructor, so that mutating the supplied Options
+ // between construction and connect() still takes effect, as it does for every other option.
+ // A rotation that arrived first is not lost: updateCredentials writes Options too.
+ authInterceptor.updateCredentials(options.getUsername(), options.getPassword());
// The auth interceptor is always installed: it sends no credentials until both a
// username and a password are set, so they can be rotated in without a reconnect.
Channel interceptedChannel = ClientInterceptors.intercept(rawChannel, authInterceptor);
@@ -236,6 +239,10 @@ public class BanyanDBClient implements Closeable {
* @param password the new password; when either half is blank no credentials are sent at all
*/
public void updateCredentials(String username, String password) {
+ // Options is written too, so a rotation that lands before connect() is not overwritten when
+ // connect() seeds the interceptor from it.
+ options.setUsername(username);
+ options.setPassword(password);
authInterceptor.updateCredentials(username, password);
}
@@ -1277,19 +1284,32 @@ public class BanyanDBClient implements Closeable {
@Override
public void close() throws IOException {
connectionEstablishLock.lock();
- if (!(this.channel instanceof ManagedChannel)) {
- return;
- }
- final ManagedChannel managedChannel = (ManagedChannel) this.channel;
try {
- if (isConnected) {
- managedChannel.shutdown().awaitTermination(5, TimeUnit.SECONDS);
+ if (!isConnected) {
+ return;
+ }
+ // The channel the stubs were built on is an interceptor wrapper rather than a ManagedChannel,
+ // so shutting it down has to go through the manager that owns the transport and the refresh
+ // scheduler. The raw channel is only used by the test-only connect(Channel) entry point.
+ final ManagedChannel managedChannel;
+ if (channelManager != null) {
+ managedChannel = channelManager;
+ } else if (this.channel instanceof ManagedChannel) {
+ managedChannel = (ManagedChannel) this.channel;
+ } else {
+ managedChannel = null;
+ }
+ if (managedChannel == null) {
isConnected = false;
+ return;
+ }
+ try {
+ managedChannel.shutdown().awaitTermination(5, TimeUnit.SECONDS);
+ } catch (InterruptedException interruptedException) {
+ Thread.currentThread().interrupt();
+ log.warn("fail to wait for channel termination, shutdown now!", interruptedException);
+ managedChannel.shutdownNow();
}
- } catch (InterruptedException interruptedException) {
- Thread.currentThread().interrupt();
- log.warn("fail to wait for channel termination, shutdown now!", interruptedException);
- managedChannel.shutdownNow();
isConnected = false;
} finally {
connectionEstablishLock.unlock();
diff --git a/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManager.java b/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManager.java
index e00e47070d..4d9442b7c1 100644
--- a/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManager.java
+++ b/oap-server/server-library/library-banyandb-client/src/main/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManager.java
@@ -54,6 +54,11 @@ public class ChannelManager extends ManagedChannel {
private final ScheduledExecutorService executor;
@VisibleForTesting
final AtomicReference<Entry> entryRef = new AtomicReference<>();
+ /**
+ * Set once {@link #shutdown()} or {@link #shutdownNow()} has run, so that a rebuild racing a shutdown
+ * cannot publish a live transport after the scheduler has been stopped.
+ */
+ private volatile boolean terminated = false;
public static ChannelManager create(ChannelManagerSettings settings, ChannelFactory channelFactory)
throws IOException {
@@ -114,12 +119,18 @@ public class ChannelManager extends ManagedChannel {
* @throws IOException if the replacement channel cannot be created.
*/
public synchronized void rebuild() throws IOException {
+ if (terminated) {
+ // The executor is already stopped; publishing a fresh transport here would leak it and break
+ // ManagedChannel's terminal-state contract.
+ return;
+ }
final Entry replacedEntry = entryRef.getAndSet(new Entry(this.channelFactory.create()));
replacedEntry.shutdown();
}
@Override
- public ManagedChannel shutdown() {
+ public synchronized ManagedChannel shutdown() {
+ terminated = true;
entryRef.get().channel.shutdown();
if (executor != null) {
// shutdownNow will cancel scheduled tasks
@@ -145,7 +156,8 @@ public class ChannelManager extends ManagedChannel {
}
@Override
- public ManagedChannel shutdownNow() {
+ public synchronized ManagedChannel shutdownNow() {
+ terminated = true;
entryRef.get().channel.shutdownNow();
if (executor != null) {
executor.shutdownNow();
diff --git a/oap-server/server-library/library-banyandb-client/src/test/java/org/apache/skywalking/library/banyandb/v1/client/BanyanDBClientCloseTest.java b/oap-server/server-library/library-banyandb-client/src/test/java/org/apache/skywalking/library/banyandb/v1/client/BanyanDBClientCloseTest.java
new file mode 100644
index 0000000000..912ba8f758
--- /dev/null
+++ b/oap-server/server-library/library-banyandb-client/src/test/java/org/apache/skywalking/library/banyandb/v1/client/BanyanDBClientCloseTest.java
@@ -0,0 +1,60 @@
+/*
+ * 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.skywalking.library.banyandb.v1.client;
+
+import io.grpc.Channel;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.mockito.Mockito.mock;
+
+public class BanyanDBClientCloseTest {
+
+ /**
+ * The channel the stubs run on is an interceptor wrapper rather than a ManagedChannel. close() used to
+ * return early on that, from inside the connection lock and before the matching unlock, so the lock was
+ * left held and every later connect() or close() blocked forever.
+ *
+ * <p>The second call has to come from another thread: the lock is reentrant, so the thread that leaked it
+ * re-acquires it without noticing and would make this test pass against the broken code.
+ */
+ @Test
+ public void closeShouldReleaseTheConnectionLock() throws Exception {
+ final BanyanDBClient client = new BanyanDBClient(new String[] {"127.0.0.1:17912"}, new Options());
+ client.connect(mock(Channel.class));
+
+ client.close();
+
+ final ExecutorService other = Executors.newSingleThreadExecutor();
+ try {
+ final Future<?> closedAgain = other.submit(() -> {
+ client.close();
+ return null;
+ });
+ // TimeoutException if the first close() walked out still holding the lock.
+ assertDoesNotThrow(() -> closedAgain.get(10, TimeUnit.SECONDS));
+ } finally {
+ other.shutdownNow();
+ }
+ }
+}
diff --git a/oap-server/server-library/library-banyandb-client/src/test/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManagerTest.java b/oap-server/server-library/library-banyandb-client/src/test/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManagerTest.java
index d37864c3ed..b1cedfc4e4 100644
--- a/oap-server/server-library/library-banyandb-client/src/test/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManagerTest.java
+++ b/oap-server/server-library/library-banyandb-client/src/test/java/org/apache/skywalking/library/banyandb/v1/client/grpc/channel/ChannelManagerTest.java
@@ -152,6 +152,39 @@ public class ChannelManagerTest {
Mockito.verify(underlyingChannel1).shutdown();
}
+ /**
+ * A CA rotation and a shutdown can race. Rebuilding after shutdown would publish a live transport while
+ * the refresh executor is already stopped, leaking it and breaking ManagedChannel's terminal-state
+ * contract, so a rebuild past that point has to be ignored.
+ */
+ @Test
+ public void rebuildShouldBeIgnoredAfterShutdown() throws IOException {
+ ManagedChannel underlyingChannel1 = mock(ManagedChannel.class);
+ ManagedChannel underlyingChannel2 = mock(ManagedChannel.class);
+
+ ScheduledExecutorService scheduledExecutorService = mock(ScheduledExecutorService.class);
+ Mockito.doReturn(null)
+ .when(scheduledExecutorService)
+ .schedule(
+ Mockito.any(Runnable.class), Mockito.anyLong(), Mockito.eq(TimeUnit.MILLISECONDS));
+
+ ChannelManager manager =
+ new ChannelManager(
+ ChannelManagerSettings.builder()
+ .refreshInterval(30)
+ .forceReconnectionThreshold(1).build(),
+ new FakeChannelFactory(ImmutableList.of(underlyingChannel1, underlyingChannel2)),
+ scheduledExecutorService);
+
+ manager.shutdown();
+ manager.rebuild();
+
+ // The second channel must never have been created, and calls still resolve to the first entry.
+ manager.newCall(FakeMethodDescriptor.<String, Integer>create(), CallOptions.DEFAULT);
+ Mockito.verify(underlyingChannel2, Mockito.never())
+ .newCall(Mockito.<MethodDescriptor<String, Integer>>any(), Mockito.any(CallOptions.class));
+ }
+
@Test
public void networkErrorStatusShouldTriggerReconnect() throws IOException {
final IndexRuleRegistryServiceGrpc.IndexRuleRegistryServiceImplBase indexRuleServiceImpl =
diff --git a/oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java b/oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java
index 2b5328b775..56d53e4104 100644
--- a/oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java
+++ b/oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java
@@ -57,12 +57,16 @@ public class MultipleFilesChangeMonitor {
* {@code scheduleAtFixedRate} task for good, silently ending all file monitoring in the JVM.
*/
private static final List<MultipleFilesChangeMonitor> MONITOR_INSTANCES = new CopyOnWriteArrayList<>();
+ private static final long NEVER_CHECKED = Long.MIN_VALUE;
/**
- * The timestamp when last time do status checked. Volatile because {@link #start()} runs the first check on the
- * caller's thread, while every later check runs on the shared scheduler thread.
+ * When the last status check ran, read from {@link System#nanoTime()} rather than the wall clock: this is an
+ * elapsed-time throttle, and a backward wall-clock step (NTP correction, VM restore) would otherwise make the
+ * delta negative and suppress every watch until real time caught up. {@link #NEVER_CHECKED} marks the first
+ * run, because a nanoTime origin is arbitrary and may be negative. Volatile because {@link #start()} runs the
+ * first check on the caller's thread, while every later check runs on the shared scheduler thread.
*/
- private volatile long lastCheckTimestamp = 0;
+ private volatile long lastCheckNanos = NEVER_CHECKED;
/**
* The period of watching thread checking the file status. Unit is the second.
*/
@@ -99,14 +103,14 @@ public class MultipleFilesChangeMonitor {
* Check file changed status, if so, send the notification.
*/
private void checkAndNotify() {
- final long now = System.currentTimeMillis();
- if (now - lastCheckTimestamp < watchingPeriodInSec * 1000) {
+ final long now = System.nanoTime();
+ if (lastCheckNanos != NEVER_CHECKED && now - lastCheckNanos < TimeUnit.SECONDS.toNanos(watchingPeriodInSec)) {
// Don't reach the period threshold, ignore this check.
return;
}
// Stamped before the files are read, so the period measures the gap between checks starting; a slow
// notification callback then delays the work it does itself, not every following check.
- lastCheckTimestamp = now;
+ lastCheckNanos = now;
boolean isChanged = false;
for (final WatchedFile watchedFile : watchedFiles) {
diff --git a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBStorageProvider.java b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBStorageProvider.java
index 47c45f532e..b123583d8e 100644
--- a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBStorageProvider.java
+++ b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBStorageProvider.java
@@ -19,7 +19,9 @@
package org.apache.skywalking.oap.server.storage.plugin.banyandb;
import java.io.ByteArrayInputStream;
+import java.io.File;
import java.io.IOException;
+import java.nio.file.Files;
import java.util.HashSet;
import java.util.List;
import java.util.Properties;
@@ -116,7 +118,7 @@ import org.apache.skywalking.oap.server.telemetry.api.MetricsTag;
@Slf4j
public class BanyanDBStorageProvider extends ModuleProvider {
private BanyanDBStorageConfig config;
- private BanyanDBStorageClient client;
+ private volatile BanyanDBStorageClient client;
private ModelInstaller modelInstaller;
@Override
@@ -161,10 +163,14 @@ public class BanyanDBStorageProvider extends ModuleProvider {
}
this.registerServiceImplementation(StorageBuilderFactory.class, new StorageBuilderFactory.Default());
- loadAndWatchSecretsManagementFile();
- rebuildChannelOnTrustCAChange();
+ loadSecretsManagementFile();
this.client = new BanyanDBStorageClient(getManager(), config);
+ // Registered only now: a callback that ran while `client` was still null would apply the file to the
+ // config and return, and the monitor would have cached that content and never deliver it again --
+ // leaving the client on the credentials the constructor above snapshotted.
+ watchSecretsManagementFile();
+ rebuildChannelOnTrustCAChange();
this.modelInstaller = new BanyanDBIndexInstaller(client, getManager(), this.config);
// Expose the installer so the runtime-rule reconciler can call isExists() after a
// hot-apply to verify that DDL landed as expected. Needed
@@ -250,44 +256,74 @@ public class BanyanDBStorageProvider extends ModuleProvider {
*
* <p>Whatever the file contains is applied, including a half-populated or empty one. That is deliberate:
* a bad file then fails visibly at the next request rather than being quietly refused, which would leave
- * the working credentials in place and the mistake undiscovered until the next restart.
+ * the working credentials in place and the mistake undiscovered until the next restart. A pair that is
+ * only half populated is applied as no credentials at all, so that the boot path and the reload path
+ * treat the same file the same way.
*
* <p>Called before {@link #client} is created so that the monitor's initial synchronous check has
* already populated the config by the time the client reads it.
*/
- private void loadAndWatchSecretsManagementFile() {
+ private void loadSecretsManagementFile() throws ModuleStartException {
final String secretsFile = config.getGlobal().getSecretsManagementFile();
if (StringUtil.isBlank(secretsFile)) {
return;
}
- new MultipleFilesChangeMonitor(10, readableContents -> {
- final byte[] secretsFileContent = readableContents.get(0);
- if (secretsFileContent == null) {
- return;
- }
- final Properties secrets = new Properties();
- secrets.load(new ByteArrayInputStream(secretsFileContent));
- final String user = secrets.getProperty("user", null);
- final String password = secrets.getProperty("password", null);
- config.getGlobal().setUser(user);
- config.getGlobal().setPassword(password);
+ final File file = new File(secretsFile);
+ if (!file.exists() || !file.isFile()) {
+ // Absent at boot is legitimate: the credentials then come from bydb.yml, and the watcher picks
+ // the file up if a 3rd party tool creates it later.
+ return;
+ }
+ try {
+ applyCredentials(Files.readAllBytes(file.toPath()), secretsFile);
+ } catch (IOException e) {
+ throw new ModuleStartException("Failed to read the secrets management file " + secretsFile, e);
+ }
+ }
- if (client != null) {
- // AuthInterceptor reads the credentials per RPC, so this applies to the next call without
- // reconnecting. Before the client exists the config above is all that is needed.
- client.client.updateCredentials(user, password);
- if (StringUtil.isNotBlank(user) && StringUtil.isNotBlank(password)) {
- log.info("Applied the BanyanDB credentials reloaded from {}, user={}", secretsFile, user);
- } else {
- // Whatever the file says is applied, so that a mistake in it surfaces instead of being
- // masked by credentials that silently keep working.
- log.error(
- "Applied incomplete credentials from {}: requests are now sent without authentication, "
- + "because a username and a password are only ever attached together. BanyanDB "
- + "answers them with UNAUTHENTICATED if it requires authentication.", secretsFile);
- }
+ private void watchSecretsManagementFile() {
+ final String secretsFile = config.getGlobal().getSecretsManagementFile();
+ if (StringUtil.isBlank(secretsFile)) {
+ return;
+ }
+ new MultipleFilesChangeMonitor(
+ 10, readableContents -> applyCredentials(readableContents.get(0), secretsFile), secretsFile).start();
+ }
+
+ private void applyCredentials(final byte[] secretsFileContent, final String secretsFile) throws IOException {
+ if (secretsFileContent == null) {
+ return;
+ }
+ final Properties secrets = new Properties();
+ secrets.load(new ByteArrayInputStream(secretsFileContent));
+ String user = secrets.getProperty("user", null);
+ String password = secrets.getProperty("password", null);
+ final boolean complete = StringUtil.isNotBlank(user) && StringUtil.isNotBlank(password);
+ if (!complete) {
+ // Normalised to no credentials rather than left half-populated. This callback also runs
+ // before the client exists, and BanyanDBStorageClient refuses to be constructed from a
+ // half-populated pair, so leaving one half set would let a file that is only momentarily
+ // incomplete during a rotation stop the OAP from booting -- while the very same file, written
+ // once it is running, merely degrades to unauthenticated requests.
+ user = null;
+ password = null;
+ log.error(
+ "Applied incomplete credentials from {}: requests are sent without authentication, because "
+ + "a username and a password are only ever attached together. BanyanDB answers them "
+ + "with UNAUTHENTICATED if it requires authentication.", secretsFile);
+ }
+ config.getGlobal().setUser(user);
+ config.getGlobal().setPassword(password);
+
+ final BanyanDBStorageClient current = client;
+ if (current != null) {
+ // AuthInterceptor reads the credentials per RPC, so this applies to the next call without
+ // reconnecting. Before the client exists the config above is all that is needed.
+ current.client.updateCredentials(user, password);
+ if (complete) {
+ log.info("Applied the BanyanDB credentials reloaded from {}, user={}", secretsFile, user);
}
- }, secretsFile).start();
+ }
}
/**
diff --git a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBSecretsConfigTest.java b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBSecretsConfigTest.java
index e9e47e173d..826135e0a5 100644
--- a/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBSecretsConfigTest.java
+++ b/oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBSecretsConfigTest.java
@@ -19,11 +19,14 @@
package org.apache.skywalking.oap.server.storage.plugin.banyandb;
import org.apache.skywalking.oap.server.library.module.ModuleDefine;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
import org.apache.skywalking.oap.server.library.module.ModuleProvider;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -49,6 +52,46 @@ public class BanyanDBSecretsConfigTest {
newLoader().loadConfig().getGlobal().getSecretsManagementFile());
}
+ /**
+ * The secrets watcher runs before the client is created and applies a half-populated file as no
+ * credentials at all. That is only safe while the client tolerates neither half being set, so the two
+ * halves of that contract are pinned here: a file that is momentarily incomplete during a rotation must
+ * not be able to stop the OAP from booting, but a genuinely half-configured bydb.yml still must.
+ */
+ @Test
+ public void shouldAcceptAConfigWithNeitherCredentialHalfSet() {
+ final BanyanDBStorageConfig config = newConnectableConfig();
+ config.getGlobal().setUser(null);
+ config.getGlobal().setPassword(null);
+
+ assertDoesNotThrow(() -> new BanyanDBStorageClient(mock(ModuleManager.class), config));
+ }
+
+ @Test
+ public void shouldRejectAConfigWithOnlyOneCredentialHalfSet() {
+ final BanyanDBStorageConfig withUserOnly = newConnectableConfig();
+ withUserOnly.getGlobal().setUser("admin");
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new BanyanDBStorageClient(mock(ModuleManager.class), withUserOnly));
+
+ final BanyanDBStorageConfig withPasswordOnly = newConnectableConfig();
+ withPasswordOnly.getGlobal().setPassword("admin");
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new BanyanDBStorageClient(mock(ModuleManager.class), withPasswordOnly));
+ }
+
+ /**
+ * compatibleServerApiVersions has no default and its getter splits the raw string, so a config built in
+ * code rather than loaded from bydb.yml has to set it before the client can be constructed.
+ */
+ private BanyanDBStorageConfig newConnectableConfig() {
+ final BanyanDBStorageConfig config = new BanyanDBStorageConfig();
+ config.getGlobal().setCompatibleServerApiVersions("0.10");
+ return config;
+ }
+
private BanyanDBConfigLoader newLoader() {
ModuleDefine moduleDefine = mock(ModuleDefine.class);
ModuleProvider provider = mock(ModuleProvider.class);