#977 Enable validation of DI scopes in Kube and PollKube discover…

…y providers (#2180) * Fixed Kube/PollKube service provider scope validation * fix pollkube tests * remove BDDfy * Fix tests * Bump KubeClient to 2.5.12 * Code review by @raman-m * no Chinese tests now! * Cover by unit tests * Validate DI scopes in acceptance tests --------- Co-authored-by: kick2nick <[email protected]> Co-authored-by: Raman Maksimchuk <[email protected]>
ThreeMammals · Oct 24, 2024 · 8b633af · 8b633af
1 parent 1cf5597
commit 8b633af
Show file tree

Hide file tree

Showing 10 changed files with 196 additions and 85 deletions.
diff --git a/src/Ocelot.Provider.Kubernetes/Kube.cs b/src/Ocelot.Provider.Kubernetes/Kube.cs
@@ -47,7 +47,7 @@ public virtual async Task<List<Service>> GetAsync()
     }
 
     private Task<EndpointsV1> GetEndpoint() => _kubeApi
-        .ResourceClient(client => new EndPointClientV1(client))
+        .ResourceClient<IEndPointClient>(client => new EndPointClientV1(client))
         .GetAsync(_configuration.KeyOfServiceInK8s, _configuration.KubeNamespace);
 
     private bool CheckErroneousState(EndpointsV1 endpoint)

diff --git a/src/Ocelot.Provider.Kubernetes/KubeServiceCreator.cs b/src/Ocelot.Provider.Kubernetes/KubeServiceCreator.cs
@@ -47,7 +47,7 @@ protected virtual ServiceHostAndPort GetServiceHostAndPort(KubeRegistryConfigura
         portV1 ??= new();
         portV1.Name ??= configuration.Scheme ?? string.Empty;
         Logger.LogDebug(() => $"K8s service with key '{configuration.KeyOfServiceInK8s}' and address {address.Ip}; Detected port is {portV1.Name}:{portV1.Port}. Total {ports.Count} ports of [{string.Join(',', ports.Select(p => p.Name))}].");
-        return new ServiceHostAndPort(address.Ip, portV1.Port, portV1.Name);
+        return new ServiceHostAndPort(address.Ip, portV1.Port ?? 80, portV1.Name);
     }
 
     protected virtual string GetServiceId(KubeRegistryConfiguration configuration, EndpointsV1 endpoint, EndpointSubsetV1 subset, EndpointAddressV1 address)

