Relax HostTailer CRD and add default attributes if not specified

sebastiangaiser commented 3 weeks ago

Describe the bug: When deploying a HostTailer custom resource some fields needs to be specified, e.g. fileTailers. But other field(s), e.g. .spec.workloadMetaOverrides needs to be specified, even if I don't want to add annotations or labels to the resource. I tried to workaround this in the Helm chart in https://github.com/kube-logging/logging-operator/pull/1833 but even passing an empty object doesn't work out, too.

Expected behaviour: The operator should inject .spec.workloadMetaOverrides automatically if not specified and the validation for this setting should be adjusted. In addition the Helm chart should provide the possibility to overwrite these settings (should already be the case then).

Steps to reproduce the bug: Install the operator via the Helm chart with the following values:

logging:
  enabled: true
  hostTailer:
    enabled: true
    name: hosttailer
    image:
      repository: fluent/fluent-bit
      tag: 3.1.9
      pullPolicy: IfNotPresent
    workloadMetaOverrides:
      labels:
        asdf: asdf

Additional context: There might be other field(s) also from other custom resources having a similar behavior, so these should be adjusted the same way.

Environment details:

Kubernetes version (e.g. v1.15.2): v1.31.1
Cloud-provider/provisioner (e.g. AKS, GKE, EKS, PKE etc): Kubeadm
logging-operator version (e.g. 2.1.1): 4.10.0
Install method (e.g. helm or static manifests): Helm
Logs from the misbehaving component (and any other relevant logs): ---
Resource definition (possibly in YAML format) that caused the issue, without sensitive data: ---

/kind bug

cmontemuino commented 3 weeks ago

I'm not quite sure I understand the issue here.

@sebastiangaiser -- this is a modified version of the example you've provided:

logging:
  enabled: true
  hostTailer:
    enabled: true
    name: hosttailer
    image:
      repository: fluent/fluent-bit
      tag: 3.1.9
      pullPolicy: IfNotPresent
#    workloadMetaOverrides:
#      labels:
#        asdf: asdf

I wonder if it is not enough to use either required or fail Helm functions to make it fail if both hostTailer is enabled and worklaodMetaOverrides is empty.

sebastiangaiser commented 3 weeks ago

@cmontemuino sorry I probably didn't get you.

My problem is that the workloadMetaOverrides is a required, non empty field, which seams to be "wrong" to me. When adding some validation to the Helm chart the original problem is mitigated but not solved. I would expect that only adding e.g. should be needed:

logging:
  enabled: true
  hostTailer:
    enabled: true
    name: my-audit-hosttailer
    fileTailers:
      - name: audit-log
        path: /var/log/kubernetes/audit/audit.log

So e.g. the image should be added automatically if not specified, also an empty or not specified workloadMetaOverrides should be accepted. All other fields should be generated by the operator. This might be a change (even a relaxing one) in the crd.

cmontemuino commented 3 weeks ago

@cmontemuino sorry I probably didn't get you.

My problem is that the workloadMetaOverrides is a required, non empty field, which seams to be "wrong" to me. When adding some validation to the Helm chart the original problem is mitigated but not solved. I would expect that only adding e.g. should be needed:
logging:
  enabled: true
  hostTailer:
    enabled: true
    name: my-audit-hosttailer
    fileTailers:
      - name: audit-log
        path: /var/log/kubernetes/audit/audit.log
So e.g. the image should be added automatically if not specified, also an empty or not specified workloadMetaOverrides should be accepted. All other fields should be generated by the operator. This might be a change (even a relaxing one) in the crd.

Ok, I was looking at it the wrong way 😄.

With the current state of affairs (i.e., the "required" element from CRDs), the Helm Chart is missing a validation to enforce logging.hostTailer.workloadMetaOverrides is NOT empty.

My point is not against what you're suggesting about relaxing the CRD, but more about what we currently have.

sebastiangaiser commented 3 weeks ago

the Helm Chart is missing a validation

TBH for me, adding the validation would only let the install fail "earlier" but in the end it would fail anyway. As an example, we deploy that via a Flux HelmRelease which gets rolled back automatically in that case. So please don't get me wrong, I don't say it's wrong to add the validation but I would prefer to have that implemented in the operator 😄

kube-logging / logging-operator

Relax HostTailer CRD and add default attributes if not specified #1834