dotnet / aspire

An opinionated, cloud ready stack for building observable, production ready, distributed applications in .NET
https://learn.microsoft.com/dotnet/aspire
MIT License
3.73k stars 430 forks source link

Allow mapping of Parameter resource values to configuration #3679

Closed mitchdenny closed 2 weeks ago

mitchdenny commented 5 months ago

After observing some folks use the Aspire application model I've realized that the builder.AddParameter(...) call adds some confusion. .NET developers naturally reach for the builder.Configuration[configKey] mechanism to get configuration values.

We have good reasons for introducing AddParameter(...) and AddConnectionString(...) in order to support deployment but they do cause some friction locally.

We should consider some options for making this more obvious. What follows is an idea for discussion:

// Support mapping configuration to a parameter. During dev time this value
// would be read from appsettings/user secrets or from an environment variable.
// During deployment time the deployment tool would prompt for these values or use
// its own injection mechanism for pre-populated values.
builder.AddParameter("myparameter", builder.Configuration["SomeOther:Key"], secret: true)

I'd be interested in thoughts around plumbing this through for deployment. The danger with deployment is that we would necessarily need to be able to output the config values into the manifest (unless we came up with some other mechanism). Perhaps we could include the values in the manifest as a default UNLESS the value is a secret?

mitchdenny commented 5 months ago

/cc @davidfowl @DamianEdwards @ReubenBond

NicoVermeir commented 5 months ago

I'm curious about the reasoning behind the switch to AddParameter. Using user secrets locally and pulling secrets from a keyvault or secret settings has been the default for so long that of course we assume that AddParameter is just a wrapper around a builder.Configuration call.

Not saying that this could / should not change, just curious about the reasoning behind it :)

NicoVermeir commented 5 months ago

While looking into this, doesn't this just work in preview 5? looking at https://github.com/dotnet/aspire/blob/1b627b7d5d399d4f9118366e7611e11e56de4554/src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs#L16707566C0-L24C145

it goes looking for Parameters:{name} in the IConfiguration it gets through the builder.

I just tried it by setting this in my user secrets of the apphost project: { "Parameters": { "sqlServerDevPass": "MyComplexPassword1234!" } }

and defining the sql resource like this: var sql = builder.AddSqlServer("MyDBServer", builder.AddParameter("sqlServerDevPass", true)) .WithVolume("VolumeMount.sqlserver.data", "/var/opt/mssql") .AddDatabase("MyDBServer", "MyDB");

and this works just fine

mitchdenny commented 5 months ago

Yes, that is the way it works.

The challenge is the discoverability. There is no hint in the API that this is where the configuration is. So, the proposal above is about allowing people to define the mapping from parameter as an abstract concept in the model to concrete configuration value used for local debugging. We would of course keep the current behavior as a fallback.

Just an idea at this stage.

As for why we have AddParameter(...) and ParameterResource. Think of a parameter as a "slot" to be filled at deployment time. If we used IConfiguration at deployment time we would necessarily have to store the value in the manifest (what azd and other tools used to deploy the apps to the cloud).

mitchdenny commented 5 months ago

Note we are making some inroads into making parameter filling a much richer experience with some integration between Aspire and VS (in an open way) where the AppHost, whilst it is running will be able to ask visual studio to ask the user to provide a value and have that value stored for them in user secrets.

It's a work in progress and won't make Aspire 8.0, but we are hoping for Aspire 8.1.

DamianEdwards commented 5 months ago

Related: https://github.com/dotnet/aspire-samples/blob/fb4adeb0800c941c97dc062335ea71eba55eec6c/samples/Shared/ParameterExtensions.cs Usage: https://github.com/dotnet/aspire-samples/blob/fb4adeb0800c941c97dc062335ea71eba55eec6c/samples/VolumeMount/VolumeMount.AppHost/Program.cs#L6

My intention was to do #1151 similar to the approach in the samples linked to above so that by default for the resources where it makes sense to, generated passwords are stored in user secrets so that they're stable in dev but marked as generated in the manifest still.

davidfowl commented 5 months ago

This issue also makes me feel like we need an overload that lets you specify the value.

https://github.com/Azure/azure-dev/issues/3857#issuecomment-2094920538

eerhardt commented 3 months ago

@mitchdenny - this is currently marked "High in 8.1", meaning we can't ship 8.1 without it. Do we have an idea of what should be done here for 8.1? Or should we move this out of 8.1?

mitchdenny commented 2 months ago

I don't think we've nailed what we want the API surface to look like. There are a few considerations:

  1. Mapping parameters to alternative configuration keys (so not necessarily Parameters:foo).
  2. Specifying a default parameter value (in the case of non-secrets).
  3. Linking storage of values to external configuration stores (e.g. a team wants to centralize secrets in a keyvault and use that to fetch the value from when someone hits F5.
mitchdenny commented 2 months ago

Realistically we probably can't get that all in for 8.1. But I'd like to take it for 8.2 (@DamianEdwards and @maddymontaquila how do you feel about punting this feature for 8.1?).

mitchdenny commented 2 months ago

When I say punt, I mean putting it down to medium priority in 8.1 so that if we decide to ship all the high priority things in 8.1 it is more likely to bubble to the top in 8.2.

maddymontaquila commented 2 months ago

@mitchdenny "Medium" for 8.1 and punted for 8.2 sounds perfect to me!!

eerhardt commented 2 months ago

Moving to 8.2 per discussion above.

davidfowl commented 2 weeks ago

We did this in https://github.com/dotnet/aspire/issues/5410