Skip to content

fix: Filter incompatible logging driver options in ContainerLogConfig #160

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.

Sign up for GitHub

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

Open
Mawiguk0 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: Filter incompatible logging driver options in ContainerLogConfig #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da83c6a
feat: Introduceenv var to stop wings from writing to its own config file
Mawiguk0 Oct 21, 2025
a737068
Merge branch 'pelican-dev:main' into main
Mawiguk0 Dec 9, 2025
ec557ad
Fix: filter log config options for non-local logging drivers
Mawiguk0 Dec 9, 2025
816cb6e
Merge branch 'pelican-dev:main' into main
Mawiguk0 Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,15 @@ func GetJwtAlgorithm() *jwt.HMACSHA {
// and will only allow one write at a time. Additional calls while writing are
// queued up.
func WriteToDisk(c *Configuration) error {

// 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" {
Copy link
Member

@rmartinoscar rmartinoscar Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Mawiguk0 Mawiguk0 Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree because the code you marked has nothing to do with that behavior?

// 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.
return nil
}

_writeLock.Lock()
defer _writeLock.Unlock()

Expand Down
16 changes: 15 additions & 1 deletion config/config_docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,23 @@ func (c DockerConfiguration) ContainerLogConfig() container.LogConfig {
return container.LogConfig{}
}

// 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
}
Comment on lines +106 to +118
Copy link

@coderabbitai coderabbitai bot Jan 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.

Copy link
Author

@Mawiguk0 Mawiguk0 Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be done.

Copy link

@coderabbitai coderabbitai bot Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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): support max-size, max-file, compress
  • local driver only: additionally supports mode option
  • Remote drivers (fluentd, syslog, etc.): do NOT support file-rotation options

Plan:

  1. Update filtering logic to preserve file-rotation options for json-file driver
  2. Only exclude mode for json-file (keep max-size, max-file, compress)
  3. For remote drivers, exclude all file-rotation options (max-size, max-file, compress, mode)
  4. 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): support max-size, max-file, compress
  • local driver only: additionally supports mode option
  • Remote drivers (fluentd, syslog, etc.): do NOT support file-rotation options

Plan:

  1. Update filtering logic to preserve file-rotation options for json-file driver
  2. Create comprehensive unit tests to verify the behavior for different drivers

💡 For code changes across multiple files, use @coderabbitai plan in an issue comment


return container.LogConfig{
Type: c.LogConfig.Type,
Config: c.LogConfig.Config,
Config: config,
}
}

Expand Down