Skip to content

#2178 DI service resolution from scoped HttpContext request services for the IConsulServiceBuilder service #2179

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Oct 23, 2024
Merged

#2178 DI service resolution from scoped HttpContext request services for the IConsulServiceBuilder service #2179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5093e2e
Make tests red by turning on scope validation
Oct 21, 2024
e271d05
Fix red tests by resolving service builder from HttpContext scope
Oct 21, 2024
2dc5bc1
Create a new unit test that detects scope validation error
Oct 21, 2024
a0debc8
Add optional validation scope for acceptance tests usage
Oct 21, 2024
56b22df
Fix a broken acceptance test
Oct 21, 2024
934a669
Trigger pipeline
Oct 21, 2024
399f57c
Update test/Ocelot.UnitTests/Consul/ProviderFactoryTests.cs
raman-m Oct 21, 2024
fefe8de
Remove BDDfy
raman-m Oct 21, 2024
aeef51d
EOL: test/Ocelot.UnitTests/Consul/ProviderFactoryTests.cs
raman-m Oct 21, 2024
d4d5af5
Review tests: AAA pattern
raman-m Oct 21, 2024
a45e030
Review tests by @raman-m
raman-m Oct 22, 2024
d91e2e5
Rename the class to `ConsulProviderFactoryTests`.
raman-m Oct 22, 2024
60ec1a0
Validate scopes in `ConsulConfigurationInConsulTests`
raman-m Oct 22, 2024
581664f
Validate scopes in `ConfigurationInConsulTests`
raman-m Oct 22, 2024
8ba2d6b
Validate scopes in `ConsulWebSocketTests`
raman-m Oct 22, 2024
79d7726
Validate scopes in `ConsulServiceDiscoveryTests`
raman-m Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/Ocelot.Provider.Consul/ConsulProviderFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@ public static class ConsulProviderFactory // TODO : IServiceDiscoveryProviderFac
public static ServiceDiscoveryFinderDelegate Get { get; } = CreateProvider;
private static IServiceDiscoveryProvider CreateProvider(IServiceProvider provider, ServiceProviderConfiguration config, DownstreamRoute route)
{
// Singleton services
var factory = provider.GetService<IOcelotLoggerFactory>();
var consulFactory = provider.GetService<IConsulClientFactory>();
var contextAccessor = provider.GetService<IHttpContextAccessor>();

// Scoped services
var context = contextAccessor.HttpContext;
var configuration = new ConsulRegistryConfiguration(config.Scheme, config.Host, config.Port, route.ServiceName, config.Token); // TODO Why not to pass 2 args only: config, route? LoL
contextAccessor.HttpContext.Items[nameof(ConsulRegistryConfiguration)] = configuration; // initialize data
var serviceBuilder = provider.GetService<IConsulServiceBuilder>(); // consume data in default/custom builder
context.Items[nameof(ConsulRegistryConfiguration)] = configuration; // initialize data
var serviceBuilder = context.RequestServices.GetService<IConsulServiceBuilder>(); // consume data in default/custom builder
Copy link
Member

@raman-m raman-m Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does reading context services implicitly create a scope? I'll demonstrate the difference in the following code:

- var scope = provider.CreateScope();
- var serviceBuilder = scope.ServiceProvider.GetService<IConsulServiceBuilder>(); // consume data in default/custom builder
+ var context = contextAccessor.HttpContext;
+ var serviceBuilder = context.RequestServices.GetService<IConsulServiceBuilder>(); // consume data in default/custom builder

Thus, are the two solutions equivalent?

Copy link
Contributor Author

@Niksson Niksson Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK ASP.NET creates a DI scope for HttpContext of each HTTP request (see this link for more info).
Technically, the solutions are equivalent, but I thought that since we already access HttpContext via IHttpContextAccessor, it would be logical to access the scope of the HTTP request instead of creating a new one.

Copy link
Member

@raman-m raman-m Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see this link for more info

Yep, good to know!
Bu gaga, Tom should read these best DI practices in 2017-2020 when designing everything as singletons and delegates → Design services for dependency injection

🆗 Not an issue. Your solution should function properly.


var consulProvider = new Consul(configuration, factory, consulFactory, serviceBuilder); // TODO It must be added to DI-container!

Expand Down
2 changes: 1 addition & 1 deletion test/Ocelot.AcceptanceTests/AggregateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ public void Should_return_response_200_with_user_forwarding()
.And(x => x.GivenServiceIsRunning(1, port2, "/", 200, "{Hello from Tom}"))
.And(x => auth.GivenIHaveAToken(identityServerUrl))
.And(x => auth.GivenThereIsAConfiguration(configuration))
.And(x => auth.GivenOcelotIsRunningWithServices(configureServices, configureApp))
.And(x => auth.GivenOcelotIsRunningWithServices(configureServices, configureApp, true))
.And(x => auth.GivenIHaveAddedATokenToMyRequest())
.When(x => auth.WhenIGetUrlOnTheApiGateway("/"))
.Then(x => auth.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
Expand Down
299 changes: 167 additions & 132 deletions test/Ocelot.AcceptanceTests/Configuration/ConfigurationInConsulTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,180 +1,215 @@
using CacheManager.Core;
using Consul;
using IdentityServer4.Extensions;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;
using Newtonsoft.Json;
using Ocelot.AcceptanceTests.Caching;
using Ocelot.Cache.CacheManager;
using Ocelot.Configuration.File;
using Ocelot.DependencyInjection;
using Ocelot.Middleware;
using Ocelot.Provider.Consul;
using System.Text;

namespace Ocelot.AcceptanceTests.Configuration
namespace Ocelot.AcceptanceTests.Configuration;

public sealed class ConfigurationInConsulTests : Steps, IDisposable
{
public class ConfigurationInConsulTests : IDisposable
private IHost _builder;
private IHost _fakeConsulBuilder;
private FileConfiguration _config;
private readonly List<ServiceEntry> _consulServices;

public ConfigurationInConsulTests()
{
private IHost _builder;
private readonly Steps _steps;
private IHost _fakeConsulBuilder;
private FileConfiguration _config;
private readonly List<ServiceEntry> _consulServices;
_consulServices = new List<ServiceEntry>();
}

public ConfigurationInConsulTests()
{
_consulServices = new List<ServiceEntry>();
_steps = new Steps();
}
public override void Dispose()
{
_builder?.Dispose();
_fakeConsulBuilder?.Dispose();
base.Dispose();
}

[Fact]
public void should_return_response_200_with_simple_url_when_using_jsonserialized_cache()
{
var consulPort = PortFinder.GetRandomPort();
var servicePort = PortFinder.GetRandomPort();
[Fact]
public void Should_return_response_200_with_simple_url_when_using_jsonserialized_cache()
{
var consulPort = PortFinder.GetRandomPort();
var servicePort = PortFinder.GetRandomPort();

var configuration = new FileConfiguration
{
Routes = new List<FileRoute>
var configuration = new FileConfiguration
{
Routes = new List<FileRoute>
{
new()
{
new()
DownstreamPathTemplate = "/",
DownstreamScheme = "http",
DownstreamHostAndPorts = new List<FileHostAndPort>
{
DownstreamPathTemplate = "/",
DownstreamScheme = "http",
DownstreamHostAndPorts = new List<FileHostAndPort>
new()
{
new()
{
Host = "localhost",
Port = servicePort,
},
Host = "localhost",
Port = servicePort,
},
UpstreamPathTemplate = "/",
UpstreamHttpMethod = new List<string> { "Get" },
},
UpstreamPathTemplate = "/",
UpstreamHttpMethod = new List<string> { "Get" },
},
GlobalConfiguration = new FileGlobalConfiguration
},
GlobalConfiguration = new FileGlobalConfiguration
{
ServiceDiscoveryProvider = new FileServiceDiscoveryProvider
{
ServiceDiscoveryProvider = new FileServiceDiscoveryProvider
{
Scheme = "http",
Host = "localhost",
Port = consulPort,
},
Scheme = "http",
Host = "localhost",
Port = consulPort,
},
};

var fakeConsulServiceDiscoveryUrl = $"http://localhost:{consulPort}";

this.Given(x => GivenThereIsAFakeConsulServiceDiscoveryProvider(fakeConsulServiceDiscoveryUrl, string.Empty))
.And(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{servicePort}", string.Empty, 200, "Hello from Laura"))
.And(x => _steps.GivenThereIsAConfiguration(configuration))
.And(x => _steps.GivenOcelotIsRunningUsingConsulToStoreConfigAndJsonSerializedCache())
.When(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
.Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
.BDDfy();
}
},
};

var fakeConsulServiceDiscoveryUrl = DownstreamUrl(consulPort);

this.Given(x => GivenThereIsAFakeConsulServiceDiscoveryProvider(fakeConsulServiceDiscoveryUrl, string.Empty))
.And(x => x.GivenThereIsAServiceRunningOn(DownstreamUrl(servicePort), string.Empty, 200, "Hello from Laura"))
.And(x => GivenThereIsAConfiguration(configuration))
.And(x => x.GivenOcelotIsRunningUsingConsulToStoreConfigAndJsonSerializedCache())
.When(x => WhenIGetUrlOnTheApiGateway("/"))
.Then(x => ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.And(x => ThenTheResponseBodyShouldBe("Hello from Laura"))
.BDDfy();
}

private Task GivenThereIsAFakeConsulServiceDiscoveryProvider(string url, string serviceName)
{
_fakeConsulBuilder = Host.CreateDefaultBuilder()
.ConfigureWebHost(webBuilder =>
{
webBuilder.UseUrls(url)
.UseKestrel()
.UseContentRoot(Directory.GetCurrentDirectory())
.UseIISIntegration()
.UseUrls(url)
.Configure(app =>
private void GivenOcelotIsRunningUsingConsulToStoreConfigAndJsonSerializedCache()
{
_webHostBuilder = new WebHostBuilder()
.UseDefaultServiceProvider(_ => _.ValidateScopes = true)
.ConfigureAppConfiguration((hostingContext, config) =>
{
config.SetBasePath(hostingContext.HostingEnvironment.ContentRootPath);
var env = hostingContext.HostingEnvironment;
config.AddJsonFile("appsettings.json", true, false)
.AddJsonFile($"appsettings.{env.EnvironmentName}.json", true, false);
config.AddJsonFile(_ocelotConfigFileName, true, false);
config.AddEnvironmentVariables();
})
.ConfigureServices(s =>
{
s.AddOcelot()
.AddCacheManager(x => x
.WithMicrosoftLogging(_ => { /*log.AddConsole(LogLevel.Debug);*/ })
.WithJsonSerializer()
.WithHandle(typeof(InMemoryJsonHandle<>)))
.AddConsul()
.AddConfigStoredInConsul();
})
.Configure(app => app.UseOcelot().GetAwaiter().GetResult()); // Turning as async/await some tests got broken

_ocelotServer = new TestServer(_webHostBuilder);
_ocelotClient = _ocelotServer.CreateClient();
}

private Task GivenThereIsAFakeConsulServiceDiscoveryProvider(string url, string serviceName)
{
_fakeConsulBuilder = Host.CreateDefaultBuilder()
.ConfigureWebHost(webBuilder =>
{
webBuilder.UseUrls(url)
.UseKestrel()
.UseContentRoot(Directory.GetCurrentDirectory())
.UseIISIntegration()
.UseUrls(url)
.Configure(app =>
{
app.Run(async context =>
{
app.Run(async context =>
if (context.Request.Method.ToLower() == "get" && context.Request.Path.Value == "/v1/kv/InternalConfiguration")
{
if (context.Request.Method.ToLower() == "get" && context.Request.Path.Value == "/v1/kv/InternalConfiguration")
{
var json = JsonConvert.SerializeObject(_config);
var json = JsonConvert.SerializeObject(_config);

var bytes = Encoding.UTF8.GetBytes(json);
var bytes = Encoding.UTF8.GetBytes(json);

var base64 = Convert.ToBase64String(bytes);
var base64 = Convert.ToBase64String(bytes);

var kvp = new FakeConsulGetResponse(base64);
var kvp = new FakeConsulGetResponse(base64);

await context.Response.WriteJsonAsync(new[] { kvp });
}
else if (context.Request.Method.ToLower() == "put" && context.Request.Path.Value == "/v1/kv/InternalConfiguration")
await context.Response.WriteJsonAsync(new[] { kvp });
}
else if (context.Request.Method.ToLower() == "put" && context.Request.Path.Value == "/v1/kv/InternalConfiguration")
{
try
{
try
{
var reader = new StreamReader(context.Request.Body);
var reader = new StreamReader(context.Request.Body);

// Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true instead.
// var json = reader.ReadToEnd();
var json = await reader.ReadToEndAsync();
// Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true instead.
// var json = reader.ReadToEnd();
var json = await reader.ReadToEndAsync();

_config = JsonConvert.DeserializeObject<FileConfiguration>(json);
_config = JsonConvert.DeserializeObject<FileConfiguration>(json);

var response = JsonConvert.SerializeObject(true);
var response = JsonConvert.SerializeObject(true);

await context.Response.WriteAsync(response);
}
catch (Exception e)
{
Console.WriteLine(e);
throw;
}
await context.Response.WriteAsync(response);
}
else if (context.Request.Path.Value == $"/v1/health/service/{serviceName}")
catch (Exception e)
{
await context.Response.WriteJsonAsync(_consulServices);
Console.WriteLine(e);
throw;
}
});
}
else if (context.Request.Path.Value == $"/v1/health/service/{serviceName}")
{
await context.Response.WriteJsonAsync(_consulServices);
}
});
}).Build();
return _fakeConsulBuilder.StartAsync();
}
});
}).Build();
return _fakeConsulBuilder.StartAsync();
}

public class FakeConsulGetResponse
public class FakeConsulGetResponse
{
public FakeConsulGetResponse(string value)
{
public FakeConsulGetResponse(string value)
{
Value = value;
}

public int CreateIndex => 100;
public int ModifyIndex => 200;
public int LockIndex => 200;
public string Key => "InternalConfiguration";
public int Flags => 0;
public string Value { get; }
public string Session => "adf4238a-882b-9ddc-4a9d-5b6758e4159e";
Value = value;
}

private Task GivenThereIsAServiceRunningOn(string url, string basePath, int statusCode, string responseBody)
{
_builder = Host.CreateDefaultBuilder()
.ConfigureWebHost(webBuilder =>
public int CreateIndex => 100;
public int ModifyIndex => 200;
public int LockIndex => 200;
public string Key => "InternalConfiguration";
public int Flags => 0;
public string Value { get; }
public string Session => "adf4238a-882b-9ddc-4a9d-5b6758e4159e";
}

private Task GivenThereIsAServiceRunningOn(string url, string basePath, int statusCode, string responseBody)
{
_builder = Host.CreateDefaultBuilder()
.ConfigureWebHost(webBuilder =>
{
webBuilder.UseUrls(url)
.UseKestrel()
.UseContentRoot(Directory.GetCurrentDirectory())
.UseIISIntegration()
.UseUrls(url)
.Configure(app =>
{
webBuilder.UseUrls(url)
.UseKestrel()
.UseContentRoot(Directory.GetCurrentDirectory())
.UseIISIntegration()
.UseUrls(url)
.Configure(app =>
app.UsePathBase(basePath);
app.Run(async context =>
{
app.UsePathBase(basePath);
app.Run(async context =>
{
context.Response.StatusCode = statusCode;
await context.Response.WriteAsync(responseBody);
});
context.Response.StatusCode = statusCode;
await context.Response.WriteAsync(responseBody);
});
})
.Build();
return _builder.StartAsync();
}

public void Dispose()
{
_builder?.Dispose();
_steps.Dispose();
}
});
})
.Build();
return _builder.StartAsync();
}
}
Loading