From 92ff6a0d8cdcad932a5be13732e551b758890902 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 16:38:38 -0400 Subject: [PATCH] =?UTF-8?q?feat(ldap):=20failover=20observability=20?= =?UTF-8?q?=E2=80=94=20warn=20per=20hop,=20info=20on=20sticky=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ZB.MOM.WW.Auth/Directory.Packages.props | 1 + .../ServiceCollectionExtensions.cs | 13 ++-- .../ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs | 31 ++++++-- .../ZB.MOM.WW.Auth.Ldap.csproj | 1 + .../LdapAuthServiceFailoverTests.cs | 60 ++++++++++++++++ ...8-10-ldap-backup-dc-failover.md.tasks.json | 70 ++++++++++++++++--- 6 files changed, 158 insertions(+), 18 deletions(-) diff --git a/ZB.MOM.WW.Auth/Directory.Packages.props b/ZB.MOM.WW.Auth/Directory.Packages.props index a61d3d3..1e7fff5 100644 --- a/ZB.MOM.WW.Auth/Directory.Packages.props +++ b/ZB.MOM.WW.Auth/Directory.Packages.props @@ -24,6 +24,7 @@ + diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs index e899e47..48e6de6 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs @@ -49,11 +49,16 @@ public static class ServiceCollectionExtensions // Fail fast at startup on a misconfigured directory rather than on first login. services.AddSingleton, LdapOptionsValidator>(); - // LdapAuthService is stateless: it holds only a snapshot of LdapOptions and a stateless - // connection factory, and opens/disposes a connection per call. It is not IDisposable. - // Singleton is correct; TryAdd mirrors the pattern in AddZbApiKeyAuth (idempotency). + // LdapAuthService opens/disposes a connection per call and is not IDisposable. Its only + // mutable state is the sticky endpoint preference used by FallbackServers failover — + // which is precisely why the instance must be shared, so a primary-DC outage is paid + // for once rather than on every login. Singleton is correct; TryAdd mirrors the pattern + // in AddZbApiKeyAuth (idempotency). The logger is resolved with GetService, not + // GetRequiredService, so a host without logging configured still resolves the service. services.TryAddSingleton(sp => - new LdapAuthService(sp.GetRequiredService>().Value)); + new LdapAuthService( + sp.GetRequiredService>().Value, + sp.GetService>())); return services; } diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs index 0f7aa86..bbce996 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging; using Novell.Directory.Ldap; using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.Auth.Ldap.Internal; @@ -43,6 +44,7 @@ public sealed class LdapAuthService : ILdapAuthService private readonly LdapOptions _options; private readonly ILdapConnectionFactory _connectionFactory; private readonly IReadOnlyList _endpoints; + private readonly ILogger? _logger; /// /// Index into of the endpoint tried first. Written on any @@ -58,9 +60,15 @@ public sealed class LdapAuthService : ILdapAuthService /// bypass) is carried into the factory so each /// connection is built with it. /// - public LdapAuthService(LdapOptions options) + /// The directory connection and search settings. + /// + /// Optional. When supplied, endpoint failover is made visible: a Warning per failed + /// endpoint hop and an Information line when the sticky preference moves. Omitting it + /// (the default) leaves the service silent — hosts without logging still work. + /// + public LdapAuthService(LdapOptions options, ILogger? logger = null) : this(options, new NovellLdapConnectionFactory( - options.AllowInsecure, options.ServerCertificateValidationCallback)) + options.AllowInsecure, options.ServerCertificateValidationCallback), logger) { } @@ -69,10 +77,14 @@ public sealed class LdapAuthService : ILdapAuthService /// so the bind/search logic can be exercised without a live directory. Internal /// because the connection seam is an implementation detail. /// - internal LdapAuthService(LdapOptions options, ILdapConnectionFactory connectionFactory) + internal LdapAuthService( + LdapOptions options, + ILdapConnectionFactory connectionFactory, + ILogger? logger = null) { _options = options; _connectionFactory = connectionFactory; + _logger = logger; _endpoints = BuildEndpoints(options); } @@ -158,7 +170,10 @@ public sealed class LdapAuthService : ILdapAuthService // Updating only on Succeeded would re-pay the dead primary's connect timeout on // every mistyped password for the whole outage. if (index != preferred) + { Volatile.Write(ref _preferredIndex, index); + _logger?.LogInformation("LDAP sticky preference moved to {Endpoint}.", _endpoints[index]); + } return result; } @@ -291,13 +306,19 @@ public sealed class LdapAuthService : ILdapAuthService } catch (StageFailure stage) { - // A stage mapped its own failure; surface it as a structured result. + // A stage mapped its own failure; surface it as a structured result. Log at Warning + // so a silent failover — the failure mode this feature exists to survive — is + // visible in the host's log rather than only in a latency graph. + _logger?.LogWarning( + "LDAP endpoint {Endpoint} failed system-side at {Stage}; treating as no answer.", + endpoint, stage.Stage); return LdapAuthResult.Fail(stage.Failure); } - catch + catch (Exception ex) { // Belt-and-braces: ANY unexpected exception fails closed to the most conservative // system-side bucket rather than propagating to the caller. + _logger?.LogWarning(ex, "LDAP endpoint {Endpoint} failed system-side with an unexpected error.", endpoint); return LdapAuthResult.Fail(LdapAuthFailure.ServiceAccountBindFailed); } } diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/ZB.MOM.WW.Auth.Ldap.csproj b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/ZB.MOM.WW.Auth.Ldap.csproj index 4c58f23..25af340 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/ZB.MOM.WW.Auth.Ldap.csproj +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/ZB.MOM.WW.Auth.Ldap.csproj @@ -22,6 +22,7 @@ + diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceFailoverTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceFailoverTests.cs index 7b01a7e..e8804f1 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceFailoverTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceFailoverTests.cs @@ -234,4 +234,64 @@ public class LdapAuthServiceFailoverTests Assert.True((await svc.AuthenticateAsync("alice", "pw", CancellationToken.None)).Succeeded); Assert.Single(factory.Created); } + + // ---- observability ---- + + /// Captures emitted log entries so failover visibility can be asserted. + private sealed class RecordingLogger : Microsoft.Extensions.Logging.ILogger + { + public List<(Microsoft.Extensions.Logging.LogLevel Level, string Message)> Entries { get; } = new(); + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(Microsoft.Extensions.Logging.LogLevel logLevel) => true; + public void Log(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, + TState state, Exception? exception, Func formatter) + => Entries.Add((logLevel, formatter(state, exception))); + } + + [Fact] + public async Task Failover_LogsWarningWithEndpointAndStage_AndInfoOnStickyChange() + { + var logger = new RecordingLogger(); + var factory = new SequenceLdapConnectionFactory( + new FakeLdapConnection().ThrowOnConnect(), HealthyWithUser()); + var svc = new LdapAuthService(Opts("dc2:3894"), factory, logger); + + await svc.AuthenticateAsync("alice", "pw", CancellationToken.None); + + Assert.Contains(logger.Entries, e => + e.Level == Microsoft.Extensions.Logging.LogLevel.Warning + && e.Message.Contains("dc1:636") && e.Message.Contains("connect")); + Assert.Contains(logger.Entries, e => + e.Level == Microsoft.Extensions.Logging.LogLevel.Information + && e.Message.Contains("dc2:3894")); + // Never log secrets: + Assert.DoesNotContain(logger.Entries, e => e.Message.Contains("svcpw") || e.Message.Contains("pw\"")); + } + + [Fact] + public async Task HappyPath_LogsNothing() + { + var logger = new RecordingLogger(); + var factory = new SequenceLdapConnectionFactory(HealthyWithUser()); + var svc = new LdapAuthService(Opts("dc2"), factory, logger); + + await svc.AuthenticateAsync("alice", "pw", CancellationToken.None); + + Assert.Empty(logger.Entries); + } + + [Fact] + public async Task ServiceBindFailure_LogsTheServiceBindStage_NotConnect() + { + var logger = new RecordingLogger(); + var factory = new SequenceLdapConnectionFactory( + new FakeLdapConnection().ThrowOnServiceBind(), HealthyWithUser()); + var svc = new LdapAuthService(Opts("dc2"), factory, logger); + + await svc.AuthenticateAsync("alice", "pw", CancellationToken.None); + + Assert.Contains(logger.Entries, e => + e.Level == Microsoft.Extensions.Logging.LogLevel.Warning + && e.Message.Contains("dc1:636") && e.Message.Contains("service-bind")); + } } diff --git a/docs/plans/2026-08-10-ldap-backup-dc-failover.md.tasks.json b/docs/plans/2026-08-10-ldap-backup-dc-failover.md.tasks.json index a4c03a2..267aecc 100644 --- a/docs/plans/2026-08-10-ldap-backup-dc-failover.md.tasks.json +++ b/docs/plans/2026-08-10-ldap-backup-dc-failover.md.tasks.json @@ -1,14 +1,66 @@ { "planPath": "docs/plans/2026-08-10-ldap-backup-dc-failover.md", "tasks": [ - {"id": 1, "subject": "Task 1: LdapEndpoint + LdapEndpointParser", "status": "pending"}, - {"id": 2, "subject": "Task 2: LdapOptions.FallbackServers + validator rules", "status": "pending", "blockedBy": [1]}, - {"id": 3, "subject": "Task 3: SequenceLdapConnectionFactory test double", "status": "pending"}, - {"id": 4, "subject": "Task 4: Endpoint walk + sticky preference in LdapAuthService", "status": "pending", "blockedBy": [1, 2, 3]}, - {"id": 5, "subject": "Task 5: Failover observability (ILogger) + DI wiring", "status": "pending", "blockedBy": [4]}, - {"id": 6, "subject": "Task 6: GLAuth failover integration test", "status": "pending", "blockedBy": [5]}, - {"id": 7, "subject": "Task 7: Version 0.2.0, pack, publish, restore-verify", "status": "pending", "blockedBy": [6]}, - {"id": 8, "subject": "Task 8: Documentation + index truth", "status": "pending", "blockedBy": [7]} + { + "id": 1, + "subject": "Task 1: LdapEndpoint + LdapEndpointParser", + "status": "completed" + }, + { + "id": 2, + "subject": "Task 2: LdapOptions.FallbackServers + validator rules", + "status": "completed", + "blockedBy": [ + 1 + ] + }, + { + "id": 3, + "subject": "Task 3: SequenceLdapConnectionFactory test double", + "status": "completed" + }, + { + "id": 4, + "subject": "Task 4: Endpoint walk + sticky preference in LdapAuthService", + "status": "pending", + "blockedBy": [ + 1, + 2, + 3 + ] + }, + { + "id": 5, + "subject": "Task 5: Failover observability (ILogger) + DI wiring", + "status": "pending", + "blockedBy": [ + 4 + ] + }, + { + "id": 6, + "subject": "Task 6: GLAuth failover integration test", + "status": "pending", + "blockedBy": [ + 5 + ] + }, + { + "id": 7, + "subject": "Task 7: Version 0.2.0, pack, publish, restore-verify", + "status": "pending", + "blockedBy": [ + 6 + ] + }, + { + "id": 8, + "subject": "Task 8: Documentation + index truth", + "status": "pending", + "blockedBy": [ + 7 + ] + } ], "lastUpdated": "2026-08-10" -} +} \ No newline at end of file