fix(fluentd): add opt-in configCheckPod overrides instead of propagating sidecars

Revert the SidecarContainers-append to the configcheck pod's Containers
list: it's a run-to-completion pod gated on PodSucceeded, so a
long-running sidecar (e.g. the sleep-infinity example in our own docs)
wedges it forever and blocks Fluentd StatefulSet reconciliation, and
its volume set diverges from the StatefulSet's so a volume-mounting
sidecar makes the pod uncreatable.

Instead, add FluentdSpec.ConfigCheckPod, a narrow opt-in override
(initContainers, volumes, activeDeadlineSeconds) merged onto the
generated check pod last, mirroring the pattern already used by
SyslogNGSpec.ConfigCheckPodOverrides. RestartPolicy is reasserted to
Never after the merge, and a PodFailed check pod with
Reason=DeadlineExceeded is now treated as not-ready and deleted for
retry instead of being reported as an invalid config.

Report: kube-logging#2103
PR: kube-logging#2303
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

Author: pujitha24

charts/logging-operator/charts/logging-operator-crds/templates/logging.banzaicloud.io_fluentdconfigs.yaml

@@ -45,6 +45,11 @@ Overrides the default logging level configCheck setup. This field is not used di
 ### configCheckAnnotations (map[string]string, optional) {#fluentdspec-configcheckannotations}
 
 
+### configCheckPod (*ConfigCheckPodOverrides, optional) {#fluentdspec-configcheckpod}
+
+ConfigCheckPod lets you add helper containers and volumes to the transient configcheck pod, which runs `fluentd --dry-run` once and must exit on its own. 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. This is a narrower counterpart to SyslogNGSpec.ConfigCheckPodOverrides: the Fluentd check pod already inherits nodeSelector/tolerations/affinity/priorityClassName/securityContext/ imagePullSecrets/dnsPolicy/dnsConfig/serviceAccount from FluentdSpec, so only extra containers, check-pod-only volumes and a deadline are exposed here. Note: the check pod is named after a hash of the rendered config and is only ever created, never updated, so changing configCheckPod alone does not re-run the check against the current pod - it takes effect on the next config change, or after manually deleting the existing check pod. 
+
+
 ### configCheckResources (corev1.ResourceRequirements, optional) {#fluentdspec-configcheckresources}
 
 
@@ -202,6 +207,27 @@ Duration in seconds for graceful pod termination. Set higher than expected clean
 
 
 
+## ConfigCheckPodOverrides
+
+ConfigCheckPodOverrides adds helper containers, volumes and a deadline to the
+transient fluentd configcheck pod, merged onto the generated pod spec last.
+
+### activeDeadlineSeconds (*int64, optional) {#configcheckpodoverrides-activedeadlineseconds}
+
+ActiveDeadlineSeconds bounds how long the configcheck pod may run before it is treated as failed and deleted so a new one can be created and retried. Without it, a helper container that never terminates leaves the pod running indefinitely and blocks config rollout, with no timeout. 
+
+
+### initContainers ([]corev1.Container, optional) {#configcheckpodoverrides-initcontainers}
+
+InitContainers to add to the configcheck pod. A plain init container is only guaranteed to have started, not to have finished, before the dry-run container starts, so it does not order a preparation step against the check - it can race it. A long-running helper must instead set restartPolicy: Always (a native sidecar, k8s 1.29+); the kubelet then terminates it once the dry-run container exits, letting the pod reach Succeeded/Failed. 
+
+
+### volumes ([]corev1.Volume, optional) {#configcheckpodoverrides-volumes}
+
+Volumes available to the configcheck pod only. This is the check-pod counterpart to FluentdSpec.ExtraVolumes, which is not mounted on the check pod, e.g. for a volume an InitContainers entry above needs. 
+
+
+
 ## FluentOutLogrotate
 
 ### age (string, optional) {#fluentoutlogrotate-age}

charts/logging-operator/charts/logging-operator-crds/templates/logging.banzaicloud.io_loggings.yaml

