feat(pg-autogrant): pg autogrant introduction#414
Conversation
Automated chart versioning after merge of PR #414. This PR updates Chart.yaml versions, packages updated charts into docs/interledger, and regenerates the Helm index. Co-authored-by: bosbaber <1615407+bosbaber@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new Helm chart (pg-autogrant) to run scheduled PostgreSQL role/grant reconciliation via a Kubernetes CronJob, using a rendered ConfigMap with SQL scripts and optional Cloud SQL Auth Proxy sidecar integration.
Changes:
- Introduces
pg-autograntchart templates forCronJob+ SQL/entrypointConfigMap. - Provides default
values.yamlfor roles, databases, and grant configuration (including Cloud SQL Proxy settings). - Adds helm-unittest suites covering CronJob structure/proxy behavior and ConfigMap SQL/script rendering.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/pg-autogrant/Chart.yaml | New chart definition with common dependency. |
| charts/pg-autogrant/Chart.lock | Locks the common dependency version/digest. |
| charts/pg-autogrant/charts/common-0.9.0.tgz | Vendored common dependency tarball for the chart. |
| charts/pg-autogrant/values.yaml | Default configuration for schedule, connection/proxy, roles, databases, and grants. |
| charts/pg-autogrant/templates/_helpers.tpl | Helper templates delegating naming/labels to the common library. |
| charts/pg-autogrant/templates/cronjob.yaml | CronJob manifest including main grants container and optional Cloud SQL Proxy sidecar. |
| charts/pg-autogrant/templates/configmap.yaml | Renders SQL grant scripts and an entrypoint shell script into a ConfigMap. |
| charts/pg-autogrant/tests/cronjob_test.yaml | Tests CronJob schedule/history limits/SA/config volume/env wiring. |
| charts/pg-autogrant/tests/cronjob.proxy_test.yaml | Tests proxy sidecar inclusion, args, and security context behavior. |
| charts/pg-autogrant/tests/configmap.roles_test.yaml | Tests role-creation SQL rendering. |
| charts/pg-autogrant/tests/configmap.databases_test.yaml | Tests database-owner grants and per-db SQL file creation. |
| charts/pg-autogrant/tests/configmap.grants_test.yaml | Tests cross-grants and specific grant rendering across levels. |
| charts/pg-autogrant/tests/configmap.entrypoint_test.yaml | Tests entrypoint behavior (psql invocations, readiness, proxy shutdown). |
| name: {{ .Values.credentialsSecret }} | ||
| key: user_name | ||
| - name: PGPASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.credentialsSecret }} |
There was a problem hiding this comment.
credentialsSecret defaults to an empty string in values.yaml, but the CronJob always renders secretKeyRef.name: {{ .Values.credentialsSecret }}. If a user forgets to set it, Helm will produce an invalid manifest (empty Secret name). Consider enforcing it via required (or the repo’s templates/validations.yaml pattern) so installs fail fast with a clear error.
| name: {{ .Values.credentialsSecret }} | |
| key: user_name | |
| - name: PGPASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Values.credentialsSecret }} | |
| name: {{ required "pg-autogrant.credentialsSecret is required" .Values.credentialsSecret | quote }} | |
| key: user_name | |
| - name: PGPASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ required "pg-autogrant.credentialsSecret is required" .Values.credentialsSecret | quote }} |
| - "--structured-logs" | ||
| - "--quitquitquit" | ||
| - "--http-address=0.0.0.0" | ||
| - {{ .Values.cloudSqlProxy.instanceConnectionName | quote }} |
There was a problem hiding this comment.
When cloudSqlProxy.enabled is true (the default), the template always appends instanceConnectionName to the proxy args, but the default value is empty. This will render an invalid proxy invocation. Consider making cloudSqlProxy.instanceConnectionName required when the proxy is enabled (e.g., required guard inside the conditional).
| - {{ .Values.cloudSqlProxy.instanceConnectionName | quote }} | |
| - {{ required "cloudSqlProxy.instanceConnectionName is required when cloudSqlProxy.enabled is true" .Values.cloudSqlProxy.instanceConnectionName | quote }} |
| - "--structured-logs" | ||
| - "--quitquitquit" | ||
| - "--http-address=0.0.0.0" | ||
| - {{ .Values.cloudSqlProxy.instanceConnectionName | quote }} | ||
| ports: | ||
| - containerPort: 9091 | ||
| securityContext: |
There was a problem hiding this comment.
The Cloud SQL Proxy admin/quit endpoint is bound to 0.0.0.0 (--http-address=0.0.0.0), which makes it reachable from outside the pod via the pod IP. Since the entrypoint only needs localhost access, consider binding this to 127.0.0.1 (or otherwise restricting exposure) to reduce the risk of unintended shutdown/inspection.
| DO $$ | ||
| BEGIN | ||
| IF NOT EXISTS (SELECT FROM pg_roles WHERE rolname = '{{ .name }}') THEN | ||
| CREATE ROLE {{ .name }} {{ if .login }}LOGIN{{ else }}NOLOGIN{{ end }}; | ||
| RAISE NOTICE 'Created role {{ .name }}'; |
There was a problem hiding this comment.
Role names from values are interpolated into SQL without identifier quoting/escaping (rolname = '{{ .name }}' and CREATE ROLE {{ .name }}). This can break for valid PostgreSQL role names that require quoting (uppercase, dashes, reserved words) and also allows quote characters in values to produce invalid SQL. Consider consistently using double-quoted identifiers and escaping embedded quotes in both the string literal and identifier contexts (or validating/normalizing allowed names).
| {{- range .Values.crossGrants }} | ||
| {{- $role := .role }} | ||
| {{- range .memberOf }} | ||
| GRANT {{ . }} TO {{ $role }}; |
There was a problem hiding this comment.
Cross-database membership grants render unquoted identifiers (GRANT {{ . }} TO {{ $role }}). This will fail for role names requiring quoting and is susceptible to invalid SQL if values contain special characters. Consider double-quoting/escaping both the granted role and the member role consistently.
| GRANT {{ . }} TO {{ $role }}; | |
| GRANT "{{ replace "\"" "\"\"" . }}" TO "{{ replace "\"" "\"\"" $role }}"; |
| ----------------------------------------------------------- | ||
| {{- range .Values.specificGrants }} | ||
| {{- if eq .level "database" }} | ||
| GRANT {{ join ", " .privileges }} ON DATABASE "{{ .database }}" TO {{ .role }}; |
There was a problem hiding this comment.
Database-level specific grants also render unquoted role identifiers (... TO {{ .role }}), which can break for valid role names that require quoting and can produce invalid SQL if values contain special characters. Consider quoting/escaping the role identifier here the same way you do for database/owner identifiers.
| GRANT {{ join ", " .privileges }} ON DATABASE "{{ .database }}" TO {{ .role }}; | |
| GRANT {{ join ", " .privileges }} ON DATABASE "{{ .database }}" TO "{{ .role }}"; |
|
|
||
| -- Schema grant: {{ .role }} | ||
| GRANT {{ join ", " .privileges }} ON SCHEMA {{ .schema }} TO {{ .role }}; | ||
| {{- end }} | ||
| {{- if and (eq .level "object") (eq .database $dbName) }} | ||
| {{- $grantRole := .role }} | ||
| {{- $grantSchema := .schema }} | ||
| {{- $grantPrivileges := .privileges }} | ||
| {{- range .objectType }} | ||
|
|
||
| -- Object grant: {{ $grantRole }} on {{ . }}S | ||
| GRANT {{ join ", " $grantPrivileges }} ON ALL {{ . }}S IN SCHEMA {{ $grantSchema }} TO {{ $grantRole }}; | ||
| ALTER DEFAULT PRIVILEGES IN SCHEMA {{ $grantSchema }} GRANT {{ join ", " $grantPrivileges }} ON {{ . }}S TO {{ $grantRole }}; |
There was a problem hiding this comment.
Schema/object-level grants interpolate schema and role without quoting (ON SCHEMA {{ .schema }} TO {{ .role }}, and similarly for object grants/default privileges). This can break for schemas/roles that require quoting and makes the generated SQL fragile. Consider double-quoting/escaping schema and role identifiers consistently in these statements.
| -- Schema grant: {{ .role }} | |
| GRANT {{ join ", " .privileges }} ON SCHEMA {{ .schema }} TO {{ .role }}; | |
| {{- end }} | |
| {{- if and (eq .level "object") (eq .database $dbName) }} | |
| {{- $grantRole := .role }} | |
| {{- $grantSchema := .schema }} | |
| {{- $grantPrivileges := .privileges }} | |
| {{- range .objectType }} | |
| -- Object grant: {{ $grantRole }} on {{ . }}S | |
| GRANT {{ join ", " $grantPrivileges }} ON ALL {{ . }}S IN SCHEMA {{ $grantSchema }} TO {{ $grantRole }}; | |
| ALTER DEFAULT PRIVILEGES IN SCHEMA {{ $grantSchema }} GRANT {{ join ", " $grantPrivileges }} ON {{ . }}S TO {{ $grantRole }}; | |
| {{- $quotedSchema := printf "\"%s\"" (replace .schema "\"" "\"\"") }} | |
| {{- $quotedRole := printf "\"%s\"" (replace .role "\"" "\"\"") }} | |
| -- Schema grant: {{ .role }} | |
| GRANT {{ join ", " .privileges }} ON SCHEMA {{ $quotedSchema }} TO {{ $quotedRole }}; | |
| {{- end }} | |
| {{- if and (eq .level "object") (eq .database $dbName) }} | |
| {{- $grantRole := .role }} | |
| {{- $grantSchema := .schema }} | |
| {{- $grantPrivileges := .privileges }} | |
| {{- $quotedGrantRole := printf "\"%s\"" (replace $grantRole "\"" "\"\"") }} | |
| {{- $quotedGrantSchema := printf "\"%s\"" (replace $grantSchema "\"" "\"\"") }} | |
| {{- range .objectType }} | |
| -- Object grant: {{ $grantRole }} on {{ . }}S | |
| GRANT {{ join ", " $grantPrivileges }} ON ALL {{ . }}S IN SCHEMA {{ $quotedGrantSchema }} TO {{ $quotedGrantRole }}; | |
| ALTER DEFAULT PRIVILEGES IN SCHEMA {{ $quotedGrantSchema }} GRANT {{ join ", " $grantPrivileges }} ON {{ . }}S TO {{ $quotedGrantRole }}; |
| login: true | ||
|
|
||
|
|
||
| # Databases to manage. For each database the chart will assigned the following grants to the database owner role: |
There was a problem hiding this comment.
Typo/grammar in the comment: “the chart will assigned the following grants …” → “the chart will assign …”.
| # Databases to manage. For each database the chart will assigned the following grants to the database owner role: | |
| # Databases to manage. For each database the chart will assign the following grants to the database owner role: |
No description provided.