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

#1658 Adds global header transforms #1659

Open
wants to merge 11 commits into
base: release/24.0
Choose a base branch
from

Conversation

marklonquist
Copy link

@marklonquist marklonquist commented May 26, 2023

Closes #1658

Proposed Changes

  • new global header transforms
  • if route has same header transform key, the global one will not be used

@raman-m raman-m added feature A new feature medium effort Likely a few days of development effort waiting Waiting for answer to question or feedback from issue raiser labels May 29, 2023
@raman-m raman-m self-requested a review May 29, 2023 11:02
@raman-m raman-m changed the title Adds global header transforms #1658 Adds global header transforms Jun 1, 2023
@raman-m raman-m added in progress Someone is working on the issue. Could be someone on the team or off. and removed waiting Waiting for answer to question or feedback from issue raiser labels Jun 1, 2023
raman-m
raman-m previously approved these changes Jun 3, 2023
@raman-m
Copy link
Member

raman-m commented Jun 3, 2023

@marklonquist Thanks Mark! Good job!
I guess overriding global settings by Route local ones are covered by current unit tests, scenarios, right?

Additionally I've added commits to make code quality better.
And finally, Approved it!

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed in progress Someone is working on the issue. Could be someone on the team or off. labels Jun 3, 2023
@raman-m raman-m requested a review from TomPallister June 3, 2023 16:52
@TomPallister
Copy link
Member

TomPallister commented Jun 4, 2023

Thanks for the PR! Can you update the docs so people know how to use it?

@raman-m
Copy link
Member

raman-m commented Jun 20, 2023

@TomPallister Yes, we can!
Thank you for your advice!

@raman-m raman-m force-pushed the feature/1658---Global-UpstreamHeaderTransform-settings-in-GlobalConfiguration-section branch from 9a2c20c to a08f9f9 Compare September 8, 2023 15:07
@raman-m raman-m added needs feedback Issue is waiting on feedback before acceptance and removed medium effort Likely a few days of development effort accepted Bug or feature would be accepted as a PR or is being worked on labels Sep 8, 2023
@raman-m
Copy link
Member

raman-m commented Sep 8, 2023

@marklonquist Hey Mark!
The feature branch has been rebased onto ThreeMammals:develop successfully!
Welcome to code review!

@raman-m
Copy link
Member

raman-m commented Sep 8, 2023

@marklonquist

@TomPallister commented on Jun 4

According to the Tom's request we have to add documentation for this feature.
Do you have time to contribute?

@raman-m raman-m removed the request for review from TomPallister September 8, 2023 15:33
@marklonquist
Copy link
Author

very busy with work and familiy-life. I don't know exactly when I can have time to make docs for this.

@raman-m
Copy link
Member

raman-m commented Oct 30, 2023

@marklonquist commented on Oct 13

🆗 No worries! Take care about your family, and work & life balance!
Our team will return to this PR back after merging of 30+ open PRs with lower numbers... Or, I'll find a mentor...

@marklonquist
Copy link
Author

fixed

@marklonquist
Copy link
Author

I would appreciate it, if someone else can take over. I simply do not have the time to do any more work on this.

@marklonquist marklonquist removed their assignment Mar 2, 2024
@raman-m
Copy link
Member

raman-m commented Mar 2, 2024

@marklonquist
Don't worry! We will complete this feature and deliver it.
I guess the readiness is ~80-90%. I need update the docs and perform final code review.
I hope the PR will be merged in 1 or 2 months. But I'm not sure on release date.

I'm just curious:
Do you use Ocelot currently in any Production environment?

@marklonquist
Copy link
Author

marklonquist commented Mar 2, 2024

@raman-m yes! We use it on multiple BIG ecom sites, one of which is https://www.jackjones.com

I actually just finished setting up kubernetes service discovery, but it doesn't work out of the box. Had to change the IKubeApiClient to be a singleton, because it doesn't work correctly when it is scoped it seems.

