[just-some-random-pal]

Fixes #716, fixes #514, fixes #277.

On any cluster where admission mutates Pods, vttablet Pods are deleted and recreated on every reconcile, about once a minute. Two cases are reported across the three issues: a namespace whose default ServiceAccount carries imagePullSecrets (the ServiceAccount admission plugin appends them to every Pod), and OpenShift, whose SecurityContextConstraints inject runAsUser, runAsNonRoot and seLinuxOptions into every container. In both cases the operator's desired Pod lacks the injected values, the reconciler sees a diff, and the tablet rolls forever. The pendingChanges in #716 shows exactly that: imagePullSecrets: null and runAsNonRoot: null, runAsUser: null on every container.

Two places overwrote instead of merged:

• vttablet.UpdatePod and etcd.UpdatePod assigned spec.ImagePullSecrets to the Pod, dropping anything admission added. They now use update.LocalObjectReferences, which already exists for this purpose and matches how Env, Volumes and Tolerations are treated.
• update.SecurityContext preserved only procMount, allowPrivilegeEscalation, capabilities, readOnlyRootFilesystem and runAsGroup when the desired context did not set them, and replaced the whole context with nil when the operator had no opinion at all (etcd with --default_etcd_run_as_user=-1, and OpenShift's documented --default_vitess_run_as_user=-1). Directly managed Pods now merge through a new update.PodContainerSecurityContext, which keeps every field the operator does not set and keeps the live context when the desired one is nil. Where the operator does have an opinion, for example the default UID 999, it still wins.

Keeping what the operator doesn't set means a removal from the spec no longer reaches a running Pod through the merge. So the desired-state hash gains two items, recorded the same way as tolerations:

• the desired pull-secret names, so dropping a secret from the spec recreates the Pod;
• the security contexts of user-supplied initContainers and sidecarContainers, so removing a field such as privileged: true from one recreates the Pod with the hardened context.

Each item is skipped when empty, so clusters that set neither see no change.

Upgrade effect. Clusters that set imagePullSecrets on tablets or etcd, or a securityContext on a user-supplied init or sidecar container, get a new desired-state hash once on upgrade, which schedules a recreate of those Pods:

• With updateStrategy: Immediate, tablet Pods roll once.
• With the default External strategy, tablet Pods show pendingChanges until the shard's rollout is released.
• etcd Pods are never released automatically. etcd members go through UpdateRollingRecreate, and only tablet Pods are released, so each affected etcd member keeps a pending change until someone releases it by hand with the rollout.planetscale.com/released annotation.

Deployment templates (vtgate, vtctld, vtorc, vtadmin) keep today's replace semantics through the unchanged update.SecurityContext. Admission mutates Pods, not templates, and nothing but the operator writes a template, so a field the operator stops setting there, such as the default UID after --default_vitess_run_as_user=-1, must be cleared rather than kept. The trade-off on directly managed Pods is that a field the operator stops setting on its own containers, such as the default UID, stays on a running Pod until something recreates it. The hash covers only user-supplied containers, because hashing the operator's own containers would roll every Pod on upgrade, and a fresh Pod picks up the new value.

Tests:

• update: injected runAsUser, runAsNonRoot, seLinuxOptions, seccompProfile survive the Pod merge; a desired value still overrides; a nil desired context keeps the live one; the template merge clears a dropped field and drops the context when the desired one is nil; LocalObjectReferences keeps an injected secret.
• vtgate: a Deployment built with the default UID loses runAsUser from its template after the flag is switched to -1 and UpdateDeployment runs.
• vttablet and etcd: UpdatePod on a Pod carrying an injected pull secret and injected security contexts leaves them in place and is idempotent on a second call, which is the property the reconciler's diff relies on; with the default UID set, the operator's UID still replaces the injected one; removing a pull secret from the spec changes the desired-state hash; removing privileged from a sidecar or init container changes the hash while the merge alone keeps it; user containers without a security context leave the hash unchanged.

All of these fail on main and pass with the change. make unit-test passes locally.

[mattlord]

1. Blocking: I think that we should keep the field-preserving merge for Pods only and leave Deployment templates on the current behavior. update.SecurityContext is shared: besides PodContainer, PodTemplateContainer (pkg/operator/update/podspec.go:168) calls it for the vtgate, vtctld, vtorc and vtadmin Deployments, so those are not untouched. Their templates get runAsUser from --default_vitess_run_as_user, and nothing but the operator writes them, so a field the operator stops setting now stays forever. Concretely, a cluster created with the default UID and then switched to --default_vitess_run_as_user=-1, the OpenShift setup from #716, keeps runAsUser: 999 in all four templates (with PodTemplateContainers, this branch keeps 999 where main clears it), and under a restricted SCC the ReplicaSets then can't create Pods with that UID. Since admission only mutates Pods, it seems like the simpler fix is to use the preserving merge in PodContainer only and keep PodTemplateContainer replacing the context as it does today, plus a template test for that flag change. No?

2. I think that we should correct the new comment on update.SecurityContext (pkg/operator/update/podspec.go:184). It says callers that manage Pods record the desired state in a hash so that removing a field recreates the Pod, but AddContainersUpdates only hashes volume mount paths and resource keys (pkg/operator/contenthash/containers.go:48), not security contexts. So dropping the default UID, for example, leaves runAsUser on running tablet and etcd Pods until something else recreates them. That seems acceptable, since a fresh Pod picks up the new value, and adding security contexts to the hash would roll every tablet on upgrade, but the comment should say that rather than promise a recreate.

The imagePullSecrets side looks right to me: update.LocalObjectReferences plus the pull-secret hash mirrors how tolerations already work, and the one-time roll for users with pull secrets is called out. Thanks for digging into three long-standing issues at once! ❤️

[just-some-random-pal]

Both points taken, thanks for the careful read.

1. You are right that update.SecurityContext was shared with PodTemplateContainer, and the flag-change case you describe would have left runAsUser: 999 in all four Deployment templates. The preserving merge is now update.PodContainerSecurityContext, used only by PodContainer; update.SecurityContext is back to exactly what main does, so templates still replace the context and drop a field the operator stops setting. Added TestSecurityContextClearsFieldsForTemplates, TestPodTemplateContainerClearsDroppedRunAsUser, and a vtgate test that builds a Deployment with the default UID, switches DefaultVitessRunAsUser to -1, runs UpdateDeployment, and asserts the template no longer carries runAsUser.

2. Corrected. The comment on PodContainerSecurityContext now says that a field the operator stops setting stays on a running Pod until something else recreates it, that the desired-state hash does not cover security contexts because hashing them would roll every Pod on upgrade, and that a fresh Pod picks up the new value. I also fixed the PR description, which wrongly claimed the Deployments were untouched.

[mattlord]

1. Blocking: I think that we should make a removal from a user-supplied container's securityContext reach the Pod. PodContainer (pkg/operator/update/podspec.go:122) is also the merge for the user's sidecarContainers and initContainers (pkg/operator/vttablet/pod.go:332-333, and the same in pkg/operator/etcd/pod.go), so with PodContainerSecurityContext (podspec.go:235) a user who deletes privileged: true or runAsUser: 0 from a sidecar now gets a merged Pod identical to the live one. With a sidecar that had privileged: true and runAsUser: 0 and a desired context of just runAsNonRoot: true, PodContainers leaves privileged=true and runAsUser=0 in place on this branch, where main clears both and schedules a recreate. There is no diff, so no pendingChanges, no event, and nothing in status says the hardening never applied, and the desired-state hash can't catch it either because ContainerUpdates (pkg/operator/contenthash/containers.go:48) only hashes mount paths and resource keys. It seems like the simplest fix is to hash the desired security contexts of the user-supplied containers, skipping the item when none are set the way AddTolerations does, so nobody without them rolls on upgrade and a removal recreates the Pod. A vttablet test that drops privileged from a sidecar and expects the hash to change would pin it. No?

2. I think that we should spell out the upgrade effect of the new imagePullSecrets hash item more precisely in the description, since it will likely become the release note. "One rolling recreate" holds for updateStrategy: Immediate, but with the default External strategy tablet Pods with pull secrets show pendingChanges until the shard is released, and etcd Pods are never released automatically (their changes go through UpdateRollingRecreate, and only tablet Pods are released), so every etcd member with pull secrets sits with a pending change until someone releases it by hand.

Thanks! ❤️

[just-some-random-pal]

Thanks, both are addressed in 9bb64de.

1. The desired-state hash now records the security contexts of user-supplied initContainers and sidecarContainers (AddContainerSecurityContexts). They're keyed by container name and serialized as JSON, so unset fields don't count. The item is skipped when no user container sets a context, the way AddTolerations is, so nobody without them rolls on upgrade. A vttablet test reproduces your case: a sidecar and an init container with privileged: true and runAsUser: 0, merged against a desired runAsNonRoot: true. The merge alone keeps privileged, and the hash changes. etcd has the same test, and the comment on PodContainerSecurityContext now says which containers the hash covers.

2. I rewrote the upgrade note in the description. It now says which clusters get a new hash, what happens under Immediate and External, and that each affected etcd member keeps a pending change until it's released by hand.

make unit-test passes.

[mattlord]

1. Blocking: I think that we should drop the imagePullSecrets hash item after all (pkg/operator/vttablet/pod.go:329, pkg/operator/etcd/pod.go:337), which reverses what I said about it in my first review (sorry about that). Every cluster that pulls from a private registry sets imagePullSecrets, so the operator upgrade alone changes the desired-state hash of all of its tablet and etcd Pods. With updateStrategy: Immediate, every shard then rolls, with a planned reparent per shard. With External, every tablet shows pendingChanges until a fleet-wide release. Each etcd member stays pending until someone releases it by hand, and since etcd Pods have no drain, a release deletes the member right away, so releasing them all at once drops topo quorum. What the item buys is a recreate when a pull secret is removed from the spec, but a leftover pull-secret reference on a running Pod is close to harmless: the kubelet only uses it when pulling an image, and a missing secret is just a warning. An added secret still changes the merged Pod and rolls as before. Leaving the extra reference until the next recreate seems like the same trade-off this PR already makes for the operator's own UID. The security-context hash for user containers is different, since a sidecar that stays privileged matters, and far fewer clusters set one, so I'd keep that one. No?

2. I think that we should test the init-container security-context item on its own. TestUpdatePodRemovedSidecarSecurityContextFieldChangesDesiredState (pkg/operator/vttablet/pod_test.go:144, and pkg/operator/etcd/pod_test.go:90) changes the sidecar and the init container together, so removing the init-containers-security-contexts line in either Pod still passes the tests. A case where only the init container loses privileged would pin it. If the pull-secret item stays, TestEmptyValues (pkg/operator/desiredstatehash/builder_test.go:25) should also cover AddLocalObjectReferenceNames with nil and empty lists: dropping its empty check passes today, and would roll every Pod in every cluster on upgrade.

3. I think that we should mention the VitessBackupStorage subcontroller Pod in the description. updateSubcontrollerPod (pkg/controller/vitessbackupstorage/reconcile_subcontroller.go:206) also merges through update.PodContainers, so it now keeps injected security-context fields too, which looks like an improvement, but it isn't listed among the Pods this changes.

Thanks for the quick turnaround on the security-context hash, and for spelling out the upgrade effect so clearly! ❤️