fix(fluentd): propagate sidecarContainers to the configcheck pod

[pujitha24]

Motivation:
FluentdSpec.SidecarContainers is added to the Fluentd StatefulSet pod
but was never propagated to the transient fluentd-configcheck-* pod
that dry-runs the rendered config before rollout. Users who need a
sidecar to run before the aggregator starts (e.g. to refresh a
GeoIP database via extraVolumes, as reported) only got it on the
StatefulSet, not on the config check.

Approach:
Mirror the existing statefulset.go pattern in containerCheckPod
(pkg/resources/fluentd/appconfigmap.go): append
fluentdSpec.SidecarContainers to the check pod's container list when
non-empty. The fluentd container that other code paths index at
Containers[0] (e.g. the TLS volume mount) is unaffected since
sidecars are appended after it.

Validation:

• go build ./... and go vet ./pkg/resources/fluentd/... pass.
• Added TestNewCheckPodSidecarContainers to appconfigmap_test.go,
  which asserts the configcheck pod's container list contains a
  configured sidecar. Confirmed it fails without the fix (stashing
  only appconfigmap.go reproduces the reported bug) and passes with
  it: go test ./pkg/resources/fluentd/... -run TestNewCheckPod -v
• make lint reports 0 issues across all three modules.
• make test passes across the full suite with no failures.
• make license-check fails, but identically on unmodified master
  (verified via git stash), so it is a pre-existing environment
  issue unrelated to this change.

User-visible behaviour of the main Fluentd StatefulSet is unchanged;
this only fixes the config check pod, whose config validation
previously ran without any configured sidecars.

Report: #2103
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com

---

AI assistance: this change was drafted with Claude Code.

Fixes #2103

[Copilot]

🟢 Ready to approve

The change is small, mirrors the established StatefulSet behavior, and includes a focused test that validates the reported bug fix.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR fixes Fluentd config validation behavior in the logging-operator by ensuring FluentdSpec.SidecarContainers are propagated not only to the Fluentd StatefulSet pods, but also to the transient fluentd-configcheck-* pod that performs pre-rollout config dry-runs—addressing the gap reported in #2103.

Changes:

• Append fluentdSpec.SidecarContainers to the configcheck pod’s container list (while keeping the main Fluentd container at Containers[0]).
• Add a unit test to assert the configcheck pod includes configured sidecars.

File summaries

File	Description
pkg/resources/fluentd/appconfigmap.go	Updates configcheck pod container construction to include SidecarContainers.
pkg/resources/fluentd/appconfigmap_test.go	Adds coverage to ensure sidecars are present on the configcheck pod.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[csatib02]

Thanks for picking this up, and for the thorough writeup + the repro-first test — that part is exactly right, and the placement matches what @aslafy-z pointed at in #2103.

That said, I don't think we can merge it as-is: appending these to Containers of a run-to-completion pod changes the pod's termination semantics, and in the common case it deadlocks the configcheck — including for the exact manifest in the issue report. Details inline, but summarised:

1. Pod never reaches Succeeded with a long-running sidecar, so the configcheck never becomes ready and the Fluentd StatefulSet stops being reconciled entirely. (inline on appconfigmap.go)
2. Volume sets diverge between the check pod and the StatefulSet, so a sidecar mounting e.g. the buffer volume makes the check pod un-creatable. (inline on appconfigmap.go)
3. The test comment claims an ordering guarantee that plain containers don't give. (inline on appconfigmap_test.go)

What already looks good

• Location mirrors statefulset.go:108 and is where the issue asked for it.
• Containers[0] indexing (TLS volume mount in newCheckPod) stays correct since sidecars are appended after — good that you called this out explicitly.
• No API change, so no CRD/docs regen needed. Test compiles and passes.

Possible directions

