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

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

Closed
ggnaegi opened this issue Apr 4, 2024 · 12 comments · Fixed by #2032 or #2033
Assignees
Labels
bug Identified as a potential bug Configuration Ocelot feature: Configuration Feb'24 February 2024 release high High priority hotfix Gitflow: Hotfix issue, PR related to hotfix branch merged Issue has been merged to dev and is waiting for the next release proposal Proposal for a new functionality in Ocelot
Milestone

Comments

@ggnaegi
Copy link
Member

ggnaegi commented Apr 4, 2024

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
Copy link
Member

raman-m commented Apr 4, 2024

@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
Copy link
Member Author

ggnaegi commented Apr 4, 2024

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

@ggnaegi
Copy link
Member Author

ggnaegi commented Apr 4, 2024

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
Copy link
Member

raman-m commented Apr 4, 2024

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 ggnaegi closed this as completed Apr 4, 2024
@ggnaegi
Copy link
Member Author

ggnaegi commented Apr 4, 2024

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
Copy link
Member

raman-m commented Apr 4, 2024

🤯 🤣

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

@raman-m raman-m reopened this Apr 4, 2024
@raman-m raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot high High priority Configuration Ocelot feature: Configuration Spring'24 Spring 2024 release labels Apr 4, 2024
@raman-m raman-m added this to the March-April'24 milestone Apr 4, 2024
@raman-m raman-m added the hotfix Gitflow: Hotfix issue, PR related to hotfix branch label Apr 4, 2024
@ggnaegi
Copy link
Member Author

ggnaegi commented Apr 4, 2024

🦄 👑 🌈

@raman-m raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label Apr 4, 2024
@raman-m raman-m self-assigned this Apr 4, 2024
@raman-m
Copy link
Member

raman-m commented Apr 4, 2024

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
Copy link
Member Author

ggnaegi commented Apr 4, 2024

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

@Fabman08
Copy link
Contributor

Fabman08 commented Apr 4, 2024

@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
Copy link
Member

raman-m commented Apr 4, 2024

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

@raman-m raman-m added Feb'24 February 2024 release and removed Spring'24 Spring 2024 release labels Apr 4, 2024
@raman-m
Copy link
Member

raman-m commented Apr 4, 2024

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

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

raman-m added a commit that referenced this issue Apr 5, 2024
* Remove faulty rules which validate explicit placeholders but not implicit ones

* Validation rule for Service Fabric placeholders

* Revert "Validation rule for Service Fabric placeholders"
This reverts commit c30fe45.
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Apr 5, 2024
@raman-m raman-m linked a pull request Apr 5, 2024 that will close this issue
@raman-m raman-m mentioned this issue Apr 5, 2024
@raman-m raman-m modified the milestones: March-April'24, February'24 May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug Configuration Ocelot feature: Configuration Feb'24 February 2024 release high High priority hotfix Gitflow: Hotfix issue, PR related to hotfix branch merged Issue has been merged to dev and is waiting for the next release proposal Proposal for a new functionality in Ocelot
Projects
None yet
3 participants