[PR #6108] [client] util/log: ensure log directory exists on first write (fixes #4392) #29049

Open
opened 2026-08-05 08:07:22 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6108
Author: @MichaelUray
Created: 5/8/2026
Status: 🔄 Open

Base: mainHead: fix/log-dir-creation-on-start


📝 Commits (1)

  • 3a0b544 [client] util/log: ensure log directory exists on first write

📊 Changes

2 files changed (+91 additions, -0 deletions)

View changed files

📝 util/log.go (+22 -0)
📝 util/log_test.go (+69 -0)

📄 Description

Summary

Closes #4392.

lumberjack.Logger creates the log file lazily on first write, but does not create the parent directory. If the directory is missing, the first write fails with no such file or directory and the daemon never produces logs. The user-reported failure mode is on a fresh install where netbird service install was never run (or where /var/log/netbird/ was removed manually): service start and direct daemon runs then produce no log output, while /var/lib/netbird/ is created automatically by the profile manager (asymmetry between data-dir and log-dir handling).

Change

util/log.go newRotatedOutput(): pre-create the parent directory before constructing the lumberjack.Logger. MkdirAll is a no-op when the directory already exists, so installs that already went through service install are unaffected.

If the daemon lacks permission to create the directory (e.g. running as a less-privileged user against a system-owned /var/log path), we log a warning and continue — lumberjack will then surface a concrete open-error on the first write rather than silently failing. Permissions match the existing 0o750 used by service_installer.go:84 so behavior is symmetric across both creation paths.

Tests

New file util/log_test.go with three cases:

  • TestNewRotatedOutputCreatesMissingParentDir — the bug-reproducer: nested log path under a tempdir whose intermediate directories don't exist; the test asserts that the parent dir is created and the subsequent write succeeds.
  • TestNewRotatedOutputWithExistingParentDir — sanity: function is a no-op when the parent already exists.
  • TestNewRotatedOutputWithRootOrCwdParentDoesNotPanic — guards against spurious MkdirAll calls for paths whose Dir() resolves to . or /.
$ go test ./util/ -run TestNewRotatedOutput -v
=== RUN   TestNewRotatedOutputCreatesMissingParentDir
--- PASS: TestNewRotatedOutputCreatesMissingParentDir (0.00s)
=== RUN   TestNewRotatedOutputWithExistingParentDir
--- PASS: TestNewRotatedOutputWithExistingParentDir (0.00s)
=== RUN   TestNewRotatedOutputWithRootOrCwdParentDoesNotPanic
--- PASS: TestNewRotatedOutputWithRootOrCwdParentDoesNotPanic (0.00s)
PASS
ok      github.com/netbirdio/netbird/util       0.004s

Hardware-validated end-to-end:

$ rm -rf /tmp/nb-test/
$ ./netbird service status --log-file /tmp/nb-test/netbird/client.log --daemon-addr unix:///nonexistent.sock
NetBird service status: Running
$ ls -la /tmp/nb-test/netbird/
drwxr-x---  2 ai-agent ai-agent  40 May  8 08:32 .
drwxr-x---  3 ai-agent ai-agent  60 May  8 08:32 ..

Without this PR, the same invocation leaves /tmp/nb-test/ non-existent because lumberjack never reaches the file-open step on a write-only logger that didn't get any writes — and worse, on the daemon run path the missing directory leads to silent log loss until the directory is manually created.

Test plan

  • Unit tests cover the create / already-exists / .-and-/ cases.
  • No regression for the path that already goes through service install (existing dir → MkdirAll is no-op).
  • Permission failure path returns a writable lumberjack logger and a clear warning rather than panicking.

Use case

Real-world: OpenWrt routers using a packaged init.d wrapper that calls /usr/sbin/netbird directly without going through netbird service install, and any host where /var/log/netbird/ was removed (e.g. log-rotation cleanup gone wrong). With this fix the daemon recreates the directory automatically on next start instead of failing silently.

  • I added/updated documentation
  • Documentation is not needed

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced log file handling to automatically create missing parent directories before writing logs, ensuring logs are reliably written even if directories have been removed or never created, preventing silent logging failures.
  • Tests

    • Added tests to verify proper log file creation and handling of various directory scenarios.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/6108 **Author:** [@MichaelUray](https://github.com/MichaelUray) **Created:** 5/8/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/log-dir-creation-on-start` --- ### 📝 Commits (1) - [`3a0b544`](https://github.com/netbirdio/netbird/commit/3a0b544a84437d0db798884b7c08ac6df2e390d6) [client] util/log: ensure log directory exists on first write ### 📊 Changes **2 files changed** (+91 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `util/log.go` (+22 -0) 📝 `util/log_test.go` (+69 -0) </details> ### 📄 Description ## Summary Closes #4392. `lumberjack.Logger` creates the log file lazily on first write, but does **not** create the parent directory. If the directory is missing, the first write fails with `no such file or directory` and the daemon never produces logs. The user-reported failure mode is on a fresh install where `netbird service install` was never run (or where `/var/log/netbird/` was removed manually): `service start` and direct daemon runs then produce no log output, while `/var/lib/netbird/` is created automatically by the profile manager (asymmetry between data-dir and log-dir handling). ## Change [util/log.go](util/log.go) `newRotatedOutput()`: pre-create the parent directory before constructing the `lumberjack.Logger`. `MkdirAll` is a no-op when the directory already exists, so installs that already went through `service install` are unaffected. If the daemon lacks permission to create the directory (e.g. running as a less-privileged user against a system-owned `/var/log` path), we log a warning and continue — `lumberjack` will then surface a concrete open-error on the first write rather than silently failing. Permissions match the existing `0o750` used by [`service_installer.go:84`](client/cmd/service_installer.go#L84) so behavior is symmetric across both creation paths. ## Tests New file `util/log_test.go` with three cases: - `TestNewRotatedOutputCreatesMissingParentDir` — the bug-reproducer: nested log path under a tempdir whose intermediate directories don't exist; the test asserts that the parent dir is created and the subsequent write succeeds. - `TestNewRotatedOutputWithExistingParentDir` — sanity: function is a no-op when the parent already exists. - `TestNewRotatedOutputWithRootOrCwdParentDoesNotPanic` — guards against spurious `MkdirAll` calls for paths whose `Dir()` resolves to `.` or `/`. ``` $ go test ./util/ -run TestNewRotatedOutput -v === RUN TestNewRotatedOutputCreatesMissingParentDir --- PASS: TestNewRotatedOutputCreatesMissingParentDir (0.00s) === RUN TestNewRotatedOutputWithExistingParentDir --- PASS: TestNewRotatedOutputWithExistingParentDir (0.00s) === RUN TestNewRotatedOutputWithRootOrCwdParentDoesNotPanic --- PASS: TestNewRotatedOutputWithRootOrCwdParentDoesNotPanic (0.00s) PASS ok github.com/netbirdio/netbird/util 0.004s ``` Hardware-validated end-to-end: ``` $ rm -rf /tmp/nb-test/ $ ./netbird service status --log-file /tmp/nb-test/netbird/client.log --daemon-addr unix:///nonexistent.sock NetBird service status: Running $ ls -la /tmp/nb-test/netbird/ drwxr-x--- 2 ai-agent ai-agent 40 May 8 08:32 . drwxr-x--- 3 ai-agent ai-agent 60 May 8 08:32 .. ``` Without this PR, the same invocation leaves `/tmp/nb-test/` non-existent because lumberjack never reaches the file-open step on a write-only logger that didn't get any writes — and worse, on the daemon `run` path the missing directory leads to silent log loss until the directory is manually created. ## Test plan - [x] Unit tests cover the create / already-exists / `.`-and-`/` cases. - [x] No regression for the path that already goes through `service install` (existing dir → `MkdirAll` is no-op). - [x] Permission failure path returns a writable lumberjack logger and a clear warning rather than panicking. ## Use case Real-world: OpenWrt routers using a packaged `init.d` wrapper that calls `/usr/sbin/netbird` directly without going through `netbird service install`, and any host where `/var/log/netbird/` was removed (e.g. log-rotation cleanup gone wrong). With this fix the daemon recreates the directory automatically on next start instead of failing silently. <!-- Docs acknowledgement --> - [ ] I added/updated documentation - [x] Documentation is **not needed** <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced log file handling to automatically create missing parent directories before writing logs, ensuring logs are reliably written even if directories have been removed or never created, preventing silent logging failures. * **Tests** * Added tests to verify proper log file creation and handling of various directory scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:07:22 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#29049