Query params aren't proxied correctly (Service Fabric)

[davidni]

Expected Behavior

Query params should be proxied correctly.

Actual Behavior

When using Service Fabric, an incoming request to https://example.com/a?b=c ends up being sent to https://localhost:19081/a??b=c&cmd=instance. I suspect the duplicated ?? problem exists without Service Fabric too.

I'm surprised this hasn't been caught before since it seems very easy to encounter. Perhaps there's a way around this that I just haven't found yet (for now I have code on my side to patch up the DownstreamContext before making the request.

Steps to Reproduce the Problem

Use a dummy rule that proxies UpstreamPath "/{everything}" to DownstreamPath "/{everything}". Make a request that matches the rule, with or without query parameters.

Specifications

• Version: 8.0.3 - 10.0.2
• Platform: Windows 10, .NET Framework 4.6.2

[davidni]

1. Root cause of the ?? issue: DownstreamRequest.ToHttpRequestMessage().

var uriBuilder = new UriBuilder
{
    Port = Port,
    Host = Host,
    Path = AbsolutePath,
    Query = Query,
    Scheme = Scheme
};

It probably shouldn't just assign to UriBuilder.Query, and should instead remove leading ? itself. MSDN docs have a not-very-obvious remark about this:

Do not append a string directly to this property. If the length of Query is greater than 1, retrieve the property value as a string, remove the leading question mark, append the new query string, and set the property with the combined string.

2. Root cause of the cmd=instance query param: DownstreamUrlCreatorMiddleware.CreateServiceFabricUri. I don't know why this was added in the first place. Was there any doc describing this should be added? I have run tests without it and it works as expected.

[TomPallister]

@davidni there is a similar issue with query string params on the "/{everything}" route. I guess there is a lack of tests here :/

With regards to cmd=instance are you asking why this is being added?

[davidni]

With regards to cmd=instance are you asking why this is being added?

Indeed. Service Fabric Reverse Proxy doesn't seem to care about this query param, and it gets passed transparently all the way to the target service.

It is problematic because it means a service behind Ocelot cannot provide functionality that relies on a query param called cmd (since Ocelot hard-codes that). I was curious why this was added in the beginning, and I am suggesting that it be removed.

Makes sense?

[TomPallister]

@davidni I think I added it because when originally making Ocelot work with SF I saw that the sf client was appending it for requests to instance services. I actually don't know SF that well so if you know we don't need it more than happy to remove!!

[davidni]

Yes, let's remove it. Documentation is here

A call to Reverse Proxy takes this shape:
http(s)://<Cluster FQDN | internal IP>:Port/<ServiceInstanceName>/<Suffix path>?PartitionKey=<key>&PartitionKind=<partitionkind>&ListenerName=<listenerName>&TargetReplicaSelector=<targetReplicaSelector>&Timeout=<timeout_in_seconds>

The query params shown have special meaning for Reverse Proxy. Everything else is proxied without change. cmd is not special in any way per the docs.

[davidni]

To summarize, there are 2 specific asks for this issue:

1. Fix the double question mark ??
2. Remove cmd=instance for Service Fabric.

You mentioned (1) might be a dupe of another issue. Do you recall which one?

[TomPallister]

1. Might be related to Issue with OData query options #548

[TomPallister]

@davidni can you share your json config for this issue? I'm having trouble replicating the problem.

[TomPallister]

If you can point me in the right direction with the commit above that could also help!

[davidni]

1. I'll be out of office for a few days, and unfortunately I won't have time to look into this before I leave -- sorry.

2. I briefly looked at your PR, seems reasonable and nothing stood out on initial review. I wonder if there could be a behavior change between .NET Core Vs. .NET Framework re. how System.Uri works. For reference, my repro was on .NET Framework 4.6.1

Thanks again for your efforts to always raise the bar and improve quality.

[TomPallister]

No worries I’m away all weekend as well so won’t be too much movement. I will try on Windows classic .net and see what happens. I really hope that is not the case!

[TomPallister]

Should be fixed in 10.0.4! Let me know if any problems.

[kdrummond]

I am experiencing the same behavior reported by @davidni wrt item 1, running Ocelot v12.0.1. I can see that the DownstreamRequest constructor sets Query = _request.RequestUri.Query, and ToHttpRequestMessage() and ToUri() both pass those values as-is to UriBuilder. The issue is that the value carries the '?' as the first character, and per the MSDN quote David referenced above, this must be manually stripped out.

I'm using very simple ReRoutes rules, such as this:

    {
      "DownstreamPathTemplate": "/routing/{catchAll}",
      "DownstreamScheme": "https",
      "DownstreamHostAndPorts": [
        {
          "Host": "localhost",
          "Port": 23651
        }
      ],
      "UpstreamPathTemplate": "/routing/{catchAll}",
      "UpstreamHttpMethod": [ "GET", "POST", "PUT", "DELETE", "OPTIONS" ],
    },

...which is turning my routes from "https://localhost:5099/routing/services/ServerVersion?myparam=myvalue" into "https://localhost:23651/routing/services/ServerVersion??myparam=myvalue".

Replacing the Query statement in the DownstreamRequest constructor with this, for example, resolves the issue:

            Query = (!string.IsNullOrEmpty(_request.RequestUri.Query) && _request.RequestUri.Query[0].Equals('?'))
                ? _request.RequestUri.Query.Substring(1)
                : _request.RequestUri.Query;

I'm using Ocelot in a ASP.NET app targeting .NET Framework 4.6.1, on Win 10, VS 2017.

[raman-m]

• Fixed by #555 added some tests for this issue but struggling to replicate #570