Upgrade to KubeClient v3 and log failed Kubernetes API requests #2266
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ | |
| using Ocelot.Logging; | ||
| using Ocelot.Provider.Kubernetes; | ||
| using Ocelot.Provider.Kubernetes.Interfaces; | ||
| using Ocelot.Values; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
|
|
@@ -19,6 +18,8 @@ namespace Ocelot.UnitTests.Kubernetes; | |
| /// </summary> | ||
| public class KubeTests : FileUnitTest | ||
| { | ||
| static JsonSerializerSettings JsonSerializerSettings => KubeClient.ResourceClients.KubeResourceClient.SerializerSettings; | ||
|
|
||
| private readonly Mock<IOcelotLoggerFactory> _factory; | ||
| private readonly Mock<IOcelotLogger> _logger; | ||
|
|
||
|
|
@@ -43,6 +44,7 @@ public async Task Should_return_service_from_k8s() | |
| given.ClientOptions.ApiEndPoint.ToString(), | ||
| given.ProviderOptions.KubeNamespace, | ||
| given.ProviderOptions.KeyOfServiceInK8s, | ||
| responseStatusCode: HttpStatusCode.OK, | ||
| endpoints, | ||
| out Lazy<string> receivedToken); | ||
|
|
||
|
|
@@ -54,6 +56,66 @@ public async Task Should_return_service_from_k8s() | |
| receivedToken.Value.ShouldBe($"Bearer {nameof(Should_return_service_from_k8s)}"); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(HttpStatusCode.BadRequest)] | ||
| [InlineData(HttpStatusCode.Forbidden)] | ||
| [InlineData(HttpStatusCode.InternalServerError)] | ||
| [InlineData(HttpStatusCode.NotFound)] | ||
| [Trait("PR", "2266")] | ||
| public async Task Should_not_return_service_from_k8s_when_k8s_api_returns_error_response(HttpStatusCode expectedStatusCode) | ||
| { | ||
| // Arrange | ||
| var given = GivenClientAndProvider(out var serviceBuilder); | ||
| serviceBuilder.Setup(x => x.BuildServices(It.IsAny<KubeRegistryConfiguration>(), It.IsAny<EndpointsV1>())) | ||
| .Returns(new Service[] { new(nameof(Should_not_return_service_from_k8s_when_k8s_api_returns_error_response), new("localhost", 80), string.Empty, string.Empty, Array.Empty<string>()) }); | ||
|
|
||
| var endpoints = GivenEndpoints(); | ||
| using var kubernetes = GivenThereIsAFakeKubeServiceDiscoveryProvider( | ||
| given.ClientOptions.ApiEndPoint.ToString(), | ||
| given.ProviderOptions.KubeNamespace, | ||
| given.ProviderOptions.KeyOfServiceInK8s, | ||
| expectedStatusCode, | ||
| endpoints, | ||
| out Lazy<string> receivedToken); | ||
|
|
||
| string expectedKubeApiErrorMessage = GetKubeApiErrorMessage(serviceName: given.ProviderOptions.KeyOfServiceInK8s, given.ProviderOptions.KubeNamespace, expectedStatusCode); | ||
| string expectedLogMessage = $"Failed to retrieve v1/Endpoints '{given.ProviderOptions.KeyOfServiceInK8s}' in namespace '{given.ProviderOptions.KubeNamespace}': (HTTP.{expectedStatusCode}/Failure/{expectedStatusCode}): {expectedKubeApiErrorMessage}"; | ||
| _logger.Setup(logger => logger.LogError(It.IsAny<Func<string>>(), It.IsAny<Exception>())) | ||
| .Callback((Func<string> messageFactory, Exception exception) => | ||
| { | ||
| messageFactory.ShouldNotBeNull(); | ||
|
|
||
| string logMessage = messageFactory(); | ||
| logMessage.ShouldNotBeNullOrWhiteSpace(); | ||
|
|
||
| // This is a little fragile, as it may change if other entries are logged due to implementation changes. | ||
|
Collaborator
Author
There was a problem hiding this comment. @raman-m small issue, as noted in these comments.
Member
There was a problem hiding this comment. Wow!... 😮
Collaborator
Author
There was a problem hiding this comment. 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 🙂
Member
There was a problem hiding this comment. 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. |
||
| // Unfortunately, the use of a factory delegate for the log message, combined with reuse of Kube's logger for Retry.OperationAsync makes this tricky to test any other way so this is probably the best we can do for now. | ||
| if (logMessage.StartsWith("Ocelot Retry strategy")) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| logMessage.ShouldBe(expectedLogMessage); | ||
|
|
||
| exception.ShouldNotBeNull(); | ||
| KubeApiException kubeApiException = exception.ShouldBeOfType<KubeApiException>(); | ||
| StatusV1 errorResponse = kubeApiException.Status; | ||
| errorResponse.Status.ShouldBe(StatusV1.FailureStatus); | ||
| errorResponse.Code.ShouldBe((int)expectedStatusCode); | ||
| errorResponse.Reason.ShouldBe(expectedStatusCode.ToString()); | ||
| errorResponse.Message.ShouldNotBeNullOrWhiteSpace(); | ||
| }) | ||
| .Verifiable($"IOcelotLogger.LogError() was not called."); | ||
|
|
||
| // Act | ||
| var services = await given.Provider.GetAsync(); | ||
|
|
||
| // Assert | ||
| services.ShouldNotBeNull().Count.ShouldBe(0); | ||
| receivedToken.Value.ShouldBe($"Bearer {nameof(Should_not_return_service_from_k8s_when_k8s_api_returns_error_response)}"); | ||
| _logger.Verify(); | ||
| } | ||
|
|
||
| [Fact] // This is not unit test! LoL :) This should be an integration test or even an acceptance test... | ||
| [Trait("Bug", "2110")] | ||
| public async Task Should_return_single_service_from_k8s_during_concurrent_calls() | ||
|
|
@@ -147,6 +209,12 @@ protected EndpointsV1 GivenEndpoints( | |
|
|
||
| protected IWebHost GivenThereIsAFakeKubeServiceDiscoveryProvider(string url, string namespaces, string serviceName, | ||
| EndpointsV1 endpointEntries, out Lazy<string> receivedToken) | ||
| { | ||
| return GivenThereIsAFakeKubeServiceDiscoveryProvider(url, namespaces, serviceName, HttpStatusCode.OK, endpointEntries, out receivedToken); | ||
| } | ||
|
|
||
| protected IWebHost GivenThereIsAFakeKubeServiceDiscoveryProvider(string url, string namespaces, string serviceName, | ||
| HttpStatusCode responseStatusCode, EndpointsV1 endpointEntries, out Lazy<string> receivedToken) | ||
| { | ||
| var token = string.Empty; | ||
| receivedToken = new(() => token); | ||
|
|
@@ -155,14 +223,33 @@ Task ProcessKubernetesRequest(HttpContext context) | |
| { | ||
| if (context.Request.Path.Value == $"/api/v1/namespaces/{namespaces}/endpoints/{serviceName}") | ||
| { | ||
| string responseBody; | ||
|
|
||
| if (context.Request.Headers.TryGetValue("Authorization", out var values)) | ||
| { | ||
| token = values.First(); | ||
| } | ||
|
|
||
| if (responseStatusCode == HttpStatusCode.OK) | ||
| { | ||
| responseBody = JsonConvert.SerializeObject(endpointEntries, JsonSerializerSettings); | ||
| } | ||
| else | ||
| { | ||
| responseBody = JsonConvert.SerializeObject(new StatusV1 | ||
| { | ||
| Message = GetKubeApiErrorMessage(serviceName, namespaces, responseStatusCode), | ||
| Reason = responseStatusCode.ToString(), | ||
| Code = (int)responseStatusCode, | ||
| Status = StatusV1.FailureStatus, | ||
|
|
||
| }, JsonSerializerSettings); | ||
| } | ||
|
|
||
| context.Response.StatusCode = (int)responseStatusCode; | ||
| context.Response.Headers.Append("Content-Type", "application/json"); | ||
|
|
||
| return context.Response.WriteAsync(responseBody); | ||
| } | ||
|
|
||
| return Task.CompletedTask; | ||
|
|
@@ -179,4 +266,9 @@ Task ProcessKubernetesRequest(HttpContext context) | |
| host.Start(); | ||
| return host; | ||
| } | ||
|
|
||
| private static string GetKubeApiErrorMessage(string serviceName, string kubeNamespace, HttpStatusCode responseStatusCode) | ||
| { | ||
| return $"Failed to retrieve v1/Endpoints '{serviceName}' in namespace '{kubeNamespace}' (HTTP.{responseStatusCode}/Failure/{responseStatusCode}): This is an error response for HTTP status code {(int)responseStatusCode} ('{responseStatusCode}') from the fake Kubernetes API."; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.