in_calyptia_fleet: Use symlinks to manage fleet config files #10755
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe patch modifies in_calyptia_fleet.c to fix timestamp filename generation, harden symlink resolution, rework deletion via a generic symlink-aware API, adjust reload/error handling and resource management, remove “load newest” fallback, update timestamp/log formatting, and add recursive deletion with refined logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Timer as Collector/Timer
participant Fleet as in_calyptia_fleet
participant FB as Fluent Bit Core
participant FS as Filesystem
rect rgba(217,237,247,0.5)
note right of Fleet: Reload flow (updated)
Timer->>Fleet: Trigger reload attempt
Fleet->>FS: Resolve cfg symlink (readlink, parse timestamp)
alt New config exists
Fleet->>FB: Create reload context
alt Context alloc OK
FB-->>Fleet: Context ready
Fleet->>FB: Apply new config
alt Apply fails
Fleet->>Fleet: Free reload context, resume collectors
Fleet->>FS: Cleanup allocated paths
Fleet-->>Timer: Return gracefully
else Apply succeeds
Fleet->>FS: Commit new config (delete old first)
Fleet-->>Timer: Reload complete
end
else Context alloc fails
Fleet->>Fleet: Resume collectors, free cfgpath/reload
Fleet-->>Timer: Return
end
else No new config
Fleet-->>Timer: No-op (no “load newest” fallback)
end
end
sequenceDiagram
autonumber
participant Fleet as in_calyptia_fleet
participant FS as Filesystem
participant Glob as Glob Matcher
rect rgba(223,240,216,0.5)
note right of Fleet: Symlink-aware deletion (new)
Fleet->>FS: readlink(symlink) -> real target
Fleet->>Fleet: Derive glob prefix (replace ext with '*')
Fleet->>Glob: Match files/dirs by prefix
loop For each match
alt Directory
Fleet->>FS: delete_dir(path) (recursive)
else File
Fleet->>FS: unlink(path)
end
end
Fleet->>FS: unlink(symlink)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
|
Process output exercising the changed code path. Issues reported by valgrind existed prior to this PR. |
Signed-off-by: Alec Holmes <[email protected]>
alecholmes
force-pushed
the
alec/2025-08-19-use-symlinks-to-manage-fleet-config
branch
from
33b7572 to
2fbb73c
Compare
alecholmes
commented
Aug 19, 2025
| char s_last_modified[32]; | ||
|
|
||
| snprintf(s_last_modified, sizeof(s_last_modified)-1, "%d", (int)t); | ||
| snprintf(s_last_modified, sizeof(s_last_modified), "%d", (int)t); |
There was a problem hiding this comment.
snprintf will properly null-terminate.
alecholmes
commented
Aug 19, 2025
| static int test_config_is_valid(struct flb_in_calyptia_fleet_config *ctx, | ||
| flb_sds_t cfgpath) | ||
| { | ||
| return FLB_TRUE; |
There was a problem hiding this comment.
test_config_is_valid is a no-op in practice and according to @pwhelan we'll never be ably to fully validate up front. Removing this function.
alecholmes
commented
Aug 19, 2025
| } | ||
|
|
||
| static int calyptia_config_delete_old_dir(const char *cfgpath) | ||
| /** |
There was a problem hiding this comment.
I deleted a couple of funcs and replaced them new ones. The diff doesn't display well since the old and new code is vaguely similar.
alecholmes
commented
Aug 19, 2025
| else if (exists_new_fleet_config(ctx) == FLB_TRUE) { | ||
| return execute_reload(ctx, new_fleet_config_filename(ctx)); | ||
| } | ||
| else { |
There was a problem hiding this comment.
The branches above will load either current or new config. No need to pick otherwise random config and a subsequent PR will refactor this function a bit anyway.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (3)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (3)
602-606: Destroy pthread attributes after useYou initialize a detached attribute, but never destroy it. Add pthread_attr_destroy to avoid leaking resources on some libpthread implementations.
Apply this diff:
pthread_attr_init(&ptha); pthread_attr_setdetachstate(&ptha, PTHREAD_CREATE_DETACHED); pthread_create(&pth, &ptha, do_reload, reload); + pthread_attr_destroy(&ptha);
1301-1316: Index type nit: prefer size_t for array iterationfiles->entry_count is a size_t. Using int and casting each iteration is unnecessary and could overflow in extreme cases.
Apply this diff:
- int idx; + size_t idx; ... - for (idx = 0; idx < ((ssize_t)files->entry_count); idx++) { + for (idx = 0; idx < files->entry_count; idx++) {
1501-1503: Avoid redundant unlink after calyptia_config_delete_old()c alyptia_config_delete_old() already unlinks the old symlink. The extra unlink likely hits ENOENT and is redundant.
Apply this diff:
- if (exists_old_fleet_config(ctx) == FLB_TRUE) { - calyptia_config_delete_old(ctx); - unlink(cfgoldname); - } + if (exists_old_fleet_config(ctx) == FLB_TRUE) { + (void) calyptia_config_delete_old(ctx); + }
📜 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 (1)
plugins/in_calyptia_fleet/in_calyptia_fleet.c(7 hunks)
⏰ 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). (23)
- 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: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 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_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- 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 (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_JEMALLOC=Off, 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_JEMALLOC=Off, 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_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (3)
plugins/in_calyptia_fleet/in_calyptia_fleet.c (3)
506-511: Good hardening of readlink usage and NUL-terminationBounds checking and explicit termination eliminate common pitfalls here. Looks solid.
569-572: Nice: added allocation failure handling and cleanup on error pathFreeing cfgpath and reload on failure avoids leaks and inconsistent state. Good catch.
Also applies to: 586-586
987-988: Correct 64-bit logging for timestampsSwitching to %lld prevents truncation when printing 64-bit time values. Good.
Comment on lines
235
to
236
| snprintf(s_last_modified, sizeof(s_last_modified), "%d", (int)t); | ||
| return fleet_config_filename(ctx, s_last_modified); |
There was a problem hiding this comment.
Fix narrowing conversion of time_t to int when formatting timestamp
On many platforms time_t is 64-bit. Casting to int truncates and can generate wrong filenames, breaking symlink targets and cleanup. Format as 64-bit.
Apply this diff:
- snprintf(s_last_modified, sizeof(s_last_modified), "%d", (int)t);
+ snprintf(s_last_modified, sizeof(s_last_modified), "%lld", (long long) t);📝 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.
Suggested change
| snprintf(s_last_modified, sizeof(s_last_modified), "%d", (int)t); | |
| return fleet_config_filename(ctx, s_last_modified); | |
| snprintf(s_last_modified, sizeof(s_last_modified), "%lld", (long long) t); | |
| return fleet_config_filename(ctx, s_last_modified); |
🤖 Prompt for AI Agents
In plugins/in_calyptia_fleet/in_calyptia_fleet.c around lines 235-236, the code
narrows time_t to int when building the timestamp string; replace the
cast/format with a 64-bit-safe approach: include <inttypes.h> (if not already
included), convert time_t to int64_t (e.g. int64_t ts = (int64_t)t;) and use
snprintf with a PRI macro (snprintf(s_last_modified, sizeof(s_last_modified),
"%" PRId64, ts);) so timestamps aren’t truncated on 64-bit time_t platforms.
Comment on lines
1331
to
1402
| static int calyptia_config_delete_by_symlink(struct flb_in_calyptia_fleet_config *ctx, | ||
| flb_sds_t (*config_filename_func)(struct flb_in_calyptia_fleet_config *)) | ||
| { | ||
| struct cfl_array *confs; | ||
| struct cfl_array *tconfs; | ||
| flb_sds_t glob_files = NULL; | ||
| flb_sds_t config_path; | ||
| char realname[CALYPTIA_MAX_DIR_SIZE] = {0}; | ||
| ssize_t len; | ||
| char *ext; | ||
| int idx; | ||
| struct stat entry_stat; | ||
| const char *entry_path; | ||
|
|
||
| if (ctx == NULL) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (generate_base_fleet_directory(ctx, &glob_files) == NULL) { | ||
| flb_sds_destroy(glob_files); | ||
| return -1; | ||
| return FLB_FALSE; | ||
| } | ||
|
|
||
| if (ctx->fleet_config_legacy_format) { | ||
| if (flb_sds_cat_safe(&glob_files, PATH_SEPARATOR "*.conf", strlen(PATH_SEPARATOR "*.conf")) != 0) { | ||
| flb_sds_destroy(glob_files); | ||
| return -1; | ||
| } | ||
| } else if (flb_sds_cat_safe(&glob_files, PATH_SEPARATOR "*.yaml", strlen(PATH_SEPARATOR "*.yaml")) != 0) { | ||
| flb_sds_destroy(glob_files); | ||
| return -1; | ||
| /* Get the symlink path and extract prefix from its target */ | ||
| config_path = config_filename_func(ctx); | ||
| if (config_path == NULL) { | ||
| flb_plg_error(ctx->ins, "error getting config path"); | ||
| return FLB_FALSE; | ||
| } | ||
|
|
||
| confs = read_glob(glob_files); | ||
| if (confs == NULL) { | ||
| flb_sds_destroy(glob_files); | ||
| return -1; | ||
| /* Follow the symlink to get the target filename */ | ||
| len = readlink(config_path, realname, sizeof(realname) - 1); | ||
| if (len < 0 || len >= (sizeof(realname) - 1)) { | ||
| flb_plg_error(ctx->ins, "error resolving symlink: %s", config_path); | ||
| flb_sds_destroy(config_path); | ||
| return FLB_FALSE; | ||
| } | ||
| realname[len] = '\0'; | ||
|
|
||
| tconfs = cfl_array_create(1); | ||
| if (tconfs == NULL) { | ||
| flb_sds_destroy(glob_files); | ||
| cfl_array_destroy(confs); | ||
| return -1; | ||
| /* Replace the extension with a glob (e.g. "/a/b.yaml" -> "/a/b*") */ | ||
| ext = strrchr(realname, '.'); | ||
| if (ext == NULL) { | ||
| flb_plg_error(ctx->ins, "symlink target has no extension: %s", realname); | ||
| flb_sds_destroy(config_path); | ||
| return FLB_FALSE; | ||
| } | ||
| strcpy(ext, "*"); | ||
|
|
||
| if (cfl_array_resizable(tconfs, FLB_TRUE) != 0) { | ||
| flb_sds_destroy(glob_files); | ||
| cfl_array_destroy(confs); | ||
| return -1; | ||
| /* Delete all files and directories that match the prefix pattern */ | ||
| confs = read_glob(realname); | ||
| if (confs == NULL) { | ||
| flb_plg_warn(ctx->ins, "config glob did not return any files: %s", realname); | ||
| flb_sds_destroy(config_path); | ||
| return FLB_FALSE; | ||
| } | ||
|
|
||
| for (idx = 0; idx < confs->entry_count; idx++) { | ||
| if (fleet_config_path_timestamp(ctx, confs->entries[idx]->data.as_string) > 0) { | ||
| cfl_array_append_string(tconfs, confs->entries[idx]->data.as_string); | ||
| entry_path = confs->entries[idx]->data.as_string; | ||
| if (stat(entry_path, &entry_stat) == 0) { | ||
| if (S_ISDIR(entry_stat.st_mode)) { | ||
| flb_plg_info(ctx->ins, "deleting config directory: %s", entry_path); | ||
| if (delete_dir(entry_path) == FLB_FALSE) { | ||
| flb_plg_warn(ctx->ins, "unable to delete config directory: %s", entry_path); | ||
| } | ||
| } else { | ||
| flb_plg_info(ctx->ins, "deleting config file: %s", entry_path); | ||
| if (unlink(entry_path) != 0) { | ||
| flb_plg_warn(ctx->ins, "unable to delete config file: %s", entry_path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| qsort(tconfs->entries, tconfs->entry_count, | ||
| sizeof(struct cfl_variant *), | ||
| cfl_array_qsort_conf_files); | ||
|
|
||
| for (idx = 0; idx < (((ssize_t)tconfs->entry_count) -3); idx++) { | ||
| unlink(tconfs->entries[idx]->data.as_string); | ||
| calyptia_config_delete_old_dir(tconfs->entries[idx]->data.as_string); | ||
| flb_plg_info(ctx->ins, "deleting config symlink: %s", config_path); | ||
| if (unlink(config_path) != 0) { | ||
| flb_plg_warn(ctx->ins, "unable to delete config symlink: %s", config_path); | ||
| } | ||
|
|
||
| cfl_array_destroy(confs); | ||
| cfl_array_destroy(tconfs); | ||
| flb_sds_destroy(glob_files); | ||
|
|
||
| return 0; | ||
| flb_sds_destroy(config_path); | ||
| return FLB_TRUE; | ||
| } |
There was a problem hiding this comment.
Windows build will fail: readlink is used without platform guards
This function unconditionally calls readlink, but on Windows readlink is unavailable. Additionally, Windows code paths in this file still call calyptia_config_delete_old(), which wraps this function, so it must compile on Windows.
Wrap the POSIX implementation in a compile guard, and provide a minimal Windows fallback that removes the symlink path best-effort:
- static int calyptia_config_delete_by_symlink(struct flb_in_calyptia_fleet_config *ctx,
- flb_sds_t (*config_filename_func)(struct flb_in_calyptia_fleet_config *))
-{
+ #ifndef FLB_SYSTEM_WINDOWS
+ static int calyptia_config_delete_by_symlink(struct flb_in_calyptia_fleet_config *ctx,
+ flb_sds_t (*config_filename_func)(struct flb_in_calyptia_fleet_config *))
+{
@@
- len = readlink(config_path, realname, sizeof(realname) - 1);
+ len = readlink(config_path, realname, sizeof(realname) - 1);
@@
- cfl_array_destroy(confs);
- flb_sds_destroy(config_path);
- return FLB_TRUE;
-}
+ cfl_array_destroy(confs);
+ flb_sds_destroy(config_path);
+ return FLB_TRUE;
+}
+ #else
+ static int calyptia_config_delete_by_symlink(struct flb_in_calyptia_fleet_config *ctx,
+ flb_sds_t (*config_filename_func)(struct flb_in_calyptia_fleet_config *))
+{
+ flb_sds_t config_path;
+ if (ctx == NULL) {
+ return FLB_FALSE;
+ }
+ config_path = config_filename_func(ctx);
+ if (config_path == NULL) {
+ flb_plg_error(ctx->ins, "error getting config path");
+ return FLB_FALSE;
+ }
+ /* No symlink support on Windows; best-effort unlink of the path */
+ flb_plg_info(ctx->ins, "deleting config path: %s", config_path);
+ if (unlink(config_path) != 0) {
+ flb_plg_warn(ctx->ins, "unable to delete config path: %s", config_path);
+ }
+ flb_sds_destroy(config_path);
+ return FLB_TRUE;
+}
+ #endif📝 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.
Suggested change
| static int calyptia_config_delete_by_symlink(struct flb_in_calyptia_fleet_config *ctx, | |
| flb_sds_t (*config_filename_func)(struct flb_in_calyptia_fleet_config *)) | |
| { | |
| struct cfl_array *confs; | |
| struct cfl_array *tconfs; | |
| flb_sds_t glob_files = NULL; | |
| flb_sds_t config_path; | |
| char realname[CALYPTIA_MAX_DIR_SIZE] = {0}; | |
| ssize_t len; | |
| char *ext; | |
| int idx; | |
| struct stat entry_stat; | |
| const char *entry_path; | |
| if (ctx == NULL) { | |
| return -1; | |
| } | |
| if (generate_base_fleet_directory(ctx, &glob_files) == NULL) { | |
| flb_sds_destroy(glob_files); | |
| return -1; | |
| return FLB_FALSE; | |
| } | |
| if (ctx->fleet_config_legacy_format) { | |
| if (flb_sds_cat_safe(&glob_files, PATH_SEPARATOR "*.conf", strlen(PATH_SEPARATOR "*.conf")) != 0) { | |
| flb_sds_destroy(glob_files); | |
| return -1; | |
| } | |
| } else if (flb_sds_cat_safe(&glob_files, PATH_SEPARATOR "*.yaml", strlen(PATH_SEPARATOR "*.yaml")) != 0) { | |
| flb_sds_destroy(glob_files); | |
| return -1; | |
| /* Get the symlink path and extract prefix from its target */ | |
| config_path = config_filename_func(ctx); | |
| if (config_path == NULL) { | |
| flb_plg_error(ctx->ins, "error getting config path"); | |
| return FLB_FALSE; | |
| } | |
| confs = read_glob(glob_files); | |
| if (confs == NULL) { | |
| flb_sds_destroy(glob_files); | |
| return -1; | |
| /* Follow the symlink to get the target filename */ | |
| len = readlink(config_path, realname, sizeof(realname) - 1); | |
| if (len < 0 || len >= (sizeof(realname) - 1)) { | |
| flb_plg_error(ctx->ins, "error resolving symlink: %s", config_path); | |
| flb_sds_destroy(config_path); | |
| return FLB_FALSE; | |
| } | |
| realname[len] = '\0'; | |
| tconfs = cfl_array_create(1); | |
| if (tconfs == NULL) { | |
| flb_sds_destroy(glob_files); | |
| cfl_array_destroy(confs); | |
| return -1; | |
| /* Replace the extension with a glob (e.g. "/a/b.yaml" -> "/a/b*") */ | |
| ext = strrchr(realname, '.'); | |
| if (ext == NULL) { | |
| flb_plg_error(ctx->ins, "symlink target has no extension: %s", realname); | |
| flb_sds_destroy(config_path); | |
| return FLB_FALSE; | |
| } | |
| strcpy(ext, "*"); | |
| if (cfl_array_resizable(tconfs, FLB_TRUE) != 0) { | |
| flb_sds_destroy(glob_files); | |
| cfl_array_destroy(confs); | |
| return -1; | |
| /* Delete all files and directories that match the prefix pattern */ | |
| confs = read_glob(realname); | |
| if (confs == NULL) { | |
| flb_plg_warn(ctx->ins, "config glob did not return any files: %s", realname); | |
| flb_sds_destroy(config_path); | |
| return FLB_FALSE; | |
| } | |
| for (idx = 0; idx < confs->entry_count; idx++) { | |
| if (fleet_config_path_timestamp(ctx, confs->entries[idx]->data.as_string) > 0) { | |
| cfl_array_append_string(tconfs, confs->entries[idx]->data.as_string); | |
| entry_path = confs->entries[idx]->data.as_string; | |
| if (stat(entry_path, &entry_stat) == 0) { | |
| if (S_ISDIR(entry_stat.st_mode)) { | |
| flb_plg_info(ctx->ins, "deleting config directory: %s", entry_path); | |
| if (delete_dir(entry_path) == FLB_FALSE) { | |
| flb_plg_warn(ctx->ins, "unable to delete config directory: %s", entry_path); | |
| } | |
| } else { | |
| flb_plg_info(ctx->ins, "deleting config file: %s", entry_path); | |
| if (unlink(entry_path) != 0) { | |
| flb_plg_warn(ctx->ins, "unable to delete config file: %s", entry_path); | |
| } | |
| } | |
| } | |
| } | |
| qsort(tconfs->entries, tconfs->entry_count, | |
| sizeof(struct cfl_variant *), | |
| cfl_array_qsort_conf_files); | |
| for (idx = 0; idx < (((ssize_t)tconfs->entry_count) -3); idx++) { | |
| unlink(tconfs->entries[idx]->data.as_string); | |
| calyptia_config_delete_old_dir(tconfs->entries[idx]->data.as_string); | |
| flb_plg_info(ctx->ins, "deleting config symlink: %s", config_path); | |
| if (unlink(config_path) != 0) { | |
| flb_plg_warn(ctx->ins, "unable to delete config symlink: %s", config_path); | |
| } | |
| cfl_array_destroy(confs); | |
| cfl_array_destroy(tconfs); | |
| flb_sds_destroy(glob_files); | |
| return 0; | |
| flb_sds_destroy(config_path); | |
| return FLB_TRUE; | |
| } | |
| #ifndef FLB_SYSTEM_WINDOWS | |
| static int calyptia_config_delete_by_symlink(struct flb_in_calyptia_fleet_config *ctx, | |
| flb_sds_t (*config_filename_func)(struct flb_in_calyptia_fleet_config *)) | |
| { | |
| struct cfl_array *confs; | |
| flb_sds_t config_path; | |
| char realname[CALYPTIA_MAX_DIR_SIZE] = {0}; | |
| ssize_t len; | |
| char *ext; | |
| int idx; | |
| struct stat entry_stat; | |
| const char *entry_path; | |
| if (ctx == NULL) { | |
| return FLB_FALSE; | |
| } | |
| /* Get the symlink path and extract prefix from its target */ | |
| config_path = config_filename_func(ctx); | |
| if (config_path == NULL) { | |
| flb_plg_error(ctx->ins, "error getting config path"); | |
| return FLB_FALSE; | |
| } | |
| /* Follow the symlink to get the target filename */ | |
| len = readlink(config_path, realname, sizeof(realname) - 1); | |
| if (len < 0 || len >= (sizeof(realname) - 1)) { | |
| flb_plg_error(ctx->ins, "error resolving symlink: %s", config_path); | |
| flb_sds_destroy(config_path); | |
| return FLB_FALSE; | |
| } | |
| realname[len] = '\0'; | |
| /* Replace the extension with a glob (e.g. "/a/b.yaml" -> "/a/b*") */ | |
| ext = strrchr(realname, '.'); | |
| if (ext == NULL) { | |
| flb_plg_error(ctx->ins, "symlink target has no extension: %s", realname); | |
| flb_sds_destroy(config_path); | |
| return FLB_FALSE; | |
| } | |
| strcpy(ext, "*"); | |
| /* Delete all files and directories that match the prefix pattern */ | |
| confs = read_glob(realname); | |
| if (confs == NULL) { | |
| flb_plg_warn(ctx->ins, "config glob did not return any files: %s", realname); | |
| flb_sds_destroy(config_path); | |
| return FLB_FALSE; | |
| } | |
| for (idx = 0; idx < confs->entry_count; idx++) { | |
| entry_path = confs->entries[idx]->data.as_string; | |
| if (stat(entry_path, &entry_stat) == 0) { | |
| if (S_ISDIR(entry_stat.st_mode)) { | |
| flb_plg_info(ctx->ins, "deleting config directory: %s", entry_path); | |
| if (delete_dir(entry_path) == FLB_FALSE) { | |
| flb_plg_warn(ctx->ins, "unable to delete config directory: %s", entry_path); | |
| } | |
| } | |
| else { | |
| flb_plg_info(ctx->ins, "deleting config file: %s", entry_path); | |
| if (unlink(entry_path) != 0) { | |
| flb_plg_warn(ctx->ins, "unable to delete config file: %s", entry_path); | |
| } | |
| } | |
| } | |
| } | |
| flb_plg_info(ctx->ins, "deleting config symlink: %s", config_path); | |
| if (unlink(config_path) != 0) { | |
| flb_plg_warn(ctx->ins, "unable to delete config symlink: %s", config_path); | |
| } | |
| cfl_array_destroy(confs); | |
| flb_sds_destroy(config_path); | |
| return FLB_TRUE; | |
| } | |
| #else | |
| static int calyptia_config_delete_by_symlink(struct flb_in_calyptia_fleet_config *ctx, | |
| flb_sds_t (*config_filename_func)(struct flb_in_calyptia_fleet_config *)) | |
| { | |
| flb_sds_t config_path; | |
| if (ctx == NULL) { | |
| return FLB_FALSE; | |
| } | |
| config_path = config_filename_func(ctx); | |
| if (config_path == NULL) { | |
| flb_plg_error(ctx->ins, "error getting config path"); | |
| return FLB_FALSE; | |
| } | |
| /* No symlink support on Windows; best-effort unlink of the path */ | |
| flb_plg_info(ctx->ins, "deleting config path: %s", config_path); | |
| if (unlink(config_path) != 0) { | |
| flb_plg_warn(ctx->ins, "unable to delete config path: %s", config_path); | |
| } | |
| flb_sds_destroy(config_path); | |
| return FLB_TRUE; | |
| } | |
| #endif |
Comment on lines
1372
to
1376
| if (confs == NULL) { | ||
| flb_plg_warn(ctx->ins, "config glob did not return any files: %s", realname); | ||
| flb_sds_destroy(config_path); | ||
| return FLB_FALSE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not bail out early on glob miss; still unlink the symlink
Per the PR objective, when the glob finds nothing you should still delete the symlink. Currently you return early and skip unlinking.
Apply this diff:
- confs = read_glob(realname);
- if (confs == NULL) {
- flb_plg_warn(ctx->ins, "config glob did not return any files: %s", realname);
- flb_sds_destroy(config_path);
- return FLB_FALSE;
- }
- for (idx = 0; idx < confs->entry_count; idx++) {
+ confs = read_glob(realname);
+ if (confs == NULL) {
+ flb_plg_warn(ctx->ins, "config glob did not return any files: %s", realname);
+ }
+ else {
+ for (idx = 0; idx < confs->entry_count; idx++) {
entry_path = confs->entries[idx]->data.as_string;
if (stat(entry_path, &entry_stat) == 0) {
if (S_ISDIR(entry_stat.st_mode)) {
flb_plg_info(ctx->ins, "deleting config directory: %s", entry_path);
if (delete_dir(entry_path) == FLB_FALSE) {
flb_plg_warn(ctx->ins, "unable to delete config directory: %s", entry_path);
}
} else {
flb_plg_info(ctx->ins, "deleting config file: %s", entry_path);
if (unlink(entry_path) != 0) {
flb_plg_warn(ctx->ins, "unable to delete config file: %s", entry_path);
}
}
}
- }
+ }
+ cfl_array_destroy(confs);
+ }
@@
- cfl_array_destroy(confs);
flb_sds_destroy(config_path);
return FLB_TRUE;Also applies to: 1394-1397, 1399-1401
🤖 Prompt for AI Agents
In plugins/in_calyptia_fleet/in_calyptia_fleet.c around lines 1372-1376 (and
similarly at 1394-1397 and 1399-1401): the code returns early when flb_glob
returns NULL which skips unlinking the symlink; change the control flow so that
after logging the warning and destroying config_path you still perform the
unlink of realname (and any needed cleanup) instead of returning from the
function, and then continue without dereferencing confs; apply the same fix to
the other two locations.
2f2c1be to
d772de3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a prefactor that changes how locally cached fleet config files are deleted. A forthcoming PR will rely on this change to fix how configs are managed when reloading a new config fails.
For background, fleet config directories typically look like this when a new config is being reloaded:
When deleting the old config, the old logic would sort the directory contents and delete everything except the last N entries.
This PR changes deletion to be explicit. When deleting
old.conf, the symlink is resolved and the related files and directories are deletes. In the example above,old.confresolves to1755653410.conf. This new code will delete all files/dirs that match the glob1755653410*(1755653410.confand the1755653410directory).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
Bug Fixes
Refactor