@@ -21,6 +21,7 @@ import (
 	"maps"
 
 	"emperror.dev/errors"
+	"github.com/cisco-open/operator-tools/pkg/merge"
 	"github.com/cisco-open/operator-tools/pkg/reconciler"
 	"github.com/cisco-open/operator-tools/pkg/utils"
 	"github.com/spf13/cast"
@@ -70,8 +71,10 @@ func (r *Reconciler) configHash() (string, error) {
 }
 
 func (r *Reconciler) hasConfigCheckPod(ctx context.Context, hashKey string, fluentdSpec v1beta1.FluentdSpec) (bool, error) {
-	var err error
-	pod := r.newCheckPod(hashKey, fluentdSpec)
+	pod, err := r.newCheckPod(hashKey, fluentdSpec)
+	if err != nil {
+		return false, err
+	}
 
 	p := &corev1.Pod{}
 	err = r.Client.Get(ctx, client.ObjectKeyFromObject(pod), p)
@@ -143,7 +146,10 @@ func (r *Reconciler) configCheck(ctx context.Context) (*ConfigCheckResult, error
 		return res, errors.WrapIf(err, "failed to find output secret for fluentd configcheck")
 	}
 
-	pod := r.newCheckPod(hashKey, *r.fluentdSpec)
+	pod, err := r.newCheckPod(hashKey, *r.fluentdSpec)
+	if err != nil {
+		return nil, err
+	}
 	configcheck.WithHashLabel(pod, hashKey)
 
 	existingPods := &corev1.PodList{}
@@ -184,6 +190,17 @@ func (r *Reconciler) configCheck(ctx context.Context) (*ConfigCheckResult, error
 		case corev1.PodRunning:
 			return &ConfigCheckResult{}, nil
 		case corev1.PodFailed:
+			if pod.Status.Reason == "DeadlineExceeded" {
+				// the check ran past configCheckPod.activeDeadlineSeconds, not a config
+				// validation failure; delete the pod so a fresh one is created and retried
+				if err := client.IgnoreNotFound(r.Client.Delete(ctx, pod)); err != nil {
+					return nil, errors.WrapIf(err, "failed to delete configcheck pod that exceeded its active deadline")
+				}
+				return &ConfigCheckResult{
+					Ready:   false,
+					Message: "configcheck pod exceeded its active deadline and was deleted, will retry",
+				}, nil
+			}
 			return &ConfigCheckResult{
 				Ready: true,
 				Valid: false,
@@ -262,7 +279,7 @@ func (r *Reconciler) newCheckOutputSecret(hashKey string) (*corev1.Secret, error
 	return nil, errors.New("output secret is invalid, unable to create output secret for config check")
 }
 
-func (r *Reconciler) newCheckPod(hashKey string, fluentdSpec v1beta1.FluentdSpec) *corev1.Pod {
+func (r *Reconciler) newCheckPod(hashKey string, fluentdSpec v1beta1.FluentdSpec) (*corev1.Pod, error) {
 	volumes := r.volumesCheckPod(hashKey, fluentdSpec)
 	container := r.containerCheckPod(fluentdSpec)
 	initContainer := r.initContainerCheckPod(fluentdSpec)
@@ -314,7 +331,15 @@ func (r *Reconciler) newCheckPod(hashKey string, fluentdSpec v1beta1.FluentdSpec
 		}
 	}
 
-	return pod
+	if fluentdSpec.ConfigCheckPod != nil {
+		if err := merge.Merge(&pod.Spec, fluentdSpec.ConfigCheckPod); err != nil {
+			return nil, errors.WrapIf(err, "failed to merge configCheckPod overrides into the configcheck pod")
+		}
+		// the merge must not be able to turn the check pod into a long-lived one
+		pod.Spec.RestartPolicy = corev1.RestartPolicyNever
+	}
+
+	return pod, nil
 }
 
 func (r *Reconciler) volumesCheckPod(hashKey string, fluentdSpec v1beta1.FluentdSpec) (v []corev1.Volume) {
@@ -430,10 +455,6 @@ func (r *Reconciler) containerCheckPod(fluentdSpec v1beta1.FluentdSpec) []corev1
 		},
 	}
 
-	if len(fluentdSpec.SidecarContainers) != 0 {
-		container = append(container, fluentdSpec.SidecarContainers...)
-	}
-
 	return container
 }
 

charts/logging-operator/crds/logging.banzaicloud.io_fluentdconfigs.yaml

@@ -90,7 +90,8 @@ func TestNewCheckPodDNSSettings(t *testing.T) {
 			}
 			r := newCheckPodReconciler(t, spec)
 
-			pod := r.newCheckPod("deadbeef", *r.fluentdSpec)
+			pod, err := r.newCheckPod("deadbeef", *r.fluentdSpec)
+			require.NoError(t, err)
 
 			assert.Equal(t, tt.expectedDNSPolicy, pod.Spec.DNSPolicy)
 			assert.Equal(t, tt.dnsConfig, pod.Spec.DNSConfig)
@@ -112,7 +113,8 @@ func TestNewCheckPodDNSSettingsMatchStatefulSet(t *testing.T) {
 	}
 	r := newCheckPodReconciler(t, spec)
 
-	checkPod := r.newCheckPod("deadbeef", *r.fluentdSpec)
+	checkPod, err := r.newCheckPod("deadbeef", *r.fluentdSpec)
+	require.NoError(t, err)
 
 	obj, _, err := r.statefulset()
 	require.NoError(t, err)
@@ -123,22 +125,38 @@ func TestNewCheckPodDNSSettingsMatchStatefulSet(t *testing.T) {
 	assert.Equal(t, sts.Spec.Template.Spec.DNSConfig, checkPod.Spec.DNSConfig)
 }
 
-// TestNewCheckPodSidecarContainers pins the configcheck pod to carry the same
-// sidecarContainers as the aggregator StatefulSet, since a sidecar that only
-// mutates shared config (e.g. refreshing a GeoIP database via extraVolumes)
-// needs to run before the check as well, or the check validates against stale
-// input.
-func TestNewCheckPodSidecarContainers(t *testing.T) {
+// TestNewCheckPodConfigCheckPodOverrides pins configCheckPod.{initContainers,
+// volumes,activeDeadlineSeconds} to land on the generated configcheck pod, and
+// the pod to stay run-to-completion (RestartPolicy: Never) even though a merged
+// init container may declare RestartPolicy: Always (a native sidecar).
+func TestNewCheckPodConfigCheckPodOverrides(t *testing.T) {
+	restartAlways := corev1.ContainerRestartPolicyAlways
 	sidecar := corev1.Container{
-		Name:  "fluentd-sidecar",
-		Image: "busybox:1.37",
+		Name:          "geoip-refresh",
+		Image:         "busybox:1.37",
+		RestartPolicy: &restartAlways,
+	}
+	extraVolume := corev1.Volume{
+		Name: "geoip-db",
+		VolumeSource: corev1.VolumeSource{
+			EmptyDir: &corev1.EmptyDirVolumeSource{},
+		},
 	}
+	deadline := int64(120)
 	spec := &v1beta1.FluentdSpec{
-		SidecarContainers: []corev1.Container{sidecar},
+		ConfigCheckPod: &v1beta1.ConfigCheckPodOverrides{
+			InitContainers:        []corev1.Container{sidecar},
+			Volumes:               []corev1.Volume{extraVolume},
+			ActiveDeadlineSeconds: &deadline,
+		},
 	}
 	r := newCheckPodReconciler(t, spec)
 
-	checkPod := r.newCheckPod("deadbeef", *r.fluentdSpec)
+	checkPod, err := r.newCheckPod("deadbeef", *r.fluentdSpec)
+	require.NoError(t, err)
 
-	assert.Contains(t, checkPod.Spec.Containers, sidecar)
+	assert.Contains(t, checkPod.Spec.InitContainers, sidecar)
+	assert.Contains(t, checkPod.Spec.Volumes, extraVolume)
+	assert.Equal(t, &deadline, checkPod.Spec.ActiveDeadlineSeconds)
+	assert.Equal(t, corev1.RestartPolicyNever, checkPod.Spec.RestartPolicy)
 }

charts/logging-operator/crds/logging.banzaicloud.io_loggings.yaml

@@ -122,6 +122,44 @@ type FluentdSpec struct {
 	// Overrides the default logging level configCheck setup.
 	// This field is not used directly, its fields are copied over the ones in the logging resource, field by field, so setting one field here does not discard the others set on the logging resource.
 	ConfigCheck *ConfigCheck `json:"configCheck,omitempty"`
+	// ConfigCheckPod lets you add helper containers and volumes to the transient
+	// configcheck pod, which runs `fluentd --dry-run` once and must exit on its
+	// own. 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. This is a narrower counterpart
+	// to SyslogNGSpec.ConfigCheckPodOverrides: the Fluentd check pod already
+	// inherits nodeSelector/tolerations/affinity/priorityClassName/securityContext/
+	// imagePullSecrets/dnsPolicy/dnsConfig/serviceAccount from FluentdSpec, so only
+	// extra containers, check-pod-only volumes and a deadline are exposed here.
+	// Note: the check pod is named after a hash of the rendered config and is
+	// only ever created, never updated, so changing configCheckPod alone does
+	// not re-run the check against the current pod - it takes effect on the
+	// next config change, or after manually deleting the existing check pod.
+	ConfigCheckPod *ConfigCheckPodOverrides `json:"configCheckPod,omitempty"`
+}
+
+// +kubebuilder:object:generate=true
+
+// ConfigCheckPodOverrides adds helper containers, volumes and a deadline to the
+// transient fluentd configcheck pod, merged onto the generated pod spec last.
+type ConfigCheckPodOverrides struct {
+	// InitContainers to add to the configcheck pod. A plain init container is
+	// only guaranteed to have started, not to have finished, before the dry-run
+	// container starts, so it does not order a preparation step against the
+	// check - it can race it. A long-running helper must instead set
+	// restartPolicy: Always (a native sidecar, k8s 1.29+); the kubelet then
+	// terminates it once the dry-run container exits, letting the pod reach
+	// Succeeded/Failed.
+	InitContainers []corev1.Container `json:"initContainers,omitempty"`
+	// Volumes available to the configcheck pod only. This is the check-pod
+	// counterpart to FluentdSpec.ExtraVolumes, which is not mounted on the
+	// check pod, e.g. for a volume an InitContainers entry above needs.
+	Volumes []corev1.Volume `json:"volumes,omitempty"`
+	// ActiveDeadlineSeconds bounds how long the configcheck pod may run before
+	// it is treated as failed and deleted so a new one can be created and
+	// retried. Without it, a helper container that never terminates leaves the
+	// pod running indefinitely and blocks config rollout, with no timeout.
+	ActiveDeadlineSeconds *int64 `json:"activeDeadlineSeconds,omitempty"`
 }
 
 // +kubebuilder:object:generate=true