-
Notifications
You must be signed in to change notification settings - Fork 464
Stop retrying http requests when script invocation result is failed #11210
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ | |
using System.Net.Http; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Azure.WebJobs.Script.Description; | ||
using Microsoft.Azure.WebJobs.Script.Exceptions; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Microsoft.Azure.WebJobs.Script.Http | ||
|
@@ -30,11 +32,23 @@ public RetryProxyHandler(HttpMessageHandler innerHandler, ILogger logger) | |
|
||
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||
{ | ||
TaskCompletionSource<ScriptInvocationResult> resultSource = null; | ||
if (request.Options.TryGetValue(ScriptConstants.HttpProxyScriptInvocationContext, out ScriptInvocationContext scriptInvocationContext)) | ||
{ | ||
resultSource = scriptInvocationContext.ResultSource; | ||
} | ||
|
||
var currentDelay = InitialDelay; | ||
for (int attemptCount = 1; attemptCount <= MaxRetries; attemptCount++) | ||
{ | ||
try | ||
{ | ||
if (resultSource is not null && resultSource.Task.IsFaulted) | ||
{ | ||
throw resultSource.Task.Exception.InnerException ?? | ||
new Exception($"The function invocation tied to this HTTP request failed. Invocation ID: {scriptInvocationContext.ExecutionContext.InvocationId}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still undecided on what exception to throw here - we don't want to throw something that will cause retries (see below lines), but what makes the most sense (some cancellation-related exception? do we change the check for HttpRequestException below so we don't retry in some cases? |
||
} | ||
|
||
return await base.SendAsync(request, cancellationToken); | ||
} | ||
catch (TaskCanceledException) when (cancellationToken.IsCancellationRequested) | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Generic; | ||
using System.Net.Http; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.Azure.WebJobs.Script.Description; | ||
using Yarp.ReverseProxy.Forwarder; | ||
|
||
namespace Microsoft.Azure.WebJobs.Script.Http | ||
{ | ||
internal sealed class ScriptInvocationRequestTransformer : HttpTransformer | ||
{ | ||
public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken) | ||
{ | ||
// this preserves previous behavior (which called the default transformer) - base method is also called inside of here | ||
await HttpTransformer.Default.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, cancellationToken); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has to be |
||
|
||
if (httpContext.Items.TryGetValue(ScriptConstants.HttpProxyScriptInvocationContext, out object result) | ||
&& result is ScriptInvocationContext scriptContext) | ||
{ | ||
proxyRequest.Options.TryAdd(ScriptConstants.HttpProxyScriptInvocationContext, scriptContext); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,199 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Net.Http; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.Azure.WebJobs.Script.Description; | ||
using Microsoft.Azure.WebJobs.Script.Http; | ||
using Xunit; | ||
|
||
namespace Microsoft.Azure.WebJobs.Script.Tests.Http | ||
{ | ||
public class ScriptInvocationRequestTransformerTests | ||
{ | ||
private readonly ScriptInvocationRequestTransformer _transformer; | ||
|
||
public ScriptInvocationRequestTransformerTests() | ||
{ | ||
_transformer = new ScriptInvocationRequestTransformer(); | ||
} | ||
|
||
[Theory] | ||
[InlineData(true)] | ||
[InlineData(false)] | ||
public async Task TransformRequestAsync_IncludesXForwardedHeaders(bool includeScriptInvocationContext) | ||
{ | ||
var httpContext = new DefaultHttpContext(); | ||
httpContext.Request.Scheme = "https"; | ||
httpContext.Request.Host = new HostString("example.com", 443); | ||
httpContext.Request.PathBase = "/api"; | ||
httpContext.Request.Path = "/test"; | ||
httpContext.Request.QueryString = new QueryString("?param=value"); | ||
|
||
var remoteAddress = "192.168.1.100"; | ||
httpContext.Connection.RemoteIpAddress = System.Net.IPAddress.Parse(remoteAddress); | ||
|
||
if (includeScriptInvocationContext) | ||
{ | ||
var scriptContext = new ScriptInvocationContext | ||
{ | ||
FunctionMetadata = new FunctionMetadata { Name = "TestFunction" }, | ||
ExecutionContext = new ExecutionContext { InvocationId = Guid.NewGuid() } | ||
}; | ||
|
||
httpContext.Items[ScriptConstants.HttpProxyScriptInvocationContext] = scriptContext; | ||
} | ||
|
||
var proxyRequest = new HttpRequestMessage(HttpMethod.Get, "http://localhost:7071/api/test"); | ||
const string destinationPrefix = "http://localhost:7071"; | ||
|
||
await _transformer.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, CancellationToken.None); | ||
|
||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-For"), "X-Forwarded-For header should be present"); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Host"), "X-Forwarded-Host header should be present"); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Proto"), "X-Forwarded-Proto header should be present"); | ||
|
||
var forwardedFor = proxyRequest.Headers.GetValues("X-Forwarded-For"); | ||
Assert.Contains(remoteAddress, forwardedFor); | ||
|
||
var forwardedHost = proxyRequest.Headers.GetValues("X-Forwarded-Host"); | ||
Assert.Contains("example.com:443", forwardedHost); | ||
|
||
var forwardedProto = proxyRequest.Headers.GetValues("X-Forwarded-Proto"); | ||
Assert.Contains("https", forwardedProto); | ||
} | ||
|
||
[Fact] | ||
public async Task TransformRequestAsync_WithScriptInvocationContext_AddsContextToRequestOptions() | ||
{ | ||
var httpContext = new DefaultHttpContext(); | ||
httpContext.Request.Scheme = "http"; | ||
httpContext.Request.Host = new HostString("localhost", 7071); | ||
var remoteAddress = "192.168.1.100"; | ||
httpContext.Connection.RemoteIpAddress = System.Net.IPAddress.Parse(remoteAddress); | ||
|
||
var scriptContext = new ScriptInvocationContext | ||
{ | ||
FunctionMetadata = new FunctionMetadata { Name = "TestFunction" }, | ||
ExecutionContext = new ExecutionContext { InvocationId = Guid.NewGuid() } | ||
}; | ||
|
||
httpContext.Items[ScriptConstants.HttpProxyScriptInvocationContext] = scriptContext; | ||
|
||
var proxyRequest = new HttpRequestMessage(HttpMethod.Get, "http://localhost:7071/api/test"); | ||
const string destinationPrefix = "http://localhost:7071"; | ||
|
||
await _transformer.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, CancellationToken.None); | ||
|
||
Assert.True(proxyRequest.Options.TryGetValue(ScriptConstants.HttpProxyScriptInvocationContext, out ScriptInvocationContext contextValue)); | ||
Assert.Equal(scriptContext.ExecutionContext.InvocationId, contextValue.ExecutionContext.InvocationId); | ||
|
||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-For")); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Host")); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Proto")); | ||
} | ||
|
||
[Fact] | ||
public async Task TransformRequestAsync_PreservesExistingXForwardedHeaders() | ||
{ | ||
var httpContext = new DefaultHttpContext(); | ||
httpContext.Request.Scheme = "https"; | ||
httpContext.Request.Host = new HostString("proxy.example.com"); | ||
var requestRemoteAddress = "172.16.0.1"; | ||
httpContext.Connection.RemoteIpAddress = System.Net.IPAddress.Parse(requestRemoteAddress); | ||
|
||
// Add existing X-Forwarded headers to simulate request through multiple proxies | ||
var originalFor = "203.0.113.195," + requestRemoteAddress; | ||
var originalHost = "proxy.example.com"; | ||
var originalProto = "https"; | ||
httpContext.Request.Headers["X-Forwarded-For"] = originalFor; | ||
httpContext.Request.Headers["X-Forwarded-Host"] = originalHost; | ||
httpContext.Request.Headers["X-Forwarded-Proto"] = originalProto; | ||
|
||
var proxyRequest = new HttpRequestMessage(HttpMethod.Get, "http://localhost:7071/api/test"); | ||
const string destinationPrefix = "http://localhost:7071"; | ||
|
||
await _transformer.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, CancellationToken.None); | ||
|
||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-For")); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Host")); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Proto")); | ||
|
||
var forwardedFor = proxyRequest.Headers.GetValues("X-Forwarded-For"); | ||
Assert.Contains(requestRemoteAddress, forwardedFor); | ||
|
||
var forwardedHost = proxyRequest.Headers.GetValues("X-Forwarded-Host"); | ||
Assert.Contains(originalHost, forwardedHost); | ||
|
||
var forwardedProto = proxyRequest.Headers.GetValues("X-Forwarded-Proto"); | ||
Assert.Contains(originalProto, forwardedProto); | ||
} | ||
|
||
[Fact] | ||
public async Task TransformRequestAsync_PreservesStandardRequestHeaders() | ||
{ | ||
var httpContext = new DefaultHttpContext(); | ||
httpContext.Request.Scheme = "https"; | ||
httpContext.Request.Host = new HostString("example.com"); | ||
httpContext.Request.Path = "/api/test"; | ||
var remoteAddress = "192.168.1.100"; | ||
httpContext.Connection.RemoteIpAddress = System.Net.IPAddress.Parse(remoteAddress); | ||
|
||
// Add various standard headers that should be preserved | ||
httpContext.Request.Headers["Authorization"] = "Bearer token123"; | ||
httpContext.Request.Headers["User-Agent"] = "TestClient/1.0"; | ||
httpContext.Request.Headers["Accept"] = "application/json"; | ||
httpContext.Request.Headers["Content-Type"] = "application/json"; | ||
httpContext.Request.Headers["X-Custom-Header"] = "custom-value"; | ||
httpContext.Request.Headers["Cache-Control"] = "no-cache"; | ||
httpContext.Request.Headers["Accept-Encoding"] = "gzip, deflate"; | ||
|
||
var proxyRequest = new HttpRequestMessage(HttpMethod.Post, "http://localhost:7071/api/test"); | ||
const string destinationPrefix = "http://localhost:7071"; | ||
|
||
await _transformer.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, CancellationToken.None); | ||
|
||
// Verify that standard headers are preserved | ||
Assert.True(proxyRequest.Headers.Contains("Authorization"), "Authorization header should be preserved"); | ||
Assert.True(proxyRequest.Headers.Contains("User-Agent"), "User-Agent header should be preserved"); | ||
Assert.True(proxyRequest.Headers.Contains("Accept"), "Accept header should be preserved"); | ||
Assert.True(proxyRequest.Headers.Contains("X-Custom-Header"), "Custom headers should be preserved"); | ||
Assert.True(proxyRequest.Headers.Contains("Cache-Control"), "Cache-Control header should be preserved"); | ||
Assert.True(proxyRequest.Headers.Contains("Accept-Encoding"), "Accept-Encoding header should be preserved"); | ||
|
||
// Verify header values | ||
var authHeader = proxyRequest.Headers.GetValues("Authorization"); | ||
Assert.Contains("Bearer token123", authHeader); | ||
|
||
var userAgentHeader = proxyRequest.Headers.GetValues("User-Agent"); | ||
Assert.Contains("TestClient/1.0", userAgentHeader); | ||
|
||
var acceptHeader = proxyRequest.Headers.GetValues("Accept"); | ||
Assert.Contains("application/json", acceptHeader); | ||
|
||
var customHeader = proxyRequest.Headers.GetValues("X-Custom-Header"); | ||
Assert.Contains("custom-value", customHeader); | ||
|
||
var cacheControlHeader = proxyRequest.Headers.GetValues("Cache-Control"); | ||
Assert.Contains("no-cache", cacheControlHeader); | ||
|
||
var acceptEncodingHeader = proxyRequest.Headers.GetValues("Accept-Encoding"); | ||
Assert.Contains("gzip", acceptEncodingHeader); | ||
Assert.Contains("deflate", acceptEncodingHeader); | ||
|
||
// Verify that Content-Type is properly handled for the request content | ||
if (proxyRequest.Content != null) | ||
{ | ||
Assert.Equal("application/json", proxyRequest.Content.Headers.ContentType?.MediaType); | ||
} | ||
|
||
// Also verify X-Forwarded headers are still added | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-For")); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Host")); | ||
Assert.True(proxyRequest.Headers.Contains("X-Forwarded-Proto")); | ||
} | ||
} | ||
} | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.