Repository navigation
fix(monitoring): strip infrastructure metadata from tenant logs - #2196
myasnikovdaniil merged 1 commit into
Conversation
📝 WalkthroughWalkthroughTwo new Fluent Bit ChangesFluent Bit Filter Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
the
size/XS
dosubot
Bot
added
the
kind/feature
7dbdcd4 to
d70e4c3
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — one of the two stripped fields doesn't exist in this pipeline, and the two-field block-list doesn't fully meet the stated goal.
Business context: stop tenants from seeing infrastructure topology (which node a pod ran on) in their container logs, by stripping fields in the fluent-bit pipeline.
Blockers
B1: Remove kubernetes_node_name is a no-op — the field never exists here
File: packages/system/monitoring-agents/values.yaml:429
The fluent-bit Kubernetes filter emits the node name as host, not node_name. The nest/lift at lines 385-391 promotes the whole kubernetes map to top-level kubernetes_*, so the real field is kubernetes_host. The kubernetes filter (lines 378-384) runs with defaults (no Use_Kubelet On), so no node_name is ever produced — nothing in the log pipeline emits it. Remove kubernetes_host does the work; Remove kubernetes_node_name silently does nothing. Drop the dead line, or identify the config that actually produces kubernetes_node_name and explain it.
B2: a two-field block-list doesn't meet "strip infrastructure metadata"
File: packages/system/monitoring-agents/values.yaml:426-430
The lift promotes the entire kubernetes map, so removing only host leaves other lifted infra fields tenant-visible (e.g. kubernetes_docker_id, kubernetes_container_image, kubernetes_annotations, kubernetes_pod_id). A block-list also silently leaks any field a future fluent-bit upgrade adds. The OUTPUT only streams kubernetes_pod_name, kubernetes_container_name, kubernetes_namespace_name (the _stream_fields allow-list at line 350), so prefer an allow-list — keep just those three — over block-listing individual fields.
Non-blocking
- Two near-duplicate
Signed-off-bytrailers for the same email (different display name) — collapse to one before merge. - No coverage: a rendered-ConfigMap assertion that
Remove kubernetes_hostis present and nokubernetes_node_nameline is emitted would pin the real behavior (chart ships no helm-unittest, and this is a vendored-subchart values file so per-chart unit tests aren't the norm here — not blocking on that alone).
| [FILTER] | ||
| Name modify | ||
| Match kube.* | ||
| Remove kubernetes_node_name |
There was a problem hiding this comment.
This line is a no-op — kubernetes_node_name never exists in this pipeline. The Kubernetes filter emits the node name as host (lifted to kubernetes_host by the nest/lift above), and it runs with defaults (no Use_Kubelet On), so nothing produces a node_name field. Remove kubernetes_host below does the actual work. Drop this line, or point to the config that produces kubernetes_node_name.
There was a problem hiding this comment.
Good catch, confirmed — kubernetes_node_name is never produced in this pipeline. The kubernetes filter runs with defaults (no Use_Kubelet On) so it emits the node name as host, which the nest/lift above promotes to kubernetes_host. The Remove kubernetes_node_name line was a no-op based on a wrong field-name assumption.
Dropped it in 61fb19d — only Remove kubernetes_host remains, which does the actual stripping. Thanks!
github-actions
Bot
added
area/monitoring
|
Addressed the review across two commits (61fb19d, 8977ca6): B1 — B2 — block-list insufficient: reworked into an allow-list. One clarification on the premise: I scoped the allow-list to the Non-blocking 1 — duplicate Non-blocking 2 — test coverage: skipping per your own note that this is a vendored-subchart values file where per-chart helm-unittest isn't the norm here. Happy to add a rendered-ConfigMap assertion if you'd like it after all. |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Both prior blockers are resolved and the approach is now correct. B1: the dead Remove kubernetes_node_name line is gone — nothing in this pipeline ever produced that field. B2: the two-field block-list is replaced by a prefix-scoped allow-list — rename the three keepers out of the way, Remove_wildcard kubernetes_, then rename them back — so only kubernetes_pod_name, kubernetes_namespace_name and kubernetes_container_name survive, matching the OUTPUT _stream_fields, and any future kubernetes_* field is dropped by default. Scoping the new filters to kube.* and the wildcard to the kubernetes_ prefix leaves merged application-log fields (Merge_Log On) and the events/audit streams untouched, so there is no regression.
Non-blocking: the first commit still carries two Signed-off-by trailers for the same email, and the branch is three commits where the last two just fix the first — since this repo merges without squashing, please rebase into one clean commit before merge so the duplicate trailer and the iteration history don't land in main.
8977ca6 to
2052e53
Compare
|
This is code-approved and CI is green now, but one thing before it can merge: the branch is three commits where the last two only fix the first, and that first commit carries a duplicate |
Allow-list the kubernetes_ fields kept on kube.* records so that infrastructure topology (node name, host) no longer leaks to tenant users, while preserving pod/namespace/container identity fields. Relates to cozystack#2194 Signed-off-by: Mattia Eleuteri <[email protected]>
mattia-eleuteri
changed the title
2052e53 to
0356cde
Compare
|
Squashed to a single clean commit (one Signed-off-by) and rebased onto current main; title moved to Conventional Commits as well. Heads-up on CI: after the rebase, Pre-Commit and the new API Review Gate pass, but every |
3 participants
Summary
modifyfilter in fluent-bit that removeskubernetes_node_nameandkubernetes_hostfrom all container logsProblem
kubernetes_node_nameis exposed in log metadata, leaking node topology to end-users. Other sensitive fields likekubernetes_hostare also visible.Relates to #2194
Test plan
helm templatemonitoring-agents and verifyRemove kubernetes_node_namefilter is presentkubernetes_node_nameis absent from logs in tenant Grafana🤖 Generated with Claude Code
Summary by CodeRabbit
kubernetes_*fields forkube.*events, while preserving the key Kubernetes identity fields (such as pod, namespace, and container-related names) for clearer, more consistent log output.