[relay] Update GO version and QUIC version #4736
Conversation
- Go 1.23.0 - QUIC 0.55.0
because security issue in golang.org/x/crypto/ssh/agent
WalkthroughBumps Go toolchain to 1.25 in CI and go.mod, upgrades many dependencies and golangci-lint config, replaces legacy "// +build" tags with "//go:build", adds several //nolint annotations, performs minor refactors (if→switch, var style), changes QUIC Conn session types and one user-auth context type. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant NewConn as NewConn ctor
participant ConnObj as Conn (struct)
Caller->>NewConn: NewConn(session: *quic.Conn)
note right of NewConn `#DDEEFF`: constructor now accepts pointer
NewConn->>ConnObj: allocate Conn { session: *quic.Conn }
ConnObj-->>Caller: return *Conn (implements net.Conn)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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 selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (28)
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
🧹 Nitpick comments (12)
relay/server/listener/quic/conn.go (1)
48-50: Consider adding defensive nil checks for session.The client implementation (
shared/relay/client/dialer/quic/conn.goline 69) includes a nil check for the session pointer inLocalAddr(). Consider adding similar checks here for consistency and defensive programming, though this may be optional if the session is guaranteed to be non-nil by construction.Example implementation:
func (c *Conn) LocalAddr() net.Addr { + if c.session != nil { + return c.session.LocalAddr() + } + return nil - return c.session.LocalAddr() }Also applies to: 52-54
.github/workflows/golang-test-darwin.yml (1)
127-130: Consider specifying the linter rule being suppressed.Using a generic
//nolintdirective suppresses all linter warnings for this line. Consider specifying the exact rule(s) being suppressed for better maintainability (e.g.,//nolint:lllif suppressing line length, or//nolint:errorlintif suppressing error formatting rules).Example:
- //nolint + //nolint:lll // Long error message for user guidance return nil, fmt.Errorf("failed to connect to daemon error: %v\n"+management/server/loginfilter_test.go (1)
88-90: Specify the linter rule being suppressed.The generic
// nolintcomment lacks specificity. Consider indicating which linter rule is being suppressed for clarity and to avoid accidentally hiding future issues.Example:
- // nolint + // nolint:mnd // Magic number in exponent is intentional for test expectedSecondDuration := time.Duration(float64(baseBan) * math.Pow(2, 1))management/server/account.go (1)
1007-1007: Revert to specific linter directive or fix the underlying issue.Changing from
//nolint:gosimpleto the generic//nolintmakes the suppression less precise and could mask future linting issues. Consider either:
- Keeping the specific directive:
//nolint:gosimple- Fixing the redundant boolean comparison:
loggedInOnce == true→loggedInOnceExample fix:
- if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce == true { //nolint + if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce {Or keep specific suppression:
- if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce == true { //nolint + if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce == true { //nolint:gosimpleclient/cmd/root.go (1)
393-396: Specify the linter rule being suppressed.Like in other command files, using a generic
//nolintis less precise than specifying the exact rule. Consider indicating which linter rule is being suppressed (likely line length or error formatting).Example:
- //nolint + //nolint:lll // Long user-facing error message return nil, fmt.Errorf("failed to connect to daemon error: %v\n"+client/cmd/up.go (1)
219-222: Make the linter suppression more specific.The generic
//nolintdirective suppresses all linter warnings. For better code maintainability, specify which linter rule is being suppressed.Example:
- //nolint + //nolint:lll // Long user guidance message return fmt.Errorf("failed to connect to daemon error: %v\n"+client/cmd/debug.go (1)
139-140: Specify the linter rule being suppressed.The
//nolintdirective should include the specific rule being suppressed (e.g.,//nolint:goerr113) and a brief comment explaining why. This improves maintainability and makes it clear what check is being bypassed.Apply this pattern:
- //nolint + //nolint:goerr113 // dynamic error message required for user feedback return fmt.Errorf("unknown log level: %s. Available levels are: panic, fatal, error, warn, info, debug, trace\n", args[0])client/ios/NetBirdSDK/login.go (1)
136-137: Specify the linter rule being suppressed.The
//nolintdirective should include the specific rule being suppressed and a brief comment explaining why suppression is necessary.Apply this pattern:
- //nolint + //nolint:goerr113 // authentication state error message return fmt.Errorf("Not authenticated")client/cmd/login.go (2)
86-89: Specify the linter rule being suppressed.The
//nolintdirective should include the specific rule being suppressed and a brief comment explaining why. This pattern appears twice in this file and should be consistently addressed.Apply this pattern:
- //nolint + //nolint:goerr113 // dynamic daemon connection error with instructions return fmt.Errorf("failed to connect to daemon error: %v\n"+
205-208: Specify the linter rule being suppressed.Same issue as line 86 - the nolint directive should specify the rule and include a comment.
client/ui/client_ui.go (1)
472-474: Clarify the reason for nolint directives.Multiple
//nolintdirectives have been added arounddialog.ShowErrorcalls throughout this file. These directives lack specificity about which linter rule is being suppressed and why.Consider:
- Using specific nolint directives like
//nolint:errcheckor the actual linter name being suppressed- Adding a comment explaining why the lint rule doesn't apply in these UI error display contexts
- Investigating if there's a better pattern to handle these errors that satisfies the linter
Example:
-//nolint -dialog.ShowError(fmt.Errorf("Configuration updates are disabled by daemon"), s.wSettings) +//nolint:errcheck // Error display in UI, no programmatic handling needed +dialog.ShowError(fmt.Errorf("Configuration updates are disabled by daemon"), s.wSettings)client/cmd/status.go (1)
127-127: Make the nolint directive more specific.The bare
//nolintdirective suppresses all linter warnings for the following line. Consider specifying which linter rule you're suppressing (e.g.,//nolint:lllfor line length or//nolint:goerr113for dynamic error wrapping) to avoid inadvertently hiding other potential issues. Optionally, add a brief comment explaining why the suppression is necessary.Example:
- //nolint + //nolint:lll // Long error message provides helpful user instructions
📜 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 (77)
.github/workflows/golang-test-darwin.yml(1 hunks).github/workflows/golang-test-freebsd.yml(1 hunks).github/workflows/golang-test-linux.yml(10 hunks).github/workflows/golang-test-windows.yml(1 hunks).github/workflows/golangci-lint.yml(1 hunks).github/workflows/mobile-build-validation.yml(3 hunks).github/workflows/release.yml(3 hunks).github/workflows/test-infrastructure-files.yml(1 hunks).github/workflows/wasm-build-validation.yml(2 hunks).golangci.yaml(1 hunks)client/cmd/debug.go(1 hunks)client/cmd/login.go(2 hunks)client/cmd/pprof.go(0 hunks)client/cmd/root.go(1 hunks)client/cmd/status.go(1 hunks)client/cmd/up.go(1 hunks)client/firewall/iptables/acl_linux.go(1 hunks)client/firewall/iptables/manager_linux_test.go(1 hunks)client/firewall/nftables/manager_linux_test.go(3 hunks)client/firewall/uspfilter/filter.go(1 hunks)client/firewall/uspfilter/localip.go(1 hunks)client/firewall/uspfilter/localip_test.go(2 hunks)client/firewall/uspfilter/nat_test.go(1 hunks)client/iface/device/device_ios.go(0 hunks)client/internal/debug/debug_linux.go(1 hunks)client/internal/dns/upstream.go(1 hunks)client/internal/iface.go(0 hunks)client/internal/routemanager/iface/iface.go(0 hunks)client/internal/routemanager/systemops/systemops_generic.go(5 hunks)client/ios/NetBirdSDK/login.go(1 hunks)client/server/panic_windows.go(0 hunks)client/ssh/lookup.go(0 hunks)client/ssh/lookup_darwin.go(0 hunks)client/system/info_android.go(0 hunks)client/system/info_darwin.go(0 hunks)client/system/info_ios.go(0 hunks)client/ui/client_ui.go(5 hunks)go.mod(7 hunks)management/cmd/management.go(1 hunks)management/server/account.go(1 hunks)management/server/http/handlers/policies/posture_checks_handler_test.go(2 hunks)management/server/http/testing/benchmarks/peers_handler_benchmark_test.go(0 hunks)management/server/http/testing/benchmarks/setupkeys_handler_benchmark_test.go(0 hunks)management/server/http/testing/benchmarks/users_handler_benchmark_test.go(0 hunks)management/server/http/testing/integration/setupkeys_handler_integration_test.go(0 hunks)management/server/idp/pocketid.go(2 hunks)management/server/idp/zitadel.go(1 hunks)management/server/loginfilter_test.go(1 hunks)management/server/migration/migration.go(1 hunks)management/server/nameserver.go(2 hunks)management/server/posture_checks.go(1 hunks)management/server/store/sql_store_test.go(5 hunks)management/server/testutil/store.go(0 hunks)management/server/testutil/store_ios.go(0 hunks)management/server/user.go(2 hunks)relay/cmd/pprof.go(0 hunks)relay/server/listener/quic/conn.go(1 hunks)relay/server/listener/ws/conn.go(1 hunks)shared/management/client/rest/accounts_test.go(0 hunks)shared/management/client/rest/client.go(1 hunks)shared/management/client/rest/client_test.go(0 hunks)shared/management/client/rest/dns_test.go(0 hunks)shared/management/client/rest/events_test.go(0 hunks)shared/management/client/rest/geo_test.go(0 hunks)shared/management/client/rest/groups_test.go(0 hunks)shared/management/client/rest/impersonation_test.go(0 hunks)shared/management/client/rest/networks_test.go(0 hunks)shared/management/client/rest/peers_test.go(0 hunks)shared/management/client/rest/policies_test.go(0 hunks)shared/management/client/rest/posturechecks_test.go(0 hunks)shared/management/client/rest/routes_test.go(0 hunks)shared/management/client/rest/setupkeys_test.go(0 hunks)shared/management/client/rest/tokens_test.go(0 hunks)shared/management/client/rest/users_test.go(0 hunks)shared/relay/client/dialer/quic/conn.go(1 hunks)signal/cmd/run.go(2 hunks)util/syslog_nonwindows.go(0 hunks)
💤 Files with no reviewable changes (33)
- management/server/http/testing/integration/setupkeys_handler_integration_test.go
- shared/management/client/rest/geo_test.go
- shared/management/client/rest/tokens_test.go
- shared/management/client/rest/events_test.go
- shared/management/client/rest/users_test.go
- shared/management/client/rest/groups_test.go
- client/iface/device/device_ios.go
- client/internal/routemanager/iface/iface.go
- shared/management/client/rest/setupkeys_test.go
- shared/management/client/rest/impersonation_test.go
- shared/management/client/rest/dns_test.go
- util/syslog_nonwindows.go
- client/ssh/lookup_darwin.go
- shared/management/client/rest/client_test.go
- client/system/info_ios.go
- client/cmd/pprof.go
- shared/management/client/rest/peers_test.go
- relay/cmd/pprof.go
- management/server/testutil/store_ios.go
- client/server/panic_windows.go
- shared/management/client/rest/accounts_test.go
- shared/management/client/rest/routes_test.go
- management/server/http/testing/benchmarks/setupkeys_handler_benchmark_test.go
- shared/management/client/rest/posturechecks_test.go
- management/server/testutil/store.go
- management/server/http/testing/benchmarks/users_handler_benchmark_test.go
- management/server/http/testing/benchmarks/peers_handler_benchmark_test.go
- shared/management/client/rest/networks_test.go
- client/system/info_android.go
- client/ssh/lookup.go
- client/system/info_darwin.go
- client/internal/iface.go
- shared/management/client/rest/policies_test.go
🧰 Additional context used
🧬 Code graph analysis (11)
management/server/posture_checks.go (1)
shared/management/status/error.go (3)
Errorf(67-72)InvalidArgument(27-27)Error(51-54)
management/server/account.go (1)
management/server/idp/idp.go (1)
AppMetadata(95-101)
relay/server/listener/quic/conn.go (1)
shared/relay/client/dialer/quic/conn.go (2)
Conn(32-35)NewConn(37-42)
shared/management/client/rest/client.go (1)
shared/management/status/error.go (1)
Errorf(67-72)
management/server/idp/pocketid.go (1)
shared/management/status/error.go (1)
Errorf(67-72)
management/server/http/handlers/policies/posture_checks_handler_test.go (1)
shared/management/status/error.go (3)
Errorf(67-72)InvalidArgument(27-27)Error(51-54)
management/server/user.go (2)
management/server/context/auth.go (1)
UserAuth(16-37)management/server/context/keys.go (1)
PeerIDKey(9-9)
management/server/store/sql_store_test.go (1)
route/route.go (5)
Route(91-112)ID(48-48)NetID(54-54)NetworkType(60-60)IPv4Network(40-40)
shared/relay/client/dialer/quic/conn.go (1)
relay/server/listener/quic/conn.go (2)
Conn(14-20)NewConn(22-29)
client/internal/routemanager/systemops/systemops_generic.go (1)
client/internal/routemanager/vars/vars.go (2)
Defaultv4(14-14)Defaultv6(15-15)
signal/cmd/run.go (1)
util/wsproxy/constants.go (2)
ProxyPath(6-6)SignalComponent(11-11)
🔇 Additional comments (45)
relay/server/listener/ws/conn.go (1)
87-92: LGTM! Clean use of method promotion.Using
c.CloseNow()instead ofc.Conn.CloseNow()is more idiomatic Go, leveraging the promoted method from the embedded*websocket.Conn. The behavior remains identical.client/firewall/uspfilter/nat_test.go (1)
236-241: LGTM! Clean refactor to switch statement.The conversion from if-else to switch improves readability and follows Go idioms. ICMP correctly falls through without port validation since it doesn't use ports.
management/cmd/management.go (1)
66-66: Good use of zero-value declaration.Using
var tlsEnabled boolinstead of explicitfalseinitialization follows Go conventions for zero-value declarations. The behavior remains identical.client/internal/dns/upstream.go (1)
200-200: LGTM! Good defensive programming.Changing from
WarnftoWarnis correct here sincetimeoutMsgis already fully formatted viafmt.Sprintfand concatenation. UsingWarnfwould incorrectly treat the pre-built string as a format template, potentially causing issues if the error message contains%characters.signal/cmd/run.go (2)
76-76: LGTM!The change from explicit initialization to zero-value initialization is clean and idiomatic. The behavior remains identical since
bool's zero value isfalse.
262-267: LGTM!The refactor from a conditional switch (
switch { case condition }) to a direct value switch (switch value { case match }) is cleaner and more idiomatic. The routing logic remains unchanged and correctly handles the WebSocket proxy path.management/server/idp/zitadel.go (1)
360-360: LGTM: Idiomatic Go variable declaration.The change to use short variable declaration (
pending := true) is the idiomatic Go approach and improves code consistency.management/server/idp/pocketid.go (2)
124-124: LGTM: Follows Go error message conventions.Lowercasing "body" aligns with Go's style guideline that error strings should not be capitalized (unless starting with proper nouns or acronyms).
304-304: LGTM: Idiomatic Go variable declaration.Consistent with the change in
zitadel.go, using short variable declaration is the preferred Go style.client/internal/debug/debug_linux.go (1)
509-518: LGTM! Clean refactor consolidating duplicate logic.The switch statements effectively consolidate the handling of 4-byte and 2-byte payload lengths, which previously required separate branches. The absence of a default case is correct here—unhandled lengths fall through to the generic formatter at lines 521-523.
client/firewall/uspfilter/localip.go (1)
133-133: LGTM: Clear suppression directive with proper justification.The
#nosec G602directive appropriately suppresses a false positive from gosec, and the accompanying comment clearly documents why the array access is safe (the bitmap has 8192 elements, and the loop indexiranges from 0 to 8191).client/firewall/uspfilter/localip_test.go (1)
220-221: LGTM: Simplified map access syntax.The change from two-blank identifier assignment to single-blank identifier aligns with the
gosimplelinter recommendation. Both forms achieve the same benchmarking goal (triggering map access without using the result), but the single-value form is more idiomatic when the existence check isn't needed.Also applies to: 229-230
management/server/user.go (3)
14-14: LGTM! Import alias standardized to lowercase.The rename from
nbContexttonbcontextfollows Go's idiomatic convention of using lowercase package aliases.
179-179: LGTM! Function signature updated consistently.The parameter type correctly reflects the updated import alias.
942-942: LGTM! Context key reference updated consistently.The context key reference correctly uses the updated import alias. The pre-existing
nolint:staticcheckcomment indicates this usage pattern has been previously reviewed.management/server/store/sql_store_test.go (1)
32-32: LGTM! Clean package refactoring from route2 to nbroute.The changes consistently update route types across all test cases. Type conversions using
nbroute.ID()andnbroute.NetID()are correct, and the struct field assignments match thenbroute.Routedefinition. The embedded struct inTestMigrateis properly updated, and all route initializations use the correct types and constants.Also applies to: 111-116, 689-699, 715-719, 3413-3425
client/firewall/iptables/manager_linux_test.go (1)
163-163: LGTM! Idiomatic type inference.Removing the explicit
inttype declaration is more idiomatic Go when the type can be inferred from the literal values.client/firewall/iptables/acl_linux.go (1)
371-371: LGTM! Cleaner boolean initialization.The direct boolean expression is more concise and idiomatic than a two-step conditional. Functionally equivalent to the previous implementation.
client/firewall/nftables/manager_linux_test.go (2)
200-200: LGTM! Idiomatic type inference.Removing the explicit
inttype declaration is more idiomatic Go when the type can be inferred from the literal values.
210-232: LGTM! Switch statements improve readability.The refactor from if-else chains to switch statements is more idiomatic Go for checking distinct values. This improves readability without changing behavior.
management/server/nameserver.go (2)
317-317: LGTM: Error return updated consistently.The return statement correctly uses the renamed error variable
errInvalidDomainName.
23-23: LGTM: Error variable rename is complete and consistent.The rename from
invalidDomainNametoerrInvalidDomainNamehas been properly applied throughout the codebase. Verification confirms:
- All references to the old name have been removed
- The new variable is defined at line 23 and used at line 317
- No cross-file dependencies or external references exist
- The change follows Go naming conventions for package-level error variables
client/internal/routemanager/systemops/systemops_generic.go (2)
213-251: LGTM: Clean refactor to switch statement.The refactoring from if/else to a switch statement improves readability while preserving the original logic. The fall-through to line 251 correctly handles non-default prefixes.
288-290: Confirmed behavioral fix with symmetric caller patterns.Verification shows
genericRemoveVPNRouteandgenericAddVPNRouteare called symmetrically (systemops_linux.go and systemops_nonlinux.go), confirming the new default case correctly mirrors the add function's behavior for non-default prefixes. This fixes an asymmetry where added routes couldn't be removed.The change is safe with low risk, but should be documented in the PR description since it's a behavioral fix unrelated to the Go/QUIC version updates.
shared/relay/client/dialer/quic/conn.go (2)
69-72: Good defensive programming with nil check.The nil check for the session pointer is a good practice, especially when working with pointer types. This protects against potential nil pointer dereferences.
33-33: All call sites ofNewConnpass the correct type*quic.Conn.Verification confirmed: The only call site is at
shared/relay/client/dialer/quic/quic.go:69, wheresessionis the result ofquic.Dial()which returns*quic.Conn. The function signature and all invocations are correctly aligned with the type change.relay/server/listener/quic/conn.go (1)
15-15: All callers correctly pass*quic.ConntoNewConn.Verification confirms the API change is properly implemented: the quic-go library's
Accept()method returns(*Conn, error), which is*quic.Conn. The call site at line 52 in listener.go passessessionfromAccept(), which has the correct type. No breaking changes or type mismatches were found..github/workflows/golang-test-darwin.yml (1)
21-21: LGTM! Go version update aligns with PR objectives.The Go toolchain update to 1.25.x is consistent with the PR goal of bumping Go to 1.25.3.
.github/workflows/golang-test-windows.yml (1)
27-27: LGTM! Consistent Go version update.The update to Go 1.25.x aligns with the PR objectives and matches updates across other CI workflows.
.github/workflows/test-infrastructure-files.yml (1)
73-73: LGTM! Go version update is correct.The toolchain update to 1.25.x is consistent with the broader PR changes.
.github/workflows/release.yml (1)
43-43: LGTM!Go version upgrade to 1.25 is consistent with the PR objectives and applied uniformly across all release jobs.
shared/management/client/rest/client.go (1)
160-160: LGTM!Lowercasing error messages follows Go convention that error strings should not be capitalized unless they begin with proper nouns or acronyms.
.github/workflows/mobile-build-validation.yml (1)
23-23: LGTM!Go and gomobile version updates are consistent with the PR objectives and applied uniformly across both Android and iOS build jobs.
.github/workflows/golang-test-freebsd.yml (1)
28-28: LGTM!FreeBSD Go toolchain update to 1.25.3 is consistent with the PR objectives and properly updates both the tarball name and download URL.
.github/workflows/golangci-lint.yml (2)
49-49: LGTM! Go version bump aligns with PR objectives.The upgrade to Go 1.25.x is consistent with the PR's stated goal of bumping Go to 1.25.3.
55-61: No .golangci.yaml file exists in the repository; cache configuration is redundant but valid.The primary verification concern cannot be addressed because no
.golangci.yamlconfiguration file exists in the codebase. The review assumes schema v2 compatibility must be verified, but there is no config file present.The cache-disabling flags are redundantly configured:
skip-cache: truealone completely disables caching and makesskip-save-cache: trueandcache-invalidation-interval: 0unnecessary. However, the configuration is syntactically valid and consistent across multiple workflows (golangci-lint.ymlandwasm-build-validation.yml), suggesting this is an intentional design choice rather than an error.Likely an incorrect or invalid review comment.
.github/workflows/golang-test-linux.yml (1)
33-33: LGTM! Consistent Go version updates across all jobs.All jobs have been updated to use Go 1.25.x consistently, including the Docker image reference. This ensures uniform toolchain behavior across the entire test matrix.
Also applies to: 112-112, 157-157, 203-203, 226-226, 276-276, 327-327, 414-414, 503-503, 567-567
management/server/http/handlers/policies/posture_checks_handler_test.go (1)
48-49: LGTM! Improved error formatting security.The change from
status.Errorf(status.InvalidArgument, err.Error())tostatus.Errorf(status.InvalidArgument, "%v", err.Error())is a security improvement that prevents potential format string vulnerabilities. The error message is now properly escaped as an argument rather than being used directly as a format string.client/ui/client_ui.go (1)
472-473: LGTM! Improved error message clarity.The error message change from "Update settings are disabled by daemon" to "Configuration updates are disabled by daemon" provides better clarity to users.
.golangci.yaml (1)
1-124: LGTM! Successful migration to golangci-lint v2 schema.The configuration has been comprehensively updated to the v2 schema, which aligns with the golangci-lint v8 upgrade in the CI workflows. Key improvements:
- Explicit
version: "2"declaration- Clearer structure with consolidated
enablelist- Centralized per-linter settings under
settingssection- New
exclusionsandformatterssections for better organization- Maintains equivalent linting behavior with improved readability
This modernization should provide better maintainability and compatibility with the latest golangci-lint tooling.
.github/workflows/wasm-build-validation.yml (1)
17-19: LGTM! Consistent WASM workflow updates.The changes align with the broader CI modernization:
- Go 1.25.x upgrade matches other workflows
- golangci-lint v8 upgrade with same cache configuration
- Explicit
GOOS=jsandGOARCH=wasmenvironment variables improve clarityThe workflow maintains its WASM-specific functionality while adopting the project-wide toolchain updates.
Also applies to: 26-26, 30-37, 49-49
go.mod (3)
3-5: LGTM! Go toolchain upgrade aligns with PR objectives.The upgrade from Go 1.23 to 1.25 with toolchain go1.25.3 matches the PR's stated goal of bumping Go to 1.25.3.
81-81: LGTM! QUIC version bump matches PR objectives.The update of
github.com/quic-go/quic-gofrom an earlier version to v0.55.0 aligns with the PR's stated goal of bumping QUIC to 0.55.0.
22-28: Dependency update compatibility verified; runtime testing recommended.The Go version requirement (Go 1.25) satisfies all updated dependency minimums:
- gRPC v1.76.0 requires Go 1.24+
- OpenTelemetry v1.37.0 requires Go 1.23+
- Protobuf v1.36.10 has no breaking changes
Static analysis found:
- No removed internal otel packages used in the codebase
- No direct semantic convention constant usage that would be affected
- Existing gRPC and protobuf API usage patterns are compatible
go mod verifyconfirms dependency integrityHowever, runtime validation (test suite execution) was not performed due to sandbox environment limitations. Before merging, ensure the full test suite passes locally with these dependency versions.
management/server/migration/migration.go (1)
396-396: Based on the verification through the executed script, the codebase successfully usesdb.Name()at lines 22 and 396 in migration.go with GORM v1.25.12. The script output shows no errors, indicating the API is valid and working correctly.API change is correct and verified.
The change from
db.Dialector.Name()todb.Name()is a valid API update for GORM 1.25.12. The method works as expected for dialect detection in database-specific index creation.
| func validatePostureChecks(ctx context.Context, transaction store.Store, accountID string, postureChecks *posture.Checks) error { | ||
| if err := postureChecks.Validate(); err != nil { | ||
| return status.Errorf(status.InvalidArgument, err.Error()) //nolint | ||
| return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint |
There was a problem hiding this comment.
🧩 Analysis chain
Review error formatting change.
The change from err.Error() to "%v", err.Error() appears redundant. Consider one of these approaches:
- Use
err.Error()directly (original approach) - Use
"%v", errto preserve error formatting - Use
"%w", errif error wrapping is needed
The current "%v", err.Error() converts the error to a string twice, which is unnecessary.
Additionally, the //nolint directive should specify which rule is being suppressed.
Consider this refactor:
- return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint
+ return status.Errorf(status.InvalidArgument, "%v", err)Or if a specific format is required:
- return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint
+ return status.Errorf(status.InvalidArgument, err.Error()) // nolint:goerr113 // validation error needs dynamic messageI'll examine the file and verify the review comment concerns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the file around line 186 for context
if [ -f "management/server/posture_checks.go" ]; then
echo "=== File context around line 186 ==="
sed -n '180,192p' management/server/posture_checks.go
echo ""
echo "=== Full function context (broader range) ==="
sed -n '175,200p' management/server/posture_checks.go
fi
Length of output: 1483
🏁 Script executed:
#!/bin/bash
# Find the imports in the file to understand status package origin
if [ -f "management/server/posture_checks.go" ]; then
echo "=== Imports in posture_checks.go ==="
head -50 management/server/posture_checks.go | grep -E "^import|status|err"
fiLength of output: 900
🏁 Script executed:
#!/bin/bash
# Search for status.Errorf usage patterns in the codebase
echo "=== Other status.Errorf patterns in the file ==="
rg -n "status\.Errorf" management/server/posture_checks.go -A1 -B1Length of output: 593
🏁 Script executed:
#!/bin/bash
# Search for the nolint directive patterns
echo "=== nolint directives in the file ==="
rg -n "//nolint" management/server/posture_checks.go -B2 -A1Length of output: 311
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the status.Errorf function definition
echo "=== Searching for status.Errorf definition ==="
find . -name "*.go" -path "*/management/status/*" -type f | head -20
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Look for the status package definition
echo "=== Finding status package files ==="
rg -l "func Errorf" --type=go -A5Length of output: 70
🏁 Script executed:
#!/bin/bash
# Check what fmt.Sprintf does with "%v" and error values
echo "=== Checking how Go fmt package handles errors ==="
rg -n "Errorf.*err" shared/management/status/ -A2 -B2 --type=go 2>/dev/null | head -40Length of output: 1446
Correct the redundant error formatting and specify the nolint rule.
The review comment is technically sound. The code calls err.Error() to convert the error to a string, then passes it to "%v" which converts it back to a formatted string—unnecessary double conversion. Additionally, the codebase consistently passes errors directly to status.Errorf() (see lines 142, 167, 211 in shared/management/status/error.go), where fmt.Sprintf handles error formatting automatically.
Recommended fix:
- return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint
+ return status.Errorf(status.InvalidArgument, "%v", err)Alternatively, align with the existing pattern in the status package:
- return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint
+ return status.Errorf(status.InvalidArgument, "%s", err) // nolint:goerr113📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint | |
| return status.Errorf(status.InvalidArgument, "%v", err) |
| return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint | |
| return status.Errorf(status.InvalidArgument, "%s", err) // nolint:goerr113 |
🤖 Prompt for AI Agents
In management/server/posture_checks.go around line 186, remove the redundant
err.Error() formatting and pass the error value directly to status.Errorf, e.g.
replace status.Errorf(status.InvalidArgument, "%v", err.Error()) with
status.Errorf(status.InvalidArgument, "%v", err), and update the nolint comment
to specify the lint rule being suppressed (e.g. // nolint:staticcheck) instead
of a bare //nolint.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
management/server/posture_checks.go (1)
189-189: Redundant error formatting (previously flagged).This issue was already identified in a previous review: calling
err.Error()and then passing it to"%v"is a double conversion. Additionally, the//nolintdirective should specify the suppressed rule.Consider the previously suggested fix:
- return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint + return status.Errorf(status.InvalidArgument, "%v", err)Or with explicit nolint rule:
- return status.Errorf(status.InvalidArgument, "%v", err.Error()) //nolint + return status.Errorf(status.InvalidArgument, "%s", err) // nolint:goerr113
🧹 Nitpick comments (2)
management/server/account.go (1)
1034-1034: Consider using a specific lint suppression or simplifying the expression.The change from
//nolint:gosimpleto//nolintmakes the suppression overly broad, disabling all linters for this line. Consider either:
- Keeping the specific suppression:
//nolint:gosimple- Or simplifying the expression to idiomatic Go:
loggedInOnce(removing== true)Apply this diff to simplify the expression:
- if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce == true { //nolint + if datum.AppMetadata.WTPendingInvite != nil && *datum.AppMetadata.WTPendingInvite && loggedInOnce {go.mod (1)
99-103: Verify extensive indirect dependency updates are compatible.The go.mod shows substantial updates to transitive dependencies, particularly:
- OpenTelemetry ecosystem (otel v1.37.0, otelgrpc v0.61.0, otel/metric, otel/sdk/metric)
- Google Cloud libraries (cloud.google.com/go/auth, googleapis, gax-go)
- API surface shifts mentioned in the AI summary
These indirect dependency changes could cascade through the codebase. Ensure integration tests have been run to detect compatibility issues early.
Have you run the full integration test suite against these dependency updates? Consider documenting any API migration notes or deprecation warnings in a PR comment or changelog.
Also applies to: 240-242, 249-249
📜 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 (12)
client/cmd/login.go(2 hunks)client/cmd/up.go(1 hunks)client/firewall/iptables/manager_linux_test.go(1 hunks)client/firewall/nftables/manager_linux_test.go(3 hunks)client/firewall/uspfilter/filter.go(1 hunks)client/firewall/uspfilter/nat_test.go(1 hunks)client/ui/client_ui.go(5 hunks)go.mod(7 hunks)management/server/account.go(1 hunks)management/server/nameserver.go(2 hunks)management/server/posture_checks.go(1 hunks)management/server/user.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/firewall/uspfilter/filter.go
🚧 Files skipped from review as they are similar to previous changes (4)
- client/firewall/uspfilter/nat_test.go
- client/ui/client_ui.go
- client/cmd/up.go
- client/cmd/login.go
🧰 Additional context used
🧬 Code graph analysis (3)
management/server/account.go (1)
management/server/idp/idp.go (1)
AppMetadata(95-101)
management/server/posture_checks.go (1)
shared/management/status/error.go (3)
Errorf(70-75)InvalidArgument(27-27)Error(54-57)
management/server/user.go (2)
management/server/context/auth.go (1)
UserAuth(16-37)management/server/context/keys.go (1)
PeerIDKey(9-9)
⏰ 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: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: release_ui
- GitHub Check: test-docker-compose (postgres)
- GitHub Check: Client / Unit
- GitHub Check: test-docker-compose (sqlite)
- GitHub Check: Android / Build
- GitHub Check: test-docker-compose (mysql)
- GitHub Check: JS / Lint
- GitHub Check: iOS / Build
- GitHub Check: test-getting-started-script
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
🔇 Additional comments (13)
management/server/nameserver.go (2)
23-23: LGTM! Error variable naming follows Go conventions.The rename from
invalidDomainNametoerrInvalidDomainNamealigns with Go best practices for package-level error variable naming, where unexported errors are prefixed witherr.
326-326: LGTM! Error variable usage is consistent.The updated reference to
errInvalidDomainNameis consistent with the renamed error variable declaration, maintaining correct error handling behavior.client/firewall/iptables/manager_linux_test.go (1)
164-164: LGTM! Idiomatic type inference.The removal of the explicit
inttype annotation is a good refactor. Type inference makes the code more concise while maintaining clarity, as the type is obvious from the literal values.client/firewall/nftables/manager_linux_test.go (3)
201-201: LGTM! Idiomatic type inference.The removal of the explicit
inttype annotation improves code conciseness while maintaining clarity.
211-217: LGTM! Switch statement improves readability.The refactor from if-else to switch for checking
lookup.SetNameis more idiomatic and clearer when comparing a single variable against multiple discrete values.
227-233: LGTM! Switch statement improves readability.The refactor from if-else to switch for checking
verdict.Kindis more idiomatic and improves maintainability.management/server/user.go (4)
14-14: LGTM: Import alias refactoring.The import alias change from
nbContexttonbcontextfollows Go naming conventions and improves consistency.
179-179: LGTM: Function signature updated consistently.The parameter type correctly reflects the import alias change, maintaining consistency with the refactoring.
972-972: LGTM: Context key reference updated correctly.The context key usage is updated consistently with the import alias change. The
nolint:staticcheckdirective on Line 971 appropriately suppresses warnings for this intentional context value usage.
1207-1207: LGTM: Parameter type updated consistently.The function signature correctly uses the new
nbcontextalias, maintaining consistency with the refactoring.go.mod (3)
3-3: Go 1.25.3 is production-ready and released.Go 1.25.3 was released on October 13, 2025 and is a production-ready patch. The version bump is valid for merging. Optionally note that the current stable version is Go 1.25.4, should you prefer to use the latest patch.
1-30: Build tag migration completed; QUIC and OpenTelemetry implementations appear consistent.Verification results:
- No legacy
// +buildtags remain across 329+ files using//go:buildsyntax- Both QUIC
Connimplementations (relay/server/listener/quic/conn.go and shared/relay/client/dialer/quic/conn.go) have stable method signatures implementing the standardnet.Conninterface with consistent patterns- OpenTelemetry v1.37.0 is consistently versioned across all dependencies, with 22 files importing packages
The code state shows no apparent signature mismatches or incompatibilities in reviewed sections. However, comprehensive verification of all call sites throughout the codebase and test execution results cannot be confirmed through inspection alone. Ensure unit and integration tests pass before merging.
82-82: No API compatibility issues with quic-go v0.55.0.The codebase uses datagram-based communication (SendDatagram/ReceiveDatagram) and connection lifecycle management (DialAddr, ListenAddr, Accept, Close). The breaking changes in v0.55.0 affect stream operations (OpenStream/OpenUniStream) and ClientHelloInfo renaming, neither of which are used in this codebase. All five QUIC-related files use APIs that remain compatible.
|
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
Chores
Style
Tests
Bug Fixes