diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index 7b7b32ec0..7842530ce 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -35,7 +35,7 @@ jobs: display_name: Linux name: ${{ matrix.display_name }} runs-on: ${{ matrix.os }} - timeout-minutes: 15 + timeout-minutes: 25 steps: - name: Checkout code uses: actions/checkout@v4 @@ -58,4 +58,4 @@ jobs: skip-cache: true skip-save-cache: true cache-invalidation-interval: 0 - args: --timeout=12m + args: --timeout=20m diff --git a/client/internal/engine.go b/client/internal/engine.go index cf77d6cf9..d44e16619 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -206,7 +206,7 @@ type Engine struct { // latestComponents is the most-recent NetworkMapComponents decoded from // a NetworkMapEnvelope (capability=3 peers only). Held alongside the - // NetworkMap that Calculate() produced from it so Step 3 incremental + // NetworkMap that Calculate() produced from it so future incremental // updates have a base to apply changes against. nil for legacy-format // peers. Guarded by syncMsgMux. latestComponents *types.NetworkMapComponents @@ -928,8 +928,8 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error { // Components-format peer: decode the envelope back to typed // components, run Calculate() locally, and convert to the wire // NetworkMap shape the rest of the engine consumes. Components are - // retained so future incremental updates (Step 3) can apply deltas - // instead of doing a full reconstruction. + // retained so future incremental updates can apply deltas instead + // of doing a full reconstruction. localKey := e.config.WgPrivateKey.PublicKey().String() dnsName := "" if pc := update.GetPeerConfig(); pc != nil { @@ -953,7 +953,7 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error { // Only retain the components view when the server sent the envelope // path. A legacy proto.NetworkMap means components == nil; writing it - // here would clobber a previously-cached snapshot, breaking the Step 3 + // here would clobber a previously-cached snapshot, breaking the // incremental-delta base on a future envelope sync. if components != nil { e.latestComponents = components diff --git a/idp/dex/sqlite_cgo.go b/idp/dex/sqlite_cgo.go index 6a5986102..5de66f647 100644 --- a/idp/dex/sqlite_cgo.go +++ b/idp/dex/sqlite_cgo.go @@ -7,14 +7,9 @@ import ( ) // newSQLite3 builds the dex SQLite3 config. CGO builds use the upstream -// struct that takes a File path. Non-CGO builds (see sqlite_nocgo.go) build -// the empty stub — `Open()` then returns the dex "SQLite not available" -// error, which is correct behaviour for binaries that can't link sqlite3. -// -// Wrapping the construction this way keeps the call sites in config.go / -// provider.go compilable under both build modes — the upstream stub for -// !cgo has no File field, so a bare `sql.SQLite3{File: file}` literal fails -// the cross-compile for targets like linux/arm/v6 in the release pipeline. +// struct that takes a File path. Non-CGO builds get an empty stub whose +// Open() returns the dex "SQLite not available" error — correct behaviour +// for binaries that can't link sqlite3 (e.g. cross-compiled ARM targets). func newSQLite3(file string) *sql.SQLite3 { return &sql.SQLite3{File: file} } diff --git a/idp/dex/sqlite_nocgo.go b/idp/dex/sqlite_nocgo.go index 1820a8e40..4def12143 100644 --- a/idp/dex/sqlite_nocgo.go +++ b/idp/dex/sqlite_nocgo.go @@ -7,12 +7,9 @@ import ( ) // newSQLite3 for non-CGO builds. The dex SQLite3 stub has no fields and its -// Open() returns an error documenting the missing CGO support, which is the -// right behaviour for the cross-compiled artefacts (linux/arm, wasm, etc.) -// — those targets never actually run the embedded IdP. -// -// The `file` argument is ignored intentionally; keeping the signature -// matched with sqlite_cgo.go lets the call sites stay identical. +// Open() returns an error documenting the missing CGO support — correct +// behaviour for cross-compiled artefacts that never actually run the +// embedded IdP. The `file` argument is ignored. func newSQLite3(_ string) *sql.SQLite3 { return &sql.SQLite3{} } diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index 938df5539..1eae5f111 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -56,13 +56,10 @@ type Controller struct { integratedPeerValidator integrated_validator.IntegratedValidator - // componentsDisabled is the kill switch for the component-based wire - // format. When true the controller emits legacy proto.NetworkMap to every - // peer regardless of capability — used to roll back instantly via a - // management restart from a bad components encoder. - // - // Set once in NewController from NB_NETWORK_MAP_COMPONENTS_DISABLE and - // never written after — readers race-free without a mutex. + // componentsDisabled, when true, forces the controller to emit legacy + // proto.NetworkMap to every peer regardless of capability. Set once at + // construction and never written after — readers race-free without a + // mutex. componentsDisabled bool } @@ -98,17 +95,14 @@ func NewController(ctx context.Context, store store.Store, metrics telemetry.App } // PeerNeedsComponents reports whether the gRPC layer should emit the -// component-based wire format for this peer. Combines the peer's advertised -// capability with the controller-level kill switch — callers ask exactly -// this question, so encapsulating it removes accidental double-checks. +// component-based wire format for this peer. func (c *Controller) PeerNeedsComponents(p *nbpeer.Peer) bool { return p != nil && p.SupportsComponentNetworkMap() && !c.componentsDisabled } // parseBoolEnv reads an env var via strconv.ParseBool so callers accept the // usual "1/t/T/TRUE/true/True" set instead of being strict about a single -// literal — matches the convention used elsewhere in the codebase -// (e.g. event.go's NB_TRAFFIC_EVENT_*) and reduces operator surprises. +// literal. func parseBoolEnv(key string) bool { v, _ := strconv.ParseBool(os.Getenv(key)) return v @@ -420,9 +414,8 @@ func (c *Controller) BufferUpdateAccountPeers(ctx context.Context, accountID str // GetValidatedPeerWithMap. It returns raw NetworkMapComponents for capable // peers along with the proxy NetworkMap fragment (BYOP / port-forwarding // data the legacy server folds in via NetworkMap.Merge). The gRPC layer -// encodes both into the wire envelope. The caller is responsible for -// checking peer capability + componentsDisabled before dispatching here — -// this method does NOT branch on capability itself. +// encodes both into the wire envelope. Callers must gate on capability +// themselves before dispatching here — this method does NOT branch on it. func (c *Controller) GetValidatedPeerWithComponents(ctx context.Context, isRequiresApproval bool, accountID string, peer *nbpeer.Peer) (*nbpeer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*posture.Checks, int64, error) { if isRequiresApproval { network, err := c.repo.GetAccountNetwork(ctx, accountID) diff --git a/management/internals/shared/grpc/components_encoder.go b/management/internals/shared/grpc/components_encoder.go index af91b008c..2164586f6 100644 --- a/management/internals/shared/grpc/components_encoder.go +++ b/management/internals/shared/grpc/components_encoder.go @@ -18,9 +18,9 @@ import ( const wgKeyRawLen = 32 // ComponentsEnvelopeInput bundles the data the component-format encoder needs. -// In Step 2 the envelope is fully self-contained — every field needed by the -// client's local Calculate() comes from the components struct itself. The -// only externally-supplied data is the receiving peer's PeerConfig (which is +// The envelope is fully self-contained — every field needed by the client's +// local Calculate() comes from the components struct itself. The only +// externally-supplied data is the receiving peer's PeerConfig (which is // computed alongside the components in the network_map controller and reused // from the legacy proto path) and the dns_domain string. type ComponentsEnvelopeInput struct { @@ -48,21 +48,19 @@ type ComponentsEnvelopeInput struct { // not byte-equal. // // Callers must NOT concatenate or merge envelopes from different encodes — -// index spaces are local to a single envelope. Delta sync (Step 3+) will -// use a different shape for the same reason. +// index spaces are local to a single envelope. func EncodeNetworkMapEnvelope(in ComponentsEnvelopeInput) *proto.NetworkMapEnvelope { c := in.Components // Graceful degrade when components is nil — matches the legacy path's - // account_components.go:43 behaviour for missing/unvalidated peers - // (return a NetworkMap with only Network populated). The receiver gets - // an envelope it can decode without crashing; AccountSettings stays - // non-nil so client-side dereferences are safe. + // behaviour for missing/unvalidated peers (return a NetworkMap with only + // Network populated). The receiver gets an envelope it can decode + // without crashing; AccountSettings stays non-nil so client-side + // dereferences are safe. if c == nil { // Match legacy missing-peer minimum: a NetworkMap with only Network - // populated (account_components.go:43). The receiver gets enough to - // bootstrap (Network identifier, dns_domain, account_settings) and - // nothing else. + // populated. The receiver gets enough to bootstrap (Network + // identifier, dns_domain, account_settings) and nothing else. return &proto.NetworkMapEnvelope{ Payload: &proto.NetworkMapEnvelope_Full{ Full: &proto.NetworkMapComponentsFull{ @@ -129,8 +127,8 @@ func EncodeNetworkMapEnvelope(in ComponentsEnvelopeInput) *proto.NetworkMapEnvel } // networkSerial returns c.Network.CurrentSerial() with a nil guard. The -// production path always populates c.Network (account_components.go:86), but -// the encoder is exported and a hand-built components struct may omit it. +// production path always populates c.Network, but the encoder is exported +// and a hand-built components struct may omit it. func networkSerial(n *types.Network) uint64 { if n == nil { return 0 diff --git a/management/internals/shared/grpc/components_encoder_test.go b/management/internals/shared/grpc/components_encoder_test.go index de859b100..e42675b03 100644 --- a/management/internals/shared/grpc/components_encoder_test.go +++ b/management/internals/shared/grpc/components_encoder_test.go @@ -814,8 +814,8 @@ func TestToProxyPatch_PopulatesAllFields(t *testing.T) { // TestEncodeNetworkMapEnvelope_ProxyPatchPropagated covers the ProxyPatch // pass-through in both encoder branches (normal path + nil-Components -// graceful-degrade). Without this test a regression that drops `ProxyPatch:` -// from one of the struct literals in components_encoder.go would slip past CI. +// graceful-degrade). Guards against a regression that drops `ProxyPatch:` +// from one of the envelope struct literals. func TestEncodeNetworkMapEnvelope_ProxyPatchPropagated(t *testing.T) { patch := &proto.ProxyPatch{ ForwardingRules: []*proto.ForwardingRule{{ @@ -850,7 +850,7 @@ func TestEncodeNetworkMapEnvelope_ProxyPatchPropagated(t *testing.T) { func TestEncodeNetworkMapEnvelope_NilComponentsGracefulDegrade(t *testing.T) { // nil Components → minimal envelope, no crash. Matches the legacy - // account_components.go:43 behaviour for missing/unvalidated peers. + // behaviour for missing/unvalidated peers. env := EncodeNetworkMapEnvelope(ComponentsEnvelopeInput{ Components: nil, DNSDomain: "netbird.cloud", diff --git a/management/internals/shared/grpc/components_envelope_response.go b/management/internals/shared/grpc/components_envelope_response.go index 5a6f8d066..edb236e7e 100644 --- a/management/internals/shared/grpc/components_envelope_response.go +++ b/management/internals/shared/grpc/components_envelope_response.go @@ -128,8 +128,7 @@ func toProxyPatch(nm *types.NetworkMap, dnsName string, includeIPv6, useSourcePr // that's the destination of an SSH-enabling policy without having // peer.SSHEnabled set locally. // -// Mirrors the two activation paths in Calculate() (`networkmap_components.go` -// `getPeerConnectionResources`): +// Mirrors the two activation paths Calculate() uses: // 1. Explicit: rule.Protocol == NetbirdSSH and peer is in the rule's // destinations. // 2. Legacy implicit: rule covers TCP/22 or TCP/22022 (or ALL), peer is in diff --git a/management/internals/shared/grpc/components_envelope_response_test.go b/management/internals/shared/grpc/components_envelope_response_test.go index dfb6b5734..bf35bb7b9 100644 --- a/management/internals/shared/grpc/components_envelope_response_test.go +++ b/management/internals/shared/grpc/components_envelope_response_test.go @@ -12,9 +12,7 @@ import ( // TestComputeSSHEnabledForPeer covers both Calculate-mirroring branches: // explicit NetbirdSSH protocol, and the legacy implicit case where a // TCP/22 (or 22022 / ALL / port-range-covering-22) rule activates SSH when -// the destination peer has SSHEnabled=true locally. Belt-and-suspenders for -// the B1 fix that the prod-DB equivalence test alone wouldn't have caught -// if no account had this combination. +// the destination peer has SSHEnabled=true locally. func TestComputeSSHEnabledForPeer(t *testing.T) { const targetPeerID = "target" const targetGroupID = "g_dst" diff --git a/management/internals/shared/grpc/server.go b/management/internals/shared/grpc/server.go index 27d77beb4..a58c83497 100644 --- a/management/internals/shared/grpc/server.go +++ b/management/internals/shared/grpc/server.go @@ -943,11 +943,11 @@ func (s *Server) sendInitialSync(ctx context.Context, peerKey wgtypes.Key, peer // (network_map.Controller.UpdateAccountPeers) skips this duplication // because it dispatches by capability before computing. // - // TODO(step-4-sync): refactor SyncPeer / SyncAndMarkPeer / their - // mocks + manager interfaces to return PeerNetworkMapResult so the - // initial-sync path stops doing duplicate work. ~13 files of churn, - // deferred until the client-side decoder lands and there's a real - // deployment of capability=3 peers worth optimizing for. + // TODO: refactor SyncPeer / SyncAndMarkPeer / their mocks + manager + // interfaces to return PeerNetworkMapResult so the initial-sync path + // stops doing duplicate work. Deferred until the client-side + // decoder lands and there's a real deployment of capability=3 peers + // worth optimizing for. _, components, proxyPatch, _, _, err := s.networkMapController.GetValidatedPeerWithComponents(ctx, false, peer.AccountID, peer) if err != nil { log.WithContext(ctx).Errorf("failed to build components for peer %s on initial sync: %v", peer.ID, err) diff --git a/management/server/types/networkmap_wire_benchmark_test.go b/management/server/types/networkmap_wire_benchmark_test.go index 43c9e1fbf..087b1cbfa 100644 --- a/management/server/types/networkmap_wire_benchmark_test.go +++ b/management/server/types/networkmap_wire_benchmark_test.go @@ -15,9 +15,8 @@ import ( "github.com/netbirdio/netbird/management/server/types" ) -// wireBenchScales mirrors the scales used by networkmap_benchmark_test.go but -// trimmed: encoding+marshal are linear, so we don't need the 30k peer extreme -// to see the trend. +// wireBenchScales — trimmed scale set for wire-size measurements. Encoding +// and marshalling are linear, so the largest extremes don't add signal. var wireBenchScales = []benchmarkScale{ {"100peers_5groups", 100, 5}, {"500peers_20groups", 500, 20}, diff --git a/shared/management/client/grpc.go b/shared/management/client/grpc.go index 97abc6961..5c8114a54 100644 --- a/shared/management/client/grpc.go +++ b/shared/management/client/grpc.go @@ -952,10 +952,7 @@ func peerCapabilities(info system.Info) []proto.PeerCapability { proto.PeerCapability_PeerCapabilitySourcePrefixes, // PeerCapabilityComponentNetworkMap signals that this client can // decode the components-format SyncResponse.NetworkMapEnvelope and - // run Calculate() locally. Always advertised by Step-4-capable - // builds — there's no opt-out flag because the server-side kill - // switch (NB_NETWORK_MAP_COMPONENTS_DISABLE) covers emergency - // rollback and the client decoder is built in. + // run Calculate() locally. proto.PeerCapability_PeerCapabilityComponentNetworkMap, } if !info.DisableIPv6 { diff --git a/shared/management/networkmap/envelope.go b/shared/management/networkmap/envelope.go index e90cb5982..e2564706b 100644 --- a/shared/management/networkmap/envelope.go +++ b/shared/management/networkmap/envelope.go @@ -19,8 +19,8 @@ import ( // running Calculate() locally + converting back through the shared // proto helpers + merging the optional ProxyPatch. // - Components is the *types.NetworkMapComponents the engine retains so -// future incremental delta updates (Step 3) have a base to apply -// changes against. The client keeps it under its sync lock. +// future incremental delta updates have a base to apply changes +// against. The client keeps it under its sync lock. type EnvelopeResult struct { NetworkMap *proto.NetworkMap Components *types.NetworkMapComponents diff --git a/shared/management/networkmap/envelope_test.go b/shared/management/networkmap/envelope_test.go index 70e705737..f0c40d67d 100644 --- a/shared/management/networkmap/envelope_test.go +++ b/shared/management/networkmap/envelope_test.go @@ -21,9 +21,7 @@ import ( // TestEnvelopeToNetworkMap_RoundTrip exercises the full client-side pipeline: // build a small components struct, encode an envelope, marshal/unmarshal the // wire bytes, decode back via EnvelopeToNetworkMap, and verify the result is -// non-empty and consistent. Deeper per-field semantic equivalence with the -// legacy server path is covered by the prod-DB equivalence test in -// management/server/store/networkmap_envelope_equivalence_test.go. +// non-empty and consistent. func TestEnvelopeToNetworkMap_RoundTrip(t *testing.T) { c, localPeerKey := buildSmokeComponents(t) @@ -51,10 +49,9 @@ func TestEnvelopeToNetworkMap_RoundTrip(t *testing.T) { // TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH guards against the // scenario where a rule with Protocol=NetbirdSSH leaks the enum value into // proto.FirewallRule.Protocol. Calculate() must rewrite NetbirdSSH → TCP -// before forming firewall rules (see networkmap_components.go:282 and -// account.go:868). Without that rewrite, agents fall into UNKNOWN-protocol -// handling, which on some platforms downgrades to allow-all — a real -// security regression. +// before forming firewall rules. Without that rewrite, agents fall into +// UNKNOWN-protocol handling, which on some platforms downgrades to +// allow-all — a real security regression. func TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH(t *testing.T) { c, localPeerKey := buildSmokeComponents(t) // Replace the smoke policy with a NetbirdSSH-protocol allow. diff --git a/shared/management/proto/management.pb.go b/shared/management/proto/management.pb.go index 11fa041b4..538b03460 100644 --- a/shared/management/proto/management.pb.go +++ b/shared/management/proto/management.pb.go @@ -4511,8 +4511,8 @@ func (*StopExposeResponse) Descriptor() ([]byte, []int) { return file_management_proto_rawDescGZIP(), []int{52} } -// NetworkMapEnvelope wraps either a full snapshot or a delta. Step 2 ships -// only Full; Delta is reserved for the incremental-update work. +// NetworkMapEnvelope wraps either a full snapshot or a delta. Only Full is +// emitted today; Delta is reserved for the incremental-update work. type NetworkMapEnvelope struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache @@ -5121,9 +5121,9 @@ func (x *AccountNetwork) GetSerial() uint64 { return 0 } -// NetworkMapComponentsDelta is reserved for the incremental update protocol -// (Step 3 of the migration plan). Field numbers 1–100 are pre-allocated to -// keep room for the planned event types without needing a renumber. +// NetworkMapComponentsDelta is reserved for the incremental update +// protocol. Field numbers 1–100 are pre-allocated to keep room for the +// planned event types without needing a renumber. type NetworkMapComponentsDelta struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache @@ -5358,7 +5358,7 @@ type PolicyCompact struct { // Per-account integer id (matches policies.account_seq_id). Used as a // stable reference for ResourcePoliciesMap.indexes and future delta - // updates (Step 3). + // updates. Id uint32 `protobuf:"varint,1,opt,name=id,proto3" json:"id,omitempty"` Action RuleAction `protobuf:"varint,2,opt,name=action,proto3,enum=management.RuleAction" json:"action,omitempty"` Protocol RuleProtocol `protobuf:"varint,3,opt,name=protocol,proto3,enum=management.RuleProtocol" json:"protocol,omitempty"` diff --git a/shared/management/proto/management.proto b/shared/management/proto/management.proto index 4031f755a..27ea0e498 100644 --- a/shared/management/proto/management.proto +++ b/shared/management/proto/management.proto @@ -734,8 +734,8 @@ message StopExposeResponse {} // NetworkMap from the server. // ===================================================================== -// NetworkMapEnvelope wraps either a full snapshot or a delta. Step 2 ships -// only Full; Delta is reserved for the incremental-update work. +// NetworkMapEnvelope wraps either a full snapshot or a delta. Only Full is +// emitted today; Delta is reserved for the incremental-update work. message NetworkMapEnvelope { oneof payload { NetworkMapComponentsFull full = 1; @@ -898,9 +898,9 @@ message AccountNetwork { uint64 serial = 5; } -// NetworkMapComponentsDelta is reserved for the incremental update protocol -// (Step 3 of the migration plan). Field numbers 1–100 are pre-allocated to -// keep room for the planned event types without needing a renumber. +// NetworkMapComponentsDelta is reserved for the incremental update +// protocol. Field numbers 1–100 are pre-allocated to keep room for the +// planned event types without needing a renumber. message NetworkMapComponentsDelta { reserved 1 to 100; } @@ -984,7 +984,7 @@ message PeerCompact { message PolicyCompact { // Per-account integer id (matches policies.account_seq_id). Used as a // stable reference for ResourcePoliciesMap.indexes and future delta - // updates (Step 3). + // updates. uint32 id = 1; RuleAction action = 2;