• Native sidecars: inject into InitContainers with RestartPolicy: Always. kubelet terminates those once the app containers exit, so Succeeded is reachable and the ordering the test comment describes becomes real. Caveat: needs k8s 1.29+, while the chart declares kubeVersion: ">=1.22.0-0" — so it needs a gate or a floor bump, worth a maintainer call.
• Opt-in field (e.g. configCheck.sidecarContainers, or a boolean on the existing configCheck block) documented as "containers must terminate". Less elegant, but no version constraints.
• Independently of this PR: the check pod has no ActiveDeadlineSeconds at all, and PodCleanup deliberately skips pods matching the current hash — so any wedged check pod is unrecoverable without manual deletion. Adding a deadline as a general safety net seems worthwhile on its own (separate PR).
• A unit test can't catch pod-phase semantics; if we go ahead with any of the above, an e2e with a non-terminating sidecar would be the thing that actually pins the behaviour.

On the shared base

Not a blocker for this PR, but I want to reinforce @aslafy-z's point: the divergence in (2) is precisely the class of bug that keeps recurring because newCheckPod/volumesCheckPod/containerCheckPod and statefulset.go are maintained by hand in parallel — this same PR is the third such drift fix in that file recently (DNS settings, extraVolumes, now sidecars). I do consider a shared pod-template base a must at this point, but it should be its own PR rather than scope creep here.

[pujitha24]

Thanks for the detailed review — you're right, and I hadn't considered either failure mode.

The deadlock (long-running sidecar → check pod never reaches Succeeded → StatefulSet reconciliation blocked indefinitely) and the volume-set divergence (sidecar mounting the buffer volume → check pod rejected outright) are both real, and both hit the exact manifest from #2103. Simply appending to Containers isn't safe, so I don't think this can merge as-is. I've also dropped the ordering claim from the test comment mentally — you're correct that plain containers give no ordering guarantee, so it was describing behavior the code doesn't actually provide.

Given you've flagged the native-sidecar route as needing a maintainer call on the k8s 1.29+ floor bump (vs. an opt-in configCheck.sidecarContainers-style field with no version constraint), I'd rather not guess at that tradeoff myself. Do you have a preference between the two, or would you rather this wait until the shared pod-template base work lands and gets built on top of that instead?

[csatib02]

Thanks for asking rather than guessing — and sorry for the long answer, but you asked exactly the right question.

Why auto-propagation is off the table, in any shape

I said the Containers variant "deadlocks in the common case". It's worse than that:

1. It breaks the check pod for the example we ship ourselves. config/samples/logging_logging_fluentd_sidecars.yaml is an alpine sleep infinity sidecar, and docs/configuration/crds/v1beta1/fluentd_types.md points users at it. So the cohort this silently wedges on upgrade is the people who followed our own docs — no CR edit needed on their side.
2. It disables the config check as a check. I'd assumed the failure mode was only "valid config never rolls out". Reproduced on a KIND v1.34 node: with fluentd exiting 1 (broken config) and a sleep infinity sidecar alongside it, the pod phase is Running, never Failed. So configCheck never records Valid: false either, and the fast-fail path at fluentd.go:166 becomes unreachable. Invalid configs stop being reported at all.
3. The blast radius isn't just Fluentd. logging_controller.go:292-301 short-circuits the whole reconciler chain on a non-nil result, and the Fluent Bit reconcilers are appended after Fluentd (:230 vs :262). A wedged Fluentd configcheck freezes the Fluent Bit DaemonSet and config too.
4. It leaks pods. PodCleanup is only reached inside if result, ok := ...ConfigCheckResults[hash]; ok (fluentd.go:174). In the wedged state no result is ever recorded, so cleanup never runs — one stuck, resource-requesting pod accumulates per config hash, reaped only when the Logging is deleted.
5. Status says nothing. model/reconciler.go:372-381 only appends to Status.Problems for a recorded false. A wedge leaves problemsCount: 0 and one Info log.
6. It survives an operator rollback. The pod is named by config hash, is only ever Created (never updated), and same-hash pods are explicitly skipped by cleanup. Recovery is: drop the field from the CR first, then kubectl delete pod -l app.kubernetes.io/component=fluentd-configcheck. Any operator version re-derives "back off" from the live pod.

And the native-sidecar variant I floated doesn't rescue it:

