Adding WebProxy support for outgoing requests #879

vdjuric · 2021-03-27T15:01:03Z

Hi,

I added initial WebProxy support for outgoing requests. This is useful for enterprise environments which frequently use internal web proxies.

All tests are successfully passing.

Kind regards,
Vladimir Djurić
https://vladimir.djuric.si/

ghost · 2021-03-27T15:01:15Z

All CLA requirements met.

Tratcher · 2021-03-29T02:18:32Z

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

+        /// <summary>
+        /// Optional web proxy used when communicating with the destination server. 
+        /// </summary>
+        public System.Net.IWebProxy WebProxy { get; init; }


Hmm, this would add another non-serializable type to the config model. We've been trying to limit that. Is there a reason we need to directly expose IWebProxy here? Would it be possible to expose the raw values from WebProxyConfigData here and then build the proxy instance in the client factory?

Yes, of course. I have done it this way to mimic ProxyHttpClientOptions.ClientCertificate implementation. I will remove IWebProxyConfigLoader and construct web proxy instance in client factory.

In future, we could maybe expose IWebProxy factory so user could have more power when generating web proxy instance. Similar pattern could be applied to ProxyHttpClientOptions.ClientCertificate implementation.

Thank you for your feedback!

Hi,

I just commited newest version. All tests are passing.

Is it worth exposing all proxy options here?
This could be just a string for the address, where one could use #869 if they need more customization.

I have done it this way to mimic ProxyHttpClientOptions.ClientCertificate implementation.

Yeah, ClientCertificate is the other problematic property that we haven't figured out what to do with.

@MihaZupan If one will be able to derive from ProxyHttpClientFactory, register new implementation (as singleton) via dependency injection and override ConfigureHandler, then WebProxyOptions can be simplified if needed (by removing BypassOnLocal and UseDefaultCredentials properties).

Although, it may be easier for end-user to configure WebProxy directly from config.

I am neutral here :)

@Tratcher For certificate loading I would create simple configuration structure with properties:
-X509FindType
-findValue
-validOnly

This would be used for X509Certificate2Collection.Find method: https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509certificate2collection.find?view=net-5.0

For more advanced scenarios one would have to derive from ProxyHttpClientFactory and override ConfigureHandler.

Although, it may be easier for end-user to configure WebProxy directly from config.

Right, I'm thinking in terms of how commonly such settings would be changed vs. us mirroring the SocketsHttpHandler surface here.

UseDefaultCredentials seems like it will be a common need. I don't know how frequently BypassOnLocal will be used, but if we already have two options then a third is harmless.

…tions

Tratcher

Looks good. I only have a few remaining cosmetic comments.

src/ReverseProxy/Service/Proxy/Infrastructure/ProxyHttpClientFactory.cs

Tratcher · 2021-03-29T17:08:58Z

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

+
+        /// <summary>
+        /// true to bypass the proxy for local addresses; otherwise, false.
+        /// If null, default value for <seealso cref="System.Net.WebProxy"/> implementation will be used.


Which is false?

Tratcher · 2021-03-29T17:09:21Z

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

+
+        /// <summary>
+        /// Controls whether the <seealso cref="System.Net.CredentialCache.DefaultCredentials"/> are sent with requests.
+        /// If null, default value for <seealso cref="System.Net.WebProxy"/> implementation will be used.


Include the default (false?)

Thanks!

I have included internal constants with default values in WebProxyOptions. I am not relying on default values from WebProxy implementation (but currently, they should be same).

Should we leave BypassOnLocal and UseDefaultCredentials properties as nullable bool? or should we change them to non-nullable bool ?

We prefer nullables in the config so an extended implementation can choose its own defaults.

…actory.cs Co-authored-by: Chris Ross <Tratcher@Outlook.com>

…main

Kahbazi · 2021-03-30T18:05:08Z

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs

+            }
+
+            var options = new WebProxyOptions();
+            webProxyConfig.Bind(options);


Bind is not used for other options, each property is read separately. Does this also need to change for consistency?

Additional question: since Bind is using reflection, is this acceptable?

Fair point. The biggest reason not to use bind is that it messes up the linker (the setters get deleted because it can't tell they're in use). It would be better for consistency to avoid Bind regardless.

Thank you! This should be now fixed.

Tratcher · 2021-03-30T19:57:58Z

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

+
+        internal const bool BypassOnLocalDefaultValue = false;


Suggested change

internal const bool BypassOnLocalDefaultValue = false;

The consumer (ProxyHttpClientFactory) gets to decide what the actual defaults are. The defaults in the doc comments reflect the defaults used by the default ProxyHttpClientFactory. (so many defaults...)

Thank you! Defaults should be now fixed :)

…tion.Bind

…ry implementation

…to CreateWebProxyOptions for better consistency

src/ReverseProxy/Service/Proxy/Infrastructure/ProxyHttpClientFactory.cs

…actory.cs Simplification for setting default value Co-authored-by: Kahbazi <akahbazi@gmail.com>

Tratcher

One comment about validation, but otherwise this looks good.

Tratcher · 2021-03-30T20:00:07Z

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs

@@ -320,6 +320,8 @@ private ProxyHttpClientOptions CreateProxyHttpClientOptions(IConfigurationSectio
                }
            }

+            var webProxy = TryGetWebProxyOptions(section.GetSection(nameof(ProxyHttpClientOptions.WebProxy)));


Style: Why isn't this called inline on line 336 like the others?

