fix: support extraVolumes/extraVolumeMounts, fix WH CA handling (OP-388) - #2800
Conversation
b28f878 to
5378268
Compare
8a3a433 to
c32fd4b
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
1 similar comment
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
c32fd4b to
c25950a
Compare
5378268 to
651ba76
Compare
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (09/09/26)2 reviewers were added to this PR based on Anton Bykov's automation. |
|
Claude finished @kristina-solovyova's task in 8m 28s —— View job Review summaryReviewed all 42 files. This is careful, well-commented work — the digest-based comparison (working around Two findings I'd consider blocking, plus a set of smaller ones. All posted as inline comments. Blocking1. Changing
2. Adoption branch can permanently swallow a pending image upgrade —
Correctness / security
Docs / API
Test coverageDetails on the test file. Not done: I could not run |
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies core reconciliation/upgrade rollout mechanics (pod-config hashing + propagation) across cluster/client paths, so a final human review is warranted despite only minor documentation nits found.
Pull request overview
Adds end-to-end support for user-specified extraVolumes / extraVolumeMounts across WekaCluster and WekaClient (validation, propagation to WekaContainer, pod construction, and rollout via pod-config hashing), and improves Weka Home CA handling for WekaClient (inheritance from target cluster, cross-namespace warning, and an admission warning for unverifiable “client-only” topology).
Changes:
- Implement
extraVolumes/extraVolumeMountsvalidation (strict JSON decoding + reserved name/path protection) for both WekaCluster and WekaClient. - Propagate extra volumes/mounts into WekaContainer specs and into created Pods, with digest-based comparison to avoid churn and to support CSI/map-containing volumes safely.
- Fix/extend Weka Home CA secret resolution for WekaClient (client→cluster(same-namespace)→env default), plus warnings/events and docs/Helm support.
File summaries
| File | Description |
|---|---|
| internal/validation/registry.go | Registers new validators for extra volumes and Weka Home CA warning. |
| internal/validation/extra_volumes.go | Shared extraVolumes/extraVolumeMounts validation logic (strict decode, reserved checks). |
| internal/validation/extra_volumes_test.go | Table-driven tests covering validation rules and field paths. |
| internal/validation/cluster_extra_volumes.go | WekaCluster wrapper validator for spec.podConfig.* extra volumes/mounts. |
| internal/validation/client_wekahome_cacert_unverifiable.go | Admission warning for unverifiable client-only Weka Home CA override. |
| internal/validation/client_wekahome_cacert_unverifiable_test.go | Tests for the client-only CA unverifiable warning behavior. |
| internal/validation/client_extra_volumes.go | WekaClient wrapper validator for flat spec.* extra volumes/mounts. |
| internal/pkg/domain/wekahome.go | Centralizes WekaClient Weka Home CA secret resolution with namespace rules. |
| internal/pkg/domain/wekahome_test.go | Unit tests for CA secret precedence and cross-namespace behavior. |
| internal/controllers/wekacluster/pod_config_version.go | Includes extra volume/mount digests in cluster pod-config hash (omitempty). |
| internal/controllers/wekacluster/pod_config_version_test.go | Tests for hash changes with extras + unchanged hash when extras unset. |
| internal/controllers/wekacluster/funcs_upgrade.go | Adds digest-based propagation of extras into containers during spec updates. |
| internal/controllers/wekacluster/funcs_upgrade_test.go | Regression tests for CSI map safety + propagation/no-churn behavior. |
| internal/controllers/wekaclient/pod_config_version.go | New client pod-config hash function including extra volume/mount digests. |
| internal/controllers/wekaclient/pod_config_version_test.go | Tests for client hash stability/change and “unset extras” compatibility. |
| internal/controllers/wekaclient/client_reconciler_loop.go | Propagates extras, resolves/inherits Weka Home CA secret, adds adoption logic for new pod-config tracking. |
| internal/controllers/wekaclient/client_reconciler_loop_test.go | Tests for extras propagation, CSI map hashing safety, adoption behavior, and upgrade/image regression coverage. |
| internal/controllers/wekaclient/client_numa_test.go | Updates tests for new NewUpdatableClientSpec(..., targetCluster) signature. |
| internal/controllers/resources/pod.go | Applies extra volumes/mounts late in pod build with collision checks. |
| internal/controllers/resources/extra_volumes.go | Defines reserved names/paths + normalization + digest helpers for extras. |
| internal/controllers/resources/extra_volumes_test.go | PodFactory-level tests for collisions, normalization guarantees, and reserved drift guard. |
| internal/controllers/factory/container_factory.go | Propagates normalized cluster extra volumes/mounts into new WekaContainers. |
| internal/admission/defaults.go | Adds default severities for new validators (extras = Error; unverifiable CA = Warn). |
| doc/summary.xml | Adds doc index entries for new extra-volumes and Weka Home TLS docs. |
| doc/operator/operations/weka-home-tls.md | New operational doc explaining Weka Home TLS + CA distribution constraints and topologies. |
| doc/operator/operations/secrets-management.md | Links to Weka Home TLS doc for CA-secret coverage clarification. |
| doc/operator/deployment/helm-install.md | Documents Helm manager.extraVolumes / manager.extraVolumeMounts. |
| doc/operator/deployment/extra-volumes.md | New doc describing all three extra-volumes surfaces, validation, rollout, and examples. |
| doc/api_dump/wekacontainer.md | API dump updated with extraVolumes fields on WekaContainer. |
| doc/api_dump/wekacluster.md | API dump updated with extraVolumes fields on WekaCluster podConfig. |
| doc/api_dump/wekaclient.md | API dump updated with extraVolumes fields + client status pod-config hash. |
| doc/api_dump/api-schema.json | Generated schema updates for new fields and removed deprecated field entry. |
| charts/weka-operator/values.yaml | Adds Helm values for manager extra volumes/mounts. |
| charts/weka-operator/templates/manager.yaml | Templates manager pod extra volumes/mounts + guards reserved name/path. |
| charts/weka-operator/resources/weka_runtime.py | Improves WH CA secret handling by concatenating all PEM keys and validating content. |
| .ainav/index.md | Adds navigation entry for validation/admission docs. |
| .ainav/controllers/wekacluster.md | Adds Weka Home CA scope notes and pointers to the new doc. |
| .ainav/controllers/wekaclient.md | Adds WekaClient Weka Home CA resolution behavior notes. |
| .ainav/config/validation.md | New navigation doc for validation/admission structure and new rules. |
| .ainav/config/index.md | Notes new manager extras and extraVolumes API surfaces with doc pointer. |
Review details
- Files reviewed: 42/42 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| **Reserved mount-path prefixes** (`ReservedMountPathPrefixes`) — a mount | ||
| cannot land on or under: |
| <file relpath="operator/deployment/extra-volumes.md"> | ||
| <tags>extraVolumes, extraVolumeMounts, podConfig, manager.extraVolumes, RawExtension, schemaless, VolumeMount, cluster_extra_volumes, client_extra_volumes, ReservedVolumeNames, ReservedMountPathPrefixes, DisallowUnknownFields, pod-config-version, podConfigHash, allowRotateNonAnnotatedPodConfigHash, upgradePolicy, CA bundle, tmpdir</tags> | ||
| <summary>Documents the three extra-volumes surfaces — WekaCluster spec.podConfig.extraVolumes/extraVolumeMounts (all pods of the cluster), WekaClient spec.extraVolumes/extraVolumeMounts (all client pods, flat spec, no podConfig), and the operator's own Helm manager.extraVolumes/extraVolumeMounts — for mounting arbitrary volumes (e.g. a private CA bundle) without a custom image. Explains that extraVolumes is a schemaless RawExtension while extraVolumeMounts is a typed VolumeMount list, and the resulting gap that only admission's DisallowUnknownFields decoding (cluster_extra_volumes/client_extra_volumes, Error at both severities) catches typos when the API server itself will not. Lists the reserved volume names/suffixes and mount-path prefixes (noting /etc/ssl and /etc/pki are deliberately not reserved), and that mounts land on the weka container only, never init containers. Covers propagation from WekaCluster/WekaClient onto owned WekaContainer specs, and the pod-config-hash rollout mechanics: pods are deleted and recreated (never patched) when the hash changes, unused digests are omitempty so non-adopters see no churn on operator upgrade, and pods that predate pod-config-hash tracking are adopted without rolling — so the first extraVolumes change on such a pod silently does nothing until it's deleted once or allowRotateNonAnnotatedPodConfigHash is set. Includes worked YAML for a cluster CA-bundle mount, a client-only (joinIpPorts, no targetCluster) OS-trust-store mount, and manager.extraVolumes for the operator's own Deployment.</summary> | ||
| </file> |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| func CalcClientPodConfigVersion(spec *weka.WekaClientSpec) string { | ||
| inputs := ClientPodConfigVersionInputs{ | ||
| PodConfigVersion: config.Config.PodConfigVersion, | ||
| Image: spec.Image, | ||
| ExtraVolumesDigest: resources.ExtraVolumesDigest(spec.ExtraVolumes), | ||
| ExtraVolumeMountsDigest: resources.ExtraVolumeMountsDigest(spec.ExtraVolumeMounts), | ||
| } | ||
| if config.Config.EnablePodConfigCodeVersionRotation { | ||
| inputs.WekaRuntimeVersion = consts.PodConfigCodeVersion |
There was a problem hiding this comment.
The resolved wekaHome cacert secret is missing from the pod-config-version inputs, so changing it never rolls the pod.
updateContainerIfChanged (client_reconciler_loop.go:851) now propagates the resolved secret onto container.Spec.AdditionalSecrets, and AdditionalSecrets becomes a pod volume + volumeMount (resources/pod.go:815-828). But a pod is only recreated when container.Spec.PodConfigHash changes (funcs_pod_ensure.go:260-315 → targetPodConfigHash(container)), and PodConfigHash only changes when CalcClientPodConfigVersion changes.
Since WekaHomeCacertSecret isn't an input here:
- User sets/changes
spec.wekaHome.cacertSecret(or the inherited cluster value changes). HandleSpecUpdateswrites the newAdditionalSecretsonto every client container. ✅HandleUpgradecomputes an unchangedtargetPodConfigHash, hits the earlyreturn nilat line 1081, and no pod is ever deleted.- The running pod keeps the old secret volume (or none at all), so the new CA is never mounted and
weka_cloud_ca_cert_pathstill points at the old/absent bundle.
The comment at client_reconciler_loop.go:849-850 says the point of the propagation is so "a change reaches already-running containers" — as written it reaches the container spec but not the pod. Suggest adding the resolved secret name to the inputs:
// WekaHomeCacertSecret is a pod-shaping input: it becomes a secret volume in the pod
// (pod.go AdditionalSecrets), so a change has to roll the pod to take effect.
WekaHomeCacertSecret string `json:"wekaHomeCacertSecret,omitempty"`and passing it through from the caller (which already resolves it via domain.GetWekaHomeClientCacertSecret). Note this needs the resolved value, so CalcClientPodConfigVersion would have to take the target cluster (or the resolved string) rather than just *WekaClientSpec.
Worth adding a test for exactly this: change spec.wekaHome.cacertSecret on an existing client → assert PodConfigHash changes.
| if c.wekaClient.Status.LastAppliedPodConfigHash == "" && !config.Config.AllowRotateNonAnnotatedPodConfigHash { | ||
| logger.Info("Adopting current pod config version (first deploy)", "targetPodConfigHash", targetPodConfigHash) | ||
| // The containers are adopted too, not just the client. handleSpecVersionMismatch declines to | ||
| // roll a pod whose annotation and container status are both empty, so a client adopted only | ||
| // at the parent level would, on its next real config change, get podConfigHash patched onto | ||
| // its containers, never roll their pods, and sit in Upgrading forever. | ||
| for _, container := range c.containers { | ||
| if container.Status.LastAppliedPodConfigHash != "" { | ||
| continue | ||
| } | ||
| container.Status.LastAppliedPodConfigHash = targetPodConfigHash | ||
| if err := c.Status().Update(ctx, container); err != nil { | ||
| return fmt.Errorf("failed to adopt pod config version on container %s: %w", container.Name, err) | ||
| } | ||
| } | ||
| c.wekaClient.Status.LastAppliedPodConfigHash = targetPodConfigHash | ||
| return c.Status().Update(ctx, c.wekaClient) |
There was a problem hiding this comment.
The adoption branch can permanently swallow a pending image upgrade for clients that predate pod-config tracking.
HandleUpgrade previously ran NewUpgradeController(..., spec.Image, ""), i.e. purely image-tracked: isContainerAligned compared container.Spec.Image == TargetImage (upgrade/upgrade.go:34-40). Now the target hash is always non-empty, so alignment is hash-based only, and this branch adopts without rolling.
Failure scenario: an existing WekaClient has Status.LastAppliedPodConfigHash == "" (any client created before this change). The operator is upgraded and spec.image is bumped to B in the same apply, so the first reconcile of the new operator sees image B already in the spec:
targetPodConfigHash = hash(B, …)— differs from"", so we don't return at line 1081.Status.LastAppliedPodConfigHash == ""→ adoption fires, stampshash(B)on the client and on every container's status, returns.- Next reconcile:
targetPodConfigHash == Status.LastAppliedPodConfigHash→return nilat line 1081.
The containers still carry Spec.Image == A and Status.LastAppliedImage == A, and nothing ever patches them. The client silently stays on the old image with no Upgrading state and no event. Previously step 3 would have rolled it.
The wekacluster counterpart at least computes imageChanged := cluster.Spec.Image != cluster.Status.LastAppliedImage before adopting; suggest gating adoption the same way here, e.g. only adopt when no image drift is outstanding:
imageDrift := slices.ContainsFunc(c.containers, func(ct *weka.WekaContainer) bool {
return ct.Spec.Image != c.wekaClient.Spec.Image || ct.Status.LastAppliedImage != c.wekaClient.Spec.Image
})
if c.wekaClient.Status.LastAppliedPodConfigHash == "" && !imageDrift && !config.Config.AllowRotateNonAnnotatedPodConfigHash {Two smaller points in the same branch:
- The per-container
c.Status().Update(ctx, container)loop races the wekacontainer controller, which also writesLastAppliedPodConfigHash. A conflict aborts the whole step mid-loop, leaving some containers adopted and some not; the client status is written last so the next reconcile re-enters and finishes, but aRetryOnConflict(or aPatchon the status subresource) would be less noisy. - Once
Status.LastAppliedPodConfigHashis set, the early return at line 1081 meansclearPrePullAnnotationForClientnever runs again. Its failure at line 1114 is only logged as a warning, so a transient failure there leaves theweka.io/prepull-*annotation behind forever.
| // Propagate the resolved wekaHome cacertSecret so a change reaches already-running containers, | ||
| // not only ones created after the change (buildClientWekaContainer only runs on container-create). | ||
| if container.Spec.AdditionalSecrets["wekahome-cacert"] != newClientSpec.WekaHomeCacertSecret { | ||
| container.Spec.AdditionalSecrets = clientAdditionalSecrets(newClientSpec.WekaHomeCacertSecret) | ||
| changed = true | ||
| } |
There was a problem hiding this comment.
Replacing the whole map means any other AdditionalSecrets key on the container is silently dropped, even though the if only tested the wekahome-cacert key. Today the map only ever has that one key (as the comment in extra_volumes.go:29-30 notes), so this is latent rather than broken — but it will break quietly the moment a second key is added, and the reserved-names list already depends on that same "exactly one entry" assumption in a second place.
Safer to mutate just the one key:
if container.Spec.AdditionalSecrets["wekahome-cacert"] != newClientSpec.WekaHomeCacertSecret {
if container.Spec.AdditionalSecrets == nil {
container.Spec.AdditionalSecrets = map[string]string{}
}
if newClientSpec.WekaHomeCacertSecret == "" {
delete(container.Spec.AdditionalSecrets, "wekahome-cacert")
} else {
container.Spec.AdditionalSecrets["wekahome-cacert"] = newClientSpec.WekaHomeCacertSecret
}
changed = true
}Also: "wekahome-cacert" is now a bare literal in three places (here, clientAdditionalSecrets, factory/container_factory.go:96) plus a derived "wekahome-cacert-secret" in resources.ReservedVolumeNames. Worth a single exported const so the reserved-name entry can be derived from it rather than hand-copied.
| // Files, not directories: the /-boundary rule means "/opt/weka" does not cover a sibling | ||
| // like "/opt/weka_runtime.py", so each of these needs its own entry. | ||
| "/opt/weka_runtime.py", // pod.go: weka_runtime.py mount | ||
| "/usr/local/bin/wekaauthcli", // pod.go: wekaauthcli mount | ||
| "/devenv.sh", // drivers.go: COS dev-env script mount |
There was a problem hiding this comment.
/usr/bin/weka is missing from this list.
init_containers.go:171 mounts the staged CLI onto container 0 at /usr/bin/weka (via copyWekaVersionToContainer, reached from setDriverDependencies when Spec.Instructions.Type == InstructionCopyWekaFilesToDriverLoader). ReservedMountPaths covers /usr/local/bin/weka and /usr/src, but neither /usr/bin nor /usr/bin/weka, so IsReservedMountPath("/usr/bin/weka") returns false.
Consequence: an extraVolumeMounts entry at /usr/bin/weka passes admission and the reserved check in applyExtraVolumes. On pods that take the copy path it then trips the dynamic existing-path check (a hard pod-build error rather than a clean admission rejection, since applyExtraVolumes runs at pod.go:941, after driver deps are set); on pods that don't, it is accepted and silently shadows the CLI the operator stages from the cluster image.
| // Files, not directories: the /-boundary rule means "/opt/weka" does not cover a sibling | |
| // like "/opt/weka_runtime.py", so each of these needs its own entry. | |
| "/opt/weka_runtime.py", // pod.go: weka_runtime.py mount | |
| "/usr/local/bin/wekaauthcli", // pod.go: wekaauthcli mount | |
| "/devenv.sh", // drivers.go: COS dev-env script mount | |
| // Files, not directories: the /-boundary rule means "/opt/weka" does not cover a sibling | |
| // like "/opt/weka_runtime.py", so each of these needs its own entry. | |
| "/opt/weka_runtime.py", // pod.go: weka_runtime.py mount | |
| "/usr/local/bin/wekaauthcli", // pod.go: wekaauthcli mount | |
| "/usr/bin/weka", // init_containers.go: staged weka CLI mount | |
| "/devenv.sh", // drivers.go: COS dev-env script mount |
Separately: these two lists are hand-maintained copies of what pod.go/drivers.go/init_containers.go actually create, and they will drift (three entries — devenv, google-cloud-key, host-usr-src — already reserve names nothing creates anymore; the live equivalents are weka-boot-scripts, gcloud-credentials, usrsrc). A unit test that builds a pod per mode/config combination and asserts every produced volume name and container-0 mount path is covered by these lists would catch the next drift, and would have caught /usr/bin/weka.
| func ExtraVolumeMountsDigest(mounts []corev1.VolumeMount) string { | ||
| if len(mounts) == 0 { | ||
| return "" | ||
| } | ||
| data, err := json.Marshal(mounts) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return util.GetHash(string(data), 16) | ||
| } |
There was a problem hiding this comment.
Returning "" on marshal failure collides with the "no mounts at all" return two lines above, and "" is exactly the value the callers treat as unset. So a marshal error makes updateContainerIfChanged (client_reconciler_loop.go:699) and HandleSpecUpdates (funcs_upgrade.go:379) conclude "nothing to propagate" and skip the update — a silently dropped config rather than a loud failure. It also feeds CalcClientPodConfigVersion, so the hash would silently not reflect the mounts.
json.Marshal of []corev1.VolumeMount can't realistically fail, which is exactly why swallowing the error buys nothing. Per CLAUDE.md ("never swallow errors"), either return the error, or make the impossible case distinguishable from unset:
func ExtraVolumeMountsDigest(mounts []corev1.VolumeMount) string {
if len(mounts) == 0 {
return ""
}
data, err := json.Marshal(mounts)
if err != nil {
// Unreachable for a plain struct slice; must not collapse to "" (== unset), which
// would make callers skip propagation instead of failing.
panic(fmt.Sprintf("marshaling extraVolumeMounts: %v", err))
}
return util.GetHash(string(data), 16)
}Same shape applies to the json.Marshal at line 105 in NormalizeExtraVolumes — there the //nolint:errcheck rationale ("volumes just decoded from JSON") is sound, but the result is assigned even if normalized were nil, which would write Raw: nil onto the container spec. Cheap to assert instead of assume.
| if [ -d /var/run/secrets/weka-operator/wekahome-cacert ]; then | ||
| rm -rf /opt/weka/k8s-runtime/vars/wh-cacert | ||
| mkdir -p /opt/weka/k8s-runtime/vars/wh-cacert/ | ||
| cp /var/run/secrets/weka-operator/wekahome-cacert/cert.pem /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem | ||
| chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem | ||
| # Secret data-key names are arbitrary, so concatenate every mounted PEM rather | ||
| # than assuming one is named cert.pem (the glob skips the ..data/..2025_* dotfiles). | ||
| for f in /var/run/secrets/weka-operator/wekahome-cacert/*; do | ||
| [ -f "$f" ] || continue | ||
| cat "$f" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem | ||
| echo "" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem | ||
| done | ||
| # Test for actual PEM content, not file size: the separator above writes a newline | ||
| # per key, so a secret holding only empty or non-PEM values still yields a non-empty | ||
| # file. An explicit CA replaces the system trust store, so pointing | ||
| # weka_cloud_ca_cert_path at a certificate-less file breaks Weka Home silently. | ||
| if grep -q "BEGIN CERTIFICATE" /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem 2>/dev/null; then | ||
| chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem | ||
| else | ||
| rm -rf /opt/weka/k8s-runtime/vars/wh-cacert | ||
| fi |
There was a problem hiding this comment.
Two things here.
1. Concatenating every file in the secret dir will inline a private key if the user points cacertSecret at a kubernetes.io/tls secret. That's a very natural mistake — a TLS secret is the obvious thing to reach for, and it has tls.crt + tls.key. The BEGIN CERTIFICATE check passes (thanks to tls.crt), so tls.key lands verbatim inside /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem, which is then pointed at by weka_cloud_ca_cert_path. Worth filtering to certificate blocks, or at least skipping files containing a private key:
for f in /var/run/secrets/weka-operator/wekahome-cacert/*; do
[ -f "$f" ] || continue
grep -q "BEGIN CERTIFICATE" "$f" || continue
grep -q "PRIVATE KEY" "$f" && continue
cat "$f" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
echo "" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
doneThat also removes the need for the post-hoc content test, since a dir with no PEM certs produces no file at all.
2. chmod 400 happens only after the loop, so the file exists with the default umask (typically 0644) for the duration of the concatenation. Harmless for a public CA bundle, but combined with (1) it's a real window on a private key. Set the mode before writing:
: > /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem(then drop the trailing chmod, keeping only the rm -rf on the no-certs path).
Nit on the comment: "the glob skips the ..data/..2025_* dotfiles" — true, but the reason is that shell globs don't match leading dots, not the date; the ..2025_* naming will read as stale in a year. # the glob skips the secret's ..data/..timestamp dotfiles is enough.
| ## Reserved names and paths | ||
|
|
||
| The operator reserves certain volume names, name suffixes, and mount-path | ||
| prefixes for its own use inside weka/client pods. An `extraVolumes` entry or | ||
| `extraVolumeMounts` entry that collides is rejected — at admission time if | ||
| enabled, and always by the pod factory as a final backstop. The authoritative | ||
| lists live in | ||
| [`internal/controllers/resources/extra_volumes.go`](../../../internal/controllers/resources/extra_volumes.go). | ||
|
|
||
| **Reserved volume names** (`ReservedVolumeNames`) — every volume name the | ||
| operator itself assigns anywhere in a weka pod (backend, client, or init | ||
| container). Volumes are pod-scoped, so a name used only by an init container | ||
| is reserved too, even though extra mounts never land in init containers (see | ||
| [Mount scope](#mount-scope-weka-container-only) below): | ||
|
|
||
| ``` | ||
| osrelease, dev, run, sys, weka-boot-scripts, hugepages, smbw-shm, | ||
| host-shared-netns, weka-container-persistence-dir, weka-container-shared-dir, | ||
| weka-cluster-persistence-dir, weka-container-global-persistence-dir, | ||
| weka-proxy-socket-dir, weka-ssdproxy-local-socket, node-info, weka-credentials, | ||
| proc-sysrq-trigger, proc-cmdline, devenv, google-cloud-key, host-modules, | ||
| host-usr-src, shared-weka-version, otel-packages, libmodules, usrsrc, | ||
| gcloud-credentials, wekahome-cacert-secret | ||
| ``` | ||
|
|
||
| `wekahome-cacert-secret` is the one name in that list the operator derives at | ||
| runtime rather than hardcodes — it comes from `spec.additionalSecrets`, which | ||
| forms `<name>-secret`. Only that literal name is reserved, so an ordinary user | ||
| name like `corp-ca-secret` is fine. | ||
|
|
||
| **Reserved mount-path prefixes** (`ReservedMountPathPrefixes`) — a mount | ||
| cannot land on or under: | ||
|
|
||
| ``` | ||
| /dev, /sys, /host, /host-binds, /hostside, /opt/weka, | ||
| /opt/weka-global-persistence, /var/run/secrets/weka-operator, | ||
| /usr/local/bin/weka, /etc/wekaio, /etc/syslog-ng, | ||
| /shared-python-packages, /shared-weka-version, /var/log, /lib/modules, | ||
| /usr/src, /var/secrets/google | ||
| ``` | ||
|
|
||
| **Reserved exact paths** (`ReservedMountPaths`) — operator mounts that are |
There was a problem hiding this comment.
This section describes an API that doesn't exist, which will send readers looking for symbols they can't find.
-
ReservedMountPathPrefixes(line 89) and "Reserved exact paths" (line 100) —extra_volumes.go:38-50has a singleReservedMountPathsslice, andIsReservedMountPathapplies the same on-or-under rule to every entry. There is no prefixes/exact split. The distinction the code actually makes is that a file entry (/opt/weka_runtime.py) reserves only itself simply because nothing lives under it — not because it's matched differently. -
"name suffixes" (line 61) — no suffix reservation exists.
extra_volumes.go:29-32explicitly rejects that approach in favour of reserving the one literal derived name, which line 84-88 then correctly explains. The two statements contradict each other. -
Line 84 says
wekahome-cacert-secret"comes fromspec.additionalSecrets" — it's the WekaContainer'sspec.additionalSecrets, which users don't set; the operator populates it fromspec.wekaHome.cacertSecret. Worth saying so, since as written it reads like a user-facing field.
Suggest collapsing 89-100 into one "Reserved mount paths (ReservedMountPaths) — a mount cannot land on, or under, any of:" list with the full 13 entries, and dropping "name suffixes" from line 61.
| |---|---|---| | ||
| | WekaCluster | `spec.wekaHome.cacertSecret` | Mounts the Secret into every backend pod, stages it at `/opt/weka/k8s-runtime/vars/wh-cacert/cert.pem`, and sets the cluster-wide `weka_cloud_ca_cert_path` from a drive container. | | ||
| | WekaClient | `spec.wekaHome.cacertSecret` | Places the same file on the client pod at the same path. When `targetCluster` is set and the cluster is in the **same namespace**, this is **derived automatically from the target cluster's own `cacertSecret`** — set it explicitly only to override that default. A cluster in another namespace is not inherited from: only the Secret *name* would be copied, and it would not resolve in the client's namespace, so the client emits a warning event and you must set the field yourself. | | ||
| | Operator (Helm) | `wekahome.cacertSecret` | Used only by the operator's own CR reporter (the process that periodically reports CRs to Weka Home). It reads the Secret through the Kubernetes API into an in-memory certificate pool at request time. **No volume, no mount, no container filesystem is involved.** | |
There was a problem hiding this comment.
"Used only by the operator's own CR reporter … No volume, no mount, no container filesystem is involved" is not correct, including for the code in this PR.
The Helm wekahome.cacertSecret lands in env.Config.WekaHome.CacertSecret, and that is:
- the last-resort fallback returned by
GetWekaHomeClientCacertSecret(internal/pkg/domain/wekahome.go:86), which this PR wires intoAdditionalSecrets→ a secret volume + mount on every client pod; - copied into
WekaHomeConfig.CacertSecretbyGetWekahomeConfig(wekahome.go:32-34), whichcontainer_factory.go:87-98turns intoadditionalSecrets["wekahome-cacert"]→ a secret volume + mount on every backend pod.
So it is a genuine cluster-wide/client-wide default that is mounted, not a reporter-only knob. Someone reading this table would set the Helm value expecting no pod impact and get every weka pod remounted (and rolled, once the pod-config-hash gap is fixed).
Also on line 85 (WekaClient row): the precedence chain is described as client → same-namespace cluster, but omits the third step. Per wekahome.go:79-87 it's actually:
wekaClient.spec.wekaHome.cacertSecret- target cluster's
spec.wekaHome.cacertSecret, only if same namespace - Helm
wekahome.cacertSecret(the operator-wide default)
Worth stating all three explicitly, since (3) is what a cross-namespace client silently falls back to after the warning event.
One more omission worth a sentence somewhere in this file: weka_runtime.py:3202-3210 deletes the staged wh-cacert directory when the concatenated file contains no BEGIN CERTIFICATE. That's a deliberate and good safety valve, but from the operator's point of view it's a silent fall back to the OS trust store — the pod starts fine and nothing surfaces the misconfigured Secret. Readers debugging "I set cacertSecret and nothing changed" need to know to check that file's existence.
| clusterSecret := "" | ||
| if sameNamespace && targetCluster.Spec.WekaHome != nil { | ||
| clusterSecret = targetCluster.Spec.WekaHome.CacertSecret | ||
| } | ||
|
|
||
| if clientSecret != "" { | ||
| return clientSecret, false | ||
| } | ||
| if clusterSecret != "" { | ||
| return clusterSecret, false | ||
| } | ||
|
|
||
| if !sameNamespace && targetCluster != nil && targetCluster.Spec.WekaHome != nil && targetCluster.Spec.WekaHome.CacertSecret != "" { | ||
| crossNamespaceSkipped = true | ||
| } | ||
|
|
||
| return env.Config.WekaHome.CacertSecret, crossNamespaceSkipped | ||
| } |
There was a problem hiding this comment.
The crossNamespaceSkipped return is only consumed in buildClientWekaContainer, which runs on container creation (client_reconciler_loop.go:485-495). NewUpdatableClientSpec — the path taken on every reconcile of an existing client — discards it with whCaCert, _ := … (line 1377). So the "you must set cacertSecret yourself" warning event never reaches the users most likely to need it: someone who adds spec.wekaHome.cacertSecret to an existing cross-namespace cluster and wonders why their clients didn't pick it up. Emitting it from the reconcile path (still throttled) would fix that.
Small cleanup in the same function: the final if re-derives what sameNamespace already told you. Since clusterSecret is only populated when sameNamespace, reaching line 84 with a non-empty cluster secret implies !sameNamespace:
if clientSecret != "" {
return clientSecret, false
}
if clusterSecret != "" {
return clusterSecret, false
}
// Only reachable with !sameNamespace, since clusterSecret is populated only when same-namespace.
if targetCluster != nil && targetCluster.Spec.WekaHome != nil && targetCluster.Spec.WekaHome.CacertSecret != "" {
crossNamespaceSkipped = true
}
return env.Config.WekaHome.CacertSecret, crossNamespaceSkippedAlso worth noting for the test suite: wekahome_test.go:70-79 uses same-secret for both the client and the cluster value, so it can't distinguish "client wins" from "cluster wins" — it passes under either precedence. Use distinct values there.
| {{- range .Values.manager.extraVolumes }} | ||
| {{- if eq .name "tmpdir" }}{{ fail "manager.extraVolumes: the volume name \"tmpdir\" is reserved by the operator" }}{{ end }} | ||
| {{- end }} | ||
| {{- range .Values.manager.extraVolumeMounts }} | ||
| {{- if eq .mountPath "/tmp" }}{{ fail "manager.extraVolumeMounts: the mount path \"/tmp\" is reserved by the operator" }}{{ end }} | ||
| {{- end }} | ||
| volumes: |
There was a problem hiding this comment.
The /tmp guard is exact-match only, so mountPath: /tmp/foo (or /tmp/) sails through and shadows part of the operator's own tmpdir emptyDir. Same on-or-under rule the Go side uses (IsReservedMountPath) would be more consistent:
{{- range .Values.manager.extraVolumeMounts }}
{{- if or (eq .mountPath "/tmp") (hasPrefix "/tmp/" .mountPath) }}{{ fail "manager.extraVolumeMounts: the mount path \"/tmp\" is reserved by the operator" }}{{ end }}
{{- end }}
Also nothing here catches duplicate names or duplicate mount paths within the user's own lists — Kubernetes will reject the Deployment, but with a much less obvious message than the two fails above. Low priority given the failure is at least loud.
| for i, m := range mounts { | ||
| idxPath := mountsPath.Index(i) | ||
|
|
||
| if _, ok := declaredNames[m.Name]; !ok { | ||
| errs = append(errs, field.Invalid(idxPath.Child("name"), m.Name, | ||
| "does not match any entry in extraVolumes; mounting an operator-managed base "+ | ||
| "volume at a second path is not supported")) | ||
| } | ||
|
|
||
| switch { | ||
| case !path.IsAbs(m.MountPath): | ||
| errs = append(errs, field.Invalid(idxPath.Child("mountPath"), m.MountPath, | ||
| "must be an absolute path")) | ||
| case path.Clean(m.MountPath) != m.MountPath: | ||
| errs = append(errs, field.Invalid(idxPath.Child("mountPath"), m.MountPath, | ||
| fmt.Sprintf("must be a cleaned path; use %q", path.Clean(m.MountPath)))) | ||
| default: | ||
| if resources.IsReservedMountPath(m.MountPath) { | ||
| errs = append(errs, field.Invalid(idxPath.Child("mountPath"), m.MountPath, | ||
| "is reserved for an operator-managed mount")) | ||
| } | ||
| if _, dup := seenPaths[m.MountPath]; dup { | ||
| errs = append(errs, field.Duplicate(idxPath.Child("mountPath"), m.MountPath)) | ||
| } | ||
| } | ||
| seenPaths[m.MountPath] = struct{}{} | ||
| } | ||
| return errs |
There was a problem hiding this comment.
Two small inconsistencies with the pod-factory backstop this mirrors:
-
seenPathsis keyed on the rawm.MountPath(line 103, outside theswitch), whereasapplyExtraVolumeskeys its collision map onpath.Clean(m.MountPath)(pod.go:975). In practice the un-cleaned cases already error out via thepath.Cleanbranch, so this can't produce a wrong result today — but keying onpath.Clean(m.MountPath)here would make the two implementations obviously equivalent instead of accidentally equivalent. -
raw.Objectis ignored.validateExtraVolumesshort-circuits onlen(raw.Raw) == 0, andNormalizeExtraVolumescollapses that same case tonil. ARawExtensioncan legitimately carryObjectwith an emptyRaw; that never happens for objects decoded from an admission request or read back from the API server, so this is theoretical — but if it ever did, the volumes would be silently dropped rather than rejected. Araw.Object != nilguard returning an "unsupported form" error would make that explicit.
Also, line 46 embeds the entire raw JSON blob into the error value: for a large extraVolumes that produces an unwieldy admission message. Truncating, or just reporting the decoder error without the payload, reads better.
| | hugepages | int | hugepages, value in megabytes | | ||
| | hugepagesOffset | *int | value in megabytes to offset | | ||
| | wekaHomeConfig | WekahomeClientConfig | DEPRECATED, kept for compatibility with old API clients, not taking any action, to be removed on new API version | | ||
| | wekaHome | *WekahomeClientConfig | | |
There was a problem hiding this comment.
Heads-up on something riding along in the pkg/weka-k8s-api submodule bump rather than in this PR's own Go code: the deprecated wekaHomeConfig field is removed from WekaClientSpec (also visible in doc/api_dump/api-schema.json at the WekaHomeConfig/wekaHomeConfig deletion).
Its doc comment said "kept for compatibility with old API clients … to be removed on new API version" — but this removes it from v1alpha1, i.e. within the same API version. Consequences for anyone still setting it:
- Plain
kubectl apply/ client-go: pruned silently (behaviour unchanged, since the field was already a no-op). - Server-side apply, and
kubectl apply --validate=strict: hard rejection on the unknown field. - Any stored object still carrying it gets it pruned on next write.
If that's intentional and coordinated, fine — but it's an unrelated breaking API change in a PR titled fix:, so it deserves an explicit callout in the PR description / release notes rather than only showing up as a line deleted from a generated doc.
| // the containers, still fails to roll their pods, and strands the client after all. | ||
| func TestHandleUpgrade_AdoptsPodConfigHashWithoutRolling(t *testing.T) { | ||
| scheme := runtime.NewScheme() | ||
| if err := weka.AddToScheme(scheme); err != nil { | ||
| t.Fatalf("failed to add weka scheme: %v", err) | ||
| } | ||
|
|
||
| wekaClient := &weka.WekaClient{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "test-client", Namespace: "default"}, | ||
| Spec: weka.WekaClientSpec{Image: "quay.io/weka.io/weka-in-container:4.5.0"}, | ||
| } | ||
| // Pre-existing container: hash never set, pod predates the annotation. | ||
| container := &weka.WekaContainer{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "test-client-container", Namespace: "default"}, | ||
| Spec: weka.WekaContainerSpec{Image: wekaClient.Spec.Image}, | ||
| } | ||
|
|
||
| fakeClient := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| WithObjects(wekaClient, container). | ||
| WithStatusSubresource(wekaClient, container). | ||
| Build() | ||
|
|
||
| c := &clientReconcilerLoop{ | ||
| Client: fakeClient, | ||
| Recorder: record.NewFakeRecorder(10), | ||
| wekaClient: wekaClient, | ||
| containers: []*weka.WekaContainer{container}, | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
Good test for the case it covers, and the doc comment is genuinely useful. Two gaps worth closing given how much behaviour this branch now gates:
-
Image changed at the same time as adoption. Here
container.Spec.Image == wekaClient.Spec.Image, so the scenario I flagged onclient_reconciler_loop.go:1091isn't exercised. A second case withcontainer.Spec.Image = "…:4.4.0"/wekaClient.Spec.Image = "…:4.5.0"should assert the container does get patched (or at minimum that the client doesn't end up permanently short-circuited on the old image). As written that case would silently pass adoption and never roll. -
AllowRotateNonAnnotatedPodConfigHash = true— the documented escape hatch for exactly this transition is untested; a case asserting the upgrade controller path is taken instead of adoption would pin it.
Also, the assertion on c.upgradeInProgress at the end doesn't test anything: nothing on the adoption path ever sets it, so it holds regardless of whether adoption or the upgrade path ran. Asserting on container.Spec.PodConfigHash still being "" (the "without rolling" half of the test's own name) would be the load-bearing check.
Neighbouring nits in this file's other new cases:
TestNewUpdatableClientSpec_HashStructSurvivesCsiVolumeAttributesMapis a genuinely valuable regression test — worth keeping the comment.wekahome_test.go:70-79("client value wins when it matches the cluster") sets both tosame-secret, so it passes under either precedence; use distinct values.client_wekahome_cacert_unverifiable_test.goonly counts errors. The validator's warning severity comes fromadmission/defaults.go:50and is what makes this rule non-blocking — that mapping is the thing worth asserting, since flipping it toErrorwould break every client-only deployment using a private CA.
No description provided.