-
Notifications
You must be signed in to change notification settings - Fork 14
Model secrets as a property of PostgreSql Databases #95
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Reshma Abdul Rahim <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>
zachcasper
left a comment
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.
I only reviewed secrets.yaml, postgreSqlDatabases.yaml, and postgresql.bicep.
| resource myApplication 'Applications.Core/Applications@2023-10-01-preview' = { ... } | ||
|
|
||
| resource frontend 'Applications.Core/containers@2023-10-01-preview' = { | ||
| resource frontend 'Radius.Compute/containers@2025-08-01-preview' = { |
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.
Needs to be
connections: {
postgresql: {
source: postgresql.id
| secretName: | ||
| type: string | ||
| description: "(Required) The name of the secret containing the database crdentials" |
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.
| secretName: | |
| type: string | |
| description: "(Required) The name of the secret containing the database crdentials" | |
| secretName: | |
| type: string | |
| description: "(Required) The name of the secret containing the database crdentials" |
I'm wondering if this should be credentials.secretName rather than just secretName. If that clearer?
| var dbSecretName = context.resource.properties.secretName | ||
| var database string = 'postgres_db' | ||
| var tag string = '16-alpine' | ||
| var port = 5432 |
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.
| var port = 5432 | |
| var port = 5432 | |
| var applicationName = context.application != null ? context.application.name : '' | |
| // Extract last segment from environment path for labels | |
| var environmentId = resourceProperties.?environment ?? '' | |
| var environmentParts = environmentId != '' ? split(environmentId, '/') : [] | |
| var environmentName = length(environmentParts) > 0 | |
| ? environmentParts[length(environmentParts) - 1] | |
| : '' | |
| // Extract resource group name | |
| // Index 4 is the resource group name | |
| var resourceGroupName = split(context.resource.id, '/')[4] | |
| // Common labels | |
| var labels = { | |
| 'radapp.io/resource': resourceName | |
| 'radapp.io/application': applicationName | |
| 'radapp.io/environment': environmentName | |
| 'radapp.io/resource-type': replace(context.resource.type, '/', '-') | |
| 'radapp.io/resource-group': resourceGroupName | |
| } |
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.
Should all of these be populated when deploying Recipes? I think we should prioritize this issue -#60 and not have users write so much code to get Radius metadata.
| metadata: { | ||
| name: uniqueName | ||
| name: resourceName | ||
| namespace: namespace |
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.
| namespace: namespace | |
| namespace: namespace | |
| labels: labels |
| name: uniqueName | ||
| } | ||
| name: resourceName | ||
| namespace: namespace |
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.
| namespace: namespace | |
| namespace: namespace | |
| labels: labels |
Co-authored-by: Will Smith <[email protected]> Co-authored-by: Zach Casper <[email protected]> Signed-off-by: Reshma Abdul Rahim <[email protected]>
|
@sk593 and @lakshmimsft - Can you help review updates to the container Recipe? |
This pull request introduces changes to how PostgreSQL database credentials are managed and injected into Kubernetes containers using secrets. The changes refactor the
postgreSqlDatabasesresource to require asecretNameproperty, ensuring that sensitive information like usernames and passwords is stored in Kubernetes secrets rather than as resource properties. The Bicep recipes, documentation, and schema definitions have been updated to reflect and support this new pattern.Short gist - https://gist.github.com/Reshrahim/5ea332db690c623de64c1c85a01071aa
postgreSqlDatabasesresource schema to require thesecretNameproperty and removedusernameandpasswordfrom the resource outputs and required properties.secretNameproperty, removingusernameandpasswordfrom output properties and instead sourcing them from a referenced Kubernetes secret. The deployment now pulls credentials usingvalueFrom.secretKeyRefreferencing the provided secret.username,password) as uppercase environment variables when asecretNameis provided, ensuring injection of credentials into containers. Also fixed this bug Environment variables from Connected resource doesn't get injected properly on the container #92app.bicepfiles to use the new secret-based pattern for PostgreSQL databases and secrets