Warn when persistent containers lack user secrets configuration#14491
Warn when persistent containers lack user secrets configuration#14491sebastienros wants to merge 2 commits intorelease/13.2from
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14491Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14491" |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #14466 where persistent containers are incorrectly recreated on every restart. The root cause is that when the AppHost project lacks user secrets configuration (UserSecretsId), generated parameter values (like passwords) cannot be persisted and are regenerated on each restart, causing DCP to compute a different lifecycle key and recreate the container.
Changes:
- Adds a
BeforeStartEventhandler that detects persistent containers when user secrets are not configured and logs a warning - Registers the warning handler only in run mode to guide developers to configure user secrets
- Provides comprehensive test coverage for the warning behavior
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
BuiltInDistributedApplicationEventSubscriptionHandlers.cs |
Adds WarnPersistentContainersWithoutUserSecrets handler that checks for persistent containers when user secrets are unavailable |
DistributedApplicationBuilder.cs |
Registers the new warning handler in run mode |
MessageStrings.resx |
Adds the warning message resource string |
MessageStrings.Designer.cs |
Auto-generated designer code for the new message string |
MessageStrings.*.xlf (13 files) |
Localization files updated with the new warning message (marked as needing translation) |
PersistentContainerWarningTests.cs |
Comprehensive tests covering warning/no-warning scenarios for persistent vs session containers and with/without user secrets |
Files not reviewed (1)
- src/Aspire.Hosting/Resources/MessageStrings.Designer.cs: Language not supported
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22002036620 |
| { | ||
| if (resource.GetContainerLifetimeType() == ContainerLifetime.Persistent) | ||
| { | ||
| logger.LogWarning(MessageStrings.PersistentContainerWithoutUserSecrets, resource.Name); |
There was a problem hiding this comment.
@copilot check to ensure the warning log level is enabled before logging
|
@DamianEdwards I've opened a new pull request, #14496, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Check warning log level before logging persistent container warning Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
| if (resource.GetContainerLifetimeType() == ContainerLifetime.Persistent) | ||
| { | ||
| logger.LogWarning(MessageStrings.PersistentContainerWithoutUserSecrets, resource.Name); | ||
| if (logger.IsEnabled(LogLevel.Warning)) |
There was a problem hiding this comment.
@DamianEdwards FYI, there is an anlyzer in 10.0.103 (an update) that would warn when it's recommended. In this case it would trigger because it's a Warning and would not (in theory) happen in a hot path. It would for anything lower though. And the "expensive" thing in this code is the fact that it's calling params string[] so it's creating an array. Not obvious, right? c.f. dotnet/sdk#53030
There was a problem hiding this comment.
Not quite following. Is the recommended pattern as simple as
always guard
LogXXXcalls with an appropriate call toIsEnabled(LogLevel.XXX)
That's my mental model today.
There was a problem hiding this comment.
What you did is totally fine, just saying that even the analyzer doesn't flag some uses because they really don't matter in non-hot paths.... e.g. when using LogWarning like here.
It's safe to just apply it everywhere and not have to think about it.
Summary
Fixes #14466
When a container resource has
ContainerLifetime.Persistentbut the AppHost project doesn't have user secrets configured (UserSecretsId), generated parameter values (such as passwords) cannot be persisted. On each restart, a new password is generated, which changes the container's environment variables, causing DCP to compute a different lifecycle key — triggering a full container recreation instead of reusing the persistent container.This PR adds a
BeforeStartEventhandler that detects this misconfiguration and logs a warning per affected persistent container, guiding users to rundotnet user-secrets initin the AppHost directory.Root Cause Chain
RunAsEmulator()(e.g., Service Bus) creates sub-resources withCreateDefaultPasswordParameter()CreateGeneratedParameter()wraps the default inUserSecretsParameterDefaultUserSecretsParameterDefault.GetDefaultValue()callsTrySetSecret()→ returnsfalsesilently viaNoopUserSecretsManagerChanges
BuiltInDistributedApplicationEventSubscriptionHandlers.csWarnPersistentContainersWithoutUserSecretshandlerDistributedApplicationBuilder.csBeforeStartEvent(run-mode only)MessageStrings.resx+.Designer.csPersistentContainerWarningTests.csKnown Limitations
Containers with persistent storage but session lifetime are not covered. Containers using
WithDataVolume()orWithBindMount()withoutContainerLifetime.Persistentalso persist state (e.g., initialized databases retain old passwords), but won't receive this warning since they haveContainerLifetime.Session. This is a different problem scope worth a follow-up issue.False positives for persistent containers without generated secrets. The warning fires for any persistent container when user secrets aren't configured, even if the container doesn't use generated parameters (e.g., a Redis container with no password). This was a deliberate trade-off for simplicity — having user secrets configured is good practice for any project using persistent containers, and the warning is actionable.