Upgrade to KubeClient v3 and log failed Kubernetes API requests #2266

tintoy · 2025-03-15T03:09:22Z

KubeClient v3 should simplify dependency-management, from Ocelot's perspective:

KubeClient no longer has an external dependency (HTTPlease) for its HttpClient support. The internalised version of this support is now located in the KubeClient.Http namespace.
KubeClient v3 is multi-targeted for net9.0, as well as a couple of older versions that some consumers are still using (support for these older versions will be tailed off over the next couple of minor releases)

Related discussion at:

tintoy/dotnet-kube-client#166
tintoy/dotnet-kube-client#167

CC: @raman-m (as previously discussed; sorry for the extended delay!)

Proposed Changes

Update KubeClient packages to v3.0.0
Add support for logging detailed error responses from the Kubernetes API, in case of failure (only enabled at the Debug log level, or higher)

KubeClient v3 should simplify dependency-management, from Ocelot's perspective: * KubeClient no longer has an external dependency (HTTPlease) for its HttpClient support. The internalised version of this support is now located in the KubeClient.Http namespace. * KubeClient v3 is multi-targeted for net9.0, as well as a couple of older versions that some consumers are still using (support for these older versions will be tailed off over the next couple of minor releases) Related discussion at: tintoy/dotnet-kube-client#166 tintoy/dotnet-kube-client#167

…it isn't going to be used

raman-m

Adam,
Thanks for opening this PR!
Overall, it is satisfactory, but the logging could be improved.
Another concern is the absence of tests, despite the fact that the current tests are passing.

P.S.

I plan to include it in the current .NET 9 release, so we need to merge this PR within a few days.

src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs

raman-m · 2025-03-15T09:39:35Z

src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs

-            : null;
+        if (!response.IsSuccessStatusCode)
+        {
+            if (_logger.IsEnabled(LogLevel.Debug))


I am not certain, but if we use the native Ocelot logger, then all Log* methods have internal check to determine whether logging is enabled. Therefore, this if clause may be redundant.

Yeah, ILogger does the same, but this check was more to decide if we want to check whether to try deserialising the error response; it’s a pattern I tend to use when we perform some non-zero-cost operation to collect info purely for logging purposes. Some rationale from the runtime folks here:

dotnet/runtime#45290 (comment)

Having said that, as designed, this method swallows any error and returns null (logging being the only place errors are surfaced) so perhaps a warning is more appropriate? just didn’t want to fill up the log with noise… 🤷🏻

I've moved that logic out of EndPointClientV1, to maintain separation of concerns; it now lives in the caller (Kube).

raman-m · 2025-03-15T09:44:41Z

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

@@ -25,8 +25,8 @@
    <NoWarn>1591</NoWarn>
  </PropertyGroup>
  <ItemGroup>
-    <PackageReference Include="KubeClient" Version="2.5.12" />
-    <PackageReference Include="KubeClient.Extensions.DependencyInjection" Version="2.5.12" />
+    <PackageReference Include="KubeClient" Version="3.0.0" />


Actually, it should be 3.0.1 😄

Yeah, fair call - I'd only just published that; will pull in the new version 🙂

Hmm - just before I push this change, I thought I'd better check - KubeClient 3.0.1 requires Microsoft.Extensions.* 9.0.3 (Ocelot's test projects currently use 9.0.1; tried upgrading, and all are tests still passing, but it's your call if you want me to upgrade).

Please go ahead and upgrade. Anyway, before the release I'll upgrade all packs

tintoy · 2025-03-15T23:09:05Z

Another concern is the absence of tests, despite the fact that the current tests are passing.

Yes, I think I can probably whip up a couple of integration tests for this, if nothing else (KubeClient has a testability framework that can be used to mock out the Kubernetes API for tests using a custom HttpMessageHandler or a TestServer so no actual sockets required).

For example:

https://github.com/tintoy/dotnet-kube-client/blob/ee1b9d29ff65b212b3b6c2da47fce1e7ebe3abb4/test/KubeClient.Tests/ErrorHandling/ExceptionTests.cs#L39
https://github.com/tintoy/dotnet-kube-client/blob/ee1b9d29ff65b212b3b6c2da47fce1e7ebe3abb4/test/KubeClient.Extensions.DataProtection.Tests/KeyPersistenceTests.cs#L57

I’ll try to get the tests done sometime today, if I can (it’s Sunday here, and next week is already starting to look pretty heavy).

…and into its caller

src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs

tintoy · 2025-03-16T00:34:22Z

src/Ocelot.Provider.Kubernetes/Kube.cs

-    private Task<EndpointsV1> GetEndpoint() => _kubeApi
-        .ResourceClient<IEndPointClient>(client => new EndPointClientV1(client))
-        .GetAsync(_configuration.KeyOfServiceInK8s, _configuration.KubeNamespace);
+    private async Task<EndpointsV1> GetEndpoint()


I've moved this logic here because it keeps the resource-client implementation cleaner (since it wouldn't otherwise be clear to someone who's not seen this code before what the lifetime of the resource client is, vs the lifetime of the containing type or its logger).

tintoy · 2025-03-16T00:35:44Z

@raman-m I've reworked the logic a bit (see comments on the commit). Can you let me know if you're ok with those changes? If so, I can get started on the tests.

tintoy · 2025-03-16T01:18:23Z

Regarding the tests - although there are existing tests for the Kubernetes provider, and I can see how to extend them to test handling of error responses, I'm not confident I can test the logging behaviour without risking reinventing the wheel (I see a couple of of internal-only/test-specific implementations of IOcelotLogger scattered across the test codebase, and I'm not sure I have time to consolidate them). I can probably create a stub implementation for the existing tests but I'd definitely prefer to keep the additional complexity to a minimum if you're wanting to release in the next couple of days.

