fix(secrets): Akka replication was inert in 0.2.0 - registration order bug (0.2.1)
AddZbSecretsAkkaReplication called AddZbSecrets FIRST, which does TryAddSingleton<ISecretReplicator, NoOpSecretReplicator>(). The package's own TryAddSingleton<ISecretReplicator> therefore found a descriptor already present and was silently discarded. Consequence: ISecretReplicator resolved to the no-op sink, so every write published into nothing; and because SecretReplicationActor is only spawned as a side effect of constructing AkkaSecretReplicator, no actor was ever created either. No exception, no log line - a cluster that reports healthy and silently never converges. The worst available failure mode for a secrets store. Fix: register ISecretReplicator BEFORE AddZbSecrets, matching what the SQL-Server package already did. Found during OtOpcUa adoption (Task 6), which is the first code that ever built a container around this extension. Root cause of the gap: the SQL-Server package had a DI test asserting its replicator type (AddZbSecretsSqlServerTests:58) and the correct order; the Akka package had neither. Every Akka test exercised the actor, serializer and reconciler in isolation - none built a container, so nothing could see it. This is the third instance of the same defect class in this library (the inert ISecretReplicator seam, the unregistered concrete SqliteSecretStore, and now this), all of which share one cause: unit tests that never construct the DI graph. Adds AddZbSecretsAkkaReplicationTests (5 tests) asserting registration at the ServiceCollection level. Verified to discriminate: with the 0.2.0 order restored, 2 of the 5 fail; with the fix, all 5 pass. They assert descriptors rather than resolving from a provider on purpose - resolving ISecretReplicator eagerly spawns the actor, whose PreStart needs DistributedPubSub and therefore a joined cluster, which would make the test hang rather than fail. Full suite Release-green: 175 passed, 15 skipped, no new warnings.
This commit is contained in:
@@ -5,7 +5,7 @@
|
||||
<Nullable>enable</Nullable>
|
||||
<ImplicitUsings>enable</ImplicitUsings>
|
||||
<LangVersion>latest</LangVersion>
|
||||
<Version>0.2.0</Version>
|
||||
<Version>0.2.1</Version>
|
||||
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
|
||||
<PackageReadmeFile>README.md</PackageReadmeFile>
|
||||
</PropertyGroup>
|
||||
|
||||
+8
-3
@@ -66,9 +66,6 @@ public static class AkkaSecretsServiceCollectionExtensions
|
||||
section.Bind(eager);
|
||||
eager.Validate();
|
||||
|
||||
// The local store + migrator come from AddZbSecrets (SQLite by default).
|
||||
services.AddZbSecrets(config, secretsSectionPath);
|
||||
|
||||
// The actor is created lazily on first use rather than at registration: the ActorSystem is
|
||||
// typically registered by the application AFTER this call, and a cluster node may also not be
|
||||
// ready to join at the moment the container is built.
|
||||
@@ -78,9 +75,17 @@ public static class AkkaSecretsServiceCollectionExtensions
|
||||
sp.GetService<ISecretCacheInvalidator>(),
|
||||
sp.GetRequiredService<IOptions<AkkaSecretsReplicationOptions>>().Value));
|
||||
|
||||
// MUST be registered BEFORE AddZbSecrets. That call does
|
||||
// TryAddSingleton<ISecretReplicator, NoOpSecretReplicator>(), so if it ran first it would
|
||||
// win and the TryAdd below would be silently discarded — leaving replication bound to the
|
||||
// no-op sink, publishing nothing, with no actor ever created and no error anywhere.
|
||||
// Shipped that way in 0.2.0; fixed in 0.2.1. The SqlServer package always had this order.
|
||||
services.TryAddSingleton<ISecretReplicator>(sp =>
|
||||
new AkkaSecretReplicator(sp.GetRequiredService<SecretReplicationActorProvider>().ActorRef));
|
||||
|
||||
// The local store + migrator come from AddZbSecrets (SQLite by default).
|
||||
services.AddZbSecrets(config, secretsSectionPath);
|
||||
|
||||
// Decorate the local store so every write publishes. Resolved from the concrete SQLite store,
|
||||
// not ISecretStore — asking the container for ISecretStore here would resolve this very
|
||||
// decorator and recurse forever.
|
||||
|
||||
+109
@@ -0,0 +1,109 @@
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using ZB.MOM.WW.Secrets.Abstractions;
|
||||
using ZB.MOM.WW.Secrets.DependencyInjection;
|
||||
using ZB.MOM.WW.Secrets.Replication;
|
||||
using ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.DependencyInjection;
|
||||
using ZB.MOM.WW.Secrets.Sqlite;
|
||||
|
||||
namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.DependencyInjection;
|
||||
|
||||
/// <summary>
|
||||
/// Registration tests for <c>AddZbSecretsAkkaReplication</c>.
|
||||
/// <para>
|
||||
/// These exist because 0.2.0 shipped with Akka replication silently inert and every other test in
|
||||
/// this assembly still passed. The bug was pure registration ORDER: the extension called
|
||||
/// <c>AddZbSecrets</c> first, which does
|
||||
/// <c>TryAddSingleton<ISecretReplicator, NoOpSecretReplicator>()</c>, so the package's own
|
||||
/// <c>TryAddSingleton<ISecretReplicator></c> found a descriptor already present and was
|
||||
/// silently discarded. Writes then published into a no-op sink and no replication actor was ever
|
||||
/// spawned — with no exception and no log line anywhere.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The equivalent SQL-Server package always had a test asserting its replicator type, and always
|
||||
/// had the correct order. That asymmetry is exactly why the defect landed in one package and not
|
||||
/// the other, so the gap is closed here.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// These assert against the <see cref="IServiceCollection"/> rather than a built provider on
|
||||
/// purpose. Resolving <c>ISecretReplicator</c> eagerly spawns the replication actor, whose
|
||||
/// <c>PreStart</c> calls <c>DistributedPubSub.Get(...)</c>; that requires a joined cluster, so a
|
||||
/// provider-based test would need a real single-node cluster and would hang without one. The
|
||||
/// defect was entirely about which descriptor wins, and the descriptor is where it is visible.
|
||||
/// End-to-end actor behaviour is already covered by <c>TwoNodeClusterReplicationTests</c>.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public sealed class AddZbSecretsAkkaReplicationTests
|
||||
{
|
||||
private static IServiceCollection Register()
|
||||
{
|
||||
IConfigurationRoot config = new ConfigurationBuilder()
|
||||
.AddInMemoryCollection(new Dictionary<string, string?>
|
||||
{
|
||||
["Secrets:SqlitePath"] = Path.Combine(Path.GetTempPath(), $"zb-akkadi-{Guid.NewGuid():N}.db"),
|
||||
["Secrets:Replication:AnnounceInterval"] = "00:00:30",
|
||||
})
|
||||
.Build();
|
||||
|
||||
var services = new ServiceCollection();
|
||||
services.AddLogging();
|
||||
services.AddZbSecretsAkkaReplication(config, "Secrets", "Secrets:Replication");
|
||||
return services;
|
||||
}
|
||||
|
||||
private static ServiceDescriptor Descriptor<T>(IServiceCollection services) =>
|
||||
services.Single(d => d.ServiceType == typeof(T));
|
||||
|
||||
[Fact]
|
||||
public void ISecretReplicator_is_not_bound_to_the_no_op_sink()
|
||||
{
|
||||
// THE REGRESSION TEST. Before the fix this descriptor was NoOpSecretReplicator, so every
|
||||
// replicated write went nowhere. A no-op secret replicator is the worst failure mode
|
||||
// available: the cluster reports healthy and silently never converges.
|
||||
ServiceDescriptor descriptor = Descriptor<ISecretReplicator>(Register());
|
||||
|
||||
Assert.NotEqual(typeof(NoOpSecretReplicator), descriptor.ImplementationType);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ISecretReplicator_is_registered_by_this_package_via_a_factory()
|
||||
{
|
||||
// The package registers through a factory (it needs the lazily-created actor ref), whereas
|
||||
// the core no-op is registered by concrete type. A factory registration is therefore proof
|
||||
// that this package's descriptor won the TryAdd race rather than core's.
|
||||
ServiceDescriptor descriptor = Descriptor<ISecretReplicator>(Register());
|
||||
|
||||
Assert.NotNull(descriptor.ImplementationFactory);
|
||||
Assert.Null(descriptor.ImplementationType);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ISecretStore_is_decorated_so_writes_actually_publish()
|
||||
{
|
||||
// AddSingleton (not TryAdd) means the decorator is appended, and the LAST registration for
|
||||
// a service type is what the container resolves. If this regressed to the bare SQLite
|
||||
// store, writes would never publish.
|
||||
ServiceDescriptor last = Register()
|
||||
.Last(d => d.ServiceType == typeof(ISecretStore));
|
||||
|
||||
Assert.NotNull(last.ImplementationFactory);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void The_undecorated_concrete_store_is_registered()
|
||||
{
|
||||
// ReplicatingSecretStore resolves SqliteSecretStore by concrete type. Core registering it
|
||||
// only behind ISecretStore was the OTHER defect this library shipped, so pin it here too.
|
||||
IServiceCollection services = Register();
|
||||
|
||||
Assert.Contains(services, d => d.ServiceType == typeof(SqliteSecretStore));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void The_store_migrator_seam_is_registered()
|
||||
{
|
||||
IServiceCollection services = Register();
|
||||
|
||||
Assert.Contains(services, d => d.ServiceType == typeof(ISecretsStoreMigrator));
|
||||
}
|
||||
}
|
||||
+1
@@ -12,6 +12,7 @@
|
||||
<PackageReference Include="Akka.TestKit.Xunit2" />
|
||||
<PackageReference Include="Akka.Remote" />
|
||||
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
|
||||
<PackageReference Include="Microsoft.Extensions.Logging" />
|
||||
</ItemGroup>
|
||||
|
||||
<ItemGroup>
|
||||
|
||||
Reference in New Issue
Block a user