[Enhanced Http] Support for http proxying scenarios #9193

satvu · 2023-03-31T20:37:17Z

Issue describing the changes in this PR

Add initial components for the enhanced HTTP model (read more here). These components are hidden behind a capability and this code path should only light up when a worker implementing the HTTP model provides the necessary info.

These are minimal changes needed to make it easier for partner teams and stakeholders to use and provide feedback as we continue to develop this feature.

Pull request checklist

My changes do not require documentation changes
- Otherwise: Documentation issue linked to PR
My changes should not be added to the release notes for the next release
- Otherwise: I've added my notes to release_notes.md
My changes do not need to be backported to a previous version
- Otherwise: Backport tracked by issue/PR #issue_or_pr
My changes do not require diagnostic events changes
- Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

…the context

* cleanup, move proxy logic to service

satvu · 2023-03-31T20:57:51Z

Working on tests but the rest is ready for review.

satvu · 2023-04-03T17:35:44Z

test/WebJobs.Script.Tests/Microsoft.Azure.WebJobs.Script.WebHost.deps.json

@@ -2190,11 +2190,11 @@
          "System.Runtime": "4.3.1"
        }
      },
-      "System.IO.Hashing/6.0.0": {


https://www.fuget.org/packages/System.IO.Hashing/7.0.0/lib/netstandard2.0/diff/6.0.0/

Reviewed the above w/ @fabiocav we believe this is a safe change.

Curious why this file is included at all in our code base? Being a build/restore artifact, shouldn't it be excluded?

Skipped this comment earlier but I am only editing it as it's needed to pass tests - it's a file in the test repo to make sure all dependency changes are caught and approval is asked from specific code owners (the test error message says so, you can see in earlier pipeline failures).

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

src/WebJobs.Script.Grpc/Server/DefaultHttpProxyService.cs

src/WebJobs.Script.WebHost/Middleware/FunctionInvocationMiddleware.cs

src/WebJobs.Script/Extensions/FunctionMetadataExtensions.cs

src/WebJobs.Script/ScriptConstants.cs

soninaren · 2023-04-03T22:31:15Z

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

@@ -81,6 +82,9 @@ internal class GrpcWorkerChannel : IRpcWorkerChannel, IDisposable
        private bool _isSharedMemoryDataTransferEnabled;
        private bool? _cancelCapabilityEnabled;
        private bool _isWorkerApplicationInsightsLoggingEnabled;
+        private IHttpProxyService _httpProxyService;
+        private Uri _httpProxyEndpoint;
+        private bool _isHttpProxyingWorker = false;


This would be cached when the worker is running under placeholder mode. Likely may not matter for dotnet worker. However, if there is customer config that controls the behavior then we may run into caching issues.

soninaren · 2023-04-04T00:35:04Z

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

@@ -420,6 +426,21 @@ internal void WorkerInitResponse(GrpcEvent initEvent)
                _isWorkerApplicationInsightsLoggingEnabled = true;
            }

+            // If http proxying is enabled, we need to get the proxying endpoint of this worker
+            var httpUri = _workerCapabilities.GetCapabilityState(RpcWorkerConstants.HttpUri);
+            if (!string.IsNullOrEmpty(httpUri) && FeatureFlags.IsEnabled(ScriptConstants.FeatureFlagEnableHttpProxying))


FeatureFlags.IsEnabled(ScriptConstants.FeatureFlagEnableHttpProxying) will always be false for worker started in placeholder mode

soninaren · 2023-04-04T00:38:05Z

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

@@ -738,6 +759,13 @@ internal async Task SendInvocationRequest(ScriptInvocationContext context)
                {
                    context.CancellationToken.Register(() => SendInvocationCancel(invocationRequest.InvocationId));
                }
+
+                if (_isHttpProxyingWorker && context.FunctionMetadata.IsHttpTriggerFunction())


IsHttpTriggerFunction explicitly looks at httpTrigger functions only. This does not include any webhooks from extensions such as durable or trigger that use http such as eventGrid. I am assuming this is by design.

satvu · 2023-04-04T03:39:13Z

src/WebJobs.Script.Grpc/GrpcServiceCollectionsExtensions.cs

@@ -16,6 +16,9 @@ public static IServiceCollection AddScriptGrpc(this IServiceCollection services)

            services.AddSingleton<IRpcServer, AspNetCoreGrpcServer>();

+            services.AddHttpForwarder();
+            services.AddSingleton<IHttpProxyService, DefaultHttpProxyService>();


Unfortunately, we can't get this behind the feature flag (can't access FeatureFlags yet at this point, host will crash) so these services will be added but not used.

soninaren · 2023-04-04T06:51:26Z

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

