Skip to content
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

Overload constructors for ClusterState and DestinationState to enable unit testing #2198

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

antonybstack
Copy link
Contributor

This addresses two open issues, #2142 and #1792, related to difficulties in unit testing custom middleware due to the inability to initialize/modify ClusterState and DestinationState.

The ClusterState and DestinationState constructors are now overloaded to accept additional parameters, which enables better unit testing when updating the ReverseProxyFeature via HttpContextFeaturesExtensions.

However it's worth noting that lifting these restrictions may allow the user to set the state in the ReverseProxyFeature that may potentially cause unintended side effects. Once I started diving deeper into the code, it seemed that there is some level of dependency on the immutable design, as there is logic related to making the state eventually consistent; I believe that's why there is some duplicate data such as the clusterId and destination configurations in the nested objects of ClusterState. But it seems that this potential risk was present before these changes, so it's most likely acceptable in a unit testing context.

Here is a sample of leveraging these new constructors in the context of the two open issues

    public void Configure(IApplicationBuilder app, IProxyStateLookup lookup)
    {
        app.UseRouting();
        app.UseAuthorization();
        app.UseEndpoints(endpoints =>
        {
            endpoints.MapControllers();
            endpoints.MapReverseProxy(proxyPipeline =>
            {
                proxyPipeline.Use((context, next) =>
                {
                    if (lookup.TryGetCluster("cluster2", out var cluster))
                    {
                        var newDestinationId = "destination3";
                        var newDestinationConfig = new DestinationConfig {
                            Address = "https://127.0.0.1:123/abcd1234/",
                            Health = "http://127.0.0.1:1234/"
                        };

                        var reverseProxyFeature = context.Features.Get<IReverseProxyFeature>();
                        // key-value stored in Metadata to trigger update
                        if(reverseProxyFeature.AvailableDestinations.Any(x => x.Model.Config.Metadata?["key"] != null)) {
                            var destinationNew = new DestinationState(
                                newDestinationId,
                                new DestinationModel(newDestinationConfig)
                            );

                            cluster = new ClusterState("cluster3", new ClusterModel(
                                    new ClusterConfig {
                                        ClusterId = "cluster3",
                                        Destinations =
                                            new Dictionary<string, DestinationConfig>(StringComparer
                                                .OrdinalIgnoreCase) { { destinationNew.DestinationId, destinationNew.Model.Config } }
                                    },
                                    new HttpMessageInvoker(new HttpClientHandler())
                                )
                            );

                            cluster.DestinationsState = new ClusterDestinationsState(
                                reverseProxyFeature.AllDestinations,
                                new List<DestinationState> { destinationNew }
                            );

                            context.ReassignProxyRequest(cluster);
                        }
                    }

                    return next();
                });
                proxyPipeline.UseSessionAffinity();
                proxyPipeline.UseLoadBalancing();
            });
        });
    }

These changes offer better granularity for unit testing through reassigning the ReverseProxyFeature with a ClusterState, via creating/modifying the ClusterModel along with its ClusterConfig, and the DestinationStates of the AllDestinations and AllDestinations properties with their associated DestinationModels and DestinationConfigs.

@antonybstack
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Tratcher Tratcher added this to the YARP 2.x milestone Jul 24, 2023
@Tratcher Tratcher merged commit 208df6b into microsoft:main Jul 24, 2023
6 checks passed
@Tratcher
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants