fix(builder): keep explicitly-set false/0 values when merging CR templates

[AndreyZa]

Summary

removeEmpty() pruned every JSON value equal to the zero value of its Go type, so an explicitly-set false or 0 in a CR pod/container template was dropped from the strategic merge patch and never reached the built workload.

The fields at risk are *bool/*int64 with omitempty in the Kubernetes API types: they are marshalled only when the user set them, so pruning them discarded configuration rather than noise. Nulls, empty strings and empty objects are still pruned, and lists are left exactly as main leaves them — an explicitly empty array is preserved — so nothing beyond booleans and numbers changes.

Concretely, spec.login.securityContext.allowPrivilegeEscalation: false on a LoginSet is stored in the CR but absent from the Deployment, so the login container runs with NoNewPrivs: 0 and a LoginSet cannot satisfy the restricted Pod Security Standard. The chart's own default (loginsetDefaults.login.securityContext.privileged: false) is lost the same way, as are readOnlyRootFilesystem: false, runAsUser: 0 and terminationGracePeriodSeconds: 0.

One clarification on the scope, since the helper is shared by BuildContainer and BuildPodTemplate: the loss happens at the root of the merge — the container itself for BuildContainer, and pod-level fields for BuildPodTemplate. removeEmpty never descended into lists, so a container sitting inside spec.containers[] already came through intact on main.

Closes #252.

Checklist

• I have read the
  CONTRIBUTING.md
  and the
  Code of Conduct.
• New or existing tests cover these changes (where applicable).
• Documentation is updated if user-visible behavior changes. (No documentation change: the fix makes the builders honour values the CRD already documents.)

Breaking Changes

No API or schema change, but the fix is retroactive, and that is worth stating plainly: a false or 0 already sitting in a CR or in chart values is inert today and becomes effective the moment the operator image is replaced. No CR is edited, no chart value changes, nothing shows up in a diff — the same configuration simply starts being honoured.

Mostly that is the point, and it hardens: allowPrivilegeEscalation: false, privileged: false and hostUsers: false start reaching the pod. Two directions deserve a look before upgrading:

• the base specs of Controller, Accounting and RestApi set runAsNonRoot: true with a non-root uid; an explicit runAsNonRoot: false or runAsUser: 0 in a CR now overrides that, where before the operator silently won;
• terminationGracePeriodSeconds: 0 now reaches the pod, which removes the window the slurmd preStop hook uses to drain the node.

A grep for zero-valued securityContext fields and for terminationGracePeriodSeconds across CRs and values is enough to see whether a given deployment is affected. A deployment that relied on an explicit false being silently ignored would change, but that would be the bug itself.

Testing Notes

• internal/utils/structutils/kube_test.go gains a subtest that merges at both roots the builders use. A pod template carrying automountServiceAccountToken: false, terminationGracePeriodSeconds: 0 and securityContext.runAsUser: 0, and a container — the root BuildContainer merges at — carrying privileged: false, allowPrivilegeEscalation: false and readOnlyRootFilesystem: false over a base that sets them true. Both halves fail on main and pass with this change. make test is green (50 packages) and golangci-lint run ./internal/... reports nothing from these files.

• List handling is deliberately untouched: removeEmpty has no list branch at all, so an explicitly empty array survives here exactly as it does on main.

• A known limit this does not change: non-pointer bool fields such as hostNetwork, hostPID and hostIPC, and non-pointer empty strings, are dropped by omitempty during json.Marshal, before removeEmpty ever sees them. No base spec sets those to true today, so there is nothing to override, but the class is not fully gone.

• Verified on a cluster (Kubernetes v1.35.8, charts slurm-operator-1.2.1 / slurm-1.2.1, one LoginSet declaring all five fields; only the operator image swapped):

  Before (ghcr.io/slinkyproject/slurm-operator:1.2.1):

  container.securityContext: {"capabilities":{"add":[...],"drop":["ALL"]}}
  pod.terminationGracePeriodSeconds: 30
  pod.securityContext: {}
  

  After (this branch):

  container.securityContext: {"allowPrivilegeEscalation":false,"capabilities":{"add":[...],"drop":["ALL"]},"privileged":false,"readOnlyRootFilesystem":false}
  pod.terminationGracePeriodSeconds: 0
  pod.securityContext: {"runAsUser":0}
  

  and in the login pod: NoNewPrivs: 1.

Additional Context

Found while trying to run a LoginSet under the restricted Pod Security Standard.

[vivian-hafener]

Please update this PR to use the provided Pull Request Template: https://github.com/SlinkyProject/slurm-operator/blob/main/.github/pull_request_template.md

[AndreyZa]

Hi @vivian-hafener — thanks for the pointer, and sorry for the silence here: I rewrote the description to follow the template the same day (Sep 8), but never said so in the thread.

The body now has the template's Summary / Checklist / Breaking Changes / Testing Notes / Additional Context sections. No code changes since it was opened — the branch is ready for review whenever you have a moment.

[AndreyZa]

Force-pushed a revised commit (cdc1e83 → 7275462) with all three points addressed; replies in the threads.

Two things beyond the review, both found while re-checking the patch:

The test was half-decorative. Its container assertions lived inside Pod.Spec.Containers[], and removeEmpty never recursed into lists — not on main either — so those assertions passed with and without the fix. They are now a second merge at the root BuildContainer actually uses, a bare corev1.Container, which does fail on main. That is also the case the description leads with (allowPrivilegeEscalation: false on a login container), and it covers readOnlyRootFilesystem: false as well. The pod-template half is unchanged and still fails on main.

The Changelog trailer would have failed CI. It was a single 149-character line, over body-max-line-length; the repo's commitlint hook rejects that. It is now folded across continuation lines — git interpret-trailers --parse still reads it as one trailer, and commitlint --strict exits 0.

make test is green (50 packages). golangci-lint run ./internal/... is clean except for a pre-existing misspell in internal/controller/token/token_controller_test.go, which main has already fixed — this branch is based on an older commit and does not touch that file. The two files this PR does touch have not moved on main since the branch point, so it still applies cleanly; happy to rebase if you would rather have it on current main.

[vivian-hafener]

Thanks for the fixes! Please rebase against main, then I'll take another look.