search

ThreeMammals / Ocelot

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

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

Open mrclayman opened 1 year ago

mrclayman commented 1 year ago

Closes #360

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

Proposed Changes

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

raman-m commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. :+1:

raman-m commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

Integrated your PR feedback, @raman-m . See https://github.com/ThreeMammals/Ocelot/pull/1684/commits/6d29040db6570a01847ef596a2cdc583e146ad6c and let me know what you think. I tried to cover all of it so here's hoping I have succeeded at that. :crossed_fingers:

mrclayman commented 1 year ago

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

raman-m commented 1 year ago

@mrclayman commented on Sep 8:

Yeah, I'm flying to the Moon and back to the Earth. 🤣

raman-m commented 1 year ago

@mrclayman commented on Aug 16

I work in Linux. There is no Visual Studio for that. I have already had to disable .editorconfig

If you cannot use Visual Studio then provide settings to emulate .editorconfig in Visual Code on Linux. please.

Rider cares about it a lot more than Visual Studio judging by the awful mixture of line endings across the code base.

This mixture of line ending is current situation when developers use eighter Windows (CRLF) or Linux (LF). We have to live with that. Please, setup Rider for Ocelot to work with mixed line-endings please, without force implicit changes by Rider editor. Otherwise we will see a lot of strange diff without real changes.

I'll see if I can set Rider to not reorder usings but can't make any promises about that.

Yes, please! Don't forget about line endings problem. We have mixed line endings, explained above.

mrclayman commented 1 year ago

@raman-m commented on Sep 15

I have disabled the line ending settings inside .editorconfig so Rider won't enforce them. I don't think I can do much more than that, I'm afraid.

raman-m commented 1 year ago

@raman-m requested changes on Aug 14

@mrclayman I've resolved all issues of the 1st code review. The 2nd code review is ongoing...

Consider renaming UpstreamHeaderRoutingOptions class by suffix Options removing. Also, JSON property should be shorten like UpstreamHeaderRouting. What do you think?

mrclayman commented 1 year ago

@raman-m requested changes on Aug 14

@mrclayman I've resolved all issues of the 1st code review. The 2nd code review is ongoing...

Consider renaming UpstreamHeaderRoutingOptions class by suffix Options removing. Also, JSON property should be shorten like UpstreamHeaderRouting. What do you think?

There is already a class called UpstreamRoutingHeaders. IMO having UpstreamHeaderRouting would be confusing. If I were to rename something, it would be UpstreamRoutingHeaders -> UpstreamHeaderRouting and I would leave the Options class as is. :thinking:

At any rate, about to commit my changes. Once you explain to me how you intended me to fix the member/local variable question, I can consider that.

raman-m commented 1 year ago

@mrclayman Conflicts in usings block mostly! Use Remove and Sort Usings refactoring operation of Visual Studio. So, usings must be sorted! And pay attention, we have global usings now in Usings.cs file

raman-m commented 1 year ago

@mrclayman commented 2 weeks ago

Also why Upstream- prefix? Do we have DownstreamHeaderRouting? It seems Not. Also, HeaderRouting could be a good name for the feature.

mrclayman commented 1 year ago

@mrclayman Conflicts in usings block mostly! Use Remove and Sort Usings refactoring operation of Visual Studio. So, usings must be sorted! And pay attention, we have global usings now in Usings.cs file

I have already told you, I am NOT using Windows and therefore Visual Studio. Right now, I am desperately trying to find some merge tool that does not f*ck up line endings.

mrclayman commented 1 year ago

All right, I have rebased this branch against the current state of develop. Tried my best with regard to the line endings.

mrclayman commented 1 year ago

As for the Upstream prefix, I prefer clarity over intuition and I would like the prefix to stay -- both in configuration and the code. Feel free to rename it if you so desire. And as for the rest of the code, take it or leave it. I am done with this whole thing.

raman-m commented 1 year ago

@mrclayman commented on Oct 9

🆗 Got it! Try to use Visual Code if your workplace is Linux! Visual Code should be tolerant to line-endings of C# files in .NET projects.

raman-m commented 1 year ago

@mrclayman commented on Oct 11

Sorry, for being silent for 3 weeks. Our team is tiny: a few people only.

I'd like to postpone this thread because of 30+ open PRs with lower number. I'll return to you back in a couple of months...

I am done with this whole thing.

Thanks for your great contribution! 👍

raman-m commented 1 week ago

Zbyněk, is it feasible to resolve all the merge conflicts at this moment?

Please fix this 60 items diff pressing the Sync fork button!