search

microsoft / reverse-proxy

A toolkit for developing high-performance HTTP reverse proxy applications.
https://microsoft.github.io/reverse-proxy
MIT License
8.52k stars 836 forks source link

ForwarderMiddleware references prior cluster #1750

Open Tratcher opened 2 years ago

Tratcher commented 2 years ago

When using the reassign proxy requests as described in https://microsoft.github.io/reverse-proxy/articles/ab-testing.html, the ForwarderMiddleware continues to reference the original cluster via the Route.

https://github.com/microsoft/reverse-proxy/blob/a2165f4d9c53595d9e080c056f04e7e83761fbf0/src/ReverseProxy/Forwarder/ForwarderMiddleware.cs#L41

This affects logs and counters: https://github.com/microsoft/reverse-proxy/blob/a2165f4d9c53595d9e080c056f04e7e83761fbf0/src/ReverseProxy/Forwarder/ForwarderMiddleware.cs#L45 https://github.com/microsoft/reverse-proxy/blob/a2165f4d9c53595d9e080c056f04e7e83761fbf0/src/ReverseProxy/Forwarder/ForwarderMiddleware.cs#L69

There shouldn't be any functional impact.

This might be hard to fix because the cluster object referenced from the route is the only one with these fields.

samsp-msft commented 2 years ago

When you call context.ReassignProxyRequest(cluster); doesn't that set the cluster on the proxyfeature - or is this a case of the snapshot vs live config objects problem?

Tratcher commented 2 years ago

ReassignProxyRequest does set the cluster on the proxy feature, but the ForwarderMiddleware has some references to the original cluster via the route. The fields it's referencing are on the ClusterState, no the ClusterModel.

karelz commented 2 years ago

Triage: We log wrong cluster (the one before reassignment).