filter_kubernetes: fix accidentally removed u in prefix#11365
filter_kubernetes: fix accidentally removed u in prefix#11365
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughA single regex macro in the Kubernetes filter was changed from an inner capturing group to a non-capturing group; a new pod manifest test file and a unit test were added, and a file-reader helper was fixed to null-terminate its buffer. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
cosmo0920
force-pushed
the
cosmo0920-fix-accidentally-removed-u-in-prefix
branch
from
67aa0a2 to
ce21b18
Compare
cosmo0920
force-pushed
the
cosmo0920-fix-accidentally-removed-u-in-prefix
branch
from
ce21b18 to
9cf4c9f
Compare
cosmo0920
force-pushed
the
cosmo0920-fix-accidentally-removed-u-in-prefix
branch
from
50e62c9 to
6343780
Compare
cosmo0920
force-pushed
the
cosmo0920-fix-accidentally-removed-u-in-prefix
branch
from
6343780 to
5fbd345
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/runtime/filter_kubernetes.c (1)
71-110: Nice fix adding NUL-termination—consider hardening empty-file handling to avoid underflow in the trim logic.If an
.outfile is empty (out_size == 0),p = out_buf + (out_size - 1)underflows; alsofread(..., st.st_size, 1, ...)fails forst.st_size == 0.Suggested hardening
static int file_to_buf(const char *path, char **out_buf, size_t *out_size) { int ret; - long bytes; + size_t bytes; char *buf; FILE *fp; struct stat st; + size_t sz; ret = stat(path, &st); if (ret == -1) { return -1; } + sz = (size_t) st.st_size; fp = fopen(path, "r"); if (!fp) { return -1; } - buf = flb_malloc(st.st_size + 1); + buf = flb_malloc(sz + 1); if (!buf) { flb_errno(); fclose(fp); return -1; } - bytes = fread(buf, st.st_size, 1, fp); - if (bytes != 1) { + bytes = fread(buf, 1, sz, fp); + if (bytes != sz) { flb_errno(); flb_free(buf); fclose(fp); return -1; } - buf[st.st_size] = '\0'; + buf[sz] = '\0'; fclose(fp); *out_buf = buf; - *out_size = st.st_size; + *out_size = sz; return 0; } @@ /* Sanitize content, get rid of ending \n */ - p = out_buf + (out_size - 1); - while (*p == '\n' || *p == '\r') p--; - *++p = '\0'; + p = out_buf + out_size; + while (p > out_buf && (p[-1] == '\n' || p[-1] == '\r')) p--; + *p = '\0';Also applies to: 134-139
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/runtime/data/kubernetes/log/core/core_uat-myapp-12345_fluent-bit.logis excluded by!**/*.logtests/runtime/data/kubernetes/out/core/core_uat-myapp-12345_fluent-bit.outis excluded by!**/*.out
📒 Files selected for processing (3)
plugins/filter_kubernetes/kube_regex.htests/runtime/data/kubernetes/meta/short-prefix-uat-podname.jsontests/runtime/filter_kubernetes.c
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: zhihonl
Repo: fluent/fluent-bit PR: 10586
File: plugins/filter_kubernetes/kube_regex.h:29-29
Timestamp: 2025-08-14T18:14:41.453Z
Learning: Kubernetes ReplicaSet suffixes generated from pod-template-hash can vary in length and are not always 10 characters. Examples include "9dbdfc456" (9 chars) from "amazon-cloudwatch-observability-controller-manager-9dbdfc456" and "914055854" (9 chars) from "kairosdb-914055854". The DEPLOYMENT_REGEX pattern using {6,10} range for the ID group correctly accommodates this variable length behavior.
Learnt from: zhihonl
Repo: fluent/fluent-bit PR: 10586
File: plugins/filter_kubernetes/kube_regex.h:29-29
Timestamp: 2025-08-14T18:14:41.453Z
Learning: Kubernetes ReplicaSet suffixes are not always 10 characters. The suffix length can vary, as demonstrated by the example "amazon-cloudwatch-observability-controller-manager-9dbdfc456" which has a 9-character suffix. The original DEPLOYMENT_REGEX pattern using {6,10} range for the ID group is more accurate than assuming a fixed 10-character length.
Learnt from: hlein
Repo: fluent/fluent-bit PR: 11168
File: conf/parsers_mult.yaml:8-14
Timestamp: 2025-11-16T22:16:26.032Z
Learning: In Fluent Bit parser configurations (both .conf and .yaml formats), the regex engine automatically strips leading and trailing `/` characters from regex patterns, so patterns like `/Processing by .../` and `Processing by ...` are functionally equivalent and both work correctly.
📚 Learning: 2025-08-14T18:14:41.453Z
Learnt from: zhihonl
Repo: fluent/fluent-bit PR: 10586
File: plugins/filter_kubernetes/kube_regex.h:29-29
Timestamp: 2025-08-14T18:14:41.453Z
Learning: Kubernetes ReplicaSet suffixes generated from pod-template-hash can vary in length and are not always 10 characters. Examples include "9dbdfc456" (9 chars) from "amazon-cloudwatch-observability-controller-manager-9dbdfc456" and "914055854" (9 chars) from "kairosdb-914055854". The DEPLOYMENT_REGEX pattern using {6,10} range for the ID group correctly accommodates this variable length behavior.
Applied to files:
plugins/filter_kubernetes/kube_regex.h
🪛 Checkov (3.2.334)
tests/runtime/data/kubernetes/meta/short-prefix-uat-podname.json
[medium] 2-17: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 2-17: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (3)
tests/runtime/filter_kubernetes.c (1)
415-423: No action required—all required fixture files already exist.The fixture files for
kube_short_prefix_uat_podnameare present:
tests/runtime/data/kubernetes/log/core/core_uat-myapp-12345_fluent-bit.log✓tests/runtime/data/kubernetes/out/core/core_uat-myapp-12345_fluent-bit.out✓tests/runtime/data/kubernetes/meta/short-prefix-uat-podname.json✓The test is properly registered at line 1128 in the test array.
Likely an incorrect or invalid review comment.
tests/runtime/data/kubernetes/meta/short-prefix-uat-podname.json (1)
1-16: No action needed—test fixtures are not scanned by any security tool in CI.This repository uses Trivy (only for container image scanning), not Checkov. The
tests/runtime/data/directory is not gated by any security scanner, and test fixtures do not require production-level security configurations. The fixture is valid as-is.Likely an incorrect or invalid review comment.
plugins/filter_kubernetes/kube_regex.h (1)
25-25: No action needed – regex extraction uses only named capture groups, not numeric positions.The code uses
flb_regex_parse()with a callback that extracts match values by group name (viastrcmp()on "pod_name", "namespace_name", "container_name", "docker_id"), not by numeric capture indices. Non-capturing groups(?:...)do not affect named group extraction, so the change is safe.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@cosmo0920 which release introduces this issue and can we update the linked issue with that info? Just so can track it for anyone who sees it. |
patrick-stephens
changed the title
|
@cosmo0920 pls squash the commits (we will need a backport of this one) |
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-fix-accidentally-removed-u-in-prefix
branch
from
5fbd345 to
3396a35
Compare
|
I squashed into two commits. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/filter_kubernetes/kube_regex.h (1)
27-27: Consider applying the same(?:...)hygiene toKUBE_JOURNAL_TO_REGEX.
KUBE_JOURNAL_TO_REGEXstill contains an outer unnamed capturing group wrapping a named group:"^(?<name_prefix>[^_]+)_(?<container_name>[^\._]+)(\.(?<container_hash>[^_]+))?_..." // ^ ^ // unnamed outer capturing group (optional)The structure is inverted from the bug fixed here (unnamed outer wrapping named inner rather than unnamed inner inside named outer), but the unnamed group is still subject to the same Oniguruma compile-option dependency. Converting it to non-capturing is a safe, zero-behavior-change cleanup:
♻️ Suggested cleanup
-#define KUBE_JOURNAL_TO_REGEX "^(?<name_prefix>[^_]+)_(?<container_name>[^\._]+)(\.(?<container_hash>[^_]+))?_(?<pod_name>[^_]+)_(?<namespace_name>[^_]+)_[^_]+_[^_]+$" +#define KUBE_JOURNAL_TO_REGEX "^(?<name_prefix>[^_]+)_(?<container_name>[^\._]+)(?:\.(?<container_hash>[^_]+))?_(?<pod_name>[^_]+)_(?<namespace_name>[^_]+)_[^_]+_[^_]+$"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/filter_kubernetes/kube_regex.h` at line 27, The regex constant KUBE_JOURNAL_TO_REGEX contains an unnecessary outer unnamed capturing group around the optional ".<container_hash>" segment — change that group to a non-capturing group so it becomes (?:\.(?<container_hash>[^_]+))? instead of (\.(?<container_hash>[^_]+))?; update the literal in KUBE_JOURNAL_TO_REGEX so the behavior is identical but no unnamed capture is created (refer to the KUBE_JOURNAL_TO_REGEX macro name to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/filter_kubernetes/kube_regex.h`:
- Line 27: The regex constant KUBE_JOURNAL_TO_REGEX contains an unnecessary
outer unnamed capturing group around the optional ".<container_hash>" segment —
change that group to a non-capturing group so it becomes
(?:\.(?<container_hash>[^_]+))? instead of (\.(?<container_hash>[^_]+))?; update
the literal in KUBE_JOURNAL_TO_REGEX so the behavior is identical but no unnamed
capture is created (refer to the KUBE_JOURNAL_TO_REGEX macro name to locate the
change).
…n regex Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-fix-accidentally-removed-u-in-prefix
branch
from
3396a35 to
ccb5137
Compare
3 participants
Closes #11322
In the current filter_kubernetes, the pod_name placeholder on regex sometimes causes backtracking and overlooked the beginning letter of u.
With this patch, preventing to execute backtracking and plug the occurrences of missing u prefix in pod_name.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Bug Fixes
Tests