Route key is not unique when using Host header for service discovery

[sliekens]

Expected Behavior

The RouteKey format should include a route's ServiceName when you use service discovery: "UpstreamPathTemplate|UpstreamHttpMethod|ServiceName".

Actual Behavior

The default format is "UpstreamPathTemplate|UpstreamHttpMethod|DownstreamHostAndPorts". The last section resolves to an empty string when you use service discovery: "/some-path|GET|".

This leads to conflicts when multiple routes have the same upstream method and path template, but are discriminated by an UpstreamHost.

Steps to Reproduce the Problem

1. Create two routes with identical UpstreamPathTemplate and UpstreamHttpMethod
2. Give each route a unique UpstreamHost
3. Do not configure the DownstreamHostAndPorts, instead configure a ServiceName and a ServiceDiscoveryProvider.

{
  "GlobalConfiguration": {
    "ServiceDiscoveryProvider": {
      "Scheme": "https",
      "Host": "localhost",
      "Port": 8500,
      "Type": "Consul"
    }
  },
  "Routes": [
    {
      "UpstreamHost": "api.contoso.de",
      "ServiceName": "BackofficeGermany",
      "UpstreamPathTemplate": "/some-path",
      "UpstreamHttpMethod": [ "Get" ],
      "DownstreamPathTemplate": "/some/path",
      "DownstreamScheme": "https"
    },
    {
      "UpstreamHost": "api.contoso.fr",
      "ServiceName": "BackofficeFrance",
      "UpstreamPathTemplate": "/some-path",
      "UpstreamHttpMethod": [ "Get" ],
      "DownstreamPathTemplate": "/some/path",
      "DownstreamScheme": "https"
    }
  ]
}

Now when you send two requests, once for each route:

GET /some-path HTTP/1.1
Host: api.contoso.de


GET /some-path HTTP/1.1
Host: api.contoso.fr

The first request initiates service discovery, which resolves the host and port for BackofficeGermany. The resolved configuration is stored in the ILoadBalancerHouse with the key "/some-path|GET|" .

The second request has the same route key so the ILoadBalancerHouse entirely skips the service discovery step. Instead it reuses the configuration for BackofficeGermany, which is wrong.

Specifications

• Version: 16.0.1
• Platform: netcoreapp3.1
• Subsystem: ??

[raman-m]

@sliekens
Hi Steven!
I don't get you!...
Seems you request new feature in service discovery feature...
Let me ask our Consul, @ggnaegi, the expert in Service Discovery feature.

[sliekens]

It's not a feature request, it's a problem with service discovery caching. The wrong route is reused from cache. A false cache hit, basically, because the route key does not include the service name.

[raman-m]

Actual Behavior

The default format is UpstreamPathTemplate|UpstreamHttpMethod|DownstreamHostAndPorts. The last section resolves to an empty string when you use service discovery: /some-path|GET|.
This leads to conflicts when multiple routes have the same upstream method and path template, but are discriminated by an UpstreamHost.

@sliekens 🆗 Got it now!
We will look into the code...
Why not create a PR by you? 😉

I guess we need this key construction template: ServiceName|UpstreamPathTemplate|UpstreamHttpMethod
So, service name should have higher priority over path and method.

Also, UpstreamHost could be a last suffix in caching key... What do you think?
Finally, the complete pattern will be ServiceName|UpstreamPathTemplate|UpstreamHttpMethod|UpstreamHost ❓

[sliekens]

Yep, that looks correct to me.

[sliekens]

@raman-m I created a PR, the final pattern is a bit different, but it shouldn't matter. The route key is treated as an opaque string once it's created.