With newest commit, I moved web proxy parsing inline to bo more aligned with other parsing (minor cosmetic changes can be still adjusted if needed).

Tratcher · 2021-03-30T23:04:58Z

src/ReverseProxy/Utilities/ConfigurationReadingExtensions.cs

@@ -23,6 +23,16 @@ internal static class ConfigurationReadingExtensions
            return configuration[name] is string value ? TimeSpan.Parse(value, CultureInfo.InvariantCulture) : null;
        }

+        internal static Uri ReadUri(this IConfiguration configuration, string name)
+        {
+            if (configuration[name] is string value && Uri.TryCreate(value, UriKind.Absolute, out var parsedUri))


We've intentionally avoided the Try APIs in the config reader because we want to actively report errors. Use the normal constructor and let it throw if the value is invalid.

This makes sense. I changed this in newest commit.

Tratcher · 2021-03-31T23:42:33Z

Thanks

Adding WebProxy support for outgoing requests

d3e3762

vdjuric requested review from alnikola, MihaZupan and Tratcher as code owners March 27, 2021 15:01

Tratcher reviewed Mar 29, 2021

View reviewed changes

Tratcher self-assigned this Mar 29, 2021

vdjuric added 2 commits March 29, 2021 17:17

WebProxy is now constructed in ProxyHttpClientFactory from WebProxyOp…

d8f7a37

…tions

Additional ProxyHttpClientOptions == and != operator tests

717fb86

Tratcher approved these changes Mar 29, 2021

View reviewed changes

vdjuric and others added 4 commits March 30, 2021 08:28

Update src/ReverseProxy/Service/Proxy/Infrastructure/ProxyHttpClientF…

cd2e236

…actory.cs Co-authored-by: Chris Ross <Tratcher@Outlook.com>

Update src/ReverseProxy/Service/Proxy/Infrastructure/ProxyHttpClientF…

fa4940d

…actory.cs Co-authored-by: Chris Ross <Tratcher@Outlook.com>

Cosmetic changes

ce638cd

Merge branch 'main' of https://github.com/vdjuric/reverse-proxy into …

e43e78e

…main

Kahbazi reviewed Mar 30, 2021

View reviewed changes

Tratcher reviewed Mar 30, 2021

View reviewed changes

vdjuric added 3 commits March 30, 2021 22:13

Manually parsing WebProxyOptions instead of calling IConfigurationSec…

cdc095d

…tion.Bind

Moving default WebProxyOptions values to default ProxyHttpClientFacto…

e392220

…ry implementation

IConfigurationSection can not be null, renamed TryGetWebProxyOptions …

ca16f42

…to CreateWebProxyOptions for better consistency

Kahbazi reviewed Mar 30, 2021

View reviewed changes

src/ReverseProxy/Service/Proxy/Infrastructure/ProxyHttpClientFactory.cs Outdated Show resolved Hide resolved

vdjuric and others added 2 commits March 30, 2021 22:33

Update src/ReverseProxy/Service/Proxy/Infrastructure/ProxyHttpClientF…

d6f6b5d

…actory.cs Simplification for setting default value Co-authored-by: Kahbazi <akahbazi@gmail.com>

Cosmetic new line changes

10f11aa

Tratcher approved these changes Mar 30, 2021

View reviewed changes

vdjuric added 2 commits March 31, 2021 07:50

Reading WebProxyOptions inline in CreateProxyHttpClientOptions method

d81a75d

ReadUri method should throw Exception for invalid URI values

a4961c6

Tratcher merged commit 4b71268 into microsoft:main Mar 31, 2021

Tratcher added this to the YARP 1.0.0-preview11 milestone Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding WebProxy support for outgoing requests #879

Adding WebProxy support for outgoing requests #879

vdjuric commented Mar 27, 2021

ghost commented Mar 27, 2021 •

edited by ghost

Tratcher Mar 29, 2021

vdjuric Mar 29, 2021

vdjuric Mar 29, 2021

MihaZupan Mar 29, 2021 •

edited

Tratcher Mar 29, 2021

vdjuric Mar 29, 2021

vdjuric Mar 29, 2021

MihaZupan Mar 29, 2021 •

edited

Tratcher Mar 29, 2021

Tratcher left a comment

Tratcher Mar 29, 2021

Tratcher Mar 29, 2021

vdjuric Mar 30, 2021

Tratcher Mar 30, 2021

Kahbazi Mar 30, 2021

vdjuric Mar 30, 2021

Tratcher Mar 30, 2021

vdjuric Mar 30, 2021

Tratcher Mar 30, 2021 •

edited

vdjuric Mar 30, 2021

Tratcher left a comment

Tratcher Mar 30, 2021 •

edited

vdjuric Mar 31, 2021

Tratcher Mar 30, 2021

vdjuric Mar 31, 2021

Tratcher commented Mar 31, 2021

Adding WebProxy support for outgoing requests #879

Adding WebProxy support for outgoing requests #879

Conversation

vdjuric commented Mar 27, 2021

ghost commented Mar 27, 2021 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MihaZupan Mar 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MihaZupan Mar 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher left a comment

Choose a reason for hiding this comment

Tratcher Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Mar 31, 2021

ghost commented Mar 27, 2021 •

edited by ghost

MihaZupan Mar 29, 2021 •

edited

MihaZupan Mar 29, 2021 •

edited

Tratcher Mar 30, 2021 •

edited

Tratcher Mar 30, 2021 •

edited