[management] enable access log cleanup by default#5842
Merged
pascal-fischer merged 4 commits intomainfrom Apr 10, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughStartPeriodicCleanup now treats Changes
Sequence Diagram(s)(omitted — changes are localized and do not introduce multi-component sequential flows) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
management/internals/modules/reverseproxy/accesslogs/manager/manager.go (1)
114-126: Add focused tests for the new retention semantics.Given the behavior change (
retentionDays == 0defaults to7,< 0disables cleanup), please add targeted unit tests for these branches to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/accesslogs/manager/manager.go` around lines 114 - 126, Add focused unit tests covering the new retention semantics around the retentionDays variable: one test where retentionDays == 0 verifies it defaults to 7 days (and any created cleanup job uses 7), and one test where retentionDays < 0 verifies cleanup is disabled (no cleanup job/scheduler started). Use the same constructor or initializer used in manager.go (the code paths that read retentionDays and cleanupIntervalHours) to create the manager and assert resulting behavior/state; also add a test for cleanupIntervalHours <= 0 defaulting to 24 hours if convenient. Name tests clearly (e.g., TestRetention_DefaultsTo7_WhenZero and TestRetention_Disabled_WhenNegative) and mock or inspect the scheduler/cleanup job to confirm the expected behavior and logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/modules/reverseproxy/accesslogs/manager/manager.go`:
- Around line 109-110: The debug message for the retention check is inconsistent
with the condition; update the log call that uses retentionDays (the
log.WithContext(ctx).Debug call in manager.go) so the message matches the check
for retentionDays < 0 (e.g., "periodic access log cleanup disabled: retention
days is negative") or alternatively change the condition to <= 0 if the intent
was to disable on zero as well; adjust the string accordingly in the
log.WithContext(ctx).Debug invocation referencing retentionDays to keep
condition and message aligned.
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/accesslogs/manager/manager.go`:
- Around line 114-126: Add focused unit tests covering the new retention
semantics around the retentionDays variable: one test where retentionDays == 0
verifies it defaults to 7 days (and any created cleanup job uses 7), and one
test where retentionDays < 0 verifies cleanup is disabled (no cleanup
job/scheduler started). Use the same constructor or initializer used in
manager.go (the code paths that read retentionDays and cleanupIntervalHours) to
create the manager and assert resulting behavior/state; also add a test for
cleanupIntervalHours <= 0 defaulting to 24 hours if convenient. Name tests
clearly (e.g., TestRetention_DefaultsTo7_WhenZero and
TestRetention_Disabled_WhenNegative) and mock or inspect the scheduler/cleanup
job to confirm the expected behavior and logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6b5371f-82e7-4c00-9257-79195247f790
📒 Files selected for processing (2)
management/internals/modules/reverseproxy/accesslogs/manager/manager.gomanagement/internals/server/config/config.go
management/internals/modules/reverseproxy/accesslogs/manager/manager.go
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/modules/reverseproxy/accesslogs/manager/manager.go`:
- Around line 114-119: The retention behavior in manager.go now defaults
retentionDays==0 to 7 days instead of disabling cleanup; update the unit test in
management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go
(the test that covers "zero retention" around Lines 123-143) to expect cleanup
to run with a 7-day retention instead of being skipped—adjust assertions to
check that StartCleanup (or the cleanup goroutine triggered by the Manager
constructor / NewManager) is invoked and that the configured retention value
equals 7, and update any log/expectation strings from "disabled" to "defaulting
to 7 days" or the new debug message emitted by the Manager.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6facc38-ebed-40b3-aee8-78b6a189ab47
📒 Files selected for processing (1)
management/internals/modules/reverseproxy/accesslogs/manager/manager.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
combined/cmd/config.go (1)
360-363:⚠️ Potential issue | 🟠 MajorCustom access-log retention/cleanup can be silently dropped in simplified mode.
At Line 361, the copy gate ignores the new fields. If users set only
server.reverseProxy.accessLogRetentionDaysorserver.reverseProxy.accessLogCleanupIntervalHours, Line 362 never runs, so management config keeps zero values instead of the configured ones.💡 Proposed fix
- // Copy reverse proxy config from server - if len(c.Server.ReverseProxy.TrustedHTTPProxies) > 0 || c.Server.ReverseProxy.TrustedHTTPProxiesCount > 0 || len(c.Server.ReverseProxy.TrustedPeers) > 0 { - c.Management.ReverseProxy = c.Server.ReverseProxy - } + // Copy reverse proxy config from server + // In simplified mode, server.reverseProxy is the source of truth. + c.Management.ReverseProxy = c.Server.ReverseProxy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@combined/cmd/config.go` around lines 360 - 363, The copy gate for reverse proxy config only checks TrustedHTTPProxies, TrustedHTTPProxiesCount, and TrustedPeers so custom access-log fields like AccessLogRetentionDays and AccessLogCleanupIntervalHours can be skipped; update the condition around assigning c.Management.ReverseProxy = c.Server.ReverseProxy to also check c.Server.ReverseProxy.AccessLogRetentionDays and c.Server.ReverseProxy.AccessLogCleanupIntervalHours (or simply always copy the struct when any reverse-proxy-related field is non-zero), ensuring the AccessLogRetentionDays and AccessLogCleanupIntervalHours values from c.Server.ReverseProxy are propagated to c.Management.ReverseProxy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@combined/cmd/config.go`:
- Around line 360-363: The copy gate for reverse proxy config only checks
TrustedHTTPProxies, TrustedHTTPProxiesCount, and TrustedPeers so custom
access-log fields like AccessLogRetentionDays and AccessLogCleanupIntervalHours
can be skipped; update the condition around assigning c.Management.ReverseProxy
= c.Server.ReverseProxy to also check
c.Server.ReverseProxy.AccessLogRetentionDays and
c.Server.ReverseProxy.AccessLogCleanupIntervalHours (or simply always copy the
struct when any reverse-proxy-related field is non-zero), ensuring the
AccessLogRetentionDays and AccessLogCleanupIntervalHours values from
c.Server.ReverseProxy are propagated to c.Management.ReverseProxy.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go (1)
124-138:⚠️ Potential issue | 🟡 MinorAdd an explicit periodic test for the zero-retention default path.
The update at Line 138 (
-1) correctly covers the disable case, but this suite now lacks a direct assertion thatretentionDays == 0triggers cleanup with the 7-day default (core behavior of this PR). Please add a subtest for that path.Suggested test addition
@@ t.Run("periodic cleanup disabled with negative retention", func(t *testing.T) { @@ }) + + t.Run("periodic cleanup defaults to 7 days when retention is zero", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, olderThan time.Time) (int64, error) { + expectedCutoff := time.Now().AddDate(0, 0, -7) + assert.Less(t, olderThan.Sub(expectedCutoff).Abs(), time.Second) + return int64(1), nil + }). + Times(1) + + manager := &managerImpl{store: mockStore} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + manager.StartPeriodicCleanup(ctx, 0, 24) + time.Sleep(100 * time.Millisecond) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go` around lines 124 - 138, Add a new subtest that calls managerImpl.StartPeriodicCleanup with retentionDays == 0 and asserts the default 7-day retention path is used: create a gomock controller and a store.NewMockStore(ctrl), set expectations on mockStore for a single cleanup invocation (or whatever method managerImpl calls to purge logs) using retentionDays == 7 (or matching a 7-day time window), instantiate manager := &managerImpl{store: mockStore}, start the periodic cleanup with ctx and interval 1, let it run briefly, then cancel and verify mock expectations; use symbols StartPeriodicCleanup and managerImpl to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go`:
- Around line 124-138: Add a new subtest that calls
managerImpl.StartPeriodicCleanup with retentionDays == 0 and asserts the default
7-day retention path is used: create a gomock controller and a
store.NewMockStore(ctrl), set expectations on mockStore for a single cleanup
invocation (or whatever method managerImpl calls to purge logs) using
retentionDays == 7 (or matching a 7-day time window), instantiate manager :=
&managerImpl{store: mockStore}, start the periodic cleanup with ctx and interval
1, let it run briefly, then cancel and verify mock expectations; use symbols
StartPeriodicCleanup and managerImpl to locate where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7df96d6a-d66e-4b15-9fae-ed66126bda9e
📒 Files selected for processing (1)
management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go
crn4
approved these changes
Apr 10, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Describe your changes
Previously the access log cleanup for the proxy was only executed if explicitly enabled. This PR changes the behaviour to have a default retention of 7 days. Use negative values to keep data forever.
Issue ticket number and link
#5773
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#693
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style / Logging
Tests