diff --git a/src/Ocelot.Provider.Kubernetes/KubernetesProviderFactory.cs b/src/Ocelot.Provider.Kubernetes/KubernetesProviderFactory.cs
@@ -5,7 +5,7 @@
 
 namespace Ocelot.Provider.Kubernetes
 {
-    public static class KubernetesProviderFactory
+    public static class KubernetesProviderFactory // TODO : IServiceDiscoveryProviderFactory
     {
         /// <summary>
         /// String constant used for provider type definition.

diff --git a/src/Ocelot.Provider.Kubernetes/Ocelot.Provider.Kubernetes.csproj b/src/Ocelot.Provider.Kubernetes/Ocelot.Provider.Kubernetes.csproj
@@ -25,8 +25,8 @@
     <NoWarn>1591</NoWarn>
   </PropertyGroup>
   <ItemGroup>
-    <PackageReference Include="KubeClient" Version="2.5.10" />
-    <PackageReference Include="KubeClient.Extensions.DependencyInjection" Version="2.5.10" />
+    <PackageReference Include="KubeClient" Version="2.5.12" />
+    <PackageReference Include="KubeClient.Extensions.DependencyInjection" Version="2.5.12" />
     <PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.556">
       <PrivateAssets>all</PrivateAssets>
     </PackageReference>

diff --git a/src/Ocelot.Provider.Kubernetes/OcelotBuilderExtensions.cs b/src/Ocelot.Provider.Kubernetes/OcelotBuilderExtensions.cs
@@ -1,4 +1,6 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 using Ocelot.DependencyInjection;
 using Ocelot.Provider.Kubernetes.Interfaces;
 
@@ -9,10 +11,27 @@ public static class OcelotBuilderExtensions
     public static IOcelotBuilder AddKubernetes(this IOcelotBuilder builder, bool usePodServiceAccount = true)
     {
         builder.Services
-            .AddKubeClient(usePodServiceAccount)
-            .AddSingleton(KubernetesProviderFactory.Get)
+
+            // Sweet couple
+            .AddSingleton<IKubeApiClient, KubeApiClient>(KubeApiClientFactory) // TODO Revert to .AddKubeClient(usePodServiceAccount) after making KubernetesProviderFactory as IServiceDiscoveryProviderFactory 
+            .AddSingleton(KubernetesProviderFactory.Get) // TODO Must be removed after deprecation of ServiceDiscoveryFinderDelegate in favor of IServiceDiscoveryProviderFactory
+
             .AddSingleton<IKubeServiceBuilder, KubeServiceBuilder>()
             .AddSingleton<IKubeServiceCreator, KubeServiceCreator>();
+
         return builder;
+
+        KubeApiClient KubeApiClientFactory(IServiceProvider sp)
+        {
+            if (usePodServiceAccount)
+            {
+                return KubeApiClient.CreateFromPodServiceAccount(sp.GetService<ILoggerFactory>());
+            }
+
+            KubeClientOptions options = sp.GetRequiredService<IOptions<KubeClientOptions>>().Value;
+            options.LoggerFactory ??= sp.GetService<ILoggerFactory>();
+
+            return KubeApiClient.Create(options);
+        }
     }
 }
diff --git a/test/Ocelot.AcceptanceTests/ServiceDiscovery/KubernetesServiceDiscoveryTests.cs b/test/Ocelot.AcceptanceTests/ServiceDiscovery/KubernetesServiceDiscoveryTests.cs
@@ -15,28 +15,26 @@
 using Ocelot.ServiceDiscovery.Providers;
 using Ocelot.Values;
 using System.Runtime.CompilerServices;
-using System.Text;
 
 namespace Ocelot.AcceptanceTests.ServiceDiscovery;
 
 public sealed class KubernetesServiceDiscoveryTests : ConcurrentSteps, IDisposable
 {
     private readonly string _kubernetesUrl;
-    private readonly IKubeApiClient _clientFactory;
     private readonly ServiceHandler _kubernetesHandler;
     private string _receivedToken;
+    private readonly Action<KubeClientOptions> _kubeClientOptionsConfigure;
 
     public KubernetesServiceDiscoveryTests()
     {
         _kubernetesUrl = DownstreamUrl(PortFinder.GetRandomPort());
-        var option = new KubeClientOptions
+        _kubeClientOptionsConfigure = opts =>
         {
-            ApiEndPoint = new Uri(_kubernetesUrl),
-            AccessToken = "txpc696iUhbVoudg164r93CxDTrKRVWG",
-            AuthStrategy = KubeAuthStrategy.BearerToken,
-            AllowInsecure = true,
+            opts.ApiEndPoint = new Uri(_kubernetesUrl);
+            opts.AccessToken = "txpc696iUhbVoudg164r93CxDTrKRVWG";
+            opts.AuthStrategy = KubeAuthStrategy.BearerToken;
+            opts.AllowInsecure = true;
         };
-        _clientFactory = KubeApiClient.Create(option);
         _kubernetesHandler = new();
     }
 
@@ -62,7 +60,7 @@ public void ShouldReturnServicesFromK8s()
         this.Given(x => GivenServiceInstanceIsRunning(downstreamUrl, downstreamResponse))
             .And(x => x.GivenThereIsAFakeKubernetesProvider(endpoints, serviceName, namespaces))
             .And(_ => GivenThereIsAConfiguration(configuration))
-            .And(_ => GivenOcelotIsRunningWithServices(WithKubernetes))
+            .And(_ => GivenOcelotIsRunningWithServices(WithKubernetes, true))
             .When(_ => WhenIGetUrlOnTheApiGateway("/"))
             .Then(_ => ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
             .And(_ => ThenTheResponseBodyShouldBe($"1:{downstreamResponse}"))
@@ -102,7 +100,7 @@ public void ShouldReturnServicesByPortNameAsDownstreamScheme(string downstreamSc
         this.Given(x => GivenServiceInstanceIsRunning(downstreamUrl, nameof(ShouldReturnServicesByPortNameAsDownstreamScheme)))
             .And(x => x.GivenThereIsAFakeKubernetesProvider(endpoints, serviceName, namespaces))
             .And(_ => GivenThereIsAConfiguration(configuration))
-            .And(_ => GivenOcelotIsRunningWithServices(WithKubernetes))
+            .And(_ => GivenOcelotIsRunningWithServices(WithKubernetes, true))
             .When(_ => WhenIGetUrlOnTheApiGateway("/api/example/1"))
             .Then(_ => ThenTheStatusCodeShouldBe(statusCode))
             .And(_ => ThenTheResponseBodyShouldBe(downstreamScheme == "http"
@@ -178,7 +176,7 @@ public void ShouldHighlyLoadOnUnstableKubeProvider_WithRoundRobinLoadBalancing(i
         var configuration = GivenKubeConfiguration(namespaces, route);
         GivenMultipleServiceInstancesAreRunning(downstreamUrls, downstreamResponses);
         GivenThereIsAConfiguration(configuration);
-        GivenOcelotIsRunningWithServices(WithKubernetesAndRoundRobin);
+        GivenOcelotIsRunningWithServices(WithKubernetesAndRoundRobin, true);
         return (endpoints, servicePorts);
     }
 
@@ -310,16 +308,18 @@ private void GivenThereIsAFakeKubernetesProvider(EndpointsV1 endpoints, bool isS
         });
     }
 
-    private void WithKubernetes(IServiceCollection services) => services
-        .AddOcelot().AddKubernetes()
-        .Services.RemoveAll<IKubeApiClient>().AddSingleton(_clientFactory);
+    private static ServiceDescriptor GetValidateScopesDescriptor()
+        => ServiceDescriptor.Singleton<IServiceProviderFactory<IServiceCollection>>(
+            new DefaultServiceProviderFactory(new() { ValidateScopes = true }));
+    private IOcelotBuilder AddKubernetes(IServiceCollection services) => services
+        .Configure(_kubeClientOptionsConfigure)
+        .Replace(GetValidateScopesDescriptor())
+        .AddOcelot().AddKubernetes(false);
 
-    private void WithKubernetesAndRoundRobin(IServiceCollection services) => services
-        .AddOcelot().AddKubernetes()
+    private void WithKubernetes(IServiceCollection services) => AddKubernetes(services);
+    private void WithKubernetesAndRoundRobin(IServiceCollection services) => AddKubernetes(services)
         .AddCustomLoadBalancer<RoundRobinAnalyzer>(GetRoundRobinAnalyzer)
-        .Services
-        .RemoveAll<IKubeApiClient>().AddSingleton(_clientFactory)
-        .RemoveAll<IKubeServiceCreator>().AddSingleton<IKubeServiceCreator, FakeKubeServiceCreator>();
+        .Services.RemoveAll<IKubeServiceCreator>().AddSingleton<IKubeServiceCreator, FakeKubeServiceCreator>();
 
     private int _k8sCounter, _k8sServiceGeneration;
     private static readonly object K8sCounterLocker = new();
@@ -343,7 +343,7 @@ protected override ServiceHostAndPort GetServiceHostAndPort(KubeRegistryConfigur
         var index = subset.Addresses.IndexOf(address);
         var portV1 = ports[index];
         Logger.LogDebug(() => $"K8s service with key '{configuration.KeyOfServiceInK8s}' and address {address.Ip}; Detected port is {portV1.Name}:{portV1.Port}. Total {ports.Count} ports of [{string.Join(',', ports.Select(p => p.Name))}].");
-        return new ServiceHostAndPort(address.Ip, portV1.Port, portV1.Name);
+        return new ServiceHostAndPort(address.Ip, (int)portV1.Port, portV1.Name);
     }
 
     protected override IEnumerable<string> GetServiceTags(KubeRegistryConfiguration configuration, EndpointsV1 endpoint, EndpointSubsetV1 subset, EndpointAddressV1 address)

diff --git a/test/Ocelot.UnitTests/Kubernetes/KubeServiceCreatorTests.cs b/test/Ocelot.UnitTests/Kubernetes/KubeServiceCreatorTests.cs
@@ -70,6 +70,7 @@ public void Create_NullArgs_ReturnedEmpty(bool isConfiguration, bool isEndpoint,
         Assert.Empty(actual);
     }
 
+    [Trait("Bug", "977")]
     [Fact(DisplayName = "Create: With empty args -> No exceptions during creation")]
     public void Create_NotNullButEmptyArgs_CreatedEmptyService()
     {
@@ -94,6 +95,8 @@ public void Create_NotNullButEmptyArgs_CreatedEmptyService()
         var actualService = actual.SingleOrDefault();
         Assert.NotNull(actualService);
         Assert.Null(actualService.Name);
+        Assert.NotNull(actualService.HostAndPort);
+        Assert.Equal(80, actualService.HostAndPort.DownstreamPort);
     }
 
     [Fact]

diff --git a/test/Ocelot.UnitTests/Kubernetes/KubeTests.cs b/test/Ocelot.UnitTests/Kubernetes/KubeTests.cs
@@ -13,6 +13,10 @@
 
 namespace Ocelot.UnitTests.Kubernetes;
 
+/// <summary>
+/// Contains integration tests.
+/// Move to integration testing, and add at least one "happy path" unit test.
+/// </summary>
 public class KubeTests
 {
     private readonly Mock<IOcelotLoggerFactory> _factory;
@@ -50,7 +54,7 @@ public async Task Should_return_service_from_k8s()
         receivedToken.Value.ShouldBe($"Bearer {nameof(Should_return_service_from_k8s)}");
     }
 
-    [Fact]
+    [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()
     {

diff --git a/test/Ocelot.UnitTests/Kubernetes/KubernetesProviderFactoryTests.cs b/test/Ocelot.UnitTests/Kubernetes/KubernetesProviderFactoryTests.cs
@@ -0,0 +1,88 @@
+using KubeClient;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using Ocelot.Configuration;
+using Ocelot.Configuration.Builder;
+using Ocelot.DependencyInjection;
+using Ocelot.Provider.Kubernetes;
+using Ocelot.Provider.Kubernetes.Interfaces;
+using Ocelot.ServiceDiscovery;
+using Ocelot.ServiceDiscovery.Providers;
+using System.Runtime.CompilerServices;
+
+namespace Ocelot.UnitTests.Kubernetes;
+
+public class KubernetesProviderFactoryTests : UnitTest
+{
+    private readonly IOcelotBuilder _builder;
+
+    public KubernetesProviderFactoryTests()
+    {
+        _builder = new ServiceCollection()
+            .AddOcelot().AddKubernetes();
+    }
+
+    [Theory]
+    [Trait("Bug", "977")]
+    [InlineData(typeof(Kube))]
+    [InlineData(typeof(PollKube))]
+    public void CreateProvider_IKubeApiClientHasOriginalLifetimeWithEnabledScopesValidation_ShouldResolveProvider(Type providerType)
+    {
+        // Arrange
+        var kubeClient = new Mock<IKubeApiClient>();
+        kubeClient.Setup(x => x.ResourceClient(It.IsAny<Func<IKubeApiClient, IEndPointClient>>()))
+            .Returns(Mock.Of<IEndPointClient>());
+        var descriptor = _builder.Services.First(x => x.ServiceType == typeof(IKubeApiClient));
+        _builder.Services.Replace(ServiceDescriptor.Describe(descriptor.ServiceType, _ => kubeClient.Object, descriptor.Lifetime));
+        var serviceProvider = _builder.Services.BuildServiceProvider(validateScopes: true);
+        var config = GivenServiceProvider(providerType.Name);
+        var route = GivenRoute();
+
+        // Act
+        IServiceDiscoveryProvider actual = null;
+        var resolving = () => actual = serviceProvider
+            .GetRequiredService<ServiceDiscoveryFinderDelegate>() // returns KubernetesProviderFactory.Get instance
+            .Invoke(serviceProvider, config, route);
+
+        // Assert
+        resolving.ShouldNotThrow();
+        actual.ShouldNotBeNull().ShouldBeOfType(providerType);
+    }
+
+    [Theory]
+    [Trait("Bug", "977")]
+    [InlineData(nameof(Kube))]
+    [InlineData(nameof(PollKube))]
+    public void CreateProvider_IKubeApiClientHasScopedLifetimeWithEnabledScopesValidation_ShouldFailToResolve(string providerType)
+    {
+        // Arrange
+        var descriptor = ServiceDescriptor.Describe(typeof(IKubeApiClient), _ => Mock.Of<IKubeApiClient>(), ServiceLifetime.Scoped);
+        _builder.Services.Replace(descriptor);
+        var serviceProvider = _builder.Services.BuildServiceProvider(validateScopes: true);
+        var config = GivenServiceProvider(providerType);
+        var route = GivenRoute();
+
+        // Act
+        IServiceDiscoveryProvider actual = null;
+        var resolving = () => actual = serviceProvider
+            .GetRequiredService<ServiceDiscoveryFinderDelegate>() // returns KubernetesProviderFactory.Get instance
+            .Invoke(serviceProvider, config, route);
+
+        // Assert
+        var ex = resolving.ShouldThrow<InvalidOperationException>();
+        ex.Message.ShouldContain("Cannot resolve scoped service 'KubeClient.IKubeApiClient' from root provider");
+        actual.ShouldBeNull();
+    }
+
+    private static ServiceProviderConfiguration GivenServiceProvider(string type) => new(
+        type: type,
+        scheme: string.Empty,
+        host: string.Empty,
+        port: 1,
+        token: string.Empty,
+        configurationKey: string.Empty,
+        pollingInterval: 1);
+
+    private static DownstreamRoute GivenRoute([CallerMemberName] string serviceName = "test-service")
+        => new DownstreamRouteBuilder().WithServiceName(serviceName).Build();
+}