Make the config object model read-only #662

Tratcher · 2021-01-13T00:18:08Z

Fixes #179

Today Cluster, ProxyRoute, etc. are mutable objects used to build the initial data model. Because these are mutable we clone them several times to prevent anyone from modifying them when we don't expect it, such as before calling user callbacks.

To avoid extra copies we want to make the input object model immutable but still have it be easy to construct. Here I've experimented with using C# 9 record types to achieve that. A record type is an immutable class with automatically generated methods like Clone and Equals. Using init only properties makes the objects still easy to construct. The new with operator allows making mutated clones of records so we can still manipulate the objects when we need to but not worry about someone else changing them unexpectedly.

There's a lot of churn-in-place here, but I'm happy with the results. All DeepClones were removed and I was able to remove many redundant classes from RuntimeModel that were only there as read-only copies of the config model.

samples/ReverseProxy.Code.Sample/CustomConfigFilter.cs

src/ReverseProxy/Abstractions/Config/IProxyConfigFilter.cs

davidfowl · 2021-01-13T01:59:08Z

src/ReverseProxy/Abstractions/Config/ITransformBuilder.cs

@@ -16,11 +16,11 @@ public interface ITransformBuilder
        /// Validates that each transform is known and has the expected parameters. All transforms are validated and
        /// so all errors can be reported.
        /// </summary>
-        IList<Exception> Validate(IList<IDictionary<string, string>> transforms);
+        IList<Exception> Validate(IReadOnlyList<IReadOnlyDictionary<string, string>> transforms);


This looks crazy 😄

Yeah. We'll see if I can sort it out as part of #60 (coming soon).

src/ReverseProxy/Abstractions/RouteDiscovery/Contract/ProxyMatch.cs

davidfowl · 2021-01-13T02:02:55Z

I like it!

samples/ReverseProxy.Config.Sample/ReverseProxy.Config.Sample.csproj

src/ReverseProxy/Abstractions/RouteDiscovery/Contract/ProxyRoute.cs

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs

src/ReverseProxy/Service/Config/ProxyRouteTransformExtensions.cs

ManickaP

Not a full pass through the change. I'll continue on Monday.

src/ReverseProxy/Abstractions/Config/IProxyConfigFilter.cs

src/ReverseProxy/Abstractions/RouteDiscovery/Contract/ProxyRoute.cs

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs

src/ReverseProxy/Utilities/ConfigurationReadingExtensions.cs

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs

ManickaP · 2021-01-15T19:45:35Z

src/ReverseProxy/Service/Config/ProxyRouteTransformExtensions.cs