Error Code: UnableToFindLoadBalancerError Message: unabe to find load balancer for /api/order/{everything}|Get,Put,Post,Delete,Head,Options| exception is System.InvalidOperationException: Cannot resolve scoped service 'KubeClient.IKubeApiClient' from root provider.

@raman-m
Copy link
Member

raman-m commented Mar 2, 2024

@marklonquist

I actually just finished setting up kubernetes service discovery, but it doesn't work out of the box. Had to change the IKubeApiClient to be a singleton, because it doesn't work correctly when it is scoped it seems.

Error Code: UnableToFindLoadBalancerError Message: unabe to find load balancer for /api/order/{everything}|Get,Put,Post,Delete,Head,Options| exception is System.InvalidOperationException: Cannot resolve scoped service 'KubeClient.IKubeApiClient' from root provider.

I believe it should be injected as singleton.
The design of K8s feature contains caching load balancing keys in memory.
With injected singleton K8s should work.

As you may have noticed, most of Ocelot features are singleton services, historically by Tom's design.

Please, keep us updated about your K8s environment with Ocelot behavior, how it is stable etc.

@raman-m
Copy link
Member

raman-m commented Mar 2, 2024

I will provide you more detailed feedback on design of the interface on Monday...

@marklonquist
Copy link
Author

marklonquist commented Mar 2, 2024

@marklonquist

I actually just finished setting up kubernetes service discovery, but it doesn't work out of the box. Had to change the IKubeApiClient to be a singleton, because it doesn't work correctly when it is scoped it seems.

Error Code: UnableToFindLoadBalancerError Message: unabe to find load balancer for /api/order/{everything}|Get,Put,Post,Delete,Head,Options| exception is System.InvalidOperationException: Cannot resolve scoped service 'KubeClient.IKubeApiClient' from root provider.

I believe it should be injected as singleton. The design of K8s feature contains caching load balancing keys in memory. With injected singleton K8s should work.

As you may have noticed, most of Ocelot features are singleton services, historically by Tom's design.

Please, keep us updated about your K8s environment with Ocelot behavior, how it is stable etc.

the kuberetes package adds it with AddKubeClient which adds it as scoped by default 😄

ServiceCollectionServiceExtensions.AddScoped<KubeApiClient>(services, new Func<IServiceProvider, KubeApiClient>(ResolveWithPodServiceAccount));
ServiceCollectionServiceExtensions.AddScoped<IKubeApiClient>(services, new Func<IServiceProvider, IKubeApiClient>(ResolveWithPodServiceAccount));

anyway this is outside if this PRs scope.

@raman-m raman-m changed the base branch from develop to release/24.0 March 5, 2024 09:52
@raman-m raman-m force-pushed the feature/1658---Global-UpstreamHeaderTransform-settings-in-GlobalConfiguration-section branch from 7c1b778 to a18b0e7 Compare April 7, 2024 13:54
@raman-m raman-m force-pushed the feature/1658---Global-UpstreamHeaderTransform-settings-in-GlobalConfiguration-section branch from a18b0e7 to 0e27766 Compare April 16, 2024 18:49
@raman-m
Copy link
Member

raman-m commented Apr 16, 2024

The feature branch has been rebased onto release/24.0 with top commit 59b63ea !

@marklonquist Mark,
Could you review and fix failed tests please❓

@raman-m raman-m force-pushed the feature/1658---Global-UpstreamHeaderTransform-settings-in-GlobalConfiguration-section branch from 0e27766 to 2b6ad65 Compare April 19, 2024 17:58
@raman-m
Copy link
Member

raman-m commented May 13, 2024

TODOs

  • Clean up the feature branch from redundant commits related to QoS feature (aka Polly related ones)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2023 Annual 2023 release feature A new feature Headers Transformation Ocelot feature: Headers Transformation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global UpstreamHeaderTransform settings in GlobalConfiguration section
5 participants