Conversation
WalkthroughSwitches ipset usage to github.com/lrh3321/ipset-go, adds unexported ipset wrapper methods for lifecycle management in firewall ACL and router modules, refines ipset error handling and IP detection, updates go.mod dependencies, and extends the GitHub Actions workflow to separate internal AGPL checks from external GPL/AGPL/LGPL checks (notes MPL-2.0). Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant ACL as aclManager
participant Router as router
participant Lib as ipset-go
rect rgb(220,235,255)
Note over Caller,ACL: AddPeerFiltering — lazy create & add
Caller->>ACL: AddPeerFiltering(peer)
ACL->>ACL: createIPSet(name) -- create if missing
ACL->>Lib: Create / Replace
Lib-->>ACL: ok / error
ACL->>ACL: addToIPSet(name, ip) -- Add / AddPrefix
ACL->>Lib: Add / AddPrefix
Lib-->>ACL: ok / error
ACL-->>Caller: complete
end
rect rgb(255,240,220)
Note over Caller,ACL: DeletePeerRule — conditional destroy
Caller->>ACL: DeletePeerRule(peer)
ACL->>ACL: delFromIPSet(name, ip)
ACL->>Lib: Del / DelPrefix
Lib-->>ACL: remainingCount / ok / ENOENT
alt remainingCount == 0
ACL->>ACL: destroyIPSet(name)
ACL->>Lib: Destroy
Lib-->>ACL: ok / ENOENT / EBUSY
else remainingCount > 0
ACL-->>Caller: no destroy
end
ACL-->>Caller: complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (14)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
.github/workflows/check-license-dependencies.yml(2 hunks)client/firewall/iptables/acl_linux.go(10 hunks)client/firewall/iptables/router_linux.go(5 hunks)go.mod(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/iptables/acl_linux.go (1)
client/firewall/iptables/manager_linux.go (1)
Create(39-61)
⏰ 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). (14)
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Check External GPL/AGPL Licenses
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/check-license-dependencies.yml(2 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). (18)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
🔇 Additional comments (2)
.github/workflows/check-license-dependencies.yml (2)
6-14: Path filters now properly include go.sum.This addresses the previous review feedback. The workflow will now correctly trigger on go.sum changes, catching transitive dependency updates and checksum bumps.
66-66: I need to gather information to verify the review comment's claims about thego install @latestpattern.The web search limit has been reached. Let me search for more specific information about Go's
@latestbehavior in workflows:Update the go-licenses version pin to a current release.
The concern about reproducibility with
@latestis valid for CI/CD pipelines. However, the latest stable version is v1.6.0, not v0.2.1 as suggested. If pinning is desired, use a current version. Alternatively, the official documentation still recommendsgo install github.com/google/go-licenses@latest, which is acceptable given Go's module caching behavior.Apply this diff if pinning:
- - name: Install go-licenses - run: go install github.com/google/go-licenses@latest + - name: Install go-licenses + run: go install github.com/google/go-licenses@v1.6.0
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/check-license-dependencies.yml (1)
74-74: Duplicate: Unresolved—stderr masking and narrow regex pattern still present.Two previously flagged critical issues remain unaddressed in the new external license check:
Line 74:
2>/dev/nullsilences real errors fromgo-licenses report, masking failures (network issues, malformed go.mod, tool crashes). The trailing|| truefurther hides failures.Line 84: The regex pattern
'GPL-[0-9]|AGPL-[0-9]|LGPL-[0-9]'only matches formats likeGPL-2but misses standard SPDX identifiers such asGPL-2.0-only,GPL-2.0-or-later,AGPL-3.0-only, orLGPL-2.1-only, causing false negatives.Apply this diff to surface real errors and match SPDX identifiers:
- COPYLEFT_DEPS=$(go-licenses report ./... 2>/dev/null | grep -E 'GPL|AGPL|LGPL' | grep -v 'github.com/netbirdio/netbird/' || true) + COPYLEFT_DEPS=$(go-licenses report ./... | grep -E 'GPL|AGPL|LGPL' | grep -v 'github.com/netbirdio/netbird/' || true)And:
- if echo "$license" | grep -qE 'GPL-[0-9]|AGPL-[0-9]|LGPL-[0-9]'; then + if echo "$license" | grep -qE 'GPL-|AGPL-|LGPL-' | grep -qv 'MPL-'; thenAlso applies to: 84-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/check-license-dependencies.yml(2 hunks)client/firewall/iptables/acl_linux.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/iptables/acl_linux.go (1)
client/firewall/iptables/manager_linux.go (1)
Create(39-61)
⏰ 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). (17)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: JS / Lint
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (2)
.github/workflows/check-license-dependencies.yml (2)
6-14: ✅ Path filters now include go.sum.The addition of
go.sumto the path filters in both push and pull_request events resolves the earlier concern about workflow not triggering on transitive dependency additions or checksum-only updates.
52-108: Verify importer detection logic correctly identifies BSD-licensed code boundaries.The new external license check at lines 85–98 uses
go list -json -depsto find packages that import GPL-licensed dependencies, then filters out importers undermanagement/,signal/, orrelay/directories. However, there are a few concerns:
Line 86 logic: The filter
select(.Imports[]? == \"$package\")assumes direct imports. Transitive imports (A → B → GPL) may not be caught, potentially missing incompatibilities.Line 89 pattern matching: The grep pattern
github.com/netbirdio/netbird/\(management\|signal\|relay\)assumes a specific directory structure. If there are nested packages likemanagement/foo/barthat are not BSD-licensed, they might incorrectly pass the check.Please verify that:
- The importer detection correctly handles both direct and transitive dependencies.
- All known BSD-licensed packages are outside the management/signal/relay directories, or that nested exceptions are properly accounted for.
|
2 participants
Describe your changes
Issue ticket number and link
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:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes
Chores