From 0cb49ab343dd6809fb96de894499a01ccdc4b71a Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 6 Feb 2026 07:59:27 +0000 Subject: [PATCH] hscontrol: fix err113 lint issues (batch 7) Add sentinel errors and use %w wrapping for dynamic errors in: - mapper/batcher.go: ErrNodeConnectionNil, ErrNodeNotFoundMapper - mapper/batcher_lockfree.go: ErrInitialMapSendTimeout, ErrBatcherShuttingDown, ErrConnectionSendTimeout - mapper/builder.go: use ErrNodeNotFoundMapper (remove errors import) - types/common.go: ErrInvalidRegistrationIDLength - types/config.go: ErrNoPrefixConfigured, ErrInvalidAllocationStrategy - types/node.go: use existing ErrInvalidNodeView - types/users.go: ErrCannotParseBoolean - util/util.go: ErrTracerouteDidNotReach - util/util_test.go: use ErrTracerouteDidNotReach Co-Authored-By: Claude Opus 4.5 --- hscontrol/mapper/batcher.go | 8 +++++--- hscontrol/mapper/batcher_lockfree.go | 22 ++++++++++++++-------- hscontrol/mapper/builder.go | 13 ++++++------- hscontrol/types/common.go | 8 ++++++-- hscontrol/types/config.go | 15 +++++++++------ hscontrol/types/node.go | 2 +- hscontrol/types/users.go | 6 +++++- hscontrol/util/util.go | 3 ++- hscontrol/util/util_test.go | 5 ++--- 9 files changed, 50 insertions(+), 32 deletions(-) diff --git a/hscontrol/mapper/batcher.go b/hscontrol/mapper/batcher.go index c4d76c41..1f092a9c 100644 --- a/hscontrol/mapper/batcher.go +++ b/hscontrol/mapper/batcher.go @@ -18,8 +18,10 @@ import ( // Mapper errors. var ( - ErrInvalidNodeID = errors.New("invalid nodeID") - ErrMapperNil = errors.New("mapper is nil") + ErrInvalidNodeID = errors.New("invalid nodeID") + ErrMapperNil = errors.New("mapper is nil") + ErrNodeConnectionNil = errors.New("nodeConnection is nil") + ErrNodeNotFoundMapper = errors.New("node not found") ) var mapResponseGenerated = promauto.NewCounterVec(prometheus.CounterOpts{ @@ -142,7 +144,7 @@ func generateMapResponse(nc nodeConnection, mapper *mapper, r change.Change) (*t // handleNodeChange generates and sends a [tailcfg.MapResponse] for a given node and [change.Change]. func handleNodeChange(nc nodeConnection, mapper *mapper, r change.Change) error { if nc == nil { - return errors.New("nodeConnection is nil") + return ErrNodeConnectionNil } nodeID := nc.nodeID() diff --git a/hscontrol/mapper/batcher_lockfree.go b/hscontrol/mapper/batcher_lockfree.go index 728b7156..b61b8067 100644 --- a/hscontrol/mapper/batcher_lockfree.go +++ b/hscontrol/mapper/batcher_lockfree.go @@ -19,7 +19,13 @@ import ( "tailscale.com/types/ptr" ) -var errConnectionClosed = errors.New("connection channel already closed") +// LockFreeBatcher errors. +var ( + errConnectionClosed = errors.New("connection channel already closed") + ErrInitialMapSendTimeout = errors.New("sending initial map: timeout") + ErrBatcherShuttingDown = errors.New("batcher shutting down") + ErrConnectionSendTimeout = errors.New("timeout sending to channel (likely stale connection)") +) // LockFreeBatcher uses atomic operations and concurrent maps to eliminate mutex contention. type LockFreeBatcher struct { @@ -92,12 +98,12 @@ func (b *LockFreeBatcher) AddNode(id types.NodeID, c chan<- *tailcfg.MapResponse case c <- initialMap: // Success case <-time.After(5 * time.Second): //nolint:mnd - nlog.Error().Err(errors.New("timeout")).Msg("initial map send timeout") //nolint:err113 - nlog.Debug().Caller().Dur("timeout.duration", 5*time.Second). //nolint:mnd - Msg("initial map send timed out because channel was blocked or receiver not ready") + nlog.Error().Err(ErrInitialMapSendTimeout).Msg("initial map send timeout") + nlog.Debug().Caller().Dur("timeout.duration", 5*time.Second). //nolint:mnd + Msg("initial map send timed out because channel was blocked or receiver not ready") nodeConn.removeConnectionByChannel(c) - return fmt.Errorf("sending initial map to node %d: timeout", id) + return fmt.Errorf("%w for node %d", ErrInitialMapSendTimeout, id) } // Update connection status @@ -241,7 +247,7 @@ func (b *LockFreeBatcher) worker(workerID int) { nc.updateSentPeers(result.mapResponse) } } else { - result.err = fmt.Errorf("node %d not found", w.nodeID) + result.err = fmt.Errorf("%w: %d", ErrNodeNotFoundMapper, w.nodeID) b.workErrors.Add(1) wlog.Error().Err(result.err). @@ -497,7 +503,7 @@ func (b *LockFreeBatcher) MapResponseFromChange(id types.NodeID, ch change.Chang case result := <-resultCh: return result.mapResponse, result.err case <-b.done: - return nil, fmt.Errorf("batcher shutting down while generating map response for node %d", id) + return nil, fmt.Errorf("%w while generating map response for node %d", ErrBatcherShuttingDown, id) } } @@ -714,7 +720,7 @@ func (entry *connectionEntry) send(data *tailcfg.MapResponse) error { case <-time.After(50 * time.Millisecond): // Connection is likely stale - client isn't reading from channel // This catches the case where Docker containers are killed but channels remain open - return fmt.Errorf("connection %s: timeout sending to channel (likely stale connection)", entry.id) + return fmt.Errorf("connection %s: %w", entry.id, ErrConnectionSendTimeout) } } diff --git a/hscontrol/mapper/builder.go b/hscontrol/mapper/builder.go index cd03d2ab..ee1da337 100644 --- a/hscontrol/mapper/builder.go +++ b/hscontrol/mapper/builder.go @@ -1,7 +1,6 @@ package mapper import ( - "errors" "net/netip" "sort" "time" @@ -70,7 +69,7 @@ func (b *MapResponseBuilder) WithCapabilityVersion(capVer tailcfg.CapabilityVers func (b *MapResponseBuilder) WithSelfNode() *MapResponseBuilder { nv, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFoundMapper) return b } @@ -132,7 +131,7 @@ func (b *MapResponseBuilder) WithDebugConfig() *MapResponseBuilder { func (b *MapResponseBuilder) WithSSHPolicy() *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFoundMapper) return b } @@ -151,7 +150,7 @@ func (b *MapResponseBuilder) WithSSHPolicy() *MapResponseBuilder { func (b *MapResponseBuilder) WithDNSConfig() *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFoundMapper) return b } @@ -164,7 +163,7 @@ func (b *MapResponseBuilder) WithDNSConfig() *MapResponseBuilder { func (b *MapResponseBuilder) WithUserProfiles(peers views.Slice[types.NodeView]) *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFoundMapper) return b } @@ -177,7 +176,7 @@ func (b *MapResponseBuilder) WithUserProfiles(peers views.Slice[types.NodeView]) func (b *MapResponseBuilder) WithPacketFilters() *MapResponseBuilder { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - b.addError(errors.New("node not found")) + b.addError(ErrNodeNotFoundMapper) return b } @@ -231,7 +230,7 @@ func (b *MapResponseBuilder) WithPeerChanges(peers views.Slice[types.NodeView]) func (b *MapResponseBuilder) buildTailPeers(peers views.Slice[types.NodeView]) ([]*tailcfg.Node, error) { node, ok := b.mapper.state.GetNodeByID(b.nodeID) if !ok { - return nil, errors.New("node not found") + return nil, ErrNodeNotFoundMapper } // Get unreduced matchers for peer relationship determination. diff --git a/hscontrol/types/common.go b/hscontrol/types/common.go index be3756a0..297225e6 100644 --- a/hscontrol/types/common.go +++ b/hscontrol/types/common.go @@ -20,7 +20,11 @@ const ( DatabaseSqlite = "sqlite3" ) -var ErrCannotParsePrefix = errors.New("cannot parse prefix") +// Common errors. +var ( + ErrCannotParsePrefix = errors.New("cannot parse prefix") + ErrInvalidRegistrationIDLength = errors.New("registration ID has invalid length") +) type StateUpdateType int @@ -175,7 +179,7 @@ func MustRegistrationID() RegistrationID { func RegistrationIDFromString(str string) (RegistrationID, error) { if len(str) != RegistrationIDLength { - return "", fmt.Errorf("registration ID must be %d characters long", RegistrationIDLength) + return "", fmt.Errorf("%w: expected %d, got %d", ErrInvalidRegistrationIDLength, RegistrationIDLength, len(str)) } return RegistrationID(str), nil diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index 418c0faf..fcb471b4 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -33,10 +33,12 @@ const ( ) var ( - errOidcMutuallyExclusive = errors.New("oidc_client_secret and oidc_client_secret_path are mutually exclusive") - errServerURLSuffix = errors.New("server_url cannot be part of base_domain in a way that could make the DERP and headscale server unreachable") - errServerURLSame = errors.New("server_url cannot use the same domain as base_domain in a way that could make the DERP and headscale server unreachable") - errInvalidPKCEMethod = errors.New("pkce.method must be either 'plain' or 'S256'") + errOidcMutuallyExclusive = errors.New("oidc_client_secret and oidc_client_secret_path are mutually exclusive") + errServerURLSuffix = errors.New("server_url cannot be part of base_domain in a way that could make the DERP and headscale server unreachable") + errServerURLSame = errors.New("server_url cannot use the same domain as base_domain in a way that could make the DERP and headscale server unreachable") + errInvalidPKCEMethod = errors.New("pkce.method must be either 'plain' or 'S256'") + ErrNoPrefixConfigured = errors.New("no IPv4 or IPv6 prefix configured, minimum one prefix is required") + ErrInvalidAllocationStrategy = errors.New("invalid prefix allocation strategy") ) type IPAllocationStrategy string @@ -931,7 +933,7 @@ func LoadServerConfig() (*Config, error) { } if prefix4 == nil && prefix6 == nil { - return nil, errors.New("no IPv4 or IPv6 prefix configured, minimum one prefix is required") + return nil, ErrNoPrefixConfigured } allocStr := viper.GetString("prefixes.allocation") @@ -945,7 +947,8 @@ func LoadServerConfig() (*Config, error) { alloc = IPAllocationStrategyRandom default: return nil, fmt.Errorf( - "config error, prefixes.allocation is set to %s, which is not a valid strategy, allowed options: %s, %s", + "%w: %q, allowed options: %s, %s", + ErrInvalidAllocationStrategy, allocStr, IPAllocationStrategySequential, IPAllocationStrategyRandom, diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 8bd0a108..11afc964 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -901,7 +901,7 @@ func (nv NodeView) PeerChangeFromMapRequest(req tailcfg.MapRequest) tailcfg.Peer // GetFQDN returns the fully qualified domain name for the node. func (nv NodeView) GetFQDN(baseDomain string) (string, error) { if !nv.Valid() { - return "", errors.New("creating valid FQDN: node view is invalid") + return "", fmt.Errorf("creating valid FQDN: %w", ErrInvalidNodeView) } return nv.ж.GetFQDN(baseDomain) diff --git a/hscontrol/types/users.go b/hscontrol/types/users.go index 7b5be75c..6f9a97e7 100644 --- a/hscontrol/types/users.go +++ b/hscontrol/types/users.go @@ -4,6 +4,7 @@ import ( "cmp" "database/sql" "encoding/json" + "errors" "fmt" "net/mail" "net/url" @@ -20,6 +21,9 @@ import ( "tailscale.com/tailcfg" ) +// ErrCannotParseBoolean is returned when a value cannot be parsed as boolean. +var ErrCannotParseBoolean = errors.New("cannot parse value as boolean") + type UserID uint64 type Users []User @@ -252,7 +256,7 @@ func (bit *FlexibleBoolean) UnmarshalJSON(data []byte) error { *bit = FlexibleBoolean(pv) default: - return fmt.Errorf("parsing %v as boolean", v) + return fmt.Errorf("%w: %v", ErrCannotParseBoolean, v) } return nil diff --git a/hscontrol/util/util.go b/hscontrol/util/util.go index 5e3acdb8..9f34b0f8 100644 --- a/hscontrol/util/util.go +++ b/hscontrol/util/util.go @@ -22,6 +22,7 @@ var ( ErrNoURLFound = errors.New("no URL found") ErrEmptyTracerouteOutput = errors.New("empty traceroute output") ErrTracerouteHeaderParse = errors.New("parsing traceroute header") + ErrTracerouteDidNotReach = errors.New("traceroute did not reach target") ) func TailscaleVersionNewerOrEqual(minimum, toCheck string) bool { @@ -263,7 +264,7 @@ func ParseTraceroute(output string) (Traceroute, error) { // If we didn't reach the target, it's unsuccessful if !result.Success { - result.Err = errors.New("traceroute did not reach target") + result.Err = ErrTracerouteDidNotReach } return result, nil diff --git a/hscontrol/util/util_test.go b/hscontrol/util/util_test.go index f416871e..c38d9105 100644 --- a/hscontrol/util/util_test.go +++ b/hscontrol/util/util_test.go @@ -1,7 +1,6 @@ package util import ( - "errors" "net/netip" "strings" "testing" @@ -323,7 +322,7 @@ func TestParseTraceroute(t *testing.T) { }, }, Success: false, - Err: errors.New("traceroute did not reach target"), + Err: ErrTracerouteDidNotReach, }, wantErr: false, }, @@ -491,7 +490,7 @@ over a maximum of 30 hops: }, }, Success: false, - Err: errors.New("traceroute did not reach target"), + Err: ErrTracerouteDidNotReach, }, wantErr: false, },