+        /// </summary>
+        public static ProxyRoute WithTransformPathSet(this ProxyRoute proxyRoute, PathString path)
+        {
+            return proxyRoute.WithTransform(new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)


When initializing from the config we went an extra mile and every Dictionary/List properly wrapped in their ReadOnly counterpart. But here we're forgoing that and just passing Dictionary itself.
So either we do care that everything is properly readonly and nobody cannot even cast the instance to non-readonly counterpart. Or we can quite substantially simplify the config reading.
Or is there any other argument why is it here more lax?

There is some awkwardness around IDictionary vs IReadOnlyDictionary vs Dictionary. Dictionary implements both interfaces, but IDictionary does not implement IReadOnlyDictionary and requires wrapping. Many of the config and linq APIs only return an IDictionary.

Dictionary is easy to use here and we don't have to worry about it being modified because nobody else ever has access to it without downcasting or other tricks. I can try a ReadOnlyDictionary to see if that's as easy.

ManickaP · 2021-01-19T15:30:28Z

Will this change have any effect on the runtime model (RouteInfo, RouteConfig) or not? Or this this just to get rid of all the DeepClone stuff?

Tratcher · 2021-01-19T16:44:44Z

Will this change have any effect on the runtime model (RouteInfo, RouteConfig) or not? Or this this just to get rid of all the DeepClone stuff?

The main goal is to get rid of the DeepClone code. I don't expect many changes to the runtime model, but we may be able to remove some redundancy.

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

Tratcher · 2021-01-21T21:21:23Z

Will this change have any effect on the runtime model (RouteInfo, RouteConfig) or not? Or this this just to get rid of all the DeepClone stuff?

The main goal is to get rid of the DeepClone code. I don't expect many changes to the runtime model, but we may be able to remove some redundancy.

There was actually a lot of redundancy on ClusterConfig I was able to remove. There were lots of Cluster* types that were read-only copies original config that we didn't need anymore.

ManickaP

Look cleaner with the records!

ManickaP · 2021-01-26T17:03:11Z

src/ReverseProxy.ServiceFabric/ServiceDiscovery/Util/LabelsParser.cs

@@ -329,5 +338,13 @@ private static bool ContainsKey(string expectedKeyName, ReadOnlySpan<char> actua
            keyRemainder = actualKey.Slice(expectedKeyName.Length);
            return true;
        }
+
+        private class RouteHeaderFields


I'm not a fan of this extra class but I guess there's no reasonable way around it unfortunately.

src/ReverseProxy/Configuration/ConfigurationConfigProvider.cs

src/ReverseProxy/Microsoft.ReverseProxy.csproj

ManickaP · 2021-01-26T18:46:10Z

src/ReverseProxy/Service/RuntimeModel/ClusterConfig.cs

@@ -23,49 +21,22 @@ public sealed class ClusterConfig
    {
        public ClusterConfig(


Could we even get rid of ClusterConfig? Maybe not necessarily as part of this PR, but in general.

I think we still need it. It serves as an atomic unit of state that's updated on the ClusterInfo and snapshotted with requests.

ghost · 2021-01-26T20:52:47Z

Hello @Tratcher!

Because this pull request has the Auto-Merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Tratcher requested review from davidfowl, davidni, Kahbazi and alnikola January 13, 2021 00:18

Tratcher self-assigned this Jan 13, 2021

davidfowl reviewed Jan 13, 2021

View reviewed changes

samples/ReverseProxy.Code.Sample/CustomConfigFilter.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 13, 2021

View reviewed changes

src/ReverseProxy/Abstractions/Config/IProxyConfigFilter.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 13, 2021

View reviewed changes

src/ReverseProxy/Abstractions/RouteDiscovery/Contract/ProxyMatch.cs Show resolved Hide resolved

WeihanLi reviewed Jan 13, 2021

View reviewed changes

samples/ReverseProxy.Config.Sample/ReverseProxy.Config.Sample.csproj Outdated Show resolved Hide resolved

Kahbazi reviewed Jan 13, 2021

View reviewed changes

ManickaP reviewed Jan 15, 2021

View reviewed changes

Tratcher force-pushed the tratcher/freeze branch 2 times, most recently from 944f3a9 to e38a95e Compare January 21, 2021 17:17

ManickaP reviewed Jan 21, 2021

View reviewed changes

src/ReverseProxy/Abstractions/ClusterDiscovery/Contract/ActiveHealthCheckOptions.cs Outdated Show resolved Hide resolved

Tratcher added this to the YARP 1.0.0-preview9 milestone Jan 22, 2021

Tratcher marked this pull request as ready for review January 22, 2021 00:28

Tratcher requested a review from ManickaP January 25, 2021 19:29

ManickaP approved these changes Jan 26, 2021

View reviewed changes

Tratcher added 7 commits January 26, 2021 12:30

RouteHeader

ad03d5a

ProxyMatch

5b58ce0

ProxyRoute

38c64f9

ValueTask, Equals, new ProxyMatch

273321f

LangVersion

a73d5d7

Style, resize

aea0724

Remove extra Order

8995c7c

Tratcher added 19 commits January 26, 2021 12:37

Make Cluster a record

20842fc

GetHashCode

97fc144

Remove redundant ClusterHealthCheckOptions

d6e635e

Move LoadBalancingPolicy

2ec9e53

Remove ClusterSessionAffinityOptions

cd050a3

Remove ProxyHttpRequestOptions

365e395

Remove ClusterProxyHttpClientOptions

63160e7

Remove ClusterConfig.Metadata

72f7117

Remove IEquatable

b98d725

Remove IDeepCloneable

0dc2874

Cleanup

f781783

Cleanup

a120fb8

Revert unneeded test changes

08c82c9

Revert more test changes

22c52f6

Test gaps

1ddb2e0

Docs

df059c3

Doc comments

82add6c

Fix rebase

601f223

Simplify

243e82a

Tratcher force-pushed the tratcher/freeze branch from c3d9622 to 243e82a Compare January 26, 2021 20:48

Tratcher added the Auto-Merge Apply this label and the msftbot will auto-merge this PR when it is eligible for merge label Jan 26, 2021

Tratcher added the Breaking-Change label Jan 26, 2021

ghost merged commit 0b314e1 into master Jan 26, 2021

ghost deleted the tratcher/freeze branch January 26, 2021 21:07

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the config object model read-only #662

Make the config object model read-only #662

Tratcher commented Jan 13, 2021 •

edited

Loading

davidfowl Jan 13, 2021

Tratcher Jan 13, 2021

davidfowl commented Jan 13, 2021

ManickaP left a comment

ManickaP Jan 15, 2021

Tratcher Jan 15, 2021

ManickaP commented Jan 19, 2021

Tratcher commented Jan 19, 2021

Tratcher commented Jan 21, 2021

ManickaP left a comment

ManickaP Jan 26, 2021

ManickaP Jan 26, 2021

Tratcher Jan 26, 2021

ghost commented Jan 26, 2021

		@@ -23,49 +21,22 @@ public sealed class ClusterConfig
		{
		public ClusterConfig(

Make the config object model read-only #662

Make the config object model read-only #662

Conversation

Tratcher commented Jan 13, 2021 • edited Loading

davidfowl Jan 13, 2021

Choose a reason for hiding this comment

Tratcher Jan 13, 2021

Choose a reason for hiding this comment

davidfowl commented Jan 13, 2021

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP Jan 15, 2021

Choose a reason for hiding this comment

Tratcher Jan 15, 2021

Choose a reason for hiding this comment

ManickaP commented Jan 19, 2021

Tratcher commented Jan 19, 2021

Tratcher commented Jan 21, 2021

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP Jan 26, 2021

Choose a reason for hiding this comment

ManickaP Jan 26, 2021

Choose a reason for hiding this comment

Tratcher Jan 26, 2021

Choose a reason for hiding this comment

ghost commented Jan 26, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Tratcher commented Jan 13, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.