@@ -902,6 +930,17 @@ internal async Task InvokeResponse(InvocationResponse invokeResponse)
            {
                if (invokeResponse.Result.IsInvocationSuccess(context.ResultSource, capabilityEnabled))


What happens if IsInvocationSuccess is false but the proxy request made it through. Also what happens when IsInvocationSuccess is false and proxy request has failed.

The design for these cases hasn't been discussed but for now if IsInvocationSuccess fails at all we will ignore any proxying results and log an error.

soninaren · 2023-04-04T06:58:54Z

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

+
+                if (_isHttpProxyingWorker && context.FunctionMetadata.IsHttpTriggerFunction())
+                {
+                    var aspNetTask = _httpProxyService.Forward(context, _httpProxyEndpoint);


Are there any down sides of awaiting the task here? From what i am understanding the proxy request is already made here and awaiting the task simply provides with the forward errors if any. we can store the proxy result in the context and log it along with invocation success of failure.

Original design awaited the task here - I believe it changed because this task often completes after function invocation due to logic on the worker side (if there are no errors, this awaits until the http response is written back) and we decided to just send it off and unify if the function invocation succeeds.

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

jviau · 2023-03-31T23:29:18Z

src/WebJobs.Script.Grpc/Server/IHttpProxyService.cs

+{
+    public interface IHttpProxyService
+    {
+        public ValueTask<ForwarderError> Forward(ScriptInvocationContext context, Uri httpUri);


nit:

Suggested change

public ValueTask<ForwarderError> Forward(ScriptInvocationContext context, Uri httpUri);

public ValueTask<ForwarderError> ForwardAsync(ScriptInvocationContext context, Uri httpUri);

Also, should this take in a CancellationToken? Or is one available on the context object?

@jviau Yes CancellationToken implementation is something on the to-do list across the entire flow between host and worker.

src/WebJobs.Script.Grpc/Server/DefaultHttpProxyService.cs

src/WebJobs.Script/ScriptConstants.cs

satvu · 2023-04-04T17:52:14Z

Closing and postponing to next deadline instead.

satvu · 2023-04-04T18:57:33Z

Noting again for the record this is for private preview and reopening again because we want to reemphasize the main concerns:

are we concerned about reliability?
are we concerned about our ability to iterate

If both are no, then this work should be fine for now. We want to put this in the hands of some PMs and ASP.NET partners for further validation.

src/WebJobs.Script.Grpc/Server/IHttpProxyService.cs

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs

jviau · 2023-04-04T19:11:38Z

So long as it is clear this is private preview and we are okay with making breaking changes later I am fine with this.

satvu and others added 14 commits March 31, 2023 13:15

wip prototype

714da52

resolve errors, send messages to both grpc and http pipeline

baaae4b

wip

4257c20

e2e working, first iteration

7f4b66a

new iteration, use httpcontext items to signal proxying to middleware

3052efc

wip, waiting on aspnet pipeline to complete before finishing execution

0ff22dd

working, functions pipeline returns dummy value, aspnet pipeline has …

93c0b81

…the context

add env var for http proxy port

a0e4048

cleanup before move to feature branch

807b46c

cleanup following rebase

9c7f967

bump yarp version

8871f18

[Enhanced Http] Move Http Proxying logic to its own service (#9149)

142257f

* cleanup, move proxy logic to service

[Enhanced HTTP] Use proxying endpoint from worker response (#9184)

2b53757

cleanup

b2dbedb

satvu marked this pull request as ready for review March 31, 2023 20:37

satvu requested a review from a team as a code owner March 31, 2023 20:37

satvu requested review from fabiocav and brettsam March 31, 2023 20:38

add service provider to grpcworkerchannel tests

2e094ee

satvu added 2 commits March 31, 2023 14:03

mock proxy service

43bd487

add new pkgs to deps test

06cb2ad

satvu requested a review from mathewc as a code owner April 3, 2023 17:28

satvu commented Apr 3, 2023

View reviewed changes

surgupta-msft reviewed Apr 3, 2023

View reviewed changes

add feature flag

24cfbe7

soninaren reviewed Apr 4, 2023

View reviewed changes

satvu added 3 commits April 3, 2023 20:08

resolve comments

2360393

adding services behind feature flag

bfe5633

revert adding services behind feature flag

697880f

cleanup

1a1d5fe

satvu commented Apr 4, 2023

View reviewed changes

soninaren reviewed Apr 4, 2023

View reviewed changes

jviau reviewed Apr 4, 2023

View reviewed changes

resolve comments

c9988cd

satvu closed this Apr 4, 2023

satvu reopened this Apr 4, 2023

soninaren approved these changes Apr 4, 2023

View reviewed changes

jviau approved these changes Apr 4, 2023

View reviewed changes

src/WebJobs.Script.Grpc/Server/IHttpProxyService.cs Outdated Show resolved Hide resolved

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs Show resolved Hide resolved

src/WebJobs.Script.Grpc/Channel/GrpcWorkerChannel.cs Outdated Show resolved Hide resolved

resolve comments, validated e2e

f22dfca

fabiocav approved these changes Apr 4, 2023

View reviewed changes

satvu merged commit 5c6c379 into dev Apr 4, 2023

satvu deleted the feature/enhanced-http branch April 4, 2023 20:08

satvu restored the feature/enhanced-http branch April 11, 2023 20:26

		@@ -902,6 +930,17 @@ internal async Task InvokeResponse(InvocationResponse invokeResponse)
		{
		if (invokeResponse.Result.IsInvocationSuccess(context.ResultSource, capabilityEnabled))

	public ValueTask<ForwarderError> Forward(ScriptInvocationContext context, Uri httpUri);
	public ValueTask<ForwarderError> ForwardAsync(ScriptInvocationContext context, Uri httpUri);

[Enhanced Http] Support for http proxying scenarios #9193

[Enhanced Http] Support for http proxying scenarios #9193

Uh oh!

Conversation

satvu commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue describing the changes in this PR

Pull request checklist

Additional information

Uh oh!

satvu commented Mar 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

satvu commented Apr 4, 2023

Uh oh!

satvu commented Apr 4, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jviau commented Apr 4, 2023

Uh oh!

Uh oh!

satvu commented Mar 31, 2023 •

edited

Loading