-
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 1 commit
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,22 @@ 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 HttpRequestException("The function invocation tied to this HTTP request failed."); | ||||||||||||||
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. The condition only checks for IsFaulted but doesn't handle IsCanceled state. If the invocation is canceled, retries should also stop. Consider checking
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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. Cancellation will be propagated to the HttpRequest and handled later in this file. I think we should leave the cancellation flow as is, and not add extra handling based on the script invocation result TCS. Edit: Here seems to be the only location this tcs is set to canceled, and this is a path that would lead to cancellation flowing to the worker and http request.
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
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 class ScriptInvocationRequestTransformer : HttpTransformer | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
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,135 @@ | ||
// 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); | ||
} | ||
} | ||
} | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.