ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.28k stars 1.63k forks source link

Release 23.2: UpstreamPathTemplate doesn't contain the same placeholders in DownstreamPathTemplate #2031

Closed ggnaegi closed 4 months ago

ggnaegi commented 4 months ago

Expected Behavior / New Feature

It should be possible to specify the placeholders freely. You might want to transform the parameters - by using a delegating handler - before calling the downstream services.

Actual Behavior / Motivation for New Feature

Ocelot throws an exception if upstream path template doesn't contain the same placeholders as downstream path template. It's a new validation introduced in Release 23.2.0. It's a breaking change with some bad side effects.

I'm using a delegating handler to transform some upstream route parameters before calling the downstream service. eg. upstream: api/v1/service/endpoint/{upParam1}/{upParam2} -> downstream api/v1/internal-service/endpoint/{downParam}. It was working like a charm, since Release 23.2.0 it's not possible anymore.

Workaround would be to add some dummy query parameters to the upstream path, but I think it's misleading.

Besides, this code part https://github.com/AlyHKafoury/Ocelot/blob/11916b6672c6ee8330cab04e1545ed6dfeb5dcfe/src/Ocelot/Configuration/Validator/FileConfigurationFluentValidator.cs#L45-L50 is clearly out of PR scope.

Steps to Reproduce the Problem

  1. Create an ocelot configuration file with an upstream route that does not contain the same placeholders as the downstream route. Ocelot will throw an exception during startup.

Specifications

raman-m commented 4 months ago

@ggnaegi Thanks for reporting! What environment did you catch the problem in? Production or development?

And, first of all, show the code of your delegating handler please.

ggnaegi commented 4 months ago

No, unfortunately, I can't show you the delegating handler code. I catched the problem on staging environment.

ggnaegi commented 4 months ago

As mentioned, there are plenty of possible use cases where you could end up with the placeholders not matching. It's quite obvious in my opinion.

raman-m commented 4 months ago

In my opinion this is not so obvious. I don't see delegation handler code or overridden services in DI. Impossible to advise or discuss what is hidden. It's very strange to report a problem and then talk about the code being closed.

Our project is an open source project ❗

All discussions and source code should be publicly available.

ggnaegi commented 4 months ago

I don't want to argue about this any longer, thanks. I think we have added to much validation there, that's it. I will come back with a new PR later.

raman-m commented 4 months ago

🤯 🤣

No, we will continue to search for an ideal solutions... This is our destiny!

ggnaegi commented 4 months ago

🦄 👑 🌈

raman-m commented 4 months ago

I'm using a delegating handler to transform some upstream route parameters before calling the downstream service. eg. upstream: api/v1/service/endpoint/{upParam1}/{upParam2} -> downstream api/v1/internal-service/endpoint/{downParam}. It was working like a charm, since Release 23.2 it's not possible anymore.

@ggnaegi How to emulate your user scenario? Can we test it? I expect it is necessary to write a fake delegating handler in acceptance test. Is it a new feature or custom hybrid setup in your app? Will you help to write acceptance test?

ggnaegi commented 4 months ago

Yes, I can provide the fake delegating handler. I could write the acceptance test too.

Fabman08 commented 4 months ago

@raman-m I've the same problem with the following configuration

"UpstreamPathTemplate": "/{serviceVersion}/myService/{any}",
"DownstreamPathTemplate": "v3.0/api/myRemoteService/{any}"

In my case it doesn't matter what {serviceVersion} is set on the UpstreamPathTemplate, the DownstreamPathTemplate must forwards request to version v3.0

raman-m commented 4 months ago

Now I'm testing the hotfix... I'll open a PR soon...

raman-m commented 4 months ago

@ggnaegi @RaynaldM @Fabman08 Welcome to code review for #2032 !

@ggnaegi You could add acceptance test for your user scenario if you want.