Skip to content

PMM-14979 Never replace an existing PMM encryption key - #921

Draft
ademidoff wants to merge 1 commit into
mainfrom
PMM-14979-share-enc-key-between-nodes
Draft

PMM-14979 Never replace an existing PMM encryption key#921
ademidoff wants to merge 1 commit into
mainfrom
PMM-14979-share-enc-key-between-nodes

Conversation

@ademidoff

@ademidoff ademidoff commented Aug 5, 2026

Copy link
Copy Markdown
Member

Ticket: PMM-14979

Chart side of the same ticket as percona/pmm#5735, which carries the pmm-managed and documentation changes.

Problem

PMM encrypts the credentials of monitored services, and they can only be read back with the key they were encrypted with. Both charts could give a replica a key that does not match the database.

pmm-ha created the pg-encryption-key secret with kubectl create --dry-run | kubectl apply, which overwrites an existing one. The secret is created by kubectl rather than Helm, so it is not owned by the release and outlives helm uninstall. Reinstalling over retained pg-db storage therefore replaced the key while the data it had encrypted remained, making every stored credential unreadable — the same failure class as the forum report behind this ticket.

pmm kept the key only on the data volume at /srv/pmm-encryption.key. That survives restarts and upgrades but not loss of the volume, which is recoverable data loss when PMM is pointed at an external PostgreSQL that outlived it.

Changes

charts/pmm-ha/templates/pmm-secret-init-job.yaml keeps an existing secret and only generates a key when there is none.

charts/pmm gains two init containers that keep the volume and a secret in sync, with adopt-not-generate semantics:

State Action
Neither exists Generate, install on volume, create secret
Both exist Nothing
Key on volume, no secret Adopt the volume's key into the secret
Secret only (volume rebuilt) Restore from the secret, do not generate

The third row is what makes this safe to upgrade into: an existing release has its key adopted, never replaced. Reaching the API server is required rather than best-effort, because continuing without it would let PMM generate a key of its own and leave the stored credentials unreadable.

The new permission is a namespaced Role granting get and create on secrets (charts/pmm/templates/encryption-key-rbac.yaml). The chart's existing ClusterRole already allows reading secrets cluster-wide, so nothing cluster-scoped is added. It can be turned off with encryptionKey.backupToSecret=false.

Versions bumped: pmm 1.9.0 -> 1.9.1, pmm-ha 1.6.0 -> 1.6.1.

Note for reviewers

Because serviceAccount.create defaults to false, the Role binds to the namespace's default ServiceAccount, so other pods using it in that namespace also gain the permission. An init container cannot use a separate ServiceAccount, and a pre-install hook Job cannot read the ReadWriteOnce data volume, so this is inherent to the approach; serviceAccount.create=true scopes it to a PMM-only account.

Flipping that default is not advisable as-is: in this chart it also creates a ClusterRole granting cluster-wide read on secrets, a static ServiceAccount token, and activates the currently inert pmmEnv.ENABLE_CLUSTER_ROLE_ADMIN path that binds cluster-admin. Decoupling ServiceAccount creation from that cluster RBAC is worth a separate ticket.

Testing

  • helm lint passes for both charts; ct lint passes for pmm up to its yamllint step, which could not run locally (not installed, pip blocked by PEP 668) — the added YAML follows the existing file conventions but CI has the last word.
  • helm template verified for both charts, including encryptionKey.backupToSecret=false rendering nothing and user-supplied initContainers still working.
  • The embedded reconcile shell was exercised against a stubbed kubectl for all four states above, confirming a pre-existing key is adopted rather than overwritten and a rebuilt volume is restored rather than regenerated.
  • Not covered: no live helm install/upgrade against a cluster. The kind-based install in CI covers pmm; charts/pmm-ha changes run through .github/workflows/pmm-ha-pr-checks.yaml.

PMM encrypts the credentials of monitored services, and those credentials
can only be read back with the key they were encrypted with. Both charts
could hand a replica a key that does not match the database.

pmm-ha created the pg-encryption-key secret with `kubectl apply`, which
overwrites it. The secret is not owned by the Helm release, so it outlives
`helm uninstall`: reinstalling over retained pg-db storage replaced the key
while the data it had encrypted remained, leaving every stored credential
unreadable. The job now keeps an existing secret and only generates a key
when there is none.

pmm kept the key solely on the data volume at /srv/pmm-encryption.key. That
survives restarts and upgrades, but not loss of the volume, which is
recoverable data loss when PMM is pointed at an external PostgreSQL that
outlived it. Two init containers now keep the volume and a secret in sync:
whichever already holds a key wins, so an existing installation has its key
adopted rather than replaced, and a rebuilt volume is restored from the
secret instead of getting a freshly generated key. Reaching the API server
is required, because continuing without it would let PMM generate a key of
its own and leave the stored credentials unreadable.

The extra permission is a namespaced Role granting get and create on
secrets. The chart's ClusterRole already allows reading them cluster-wide,
so nothing cluster-scoped is added.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the pmm and pmm-ha Helm charts to ensure PMM encryption keys are never replaced unintentionally, adding “adopt/restore” behavior so existing encrypted credentials remain readable across reinstalls, upgrades, or volume loss scenarios.

Changes:

  • pmm-ha: stop overwriting pg-encryption-key; only create it when absent.
  • pmm: add init-container based reconciliation to keep /srv/pmm-encryption.key and a backup Secret in sync with “adopt-not-generate” semantics.
  • Add namespaced RBAC for Secret get/create and bump chart versions.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
charts/pmm/values.yaml Adds configurable encryptionKey settings for secret backup and kubectl image.
charts/pmm/templates/statefulset.yaml Introduces init containers to generate/reconcile encryption key between volume and Secret.
charts/pmm/templates/encryption-key-rbac.yaml Adds a Role/RoleBinding allowing Secret get/create in the release namespace.
charts/pmm/templates/_helpers.tpl Adds helper to compute the encryption key Secret name.
charts/pmm/README.md Documents encryption key backup/restore behavior and new values.
charts/pmm/Chart.yaml Bumps chart version to 1.9.1.
charts/pmm-ha/templates/pmm-secret-init-job.yaml Ensures existing pg-encryption-key is preserved; avoids apply/overwrite behavior.
charts/pmm-ha/Chart.yaml Bumps chart version to 1.6.1.
Suppressed comments (1)

charts/pmm/templates/statefulset.yaml:136

  • Same permissions concern as above: installing the generated key sets mode 0644. Consider using 0600 so the key is not world-readable within the pod/volume.
              chmod 644 "$KEY_FILE"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

echo "Error: secret $SECRET holds no key" >&2
exit 1
fi
chmod 644 "$KEY_FILE"
Comment thread charts/pmm/README.md
The secret is not owned by the Helm release, so it outlives `helm uninstall`. Back it up together with the rest of your PMM configuration:

```sh
kubectl get secret pmm-encryption-key -o yaml > pmm-encryption-key-backup.yaml
Comment on lines +105 to +109
if kubectl get secret "$SECRET" --namespace="$NAMESPACE" >/dev/null 2>&1; then
SECRET_EXISTS=yes
else
SECRET_EXISTS=no
fi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants