#977 Enable validation of DI scopes in `Kube` and `PollKube` discovery providers #2180

kick2nick · 2024-10-21T20:27:58Z

Fixes #977

Cannot resolve scoped service KubeClient.IKubeApiClient from root provider #977

Proposed Changes

Added unit tests for verifying that Kube & PollKube providers can be resolved from DI with enabled scope validation.
Fixed Kubernetes acceptance tests. Used registered by Ocelot extension KubeApiClient instead of manually created one. Enabled scope validation for ServiceProvider
Made IKubeApiClient singleton lifetime explicit.

Successor

#2168 WatchKube discovery provider #2174

Docs

Best practices: Dependency injection in ASP.NET Core

kick2nick · 2024-10-21T20:29:35Z

@raman-m,
please see #977 (comment)

raman-m

I'm not as confident as you; the proposed changes seem to be breaking ones.
There's no acceptance test for the original user scenario in bug Cannot resolve scoped service KubeClient.IKubeApiClient from root provider #977.
In BDD, it's quite clear that we should test it first before any actual refactoring.
Why were so many changes proposed for a minor scope validation change? I don't understand.

I'm convinced that scope validation can be implemented with less code.

src/Ocelot.Provider.Kubernetes/Ocelot.Provider.Kubernetes.csproj

raman-m · 2024-10-22T09:26:53Z

src/Ocelot.Provider.Kubernetes/OcelotBuilderExtensions.cs

