From 8f41d419eed55cbc5f01c2ea0227fbf69795b796 Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 8 May 2024 10:35:59 +0100 Subject: [PATCH] Allow ActivityTimeout to be set per destination --- .../Integration/ImageHandlingTests.cs | 2 ++ .../Infrastructure/TestProxyHttpClientFactory.cs | 6 ++++-- .../Orchestrator.Tests/appsettings.Testing.json | 3 +++ .../Features/Files/FileRouteHandlers.cs | 2 +- .../Features/Images/ImageRouteHandlers.cs | 16 +++++++++------- .../Features/TimeBased/TimeBasedRouteHandlers.cs | 3 +-- .../DownstreamDestinationSelector.cs | 6 +++--- .../ReverseProxy/ReverseProxyHandling.cs | 15 ++++++++++++++- src/protagonist/Orchestrator/appsettings.json | 7 +++++++ 9 files changed, 44 insertions(+), 16 deletions(-) diff --git a/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs b/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs index d10d14ed1..1102b075e 100644 --- a/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs +++ b/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs @@ -1215,6 +1215,7 @@ public async Task Get_ImageRequiresAuth_RedirectsToImageServer_AndSetsCookie_IfC response.StatusCode.Should().Be(HttpStatusCode.OK); response.Headers.Should().ContainKey("Set-Cookie"); response.Headers.Should().ContainKey("x-asset-id").WhoseValue.Should().ContainSingle(id.ToString()); + proxyResponse.ActivityTimeout.Should().BeCloseTo(TimeSpan.FromSeconds(10), TimeSpan.FromSeconds(2)); } [Theory] @@ -1464,6 +1465,7 @@ public async Task Get_RedirectsSpecialServer_AsFallThrough_ForFullRequests(strin response.Headers.CacheControl.MaxAge.Should().Be(TimeSpan.FromDays(28)); response.Headers.Should().ContainKey("x-test-key").WhoseValue.Should().BeEquivalentTo("foo bar"); response.Headers.Should().ContainKey("x-asset-id").WhoseValue.Should().ContainSingle(id.ToString()); + proxyResponse.ActivityTimeout.Should().BeCloseTo(TimeSpan.FromSeconds(60), TimeSpan.FromSeconds(2)); } [Theory] diff --git a/src/protagonist/Orchestrator.Tests/Integration/Infrastructure/TestProxyHttpClientFactory.cs b/src/protagonist/Orchestrator.Tests/Integration/Infrastructure/TestProxyHttpClientFactory.cs index 522abf54e..c20859dcf 100644 --- a/src/protagonist/Orchestrator.Tests/Integration/Infrastructure/TestProxyHttpClientFactory.cs +++ b/src/protagonist/Orchestrator.Tests/Integration/Infrastructure/TestProxyHttpClientFactory.cs @@ -60,7 +60,7 @@ public class TestProxyForwarder : IHttpForwarder await transformer.TransformResponseAsync(context, proxyResponse); // Write request parameters into response - await context.Response.WriteAsJsonAsync(new ProxyResponse(requestMessage)); + await context.Response.WriteAsJsonAsync(new ProxyResponse(requestMessage, requestConfig.ActivityTimeout)); return ForwarderError.None; } } @@ -72,13 +72,15 @@ public class ProxyResponse { public Uri Uri { get; set; } public HttpMethod Method { get; set;} + public TimeSpan? ActivityTimeout { get; set; } public ProxyResponse() { } - public ProxyResponse(HttpRequestMessage request) + public ProxyResponse(HttpRequestMessage request, TimeSpan? activityTimeout = null) { + ActivityTimeout = activityTimeout; Uri = request.RequestUri; Method = request.Method; } diff --git a/src/protagonist/Orchestrator.Tests/appsettings.Testing.json b/src/protagonist/Orchestrator.Tests/appsettings.Testing.json index 65d759aa4..8f65db871 100644 --- a/src/protagonist/Orchestrator.Tests/appsettings.Testing.json +++ b/src/protagonist/Orchestrator.Tests/appsettings.Testing.json @@ -71,6 +71,9 @@ "cantaloupe/one": { "Address": "http://image-server" } + }, + "HttpRequest": { + "ActivityTimeout": "00:00:10" } }, "specialserver": { diff --git a/src/protagonist/Orchestrator/Features/Files/FileRouteHandlers.cs b/src/protagonist/Orchestrator/Features/Files/FileRouteHandlers.cs index 543b07ae2..03fd61961 100644 --- a/src/protagonist/Orchestrator/Features/Files/FileRouteHandlers.cs +++ b/src/protagonist/Orchestrator/Features/Files/FileRouteHandlers.cs @@ -87,7 +87,7 @@ public static void MapFileHandling(this IEndpointRouteBuilder endpoints) if (error != ForwarderError.None) { - error.HandleProxyError(httpContext, logger); + error.HandleProxyError(httpContext, RequestOptions, logger); } } } \ No newline at end of file diff --git a/src/protagonist/Orchestrator/Features/Images/ImageRouteHandlers.cs b/src/protagonist/Orchestrator/Features/Images/ImageRouteHandlers.cs index 2c585c412..31bf7ada2 100644 --- a/src/protagonist/Orchestrator/Features/Images/ImageRouteHandlers.cs +++ b/src/protagonist/Orchestrator/Features/Images/ImageRouteHandlers.cs @@ -20,11 +20,10 @@ namespace Orchestrator.Features.Images; public static class ImageRouteHandlers { private static readonly HttpMessageInvoker HttpClient; - private static readonly ForwarderRequestConfig RequestOptions; + private static readonly ForwarderRequestConfig DefaultRequestOptions; static ImageRouteHandlers() { - // TODO - should this be shared by AV + Image handling? HttpClient = new HttpMessageInvoker(new SocketsHttpHandler { UseProxy = false, @@ -34,8 +33,7 @@ static ImageRouteHandlers() ActivityHeadersPropagator = new ReverseProxyPropagator(DistributedContextPropagator.Current) }); - // TODO - make this configurable, potentially by target - RequestOptions = new ForwarderRequestConfig { ActivityTimeout = TimeSpan.FromSeconds(60) }; + DefaultRequestOptions = new ForwarderRequestConfig { ActivityTimeout = TimeSpan.FromSeconds(60) }; } /// @@ -113,7 +111,7 @@ private static void HandleStatusCodeResult(HttpContext httpContext, StatusCodeRe return; } - var destination = destinationSelector.GetClusterTarget(httpContext, cluster!); + var destination = destinationSelector.GetClusterTarget(httpContext, cluster); if (destination == null) { @@ -125,11 +123,15 @@ private static void HandleStatusCodeResult(HttpContext httpContext, StatusCodeRe var root = destination.Model.Config.Address; var transformer = new PathRewriteTransformer(proxyAction); - var error = await forwarder.SendAsync(httpContext, root, HttpClient, RequestOptions, transformer); + var requestOptions = GetRequestOptions(cluster); + var error = await forwarder.SendAsync(httpContext, root, HttpClient, requestOptions, transformer); if (error != ForwarderError.None) { - error.HandleProxyError(httpContext, logger); + error.HandleProxyError(httpContext, requestOptions, logger); } } + + private static ForwarderRequestConfig GetRequestOptions(ClusterState clusterState) => + clusterState.Model.Config.HttpRequest ?? DefaultRequestOptions; } \ No newline at end of file diff --git a/src/protagonist/Orchestrator/Features/TimeBased/TimeBasedRouteHandlers.cs b/src/protagonist/Orchestrator/Features/TimeBased/TimeBasedRouteHandlers.cs index 5bcf3758f..9dd150609 100644 --- a/src/protagonist/Orchestrator/Features/TimeBased/TimeBasedRouteHandlers.cs +++ b/src/protagonist/Orchestrator/Features/TimeBased/TimeBasedRouteHandlers.cs @@ -21,7 +21,6 @@ public static class TimeBasedRouteHandlers static TimeBasedRouteHandlers() { - // TODO - should this be shared by AV + Image handling? HttpClient = new HttpMessageInvoker(new SocketsHttpHandler { UseProxy = false, @@ -87,7 +86,7 @@ public static void MapTimeBasedHandling(this IEndpointRouteBuilder endpoints) // Check if the proxy operation was successful if (error != ForwarderError.None) { - error.HandleProxyError(httpContext, logger); + error.HandleProxyError(httpContext, RequestOptions, logger); } } } \ No newline at end of file diff --git a/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/DownstreamDestinationSelector.cs b/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/DownstreamDestinationSelector.cs index 46aa050a0..d821f7696 100644 --- a/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/DownstreamDestinationSelector.cs +++ b/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/DownstreamDestinationSelector.cs @@ -1,6 +1,6 @@ using System; using System.Collections.Generic; -using System.Linq; +using System.Diagnostics.CodeAnalysis; using DLCS.Core.Collections; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging; @@ -34,14 +34,14 @@ public class DownstreamDestinationSelector this.proxyStateLookup = proxyStateLookup; this.orchestratorSettings = orchestratorSettings; } - + /// /// Attempt to get object for specified /// /// Destination to get ClusterState for /// ClusterState, if found /// true if ClusterState found, else false - public bool TryGetCluster(ProxyDestination destination, out ClusterState? clusterState) + public bool TryGetCluster(ProxyDestination destination, [NotNullWhen(true)] out ClusterState? clusterState) { clusterState = null; if (destination is ProxyDestination.S3 or ProxyDestination.Unknown) diff --git a/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/ReverseProxyHandling.cs b/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/ReverseProxyHandling.cs index 98e6f9e35..d11d73ddc 100644 --- a/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/ReverseProxyHandling.cs +++ b/src/protagonist/Orchestrator/Infrastructure/ReverseProxy/ReverseProxyHandling.cs @@ -9,7 +9,8 @@ public static class ReverseProxyHandling /// /// Log any errors that arise during reverse-proxy forwarding operations /// - public static void HandleProxyError(this ForwarderError error, HttpContext httpContext, ILogger logger) + public static void HandleProxyError(this ForwarderError error, HttpContext httpContext, + ForwarderRequestConfig requestOptions, ILogger logger) { if (error is ForwarderError.RequestCanceled or ForwarderError.RequestBodyCanceled or ForwarderError.ResponseBodyCanceled or ForwarderError.UpgradeRequestCanceled @@ -28,6 +29,18 @@ public static void HandleProxyError(this ForwarderError error, HttpContext httpC return; } + if (error is ForwarderError.RequestTimedOut) + { + logger.LogError("Proxy error {Error} for {Path}, ActivityTimeout: {ActivityTimeout}", error, + httpContext.Request.Path, GetActivityTimeoutForLog(requestOptions)); + return; + } + logger.LogError(errorFeature.Exception, "Proxy error {Error} for {Path}", error, httpContext.Request.Path); } + + private static string GetActivityTimeoutForLog(ForwarderRequestConfig requestOptions) => + requestOptions.ActivityTimeout.HasValue + ? $"{requestOptions.ActivityTimeout.Value.TotalMilliseconds}ms" + : "default"; } \ No newline at end of file diff --git a/src/protagonist/Orchestrator/appsettings.json b/src/protagonist/Orchestrator/appsettings.json index eee15a7e2..d18b51df7 100644 --- a/src/protagonist/Orchestrator/appsettings.json +++ b/src/protagonist/Orchestrator/appsettings.json @@ -71,6 +71,13 @@ } }, "ReverseProxy": { + "Clusters": { + "cantaloupe": { + "HttpRequest": { + "ActivityTimeout": "00:00:30" + } + } + }, "Routes": { "thumbs": { "ClusterId": "thumbs",