fix: Filter incompatible logging driver options in ContainerLogConfig #160
Conversation
📝 WalkthroughWalkthroughTwo configuration functions were enhanced: WriteToDisk now supports an environment variable to skip disk writes, and ContainerLogConfig now filters logging options based on the configured driver type. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@config/config_docker.go`:
- Around line 106-118: The current loop over c.LogConfig.Config incorrectly
strips local-only keys for all non-"local" drivers, which removes valid
json-file options like "max-size" and "max-file"; update the filtering logic in
the block that builds the config map so it checks c.LogConfig.Type and only
excludes keys that are incompatible with that specific driver (e.g., if
c.LogConfig.Type == "local" allow local keys; if c.LogConfig.Type == "json-file"
preserve "max-size" and "max-file"; for other drivers exclude only their known
incompatible keys such as "mode" or "compress" when appropriate), using the
existing symbols c.LogConfig.Config, c.LogConfig.Type and the local config map
to perform driver-specific whitelist/blacklist checks.
🧹 Nitpick comments (1)
config/config.go (1)
470-477: Optional: log when config writes are skipped.
Silently returning makes it hard to diagnose why config never persists; a low-noise info/debug log would help operators.♻️ Proposed tweak (optional)
if os.Getenv("WINGS_NO_CONFIG_WRITE") == "1" { - // Note: use your logger package as appropriate; using apex/log.Logger in this file - // may require importing it. For clarity we keep this a simple no-op. + log.WithField("env", "WINGS_NO_CONFIG_WRITE").Info("skipping config write to disk") return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.goconfig/config_docker.go
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Filter config options based on the logging driver type | ||
| // The "local" driver has specific options that aren't compatible with other drivers | ||
| config := make(map[string]string) | ||
| for k, v := range c.LogConfig.Config { | ||
| // Only include these options if using the "local" driver | ||
| if c.LogConfig.Type != "local" { | ||
| // Exclude local-specific options for non-local drivers like fluentd, json-file, etc. | ||
| if k == "max-size" || k == "max-file" || k == "compress" || k == "mode" { | ||
| continue | ||
| } | ||
| } | ||
| config[k] = v | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n config/config_docker.go | sed -n '100,125p'Repository: pelican-dev/wings
Length of output: 983
🌐 Web query:
Docker json-file logging driver supported options max-size max-file
💡 Result:
-
max-size: maximum size of a container log file before rotation. Specify a positive integer plus unit k, m, or g (e.g., 10m). Defaults to -1 (unlimited). Use as --log-opt max-size=10m (or in daemon.json as "max-size": "10m"). [1]
-
max-file: maximum number of rotated log files to keep. Positive integer; only honored when max-size is set. Defaults to 1. Use as --log-opt max-file=3 (or in daemon.json as "max-file": "3"). [1]
Example (daemon.json):
{
"log-driver": "json-file",
"log-opts": {
"max-size": "10m",
"max-file": "3"
}
}
Notes: daemon.json log-opts values must be strings; restart Docker for new defaults to apply to newly created containers. [1][2]
Sources:
[1] Docker docs — JSON File logging driver.
[2] Docker docs — Configure logging drivers.
Fix json-file driver log rotation by excluding only incompatible options.
The current filter removes max-size and max-file from all non-local drivers, but these are valid options for the json-file driver and necessary for log rotation. Filtering them out disables rotation and allows logs to grow unbounded. Instead, identify which options are truly incompatible with each specific driver and exclude only those—e.g., max-size, max-file, compress, and mode are local-specific; preserve them for json-file and filter based on actual driver compatibility.
🤖 Prompt for AI Agents
In `@config/config_docker.go` around lines 106 - 118, The current loop over
c.LogConfig.Config incorrectly strips local-only keys for all non-"local"
drivers, which removes valid json-file options like "max-size" and "max-file";
update the filtering logic in the block that builds the config map so it checks
c.LogConfig.Type and only excludes keys that are incompatible with that specific
driver (e.g., if c.LogConfig.Type == "local" allow local keys; if
c.LogConfig.Type == "json-file" preserve "max-size" and "max-file"; for other
drivers exclude only their known incompatible keys such as "mode" or "compress"
when appropriate), using the existing symbols c.LogConfig.Config,
c.LogConfig.Type and the local config map to perform driver-specific
whitelist/blacklist checks.
There was a problem hiding this comment.
🧩 Analysis chain
@Mawiguk0, I'll help you implement the fix for driver-specific option filtering. Let me first gather information about which options are supported by different Docker logging drivers.
🌐 Web query:
Docker logging driver options compatibility json-file fluentd syslog local
💡 Result:
Short compatibility summary — which log-opt keys are shared vs driver-specific:
Common/available to multiple drivers
- labels / labels-regex — supported by json-file, fluentd (and used in daemon defaults). [1][3][5]
- env / env-regex — supported by json-file, fluentd (and usable in daemon defaults). [1][3][5]
- tag — supported by drivers that use tags (fluentd explicitly; syslog has RFC/format options). Use depends on driver. [3][4]
json-file (driver-specific options)
- max-size, max-file, compress, labels, env, labels-regex, env-regex. (Log format = JSON file). [1]
local (driver-specific; recommended over json-file)
- max-size, max-file, compress (local stores in optimized internal format and handles rotation by default). Not all json-file internals/behavior are identical to local. [2]
fluentd (driver-specific)
- fluentd-address, tag, fluentd-async, fluentd-max-retries, labels, env (and their regex variants). Connection and delivery semantics differ from file drivers. [3]
syslog (driver-specific)
- syslog-address, syslog-facility, syslog-tls-ca-cert, syslog-tls-cert, syslog-tls-key, syslog-tls-skip-verify, syslog-format, etc.; plus some tag/label behavior. [4]
Notes
- Many options are driver-specific; only labels/env and their regex variants (and tag-like options) are commonly implemented across these drivers. File-rotation options (max-size/max-file/compress) apply to file-based drivers (json-file and local) but not to remote-forwarding drivers (fluentd, syslog). [1][2][3][4]
- For authoritative details and the full option lists, see the Docker docs for each driver. [1][2][3][4]
Sources
[1] JSON-file logging driver options — Docker Docs.
[2] Local logging driver — Docker Docs.
[3] Fluentd logging driver — Docker Docs.
[4] Syslog logging driver — Docker Docs.
Task: Implement driver-specific logging option filtering
Based on Docker documentation:
- File-based drivers (
json-file,local): supportmax-size,max-file,compress localdriver only: additionally supportsmodeoption- Remote drivers (
fluentd,syslog, etc.): do NOT support file-rotation options
Plan:
- Update filtering logic to preserve file-rotation options for json-file driver
- Only exclude
modefor json-file (keep max-size, max-file, compress) - For remote drivers, exclude all file-rotation options (max-size, max-file, compress, mode)
- Update unit tests to verify json-file driver gets rotation options
🏁 Script executed:
#!/bin/bash
# Find and display the test file for ContainerLogConfig
fd -e go -g '*test*' config/ --exec grep -l "TestContainerLogConfig" {} \;
# Display the test content
fd -e go -g '*test*' config/ --exec grep -l "TestContainerLogConfig" {} \; | head -1 | xargs cat
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all test files in config directory
fd -e go . config/ | grep -i test
# Also search for any references to ContainerLogConfig in the codebase
rg "ContainerLogConfig" -n --type go
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Check what files exist in config directory
fd -e go . config/
# Check git status and recent changes in config directory
git log --oneline --name-only -5 -- config/ | head -30
# Try to find if there's a test file that was added recently
git diff --name-only HEAD~5..HEAD | grep config
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Check the actual diff for the PR to see if test file was added
git show ec557ad --name-status
# Look for all files changed in the current branch compared to main
git diff --name-status origin/main..HEAD 2>/dev/null || git diff --name-status HEAD~2..HEAD
# Check if there's a config_docker_test.go file
ls -la config/ | grep -i test
Length of output: 274
🏁 Script executed:
#!/bin/bash
# Check the entire git diff for this commit to see all changes
git show ec557ad --stat
# Look at what the PR description says about tests
# Check if there's a config_docker_test.go that should exist
cat config/config_docker_test.go 2>/dev/null || echo "No test file found"
# List all files in config directory
ls -la config/
Length of output: 563
Task: Implement driver-specific logging option filtering
Based on Docker documentation:
- File-based drivers (
json-file,local): supportmax-size,max-file,compress localdriver only: additionally supportsmodeoption- Remote drivers (
fluentd,syslog, etc.): do NOT support file-rotation options
Plan:
- Update filtering logic to preserve file-rotation options for json-file driver
- Create comprehensive unit tests to verify the behavior for different drivers
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
|
|
||
| // If the environment variable is set, skip actual writes. Useful for containers | ||
| // or managed environments where you want Wings to never modify files. | ||
| if os.Getenv("WINGS_NO_CONFIG_WRITE") == "1" { |
There was a problem hiding this comment.
If wings sets a config value to something else then what you provided then it means its wrong this is not a proper way to fix it imo.
There was a problem hiding this comment.
I disagree because the code you marked has nothing to do with that behavior?
2 participants
Description
This PR fixes an issue where Docker logging driver-specific configuration options could cause errors when switching between different logging drivers.
The
locallogging driver supports specific options (max-size,max-file,compress,mode) that are not compatible with other drivers likefluentd,json-file,syslog, etc. Previously, if a user had these options configured and switched to a different driver, Docker would reject the incompatible options.Related
Following Issues are related to this PR:
log_optionsresets after restart #129Changes
ContainerLogConfig()inconfig/config_docker.goto filter outlocal-driver-specific options when using other logging driversTesting
Added unit tests covering:
localdriver preserves all config optionsfluentddriver excludes local-specific optionsjson-filedriver excludes local-specific optionssyslogdriver excludes local-specific optionsgo test -v ./config/... -run TestContainerLogConfig