fix(grpc): keep the RpcException off the failover warning so no sink can render Status.Detail
Review follow-ups: the warning now carries only the two endpoint URIs and the status code — attaching the exception object would let any real sink render its ToString(), which for RpcException includes server-authored Status.Detail. The leak pin test now also asserts no exception rides on the warning, via a new WarningExceptions capture on RecordingLogger. Plus two cosmetic nits: unified ThrowIfNull idiom, documented that FallbackEndpoints mutation after Validate is unsupported. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1
This commit is contained in:
+10
-6
@@ -78,9 +78,11 @@ internal sealed partial class FailoverSecretsHubReader : ISecretsHubReader
|
|||||||
nameof(endpointLabels));
|
nameof(endpointLabels));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
ArgumentNullException.ThrowIfNull(logger);
|
||||||
|
|
||||||
_readers = readers;
|
_readers = readers;
|
||||||
_endpointLabels = endpointLabels;
|
_endpointLabels = endpointLabels;
|
||||||
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
|
_logger = logger;
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
@@ -115,11 +117,13 @@ internal sealed partial class FailoverSecretsHubReader : ISecretsHubReader
|
|||||||
|
|
||||||
if (attempt < _readers.Count - 1)
|
if (attempt < _readers.Count - 1)
|
||||||
{
|
{
|
||||||
// The message carries the two endpoint URIs and the status code — nothing from
|
// The exception object is deliberately NOT attached to this warning: sinks
|
||||||
// the exception detail, which is server-authored text no credential belongs in
|
// render an attached exception's ToString(), which for RpcException includes
|
||||||
// but this node cannot vouch for.
|
// Status.Detail — server-authored text no credential belongs in but this node
|
||||||
|
// cannot vouch for. The two endpoint URIs plus the status code are what make
|
||||||
|
// the warning actionable, and the terminal failure still reaches the sweep's
|
||||||
|
// catch-all with the full exception when every endpoint has failed.
|
||||||
LogEndpointFailedOver(
|
LogEndpointFailedOver(
|
||||||
ex,
|
|
||||||
_endpointLabels[index],
|
_endpointLabels[index],
|
||||||
ex.StatusCode,
|
ex.StatusCode,
|
||||||
_endpointLabels[(start + attempt + 1) % _readers.Count]);
|
_endpointLabels[(start + attempt + 1) % _readers.Count]);
|
||||||
@@ -137,5 +141,5 @@ internal sealed partial class FailoverSecretsHubReader : ISecretsHubReader
|
|||||||
Level = LogLevel.Warning,
|
Level = LogLevel.Warning,
|
||||||
Message = "Secrets hub endpoint {FailedEndpoint} did not answer ({StatusCode}); failing over to {NextEndpoint}.")]
|
Message = "Secrets hub endpoint {FailedEndpoint} did not answer ({StatusCode}); failing over to {NextEndpoint}.")]
|
||||||
private partial void LogEndpointFailedOver(
|
private partial void LogEndpointFailedOver(
|
||||||
Exception exception, string failedEndpoint, StatusCode statusCode, string nextEndpoint);
|
string failedEndpoint, StatusCode statusCode, string nextEndpoint);
|
||||||
}
|
}
|
||||||
|
|||||||
+4
-2
@@ -38,8 +38,10 @@ public sealed class SecretsGrpcHubClientOptions
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// Getter-only so the configuration binder adds entries to this instance rather than needing a
|
/// Getter-only so the configuration binder adds entries to this instance rather than needing a
|
||||||
/// setter. Each entry carries the same http-cleartext caveat as <see cref="Endpoint"/>: a plain
|
/// setter. Mutating the list after <see cref="Validate"/> has run is unsupported — the DI
|
||||||
/// <c>http</c> fallback sends <see cref="BearerToken"/> in cleartext on every failed-over call.
|
/// extension reads it exactly once, at registration, to shape the channel graph. Each entry
|
||||||
|
/// carries the same http-cleartext caveat as <see cref="Endpoint"/>: a plain <c>http</c>
|
||||||
|
/// fallback sends <see cref="BearerToken"/> in cleartext on every failed-over call.
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
public IList<string> FallbackEndpoints { get; } = [];
|
public IList<string> FallbackEndpoints { get; } = [];
|
||||||
|
|
||||||
|
|||||||
+6
@@ -175,6 +175,12 @@ public sealed class FailoverSecretsHubReaderTests
|
|||||||
}
|
}
|
||||||
|
|
||||||
Assert.Equal(1, _logger.WarningCount);
|
Assert.Equal(1, _logger.WarningCount);
|
||||||
|
|
||||||
|
// The exception object must not ride along either: sinks render an attached exception's
|
||||||
|
// ToString(), which for RpcException includes Status.Detail — the very text the message
|
||||||
|
// template deliberately excludes. A clean message with the exception attached would still
|
||||||
|
// leak in production, so the warning must carry NO exception at all.
|
||||||
|
Assert.All(_logger.WarningExceptions, Assert.Null);
|
||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
|
|||||||
@@ -130,6 +130,14 @@ internal sealed class RecordingLogger<TCategory> : ILogger<TCategory>
|
|||||||
/// <summary>Formatted messages logged at <see cref="LogLevel.Warning"/>.</summary>
|
/// <summary>Formatted messages logged at <see cref="LogLevel.Warning"/>.</summary>
|
||||||
public List<string> Warnings { get; } = [];
|
public List<string> Warnings { get; } = [];
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The exception object attached to each Warning entry, index-aligned with
|
||||||
|
/// <see cref="Warnings"/>. Captured separately from the formatted message because real sinks
|
||||||
|
/// ALSO render an attached exception's <c>ToString()</c> — for <c>RpcException</c> that includes
|
||||||
|
/// <c>Status.Detail</c> — so a leak test that only scans the message would miss that channel.
|
||||||
|
/// </summary>
|
||||||
|
public List<Exception?> WarningExceptions { get; } = [];
|
||||||
|
|
||||||
/// <summary>Count of information entries.</summary>
|
/// <summary>Count of information entries.</summary>
|
||||||
public int InformationCount => Information.Count;
|
public int InformationCount => Information.Count;
|
||||||
|
|
||||||
@@ -161,6 +169,7 @@ internal sealed class RecordingLogger<TCategory> : ILogger<TCategory>
|
|||||||
break;
|
break;
|
||||||
case LogLevel.Warning:
|
case LogLevel.Warning:
|
||||||
Warnings.Add(message);
|
Warnings.Add(message);
|
||||||
|
WarningExceptions.Add(exception);
|
||||||
break;
|
break;
|
||||||
default:
|
default:
|
||||||
break;
|
break;
|
||||||
|
|||||||
Reference in New Issue
Block a user