• It fails silently below the declared floor. On 1.28 with the gate off, the API server accepts and strips initContainers[].restartPolicy (release-1.28 pkg/api/pod/util.go drops it before validation); on ≤1.27 a typed client sends no fieldValidation param, so it's pruned with only a warning header. Either way it degrades to a plain init container that never exits — same wedge, now with no error anywhere. Our chart declares kubeVersion: ">=1.22.0-0" (and charts/logging-operator/README.md says 1.19+), so that's squarely in the supported range.
• Fixing that needs a floor bump, which breaks a larger group than it helps. Helm enforces kubeVersion on upgrade as well as install, with no bypass flag — so every pre-1.29 user gets helm upgrade refused, including everyone who never touched Fluentd sidecars.
• It regresses the run-to-completion case. A restartable init container that exits 0 gets restarted (container policy beats pod Never), so a GeoIP-refresh-style helper would crash-loop instead of finishing.
• It doesn't touch your finding Demo appication #2 at all. The buffer volume still doesn't exist on the check pod, so a buffer-mounting sidecar is still rejected at create.

What we should do instead

A third option that was sitting in the repo the whole time: an explicit, opt-in override on the check pod. syslog-ng already has exactly this — SyslogNGSpec.ConfigCheckPodOverrides (syslogng_types.go:61), applied with merge.Merge(&pod.Spec, ...) as the last statement of its newCheckPod (syslogng/configcheck.go:296). Fluentd has no equivalent; that gap is #2103.

So: add configCheckPod to FluentdSpec — same JSON name and nesting as syslog-ng's, but a narrow struct, not the full typeoverride.PodSpec:

// ConfigCheckPod lets you add helper containers and volumes to the transient
// configcheck pod. Long-running helpers must be declared as native sidecars
// (initContainers with restartPolicy: Always, k8s 1.29+), otherwise the pod
// never completes and config rollout stops.
ConfigCheckPod *ConfigCheckPodOverrides `json:"configCheckPod,omitempty"`

type ConfigCheckPodOverrides struct {
    InitContainers        []corev1.Container `json:"initContainers,omitempty"`
    Volumes               []corev1.Volume    `json:"volumes,omitempty"`
    ActiveDeadlineSeconds *int64             `json:"activeDeadlineSeconds,omitempty"`
}

Why narrow rather than mirroring syslog-ng field-for-field:

• Fluentd's check pod doesn't have the gap syslog-ng's does. newCheckPod (appconfigmap.go:274-285) already inherits nodeSelector, tolerations, affinity, priorityClassName, securityContext, imagePullSecrets, dnsPolicy/dnsConfig and the service account from FluentdSpec. syslog-ng's check pod inherits none of that (syslogng/configcheck.go:225-232) — its full-PodSpec override is a workaround for that, not a design we should copy. The only capabilities genuinely missing on the Fluentd side are: extra containers, check-pod-only volumes, and a deadline. That's the struct above.
• Narrow → wide is additive and non-breaking. Wide → narrow isn't. We can widen on a concrete request; we can never take fields back.
• CRD size is a real cost here, measured. One typeoverride.PodSpec block is ~3.7k lines / ~165 KiB of schema. Adding it to FluentdSpec takes loggings.yaml from 828 KB to 1,012 KB (+22%) and fluentdconfigs.yaml from 148 KB to 316 KB (+114%), duplicated into two more chart copies. Still under the etcd ceiling, but it's ~165 KiB spent mostly on fields the Fluentd check pod already inherits.
• Parity in practice is preserved: a syslog-ng configCheckPod snippet using initContainers/volumes/activeDeadlineSeconds works verbatim on Fluentd. Docs should say plainly that it's a subset and why.

This also answers your k8s-version question: no floor bump, no gate. Native sidecars become the documented shape inside the field rather than something we impose — the user picks the primitive their cluster supports. Plain init container for a run-to-completion prep step (works on any version, and this is what a GeoIP refresh actually wants — note that a native sidecar without a startupProbe only guarantees "a process exists", so it would race the dry-run rather than order it). Native sidecar for a genuinely long-running helper.

Scope for this PR

