Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ActivityTimeout to be set per destination #842

Merged
merged 1 commit into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand All @@ -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;
}
Expand Down
3 changes: 3 additions & 0 deletions src/protagonist/Orchestrator.Tests/appsettings.Testing.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@
"cantaloupe/one": {
"Address": "http://image-server"
}
},
"HttpRequest": {
"ActivityTimeout": "00:00:10"
}
},
"specialserver": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public static void MapFileHandling(this IEndpointRouteBuilder endpoints)

if (error != ForwarderError.None)
{
error.HandleProxyError(httpContext, logger);
error.HandleProxyError(httpContext, RequestOptions, logger);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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) };
}

/// <summary>
Expand Down Expand Up @@ -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)
{
Expand All @@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -34,14 +34,14 @@ public class DownstreamDestinationSelector
this.proxyStateLookup = proxyStateLookup;
this.orchestratorSettings = orchestratorSettings;
}

/// <summary>
/// Attempt to get <see cref="ClusterState"/> object for specified <see cref="ProxyDestination"/>
/// </summary>
/// <param name="destination">Destination to get ClusterState for</param>
/// <param name="clusterState">ClusterState, if found</param>
/// <returns>true if ClusterState found, else false</returns>
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ public static class ReverseProxyHandling
/// <summary>
/// Log any errors that arise during reverse-proxy forwarding operations
/// </summary>
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
Expand All @@ -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";
}
7 changes: 7 additions & 0 deletions src/protagonist/Orchestrator/appsettings.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@
}
},
"ReverseProxy": {
"Clusters": {
"cantaloupe": {
"HttpRequest": {
"ActivityTimeout": "00:00:30"
}
}
},
"Routes": {
"thumbs": {
"ClusterId": "thumbs",
Expand Down
Loading