out_s3: add time offset support for s3 key format #10752
Conversation
Noahnut
requested review from
PettitWesley,
edsiper,
fujimotos and
koleini
as code owners
WalkthroughAdds timezone-offset-aware S3 key generation. Public API flb_get_s3_key gains a time_offset parameter; a new flb_aws_parse_tz_offset parser is introduced. The S3 output plugin adds a time_offset config (default "+0000"), stores it in context, and passes it to key generation. Tests and fuzzer updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as S3 Output Plugin
participant AU as AWS Util (flb_get_s3_key)
participant TZ as TZ Parser (flb_aws_parse_tz_offset)
U->>U: Load config (time_offset, default "+0000")
U->>AU: flb_get_s3_key(format, time, tag, delim, seq, time_offset)
AU->>TZ: flb_aws_parse_tz_offset(time_offset)
TZ-->>AU: offset_seconds (0 if invalid)
AU->>AU: adjusted_time = time + offset_seconds
AU-->>U: formatted S3 key (using adjusted_time)
U->>U: Use key for upload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
The S3 key format is not time zone aware, which makes it hard to use when logs need to be stored in a different timezone than the S3 bucket's default. This patch adds support for a configurable time offset in the S3 key format. The offset is specified as a string like +0100 or -0100, following the same convention used in other parse configuration parameters. Signed-off-by: Fan Zhuang <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
plugins/out_s3/s3.h (1)
117-117: Trim trailing whitespaceMinor formatting nit: trailing spaces after profile.
- char *profile; + char *profile;tests/internal/fuzzers/aws_util_fuzzer.c (1)
70-77: Broaden fuzz coverage for time_offset pathsConsider exercising more paths (NULL, negative, and minutes offsets) to catch corner cases earlier.
Apply this diff to add a few extra calls without changing existing behavior:
@@ - s3_key_format = flb_get_s3_key(format, t, tag, tag_delimiter, 0, "+0000"); + s3_key_format = flb_get_s3_key(format, t, tag, tag_delimiter, 0, "+0000"); if (s3_key_format) { flb_sds_destroy(s3_key_format); } + /* Also exercise negative, and NULL offsets */ + s3_key_format = flb_get_s3_key(format, t, tag, tag_delimiter, 0, "-0030"); + if (s3_key_format) { + flb_sds_destroy(s3_key_format); + } + s3_key_format = flb_get_s3_key(format, t, tag, tag_delimiter, 0, NULL); + if (s3_key_format) { + flb_sds_destroy(s3_key_format); + }tests/internal/aws_util.c (2)
62-64: Remove stray placeholder commentThe standalone /* */ adds noise.
- - /* */ +
382-502: Great coverage of time_offset including edge casesThorough tests for positive/negative offsets, -0000/+0000/NULL equivalence, minutes, cross-year, leap-year, and invalid inputs. Two optional improvements:
- Add tests for extreme but valid offsets like "+1400" and "-1200".
- Add a minutes edge like "-1230" to cross day-month boundaries.
If you want, I can draft those additional assertions.
plugins/out_s3/s3.c (1)
4037-4043: Add init-time validation and tune help text for time_offsetThe config option looks good. Two suggestions:
- Validate the configured time_offset at init and warn once if it's invalid (fallback to UTC still works).
- Clarify description to reflect UTC and minutes support.
Apply this diff to improve the description:
- "Time offset to apply to the s3 key format. This is useful when the s3 key format is " - "in a different timezone than the S3 bucket. The format is +HHMM or -HHMM. " - "Default is +0000." + "Time offset applied to the timestamp used in s3_key_format. Use this to align key " + "naming with a non-UTC timezone (e.g., local time). Accepted formats: +HHMM or -HHMM " + "(minutes supported). Default: +0000 (UTC)."And add this init-time validation (outside this hunk) after flb_output_config_map_set():
/* Validate time_offset once at init; invalid values are ignored by the formatter */ if (ctx->time_offset != NULL) { int tz = flb_aws_parse_tz_offset(ctx->time_offset); if (tz == 0 && strcmp(ctx->time_offset, "+0000") != 0 && strcmp(ctx->time_offset, "-0000") != 0) { flb_plg_warn(ctx->ins, "Invalid time_offset '%s'; falling back to UTC", ctx->time_offset); } }I can wire this in at the exact spot you prefer in cb_s3_init if you want a patch.
src/aws/flb_aws_util.c (1)
1013-1015: Ensure external compatibility for the updated flb_get_s3_key APIAll internal headers and call sites have been updated to the new six-parameter signature:
- Prototype in
include/fluent-bit/flb_aws_util.h(lines 196–197)- Definition in
src/aws/flb_aws_util.c(lines 1013–1015)- Calls in
plugins/out_s3/s3.c(around 1462, 1650)- Tests in
tests/internal/aws_util.cand fuzzersHowever, this change breaks any out-of-tree consumers of
flb_get_s3_key. To maintain backward compatibility, consider:
- Introducing a new symbol
flb_get_s3_key_exwith the six-parameter signature- Retaining the original
flb_get_s3_keyas a thin wrapper that callsflb_get_s3_key_ex(..., "+0000")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
include/fluent-bit/flb_aws_util.h(2 hunks)plugins/out_s3/s3.c(3 hunks)plugins/out_s3/s3.h(2 hunks)src/aws/flb_aws_util.c(4 hunks)tests/internal/aws_util.c(14 hunks)tests/internal/fuzzers/aws_util_fuzzer.c(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
include/fluent-bit/flb_aws_util.h (1)
src/aws/flb_aws_util.c (1)
flb_aws_parse_tz_offset(1338-1370)
tests/internal/aws_util.c (4)
tests/internal/fuzzers/aws_util_fuzzer.c (1)
initialization_crutch(27-36)src/flb_config.c (1)
flb_config_init(212-416)src/aws/flb_aws_util.c (1)
flb_get_s3_key(1013-1242)src/flb_sds.c (1)
flb_sds_destroy(389-399)
tests/internal/fuzzers/aws_util_fuzzer.c (1)
src/aws/flb_aws_util.c (1)
flb_get_s3_key(1013-1242)
🔇 Additional comments (8)
plugins/out_s3/s3.h (1)
108-108: Add time_offset to S3 context: LGTMThe new field is appropriately placed and typed for passing through to key generation.
tests/internal/fuzzers/aws_util_fuzzer.c (1)
73-73: Fuzzer API update: LGTMPassing "+0000" keeps existing semantics and aligns with the new flb_get_s3_key signature.
tests/internal/aws_util.c (2)
55-55: TIME_OFFSET macro addition: LGTMKeeps tests concise and consistent with the new API.
180-181: Updated call sites to pass TIME_OFFSET: LGTMThe transitions to the new flb_get_s3_key signature look correct across these tests.
Also applies to: 195-196, 210-212, 227-228, 243-244, 258-259, 281-282, 296-298, 312-314, 328-330, 336-337, 354-355, 360-361, 376-377
plugins/out_s3/s3.c (2)
1462-1464: Pass-through of time_offset to key generation: LGTMCorrectly supplies ctx->time_offset to flb_get_s3_key for PutObject keys.
1650-1652: Multipart upload key generation now time-offset aware: LGTMConsistent with PutObject path; good.
include/fluent-bit/flb_aws_util.h (1)
196-198: No stale flb_get_s3_key calls remain
All existing call sites have been updated to pass six arguments (including the newtime_offset), and our regex search found no five-argument calls in the repository. Great work!src/aws/flb_aws_util.c (1)
30-30: Includes look appropriate for new usagestddef.h for size_t and ctype.h for isdigit are justified by the added code.
Also applies to: 35-35
|
|
||
| /* | ||
| * Parses the time offset and returns the offset in seconds. | ||
| */ | ||
| size_t flb_aws_parse_tz_offset(const char *time_offset); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: flb_aws_parse_tz_offset must return a signed type
Returning size_t breaks negative offsets (wrap-around to huge positive), potentially corrupting timestamps. Use a signed integer type (int is sufficient; range ±86399).
Apply this header change:
-/*
- * Parses the time offset and returns the offset in seconds.
- */
-size_t flb_aws_parse_tz_offset(const char *time_offset);
+/*
+ * Parses the time offset and returns the offset in seconds (signed).
+ * Returns 0 on invalid input.
+ */
+int flb_aws_parse_tz_offset(const char *time_offset);And align the implementation in src/aws/flb_aws_util.c:
-size_t flb_aws_parse_tz_offset(const char *time_offset) {
+int flb_aws_parse_tz_offset(const char *time_offset) {
int sign, hh, mm;
@@
- return sign * (hh * 3600 + mm * 60);
+ return sign * (hh * 3600 + mm * 60);
}Also ensure flb_get_s3_key uses a signed type when applying the offset to time_t:
/* Example inside flb_get_s3_key */
int tz_offset = flb_aws_parse_tz_offset(time_offset);
if (tz_offset != 0) {
base_time += tz_offset;
}🤖 Prompt for AI Agents
In include/fluent-bit/flb_aws_util.h around lines 211-216, change the prototype
return type of flb_aws_parse_tz_offset from size_t to a signed type (int) so
negative timezone offsets are represented correctly; update the implementation
in src/aws/flb_aws_util.c to return the same signed type and ensure all internal
math uses signed arithmetic; finally update callers such as flb_get_s3_key to
store the return in an int (e.g., int tz_offset =
flb_aws_parse_tz_offset(time_offset)) and apply that signed offset to time_t
(base_time += tz_offset) to avoid unsigned wraparound.
| size_t tz_offset = 0; | ||
|
|
||
| tz_offset = flb_aws_parse_tz_offset(time_offset); | ||
|
|
||
| if (tz_offset != 0) { | ||
| time += tz_offset; | ||
| } |
There was a problem hiding this comment.
Bug: negative offsets break due to unsigned type; use a signed type for tz offset
tz_offset is declared as size_t, but flb_aws_parse_tz_offset can return negative seconds for "-HHMM". Assigning a negative value to size_t causes wrap-around to a huge positive number, producing a wildly incorrect timestamp when added to time. Use a signed type (int or time_t) for both the function return and the local variable.
Apply this diff:
- size_t tz_offset = 0;
+ int tz_offset = 0;
@@
- tz_offset = flb_aws_parse_tz_offset(time_offset);
+ tz_offset = flb_aws_parse_tz_offset(time_offset);
@@
- if (tz_offset != 0) {
- time += tz_offset;
- }
+ if (tz_offset != 0) {
+ time += tz_offset;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/aws/flb_aws_util.c around lines 1032 to 1038, tz_offset is declared as
size_t but flb_aws_parse_tz_offset can return negative values; change the local
tz_offset to a signed type (e.g., long or time_t/int) and update
flb_aws_parse_tz_offset's return type to the same signed type so negative
offsets are preserved; after changing types, adjust any callers/assignments and
ensure arithmetic on time uses the signed tz_offset to correctly add/subtract
negative offsets and recompile to satisfy type changes.
| size_t flb_aws_parse_tz_offset(const char *time_offset) { | ||
| int sign, hh, mm; | ||
|
|
||
| if (!time_offset) { | ||
| return 0; | ||
| } | ||
|
|
||
| if (strlen(time_offset) < 5) { | ||
| return 0; | ||
| } | ||
|
|
||
| if (time_offset[0] != '+' && time_offset[0] != '-') { | ||
| return 0; | ||
| } | ||
|
|
||
| sign = (time_offset[0] == '+') ? 1 : -1; | ||
|
|
||
| if (!isdigit(time_offset[1]) || | ||
| !isdigit(time_offset[2]) || | ||
| !isdigit(time_offset[3]) || | ||
| !isdigit(time_offset[4])) { | ||
| return 0; | ||
| } | ||
|
|
||
| hh = ((time_offset[1] - '0') * 10) + (time_offset[2] - '0'); | ||
| mm = ((time_offset[3] - '0') * 10) + (time_offset[4] - '0'); | ||
|
|
||
| if (hh < 0 || hh > 23 || mm < 0 || mm > 59) { | ||
| return 0; | ||
| } | ||
|
|
||
| return sign * (hh * 3600 + mm * 60); | ||
| } |
There was a problem hiding this comment.
Incorrect return type for offset parser; tighten validation and avoid ctype UB
flb_aws_parse_tz_offset returns size_t but computes signed offsets. This will misrepresent negative offsets. Also, accept exactly 5 characters (±HHMM) and cast to unsigned char for isdigit to avoid undefined behavior.
Apply this diff:
-size_t flb_aws_parse_tz_offset(const char *time_offset) {
+int flb_aws_parse_tz_offset(const char *time_offset) {
int sign, hh, mm;
if (!time_offset) {
return 0;
}
- if (strlen(time_offset) < 5) {
+ /* Expect exactly 5 characters: ±HHMM */
+ if (strlen(time_offset) != 5) {
return 0;
}
if (time_offset[0] != '+' && time_offset[0] != '-') {
return 0;
}
sign = (time_offset[0] == '+') ? 1 : -1;
- if (!isdigit(time_offset[1]) ||
- !isdigit(time_offset[2]) ||
- !isdigit(time_offset[3]) ||
- !isdigit(time_offset[4])) {
+ if (!isdigit((unsigned char) time_offset[1]) ||
+ !isdigit((unsigned char) time_offset[2]) ||
+ !isdigit((unsigned char) time_offset[3]) ||
+ !isdigit((unsigned char) time_offset[4])) {
return 0;
}
hh = ((time_offset[1] - '0') * 10) + (time_offset[2] - '0');
mm = ((time_offset[3] - '0') * 10) + (time_offset[4] - '0');
if (hh < 0 || hh > 23 || mm < 0 || mm > 59) {
return 0;
}
return sign * (hh * 3600 + mm * 60);
}Optional: consider logging a warning when an invalid string is provided so users know their configured offset was ignored.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size_t flb_aws_parse_tz_offset(const char *time_offset) { | |
| int sign, hh, mm; | |
| if (!time_offset) { | |
| return 0; | |
| } | |
| if (strlen(time_offset) < 5) { | |
| return 0; | |
| } | |
| if (time_offset[0] != '+' && time_offset[0] != '-') { | |
| return 0; | |
| } | |
| sign = (time_offset[0] == '+') ? 1 : -1; | |
| if (!isdigit(time_offset[1]) || | |
| !isdigit(time_offset[2]) || | |
| !isdigit(time_offset[3]) || | |
| !isdigit(time_offset[4])) { | |
| return 0; | |
| } | |
| hh = ((time_offset[1] - '0') * 10) + (time_offset[2] - '0'); | |
| mm = ((time_offset[3] - '0') * 10) + (time_offset[4] - '0'); | |
| if (hh < 0 || hh > 23 || mm < 0 || mm > 59) { | |
| return 0; | |
| } | |
| return sign * (hh * 3600 + mm * 60); | |
| } | |
| int flb_aws_parse_tz_offset(const char *time_offset) { | |
| int sign, hh, mm; | |
| if (!time_offset) { | |
| return 0; | |
| } | |
| /* Expect exactly 5 characters: ±HHMM */ | |
| if (strlen(time_offset) != 5) { | |
| return 0; | |
| } | |
| if (time_offset[0] != '+' && time_offset[0] != '-') { | |
| return 0; | |
| } | |
| sign = (time_offset[0] == '+') ? 1 : -1; | |
| if (!isdigit((unsigned char)time_offset[1]) || | |
| !isdigit((unsigned char)time_offset[2]) || | |
| !isdigit((unsigned char)time_offset[3]) || | |
| !isdigit((unsigned char)time_offset[4])) { | |
| return 0; | |
| } | |
| hh = ((time_offset[1] - '0') * 10) + (time_offset[2] - '0'); | |
| mm = ((time_offset[3] - '0') * 10) + (time_offset[4] - '0'); | |
| if (hh < 0 || hh > 23 || mm < 0 || mm > 59) { | |
| return 0; | |
| } | |
| return sign * (hh * 3600 + mm * 60); | |
| } |
🤖 Prompt for AI Agents
In src/aws/flb_aws_util.c around lines 1338 to 1370, flb_aws_parse_tz_offset
currently returns size_t and performs isdigit calls on plain char and allows
strings shorter than 5; change the function to return a signed integer type
(e.g. int or long) so negative offsets are represented correctly, require the
input length to be exactly 5 characters (format ±HHMM), cast characters to
(unsigned char) before calling isdigit to avoid ctype UB, and keep the same
numeric validation for HH (0-23) and MM (0-59) while returning sign * (hh*3600 +
mm*60); optionally add a warning log when an invalid string is provided.
Summary
The S3 key format is not time zone aware, which makes it difficult to use when logs need to be stored with a different timezone alignment than UTC (the S3 bucket default).
This patch adds support for a configurable time offset in the S3 key format.
The offset is specified as a string like
+0100or-0100, following the same convention used in other parse configuration parameters.Changes
Add optional time_offset parameter for the S3 output plugin.
Parse offset as
±HHMMand apply it to the timestamp used in s3_key_format.Update S3 key generation utility (flb_get_s3_key) to support offsets.
Added unit tests for both positive and negative offsets.
Backward-compatible: if time_offset is not configured, behavior remains unchanged (UTC).
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