Upstream header routing #964

mrclayman · 2019-07-17T07:54:33Z

Closes #360

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

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.

…). Tests are WIP.

* Fixed issue with request headers not getting converted to lowercase prior to matching * Made request value matching case-insensitive * Removed overriden Equals() from UpstreamRoutingHeaders * Made proper use of Shouldly in tests

TomPallister · 2020-03-11T20:23:33Z

please open PR again against master branch

raman-m · 2023-06-28T18:23:52Z

@mrclayman
What's the new PR number?
Let's link both of them.

Update 1

I have found nothing! 😢

raman-m · 2023-06-28T18:27:58Z

As a compromise we can retarget current PR still to develop, because develop branch is protected now and it is default branch of the repo! And, master branch was default one in 2020, I suppose.
So, just reopening this PR is required. 🤣

But the feature branch is too old! 😞 I expect a lot of merge conflicts.
That's why it is better to create new PR, or fight with merge conflicts...

mrclayman · 2023-06-30T18:44:54Z

Just tried rebasing my branch on the current state in develop and yeah, I might as well scrap the current branch and start a new one since some of the basic components I used in the past are gone now. It makes perfect sense anyway since so much must have changed in the meantime.

mrclayman · 2023-07-04T16:01:18Z

Ok, so I have picked out my changes from the ancient branch and applied them to a new one. It's been straightforward for the most part, but I am wondering -- there are two implementations of IDownstreamRouteProvider: the DownstreamRouteFinder, which I have already covered, but also DownstreamRouteCreator. I wonder what scenarios the creator is supposed to serve. Is it creating routes dynamically? I am not sure it applies to this feature. Can you elaborate, @raman-m? Cheers. 🙂

raman-m · 2023-07-06T14:54:39Z

@mrclayman commented on July 4, 2023

DownstreamRouteCreator is builder for the Ocelot.Configuration.DownstreamRoute object which contains complete info for a downstream route. It will be used in HttpClientBuilder.

DownstreamRouteFinder is used by DownstreamRouteProviderFactory class being looked up by IDownstreamRouteProvider interface. The provider (IDownstreamRouteProvider) is returned finally.
This quate complicated design is required because of Service Discovery feature. This is my assumption.
This is Tom's design! 😄

raman-m · 2023-07-20T12:02:21Z

@mrclayman commented on July 19:

Hm, for some reason, many of the changed files are coming up as "rewritten" even though I really only just added the bits I had implemented in the old branch, dammit.

Let me know if that is a problem. 😕 Right now I feel like redoing everything. Those diffs are pretty much useless.

Are you saying about this commit: mrclayman@022e817 ?
Or about this PR?

Anyway you cannot use diff of this PR because the feature branch is very old! Don't even try to cherry pick because you will get multiple merge conflicts! Just apply ideas and changes manually in Visual Studio.
You have to create new feature branch from updated develop one.

I see you've already updated develop branch by merging PR 1. But I forgot to ask to use Rebase option button. Sorry! It is always better to use fast-forwarding instead of merge commits.
And now develop branch is 1 commit ahead of ThreeMammals:develop.
Let's remove this redundant merge commit and pull top commits once again, please!
Use these instructions plz I've provided for J.Lukawska.

After fixing and updating your develop branch, you will be able to create new feature branch.
Let me know updating results please!

mrclayman · 2023-07-20T12:08:14Z

No, I have created a new branch based on the current state of develop, see here. I basically went over my old changes, picked them out and adapted them for the new state of the code base. It's a branch that might be ready for PR, but I am wondering about the diffs.

raman-m · 2023-07-20T13:36:54Z

And you will wonder more because you've created feature branch from develop branch which is in invalid state!

Develop should be synced to head repo develop always, and it should have zero diff! It is important!
Use these instructions plz to recover your develop!

mrclayman · 2023-07-20T14:29:06Z

I guess that is my exit cue, Raman. I don't see the reason to continue working on this since, as you have stated, there is already another PR in the pipeline ready to be merged soon. I haven't seen @jlukawska 's changes but if they are otherwise okay, trying to keep my version alive is a waste of effort as far as I can tell.

raman-m · 2023-07-22T11:49:53Z

🆗 As you wish...
Hope for your future contributions to Ocelot!

P.S. Could you at least to use these instructions please to recover your develop?

mrclayman · 2023-07-24T18:23:13Z

Fair enough, I guess. Do you want me to drop the topmost merge commit on my develop branch and the rebase the latest from origin on top of that? I am not entirely sure I understand the goal of those instructions.

raman-m · 2023-07-24T19:03:54Z

@mrclayman commented on July 24:

Fair enough, I guess. Do you want me to drop the topmost merge commit on my develop branch and the rebase the latest from origin on top of that? I am not entirely sure I understand the goal of those instructions.

