-
Notifications
You must be signed in to change notification settings - Fork 987
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
Fixing missing prefix for extraInputs #899
base: master
Are you sure you want to change the base?
Conversation
The `extraInputs` option is actually `input.extraInputs` according to the configmap template.
@@ -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 | | |
There was a problem hiding this comment.
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.
@@ -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 | | | |||
| `extraInputs` | Append to existing input with this value | `""` | | |||
| `input.extraInputs` | Append to existing input with this value | `""` | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Issue
NA.
Description of changes
The
extraInputs
option is actuallyinput.extraInputs
according to the configmap template.Checklist
README.md
for modified charts)version
inChart.yaml
for the modified chart(s)Testing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.