out_cloudwatch_logs: Ensure escaping backslashes on cloudwatch API requests' body#11933
Open
cosmo0920 wants to merge 5 commits into
Open
out_cloudwatch_logs: Ensure escaping backslashes on cloudwatch API requests' body#11933cosmo0920 wants to merge 5 commits into
cosmo0920 wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes JSON escaping for CloudWatch log group and stream names, adds builders for CreateLogStream and PutLogEvents payloads that use escaped names (with sizing fixes), adds runtime tests validating escaping, and updates Windows CI matrix to choose OS per matrix entry and add an ARM64 runner and conditional WiX install. ChangesCreateLogStream and PutLogEvents JSON escaping
Windows CI matrix update
Sequence Diagram(s)sequenceDiagram
participant Client
participant send_log_events
participant flb_cloudwatch_init_put_payload
participant escape_stream_names
participant HTTP_Buffer
Client->>send_log_events: trigger flush
send_log_events->>flb_cloudwatch_init_put_payload: init PutLogEvents header
flb_cloudwatch_init_put_payload->>escape_stream_names: request escaped group/name
escape_stream_names->>flb_cloudwatch_init_put_payload: return escaped strings
flb_cloudwatch_init_put_payload->>HTTP_Buffer: write header with escaped names
flb_cloudwatch_init_put_payload->>send_log_events: return offset/size
send_log_events->>HTTP_Buffer: append events and send HTTP request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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: c5b8657cdb
ℹ️ 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".
cosmo0920
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-ensure-escape-backslashes-on-cloudwatch
branch
from
1baae41 to
7308334
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_cloudwatch_logs/cloudwatch_api.c (1)
1852-1852:⚠️ Potential issue | 🟠 MajorEscaping is missing for
logGroupNamein CreateLogGroup / PutRetentionPolicy JSON bodies
create_log_group()builds{"logGroupName":"%s"}using rawstream->group(e.g.,cloudwatch_api.c:1852) without applyingescape_stream_names().set_log_group_retention()builds{"logGroupName":"%s","retentionInDays":%d}using rawstream->group(e.g.,cloudwatch_api.c:1786) with the same lack of escaping.escape_stream_names()is used for PutLogEvents (flb_cloudwatch_init_put_payload), so these requests should apply the same escaping when interpolatingstream->groupinto JSON to avoid invalid payloads when names contain JSON-sensitive characters.🤖 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 `@plugins/out_cloudwatch_logs/cloudwatch_api.c` at line 1852, create_log_group() and set_log_group_retention() interpolate stream->group directly into JSON bodies which can produce invalid payloads; use the existing escape_stream_names() (the same escaping applied by flb_cloudwatch_init_put_payload for PutLogEvents) before formatting the JSON. Modify create_log_group() and set_log_group_retention() to call escape_stream_names(&escaped, stream->group) (or equivalent existing API), use the escaped value when building the body (instead of stream->group), and free the escaped buffer after use to avoid leaks.
🧹 Nitpick comments (2)
.github/workflows/call-build-windows.yaml (2)
210-214: ⚡ Quick winDocument the WiX pre-installation assumption.
This step installs WiX only for ARM64, implying that WiX is pre-installed on
windows-latestbut not onwindows-11-arm. This assumption is fragile and could break silently if GitHub runner images change.📝 Suggested improvement
Add a comment documenting the assumption:
+ # WiX is pre-installed on windows-latest runners but not on windows-11-arm + # Install WiX explicitly for ARM64 builds to ensure MSI packaging works - name: Get WiX Toolset w/ chocolatey if: ${{ matrix.config.arch == 'amd64_arm64' }}🤖 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 @.github/workflows/call-build-windows.yaml around lines 210 - 214, Add an explicit comment above the "Get WiX Toolset w/ chocolatey" job step (the step using crazy-max/ghaction-chocolatey with args: install wixtoolset -y and guarded by if: ${{ matrix.config.arch == 'amd64_arm64' }}) documenting the assumption that the windows-latest runner already includes WiX while the windows-11-arm runner does not; note that this is a fragile image-specific assumption and call out that the conditional intentionally restricts installation to the ARM64 matrix case so future maintainers know why the install is skipped for other runners.
210-214: ⚡ Quick winConsider verifying WiX availability for all configurations.
Since MSI packages are created for all architectures (line 277:
*-bit-*.msi), WiX must be available for all builds, not just ARM64. The current approach assumes WiX is pre-installed onwindows-latestbut provides no verification. If that assumption breaks, x86/x64 builds could fail with unclear errors.🛡️ Suggested verification step
Add a verification step after WiX installation (for all configs or just ARM64) to ensure the tool is available:
- name: Verify WiX Toolset availability run: | if (-not (Test-Path "$ENV:WIX\bin\candle.exe")) { throw "WiX Toolset not found. Expected at: $ENV:WIX\bin\candle.exe" } Write-Host "WiX Toolset found at: $ENV:WIX" & "$ENV:WIX\bin\candle.exe" -? shell: pwshAlternatively, install WiX explicitly for all configurations to remove the pre-installation dependency:
- name: Get WiX Toolset w/ chocolatey - if: ${{ matrix.config.arch == 'amd64_arm64' }} uses: crazy-max/ghaction-chocolatey@dff3862348493b11fba2fbc49147b6d2dfe09b66 # v4.0.0 with: args: install wixtoolset -y🤖 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 @.github/workflows/call-build-windows.yaml around lines 210 - 214, The workflow currently only installs WiX in the "Get WiX Toolset w/ chocolatey" step for the amd64_arm64 matrix and assumes WiX exists for other Windows runners, which can cause unclear failures when producing the '*-bit-*.msi' artifacts; either install WiX for all Windows configs (remove the if: condition on the crazy-max/ghaction-chocolatey step) or add a verification step after installation that uses the WIX environment variable to check "$ENV:WIX\bin\candle.exe" exists and exercises it (e.g., run candle.exe -?) so the build fails fast with a clear error if WiX is missing.
🤖 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.
Outside diff comments:
In `@plugins/out_cloudwatch_logs/cloudwatch_api.c`:
- Line 1852: create_log_group() and set_log_group_retention() interpolate
stream->group directly into JSON bodies which can produce invalid payloads; use
the existing escape_stream_names() (the same escaping applied by
flb_cloudwatch_init_put_payload for PutLogEvents) before formatting the JSON.
Modify create_log_group() and set_log_group_retention() to call
escape_stream_names(&escaped, stream->group) (or equivalent existing API), use
the escaped value when building the body (instead of stream->group), and free
the escaped buffer after use to avoid leaks.
---
Nitpick comments:
In @.github/workflows/call-build-windows.yaml:
- Around line 210-214: Add an explicit comment above the "Get WiX Toolset w/
chocolatey" job step (the step using crazy-max/ghaction-chocolatey with args:
install wixtoolset -y and guarded by if: ${{ matrix.config.arch == 'amd64_arm64'
}}) documenting the assumption that the windows-latest runner already includes
WiX while the windows-11-arm runner does not; note that this is a fragile
image-specific assumption and call out that the conditional intentionally
restricts installation to the ARM64 matrix case so future maintainers know why
the install is skipped for other runners.
- Around line 210-214: The workflow currently only installs WiX in the "Get WiX
Toolset w/ chocolatey" step for the amd64_arm64 matrix and assumes WiX exists
for other Windows runners, which can cause unclear failures when producing the
'*-bit-*.msi' artifacts; either install WiX for all Windows configs (remove the
if: condition on the crazy-max/ghaction-chocolatey step) or add a verification
step after installation that uses the WIX environment variable to check
"$ENV:WIX\bin\candle.exe" exists and exercises it (e.g., run candle.exe -?) so
the build fails fast with a clear error if WiX is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 354e4ece-c52e-4690-bdbf-7181610c3886
📒 Files selected for processing (4)
.github/workflows/call-build-windows.yamlplugins/out_cloudwatch_logs/cloudwatch_api.cplugins/out_cloudwatch_logs/cloudwatch_api.htests/runtime/out_cloudwatch.c
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/out_cloudwatch_logs/cloudwatch_api.h
- tests/runtime/out_cloudwatch.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
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.
In the current implementation of cloudwatch logs does not ensure escaping special characters to create log stream.
This causes request failures for creating streams.
This PR fixes this issue.
Closes #11931.
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
Refactor
Tests
Chores