Configuration of the proxied HTTP request version. #512

ManickaP · 2020-11-03T16:50:54Z

Adds configuration side to setting up Version and VersioningPolicy (for .NET 5 and higher) of an outbound HTTP Request.

Fixes #282

src/ReverseProxy/Middleware/ProxyInvokerMiddleware.cs

test/ReverseProxy.Tests/Middleware/ProxyInvokerMiddlewareTests.cs

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs

Tratcher · 2020-11-04T00:49:39Z

src/ReverseProxy/Service/RuntimeModel/ClusterProxyHttpRequestOptions.cs

+
+namespace Microsoft.ReverseProxy.RuntimeModel
+{
+    public readonly struct ClusterProxyHttpRequestOptions


Discussed offline: we'll want to see how we can unify this with RequestProxyOptions so we don't have to allocate that per request. The big differences are:

RequestProxyOptions has Transforms on it which come from routeConfig.

RequestProxyOptions has the Timeout which should be easy to move/mirror.

struct vs class, readonly vs mutable

Naming

Some possibilities:

We come up with a type that works in both scenarios and use that on ClusterConfig.

RequestProxyOptions becomes a readonly struct so at least we don't have to allocate it. We'd still end up copying the paramters across.

Transforms get passed into IHttpProxy.ProxyAsync as a separate parameter, making the options types more compatible.

src/ReverseProxy/Abstractions/ClusterDiscovery/Contract/ProxyHttpRequestOptions.cs

src/ReverseProxy/Middleware/ProxyInvokerMiddleware.cs

src/ReverseProxy/Abstractions/ClusterDiscovery/Contract/ProxyHttpRequestOptions.cs

alnikola

Consider applying the request version settings to active health probes.

src/ReverseProxy/Middleware/ProxyInvokerMiddleware.cs

src/ReverseProxy/Service/Management/ProxyConfigManager.cs

alnikola · 2020-11-04T14:04:30Z

test/ReverseProxy.Tests/Service/HealthChecks/DefaultProbingRequestFactoryTests.cs

@@ -31,7 +31,7 @@ public void CreateRequest_HealthEndpointIsNotDefined_UseDestinationAddress(strin
        private ClusterConfig GetClusterConfig(string id, ClusterActiveHealthCheckOptions healthCheckOptions)
        {
            return new ClusterConfig(
-                new Cluster { Id = id }, new ClusterHealthCheckOptions(default, healthCheckOptions), default, default, null, default, null);
+                new Cluster { Id = id }, new ClusterHealthCheckOptions(default, healthCheckOptions), default, default, null, default, default, null);


If the request version settings are also applied to active health probes, it must be verified in this test.

alnikola

Production code looks good now, only a couple test-related suggestions.

src/ReverseProxy/Service/Config/ConfigValidator.cs

test/ReverseProxy.Tests/Service/HealthChecks/DefaultProbingRequestFactoryTests.cs

src/ReverseProxy/Abstractions/ClusterDiscovery/Contract/ProxyHttpRequestOptions.cs

src/ReverseProxy/Service/HealthChecks/DefaultProbingRequestFactory.cs

src/ReverseProxy/Service/Management/ProxyConfigManager.cs

ManickaP · 2020-11-09T16:13:18Z

@Tratcher, @alnikola: I think I've addressed all comments except for the type-merging one, which I'd rather do separately if you're OK with it. I'll file an issue and look into it immediately. I'd just prefer to split the work.

Tratcher

File an issue for the type merging.

Tratcher · 2020-11-09T18:06:56Z

docs/docfx/articles/proxyhttpclientconfig.md

+
+Configuration settings:
+- RequestTimeout - the timeout for the outgoing request sent by [HttpMessageInvoker.SendAsync](https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpmessageinvoker.sendasync?view=netcore-3.1). If not specified, 100 seconds is used.
+- Version - outgoing request [version](https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httprequestmessage.version?view=netcore-3.1). The supported values at the moment are 1.0, 1.1 and 2. Default value is 2.


Suggested change

- Version - outgoing request [version](https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httprequestmessage.version?view=netcore-3.1). The supported values at the moment are 1.0, 1.1 and 2. Default value is 2.

- Version - outgoing request [version](https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httprequestmessage.version?view=netcore-3.1). The supported values at the moment are `1.0`, `1.1` and `2`. Default value is `2`.

src/ReverseProxy/Abstractions/ClusterDiscovery/Contract/ProxyHttpRequestOptions.cs

src/ReverseProxy/Service/RuntimeModel/ClusterProxyHttpRequestOptions.cs

test/ReverseProxy.Tests/Service/Config/ConfigValidatorTests.cs

davidfowl · 2020-11-09T20:12:45Z

@ManickaP Hey, I'm working on this PR to change configuration reading #521. I'm going to wait until this PR is merged before reacting (since they conflict)

Tratcher · 2020-11-09T22:44:01Z

src/ReverseProxy/Service/RuntimeModel/ClusterProxyHttpRequestOptions.cs

+#else
+        public ClusterProxyHttpRequestOptions(TimeSpan? requestTimeout, Version version)
+        {
+            RequestTimeout = requestTimeout;
+            Version = version;
+        }
+#endif


This constructor should exist in both versions. We don't change/remove public API across versions if we can avoid it.

Suggested change

#else

public ClusterProxyHttpRequestOptions(TimeSpan? requestTimeout, Version version)

{

RequestTimeout = requestTimeout;

Version = version;

}

#endif

#endif

public ClusterProxyHttpRequestOptions(TimeSpan? requestTimeout, Version version)

{

RequestTimeout = requestTimeout;

Version = version;

}

ManickaP · 2020-11-10T09:06:08Z

@alnikola : could you please give this another look? I cannot merge till you approve since you originally requested changes.

alnikola

LGTM

Tratcher · 2020-11-10T17:12:03Z

src/ReverseProxy/Service/RuntimeModel/ClusterProxyHttpRequestOptions.cs

+            RequestTimeout = requestTimeout;
+            Version = version;
+#if NET
+            VersionPolicy = HttpVersionPolicy.RequestVersionOrLower;


VersionPolicy = null; The default is applied later.

Aaaa, you're right.

ManickaP added 2 commits November 3, 2020 17:01

Configuration of the proxied HTTP request version.

dc85381

Debug test.

47adcdb

ManickaP commented Nov 3, 2020

View reviewed changes

src/ReverseProxy/Middleware/ProxyInvokerMiddleware.cs Outdated Show resolved Hide resolved

ManickaP commented Nov 3, 2020

View reviewed changes

test/ReverseProxy.Tests/Middleware/ProxyInvokerMiddlewareTests.cs Outdated Show resolved Hide resolved

ManickaP commented Nov 3, 2020

View reviewed changes

test/ReverseProxy.Tests/Middleware/ProxyInvokerMiddlewareTests.cs Outdated Show resolved Hide resolved

Fixed tests, fixed Version parsing.

403d62b

ManickaP commented Nov 3, 2020

View reviewed changes

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs Outdated Show resolved Hide resolved

Tratcher reviewed Nov 4, 2020

View reviewed changes

alnikola suggested changes Nov 4, 2020

View reviewed changes

ManickaP added 2 commits November 4, 2020 19:01

Addressed some PR feedback.

fca0d54

Merge master

dfff014

alnikola reviewed Nov 5, 2020

View reviewed changes

src/ReverseProxy/Service/Config/ConfigValidator.cs Show resolved Hide resolved

test/ReverseProxy.Tests/Service/HealthChecks/DefaultProbingRequestFactoryTests.cs Outdated Show resolved Hide resolved

ManickaP added 3 commits November 5, 2020 14:52

Configurable request timeout.

885349b

Addressed more PR feedback.

48ec715

Specific probing tests for request version.

0b2fe2d

Tratcher reviewed Nov 6, 2020

View reviewed changes

test/ReverseProxy.Tests/Service/HealthChecks/DefaultProbingRequestFactoryTests.cs Outdated Show resolved Hide resolved

Tratcher reviewed Nov 6, 2020

View reviewed changes

ManickaP added 4 commits November 9, 2020 13:48

Addressed feedback.

5818b8e

Merge branch 'master' into mapichov/282_configuration

38a2434

Added documentation comments.

0e18423

Documentation.

dc10c82

ManickaP force-pushed the mapichov/282_configuration branch from aca9b23 to dc10c82 Compare November 9, 2020 16:08

Tratcher approved these changes Nov 9, 2020

View reviewed changes

ManickaP added 2 commits November 9, 2020 22:43

Reverted unintentional styling change.

a4b3dd7

More rounds of feedback.

cd097ed

ManickaP marked this pull request as ready for review November 9, 2020 21:54

ManickaP requested a review from halter73 as a code owner November 9, 2020 21:54

Tratcher reviewed Nov 9, 2020

View reviewed changes

Feedback.

067beaf

ManickaP mentioned this pull request Nov 10, 2020

Merge RequestProxyOptions with ClusterProxyHttpRequestOptions #525

Closed

alnikola approved these changes Nov 10, 2020

View reviewed changes

ManickaP merged commit 6d04918 into microsoft:master Nov 10, 2020

Tratcher reviewed Nov 10, 2020

View reviewed changes

ManickaP mentioned this pull request Nov 10, 2020

#512 Follow-up #526

Merged

Tratcher added this to the YARP 1.0.0-preview7 milestone Nov 10, 2020

ManickaP deleted the mapichov/282_configuration branch November 10, 2020 18:06

ManickaP mentioned this pull request Nov 12, 2020

Make cancelation configurable #319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration of the proxied HTTP request version. #512

Configuration of the proxied HTTP request version. #512

ManickaP commented Nov 3, 2020

Tratcher Nov 4, 2020

ManickaP Nov 10, 2020

alnikola left a comment

alnikola Nov 4, 2020

ManickaP Nov 4, 2020

alnikola left a comment

ManickaP commented Nov 9, 2020

Tratcher left a comment

Tratcher Nov 9, 2020

ManickaP Nov 9, 2020

davidfowl commented Nov 9, 2020 •

edited

Loading

Tratcher Nov 9, 2020

ManickaP Nov 10, 2020

ManickaP commented Nov 10, 2020

alnikola left a comment

Tratcher Nov 10, 2020

ManickaP Nov 10, 2020

	- Version - outgoing request [version](https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httprequestmessage.version?view=netcore-3.1). The supported values at the moment are 1.0, 1.1 and 2. Default value is 2.
	- Version - outgoing request [version](https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httprequestmessage.version?view=netcore-3.1). The supported values at the moment are `1.0`, `1.1` and `2`. Default value is `2`.

Configuration of the proxied HTTP request version. #512

Configuration of the proxied HTTP request version. #512

Conversation

ManickaP commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnikola left a comment

Choose a reason for hiding this comment

ManickaP commented Nov 9, 2020

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManickaP commented Nov 10, 2020

alnikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Nov 9, 2020 •

edited

Loading