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 099bb5c temporary commit using fluent/fluent-bit-ci#153 - refer to https://github.com/fluent/fluent-bit/actions/runs/24943722795?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 HTTP client connection setup.
  • Tests

    • Reduced the default test skip list so Elasticsearch and forward outputs are 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

Reduced default skipped-tests in the analysis script, added a null check when initializing HTTP client network pointers to avoid dereferencing a NULL connection, and changed CI unit-test jobs to checkout reusable CI scripts from a different repository/ref.

Changes

Cohort / File(s) Summary
Build script
run_code_analysis.sh
Changed default SKIP_TESTS assignment to remove flb-rt-out_elasticsearch and flb-rt-out_forward, resulting in fewer -DFLB_WITHOUT_* CMake flags when SKIP_TESTS is unset.
HTTP client
src/flb_http_client.c
create_http_client() now checks u_conn != NULL before reading or reassigning u_conn->net, preventing a potential null dereference.
CI workflow
.github/workflows/unit-tests.yaml
Unit-test jobs now checkout mabrarov/fluent-bit-ci at ref: feat/enable_tests into ci/ instead of the previous calyptia/fluent-bit-ci.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • edsiper
  • niedbalski
  • patrick-stephens
  • celalettin1286

Poem

🐇 I hopped through diffs at break of day,

pruned two skips that used to stray.
I fenced a NULL with gentle care,
pointed CI to fresh new air.
Tiny hops, a tidy lair.

🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing HTTP client support for runtime tests, which directly corresponds to the primary modification in src/flb_http_client.c.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
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
@mabrarov mabrarov requested a review from cosmo0920 as a code owner April 17, 2026 00:25
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

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
@cosmo0920
Copy link
Copy Markdown
Contributor

We need to use shorter than 80 characters commit messages:


❌ Commit cd068ee223 failed:
Commit subject too long (>80 chars): 'workflows: temporarily switched to CI scripts with enabled runtime tests for Forward and Elasticsearch output plugins, for Disk I/O metrics and Process metrics input plugins.'

❌ Commit 9664979ccd failed:
Commit subject too long (>80 chars): 'scripts: enabled runtime tests for Forward output plugin in dev code analysis script'


Commit prefix validation failed.
Error: Process completed with exit code 1.

cosmo0920
cosmo0920 previously approved these changes Apr 21, 2026
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
… tests

Signed-off-by: Marat Abrarov <abrarov@gmail.com>
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
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.

2 participants