comments cleanup

.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

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
+		// retained so future incremental updates can apply deltas instead
-		// instead of doing a full reconstruction.
+		// 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

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
+// struct that takes a File path. Non-CGO builds get an empty stub whose
-// the empty stub — `Open()` then returns the dex "SQLite not available"
+// Open() returns the dex "SQLite not available" error — correct behaviour
-// error, which is correct behaviour for binaries that can't link sqlite3.
+// for binaries that can't link sqlite3 (e.g. cross-compiled ARM targets).
-//
-// 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.
 func newSQLite3(file string) *sql.SQLite3 {
 	return &sql.SQLite3{File: file}
 }

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
+// Open() returns an error documenting the missing CGO support — correct
-// right behaviour for the cross-compiled artefacts (linux/arm, wasm, etc.)
+// behaviour for cross-compiled artefacts that never actually run the
-// — those targets never actually run the embedded IdP.
+// embedded IdP. The `file` argument is ignored.
-//
-// The `file` argument is ignored intentionally; keeping the signature
-// matched with sqlite_cgo.go lets the call sites stay identical.
 func newSQLite3(_ string) *sql.SQLite3 {
 	return &sql.SQLite3{}
 }

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
+	// componentsDisabled, when true, forces the controller to emit legacy
-	// format. When true the controller emits legacy proto.NetworkMap to every
+	// proto.NetworkMap to every peer regardless of capability. Set once at
-	// peer regardless of capability — used to roll back instantly via a
+	// construction and never written after — readers race-free without a
-	// management restart from a bad components encoder.
+	// mutex.
-	//
-	// Set once in NewController from NB_NETWORK_MAP_COMPONENTS_DISABLE 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
+// component-based wire format for this peer.
-// capability with the controller-level kill switch — callers ask exactly
-// this question, so encapsulating it removes accidental double-checks.
 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
+// literal.
-// (e.g. event.go's NB_TRAFFIC_EVENT_*) and reduces operator surprises.
 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
+// encodes both into the wire envelope. Callers must gate on capability
-// checking peer capability + componentsDisabled before dispatching here —
+// themselves before dispatching here — this method does NOT branch on it.
-// this method does NOT branch on capability itself.
 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)

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
+// The envelope is fully self-contained — every field needed by the client's
-// client's local Calculate() comes from the components struct itself. The
+// local Calculate() comes from the components struct itself. The only
-// only externally-supplied data is the receiving peer's PeerConfig (which is
+// 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
+// index spaces are local to a single envelope.
-// use a different shape for the same reason.
 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
+	// behaviour for missing/unvalidated peers (return a NetworkMap with only
-	// (return a NetworkMap with only Network populated). The receiver gets
+	// Network populated). The receiver gets an envelope it can decode
-	// an envelope it can decode without crashing; AccountSettings stays
+	// without crashing; AccountSettings stays non-nil so client-side
-	// non-nil so client-side dereferences are safe.
+	// 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
+		// populated. The receiver gets enough to bootstrap (Network
-		// bootstrap (Network identifier, dns_domain, account_settings) and
+		// identifier, dns_domain, account_settings) and nothing else.
-		// 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
+// production path always populates c.Network, but the encoder is exported
-// the encoder is exported and a hand-built components struct may omit it.
+// and a hand-built components struct may omit it.
 func networkSerial(n *types.Network) uint64 {
 	if n == nil {
 		return 0

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:`
+// graceful-degrade). Guards against a regression that drops `ProxyPatch:`
-// from one of the struct literals in components_encoder.go would slip past CI.
+// 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",

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`
+// Mirrors the two activation paths Calculate() uses:
-// `getPeerConnectionResources`):
 //  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

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 destination peer has SSHEnabled=true locally.
-// the B1 fix that the prod-DB equivalence test alone wouldn't have caught
-// if no account had this combination.
 func TestComputeSSHEnabledForPeer(t *testing.T) {
 	const targetPeerID = "target"
 	const targetGroupID = "g_dst"

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
+		// TODO: refactor SyncPeer / SyncAndMarkPeer / their mocks + manager
-		// mocks + manager interfaces to return PeerNetworkMapResult so the
+		// interfaces to return PeerNetworkMapResult so the initial-sync path
-		// initial-sync path stops doing duplicate work. ~13 files of churn,
+		// stops doing duplicate work. Deferred until the client-side
-		// deferred until the client-side decoder lands and there's a real
+		// decoder lands and there's a real deployment of capability=3 peers
-		// deployment of capability=3 peers worth optimizing for.
+		// 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)

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
+// wireBenchScales — trimmed scale set for wire-size measurements. Encoding
-// trimmed: encoding+marshal are linear, so we don't need the 30k peer extreme
+// and marshalling are linear, so the largest extremes don't add signal.
-// to see the trend.
 var wireBenchScales = []benchmarkScale{
 	{"100peers_5groups", 100, 5},
 	{"500peers_20groups", 500, 20},

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
+		// run Calculate() locally.
-		// 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.
 		proto.PeerCapability_PeerCapabilityComponentNetworkMap,
 	}
 	if !info.DisableIPv6 {

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
+//     future incremental delta updates have a base to apply changes
-//     changes against. The client keeps it under its sync lock.
+//     against. The client keeps it under its sync lock.
 type EnvelopeResult struct {
 	NetworkMap *proto.NetworkMap
 	Components *types.NetworkMapComponents

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
+// non-empty and consistent.
-// legacy server path is covered by the prod-DB equivalence test in
-// management/server/store/networkmap_envelope_equivalence_test.go.
 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
+// before forming firewall rules. Without that rewrite, agents fall into
-// account.go:868). Without that rewrite, agents fall into UNKNOWN-protocol
+// UNKNOWN-protocol handling, which on some platforms downgrades to
-// handling, which on some platforms downgrades to allow-all — a real
+// allow-all — a real security regression.
-// security regression.
 func TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH(t *testing.T) {
 	c, localPeerKey := buildSmokeComponents(t)
 	// Replace the smoke policy with a NetbirdSSH-protocol allow.

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
+// NetworkMapEnvelope wraps either a full snapshot or a delta. Only Full is
-// only Full; Delta is reserved for the incremental-update work.
+// 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
+// NetworkMapComponentsDelta is reserved for the incremental update
-// (Step 3 of the migration plan). Field numbers 1–100 are pre-allocated to
+// protocol. Field numbers 1–100 are pre-allocated to keep room for the
-// keep room for the planned event types without needing a renumber.
+// 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"`

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
+// NetworkMapEnvelope wraps either a full snapshot or a delta. Only Full is
-// only Full; Delta is reserved for the incremental-update work.
+// 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
+// NetworkMapComponentsDelta is reserved for the incremental update
-// (Step 3 of the migration plan). Field numbers 1–100 are pre-allocated to
+// protocol. Field numbers 1–100 are pre-allocated to keep room for the
-// keep room for the planned event types without needing a renumber.
+// 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;