aws_credential_sts: internal: Fix unit test failures #11077
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
WalkthroughRefactors STS/ EKS test code to replace static XML response strings with runtime-generated payloads that include TTL-based Expiration timestamps. Adds builders returning allocated buffers and an attach helper to set those buffers as HTTP client responses; updates tests to allocate, attach, pass payload lengths, and free payloads. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test code
participant Builder as Payload Builder
participant HTTP as flb_http_client (test harness)
participant Parser as flb_parse_sts_resp
Test->>Builder: build_*_response_with_ttl_calloc(ttl)
Note right of Builder: returns allocated buffer + length
Builder-->>Test: payload (owned), payload_len
Test->>HTTP: http_test_attach_owned_payload(client, payload, payload_len)
Note right of HTTP: sets status, len, body pointer (owns buf)
Test->>Parser: flb_parse_sts_resp(body, payload_len)
Parser-->>Test: parsed credentials / result
Test->>HTTP: (test teardown) ensure payload freed / client cleaned
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/internal/aws_credentials_sts.c(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/internal/aws_credentials_sts.c (4)
include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_config.c (1)
flb_config_init(226-440)src/aws/flb_aws_credentials.c (1)
flb_aws_credentials_destroy(752-767)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (2)
tests/internal/aws_credentials_sts.c (2)
55-101: Excellent solution for time-adaptive test responses.The introduction of
build_eks_response_with_ttl_callocandbuild_sts_response_with_ttl_calloceffectively addresses the timing issues described in the PR objectives by generating responses with dynamic expiration timestamps. The implementation demonstrates several good practices:
- Thread-safe time handling with
gmtime_r- Correct buffer sizing using two-pass
snprintf- Proper error handling with NULL checks and
flb_errno()- Clean API with optional
out_lenparameterThis approach makes the tests resilient to timing variations that could cause failures.
Also applies to: 103-148
150-166: Clarify that payloads are freed but mock client structs may leak.The dynamically allocated payloads in
request_eks_test1,request_eks_flb_sts_session_name, andrequest_sts_test1are correctly freed viaflb_http_client_destroy, which callsflb_free(c->resp.data)when processing the mock clients. However, the mockflb_http_clientstructs themselves (allocated withflb_calloc) are not freed anywhere—flb_http_client_destroyonly frees the fields within the struct, not the struct itself.The test functions should ensure that all allocated mock client structs are explicitly freed after the provider processes them, either by tracking them or by wrapping the cleanup in the test teardown logic.
Verify by inspecting the test cleanup code: Do any test functions track or manually free the mock HTTP clients returned by the request helper functions? If not, add explicit cleanup to prevent unreachable memory blocks (which Valgrind may report as reachable leaks).
cosmo0920
force-pushed
the
cosmo0920-fix-aws-sts-unit-test-errors
branch
from
924e2d6 to
135577b
Compare
…iled Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-fix-aws-sts-unit-test-errors
branch
from
135577b to
2ac9e3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/internal/aws_credentials_sts.c (3)
213-217: Memory leak on allocation failure (previously flagged).If
build_eks_response_with_ttl_callocreturns NULL, the allocatedflb_http_clientstructc(line 207) is leaked when returning NULL at line 216.Apply this diff to free
cbefore returning:payload = build_eks_response_with_ttl_calloc(3600, &payload_len); TEST_CHECK(payload != NULL); if (!payload) { + flb_free(c); return NULL; }
250-254: Memory leak on allocation failure (previously flagged).Same issue as
request_eks_test1: ifbuild_eks_response_with_ttl_callocreturns NULL, the allocated clientc(line 244) is leaked.Apply this diff:
payload = build_eks_response_with_ttl_calloc(3600, &payload_len); TEST_CHECK(payload != NULL); if (!payload) { + flb_free(c); return NULL; }
312-316: Memory leak on allocation failure (previously flagged).Same issue: if
build_sts_response_with_ttl_callocreturns NULL, the allocated clientc(line 306) is leaked.Apply this diff:
payload = build_sts_response_with_ttl_calloc(3600, &payload_len); TEST_CHECK(payload != NULL); if (!payload) { + flb_free(c); return NULL; }
🧹 Nitpick comments (1)
tests/internal/aws_credentials_sts.c (1)
103-148: LGTM: STS payload builder follows same reliable pattern.The implementation correctly mirrors
build_eks_response_with_ttl_callocwith the appropriate AssumeRole XML template.The two builder functions share substantial logic. Consider extracting common code into a helper that takes the template as a parameter to reduce duplication, though this is a minor optimization for test code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/internal/aws_credentials_sts.c(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/internal/aws_credentials_sts.c (4)
include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_config.c (1)
flb_config_init(226-440)src/aws/flb_aws_credentials.c (1)
flb_aws_credentials_destroy(752-767)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (3)
tests/internal/aws_credentials_sts.c (3)
55-101: LGTM: Dynamic payload builder is well-implemented.The function correctly generates time-adaptive XML responses. The two-pass
snprintfpattern safely calculates buffer size, andgmtime_rensures thread safety. Memory ownership is clear: callers must free the returned buffer.
150-166: LGTM: Test helper correctly attaches owned payload.The function properly transfers ownership of the dynamically allocated payload to the HTTP client mock. Setting
headers_endtodata(line 159) is unusual but likely intentional for simplified test mocking where header parsing is skipped.
466-494: LGTM: Test properly manages dynamic payload lifecycle.The test correctly generates a time-adaptive payload, uses it, and frees it (line 492). Memory management is sound with appropriate early returns and cleanup.
|
ended up hitting this as well and then found this. if you wanted a simpler idea, i just wrote the date way into the future diff --git a/tests/internal/aws_credentials_sts.c b/tests/internal/aws_credentials_sts.c
index 146d937e7..52b705c91 100644
--- a/tests/internal/aws_credentials_sts.c
+++ b/tests/internal/aws_credentials_sts.c
@@ -45,7 +45,7 @@ xmlns=\"https://sts.amazonaws.com/doc/2011-06-15/\">\n\
<Credentials>\n\
<SessionToken>eks_token</SessionToken>\n\
<SecretAccessKey>eks_skid</SecretAccessKey>\n\
- <Expiration>2025-10-24T23:00:23Z</Expiration>\n\
+ <Expiration>2099-12-31T23:59:59Z</Expiration>\n\
<AccessKeyId>eks_akid</AccessKeyId>\n\
</Credentials>\n\
<Provider>www.amazon.com</Provider>\n\
@@ -67,7 +67,7 @@ xmlns=\"https://sts.amazonaws.com/doc/\n\
<AccessKeyId>sts_akid</AccessKeyId>\n\
<SecretAccessKey>sts_skid</SecretAccessKey>\n\
<SessionToken>sts_token</SessionToken>\n\
- <Expiration>2025-11-09T13:34:41Z</Expiration>\n\
+ <Expiration>2099-12-31T23:59:59Z</Expiration>\n\
</Credentials>\n\
<PackedPolicySize>6</PackedPolicySize>\n\
</AssumeRoleResult>\n\ |
Yeah, it's simple solution but I don't like it. Because the actual AWS responses have 3600 or 7200 minutes expiry constraints. |
Currently, one of the AWS STS Credentials test is failed.
So, we need to create the responses dynamically to fit the time constraint.
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