Skip to content

#360 Routing based on values in designated upstream request headers #1684

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

#360 Routing based on values in designated upstream request headers #1684

wants to merge 13 commits into from

Conversation

mrclayman
Copy link

@mrclayman mrclayman commented Jul 28, 2023 •
edited by raman-m
Loading

Closes #360

Addition of support for request re-routing based also on upstream header configuration as per issues

Proposed Changes

  • Implementation of support for re-routing based on request's header configuration
  • Update of relevant tests to cover the new functionality
  • Update of relevant documentation to make clear how to make use of the new functionality

Downstream URL placeholder replacement mentioned in issue #360 is not yet supported although I do have plans to add support for that as well.

@mrclayman mrclayman mentioned this pull request Jul 28, 2023
Closed
@raman-m raman-m changed the title Implementation of support for routing based on values in designated u… #360 Routing based on values in designated upstream request headers Jul 31, 2023
@raman-m
Copy link
Member

raman-m commented Jul 31, 2023 •
edited
Loading

Zbyněk, thanks for the great PR! 💪

Does the PR close the #624 ?
I know it is closed, but maybe some implementations of #624 are included into this PR ?!

I am a bit confused by this PR because you said that it makes little sense to me to keep working on this any longer.
Anyway, I welcome your PR.
You need to understand that code review of this PR will take more time as for regular one because both solutions should be compared after a classic review.
This situation of having 2 solutions for the same issue is quite strange. 😄 But it is OK, we will have this experience. 😉

@raman-m raman-m added feature A new feature needs feedback Issue is waiting on feedback before acceptance labels Jul 31, 2023
@raman-m
Copy link
Member

raman-m commented Jul 31, 2023

Zbyněk,
Please request code review manually from the people you've communicated to in #360, if it is required on your opinion.
As for me it is hard to track your network.

@mrclayman
Copy link
Author

mrclayman commented Jul 31, 2023 •
edited
Loading

Well, you wanted to salvage my changes from the old branch, so I thought it shouldn't hurt to just to take them and put them in a new branch. 😄 At any rate, I have been thinking about the practical benefit of the current solution. You see, since users will still have to provide unique upstream URL for different downstreams, I thought that maybe we could consider allowing identical upstream URL, but differentiating the mappings based on the headers.It kind of makes sense in my head, but let me know what you and maybe also other maintainers think.

EDIT: Actually, just looking at the first post in #360 , it becomes clear that the above is a requirement, not just an option. I will have to look into allowing it as it looks like Ocelot currently won't allow multiple upstream routes to be the same.

@mrclayman
Copy link
Author

mrclayman commented Aug 5, 2023 •
edited
Loading

Cheers for the feedback, @RaynaldM . I have integrated it along with a modification to the FileConfigurationFluentValidator to allow for multiple routes having the same upstream path but different routing header layout. Seems to work nicely on my end but please do check out the changes and let me know if anything doesn't sit. In the meantime, I should probably look into placeholder replacement as some people voiced interest in having that working with headers also.

@mrclayman
Copy link
Author

Having taken a look at how URL placeholder extraction and replacement works, it's quite obvious that the current algorithm (implemented in UrlPathPlaceholderNameAndValueFinder) has been designed to work with URL's (routes and query arguments), which is suboptimal for header values as those usually don't contain either. I suppose I would therefore look into adapting some of the parts of the parser for use outside of URL patterns so that they could be reused in header value parser as well. It's just an initial idea that I have not thought a lot about, but let me know your take so we can agree on something. 👍

raman-m
raman-m requested changes Aug 14, 2023
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrclayman Hi Zbyněk!
Some suggestions:

  • Use file-scoped namespaces for new added files always, because we have .NET 6+ environment
  • Remove and Sort Usings by standard refactoring action in Visual Studio. I see you use some other RAD tool.
  • Use var option to define a variable for reference types

Don't judge me much while resolving these issues please! 🤣 👇

Helpful unit tests:

@raman-m
Copy link
Member

raman-m commented Aug 14, 2023 •
edited
Loading

@mrclayman commented on Aug 7, 2023

This is crazy idea!
I saw a lot of PRs and issues in the backlog which change and/or fix the logic of placeholders. So, placeholders-related logic is quite unstable, it has some unclear bug fixes, and not documented well...
Your refactoring idea can be a challenge for you! 🤣
Should I say "Good luck"? 😄

Why not to fix code review issues first? 😉
And, don't forget about @RaynaldM's code review suggestions too! 😺

@mrclayman
Copy link
Author

mrclayman commented Aug 16, 2023 •
edited by raman-m
Loading

@raman-m requested changes on Aug 14

I work in Linux. There is no Visual Studio for that. I have already had to disable .editorconfig because apparently Rider cares about it a lot more than Visual Studio judging by the awful mixture of line endings across the code base. I'll see if I can set Rider to not reorder usings but can't make any promises about that.

@mrclayman
Copy link
Author

Integrated your PR feedback, @raman-m . See 6d29040 and let me know what you think. I tried to cover all of it so here's hoping I have succeeded at that. 🤞

@mrclayman
Copy link
Author

Ping @raman-m, hope you didn't fall off the edge of the earth. 🙂

@raman-m raman-m force-pushed the develop branch 8 times, most recently from c216cdf to b458f0f Compare April 7, 2025 16:13
@raman-m raman-m force-pushed the develop branch 22 times, most recently from 908d84f to 0678e7a Compare April 19, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaynaldM RaynaldM RaynaldM left review comments

@raman-m raman-m raman-m requested changes

@binarymash binarymash Awaiting requested review from binarymash

@jps jps Awaiting requested review from jps

@geffzhang geffzhang Awaiting requested review from geffzhang

@philproctor philproctor Awaiting requested review from philproctor

@anktsrkr anktsrkr Awaiting requested review from anktsrkr

@sharpn sharpn Awaiting requested review from sharpn

@abooraja abooraja Awaiting requested review from abooraja

@wast wast Awaiting requested review from wast

@yangzhongke yangzhongke Awaiting requested review from yangzhongke

@thiagoloureiro thiagoloureiro Awaiting requested review from thiagoloureiro

Requested changes must be addressed to merge this pull request.

Assignees

@mrclayman mrclayman

Labels
conflicts Feature branch has merge conflicts feature A new feature Routing Ocelot feature: Routing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Routing based on Request Header
3 participants
@mrclayman @raman-m @RaynaldM