fix(shard): apply default runtime identity to any pool image - #628
Open
graveland wants to merge 2 commits into
Open
fix(shard): apply default runtime identity to any pool image#628graveland wants to merge 2 commits into
graveland wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
graveland
force-pushed
the
fix/pg-identity
branch
from
September 4, 2026 16:50
9763184 to
78d9e25
Compare
This comment has been minimized.
This comment has been minimized.
effectivePgctldIdentity supplied the numeric UID/GID only when the image string was exactly DefaultPostgresImage, so overriding spec.images.postgres left containers carrying RunAsNonRoot with no RunAsUser. pgctld declares USER postgres by name, so the kubelet cannot prove the user is not root and every pool pod fails: CreateContainerConfigError: container has runAsNonRoot and image has non-numeric user (postgres) Reproduced with ghcr.io/multigres/pgctld:main, which differs from the pinned sha- tag by reference alone: the pool never converges and the e2e suite times out. The constant's own comment already gave the reason — pgctld declares USER by name — and that holds for every pgctld build rather than one tag. buildPostgresExporterContainer already supplies its identity unconditionally. effectiveMultipoolerIdentity carried the same gate, latent only because the multigres image uses numeric USER 65532. Leaving it would break the invariant validatePoolRuntimeIdentity enforces, that postgres and multipooler share a UID to reach PGDATA. An image that runs as a different numeric user must now set pool.Postgres.RunAsUser rather than inheriting its identity implicitly. Closes #466 Signed-off-by: Brent Graveland <graveland@supabase.io>
graveland
force-pushed
the
fix/pg-identity
branch
from
September 10, 2026 21:21
78d9e25 to
93fe05e
Compare
This comment has been minimized.
This comment has been minimized.
graveland
marked this pull request as ready for review
September 10, 2026 21:31
Verolop
reviewed
Sep 10, 2026
Verolop
left a comment
Verolop
left a comment
Contributor
There was a problem hiding this comment.
Thanks, this makes sense!
Before merging, can you please update the API comments and generated CRDs too? They still say unset IDs use the image defaults. While you're there, also mention that custom images may now need explicit runAsUser and runAsGroup, especially with existing volumes.
regarding auditing IDs, yes please.
Also note Alpine is only the builder, and the Debian img already sets postgres to 999:999, so maybe keep that for the containers sharing PGDATA and handle the broader audit separately, WDYT?
The API comments and generated CRDs still said an unset identity falls back to the image's own user. Since #628, Postgres and Multipooler get an operator-supplied default UID/GID instead, so update the comment to say that and call out that a custom image needing a different user must now set this explicitly, including when reusing a PersistentVolume whose data is already owned by another UID. Regenerated via `make manifests generate`. Signed-off-by: Brent Graveland <graveland@supabase.io>
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
Author
Good catch! I pushed a doc update.
yeah, auditing is definitely separate! |
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
2 participants
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.
What changed
Pool
postgres/multipoolercontainers now always get a default numericRunAsUser/RunAsGroupunless the CR sets one explicitly, instead of only when the image string exactly matches the pinned default.Why
The old gate meant the default identity applied only on an exact string match against
DefaultPostgresImage. Any drift — an image override, or just:mainvs. the pinnedsha-tag — leftRunAsNonRootset with noRunAsUser.pgctlddeclaresUSER postgresby name, so Kubernetes can't verify non-root and the pod fails:Reproduces on stock
minimal.yaml, no override needed. Closes #466.Notable details / risks
effectiveMultipoolerIdentityhad the same gate, latent only because the default multigres image happens to use numeric UID 65532 — fixed the same way, preserving the postgres/multipooler shared-UID invariantvalidatePoolRuntimeIdentityenforces. An image needing a different numeric user must now setpool.Postgres.RunAsUserexplicitly; it's no longer inferred.One thing I did notice because of this: our postgresql images are alpine-based, and the add-user command in them don't specify a numeric ID, but just take whatever alpine has next available. We should probably think about the user/group ID, and make it static, and audit the
postgresuser in all containers across the whole fleet and ideally consolidate them?