diff --git a/src/Testing/CoreTests/Acceptance/service_capabilities_resilience_3740.cs b/src/Testing/CoreTests/Acceptance/service_capabilities_resilience_3740.cs new file mode 100644 index 000000000..123589858 --- /dev/null +++ b/src/Testing/CoreTests/Acceptance/service_capabilities_resilience_3740.cs @@ -0,0 +1,89 @@ +using Microsoft.Extensions.Hosting; +using Shouldly; +using Wolverine; +using Wolverine.Configuration; +using Wolverine.Configuration.Capabilities; +using Wolverine.Runtime; +using Wolverine.Tracking; +using Wolverine.Transports; +using Xunit; + +namespace CoreTests.Acceptance; + +// GH-3740: ServiceCapabilities.ReadFrom is a purely diagnostic snapshot of the application's own +// configuration, and it is read over and over by monitoring tools like CritterWatch. A single throwing +// property getter anywhere in the object graph used to abort the entire read, so a monitoring console +// received *no* capabilities at all -- forever, since every subsequent attempt re-ran the same doomed code. +// It's now best effort: the offending item is logged and skipped, everything else still gets reported. +public class service_capabilities_resilience_3740 +{ + [Fact] + public async Task one_misbehaving_transport_does_not_lose_the_whole_snapshot() + { + using var host = await Host.CreateDefaultBuilder() + .UseWolverine(opts => + { + opts.Transports.Add(new BadlyBehavedTransport()); + opts.Transports.Add(new WellBehavedTransport()); + }) + .StartAsync(cancellationToken: TestContext.Current.CancellationToken); + + var capabilities = + await ServiceCapabilities.ReadFrom(host.GetRuntime(), null, TestContext.Current.CancellationToken); + + // The broker that can't describe itself is simply missing... + capabilities.Brokers.ShouldNotContain(x => x.ProtocolName == BadlyBehavedTransport.ProtocolName); + + // ...while every other broker, and the rest of the snapshot, comes through + capabilities.Brokers.ShouldContain(x => x.ProtocolName == WellBehavedTransport.ProtocolName); + capabilities.PropertyFor("ServiceName").ShouldNotBeNull(); + capabilities.DurabilitySettings.ShouldNotBeNull(); + } + + [Fact] + public async Task cancellation_is_not_swallowed_as_a_section_failure() + { + using var host = await Host.CreateDefaultBuilder() + .UseWolverine(opts => opts.Transports.Add(new BadlyBehavedTransport())) + .StartAsync(cancellationToken: TestContext.Current.CancellationToken); + + using var cancellation = new CancellationTokenSource(); + await cancellation.CancelAsync(); + + await Should.ThrowAsync(() => + ServiceCapabilities.ReadFrom(host.GetRuntime(), null, cancellation.Token)); + } +} + +internal class WellBehavedTransport : TransportBase +{ + public const string ProtocolName = "well-behaved"; + + public WellBehavedTransport() : base(ProtocolName, "Well Behaved Test Transport", [ProtocolName]) + { + } + + protected override IEnumerable endpoints() => []; + + protected override Endpoint findEndpointByUri(Uri uri) => throw new NotSupportedException(); +} + +internal class BadlyBehavedTransport : TransportBase +{ + public const string ProtocolName = "badly-behaved"; + + public BadlyBehavedTransport() : base(ProtocolName, "Badly Behaved Test Transport", [ProtocolName]) + { + } + + /// + /// Stands in for the real world case from GH-3740: AzureServiceBusTransport.HostName threw a + /// NullReferenceException whenever the connection was credential-based rather than connection-string + /// based, and JasperFx's OptionsDescription reads every public property reflectively. + /// + public string Explosive => throw new InvalidOperationException("Simulated bad property getter for GH-3740"); + + protected override IEnumerable endpoints() => []; + + protected override Endpoint findEndpointByUri(Uri uri) => throw new NotSupportedException(); +} diff --git a/src/Transports/Azure/Wolverine.AzureServiceBus.Tests/broker_description_with_credentials_3740.cs b/src/Transports/Azure/Wolverine.AzureServiceBus.Tests/broker_description_with_credentials_3740.cs new file mode 100644 index 000000000..eff95abd7 --- /dev/null +++ b/src/Transports/Azure/Wolverine.AzureServiceBus.Tests/broker_description_with_credentials_3740.cs @@ -0,0 +1,87 @@ +using Azure.Core; +using Shouldly; +using Wolverine.Configuration.Capabilities; +using Xunit; + +namespace Wolverine.AzureServiceBus.Tests; + +// GH-3740: AzureServiceBusTransport.HostName dereferenced ConnectionString! and therefore threw a +// NullReferenceException for every credential-based connection (FullyQualifiedNamespace + a TokenCredential / +// NamedKeyCredential / SasCredential), where there is no connection string at all. Because HostName is a +// public getter, JasperFx's OptionsDescription reached it reflectively while building Wolverine's +// ServiceCapabilities snapshot -- so a stock managed-identity setup could never hand its capabilities to a +// monitoring console. +public class broker_description_with_credentials_3740 +{ + [Fact] + public void host_name_falls_back_to_the_fully_qualified_namespace() + { + var transport = new AzureServiceBusTransport + { + FullyQualifiedNamespace = "myns.servicebus.windows.net", + TokenCredential = new StubTokenCredential() + }; + + transport.HostName.ShouldBe("myns.servicebus.windows.net"); + } + + [Fact] + public void can_describe_a_broker_connected_with_a_token_credential() + { + var transport = new AzureServiceBusTransport + { + FullyQualifiedNamespace = "myns.servicebus.windows.net", + TokenCredential = new StubTokenCredential() + }; + + var description = new BrokerDescription(transport); + + description.Endpoint.ShouldBe("myns.servicebus.windows.net"); + description.PropertyFor("HostName")!.Value.ShouldBe("myns.servicebus.windows.net"); + } + + [Fact] + public void host_name_is_null_when_there_is_no_connection_information_at_all() + { + new AzureServiceBusTransport().HostName.ShouldBeNull(); + + // and describing it is still harmless + new BrokerDescription(new AzureServiceBusTransport()).Endpoint.ShouldBeNull(); + } + + [Fact] + public void host_name_still_comes_from_the_connection_string_when_there_is_one() + { + var transport = new AzureServiceBusTransport + { + // The base64 SharedAccessKey has '=' padding -- the Endpoint parse has to stop at the first '=' + ConnectionString = + "Endpoint=sb://fromconnectionstring.servicebus.windows.net/;SharedAccessKeyName=root;SharedAccessKey=abc123==", + FullyQualifiedNamespace = "ignored.servicebus.windows.net" + }; + + transport.HostName.ShouldBe("fromconnectionstring.servicebus.windows.net"); + } + + [Fact] + public void host_name_falls_back_when_the_connection_string_has_no_endpoint_segment() + { + var transport = new AzureServiceBusTransport + { + ConnectionString = "SharedAccessKeyName=root;SharedAccessKey=abc123==", + FullyQualifiedNamespace = "myns.servicebus.windows.net" + }; + + transport.HostName.ShouldBe("myns.servicebus.windows.net"); + } + + internal class StubTokenCredential : TokenCredential + { + public override AccessToken GetToken(TokenRequestContext requestContext, CancellationToken cancellationToken) + => throw new NotSupportedException(); + + public override ValueTask GetTokenAsync(TokenRequestContext requestContext, + CancellationToken cancellationToken) + => throw new NotSupportedException(); + } +} diff --git a/src/Transports/Azure/Wolverine.AzureServiceBus/AzureServiceBusTransport.cs b/src/Transports/Azure/Wolverine.AzureServiceBus/AzureServiceBusTransport.cs index c95ec8355..8d1fdf6c6 100644 --- a/src/Transports/Azure/Wolverine.AzureServiceBus/AzureServiceBusTransport.cs +++ b/src/Transports/Azure/Wolverine.AzureServiceBus/AzureServiceBusTransport.cs @@ -22,7 +22,7 @@ public partial class AzureServiceBusTransport : BrokerTransport _managementClient; public readonly List Subscriptions = new(); - private string _hostName = null!; + private string? _hostName; public const string DeadLetterQueueName = DeadLetterQueueConstants.DefaultQueueName; public AzureServiceBusTransport() : this(ProtocolName) @@ -210,9 +210,14 @@ public async Task WithServiceBusClientAsync(Func action) } } + // GH-3740: both of these lazily *build a live Azure client* on first read. Diagnostic descriptions + // (JasperFx OptionsDescription, and through it Wolverine's ServiceCapabilities) reflect over every public + // property, so without this they would connect to Azure Service Bus as a side effect of describing the + // configuration -- and throw outright on any transport that has no connection information yet. + [IgnoreDescription] public ServiceBusClient BusClient => _busClient.Value; - + [IgnoreDescription] public ServiceBusAdministrationClient ManagementClient => _managementClient.Value; public async ValueTask DisposeAsync() @@ -271,24 +276,41 @@ protected override void tryBuildSystemEndpoints(IWolverineRuntime runtime) internal AzureServiceBusQueue? RetryQueue { get; set; } - public string HostName + /// + /// The host name of the connected Azure Service Bus namespace, parsed from the Endpoint segment of + /// when there is one, and otherwise the configured + /// (which *is* the host name for credential-based connections). + /// Null only when this transport has no connection information at all. + /// + public string? HostName { get { + // GH-3740: this used to blow up with a NullReferenceException on `ConnectionString!` for every + // credential-based connection (FullyQualifiedNamespace + TokenCredential / NamedKeyCredential / + // SasCredential), where there is no connection string at all. Being a public getter, it was + // reached reflectively by JasperFx's OptionsDescription while building the Wolverine + // ServiceCapabilities snapshot, so a perfectly valid managed-identity setup could never complete + // a CritterWatch handshake. if (_hostName == null) { - var parts = ConnectionString!.Split(';'); - foreach (var part in parts) + foreach (var part in ConnectionString?.Split(';') ?? []) { - var split = part.Split('='); - if (split[0].EqualsIgnoreCase("Endpoint")) + // Only split on the FIRST '=' -- SharedAccessKey values are base64 and routinely + // carry trailing '=' padding. + var split = part.Split('=', 2); + if (split.Length == 2 && split[0].Trim().EqualsIgnoreCase("Endpoint") && + Uri.TryCreate(split[1].Trim(), UriKind.Absolute, out var uri)) { - _hostName = new Uri(split[1]).Host; + _hostName = uri.Host; + break; } } + + _hostName ??= FullyQualifiedNamespace; } - return _hostName!; + return _hostName; } } diff --git a/src/Transports/Pulsar/Wolverine.Pulsar/PulsarTransport.cs b/src/Transports/Pulsar/Wolverine.Pulsar/PulsarTransport.cs index 26617f952..71d0d5c5a 100644 --- a/src/Transports/Pulsar/Wolverine.Pulsar/PulsarTransport.cs +++ b/src/Transports/Pulsar/Wolverine.Pulsar/PulsarTransport.cs @@ -36,6 +36,10 @@ public PulsarTransport(string protocol) : base(protocol, "Pulsar", [protocol]) new LightweightCache(uri => new PulsarEndpoint(uri, this)); } + // GH-3740: an indexer is still a public property as far as reflection is concerned, and reading one + // without index arguments throws TargetParameterCountException -- which used to abort the whole + // ServiceCapabilities snapshot for any application using Pulsar + [IgnoreDescription] public PulsarEndpoint this[Uri uri] => _endpoints[uri]; [IgnoreDescription] diff --git a/src/Wolverine/Configuration/Capabilities/ServiceCapabilities.cs b/src/Wolverine/Configuration/Capabilities/ServiceCapabilities.cs index 5d78e90be..50cbc44d8 100644 --- a/src/Wolverine/Configuration/Capabilities/ServiceCapabilities.cs +++ b/src/Wolverine/Configuration/Capabilities/ServiceCapabilities.cs @@ -7,6 +7,7 @@ using JasperFx.Events; using JasperFx.Events.Descriptors; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Wolverine.Persistence; using Wolverine.Persistence.Sagas; using Wolverine.Runtime; @@ -158,33 +159,79 @@ public static async Task ReadFrom(IWolverineRuntime runtime SystemControlUri = systemControlUri }; - readTransports(runtime, capabilities); + // GH-3740: every section below is a purely *diagnostic* read of the application's own configuration, + // and the snapshot as a whole is best effort. Before this, one misbehaving property getter, + // descriptor source, or unreachable database anywhere in the graph aborted ReadFrom outright -- and + // a monitoring console (CritterWatch) consequently received *no* capabilities at all for the + // service, permanently, since it retries the same doomed call on every batch. Each section is now + // isolated: it either contributes its data or logs a warning and is left out. + readSection(runtime, nameof(Brokers), token, () => readTransports(runtime, capabilities, token)); - await readMessageStores(runtime, capabilities); + await readSectionAsync(runtime, nameof(MessageStores), token, () => readMessageStores(runtime, capabilities)); - await readEventStores(runtime, token, capabilities); + await readSectionAsync(runtime, nameof(EventStores), token, () => readEventStores(runtime, token, capabilities)); - await readDocumentStores(runtime, token, capabilities); + await readSectionAsync(runtime, nameof(DocumentStores), token, + () => readDocumentStores(runtime, token, capabilities)); - await readDbContexts(runtime, token, capabilities); + await readSectionAsync(runtime, nameof(DbContexts), token, () => readDbContexts(runtime, token, capabilities)); - await readHttpGraphs(runtime, token, capabilities); + await readSectionAsync(runtime, nameof(HttpGraphs), token, () => readHttpGraphs(runtime, token, capabilities)); - readAspNetEndpoints(runtime, capabilities); + readSection(runtime, nameof(AspNetEndpoints), token, () => readAspNetEndpoints(runtime, capabilities)); - readGrpcEndpoints(runtime, capabilities); + readSection(runtime, nameof(GrpcEndpoints), token, () => readGrpcEndpoints(runtime, capabilities)); - readMessageTypes(runtime, capabilities); + readSection(runtime, nameof(Messages), token, () => readMessageTypes(runtime, capabilities)); - readEndpoints(runtime, capabilities); + readSection(runtime, nameof(MessagingEndpoints), token, () => readEndpoints(runtime, capabilities, token)); - readSagas(runtime, capabilities); + readSection(runtime, nameof(Sagas), token, () => readSagas(runtime, capabilities)); - readAdditionalCapabilities(runtime, capabilities); + readSection(runtime, nameof(AdditionalCapabilities), token, + () => readAdditionalCapabilities(runtime, capabilities)); return capabilities; } + private static void readSection(IWolverineRuntime runtime, string section, CancellationToken token, Action read) + { + try + { + read(); + } + catch (Exception e) + { + warnAboutFailedSection(runtime, section, token, e); + } + } + + private static async Task readSectionAsync(IWolverineRuntime runtime, string section, CancellationToken token, + Func read) + { + try + { + await read(); + } + catch (Exception e) + { + warnAboutFailedSection(runtime, section, token, e); + } + } + + private static void warnAboutFailedSection(IWolverineRuntime runtime, string section, CancellationToken token, + Exception e) + { + // A cancelled token means the host is shutting down or the caller gave up waiting -- that's not a + // failure of this particular section, and there's no point grinding through the remaining ones or + // logging a dozen warnings about it. + token.ThrowIfCancellationRequested(); + + runtime.Logger.LogWarning(e, + "Wolverine was unable to read the {Section} section of the ServiceCapabilities diagnostic snapshot for service {ServiceName}. That section will be missing or incomplete, the rest of the snapshot is unaffected.", + section, runtime.Options.ServiceName); + } + /// /// Mirror of for Wolverine HTTP /// graphs. Walks every @@ -248,12 +295,27 @@ private static void readGrpcEndpoints(IWolverineRuntime runtime, ServiceCapabili capabilities.GrpcEndpoints.AddRange(list.OrderBy(x => x.ServiceName + "::" + x.MethodName, StringComparer.Ordinal)); } - private static void readEndpoints(IWolverineRuntime runtime, ServiceCapabilities capabilities) + private static void readEndpoints(IWolverineRuntime runtime, ServiceCapabilities capabilities, + CancellationToken token) { foreach (var endpoint in runtime.Options.Transports.AllEndpoints().OrderBy(x => x.Uri.ToString())) { if (endpoint.Role == EndpointRole.System) continue; - capabilities.MessagingEndpoints.Add(new EndpointDescriptor(endpoint)); + + // GH-3740: same reasoning as readTransports() -- EndpointDescriptor reflects over the endpoint's + // public properties, and one bad getter shouldn't empty out the whole endpoint list + try + { + capabilities.MessagingEndpoints.Add(new EndpointDescriptor(endpoint)); + } + catch (Exception e) + { + token.ThrowIfCancellationRequested(); + + runtime.Logger.LogWarning(e, + "Wolverine was unable to build a diagnostic description of the endpoint at {EndpointUri}. It will be missing from the ServiceCapabilities snapshot.", + endpoint.Uri); + } } } @@ -451,13 +513,28 @@ private static void readAdditionalCapabilities(IWolverineRuntime runtime, Servic } } - private static void readTransports(IWolverineRuntime runtime, ServiceCapabilities capabilities) + private static void readTransports(IWolverineRuntime runtime, ServiceCapabilities capabilities, + CancellationToken token) { foreach (var transport in runtime.Options.Transports) { - if (transport.TryBuildBrokerUsage(out var usage)) + // GH-3740: BrokerDescription reflects over every public property of the transport, so one + // throwing getter on one broker's configuration must not cost us the description of every + // *other* broker in the application + try + { + if (transport.TryBuildBrokerUsage(out var usage)) + { + capabilities.Brokers.Add(usage); + } + } + catch (Exception e) { - capabilities.Brokers.Add(usage); + token.ThrowIfCancellationRequested(); + + runtime.Logger.LogWarning(e, + "Wolverine was unable to build a diagnostic description of the {Protocol} ({TransportName}) transport. It will be missing from the ServiceCapabilities snapshot.", + transport.Protocol, transport.Name); } } }