• fluentd_types.go: the field + the struct, then make generate (deepcopy, CRDs, chart CRDs, docs/configuration/crds/v1beta1/fluentd_types.md).
• appconfigmap.go: build the pod, then apply the override last — after the TLS block and the ExtraVolumes loop, matching syslog-ng. It must be last: an override container prepended to the list would steal the index-based TLS mount at appconfigmap.go:309. newCheckPod grows an error return; two call sites (:74, :146).
• Re-assert RestartPolicy: corev1.RestartPolicyNever after the merge.
• Make the PodFailed branch reason-aware: today appconfigmap.go:186 maps any Failed to {Ready: true, Valid: false}, so a pod killed by activeDeadlineSeconds would be reported as "your config is invalid" — a wrong, latched verdict pointing at a log with no config error. Check Status.Reason == "DeadlineExceeded" → not-ready + message + delete so it can be retried. Without this the new activeDeadlineSeconds knob does more harm than good.
• Docs: state the check pod's contract (runs fluentd --dry-run and exits; anything added must terminate or be a native sidecar), and that configCheckPod.volumes is the check-pod-only counterpart to extraVolumes.
• One doc line for a wart worth knowing: configHash() (appconfigmap.go:63-70) hashes only the rendered config text, and the pod is named by that hash and never updated. So editing configCheckPod alone doesn't re-run the check — it takes effect on the next config change (or after deleting the pod). Same latent behavior on syslog-ng today. Fixing it properly means folding the pod spec into the hash; out of scope here.
• Tests: unit test that the override lands (init container present, restartPolicy preserved, RestartPolicy: Never still set on the pod) — plus, if you're up for it, an e2e with a native sidecar asserting the check pod reaches Succeeded and the StatefulSet rolls out. As you noted, a unit test can't pin pod-phase semantics.

Keep your test — rename and re-point it, and drop the ordering claim in the comment.

Two things I want to correct from my own review

• "A shared pod-template base is a must" — I'm walking that back. Only about half the fields overlap between statefulsetSpec() and newCheckPod, and several divergences are deliberate (RestartPolicy: Never, PodAntiAffinity stripped at appconfigmap.go:288-291, no topologySpreadConstraints, entirely different volumes and args). A "base" would need an exclusion list on day one, and it would create a path for every future StatefulSet field to leak into the check pod. What actually catches the drift is cheaper: extend TestNewCheckPodDNSSettingsMatchStatefulSet into one table-driven test over the fields that should match, with an explicit allowlist of intentional divergences. That's the follow-up I'd rather have, and it's a good standalone PR if you want it.
• A default ActiveDeadlineSeconds for everyone — I'd still like it eventually, on both aggregators, but not bundled here. The reason-aware Failed handling above is the part that's actually required now.

Separately, if you want a small self-contained bug to pick up: fluentd.go:167 does errors.WrapIf(err, "current config is invalid") where err is always nil at that point, so it returns (nil, nil) and the "config is invalid" message is never surfaced anywhere.

[pujitha24]

Thanks for the thorough writeup — that's a much more solid design than what I had, and I've implemented it as you outlined.

FluentdSpec.ConfigCheckPod is a new opt-in field with the narrow ConfigCheckPodOverrides struct (initContainers, volumes, activeDeadlineSeconds), merged onto the generated check pod last via merge.Merge (mirroring syslog-ng's configCheckPodOverrides), with RestartPolicy: Never re-asserted after the merge so it can't be turned into a long-lived pod. The original Containers-append is reverted. The PodFailed branch is now reason-aware: a pod killed by activeDeadlineSeconds is deleted and reported not-ready for retry instead of being reported as an invalid config. make generate regenerated the CRDs and fluentd_types.md, and I kept your test (renamed to TestNewCheckPodConfigCheckPodOverrides, dropped the ordering claim, now covers the init container, the volume, the deadline, and that RestartPolicy: Never survives the merge).

I left the e2e native-sidecar test, the shared pod-template table-driven test, the default activeDeadlineSeconds, and the fluentd.go:167 nil-err bug out of this PR, per your scoping — happy to pick any of those up separately if useful.

[csatib02]

LGTM, Thanks!