Repository navigation
feat: support Azure Managed Identity and Workload Identity authentication - #34
Merged
Merged
Conversation
- align appVersion to 0.3.0 in Helm charts - add [Unreleased] section and compare links to CHANGELOG.md - pass Helm login credentials via environment variables and upgrade checkout to v7
…tion - add AzureTokenProvider in postgres package using azidentity.DefaultAzureCredential - implement pgxpool BeforeConnect token refresh for dynamic Entra ID tokens - support specifying database user via POSTGRES_USER or connection DSN - configure connection recycling with POSTGRES_MAX_CONN_LIFETIME (default 45m for Azure auth) - decouple schema migrations with POSTGRES_MIGRATIONS_AUTH_TYPE and POSTGRES_RUN_MIGRATIONS - add POSTGRES_MIGRATE_ONLY flag to support dedicated migration jobs - update Helm charts with Azure Workload Identity configuration and examples - add unit tests covering Azure token provider, BeforeConnect hook, and options
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect migration-only execution, migration DSN user handling, and Azure client-ID selection.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Azure Managed Identity and Workload Identity authentication for PostgreSQL, with token refresh, migration controls, and Helm support.
Changes:
- Added Azure token acquisition and pgx connection-pool integration.
- Added configurable migration DSNs, users, skip, and migrate-only modes.
- Updated Helm charts, documentation, dependencies, release metadata, and publishing workflow.
File summaries
| File | Summary and review findings |
|---|---|
README.md |
Updated installation version. |
internal/server/storage/postgres/store.go |
Added token-aware pool configuration. |
internal/server/storage/postgres/options.go |
Added database and migration options. |
internal/server/storage/postgres/migrate.go |
Added authenticated migration connections. Nit (1 vote): migration and confirm-retirement wrappers duplicate the existing postgres: error prefix. |
internal/server/storage/postgres/auth.go |
Added Azure token provider. Moderate (3 votes): explicitly supplied ClientID can be ignored when AZURE_CLIENT_ID is set. |
internal/server/storage/postgres/auth_test.go |
Added authentication tests. Nit (3 votes): the test bypasses Open and may not verify hook, token, or lifetime configuration. |
infra/README.md |
Updated deployment version. |
infra/charts/spool-rack/values.yaml |
Added Azure and migration settings. |
infra/charts/spool-rack/values.schema.json |
Extended the Helm values schema. |
infra/charts/spool-rack/templates/deployment.yaml |
Added authentication and migration environment wiring. |
infra/charts/spool-rack/README.md |
Documented Azure configuration. |
infra/charts/spool-rack/examples/values-azure-workload-identity.yaml |
Added Workload Identity example. |
infra/charts/spool-rack/Chart.yaml |
Updated chart metadata. Nit (3 votes): default image tag 0.3.0 conflicts with the binary’s hardcoded 0.1.0-mvp version. |
go.work.sum |
Updated workspace checksums. |
go.sum |
Added Azure dependency checksums. |
go.mod |
Added Azure Identity dependencies. |
docker-compose.yml |
Updated default image version. |
cmd/spool-rack/main.go |
Added authentication and migration modes. Moderate (3 votes): migration-only execution with only POSTGRES_MIGRATIONS_DSN skips migrations and starts the HTTP server. Moderate (3 votes): migration DSN fallback can override an independent DSN’s user with POSTGRES_USER (lines 93 and 208). |
cmd/spool-rack/main_test.go |
Added authentication helper tests. |
cmd/spool-rack/go.sum |
Updated command-module checksums. |
cmd/spool-rack/go.mod |
Updated command-module dependencies. |
charts/spool-rack/values.yaml |
Added Azure and migration settings. |
charts/spool-rack/values.schema.json |
Mirrored Helm schema changes. |
charts/spool-rack/templates/deployment.yaml |
Mirrored authentication and migration environment wiring. |
charts/spool-rack/README.md |
Documented Azure configuration. |
charts/spool-rack/examples/values-azure-workload-identity.yaml |
Added Workload Identity example. |
charts/spool-rack/Chart.yaml |
Updated chart metadata. Nit (3 votes): default image tag 0.3.0 conflicts with the binary’s hardcoded 0.1.0-mvp version. |
CHANGELOG.md |
Documented the authentication features. |
.github/workflows/publish-chart.yml |
Updated chart publishing workflow. |
Review details
Suppressed comments (3)
cmd/spool-rack/main.go:216
- The dev-seeding path repeats the same override: with a separate
POSTGRES_MIGRATIONS_DSN, it falls back toPOSTGRES_USER/POSTGRES_AZURE_USERand replaces the migration DSN's own user. This makes an independently authenticated migration DSN ineffective for seeding unless another environment variable is supplied; preserve the embedded user whenever the DSNs differ.
dbUser := os.Getenv("POSTGRES_MIGRATIONS_USER")
if dbUser == "" {
dbUser = os.Getenv("POSTGRES_USER")
}
if dbUser == "" {
dbUser = os.Getenv("POSTGRES_AZURE_USER")
}
if dbUser != "" {
seedOpts = append(seedOpts, postgres.WithMigrateUser(dbUser))
internal/server/storage/postgres/migrate.go:266
Connectalready prefixes its errors withpostgres:, so this wrapper now produces messages such aspostgres: migrate: postgres: connect: ...instead of the previouspostgres: migrate: connect: .... Avoid duplicating the package prefix while retaining the migration context.
return nil, fmt.Errorf("postgres: migrate: %w", err)
internal/server/storage/postgres/migrate.go:379
Connectalready prefixes its errors withpostgres:, so this wrapper now produces messages such aspostgres: confirm retirement: postgres: connect: .... Avoid duplicating the package prefix while retaining the confirm-retirement context.
return fmt.Errorf("postgres: confirm retirement: %w", err)
- Files reviewed: 27/29 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
All Copilot review comments have been addressed in commit
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds dual authentication support to Spool Rack, allowing it to authenticate to PostgreSQL databases using either standard password-based authentication (default, backwards-compatible) or Microsoft Entra ID OAuth2 access tokens via Azure Workload Identity in AKS, VM-based Managed Identity, or local Azure CLI logins.
Key Changes
AzureTokenProviderusingazidentity.NewDefaultAzureCredentialto acquire tokens for the PostgreSQL resource scopehttps://ossrdbms-aad.database.windows.net/.default.BeforeConnecthook onpgxpool.Poolto dynamically fetch and assign fresh OAuth2 tokens on every new connection opened by the pool.MaxConnLifetimeconfiguration (defaulting to 45m with Azure auth) to cycle pooled connections before standard 60-90 minute token expiration.id-ixs-rng-dev-em20-spl-02) viaPOSTGRES_USERor directly in the connection DSN.POSTGRES_MIGRATIONS_DSN), or skipping startup migrations (POSTGRES_RUN_MIGRATIONS=false) when executed in a separate CI/CD step or dedicated Kubernetes Job (POSTGRES_MIGRATE_ONLY=true).serviceAccount.automount: true, Workload Identity annotations and pod labels), direct passwordless DSNs, and addedvalues-azure-workload-identity.yamlexample.postgrespackage for token acquisition, mock credentials, error handling, andBeforeConnectpassword assignment, plus helper tests incmd/spool-rack.Verification
make all: Passed (tidy-check,lintwith 0 issues, unit tests, race detection, build).helm lint charts/spool-rackandhelm lint infra/charts/spool-rack: 0 failures.helm templatevalidated for both default values andvalues-azure-workload-identity.yaml.