Skip to content
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

NATS Aspire container #1175

Merged
merged 57 commits into from Mar 7, 2024
Merged

NATS Aspire container #1175

merged 57 commits into from Mar 7, 2024

Conversation

mtmk
Copy link
Contributor

@mtmk mtmk commented Dec 2, 2023

This PR is an effort to introduce NATS container and client support.

Implemented most of the features as much as I could gather from other implementations. There are a few open tasks as far as I can see which may be left for another PR:

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-components Issues pertaining to Aspire Component packages label Dec 2, 2023
@oising
Copy link
Contributor

oising commented Dec 8, 2023

@mtmk I'm rebasing this on latest main to fix merge conflicts and update any broken integration points. Also updating to use centralized package management /cc @ReubenBond

@joperezr joperezr added the community-contribution Indicates that the PR has been added by a community member label Dec 11, 2023
@stebet
Copy link

stebet commented Jan 13, 2024

Oooh exciting! Does it also have cluster/JetStream support?

@mtmk
Copy link
Contributor Author

mtmk commented Jan 14, 2024

Oooh exciting! Does it also have cluster/JetStream support?

Not yet 😅 but I think you can add the argument yourself for now if you like:

var nats = builder.AddNatsContainer("nats-server").WithArgs("-js");

Edit: also added options to AddNatsContainer which essentially does the above, even though I'm not sure if that's the correct approach. (Really need to look at others to see how they've done it)

var nats = builder.AddNatsContainer("nats-server", enableJetStream = true);

@mtmk
Copy link
Contributor Author

mtmk commented Feb 12, 2024

@oising do you think we're ready for review?

@mtmk
Copy link
Contributor Author

mtmk commented Feb 12, 2024

@azure-pipelinesazure-pipelines
/ dotnet.aspire
src/Aspire.Hosting.Nats/Aspire.Hosting.Nats.csproj#L0
src/Aspire.Hosting.Nats/Aspire.Hosting.Nats.csproj(0,0): error NU1102: (NETCORE_ENGINEERING_TELEMETRY=Restore) Unable to find package NATS.Net with version (>= 2.1.0)

@mitchdenny, i'm not sure why ci is failing NATS.Net v2.1.0 in up on NuGet https://www.nuget.org/packages/NATS.Net/2.1.0

@@ -0,0 +1,17 @@
using NATS.Client.Core;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this app a little more real? Can it use pubsub as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, let me have a go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a simple JetStream example since it's easier demonstrate persisted messages using Web API. As for pubsub I suppose we'd need a frontend using SignalR or Blazor. Maybe a simple chat or something?

{
await foreach (var msg in nats.SubscribeAsync<AppEvent>("events.>", cancellationToken: _cts.Token).ConfigureAwait(false))
{
Console.WriteLine($"Processing event: {msg.Data}");
Copy link
Member

Choose a reason for hiding this comment

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

ILogger?


var manifest = await ManifestUtils.GetManifest(nats.Resource);

Assert.Equal("container.v0", manifest["type"]?.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

@mtmk we made some changes here

var manifest = await ManifestUtils.GetManifest(kafka.Resource);
var expectedManifest = """
{
"type": "container.v0",
"connectionString": "{kafka.bindings.tcp.host}:{kafka.bindings.tcp.port}",
"image": "confluentinc/confluent-local:7.6.0",
"env": {
"KAFKA_ADVERTISED_LISTENERS": "PLAINTEXT://localhost:29092,PLAINTEXT_HOST://localhost:9092"
},
"bindings": {
"tcp": {
"scheme": "tcp",
"protocol": "tcp",
"transport": "tcp",
"containerPort": 9092
}
}
}
""";
Assert.Equal(expectedManifest, manifest.ToString());
.

BTW I'm sorry that these changes must feel like a roller coaster 😄. FWIW, it's helping us land on solid patterns that seem to scale well 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries, my fault really having the PR open so long. thanks for bearing with me 🙏 also loving to witness the evolution 😍

Copy link
Member

Choose a reason for hiding this comment

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

If you are interested in more evolution:

.WithEndpoint(hostPort: port, containerPort: 6379, name: RedisResource.PrimaryEndpointName)

/// <summary>
/// Gets the primary endpoint for the Redis server.
/// </summary>
public EndpointReference PrimaryEndpoint => _primaryEndpoint ??= new(this, PrimaryEndpointName);

return $"{PrimaryEndpoint.GetExpression(EndpointProperty.Host)}:{PrimaryEndpoint.GetExpression(EndpointProperty.Port)}";

return $"{PrimaryEndpoint.Host}:{PrimaryEndpoint.Port}";

We're trying to get rid of all the manual string formatting and building up the object model to represent all kids of references. Makes it less error prone and brings consistency to the model.

<PackageReference Include="Microsoft.Extensions.Configuration.Binder" />
<PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks" />
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" />
<PackageReference Include="NATS.Net" />
Copy link
Member

Choose a reason for hiding this comment

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

Our naming rules are https://github.com/dotnet/aspire/tree/main/src/Components#naming.

The name of this component should be Aspire.NATS.Net to align with those rules.

@@ -0,0 +1,23 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

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

This project and folder should be named Nats.AppHost.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

tests/Aspire.Nats.Client.Tests/ConfigurationTests.cs Outdated Show resolved Hide resolved
Comment on lines 43 to 44
protected override void SetHealthCheck(NatsClientSettings options, bool enabled)
=> throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

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

We have health checks in the component. We should enable these tests.


namespace Aspire.Nats.Client;

public class NatsHealthCheck(INatsConnection connection) : IHealthCheck
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be public.

Comment on lines 13 to 16
return Task.FromResult(connection.ConnectionState == NatsConnectionState.Open
? HealthCheckResult.Healthy()
: HealthCheckResult.Unhealthy());
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to be a bit more granular i.e. added degraded state. But description and stats only exist on the old client Nats.Client v1. One we're using here is v2 and it's a rewrite and most of that is not exposed yet.

"properties": {
"Net": {
"type": "object",
"properties": {
Copy link
Member

Choose a reason for hiding this comment

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

Are any of the underlying NatsOpts worth being able to be configured in the configuration? See some of the other components for an example on how we hook up the config binder.

Ex:

private static ConfigurationOptions BindToConfiguration(ConfigurationOptions options, IConfiguration configuration)
{
var configurationOptionsSection = configuration.GetSection("ConfigurationOptions");
configurationOptionsSection.Bind(options);
return options;

"ConfigurationOptions": {
"type": "object",
"properties": {
"AbortOnConnectFail": {
"type": "boolean",
"description": "Gets or sets whether connect/configuration timeouts should be explicitly notified via a TimeoutException."
},
"AllowAdmin": {
"type": "boolean",
"description": "Indicates whether admin operations should be allowed."
},
"AsyncTimeout": {
"type": "integer",
"description": "Specifies the time in milliseconds that the system should allow for asynchronous operations (defaults to SyncTimeout)."
},
"ChannelPrefix": {
"type": "object",
"properties": {
"UseImplicitAutoPattern": {
"type": "boolean",
"description": "Indicates whether channels should use 'StackExchange.Redis.RedisChannel.PatternMode.Auto' when no 'StackExchange.Redis.RedisChannel.PatternMode' is specified; this is enabled by default, but can be disabled to avoid unexpected wildcard scenarios."
}
},
"description": "Automatically encodes and decodes channels."
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I shall look into this in another PR

@sebastienros
Copy link
Member

sebastienros commented Mar 7, 2024

Fixed conflicts, added manifest.json and renamed AddNats() to AddNatsClient to follow a recent change we did in other libraries to prevent conflict with the distributed app extension methods.

Tried the sample, worked for me, then using the Swagger I found an issue, it's not taking the custom json converter for TimeSpan properties in the config object, I believe it's an issue in SwaggerUI. (found domaindrivendev/Swashbuckle.AspNetCore#2500)

@mtmk if you are ok with my changes I'll merge it.

@sebastienros sebastienros merged commit 44cde5d into dotnet:main Mar 7, 2024
8 checks passed
@mtmk mtmk deleted the nats branch March 7, 2024 07:24
@davidfowl
Copy link
Member

Thank you @mtmk ! I look forward to future contributions!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-components Issues pertaining to Aspire Component packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants