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.

Sign up for GitHub

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

Fixing missing prefix for extraInputs #899

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixing missing prefix for extraInputs #899

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion stable/aws-for-fluent-bit/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ helm delete aws-for-fluent-bit --namespace kube-system
| `service.parsersFiles` | List of available parser files | `/fluent-bit/parsers/parsers.conf` |
| `service.extraParsers` | Adding more parsers with this value | `""` |
| `input.*` | Values for Kubernetes input | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you fix the language above too? This is the kubernetes tail log file input.

| `extraInputs` | Append to existing input with this value | `""` |
| `input.extraInputs` | Append to existing input with this value | `""` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what this is meant to do is to append to the kubernetes tail input. Because when I do a simple silly test:

$ helm template --dry-run . --set input.extraInputs="[INPUT]somekey someval"
...

  fluent-bit.conf: |
    [SERVICE]
        Parsers_File /fluent-bit/parsers/parsers.conf

    [INPUT]
        Name              tail
        Tag               kube.*
        Path              /var/log/containers/*.log
        DB                /var/log/flb_kube.db
        Parser            docker
        Docker_Mode       On
        Mem_Buf_Limit     5MB
        Skip_Long_Lines   On
        Refresh_Interval  10
        [INPUT]somekey someval

The new data is added indented to the existing tail input. So you could use this to set other values in it, like for example Ignore_Older:

$ helm template --dry-run . --set input.extraInputs="Ignore_Older 1d"
  fluent-bit.conf: |
    [SERVICE]
        Parsers_File /fluent-bit/parsers/parsers.conf

    [INPUT]
        Name              tail
        Tag               kube.*
        Path              /var/log/containers/*.log
        DB                /var/log/flb_kube.db
        Parser            docker
        Docker_Mode       On
        Mem_Buf_Limit     5MB
        Skip_Long_Lines   On
        Refresh_Interval  10
        Ignore_Older 1d

I think this was added because the input.* only allows you to customize a few things: https://github.com/aws/eks-charts/blob/master/stable/aws-for-fluent-bit/values.yaml#L29

So basically, the name here is wrong/misleading. It should be something like input.extraSettings.

I think the additionalInputs key is for adding other other inputs.

I don't know how this works- can we bump the chart version and change the setting name?

If not, can you please make the help text here much more clear: "Append other config options to the kubernetes tail input" something like that.

Copy link
Author

@perry-mitchell perry-mitchell Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @PettitWesley - While I understand your points, I simply wanted to correct a typo in the readme. The functionality and naming therein should be out of scope for this, right? At least people working with the current version will then not make the same mistake I did, as the name will be correct.

Imo all of the names need to be reworked, because they're not consistent and are all confusing:

  • service.extraParsers
  • input.*
  • extraInputs
  • additionalInputs
  • filter.*
  • filter.extraFilters
  • additionalFilters

Unfortunately I don't have time right now to devote to such a PR, however 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I still think the language in your PR needs to be edited even just for this option. It should be clear what "existing input" means.

Append to existing kubernetes log file tail input with this value

would be prefered by me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is also another issue here that the CI only lets you merge updates that change the chart version, which I'll have to look into.

| `additionalInputs` | Adding more inputs with this value | `""` |
| `filter.*` | Values for kubernetes filter | |
| `filter.extraFilters` | Append to existing filter with value |
Expand Down