Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/fluent-bit] add extraLuaScripts config and inject lua script to dedot kube- labels and annotations #22328

Closed
wants to merge 5 commits into from

Conversation

kruftik
Copy link

@kruftik kruftik commented May 11, 2020

What this PR does / why we need it:

Fixes fluent/fluent-bit#1134. Injects lua function from fluent/fluent-bit#1134 (comment) to fluent-bit containers and use it to replace dots in the keys of kubernetes labels and annotations with underscore to fix data type-related conflicts

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [stable/mychartname])

…keys lua filter implementation

Signed-off-by: Dmitry Gadeev <[email protected]>
@helm-bot helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 11, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @kruftik. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 11, 2020
@kruftik kruftik force-pushed the feat/fluentbit-add-dedot-script branch from 009e9af to 2647e3a Compare May 11, 2020 07:06
@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). and removed Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels May 11, 2020
Signed-off-by: Dmitry Gadeev <[email protected]>
@kruftik kruftik force-pushed the feat/fluentbit-add-dedot-script branch from b98ddea to e8c7302 Compare May 11, 2020 07:10
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label May 11, 2020
@kruftik
Copy link
Author

kruftik commented May 11, 2020

/assign @hectorj2f

local new_map = {}
local changed_keys = {}
for k, v in pairs(map) do
local deslashed = string.gsub(k, "%/", "_")

Choose a reason for hiding this comment

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

Since this is removing slashes too, I don't think filter.enableDedotKeys as the value name is appropriate, but filter.enableDedotAndDeslashKeys feels too verbose and specific.

Maybe we could abstract this functionality into a generic filter.kubeRemapMetaKeys value so the Lua code here would look as follows:

{{- with .Values.filter.kubeRemapMetaKeys }}
local remapped = string.gsub(k, {{ .pattern | quote }}, {{ .replacement | quote }})
{{- end }}

Now a user can specify exactly how they'd like to remap K8s labels and annotations. The documentation would have to clarify the pattern has to be a valid Lua pattern as per https://www.lua.org/pil/20.2.html though, but for the most part regex is okay too.

Removing both slashes and dots as a chart consumer would then look like:

filter.kubeRemapMetaKeys:
  pattern: '[/.]'
  replacement: ''

Copy link
Author

Choose a reason for hiding this comment

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

sounds reasonable. now the proposed implementation is used. in addition, made kubeRemapMetaKeys as a list to allow multiple different remappings.

@@ -151,6 +151,7 @@ fullConfigMap: false
##
existingConfigMap: ""

extraLuaScripts: {}

Choose a reason for hiding this comment

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

This new value is missing in the README.

Disregarding that though, how would one use this value? I guess what's the benefit over just using extraVolumeMounts?

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

imo, the extraLuaScripts value is a quite simple way to inject additional Lua scripts to the container while existing extraVolumeMounts is not. the main "problem" here is the need to place the scripts content by some way into the volume where extraLuaScripts is a simple plain-text setting.

@helm-bot helm-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label May 17, 2020
Signed-off-by: Dmitry Gadeev <[email protected]>
@kruftik kruftik force-pushed the feat/fluentbit-add-dedot-script branch from e1e830d to 87f036d Compare May 17, 2020 12:14
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label May 17, 2020
@stale
Copy link

stale bot commented Jun 24, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 24, 2020
@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kruftik
To complete the pull request process, please assign hectorj2f
You can assign the PR to them by writing /assign @hectorj2f in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruftik
Copy link
Author

kruftik commented Jun 29, 2020

/assign @hectorj2f

@stale
Copy link

stale bot commented Jul 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2020
@stale
Copy link

stale bot commented Aug 16, 2020

This issue is being automatically closed due to inactivity.

@stale stale bot closed this Aug 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spurious field mapping errors in ES output
5 participants