tintoy · 2025-03-16T01:22:16Z

KubeClient has a testability framework that can be used to mock out the Kubernetes API for tests using a custom HttpMessageHandler or a TestServer so no actual sockets required

Ah, good - I see there's already a framework for serving up a fake Kubernetes API; will use that, instead.

…I used by service-discovery tests

raman-m · 2025-03-16T10:07:07Z

@raman-m I've reworked the logic a bit (see comments on the commit). Can you let me know if you're ok with those changes? If so, I can get started on the tests.

Please proceed. It is to upgrade the tests in the unit testing project: Kubernetes.
Additionally, I believe that extensive upgrades to the acceptance tests are not required, as they utilize a testing server. Therefore, I require the upgrading and testing of new internal implementation through unit tests.

Regarding the tests - although there are existing tests for the Kubernetes provider, and I can see how to extend them to test handling of error responses, I'm not confident I can test the logging behaviour without risking reinventing the wheel (I see a couple of of internal-only/test-specific implementations of IOcelotLogger scattered across the test codebase, and I'm not sure I have time to consolidate them). I can probably create a stub implementation for the existing tests but I'd definitely prefer to keep the additional complexity to a minimum if you're wanting to release in the next couple of days.

Please update unit tests only. There is no need to rewrite or refactor acceptance tests. They should be solid, meaning they require minimal updating if they fail. Therefore, you need to test all new implementations with unit tests.

Ah, good - I see there's already a framework for serving up a fake Kubernetes API; will use that, instead.

Indeed, we utilize Microsoft.AspNetCore.TestHost for acceptance testing, we generate actual requests directed at the test server endpoints.
FYI I have no objections if you use the native framework you proposed for Kube objects, but only for integration/unit testing.

raman-m · 2025-03-16T10:15:47Z

Adam FYI, you probably saw PR #2257 and commit 4b6b96a
I am afraid we will need to update the K8s feature doc well. However, I will reconsider this after the Dev Complete stage.

tintoy · 2025-03-16T21:56:14Z

Therefore, I require the upgrading and testing of new internal implementation through unit tests.

Just to confirm - by this, do you mean unit tests verifying the underlying implementation (using Moq or similar) or do you mean unit tests for the behaviour changes resulting from the implementation change?

…netes API used by service-discovery tests" This reverts commit 9a8bf9c.

tintoy · 2025-03-16T22:42:01Z

Just to confirm - by this, do you mean unit tests verifying the underlying implementation (using Moq or similar) or do you mean unit tests for the behaviour changes resulting from the implementation change?

Never mind - I see the tests you're referring to; I'll extend those.

tintoy · 2025-03-16T23:59:14Z

test/Ocelot.UnitTests/Kubernetes/KubeTests.cs

+                string logMessage = messageFactory();
+                logMessage.ShouldNotBeNullOrWhiteSpace();
+
+                // This is a little fragile, as it may change if other entries are logged due to implementation changes.


@raman-m small issue, as noted in these comments.

Wow!... 😮
I don't understand why it is difficult to test. Can't we set up the mocker accordingly?

The problem (as it looks to me) is that the test can't know for sure what other components may log messages as well (in this case, the retry facility uses the same logger and trying to verify those logged messages would make the whole thing too fragile by making this unit test coupled to the implementation of other components such as the retry).

I've always aimed to make unit tests, in particular, to test behaviour without being coupled to the implementation; otherwise the tests may have to be changed each time the implementation changes which removes one of the (in my opinion) best benefits of unit testing.

But I'm not that familiar with the code-base and so I'm open to changing it if you believe that would be better 🙂

So, the issue lies in the retry logic of the upper context, correct? It seems we have been debating this extensively. If the problem truly exists, I will identify it in the next round of refactoring the logic. For now, we can omit this.

tintoy · 2025-03-17T00:07:38Z

Adam FYI, you probably saw PR #2257 and commit 4b6b96a I am afraid we will need to update the K8s feature doc well.

No worries, happy to discuss / fill in details if required.

Potentially useful KubeClient docs for registering IKubeApiClient / KubeClientOptions:

https://github.com/tintoy/dotnet-kube-client/?tab=readme-ov-file#make-the-client-available-for-dependency-injection

raman-m · 2025-03-17T10:08:27Z

@tintoy commented on March 17

No worries, happy to discuss / fill in details if required.

Potentially useful KubeClient docs for registering IKubeApiClient / KubeClientOptions:

https://github.com/tintoy/dotnet-kube-client/?tab=readme-ov-file#make-the-client-available-for-dependency-injection

No, I meant: How will your changes be reflected in the current K8s doc for the development version? Please note that the development version is still unreleased. Are these documents relevant? Please review them.

P. S. Updating the documentation will be the final step. For now, I will conduct the final code review today by loading the solution into Visual Studio.

tintoy · 2025-03-17T10:32:58Z

No, I meant: How will your changes be reflected in the current K8s doc for the development version? Please note that the development version is still unreleased. Are these documents relevant? Please review them

Oh - sure, will do.

raman-m

I believe the development is complete. The final step is review the documentation.

test/Ocelot.UnitTests/Kubernetes/KubeTests.cs

src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs

src/Ocelot.Provider.Kubernetes/Kube.cs

…ce it only has a single await as the last statement (return await)

into feature/kubeclient-v3

tintoy · 2025-03-17T23:53:46Z

Regarding the documentation, it looks to me like no changes are required as a result of this PR's changes.

Later, however, it might be worth adding some information to the docs about KubeClient's support for:

For example, the code from step 2:

Action<KubeClientOptions> configureKubeClient = opts =>
 {
     opts.ApiEndPoint = new UriBuilder("https", "my-host", 443).Uri;
     opts.AccessToken = "my-token";
     opts.AuthStrategy = KubeAuthStrategy.BearerToken;
     opts.AllowInsecure = true;
 };
 builder.Services
     .AddOptions<KubeClientOptions>()
     .Configure(configureKubeClient); // manual binding options via IOptions<KubeClientOptions>

Could also be written like this:

services.AddKubeClientOptions(clientOptions =>
{
    clientOptions.ApiEndPoint = new UriBuilder("https", "my-host", 443).Uri;
    clientOptions.AuthStrategy = KubeAuthStrategy.BearerToken;
    clientOptions.AccessToken = "my-token";
    clientOptions.AllowInsecure = true;
});

raman-m · 2025-03-18T17:29:02Z

@tintoy commented on March 18

Regarding the documentation, it looks to me like no changes are required as a result of this PR's changes.

Great! If no changes needed when upgrading to v3 version then I have a plan to release current docs with this extra method:

AddKubernetes(Action<KubeClientOptions>) method (doc in develop)

Later, however, it might be worth adding some information to the docs about KubeClient's support for:

Dependency-injection

~/.kube/config

For example, the code from step 2:
XXX

Could also be written like this:
YYY

Good suggestion. However, the Ocelot documentation recommends adding Kubernetes support using the native Ocelot AddKubernetes method with two overloaded versions (see docs). Therefore, as a team, we encapsulate all native client settings and pass only high-level required options to the internal setup.

Switching to services.AddKubeClientOptions method will provide no benefits due to the internally encapsulated setup of the integrated client. Another point regarding Step 2 is explaining to users how it works in the case of manual setup. However, currently we do not draw users' attention to the use of the native helper with the Action-delegate argument. Therefore, I will explain this briefly in Note 2 of the doc...

raman-m

Ready for delivery ✔️

@tintoy Congrats! 🎉

tintoy added 2 commits March 15, 2025 14:01

Don't attempt to deserialise response payload from Kubernetes API if …

fd98410

…it isn't going to be used

raman-m requested changes Mar 15, 2025

View reviewed changes

raman-m assigned tintoy Mar 15, 2025

raman-m added proposal Proposal for a new functionality in Ocelot Service Discovery Ocelot feature: Service Discovery Kubernetes Service discovery by Kubernetes NET9 .NET 9 release labels Mar 15, 2025

raman-m added this to the .NET 9 milestone Mar 15, 2025

raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label Mar 15, 2025

Simplify logging implementation by moving it out of EndPointClientV1 …

4b5a6ef

…and into its caller

tintoy commented Mar 16, 2025

View reviewed changes

src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs Show resolved Hide resolved

tintoy commented Mar 16, 2025

View reviewed changes

Add support for returning error responses from the fake Kubernetes AP…

9a8bf9c

…I used by service-discovery tests

tintoy added 3 commits March 17, 2025 09:39

Revert "Add support for returning error responses from the fake Kuber…

8472f4a

…netes API used by service-discovery tests" This reverts commit 9a8bf9c.

Upgrade KubeClient to v3.0.1

8a4faa3

Use consistent serialisation settings for fake Kubernetes API responses

6c1ac8a

Add unit tests for handling of error responses from Kubernetes API

6a6c374

tintoy commented Mar 16, 2025

View reviewed changes

raman-m requested a review from RaynaldM March 17, 2025 10:09

raman-m requested a review from ggnaegi March 17, 2025 10:09

code review by @raman-m

c853a6f

raman-m approved these changes Mar 17, 2025

View reviewed changes

test/Ocelot.UnitTests/Kubernetes/KubeTests.cs Outdated Show resolved Hide resolved

src/Ocelot.Provider.Kubernetes/EndPointClientV1.cs Outdated Show resolved Hide resolved

src/Ocelot.Provider.Kubernetes/Kube.cs Outdated Show resolved Hide resolved

tintoy added 2 commits March 18, 2025 10:39

Change EndPointClientV1.GetAsync into a non-async wrapper method, sin…

9b4e5cd

…ce it only has a single await as the last statement (return await)

Merge branch 'feature/kubeclient-v3' of https://github.com/tintoy/Ocelot

f083fda

into feature/kubeclient-v3

Update docs with user's instructions

71c36e4

raman-m approved these changes Mar 18, 2025

View reviewed changes

raman-m merged commit a8ba984 into ThreeMammals:develop Mar 18, 2025
1 check passed

raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Mar 18, 2025

raman-m mentioned this pull request Apr 13, 2025

Release v24.0 | Upgrade to net9.0 TFM | .NET 9 release | +semver: breaking #2286

Merged

Upgrade to KubeClient v3 and log failed Kubernetes API requests #2266

Upgrade to KubeClient v3 and log failed Kubernetes API requests #2266

Uh oh!

Conversation

tintoy commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

raman-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

P.S.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tintoy commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tintoy commented Mar 16, 2025

Uh oh!

tintoy commented Mar 16, 2025

Uh oh!

tintoy commented Mar 16, 2025

Uh oh!

raman-m commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raman-m commented Mar 16, 2025

Uh oh!

tintoy commented Mar 16, 2025

Uh oh!

tintoy commented Mar 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tintoy commented Mar 17, 2025

Uh oh!

raman-m commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tintoy commented Mar 17, 2025

Uh oh!

raman-m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tintoy commented Mar 17, 2025

Uh oh!

raman-m commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raman-m left a comment

tintoy commented Mar 15, 2025 •

edited

Loading

raman-m left a comment •

edited

Loading

tintoy commented Mar 15, 2025 •

edited

Loading

raman-m commented Mar 16, 2025 •

edited

Loading

raman-m Mar 17, 2025 •

edited

Loading

raman-m commented Mar 17, 2025 •

edited

Loading

raman-m commented Mar 18, 2025 •

edited

Loading