Support vhost-style S3 URLs #10878
Support vhost-style S3 URLs #10878
Conversation
WalkthroughRefactors endpoint setup by adding init_endpoint and invoking it in initialization. Introduces a new vhost_style_urls config and struct field, and updates URI construction for PutObject and multipart operations to support virtual-host addressing. Adds a runtime test enabling vhost-style URLs. No external APIs changed beyond configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant FB as Fluent Bit S3 Plugin
participant Cfg as Config Map
participant EP as init_endpoint()
participant S3 as S3/OSS Service
rect rgb(239,247,255)
note over FB,Cfg: Initialization
FB->>Cfg: Read settings (region, endpoint, vhost_style_urls)
FB->>EP: Build endpoint (scheme/host/port)
EP-->>FB: ctx.endpoint
end
rect rgb(245,252,240)
note over FB,S3: PutObject upload
alt vhost_style_urls = true
FB->>S3: PUT https://<bucket>.<host>/<object-key>
else path-style
FB->>S3: PUT https://<host>/<bucket>/<object-key>
end
S3-->>FB: Response
end
sequenceDiagram
autonumber
participant FB as Fluent Bit S3 Plugin
participant S3 as S3/OSS Service
rect rgb(245,252,240)
note over FB,S3: Multipart upload (no presigned URL)
alt vhost_style_urls = true
FB->>S3: POST https://<bucket>.<host>/<key>?uploads=
S3-->>FB: UploadId
loop parts
FB->>S3: PUT https://<bucket>.<host>/<key>?partNumber=N&uploadId=ID
S3-->>FB: ETag
end
FB->>S3: POST https://<bucket>.<host>/<key>?uploadId=ID
else path-style
FB->>S3: POST https://<host>/<bucket>/<key>?uploads=
S3-->>FB: UploadId
loop parts
FB->>S3: PUT https://<host>/<bucket>/<key>?partNumber=N&uploadId=ID
S3-->>FB: ETag
end
FB->>S3: POST https://<host>/<bucket>/<key>?uploadId=ID
end
S3-->>FB: Complete response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
|
![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
🧹 Nitpick comments (2)
tests/runtime/out_s3.c (1)
231-275: Test skips silently when environment variables are missing.The test returns early without any indication when
FLB_OUT_S3_TEST_BUCKETorFLB_OUT_S3_TEST_REGIONare not set. This could lead to false positives in test suites.Consider using
TEST_CHECKwith a message orflb_warnto indicate the test was skipped:bucket = getenv("FLB_OUT_S3_TEST_BUCKET"); if (bucket == NULL) { + TEST_MSG("Test skipped: FLB_OUT_S3_TEST_BUCKET not set"); return; }plugins/out_s3/s3.c (1)
663-664: Potential memory leak on endpoint reassignment.When vhost-style URLs are enabled, the code frees the old endpoint only if
ctx->free_endpoint == FLB_TRUE. However,ctx->free_endpointis set toFLB_TRUEearlier in the function (line 628 or 643), so this condition will always be true at this point. This is correct, but the code could be clearer.Consider simplifying the logic since
free_endpointis always true at this point:-// Free the old one since we no longer need it -if (ctx->free_endpoint == FLB_TRUE) { - flb_free(ctx->endpoint); -} +// Free the old endpoint since we allocated it +flb_free(ctx->endpoint);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/out_s3/s3.c(6 hunks)plugins/out_s3/s3.h(1 hunks)plugins/out_s3/s3_multipart.c(3 hunks)tests/runtime/out_s3.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
plugins/out_s3/s3_multipart.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
tests/runtime/out_s3.c (1)
src/flb_lib.c (9)
flb_create(138-220)flb_input(261-271)flb_input_set(300-330)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_lib_push(774-801)flb_stop(942-985)flb_destroy(223-258)
plugins/out_s3/s3.c (5)
src/flb_output.c (1)
flb_output_get_property(1096-1099)src/aws/flb_aws_util.c (2)
removeProtocol(165-170)flb_aws_endpoint(75-117)src/flb_utils.c (2)
flb_utils_split(464-467)flb_utils_split_free(477-489)src/flb_sds.c (3)
flb_sds_create_size(92-95)flb_sds_printf(336-387)flb_sds_destroy(389-399)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (10)
plugins/out_s3/s3.h (1)
125-125: LGTM! Field placement is appropriate.The new
vhost_style_urlsfield is correctly placed in the configuration section of the struct, grouped with other connection-related settings likeinsecureandport.tests/runtime/out_s3.c (1)
264-265: Good test coverage for the new feature.The test correctly validates the vhost-style URLs feature by:
- Setting
vhost_style_urlsto "true"- Using
use_put_objectto test with the simpler API path- Following the existing test pattern
plugins/out_s3/s3_multipart.c (3)
432-438: Consistent vhost-style URL handling in multipart operations.The implementation correctly handles vhost-style URLs for
complete_multipart_uploadby:
- Checking if
ctx->vhost_style_urls == FLB_TRUE- Constructing the URI without the bucket prefix when vhost-style is enabled
- Maintaining backward compatibility with path-style URLs
580-584: Consistent implementation across all multipart operations.The vhost-style URL logic is correctly applied to
create_multipart_uploadwith the same pattern as other operations.
708-716: Complete coverage of multipart operations.The vhost-style URL support is consistently implemented across all three multipart operations (complete, create, upload_part).
plugins/out_s3/s3.c (5)
589-677: Well-structured endpoint initialization with vhost-style support.The new
init_endpointfunction properly:
- Handles both custom endpoints and default region-based endpoints
- Correctly parses protocol (http/https) and ports
- Implements vhost-style URL construction by prepending the bucket name
- Properly manages memory with appropriate cleanup
880-884: Clean integration of endpoint initialization.The endpoint initialization is properly integrated into the plugin initialization flow with appropriate error handling.
1552-1555: Correct URI construction for vhost-style in PutObject.The implementation properly handles vhost-style URLs in the
s3_put_objectfunction:
- Excludes bucket prefix from URI when vhost-style is enabled
- Maintains consistent behavior for both random-suffix and standard key formats
Also applies to: 1569-1584
1640-1644: URI display logic correctly handles vhost-style URLs.The code properly adjusts the displayed object key based on whether vhost-style URLs are in use.
4098-4103: Clear and helpful configuration option.The new configuration option is well-documented with:
- Clear description of the feature
- Sensible default value (false for backward compatibility)
- Link to AWS documentation for reference
| if (ctx->vhost_style_urls == FLB_TRUE) { | ||
| // Add 1 because we need an extra dot | ||
| len = strlen(ctx->endpoint) + strlen(ctx->bucket) + 1; | ||
| url = flb_sds_create_size(len); | ||
| tmp_sds = flb_sds_printf(&url, "%s.%s", ctx->bucket, ctx->endpoint); | ||
| if (!tmp_sds) { | ||
| flb_sds_destroy(url); | ||
| flb_plg_error(ctx->ins, "Could not construct vhost-style S3 endpoint"); | ||
| return -1; | ||
| } | ||
| url = tmp_sds; | ||
|
|
||
| // Free the old one since we no longer need it | ||
| if (ctx->free_endpoint == FLB_TRUE) { | ||
| flb_free(ctx->endpoint); | ||
| } | ||
|
|
||
| ctx->endpoint = flb_strndup(url, flb_sds_len(url)); | ||
| flb_sds_destroy(url); | ||
| if (ctx->endpoint == NULL) { | ||
| flb_plg_error(ctx->ins, "error duplicating endpoint string"); | ||
| return -1; | ||
| } | ||
| flb_plg_info(ctx->ins, "New endpoint: %s", ctx->endpoint); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify bucket name compatibility with vhost-style URLs.
When using vhost-style URLs, bucket names must be DNS-compliant. The code should validate that the bucket name doesn't contain characters that would make it invalid as a DNS hostname (e.g., underscores, uppercase letters).
Consider adding validation for DNS-compliant bucket names when vhost-style URLs are enabled:
if (ctx->vhost_style_urls == FLB_TRUE) {
// Validate bucket name is DNS-compliant
// - Must be 3-63 characters
// - Must start and end with lowercase letter or number
// - Can contain lowercase letters, numbers, and hyphens
// - Cannot contain consecutive periods or be formatted as IP address
}🏁 Script executed:
#!/bin/bash
# Check if there's existing bucket name validation in the codebase
rg -n "bucket.*valid|valid.*bucket" --type c
# Look for DNS validation patterns
rg -n "DNS|dns.*compliant|hostname.*valid" --type c
# Check for bucket name restrictions in documentation
fd README -e md -x grep -l "bucket.*name.*restriction\|DNS"Length of output: 107459
Validate bucket name before constructing vhost-style endpoint (DNS‑compliant).
plugins/out_s3/s3.c (around lines 650–674) currently concatenates ctx->bucket and ctx->endpoint into a hostname without validating the bucket. Add a DNS-compatibility check and fall back to path-style (or return an error) when the bucket is not DNS-compatible. Enforce: 3–63 chars; only lowercase letters, digits, hyphen and dot; start/end with letter or digit; no consecutive dots; not IPv4-formatted. A practical check: regex ^[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$ AND reject /(\d+.){3}\d+/ and '..' before using vhost-style formatting.
Vhost-style URLs for S3 have been an option for a while, yet fluent-bit exclusively supports path-style URL's, which forbids it to work with the S3 FIPS endpoints according to the documentation.
This PR adds optional, opt-in support for vhost-style URLs on a per-output basis.
Fixes #10390.
Testing
Before we can approve your change; please submit the following in a comment:
Documentation
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