opentelemetry: validate JSON container value types - #12131
Merged
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOpenTelemetry JSON wrapped-value parsing now validates nested container shapes and dispatches arrays and maps by their actual msgpack types. Conversion helpers reject invalid arguments, empty maps can represent empty arrays, and integration coverage exercises malformed AnyValue containers. ChangesOpenTelemetry JSON validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
edsiper
changed the title
edsiper
marked this pull request as ready for review
July 22, 2026 03:37
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1253cb3949
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+184
to
+186
| if (internal_type == MSGPACK_OBJECT_ARRAY && | ||
| kv_value->type != MSGPACK_OBJECT_ARRAY) { | ||
| return -2; |
There was a problem hiding this comment.
When an OTLP/JSON client sends an empty ArrayValue as {"arrayValue": {}}, the protobuf JSON mapping omits the repeated values field, so this is a valid empty array. This new check returns -2 whenever the resolved array wrapper is a map rather than an array, so the log body path no longer unwraps it as [] and instead falls back to serializing the raw wrapper map; the same wrapper is also rejected by shared AnyValue callers. Please keep the empty-map wrapper case for arrayValue and only reject scalar/non-container values payloads.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/opentelemetry/flb_opentelemetry_utils.c`:
- Around line 184-191: Update the nested-wrapper handling around
flb_otel_utils_json_payload_append_unwrapped_value() and
flb_otel_utils_json_payload_append_converted_map() so malformed wrapper shapes
rejected by the internal_type checks propagate a distinct error instead of being
treated as ordinary maps. Preserve the separate “not wrapped” result for valid
non-wrapper maps, and add a regression covering an arrayValue containing an
invalid values field that must be rejected rather than emitted verbatim.
- Around line 178-180: Update the key validation around the values-wrapper check
to require both an exact length of 6 and a case-insensitive match against
“values”; return -3 for shorter, longer, or otherwise different keys while
preserving acceptance of the exact key.
In
`@tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py`:
- Around line 854-900: Update
test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container to
distinguish scalar invalid containers from the map-form kvlist case. Assert that
arrayValue and scalar kvlistValue inputs are dropped or emit no records (or
verify the relevant drop metric), while map-form kvlistValue produces the
expected emitted structure; retain the existing status and process-liveness
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61d61ef7-760e-42ef-bc7b-3f1f4d4aa340
📒 Files selected for processing (4)
plugins/in_opentelemetry/opentelemetry_utils.cplugins/in_opentelemetry/opentelemetry_utils.hsrc/opentelemetry/flb_opentelemetry_utils.ctests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py
💤 Files with no reviewable changes (2)
- plugins/in_opentelemetry/opentelemetry_utils.h
- plugins/in_opentelemetry/opentelemetry_utils.c
edsiper
force-pushed
the
otlp-json-anyvalue-noissue
branch
from
July 29, 2026 17:43
1253cb3 to
ce1cdec
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/opentelemetry/flb_opentelemetry_utils.c (1)
171-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required Allman brace layout in the new C blocks.
src/opentelemetry/flb_opentelemetry_utils.c#L171-L193: move newif/else ifopening braces to their own lines.src/opentelemetry/flb_opentelemetry_utils.c#L345-L378: apply the same layout to array/map dispatch.src/opentelemetry/flb_opentelemetry_utils.c#L407-L410: apply it to the argument guard.src/opentelemetry/flb_opentelemetry_utils.c#L464-L467: apply it to the array guard.src/opentelemetry/flb_opentelemetry_utils.c#L508-L511: apply it to the kvlist guard.src/opentelemetry/flb_opentelemetry_utils.c#L898-L917: apply it to CFL array conversion branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/opentelemetry/flb_opentelemetry_utils.c` around lines 171 - 193, Reformat the new conditional blocks to use Allman braces, placing every if, else if, and else opening brace on its own line. Apply this consistently in src/opentelemetry/flb_opentelemetry_utils.c at ranges 171-193, 345-378, 407-410, 464-467, 508-511, and 898-917, without changing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/opentelemetry/flb_opentelemetry_utils.c`:
- Around line 171-193: Reformat the new conditional blocks to use Allman braces,
placing every if, else if, and else opening brace on its own line. Apply this
consistently in src/opentelemetry/flb_opentelemetry_utils.c at ranges 171-193,
345-378, 407-410, 464-467, 508-511, and 898-917, without changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0531f26-b59c-4cec-b0aa-e0704ddbc734
📒 Files selected for processing (4)
plugins/in_opentelemetry/opentelemetry_utils.cplugins/in_opentelemetry/opentelemetry_utils.hsrc/opentelemetry/flb_opentelemetry_utils.ctests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py
💤 Files with no reviewable changes (2)
- plugins/in_opentelemetry/opentelemetry_utils.c
- plugins/in_opentelemetry/opentelemetry_utils.h
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
force-pushed
the
otlp-json-anyvalue-noissue
branch
from
July 29, 2026 18:27
ce1cdec to
f16e3f8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
arrayValueandkvlistValuecontainers before conversionvaluesfields before any MessagePack array/map union accessRoot cause
flb_otel_utils_json_payload_get_wrapped_value()assigned the semantic AnyValue type from the wrapper key, then returned the nestedvaluesobject without consistently validating its actual MessagePack type. The downstream array and kvlist conversion paths could therefore interpret a scalar object's union as a container.Related-path audit
The same missing validation affected
kvlistValue.values, so this PR fixes both container paths and adds defensive checks at all conversion entry points. The metrics and traces callers share the corrected Fluent Bit utility. The cmetrics and ctraces bundled decoders use different conversion paths and do not require changes.Compatibility
Valid OTLP/JSON arrays and kvlists continue to decode as before. Invalid scalar container payloads are rejected without terminating the worker. Existing map-form kvlists remain supported.
Validation
cmake --build build -j8— passedctest --test-dir build -R flb-it-opentelemetry --output-on-failure— passedtests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container -q— 3 passedVALGRIND=1 VALGRIND_STRICT=1 tests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container -q— 3 passedGITHUB_EVENT_NAME=pull_request GITHUB_BASE_REF=master tests/integration/.venv/bin/python .github/scripts/commit_prefix_check.py— passedSummary by CodeRabbit
arrayValue/kvlistValuebody shapes, asserting HTTP status and emitted output.