adding a read-only ContainerApps secret repository #11202
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for Container Apps secrets by implementing a read-only secrets repository that reads secrets from mounted files in the container filesystem, transitioning from blob storage to Container Apps native secret management.
- Introduces
ContainerAppsSecretsRepositoryas a new read-only secrets repository implementation - Modifies
SecretManagerto handle nullable master keys for scenarios where secrets don't exist - Updates the secret manager provider to support the new "containerapps" secret storage type
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ContainerAppsSecretsRepository.cs | New read-only repository implementation that reads secrets from /run/secrets/functions-keys directory |
| DefaultSecretManagerProvider.cs | Adds support for "containerapps" secret storage type and instantiation of the new repository |
| SecretManager.cs | Updates to handle nullable master keys and removes unused import |
| ContainerAppsSecretsRepositoryTests.cs | Comprehensive test suite covering secret reading, empty scenarios, and SecretManager integration |
| // before caching any secrets, validate them | ||
| string masterKeyValue = hostSecrets.MasterKey.Value; | ||
| string masterKeyValue = hostSecrets.MasterKey?.Value; |
There was a problem hiding this comment.
The change to make masterKeyValue nullable without updating the ValidateHostSecrets method call could cause issues. The ValidateHostSecrets method may not be designed to handle null masterKeyValue parameter.
Copilot uses AI. Check for mistakes.
src/WebJobs.Script.WebHost/Security/KeyManagement/ContainerAppsSecretsRepository.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| // Always return a HostSecrets object, even if empty. This will prevent the SecretManager from thinking |
There was a problem hiding this comment.
So who is responsible for ensuring that require keys like the master key are present? What happens if the key isn't present but /admin APIs are called by control plane?
src/WebJobs.Script.WebHost/Security/KeyManagement/ContainerAppsSecretsRepository.cs
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Security/KeyManagement/ContainerAppsSecretsRepository.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Security/KeyManagement/ContainerAppsSecretsRepository.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Security/KeyManagement/ContainerAppsSecretsRepository.cs
Show resolved
Hide resolved
brettsam
force-pushed
the
brettsam/env-secrets
branch
2 times, most recently
from
3dd38cf to
3bf0a43
Compare
brettsam
force-pushed
the
brettsam/env-secrets
branch
from
3bf0a43 to
bba7bad
Compare
| } | ||
|
|
||
| public Task WriteAsync(ScriptSecretsType type, string functionName, ScriptSecrets secrets) | ||
| => throw new NotImplementedException(); |
There was a problem hiding this comment.
Should this be NotSupportedException with a message saying this is a read-only repository?
| { | ||
| if (type == ScriptSecretsType.Function && string.IsNullOrEmpty(functionName)) | ||
| { | ||
| throw new ArgumentNullException(nameof(functionName), $"{nameof(functionName)} cannot be null or empty with {nameof(type)} = {nameof(ScriptSecretsType.Function)}"); |
There was a problem hiding this comment.
very minor nit: technically you throw ArgumentNullException when null and ArgumentException when empty.
There is a helper for that: ArgumentException.ThrowIfNullOrEmpty
if (type == ScriptSecretsType.Function)
{
ArgumentException.ThrowIfNullOrEmpty(functionName);
}|
|
||
| private async Task<ScriptSecrets> ReadHostSecretsAsync() | ||
| { | ||
| var secrets = await GetFromFilesAsync(ContainerAppsSecretsDir); |
There was a problem hiding this comment.
nit: prefer actual types over var
| { | ||
| var secrets = await GetFromFilesAsync(ContainerAppsSecretsDir); | ||
|
|
||
| HostSecrets hostSecrets = new HostSecrets() |
There was a problem hiding this comment.
nit: can be made more concise
| HostSecrets hostSecrets = new HostSecrets() | |
| HostSecrets hostSecrets = new() |
|
|
||
| private async Task<IDictionary<string, string>> GetFromFilesAsync(string path) | ||
| { | ||
| string[] files = await FileUtility.GetFilesAsync(path, "*"); |
There was a problem hiding this comment.
For testability, can you inject a IFileSystem? Not sure if we have that type in the DI container. But would be helpful to add it if not there.
This way you can mock the file system and not need to perform any actual I/O in tests.
| sb.AppendLine($" {file}"); | ||
| } | ||
|
|
||
| _logger.LogDebug(sb.ToString()); |
There was a problem hiding this comment.
nit: not sure if we have a performance concern here, but you could do a _logger.IsEnabled(LogLevel.Debug) check and no-op all logging if it is false. This will avoid the string builder allocation.
| /// no-op - allow stale secrets to remain. | ||
| /// </summary> | ||
| public async Task PurgeOldSecretsAsync(IList<string> currentFunctions, ILogger logger) | ||
| => await Task.Yield(); |
There was a problem hiding this comment.
nit: remove async and return Task.CompletedTask.
| /// Runtime is not responsible for encryption so this code will never be executed. | ||
| /// </summary> | ||
| public Task WriteSnapshotAsync(ScriptSecretsType type, string functionName, ScriptSecrets secrets) | ||
| => throw new NotSupportedException(); |
There was a problem hiding this comment.
Can we add a message explaining this is a read-only repository?
fixes #11166
Container Apps currently uses blob storage for it's secrets, which are not manageable easily. We're transitioning secrets to be managed via Container Apps itself. In this case, the secrets will be in a drive in the container the same way as our K8S secrets are.
The main difference here is that these are read-only. We have no mechanism for auto-generating these in Container Apps, so they need to be created separately.
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.md