Stop one bad property getter from killing the whole capabilities snapshot (GH-3740)

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<OperationCanceledException>(() =>
+            ServiceCapabilities.ReadFrom(host.GetRuntime(), null, cancellation.Token));
+    }
+}
+
+internal class WellBehavedTransport : TransportBase<Endpoint>
+{
+    public const string ProtocolName = "well-behaved";
+
+    public WellBehavedTransport() : base(ProtocolName, "Well Behaved Test Transport", [ProtocolName])
+    {
+    }
+
+    protected override IEnumerable<Endpoint> endpoints() => [];
+
+    protected override Endpoint findEndpointByUri(Uri uri) => throw new NotSupportedException();
+}
+
+internal class BadlyBehavedTransport : TransportBase<Endpoint>
+{
+    public const string ProtocolName = "badly-behaved";
+
+    public BadlyBehavedTransport() : base(ProtocolName, "Badly Behaved Test Transport", [ProtocolName])
+    {
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    public string Explosive => throw new InvalidOperationException("Simulated bad property getter for GH-3740");
+
+    protected override IEnumerable<Endpoint> endpoints() => [];
+
+    protected override Endpoint findEndpointByUri(Uri uri) => throw new NotSupportedException();
+}

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<AccessToken> GetTokenAsync(TokenRequestContext requestContext,
+            CancellationToken cancellationToken)
+            => throw new NotSupportedException();
+    }
+}

src/Transports/Azure/Wolverine.AzureServiceBus/AzureServiceBusTransport.cs

@@ -22,7 +22,7 @@ public partial class AzureServiceBusTransport : BrokerTransport<AzureServiceBusE
     private readonly Lazy<ServiceBusAdministrationClient> _managementClient;
 
     public readonly List<AzureServiceBusSubscription> 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<ServiceBusClient, Task> 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
+    /// <summary>
+    /// The host name of the connected Azure Service Bus namespace, parsed from the <c>Endpoint</c> segment of
+    /// <see cref="ConnectionString"/> when there is one, and otherwise the configured
+    /// <see cref="FullyQualifiedNamespace"/> (which *is* the host name for credential-based connections).
+    /// Null only when this transport has no connection information at all.
+    /// </summary>
+    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;
         }
     }
 

src/Transports/Pulsar/Wolverine.Pulsar/PulsarTransport.cs

@@ -36,6 +36,10 @@ public PulsarTransport(string protocol) : base(protocol, "Pulsar", [protocol])
             new LightweightCache<Uri, PulsarEndpoint>(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]