From 86b4d8781ca5c781753b3d03f3bef262a49cf57a Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Tue, 14 May 2024 21:28:35 +0200 Subject: [PATCH 1/2] Improve UseRequestTimeouts validation --- .../ProxyPipelineInitializerMiddleware.cs | 92 ++++++++++++++++--- ...ProxyPipelineInitializerMiddlewareTests.cs | 20 +++- 2 files changed, 93 insertions(+), 19 deletions(-) diff --git a/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs b/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs index c11050b6f..dbfe6a933 100644 --- a/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs +++ b/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs @@ -6,12 +6,11 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; #if NET8_0_OR_GREATER +using System.Threading; using Microsoft.AspNetCore.Http.Timeouts; +using Microsoft.Extensions.Options; #endif using Microsoft.Extensions.Logging; -#if NET8_0_OR_GREATER -using Yarp.ReverseProxy.Configuration; -#endif using Yarp.ReverseProxy.Utilities; namespace Yarp.ReverseProxy.Model; @@ -23,12 +22,23 @@ internal sealed class ProxyPipelineInitializerMiddleware { private readonly ILogger _logger; private readonly RequestDelegate _next; +#if NET8_0_OR_GREATER + private readonly IOptionsMonitor _timeoutOptions; +#endif public ProxyPipelineInitializerMiddleware(RequestDelegate next, - ILogger logger) + ILogger logger +#if NET8_0_OR_GREATER + , IOptionsMonitor timeoutOptions +#endif + ) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _next = next ?? throw new ArgumentNullException(nameof(next)); + +#if NET8_0_OR_GREATER + _timeoutOptions = timeoutOptions ?? throw new ArgumentNullException(nameof(timeoutOptions)); +#endif } public Task Invoke(HttpContext context) @@ -47,19 +57,11 @@ public Task Invoke(HttpContext context) context.Response.StatusCode = StatusCodes.Status503ServiceUnavailable; return Task.CompletedTask; } + #if NET8_0_OR_GREATER - // There's no way to detect the presence of the timeout middleware before this, only the options. - if (endpoint.Metadata.GetMetadata() != null - && context.Features.Get() == null - // The feature is skipped if the request is already canceled. We'll handle canceled requests later for consistency. - && !context.RequestAborted.IsCancellationRequested) - { - Log.TimeoutNotApplied(_logger, route.Config.RouteId); - // Out of an abundance of caution, refuse the request rather than allowing it to proceed without the configured timeout. - throw new InvalidOperationException($"The timeout was not applied for route '{route.Config.RouteId}', ensure `IApplicationBuilder.UseRequestTimeouts()`" - + " is called between `IApplicationBuilder.UseRouting()` and `IApplicationBuilder.UseEndpoints()`."); - } + EnsureRequestTimeoutPolicyIsAppliedCorrectly(context, endpoint, route); #endif + var destinationsState = cluster.DestinationsState; context.Features.Set(new ReverseProxyFeature { @@ -91,6 +93,66 @@ private async Task AwaitWithActivity(HttpContext context, Activity activity) } } +#if NET8_0_OR_GREATER + private void EnsureRequestTimeoutPolicyIsAppliedCorrectly(HttpContext context, Endpoint endpoint, RouteModel route) + { + // There's no way to detect the presence of the timeout middleware before this, only the options. + if (endpoint.Metadata.GetMetadata() is { } requestTimeout && + context.Features.Get() is null && + // The feature is skipped if the request is already canceled. We'll handle canceled requests later for consistency. + !context.RequestAborted.IsCancellationRequested && + // The policy may set the timeout to null / infinite. + TimeoutPolicyRequestedATimeoutBeSet(requestTimeout)) + { + // A timeout should have been set. + // Out of an abundance of caution, refuse the request rather than allowing it to proceed without the configured timeout. + Throw(route); + } + + void Throw(RouteModel route) + { + // The feature is skipped if the debugger is attached. + if (!Debugger.IsAttached) + { + Log.TimeoutNotApplied(_logger, route.Config.RouteId); + + throw new InvalidOperationException( + $"The timeout was not applied for route '{route.Config.RouteId}', " + + "ensure `IApplicationBuilder.UseRequestTimeouts()` is called between " + + "`IApplicationBuilder.UseRouting()` and `IApplicationBuilder.UseEndpoints()`."); + } + } + } + + private bool TimeoutPolicyRequestedATimeoutBeSet(RequestTimeoutAttribute requestTimeout) + { + if (requestTimeout.Timeout is not TimeSpan timeout) + { + if (requestTimeout.PolicyName is not string policyName) + { + Debug.Fail("Either Timeout or PolicyName should have been set."); + return false; + } + + if (!_timeoutOptions.CurrentValue.Policies.TryGetValue(policyName, out var policy)) + { + // This should only happen if the policy existed at some point, but the options were updated to remove it. + return false; + } + + if (policy.Timeout is null) + { + // The policy requested no timeout. + return false; + } + + timeout = policy.Timeout.Value; + } + + return timeout != Timeout.InfiniteTimeSpan; + } +#endif + private static class Log { private static readonly Action _noClusterFound = LoggerMessage.Define( diff --git a/test/ReverseProxy.Tests/Model/ProxyPipelineInitializerMiddlewareTests.cs b/test/ReverseProxy.Tests/Model/ProxyPipelineInitializerMiddlewareTests.cs index fdf917ab0..aaca94583 100644 --- a/test/ReverseProxy.Tests/Model/ProxyPipelineInitializerMiddlewareTests.cs +++ b/test/ReverseProxy.Tests/Model/ProxyPipelineInitializerMiddlewareTests.cs @@ -17,6 +17,7 @@ using Yarp.Tests.Common; using Yarp.ReverseProxy.Configuration; using Yarp.ReverseProxy.Forwarder; +using System.Diagnostics; namespace Yarp.ReverseProxy.Model.Tests; @@ -122,9 +123,12 @@ public async Task Invoke_NoHealthyEndpoints_CallsNext() Assert.Equal(StatusCodes.Status418ImATeapot, httpContext.Response.StatusCode); } + #if NET8_0_OR_GREATER - [Fact] - public async Task Invoke_MissingTimeoutMiddleware_RefuseRequest() + [Theory] + [InlineData(1)] + [InlineData(Timeout.Infinite)] + public async Task Invoke_MissingTimeoutMiddleware_RefuseRequest(int timeoutMs) { var httpClient = new HttpMessageInvoker(new Mock().Object); var cluster1 = new ClusterState(clusterId: "cluster1") @@ -140,7 +144,7 @@ public async Task Invoke_MissingTimeoutMiddleware_RefuseRequest() var aspNetCoreEndpoint = CreateAspNetCoreEndpoint(routeConfig, builder => { - builder.Metadata.Add(new RequestTimeoutAttribute(1)); + builder.Metadata.Add(new RequestTimeoutAttribute(timeoutMs)); }); aspNetCoreEndpoints.Add(aspNetCoreEndpoint); var httpContext = new DefaultHttpContext(); @@ -148,7 +152,15 @@ public async Task Invoke_MissingTimeoutMiddleware_RefuseRequest() var sut = Create(); - await Assert.ThrowsAsync(() => sut.Invoke(httpContext)); + if (timeoutMs == Timeout.Infinite || Debugger.IsAttached) + { + // If the timeout was infinite or the debugger is attached, we shouldn't refuse the request. + await sut.Invoke(httpContext); + } + else + { + await Assert.ThrowsAsync(() => sut.Invoke(httpContext)); + } } #endif From 7a6ce92c969f747c1fc172a58ed1e0c67ffb21b0 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 15 May 2024 23:40:48 +0200 Subject: [PATCH 2/2] Clarify helper name --- src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs b/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs index dbfe6a933..c5cb5d574 100644 --- a/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs +++ b/src/ReverseProxy/Model/ProxyPipelineInitializerMiddleware.cs @@ -106,10 +106,10 @@ private void EnsureRequestTimeoutPolicyIsAppliedCorrectly(HttpContext context, E { // A timeout should have been set. // Out of an abundance of caution, refuse the request rather than allowing it to proceed without the configured timeout. - Throw(route); + ThrowIfDebuggerNotAttached(route); } - void Throw(RouteModel route) + void ThrowIfDebuggerNotAttached(RouteModel route) { // The feature is skipped if the debugger is attached. if (!Debugger.IsAttached)