Skip to content

http_client: fixed support of runtime tests#11724

Open
mabrarov wants to merge 4 commits intofluent:masterfrom
mabrarov:feat/http_client_test
Open

http_client: fixed support of runtime tests#11724
mabrarov wants to merge 4 commits intofluent:masterfrom
mabrarov:feat/http_client_test

Conversation

@mabrarov
Copy link
Copy Markdown
Contributor

@mabrarov mabrarov commented Apr 16, 2026

Summary

Fixed support of runtime tests in HTTP client. Refer to #11686 (comment).

The enabled tests were successfully executed on CI within cd068ee temporary commit using fluent/fluent-bit-ci#153 - refer to https://github.com/fluent/fluent-bit/actions/runs/24608193781?pr=11724.


Testing

  • [N/A] Example configuration file for the change.
  • [N/A] Debug log output from testing the change.
  • Attached Valgrind output that shows no leaks or memory corruption was found - refer to flb_run_code_analysis.log for the output of command
    TEST_PRESET=valgrind ./run_code_analysis.sh
  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • [N/A] Documentation required for this feature

Backporting

  • [N/A] Backport to latest stable release.

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

    • Added a null-pointer safety check to the HTTP client connection setup.
  • Tests

    • Elasticsearch and forward output components are now included in default test runs.
  • Chores

    • CI unit-test workflow updated to source reusable test scripts from a different repository reference.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 16, 2026

📝 Walkthrough

Walkthrough

Default test-skip list in the analysis script was reduced (two outputs removed), the HTTP client init now guards access to a possibly-NULL union connection, and CI workflow checkouts were redirected to a different reusable scripts repository/ref.

Changes

Cohort / File(s) Summary
Build script
run_code_analysis.sh
Default SKIP_TESTS assignment changed to remove flb-rt-out_elasticsearch and flb-rt-out_forward, altering which tests are skipped when SKIP_TESTS is unset.
HTTP client
src/flb_http_client.c
create_http_client() now checks u_conn != NULL before reading or temporarily reassigning u_conn->net, preventing null dereference.
CI workflow
.github/workflows/unit-tests.yaml
Unit-test jobs' actions/checkout now pulls reusable ci/ scripts from mabrarov/fluent-bit-ci at ref: feat/enable_tests instead of the previous repository.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • edsiper
  • cosmo0920
  • niedbalski
  • patrick-stephens
  • celalettin1286

Poem

🐇 I hop through patches, light and spry,

trimming skips and guarding why.
A null saved from the midnight fray,
CI points to a new pathway.
Code snug as carrots — hip hooray! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'http_client: fixed support of runtime tests' directly corresponds to the main objective of the PR and is clearly reflected in the code changes, particularly in src/flb_http_client.c which contains the core fix.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mabrarov mabrarov force-pushed the feat/http_client_test branch from c86abe0 to 31424a8 Compare April 16, 2026 17:17
@mabrarov mabrarov force-pushed the feat/http_client_test branch from 76edf7c to f006d83 Compare April 16, 2026 21:44
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/flb_http_client.c (1)

1020-1032: LGTM — null-guard correctly protects u_conn access in test paths.

The added if (u_conn != NULL) wrapper correctly prevents the NPE on u_conn->net / u_conn->upstream when create_http_client() is invoked in runtime-test mode with a NULL connection, while preserving the original behavior for the non-NULL path. When u_conn == NULL, c->original_net_setup stays NULL (from flb_calloc), which is consistent with the NULL checks in http_client_clamp_connection_io_timeout (Line 1499), http_client_update_connection_io_timeout (Line 1524), and http_client_unbind_connection (Line 1592).

Optional: this block now duplicates the net-setup selection logic already centralized in http_client_bind_connection() (Lines 1567–1588). Consider replacing the body with a call to http_client_bind_connection(c, u_conn) after the c->u_conn = u_conn; assignment is removed from Line 1019 — the helper already sets c->u_conn, selects original_net_setup, copies request_net_setup, and repoints u_conn->net. The only behavioral difference is that the helper additionally invokes http_client_update_connection_io_timeout(), which is a no-op here since response_timeout/read_idle_timeout are still 0 at this point, so it should be safe. Feel free to defer if you prefer to keep this PR minimally scoped.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/flb_http_client.c` around lines 1020 - 1032, The duplicated net-setup
logic in create_http_client (the if (u_conn != NULL) block that sets
c->original_net_setup, copies into c->request_net_setup and rewires u_conn->net)
should be replaced by a call to the existing helper
http_client_bind_connection(c, u_conn); remove the manual c->u_conn = u_conn
assignment that precedes it so the helper can set c->u_conn itself; keep in mind
http_client_bind_connection also calls http_client_update_connection_io_timeout
(which is harmless here because response_timeout/read_idle_timeout are 0), so
simply call http_client_bind_connection(c, u_conn) instead of duplicating the
selection and rewire code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/flb_http_client.c`:
- Around line 1020-1032: The duplicated net-setup logic in create_http_client
(the if (u_conn != NULL) block that sets c->original_net_setup, copies into
c->request_net_setup and rewires u_conn->net) should be replaced by a call to
the existing helper http_client_bind_connection(c, u_conn); remove the manual
c->u_conn = u_conn assignment that precedes it so the helper can set c->u_conn
itself; keep in mind http_client_bind_connection also calls
http_client_update_connection_io_timeout (which is harmless here because
response_timeout/read_idle_timeout are 0), so simply call
http_client_bind_connection(c, u_conn) instead of duplicating the selection and
rewire code.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f71e2bd1-e11f-444f-bf3d-d8ce25282e9e

📥 Commits

Reviewing files that changed from the base of the PR and between 856d762 and 2ebb0e7.

📒 Files selected for processing (2)
  • run_code_analysis.sh
  • src/flb_http_client.c

…sts for Forward and Elasticsearch output plugins, for Disk I/O metrics and Process metrics input plugins.

Signed-off-by: Marat Abrarov <[email protected]>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/unit-tests.yaml:
- Around line 116-117: In the unit-test workflow jobs that currently reference
the personal fork (the repository: mabrarov/fluent-bit-ci and ref:
feat/enable_tests entries in the unit-test job), revert those two fields to the
official CI repository and branch used elsewhere (e.g., repository:
fluent/fluent-bit-ci and the stable ref previously used) so both Linux and macOS
unit-test jobs match the other workflows (pr-perf-test.yaml and
call-run-integration-test.yaml); update the repository and ref values in the
unit-test job definitions to remove the personal fork and feature branch.
🪄 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: 434bfef2-66e6-4d3f-a46e-8e3cb76877ee

📥 Commits

Reviewing files that changed from the base of the PR and between 2ebb0e7 and cd068ee.

📒 Files selected for processing (2)
  • .github/workflows/unit-tests.yaml
  • run_code_analysis.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • run_code_analysis.sh

Comment thread .github/workflows/unit-tests.yaml Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant