Save password parameter default values to user secrets by DamianEdwards · Pull Request #4507 · microsoft/aspire

DamianEdwards · 2024-06-14T01:32:02Z

Adds UserSecretsParameterDefault which wraps a ParameterDefault and saves the value returned by its GetDefaultValue method to the AppHost user secrets.

Updated all resources that use GeneratedParameterDefault to generate a default password/apikey to wrap it in UserSecretsParameterDefault so that the default value is saved to user secrets.

Fixes #1151

Microsoft Reviewers: Open in CodeFlow

#1151

davidfowl · 2024-06-14T02:17:49Z

This feels like an opportunity to design a better API for parameter defaults.

DamianEdwards · 2024-06-14T03:05:04Z

What did you have in mind?

DamianEdwards · 2024-06-14T20:55:06Z

This file is no longer needed as it previously contained the hacked workaround for the lack of this feature.

davidfowl · 2024-06-14T20:59:01Z

        if (!TrySetUserSecret(applicationName, configurationKey, value))
        {
-            throw new DistributedApplicationException($"Failed to set value for parameter '{parameterName}' in application '{applicationName}' to user secrets.");
+            Debug.WriteLine($"Failed to set value for parameter '{parameterName}' in application '{applicationName}' to user secrets.");


To make it a bit easier to see if it's doesn't succeed during dev. There's no logging at this stage and swallowing blindly just seems bad mmm'k

Would it be possible for us to get the ability to log here? (obviously not in this PR) It seems like a big hole that we aren't able to log things until the app is built. Especially as we add more functionality earlier in the app. Our only options are throw or eat...

Another cause for this to not succeed is if appHostAssembly is null. Do we need to support that scenario? It feels odd that we would Debug.WriteLine if someone passed null (which we claim to support). We basically wouldn't do anything.

Logged #4597 to track enabling logging pre-host build.

Another cause for this to not succeed is if appHostAssembly is null. Do we need to support that scenario?

@eerhardt I made it non-nullable and don't call in now if the appHostAssembly is null for some reason.

Now that it's internal, it is less of a concern. But this sounds good.

…-secrets

mitchdenny · 2024-06-16T23:57:19Z

Couple of things playing around with this locally.

~~Seems to have broken pgadmin ... is it working for you?~~
~~CatalogDbApp is crashing with an error saying it can't authenticate.~~

PEBCAK

mitchdenny · 2024-06-17T00:19:02Z

This feels like an opportunity to design a better API for parameter defaults.

Are you talking about supporting binding to KeyVault etc?

mitchdenny · 2024-06-17T00:20:04Z

@DamianEdwards I'm pretty happy with this. I made a minor change so that the secrets file is formatted when we save it. I'm not sure I like the IsDevelopment() checks .. so if you could explain your thinking there that would be good.

davidfowl · 2024-06-17T16:48:20Z

@eerhardt can you review please

eerhardt · 2024-06-18T23:17:23Z

        if (!TrySetUserSecret(applicationName, configurationKey, value))
        {
-            throw new DistributedApplicationException($"Failed to set value for parameter '{parameterName}' in application '{applicationName}' to user secrets.");
+            Debug.WriteLine($"Failed to set value for parameter '{parameterName}' in application '{applicationName}' to user secrets.");


Would it be possible for us to get the ability to log here? (obviously not in this PR) It seems like a big hole that we aren't able to log things until the app is built. Especially as we add more functionality earlier in the app. Our only options are throw or eat...

eerhardt · 2024-06-18T23:29:37Z

        if (!TrySetUserSecret(applicationName, configurationKey, value))
        {
-            throw new DistributedApplicationException($"Failed to set value for parameter '{parameterName}' in application '{applicationName}' to user secrets.");
+            Debug.WriteLine($"Failed to set value for parameter '{parameterName}' in application '{applicationName}' to user secrets.");


Another cause for this to not succeed is if appHostAssembly is null. Do we need to support that scenario? It feels odd that we would Debug.WriteLine if someone passed null (which we claim to support). We basically wouldn't do anything.

eerhardt

LGTM.

Nice work! This is going to solve a lot of problems with container persistence.

davidfowl · 2024-06-20T07:41:11Z

+    /// <summary>
+    /// The assembly of the app host.
+    /// </summary>
+    Assembly? AppHostAssembly { get; }


Is this new public API?

davidfowl · 2024-06-20T07:41:47Z

    public string AppHostDirectory { get; }

+    /// <inheritdoc />
+    public Assembly? AppHostAssembly => _options.Assembly;


Do we really need to expose this as public API?

We can revert but it's kinda useful to have. I think originally it was required when the user secrets stuff was public and each resource extension method was doing the logic to add the user secrets parameter default, but it could all be internal now.

Logged #4612

Save password parameter default values to user secrets

03ffc39

#1151

DamianEdwards requested a review from eerhardt June 14, 2024 01:32

ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 14, 2024

DamianEdwards requested review from davidfowl and mitchdenny June 14, 2024 01:32

Fix copy pasta

950b524

DamianEdwards added 5 commits June 14, 2024 09:34

Don't throw if parameter default can't be saved in user secrets

13082f3

Revert adding constant

882091c

Update PublicAPI.Unshipped.txt

b033cdf

Flow AppHost assembly from options to IDistributedApplicationBuilder

3031a89

Add tests

8212d9f

DamianEdwards marked this pull request as ready for review June 14, 2024 20:54

DamianEdwards commented Jun 14, 2024

View reviewed changes

Fix PublicAPI.Unshipped

7661262

davidfowl reviewed Jun 14, 2024

View reviewed changes

DamianEdwards added 2 commits June 14, 2024 14:08

More tests

3c61052

Merge branch 'main' into damianedwards/save-default-passwords-in-user…

6f360bb

…-secrets

mitchdenny reviewed Jun 17, 2024

View reviewed changes

Comment thread src/Aspire.Hosting.MySql/MySqlBuilderExtensions.cs Outdated

Save the file with formatting.

d77d9ba

Remove environment check

e11e23f

eerhardt reviewed Jun 18, 2024

View reviewed changes

PR feedback

c80a1cf

DamianEdwards requested a review from eerhardt June 19, 2024 01:07

eerhardt reviewed Jun 19, 2024

View reviewed changes

DamianEdwards mentioned this pull request Jun 19, 2024

Update Azure provisioning to use SecretsStore #4596

Open

DamianEdwards added 2 commits June 19, 2024 10:32

PR feedback

a60988c

PR feedback

742a369

eerhardt approved these changes Jun 19, 2024

View reviewed changes

DamianEdwards added 2 commits June 19, 2024 11:34

Add test for comments in user secrets file case

b14eb1b

Remove the Lazy

a144937

DamianEdwards merged commit 4e06e32 into main Jun 19, 2024

DamianEdwards deleted the damianedwards/save-default-passwords-in-user-secrets branch June 19, 2024 20:22

davidfowl reviewed Jun 20, 2024

View reviewed changes

github-actions Bot locked and limited conversation to collaborators Jul 21, 2024

Conversation

DamianEdwards commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

davidfowl commented Jun 14, 2024

Uh oh!

DamianEdwards commented Jun 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DamianEdwards Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchdenny commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mitchdenny commented Jun 17, 2024

Uh oh!

mitchdenny commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Jun 17, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DamianEdwards commented Jun 14, 2024 •

edited

Loading

DamianEdwards Jun 14, 2024 •

edited

Loading

mitchdenny commented Jun 16, 2024 •

edited

Loading

mitchdenny commented Jun 17, 2024 •

edited

Loading

davidfowl Jun 20, 2024 •

edited

Loading