With pleasure!
The manipulation with protected branch requires having the write permission (you have only as repo owner).
If you will add me as collaborator of your repo, I will try to fix manually.

The problem is that I have to push changes forcibly to develop to remove redundant merge commit.
As repo owner you can make force push operation always. But not me.
Currently, when I try to git push -f to develop branch the command fails because of lack of permissions.

If you don't want to give me Collaborator role then you can just remove develop branch from GitHub and Sync fork please? So, new develop branch will occur with all top commits!

raman-m · 2023-07-24T19:18:09Z

First of all disable protected branch policy for develop branch please!
After that maybe I will be able to fix the repo.

But it's better to add me as collaborator.

mrclayman · 2023-07-24T19:20:21Z

Just did that. 👍

raman-m · 2023-07-24T19:26:34Z

Congrats! 🥳 Your develop branch is canonic now (has zero diff with upstream)!
Updated!

raman-m · 2023-07-24T19:29:51Z

Will do the rebasing tomorrow! Too late today!

Do you ask to rebase https://github.com/mrclayman/Ocelot/tree/upstream-header-routing branch?

mrclayman · 2023-07-24T20:46:08Z

Thanks for the assist. 👍 I've put all of my changes into the upstream-header-routing-2023 branch after adapting them from the old branch this PR was based on.

raman-m · 2023-07-25T18:06:56Z

Comparing ThreeMammals:develop...mrclayman:upstream-header-routing-2023 · ThreeMammals/Ocelot
The diff looks quite strange cause I see a lot of the same lines which are highlighted.
For example, Lines 9-18 are identical but they are included in diff and highlighted...
This is happen because of merge commit and conflicts resolving.

Have you done rebasing or just copied the files into this branch?
Would you like to remove unnecessary highlighting? That will help to read the changes better and easily.
Are you going to rebase upstream-header-routing-2023 onto canonic develop branch?

mrclayman · 2023-07-26T18:54:32Z

What I did was create a new branch based on the current state of develop and just went over the changes I made in the old branch, picked them out and adapted for the new develop. I did not copy any files. I don't really understand why the diffs turned out like this, to be honest. Like I said earlier, I am just tempted to re-do everything from scratch.

mrclayman · 2023-07-26T20:10:03Z

I think the issue is the line endings. I mostly work in Linux and use system-dependent line ending setting in Rider. I'll see if switching to Windows-style LE solves this problem.

EDIT: Didn't help much. What kind of editor settings do you guys usually use?

raman-m · 2023-07-27T11:03:03Z

Absolutely right!
This is line endings problem!

In develop branch line endings settings are:

In feature branch the settings are:

So, it seems you've overridden Line Endings settings somehow, probably manually...
Or, your .gitattributes differs from repository settings, or...

Don't do that now and in future!
You have to use common solution settings: .editorconfig

I have a lack of management rights to provide complete git setup for this problem.
It seems the easiest way to fix that, you have to create new feature branch and ensure you use repo settings, and provide changes manually.

Other approaches like .editorconfig or .gitattributes or git add --renormalize will not help because files of the repo, most of them have MIXED state of line endings. 😢
This is the huge problem of repository now! So, I have to open issue in backlog now...

mrclayman · 2023-07-28T19:30:25Z

Well, actually your .editorconfig enforces LF endings, which is what Rider respects and then enforces on save. Seems like
VS or some other editor doesn't care about .editorconfig, or the endings would've been the same everywhere. Not sure how I wound up with CRLF endings, but it seems that for now, I am going to disable LF line ending enforcement to make Rider keep whatever is in the files. 😕

mrclayman · 2023-07-28T20:54:24Z

Okay, I have created a brand new PR #1684 . Take a look at it and let me know. The diffs are now much more workable. I am wondering about the support for placeholders that some people voiced their interest in us supporting. I might look into it if it's still worth the hassle.

The unit tests pass but I will try to find the time to thoroughly test the changes once more over the upcoming weekend so please hold off on merging if you otherwise find the changes acceptable.

raman-m · 2023-07-31T16:12:51Z

@mrclayman commented on July 28:

I am going to disable LF line ending enforcement to make Rider keep whatever is in the files. 😕

A-ha!
You cannot override repo settings! Otherwise we will see strange diff in PRs.

raman-m · 2023-07-31T16:19:50Z

@mrclayman commented on July 28:

Congrats! 🎉
And thanks for the PR! I've already replied in the PR.

so please hold off on merging if you otherwise find the changes acceptable.

No worries! I have no right to merge. 🤣 Tom has the right, only.
You/we have to pass and complete code review first.

Please write to PR discussion. I don't want to update this thread anymore...

raman-m · 2023-07-31T16:20:10Z

Duplicate of #1684

mrclayman added 6 commits July 13, 2019 13:48

Initial commit of new routing feature based on upstream HTTP header(s…

784034e

…). Tests are WIP.

Implemented UpstreamHeaderRoutingOptionsCreatorTests

d8359f4

Updated DownstreamRouteFinderTests

3d36722

Single return statement in RouteHasRequiredUpstreamHeaders

9d9b868

A few fixes and improvements

aff340d

* Fixed issue with request headers not getting converted to lowercase prior to matching * Made request value matching case-insensitive * Removed overriden Equals() from UpstreamRoutingHeaders * Made proper use of Shouldly in tests

Added upstream header-based routing paragraph to the routing docs

34087ae

mrclayman mentioned this pull request Sep 23, 2019

Routing based on Request Header #360

Closed

TomPallister closed this Jan 19, 2020

TomPallister reopened this Jan 19, 2020

TomPallister closed this Mar 11, 2020

raman-m assigned mrclayman Jun 28, 2023

raman-m marked this as a duplicate of #1684 Jul 31, 2023

ThreeMammals locked as too heated and limited conversation to collaborators Jul 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream header routing #964

Upstream header routing #964

mrclayman commented Jul 17, 2019 •

edited by raman-m

Loading

TomPallister commented Mar 11, 2020

raman-m commented Jun 28, 2023 •

edited

Loading

raman-m commented Jun 28, 2023 •

edited

Loading

mrclayman commented Jun 30, 2023

mrclayman commented Jul 4, 2023 •

edited

Loading

raman-m commented Jul 6, 2023 •

edited

Loading

raman-m commented Jul 20, 2023

mrclayman commented Jul 20, 2023 •

edited

Loading

raman-m commented Jul 20, 2023

mrclayman commented Jul 20, 2023

raman-m commented Jul 22, 2023

mrclayman commented Jul 24, 2023

raman-m commented Jul 24, 2023 •

edited

Loading

raman-m commented Jul 24, 2023 •

edited

Loading

mrclayman commented Jul 24, 2023

raman-m commented Jul 24, 2023 •

edited

Loading

raman-m commented Jul 24, 2023

mrclayman commented Jul 24, 2023

raman-m commented Jul 25, 2023

mrclayman commented Jul 26, 2023

mrclayman commented Jul 26, 2023 •

edited

Loading

raman-m commented Jul 27, 2023

mrclayman commented Jul 28, 2023 •

edited

Loading

mrclayman commented Jul 28, 2023 •

edited

Loading

raman-m commented Jul 31, 2023

raman-m commented Jul 31, 2023

raman-m commented Jul 31, 2023

Upstream header routing #964

Upstream header routing #964

Conversation

mrclayman commented Jul 17, 2019 • edited by raman-m Loading

Closes #360

Proposed Changes

TomPallister commented Mar 11, 2020

raman-m commented Jun 28, 2023 • edited Loading

Update 1

raman-m commented Jun 28, 2023 • edited Loading

mrclayman commented Jun 30, 2023

mrclayman commented Jul 4, 2023 • edited Loading

raman-m commented Jul 6, 2023 • edited Loading

raman-m commented Jul 20, 2023

mrclayman commented Jul 20, 2023 • edited Loading

raman-m commented Jul 20, 2023

mrclayman commented Jul 20, 2023

raman-m commented Jul 22, 2023

mrclayman commented Jul 24, 2023

raman-m commented Jul 24, 2023 • edited Loading

raman-m commented Jul 24, 2023 • edited Loading

mrclayman commented Jul 24, 2023

raman-m commented Jul 24, 2023 • edited Loading

raman-m commented Jul 24, 2023

mrclayman commented Jul 24, 2023

raman-m commented Jul 25, 2023

mrclayman commented Jul 26, 2023

mrclayman commented Jul 26, 2023 • edited Loading

raman-m commented Jul 27, 2023

mrclayman commented Jul 28, 2023 • edited Loading

mrclayman commented Jul 28, 2023 • edited Loading

raman-m commented Jul 31, 2023

raman-m commented Jul 31, 2023

raman-m commented Jul 31, 2023

mrclayman commented Jul 17, 2019 •

edited by raman-m

Loading

raman-m commented Jun 28, 2023 •

edited

Loading

raman-m commented Jun 28, 2023 •

edited

Loading

mrclayman commented Jul 4, 2023 •

edited

Loading

raman-m commented Jul 6, 2023 •

edited

Loading

mrclayman commented Jul 20, 2023 •

edited

Loading

raman-m commented Jul 24, 2023 •

edited

Loading

raman-m commented Jul 24, 2023 •

edited

Loading

raman-m commented Jul 24, 2023 •

edited

Loading

mrclayman commented Jul 26, 2023 •

edited

Loading

mrclayman commented Jul 28, 2023 •

edited

Loading

mrclayman commented Jul 28, 2023 •

edited

Loading