Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sebastiangaiser opened this issue Oct 23, 2024 · 4 comments
Open
Labels
bug Something isn't working

Comments

@sebastiangaiser
Copy link
Contributor

sebastiangaiser commented Oct 23, 2024

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 #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

@sebastiangaiser sebastiangaiser added the bug Something isn't working label Oct 23, 2024
@cmontemuino
Copy link
Contributor

cmontemuino commented Oct 28, 2024

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
Copy link
Contributor Author

sebastiangaiser commented Oct 28, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants