-
Notifications
You must be signed in to change notification settings - Fork 464
adding a read-only ContainerApps secret repository #11202
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ContainerAppsSecretsRepository
as a new read-only secrets repository implementation - Modifies
SecretManager
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
3dd38cf
to
3bf0a43
Compare
3bf0a43
to
bba7bad
Compare
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-proc
branch to be included in Core Tools and non-Flex deployments.in-proc
branch is not requiredrelease_notes.md