@@ -9,10 +11,23 @@ public static class OcelotBuilderExtensions
    public static IOcelotBuilder AddKubernetes(this IOcelotBuilder builder, bool usePodServiceAccount = true)
    {
        builder.Services
-            .AddKubeClient(usePodServiceAccount)
+            .AddSingleton<IKubeApiClient, KubeApiClient>(KubeApiClientFactory)


It appears to be a breaking change 🤯 Lovely singletons once more❗
The main purpose of the Ocelot.Provider.Kubernetes NuGet package is to facilitate the integration of the KubeClient package with Ocelot. The proposed changes undermine this objective by removing AddKubeClient. This design direction is flawed because our solutions should be developed based on the official KubeClient package's functionality, not by altering the library's behavior to suit our needs!
You have to understand that the issue is not AddKubeClient extension, but singleton factory aka KubernetesProviderFactory.Get with bad scope management!

It appears to be a breaking change 🤯

Why is it a breaking change?
✔️ Public API hasn't changed
✔️ IKubeApiClient has the same singleton lifetime as it had before this fix(implicitly promoted)
Proposed change - make IKubeApiClient singleton lifetime explicit by registering it as singleton.

our solutions should be developed based on the official KubeClient package's functionality, not by altering the library's behavior to suit our needs!

KubeClient package's functionality - talking to kube api as httpclient wrapper. I don't understand why it's lifetime can't be managed by Ocelot.Provider.Kubernetes. (like it was done for consul).

src/Ocelot.Provider.Kubernetes/OcelotBuilderExtensions.cs

test/Ocelot.AcceptanceTests/ServiceDiscovery/KubernetesServiceDiscoveryTests.cs

test/Ocelot.UnitTests/Kubernetes/KubernetesProviderFactoryTests.cs

test/Ocelot.AcceptanceTests/ServiceDiscovery/KubernetesServiceDiscoveryTests.cs

raman-m · 2024-10-22T10:37:44Z

@kick2nick commented on Oct 21

Initially, I was uncertain if the provider.CreateScope() approach was ideal for our needs until I saw the HttpContext.RequestServices in PR #2179. To be honest, I'm not concerned with Sergio's solutions as they addressed issues in an older version of Ocelot, which may not be applicable to the current versions. We should consider your suggestions only if there is a consensus within the team.

I have some doubts about this approach:

HttpClient MessageHandlers won't be reused - KubeClient creates new one for each KubeApiClient instance, that can cause socket exhaustion issues on heavy loads.

Excellent! How did you validate this hypothesis? Theoretical assumptions differ from empirical testing. Additionally, as previously mentioned in this message, we will pursue alternative solutions, setting aside my provider.CreateScope() design recommendation, as it appears there are superior design options based on HttpContext.

This scope is separate from request HttpContext scope and no services would be potentially reused.

Using HttpContext.RequestServices seems to be the appropriate method, though the consequences for the PollKube provider are still unclear to me.

There is no option to use long polling with this approach, HttpClient will be disposed with scope
So my opinion is better leave KubeApiClient lifetime as is (it's now promoted to singleton), but make it explicit by registering it as singleton.

If it's not implemented, why are we discussing it? You're correct to assume that we might encounter issues with disposed objects. Couldn't this be managed by HttpContext? I'm quite confident that using HttpContext.RequestServices would prevent any disposed objects, eliminating the need to manage scope lifetime.

But if you insist on Sergio's approach than no problem, I'll implement it :)

Please disregard the previous approach for Sergio; we will devise an alternative solution. I find these debates to be unproductive.

test/Ocelot.UnitTests/Kubernetes/KubernetesProviderFactoryTests.cs

kick2nick · 2024-10-22T21:12:54Z

until I saw the HttpContext.RequestServices in PR #2179.

There you are resolving IConsulServiceBuilder once from the scope of the first http request to downstream service and then cache it along with loadBalancer. IConsulServiceBuilder is not IDisposable and it works (but you don't have truly scoped IConsulServiceBuilder ).
The MS doc states →

The framework creates a scope per request, and RequestServices exposes the scoped service provider. All scoped services are valid for as long as the request is active.

Using HttpContext.RequestServices seems to be the appropriate method, though the consequences for the PollKube provider are still unclear to me.

Usual Kube won't work if IKubeApiClientFactory is resolved from HttpContext.RequestServices inside KubernetesProviderFactory.Get because HttpClient will be disposed(like in example). We have another option - inject IHttpContextAccessor into Kube and on each request resolve IKubeApiClient, but for PollKube it won't work (it calls Kube on timer ticks without HttpContext).

Excellent! How did you validate this hypothesis? Theoretical assumptions differ from empirical testing.

There is whole concept - HttpClientFactory that adresses this issues. Please read first 2 paragraphs.

raman-m · 2024-10-23T07:09:14Z

What's up, man? Are you online?

kick2nick · 2024-10-23T08:27:39Z

What's up, man? Are you online?

Yes, I'm here. Fixed review comments, pointed why HttpContext.RequestServices can't be used. Anything else?

kick2nick · 2024-10-23T10:04:55Z

Couldn't this be managed by HttpContext? I'm quite confident that using HttpContext.RequestServices would prevent any disposed objects, eliminating the need to manage scope lifetime.

No! Because IKubeApiClient will be resolved once, stored inside Kube singleton, and then disposed after HttpContext gone. Provided test as proof.
kick2nick@bc2f7e6
results:

raman-m · 2024-10-23T14:05:43Z

Yes, I'm here. Fixed review comments, pointed why HttpContext.RequestServices can't be used.

Well done, my astute developer! I concur, HttpContext.RequestServices is not suitable for Kube, and your evidence supports this. I'm weary of these debates as well. It seems the upcoming debate session will be unproductive.

Why suggest refactoring everything for a minor issue that throws an InvalidOperationException with the message Cannot resolve scoped service 'KubeClient.IKubeApiClient' from root provider?

I've asked you:

I'm convinced that scope validation can be implemented with less code.

You didn't respond but instead advocated for your ideal solution, which I believe is commendable and, in my view, should have been implemented 3-5 years ago, utilizing a design based on singleton factories. Writing less code is possible by creating a fake scope and using it during the operation of reading the list of services, as suggested here, but sure you didn't like it. This approach is quite straightforward and minimalistic. However, sometimes proposing ideal solutions can be a waste of time. As I have previously mentioned, both Consul and Kube classes will be transitioned to DI converting them to IServiceDiscoveryProvider objects, and the ServiceDiscoveryFinderDelegate will be eliminated forever.

Anything else?

Code review has done.
Please halt any further development at this point; I will proceed with the final review of the PR...

raman-m · 2024-10-24T10:08:42Z

Successor

#2168 WatchKube discovery provider #2174

raman-m

Ready for delivery ✅

raman-m · 2024-10-24T17:51:27Z

@kick2nick Nikolay,
Thank you for your contribution! 💪

* OcelotBuilderExtensionsTests for Eureka SD * BuildServiceProvider(true) #2179 (comment) * Add TestHostBuilder with enabled scopes validation by default across all testing projects #2179 (comment) * Update Sample projects with force scopes validation. Add Ocelot.Samples.Web project. Add OcelotHostBuilder and DownstreamHostBuilder helpers. Add Ocelot.Samples.Web project reference to all samples projects. TargetFramework is `net8.0` only.

Fixed Kube/PollKube service provider scope validation

faaa627

kick2nick added 2 commits October 21, 2024 23:54

fix pollkube tests

ec1e379

remove BDDfy

d23a06a

raman-m assigned raman-m and kick2nick Oct 22, 2024

raman-m added Service Discovery Ocelot feature: Service Discovery hotfix Gitflow: Hotfix issue, PR related to hotfix branch Kubernetes Service discovery by Kubernetes Oct'24 October 2024 release labels Oct 22, 2024

raman-m added this to the October'24 milestone Oct 22, 2024

raman-m changed the title ~~#977 Fixed Kube/PollKube service provider scope validation~~ #977 Enable validation of DI scopes in Kube and PollKube service providers Oct 22, 2024

raman-m requested changes Oct 22, 2024

View reviewed changes

raman-m requested review from RaynaldM and ggnaegi October 22, 2024 09:58

raman-m reviewed Oct 22, 2024

View reviewed changes

test/Ocelot.UnitTests/Kubernetes/KubernetesProviderFactoryTests.cs Show resolved Hide resolved

kick2nick and others added 2 commits October 23, 2024 00:41

Fix tests

69ed946

Merge branch 'develop' into fix/977-kube-provider-scopes-validation

800e4fe

raman-m changed the title ~~#977 Enable validation of DI scopes in Kube and PollKube service providers~~ #977 Enable validation of DI scopes in Kube and PollKube discovery providers Oct 23, 2024

raman-m mentioned this pull request Oct 24, 2024

#2168 WatchKube discovery provider #2174

Open

raman-m added 3 commits October 24, 2024 13:25

Bump KubeClient to 2.5.12

b8cd234

Code review by @raman-m

73d3c8f

no Chinese tests now!

a4216f2

raman-m added 2 commits October 24, 2024 20:18

Cover by unit tests

1bb4996

Validate DI scopes in acceptance tests

eadc2b6

raman-m approved these changes Oct 24, 2024

View reviewed changes

raman-m merged commit 8b633af into ThreeMammals:develop Oct 24, 2024
1 check passed

raman-m mentioned this pull request Oct 26, 2024

Release 23.3.6 | v23.3.4 Hotfixes | October'24 release | +semver: patch #2185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#977 Enable validation of DI scopes in `Kube` and `PollKube` discovery providers #2180

#977 Enable validation of DI scopes in `Kube` and `PollKube` discovery providers #2180

kick2nick commented Oct 21, 2024 •

edited by raman-m

Loading

kick2nick commented Oct 21, 2024

raman-m left a comment

raman-m Oct 22, 2024

kick2nick Oct 22, 2024 •

edited

Loading

raman-m commented Oct 22, 2024

kick2nick commented Oct 22, 2024 •

edited by raman-m

Loading

raman-m commented Oct 23, 2024

kick2nick commented Oct 23, 2024

kick2nick commented Oct 23, 2024

raman-m commented Oct 23, 2024

raman-m commented Oct 24, 2024

raman-m left a comment

raman-m commented Oct 24, 2024

#977 Enable validation of DI scopes in Kube and PollKube discovery providers #2180

#977 Enable validation of DI scopes in Kube and PollKube discovery providers #2180

Conversation

kick2nick commented Oct 21, 2024 • edited by raman-m Loading

Fixes #977

Proposed Changes

Successor

Docs

kick2nick commented Oct 21, 2024

raman-m left a comment

Choose a reason for hiding this comment

raman-m Oct 22, 2024

Choose a reason for hiding this comment

kick2nick Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m commented Oct 22, 2024

kick2nick commented Oct 22, 2024 • edited by raman-m Loading

raman-m commented Oct 23, 2024

kick2nick commented Oct 23, 2024

kick2nick commented Oct 23, 2024

raman-m commented Oct 23, 2024

raman-m commented Oct 24, 2024

Successor

raman-m left a comment

Choose a reason for hiding this comment

Ready for delivery ✅

raman-m commented Oct 24, 2024

#977 Enable validation of DI scopes in `Kube` and `PollKube` discovery providers #2180

#977 Enable validation of DI scopes in `Kube` and `PollKube` discovery providers #2180

kick2nick commented Oct 21, 2024 •

edited by raman-m

Loading

kick2nick Oct 22, 2024 •

edited

Loading

kick2nick commented Oct 22, 2024 •

edited by raman-m

Loading