-
Notifications
You must be signed in to change notification settings - Fork 2
Eagerly load services #8
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| using System.Net.Http.Json; | ||
| using System.Text.Json.Nodes; | ||
| using Microsoft.Extensions.Diagnostics.HealthChecks; | ||
|
|
||
| namespace Aspire.Hosting.LocalStack.Internal; | ||
|
|
||
| internal sealed class LocalStackHealthCheck(Uri uri, string[] services) : IHealthCheck, IDisposable | ||
| { | ||
| private readonly HttpClient _client = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Letβs avoid instantiating See my general comment: #8 (comment) |
||
| new(new SocketsHttpHandler { ActivityHeadersPropagator = null }) | ||
| { | ||
| BaseAddress = uri, Timeout = TimeSpan.FromSeconds(1) | ||
| }; | ||
|
|
||
| public async Task<HealthCheckResult> CheckHealthAsync(HealthCheckContext context, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| try | ||
| { | ||
| #pragma warning disable CA2234 | ||
| using var response = await _client.GetAsync("_localstack/health", cancellationToken).ConfigureAwait(false); | ||
| #pragma warning restore CA2234 | ||
| if (response.IsSuccessStatusCode) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider a fail-fast approach here. We can invert the condition and return early to improve readability. |
||
| { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somewhere after // LAZY LOADING MODE: No specific services to check
if (services is null || services.Length == 0)
{
return HealthCheckResult.Healthy("LocalStack is healthy");
}That way, the lazy-loading case is explicitly covered. Alternatively, instead of relying on the services array, it might be more robust to pass an explicit Lazy/Eager mode flag into the LocalStackHealthCheck class, so the behavior is intentional and less dependent on state. |
||
| var responseJson = | ||
| await response.Content.ReadFromJsonAsync<JsonNode>(cancellationToken: cancellationToken).ConfigureAwait(false); | ||
| var servicesNode = responseJson?["services"]?.AsObject(); | ||
|
|
||
| if (servicesNode is null) | ||
| { | ||
| return HealthCheckResult.Unhealthy( | ||
| "LocalStack health response did not contain a 'services' object." | ||
| ); | ||
| } | ||
|
|
||
| var failingServices = services | ||
| .Where(s => | ||
| !servicesNode.ContainsKey(s) | ||
| || servicesNode[s]?.ToString() != "running" | ||
| ) | ||
| .ToList(); | ||
|
|
||
| if (failingServices.Count == 0) | ||
| { | ||
| return HealthCheckResult.Healthy("LocalStack is healthy."); | ||
| } | ||
|
|
||
| var reason = | ||
| $"The following required services are not running: {string.Join(", ", failingServices)}."; | ||
| return HealthCheckResult.Unhealthy( | ||
| $"LocalStack is unhealthy. {reason}" | ||
| ); | ||
| } | ||
|
|
||
| return HealthCheckResult.Unhealthy("LocalStack is unhealthy."); | ||
| } | ||
| #pragma warning disable CA1031 // Do not catch general exception types | ||
| catch (Exception ex) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just nitpicking here. It would also be useful to handle cases like: catch (HttpRequestException ex)
{
return HealthCheckResult.Unhealthy("LocalStack health check failed: network error", ex);
}
catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException)
{
return HealthCheckResult.Unhealthy("LocalStack health check timed out", ex);
}This way we can capture network errors and timeouts explicitly, making the health check more easier to diagnose. |
||
| #pragma warning restore CA1031 // Do not catch general exception types | ||
| { | ||
| return HealthCheckResult.Unhealthy("LocalStack is unhealthy.", ex); | ||
| } | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| _client.Dispose(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,8 +13,11 @@ | |
| using Aspire.Hosting.LocalStack.Container; | ||
| using Aspire.Hosting.LocalStack.Internal; | ||
| using LocalStack.Client.Contracts; | ||
| using LocalStack.Client.Enums; | ||
| using LocalStack.Client.Options; | ||
| using Microsoft.Extensions.Configuration; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Diagnostics.HealthChecks; | ||
|
|
||
| namespace Aspire.Hosting; | ||
|
|
||
|
|
@@ -163,7 +166,6 @@ public static IDistributedApplicationBuilder UseLocalStack(this IDistributedAppl | |
| .WithImageRegistry(LocalStackContainerImageTags.Registry) | ||
| .WithImageTag(LocalStackContainerImageTags.Tag) | ||
| .WithHttpEndpoint(targetPort: Constants.DefaultContainerPort, name: LocalStackResource.PrimaryEndpointName) | ||
| .WithHttpHealthCheck("/_localstack/health", 200, LocalStackResource.PrimaryEndpointName) | ||
| .WithLifetime(containerOptions.Lifetime) | ||
| .WithEnvironment("DEBUG", containerOptions.DebugLevel.ToString(CultureInfo.InvariantCulture)) | ||
| .WithEnvironment("LS_LOG", containerOptions.LogLevel.ToEnvironmentValue()) | ||
|
|
@@ -177,6 +179,31 @@ public static IDistributedApplicationBuilder UseLocalStack(this IDistributedAppl | |
| resourceBuilder = resourceBuilder.WithEnvironment(key, value); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somewhere around here we can register our HttpClient: builder.Services.AddHttpClient("localstack");It might be cleaner to avoid using a magic string maybe move "localstack" into a constant. Iβm not entirely sure which approach is better here, but leaning toward a constant for clarity. |
||
| if (containerOptions.EagerLoadedServices.Count == 0) | ||
| { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Letβs drop this block altogether, since both lazy and eager cases will be handled directly inside |
||
| resourceBuilder = resourceBuilder.WithHttpHealthCheck("/_localstack/health", 200, LocalStackResource.PrimaryEndpointName); | ||
| } | ||
| else | ||
| { | ||
| resourceBuilder = resourceBuilder.WithEnvironment("EAGER_SERVICE_LOADING", "1"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think setting the env variables here is reasonable. That said, thereβs a subtle edge case: Inside // Add any additional environment variables
foreach (var (key, value) in containerOptions.AdditionalEnvironmentVariables)
{
resourceBuilder = resourceBuilder.WithEnvironment(key, value);
}If the same env variables are passed in both places, Iβm not sure which one would win, probably the ones added last, but itβs not obvious. I donβt have a perfect solution right now, but a couple of future options could be:
The first approach might be too restrictive, so Iβm leaning toward the validation idea as a safer middle ground. Either way, I donβt think we need to solve this in this PR, Iβm not expecting that. I just wanted to share this thought for future consideration. |
||
|
|
||
| List<string> serviceNames = []; | ||
| foreach (var awsService in containerOptions.EagerLoadedServices) | ||
| { | ||
| var serviceName = AwsServiceEndpointMetadata.ByEnum(awsService)!.CliName; | ||
| if (serviceName is null) | ||
| { | ||
| throw new InvalidOperationException($"Eager loaded service '{awsService}' is not supported by LocalStack."); | ||
| } | ||
| serviceNames.Add(serviceName); | ||
| } | ||
|
|
||
| var servicesValue = string.Join(',', serviceNames); | ||
| resourceBuilder = resourceBuilder | ||
| .WithEnvironment("SERVICES", servicesValue) | ||
| .WithLocalStackHealthCheck(serviceNames.ToArray()); | ||
| } | ||
|
|
||
| // Configure callback for dynamic resource configuration | ||
| var callback = LocalStackConnectionStringAvailableCallback.CreateCallback(builder); | ||
| resourceBuilder.OnConnectionStringAvailable(callback); | ||
|
|
@@ -258,4 +285,54 @@ public static ILocalStackOptions AddLocalStackOptions(this IDistributedApplicati | |
|
|
||
| return options; | ||
| } | ||
|
|
||
| private static IResourceBuilder<T> WithLocalStackHealthCheck<T>(this IResourceBuilder<T> builder, string[] services) where T : IResourceWithEndpoints | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my general comment: #8 (comment) |
||
| { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Help me here, I think I should delete most of this method and use what you already have with |
||
| ArgumentNullException.ThrowIfNull(builder); | ||
|
|
||
| var endpoint = builder.Resource.GetEndpoint(LocalStackResource.PrimaryEndpointName); | ||
| if (endpoint.Scheme != "http") | ||
| { | ||
| throw new DistributedApplicationException($"Could not create HTTP health check for resource '{builder.Resource.Name}' as the endpoint with name '{endpoint.EndpointName}' and scheme '{endpoint.Scheme}' is not an HTTP endpoint."); | ||
| } | ||
|
|
||
| builder.EnsureEndpointIsAllocated(endpoint); | ||
|
|
||
| Uri? baseUri = null; | ||
| builder.ApplicationBuilder.Eventing.Subscribe<BeforeResourceStartedEvent>(builder.Resource, (@event, ct) => | ||
| { | ||
| baseUri = new Uri(endpoint.Url, UriKind.Absolute); | ||
| return Task.CompletedTask; | ||
| }); | ||
|
|
||
| var healthCheckKey = $"{builder.Resource.Name}_localstack_check"; | ||
|
|
||
| builder.ApplicationBuilder.Services.AddHealthChecks().Add(new HealthCheckRegistration(healthCheckKey, | ||
| _ => | ||
| { | ||
| return baseUri switch | ||
| { | ||
| null => throw new DistributedApplicationException( | ||
| "The URI for the health check is not set. Ensure that the resource has been allocated before the health check is executed."), | ||
| _ => new LocalStackHealthCheck(baseUri!, services) | ||
| }; | ||
| }, failureStatus: null, tags: null)); | ||
|
|
||
| builder.WithHealthCheck(healthCheckKey); | ||
|
|
||
| return builder; | ||
| } | ||
|
|
||
| private static void EnsureEndpointIsAllocated<T>(this IResourceBuilder<T> builder, EndpointReference endpoint) where T : IResourceWithEndpoints | ||
| { | ||
| var endpointName = endpoint.EndpointName; | ||
|
|
||
| builder.OnResourceEndpointsAllocated((_, _, _) => | ||
| endpoint.Exists switch | ||
| { | ||
| true => Task.CompletedTask, | ||
| false => throw new DistributedApplicationException( | ||
| $"The endpoint '{endpointName}' does not exist on the resource '{builder.Resource.Name}'.") | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,18 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <Sdk Name="Aspire.AppHost.Sdk" Version="9.4.0" /> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes in this file is to allow inline creation of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this PR (#9), I added some new useful helpers to the |
||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>$(DefaultTargetFramework)</TargetFramework> | ||
| <OutputType>Exe</OutputType> | ||
| <ContinuousIntegrationBuild>false</ContinuousIntegrationBuild> | ||
| <IsAspireHost>false</IsAspireHost> | ||
| <IsTestProject>true</IsTestProject> | ||
| <NoWarn>$(NoWarn);CA2007</NoWarn> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Aspire.Hosting.AppHost" /> | ||
| <PackageReference Include="AWSSDK.Core"/> | ||
| <PackageReference Include="AWSSDK.DynamoDBv2"/> | ||
| <PackageReference Include="AWSSDK.SQS"/> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| using System.Net.Http.Json; | ||
| using System.Text.Json.Nodes; | ||
| using Amazon; | ||
| using Amazon.SQS.Model; | ||
| using Aspire.Hosting.LocalStack.Container; | ||
| using LocalStack.Client.Enums; | ||
|
|
||
| namespace Aspire.Hosting.LocalStack.Integration.Tests.EagerLoadedServices; | ||
|
|
||
| public class EagerLoadedServicesTests | ||
| { | ||
| [Fact] | ||
| public async Task LocalStack_Should_Lazy_Load_Services_By_Default_Async() | ||
| { | ||
| using var parentCts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); | ||
| using var cts = CancellationTokenSource.CreateLinkedTokenSource(parentCts.Token, TestContext.Current.CancellationToken); | ||
|
|
||
| #pragma warning disable CA1849 | ||
| await using var builder = DistributedApplicationTestingBuilder.Create("LocalStack:UseLocalStack=true"); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've decided that this is simple enough to not warrant a "playground" project and just create these builders inline |
||
| #pragma warning restore CA1849 | ||
|
|
||
| var awsConfig = builder.AddAWSSDKConfig().WithRegion(RegionEndpoint.EUCentral1); | ||
| builder.AddLocalStack(awsConfig: awsConfig, configureContainer: container => | ||
| { | ||
| container.Lifetime = ContainerLifetime.Session; | ||
| container.DebugLevel = 1; | ||
| container.LogLevel = LocalStackLogLevel.Debug; | ||
| }); | ||
|
|
||
| await using var app = await builder.BuildAsync(cts.Token); | ||
| await app.StartAsync(cts.Token); | ||
|
|
||
| var resourceNotificationService = app.Services.GetRequiredService<ResourceNotificationService>(); | ||
|
|
||
| await resourceNotificationService.WaitForResourceHealthyAsync("localstack", cts.Token); | ||
|
|
||
| using var httpClient = app.CreateHttpClient("localstack", "http"); | ||
| var healthResponse = await httpClient.GetAsync(new Uri("/_localstack/health", UriKind.Relative), cts.Token); | ||
| var healthContent = await healthResponse.Content.ReadFromJsonAsync<JsonNode>(cts.Token); | ||
| Assert.Equal(HttpStatusCode.OK, healthResponse.StatusCode); | ||
|
|
||
| var servicesNode = healthContent?["services"]?.AsObject(); | ||
| Assert.NotNull(servicesNode); | ||
| Assert.True(servicesNode.ContainsKey("sqs")); | ||
| Assert.NotEqual("running", servicesNode["sqs"]?.ToString()); | ||
|
|
||
| var connectionString = await app.GetConnectionStringAsync("localstack", cancellationToken: cts.Token); | ||
| Assert.NotNull(connectionString); | ||
| Assert.NotEmpty(connectionString); | ||
|
|
||
| var connectionStringUri = new Uri(connectionString); | ||
|
|
||
| var configOptions = new ConfigOptions(connectionStringUri.Host, edgePort: connectionStringUri.Port); | ||
| var sessionOptions = new SessionOptions(regionName: awsConfig.Region!.SystemName); | ||
| var session = SessionStandalone.Init().WithSessionOptions(sessionOptions).WithConfigurationOptions(configOptions).Create(); | ||
|
|
||
| var sqsClient = session.CreateClientByImplementation<AmazonSQSClient>(); | ||
| await sqsClient.ListQueuesAsync(new ListQueuesRequest(), cts.Token); | ||
|
|
||
| var laterHealthResponse = await httpClient.GetAsync(new Uri("/_localstack/health", UriKind.Relative), cts.Token); | ||
| var laterHealthContent = await laterHealthResponse.Content.ReadFromJsonAsync<JsonNode>(cts.Token); | ||
| Assert.Equal(HttpStatusCode.OK, laterHealthResponse.StatusCode); | ||
|
|
||
| var sqsServicesNode = laterHealthContent?["services"]?.AsObject(); | ||
| Assert.NotNull(sqsServicesNode); | ||
| Assert.True(sqsServicesNode.ContainsKey("sqs")); | ||
| Assert.Equal("running", sqsServicesNode["sqs"]?.ToString()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task LocalStack_Should_Eagerly_Load_Services_When_Configured_Async() | ||
| { | ||
| using var parentCts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); | ||
| using var cts = CancellationTokenSource.CreateLinkedTokenSource(parentCts.Token, TestContext.Current.CancellationToken); | ||
|
|
||
| #pragma warning disable CA1849 | ||
| await using var builder = DistributedApplicationTestingBuilder.Create("LocalStack:UseLocalStack=true"); | ||
| #pragma warning restore CA1849 | ||
|
|
||
| var awsConfig = builder.AddAWSSDKConfig().WithRegion(RegionEndpoint.EUCentral1); | ||
| builder.AddLocalStack(awsConfig: awsConfig, configureContainer: container => | ||
| { | ||
| container.Lifetime = ContainerLifetime.Session; | ||
| container.DebugLevel = 1; | ||
| container.LogLevel = LocalStackLogLevel.Debug; | ||
| container.EagerLoadedServices = [AwsService.Sqs]; | ||
| }); | ||
|
|
||
| await using var app = await builder.BuildAsync(cts.Token); | ||
| await app.StartAsync(cts.Token); | ||
|
|
||
| var resourceNotificationService = app.Services.GetRequiredService<ResourceNotificationService>(); | ||
|
|
||
| await resourceNotificationService.WaitForResourceHealthyAsync("localstack", cts.Token); | ||
|
|
||
| using var httpClient = app.CreateHttpClient("localstack", "http"); | ||
| var healthResponse = await httpClient.GetAsync(new Uri("/_localstack/health", UriKind.Relative), cts.Token); | ||
| var healthContent = await healthResponse.Content.ReadFromJsonAsync<JsonNode>(cts.Token); | ||
| Assert.Equal(HttpStatusCode.OK, healthResponse.StatusCode); | ||
|
|
||
| var servicesNode = healthContent?["services"]?.AsObject(); | ||
| Assert.NotNull(servicesNode); | ||
| Assert.True(servicesNode.ContainsKey("sqs")); | ||
| Assert.Equal("running", servicesNode["sqs"]?.ToString()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change, just spotted the doc comments refer to an option that doesn't exist