plugins: fix dry-run segmentation faults#12114
Open
patrick-stephens wants to merge 3 commits into
Open
Conversation
Signed-off-by: Patrick Stephens <pat@telemetryforge.io>
Signed-off-by: Patrick Stephens <pat@telemetryforge.io>
Signed-off-by: Patrick Stephens <pat@telemetryforge.io>
📝 WalkthroughWalkthroughOutput teardown functions for Azure Blob, Kafka REST, and NATS now guard against ChangesOutput teardown safety
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Contributor
Author
|
@cosmo0920 probably need to add some dry run tests for these plugins but is there a way we can add dry run tests to every plugin easily? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/out_azure_blob/azure_blob.c (1)
2433-2435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required brace style for the new guards.
The four added
ifstatements place the opening brace on the same line. Reformat them with the opening brace on the next line to match the repository’s C/C++ convention:
plugins/out_azure_blob/azure_blob.cLines 2433-2435plugins/out_kafka_rest/kafka.cLines 341-343plugins/out_kafka_rest/kafka_conf.cLines 200-202plugins/out_nats/nats.cLines 234-236As per coding guidelines, C/C++ control-statement opening braces must be placed on the next line.
🤖 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_azure_blob/azure_blob.c` around lines 2433 - 2435, Reformat the four newly added guard statements so each opening brace is on the following line, matching the repository’s C/C++ brace convention: apply this change to plugins/out_azure_blob/azure_blob.c lines 2433-2435, plugins/out_kafka_rest/kafka.c lines 341-343, plugins/out_kafka_rest/kafka_conf.c lines 200-202, and plugins/out_nats/nats.c lines 234-236. Preserve the existing guard logic and returns.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 `@plugins/out_azure_blob/azure_blob.c`:
- Around line 2433-2435: Reformat the four newly added guard statements so each
opening brace is on the following line, matching the repository’s C/C++ brace
convention: apply this change to plugins/out_azure_blob/azure_blob.c lines
2433-2435, plugins/out_kafka_rest/kafka.c lines 341-343,
plugins/out_kafka_rest/kafka_conf.c lines 200-202, and plugins/out_nats/nats.c
lines 234-236. Preserve the existing guard logic and returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9ffd8a0-5d6f-45f0-80d9-adb134182655
📒 Files selected for processing (4)
plugins/out_azure_blob/azure_blob.cplugins/out_kafka_rest/kafka.cplugins/out_kafka_rest/kafka_conf.cplugins/out_nats/nats.c
Contributor
For output tests, we can implement it like as below: For windows testing: dry_run_outputs.ps1. "$PSScriptRoot/common.ps1"
try {
Assert-RuntimeEnvironment
$schemaOutput = & $env:FLB_BIN -J 2>&1 | Out-String
$schemaExitCode = $LASTEXITCODE
if ($schemaExitCode -ne 0) {
throw "Could not list compiled plugins (exit code $schemaExitCode)"
}
$schema = $schemaOutput | ConvertFrom-Json
$plugins = @($schema.outputs | ForEach-Object { $_.name })
if ($plugins.Count -eq 0) {
throw "No compiled output plugins were reported"
}
$failures = [System.Collections.Generic.List[string]]::new()
foreach ($plugin in $plugins) {
$output = & $env:FLB_BIN --dry-run -i dummy -o $plugin 2>&1 |
Out-String
$exitCode = $LASTEXITCODE
$hasSuccessMarker = $output.Contains(
"configuration test is successful")
if ($exitCode -eq 0 -and $hasSuccessMarker) {
Write-Host "PASS: $plugin"
}
else {
Write-Host "FAIL: $plugin (exit code $exitCode)"
Write-Host $output
$failures.Add($plugin)
}
}
if ($failures.Count -ne 0) {
throw ("Dry-run failed for output plugin(s): {0}" -f
($failures -join ", "))
}
Write-Host "Dry-run passed for all $($plugins.Count) compiled output plugins"
exit 0
}
catch {
Write-Host "ERROR: $($_.Exception.Message)"
exit 1
}For unix-shell enabled environments: dry_run_outputs.sh#!/bin/sh
FLB_ROOT=${FLB_ROOT:-$(cd "$(dirname "$0")/../.." && pwd)}
FLB_BIN=${FLB_BIN:-$FLB_ROOT/build/bin/fluent-bit}
SCHEMA_OUTPUT=$("$FLB_BIN" -J 2>&1)
SCHEMA_EXIT_CODE=$?
if [ "$SCHEMA_EXIT_CODE" -ne 0 ]; then
echo "Could not list compiled plugins (exit code $SCHEMA_EXIT_CODE)"
echo "$SCHEMA_OUTPUT"
exit 1
fi
PLUGINS=$(printf '%s\n' "$SCHEMA_OUTPUT" |
grep -o '"type":"output","name":"[^"]*"' |
sed 's/^"type":"output","name":"//; s/"$//')
if [ -z "$PLUGINS" ]; then
echo "No compiled output plugins were reported"
exit 1
fi
FAILURES=""
FAILURE_COUNT=0
PLUGIN_COUNT=0
for PLUGIN in $PLUGINS; do
PLUGIN_COUNT=$((PLUGIN_COUNT + 1))
OUTPUT=$("$FLB_BIN" --dry-run -i dummy -o "$PLUGIN" 2>&1)
EXIT_CODE=$?
if [ "$EXIT_CODE" -eq 0 ] &&
printf '%s\n' "$OUTPUT" | grep -Fq "configuration test is successful"; then
echo "PASS: $PLUGIN"
else
echo "FAIL: $PLUGIN (exit code $EXIT_CODE)"
echo "$OUTPUT"
FAILURES="$FAILURES $PLUGIN"
FAILURE_COUNT=$((FAILURE_COUNT + 1))
fi
done
if [ "$FAILURE_COUNT" -ne 0 ]; then
echo "Dry-run failed for output plugin(s):$FAILURES"
exit 1
fi
echo "Dry-run passed for all $PLUGIN_COUNT compiled output plugins"
exit 0And adding the below lines are needed to enabled these test cases: |
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
2 participants
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.
Various plugins trigger segmentation faults on exit due to missing null checks similar to #11800. This was detected during documentation checks of examples configurations: fluent/fluent-bit-docs#2627
Resolves #12113.
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:
Using the official documented configuration for each of these plugins:
Debug log output from testing the change
Each reports
configuration test is successfulnow when using the container image.Attached Valgrind output that shows no leaks or memory corruption was found
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