-
Notifications
You must be signed in to change notification settings - Fork 1.8k
out_s3: blob: fix handling of pre-signed URLs #11246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
When using pre-signed URLs for blob uploads via authorization_endpoint_url), the plugin didn't extract or use the host from the pre-signed URL. It treated the URL as a URI path, so requests went to the wrong host or failed. This patch added s3_parse_presigned_url() to parse pre-signed URLs and extract host, URI, and port Updated put_blob_object(), complete_multipart_upload(), and abort_multipart_upload() to: - Extract the host from the pre-signed URL - Temporarily set ctx->s3_client->host to the extracted host - Validate the port matches the configuration - Restore the original host after the request Now blob uploads using pre-signed URLs now correctly use the host specified in the URL instead of the default S3 client host. Signed-off-by: Eduardo Silva <[email protected]>
WalkthroughA new presigned URL parsing helper is introduced to the S3 output plugin, enabling dynamic endpoint resolution. The function is integrated into blob upload and multipart operations, replacing the original host with the presigned endpoint during transfers and restoring it afterward. Error handling is centralized via cleanup labels to ensure consistent resource deallocation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/out_s3/s3.c (1)
2590-2606: Consider extracting common presigned URL host-swap pattern.The presigned URL handling pattern (parse → validate port → swap host → cleanup) is repeated across five functions (
put_blob_object,complete_multipart_upload,abort_multipart_upload,create_multipart_upload,upload_part).A helper struct + setup/teardown functions could reduce duplication:
struct presigned_context { flb_sds_t host; flb_sds_t uri; const char *original_host; int port; }; int presigned_setup(struct flb_s3 *ctx, const char *url, struct presigned_context *pc); void presigned_cleanup(struct flb_s3 *ctx, struct presigned_context *pc);This is optional and can be deferred if the current approach is preferred for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_s3/s3.c(7 hunks)plugins/out_s3/s3.h(1 hunks)plugins/out_s3/s3_multipart.c(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
plugins/out_s3/s3.h (1)
plugins/out_s3/s3.c (1)
s3_parse_presigned_url(2208-2284)
plugins/out_s3/s3_multipart.c (1)
plugins/out_s3/s3.c (1)
s3_parse_presigned_url(2208-2284)
plugins/out_s3/s3.c (3)
src/flb_utils.c (1)
flb_utils_url_split(1441-1534)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
⏰ 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). (31)
- 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 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 - fuzzing test
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=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 (-DSANITIZE_ADDRESS=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_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_SANITIZE_THREAD=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_SIMD=Off, 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 (-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_SANITIZE_MEMORY=On, 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_SANITIZE_MEMORY=On, 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=Off, 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=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- 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-centos-7
- 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)
🔇 Additional comments (8)
plugins/out_s3/s3.h (1)
221-224: LGTM!The function declaration is well-structured with appropriate parameters for parsing presigned URLs. The signature matches the implementation in
s3.cand follows the existing coding conventions in this header.plugins/out_s3/s3_multipart.c (4)
405-516: LGTM - Cleanup flow is well-structured.The presigned URL handling and cleanup logic is correctly implemented:
- Resources (
body,presigned_host,uri) are properly freed in all pathsoriginal_hostrestoration is guarded by NULL check- Early returns before resource allocation don't leak memory
518-615: LGTM!The abort_multipart_upload function follows the same consistent pattern as complete_multipart_upload. Resource cleanup is properly handled via the
abort_cleanuplabel.
617-723: LGTM!The create_multipart_upload function correctly places the cleanup section before response handling. This ensures that the host is restored and resources are freed regardless of the response outcome, with subsequent returns exiting cleanly.
769-910: LGTM!The upload_part function follows the established pattern correctly. Headers are freed immediately after the request (lines 847-850), and the cleanup section properly handles host restoration and URI cleanup.
plugins/out_s3/s3.c (3)
2208-2284: LGTM - Robust URL parsing with proper cleanup.The implementation correctly:
- Validates required parameters upfront
- Handles port resolution from explicit port or scheme defaults
- Creates owned
flb_sds_tcopies for output- Frees all temporary allocations in both success and error paths
2583-2607: LGTM - Presigned URL handling is correctly integrated.The presigned path properly:
- Fetches the full presigned URL and immediately parses it
- Destroys the temporary
presigned_fullafter parsing to avoid leaks- Validates port consistency before swapping the host
- Cleans up
presigned_hostandurion port validation failure
2660-2670: LGTM - Cleanup section is well-designed.The cleanup label consolidates all resource deallocation and state restoration, ensuring consistent behavior across all exit paths (success, failure, and early error conditions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| original_host = ctx->s3_client->host; | ||
| ctx->s3_client->host = presigned_host; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-signed host never used for network connection
The new pre-signed flow swaps ctx->s3_client->host to the parsed host before issuing the request (plugins/out_s3/s3_multipart.c:440-441), but the HTTP connection still comes from ctx->s3_client->upstream, which is created once with the configured endpoint (plugins/out_s3/s3.c:984-989) and reused in request_do (src/aws/flb_aws_util.c:342-376). When the pre-signed URL points at a different host—the case this change is meant to fix—the TCP/TLS connection (and SNI) stay bound to the original endpoint, so the request is still sent to the wrong server even though the Host header is rewritten. The upstream needs to be rebuilt or the connection opened against the pre-signed host for these URLs to work.
Useful? React with 👍 / 👎.
When using pre-signed URLs for blob uploads via authorization_endpoint_url), the plugin didn't extract or use the host from the pre-signed URL. It treated the URL as a URI path, so requests went to the wrong host or failed.
This patch added s3_parse_presigned_url() to parse pre-signed URLs and extract host, URI, and port Updated put_blob_object(), complete_multipart_upload(), and abort_multipart_upload() to:
Now blob uploads using pre-signed URLs now correctly use the host specified in the URL instead of the default S3 client host.
Example test:
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
✏️ Tip: You can customize this high-level summary in your review settings.