Remove allocations of lists before use (#1989)

* policy: remove allocs before appends in acls

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* notifier: make batcher tests stable/non-flaky

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* {db,derp,mapper}: dont allocate until append

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

hscontrol/db/node.go

@@ -215,7 +215,7 @@ func SetTags(
 		return nil
 	}
 
-	newTags := types.StringList{}
+	var newTags types.StringList
 	for _, tag := range tags {
 		if !util.StringOrPrefixListContains(newTags, tag) {
 			newTags = append(newTags, tag)
@@ -452,7 +452,7 @@ func GetAdvertisedRoutes(tx *gorm.DB, node *types.Node) ([]netip.Prefix, error)
 		return nil, fmt.Errorf("getting advertised routes for node(%d): %w", node.ID, err)
 	}
 
-	prefixes := []netip.Prefix{}
+	var prefixes []netip.Prefix
 	for _, route := range routes {
 		prefixes = append(prefixes, netip.Prefix(route.Prefix))
 	}
@@ -478,7 +478,7 @@ func GetEnabledRoutes(tx *gorm.DB, node *types.Node) ([]netip.Prefix, error) {
 		return nil, fmt.Errorf("getting enabled routes for node(%d): %w", node.ID, err)
 	}
 
-	prefixes := []netip.Prefix{}
+	var prefixes []netip.Prefix
 	for _, route := range routes {
 		prefixes = append(prefixes, netip.Prefix(route.Prefix))
 	}

hscontrol/db/routes.go

@@ -222,7 +222,7 @@ func DeleteRoute(
 			return nil, err
 		}
 
-		routesToDelete := types.Routes{}
+		var routesToDelete types.Routes
 		for _, r := range routes {
 			if r.IsExitRoute() {
 				routesToDelete = append(routesToDelete, r)
@@ -623,7 +623,7 @@ func EnableAutoApprovedRoutes(
 
 	log.Trace().Interface("routes", routes).Msg("routes for autoapproving")
 
-	approvedRoutes := types.Routes{}
+	var approvedRoutes types.Routes
 
 	for _, advertisedRoute := range routes {
 		if advertisedRoute.Enabled {

hscontrol/derp/derp.go

@@ -81,7 +81,7 @@ func mergeDERPMaps(derpMaps []*tailcfg.DERPMap) *tailcfg.DERPMap {
 }
 
 func GetDERPMap(cfg types.DERPConfig) *tailcfg.DERPMap {
-	derpMaps := make([]*tailcfg.DERPMap, 0)
+	var derpMaps []*tailcfg.DERPMap
 
 	for _, path := range cfg.Paths {
 		log.Debug().

hscontrol/mapper/mapper.go

@@ -102,7 +102,7 @@ func generateUserProfiles(
 		userMap[peer.User.Name] = peer.User // not worth checking if already is there
 	}
 
-	profiles := []tailcfg.UserProfile{}
+	var profiles []tailcfg.UserProfile
 	for _, user := range userMap {
 		displayName := user.Name
 

hscontrol/notifier/notifier_test.go

@@ -3,6 +3,7 @@ package notifier
 import (
 	"context"
 	"net/netip"
+	"sort"
 	"testing"
 	"time"
 
@@ -221,6 +222,11 @@ func TestBatcher(t *testing.T) {
 					// We will call flush manually for the tests,
 					// so do not run the worker.
 					BatchChangeDelay: time.Hour,
+
+					// Since we do not load the config, we wont get the
+					// default, so set it manually so we dont time out
+					// and have flakes.
+					NotifierSendTimeout: time.Second,
 				},
 			})
 
@@ -241,6 +247,16 @@ func TestBatcher(t *testing.T) {
 				got = append(got, out)
 			}
 
+			// Make the inner order stable for comparison.
+			for _, u := range got {
+				sort.Slice(u.ChangeNodes, func(i, j int) bool {
+					return u.ChangeNodes[i] < u.ChangeNodes[j]
+				})
+				sort.Slice(u.ChangePatches, func(i, j int) bool {
+					return u.ChangePatches[i].NodeID < u.ChangePatches[j].NodeID
+				})
+			}
+
 			if diff := cmp.Diff(tt.want, got, util.Comparers...); diff != "" {
 				t.Errorf("batcher() unexpected result (-want +got):\n%s", diff)
 			}

hscontrol/policy/acls.go

@@ -180,14 +180,14 @@ func (pol *ACLPolicy) CompileFilterRules(
 		return tailcfg.FilterAllowAll, nil
 	}
 
-	rules := []tailcfg.FilterRule{}
+	var rules []tailcfg.FilterRule
 
 	for index, acl := range pol.ACLs {
 		if acl.Action != "accept" {
 			return nil, ErrInvalidAction
 		}
 
-		srcIPs := []string{}
+		var srcIPs []string
 		for srcIndex, src := range acl.Sources {
 			srcs, err := pol.expandSource(src, nodes)
 			if err != nil {
@@ -221,7 +221,7 @@ func (pol *ACLPolicy) CompileFilterRules(
 				return nil, err
 			}
 
-			dests := []tailcfg.NetPortRange{}
+			var dests []tailcfg.NetPortRange
 			for _, dest := range expanded.Prefixes() {
 				for _, port := range *ports {
 					pr := tailcfg.NetPortRange{
@@ -251,8 +251,7 @@ func ReduceFilterRules(node *types.Node, rules []tailcfg.FilterRule) []tailcfg.F
 
 	for _, rule := range rules {
 		// record if the rule is actually relevant for the given node.
-		dests := []tailcfg.NetPortRange{}
+		var dests []tailcfg.NetPortRange
-
 	DEST_LOOP:
 		for _, dest := range rule.DstPorts {
 			expanded, err := util.ParseIPSet(dest.IP, nil)
@@ -301,7 +300,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
 		return nil, nil
 	}
 
-	rules := []*tailcfg.SSHRule{}
+	var rules []*tailcfg.SSHRule
 
 	acceptAction := tailcfg.SSHAction{
 		Message:                  "",
@@ -533,8 +532,7 @@ func (pol *ACLPolicy) expandSource(
 		return []string{}, err
 	}
 
-	prefixes := []string{}
+	var prefixes []string
-
 	for _, prefix := range ipSet.Prefixes() {
 		prefixes = append(prefixes, prefix.String())
 	}
@@ -615,8 +613,8 @@ func excludeCorrectlyTaggedNodes(
 	nodes types.Nodes,
 	user string,
 ) types.Nodes {
-	out := types.Nodes{}
+	var out types.Nodes
-	tags := []string{}
+	var tags []string
 	for tag := range aclPolicy.TagOwners {
 		owners, _ := expandOwnersFromTag(aclPolicy, user)
 		ns := append(owners, user)
@@ -661,7 +659,7 @@ func expandPorts(portsStr string, isWild bool) (*[]tailcfg.PortRange, error) {
 		return nil, ErrWildcardIsNeeded
 	}
 
-	ports := []tailcfg.PortRange{}
+	var ports []tailcfg.PortRange
 	for _, portStr := range strings.Split(portsStr, ",") {
 		log.Trace().Msgf("parsing portstring: %s", portStr)
 		rang := strings.Split(portStr, "-")
@@ -737,7 +735,7 @@ func expandOwnersFromTag(
 func (pol *ACLPolicy) expandUsersFromGroup(
 	group string,
 ) ([]string, error) {
-	users := []string{}
+	var users []string
 	log.Trace().Caller().Interface("pol", pol).Msg("test")
 	aclGroups, ok := pol.Groups[group]
 	if !ok {
@@ -772,7 +770,7 @@ func (pol *ACLPolicy) expandIPsFromGroup(
 	group string,
 	nodes types.Nodes,
 ) (*netipx.IPSet, error) {
-	build := netipx.IPSetBuilder{}
+	var build netipx.IPSetBuilder
 
 	users, err := pol.expandUsersFromGroup(group)
 	if err != nil {
@@ -792,7 +790,7 @@ func (pol *ACLPolicy) expandIPsFromTag(
 	alias string,
 	nodes types.Nodes,
 ) (*netipx.IPSet, error) {
-	build := netipx.IPSetBuilder{}
+	var build netipx.IPSetBuilder
 
 	// check for forced tags
 	for _, node := range nodes {
@@ -841,7 +839,7 @@ func (pol *ACLPolicy) expandIPsFromUser(
 	user string,
 	nodes types.Nodes,
 ) (*netipx.IPSet, error) {
-	build := netipx.IPSetBuilder{}
+	var build netipx.IPSetBuilder
 
 	filteredNodes := filterNodesByUser(nodes, user)
 	filteredNodes = excludeCorrectlyTaggedNodes(pol, filteredNodes, user)
@@ -866,7 +864,7 @@ func (pol *ACLPolicy) expandIPsFromSingleIP(
 
 	matches := nodes.FilterByIP(ip)
 
-	build := netipx.IPSetBuilder{}
+	var build netipx.IPSetBuilder
 	build.Add(ip)
 
 	for _, node := range matches {
@@ -881,7 +879,7 @@ func (pol *ACLPolicy) expandIPsFromIPPrefix(
 	nodes types.Nodes,
 ) (*netipx.IPSet, error) {
 	log.Trace().Str("prefix", prefix.String()).Msg("expandAlias got prefix")
-	build := netipx.IPSetBuilder{}
+	var build netipx.IPSetBuilder
 	build.AddPrefix(prefix)
 
 	// This is suboptimal and quite expensive, but if we only add the prefix, we will miss all the relevant IPv6
@@ -931,8 +929,8 @@ func isAutoGroup(str string) bool {
 func (pol *ACLPolicy) TagsOfNode(
 	node *types.Node,
 ) ([]string, []string) {
-	validTags := make([]string, 0)
+	var validTags []string
-	invalidTags := make([]string, 0)
+	var invalidTags []string
 
 	// TODO(kradalby): Why is this sometimes nil? coming from tailNode?
 	if node == nil {
@@ -973,7 +971,7 @@ func (pol *ACLPolicy) TagsOfNode(
 }
 
 func filterNodesByUser(nodes types.Nodes, user string) types.Nodes {
-	out := types.Nodes{}
+	var out types.Nodes
 	for _, node := range nodes {
 		if node.User.Name == user {
 			out = append(out, node)
@@ -989,7 +987,7 @@ func FilterNodesByACL(
 	nodes types.Nodes,
 	filter []tailcfg.FilterRule,
 ) types.Nodes {
-	result := types.Nodes{}
+	var result types.Nodes
 
 	for index, peer := range nodes {
 		if peer.ID == node.ID {

hscontrol/policy/acls_test.go

@@ -943,7 +943,7 @@ func Test_listNodesInUser(t *testing.T) {
 				},
 				user: "mickael",
 			},
-			want: types.Nodes{},
+			want: nil,
 		},
 	}
 	for _, test := range tests {
@@ -1645,7 +1645,7 @@ func TestACLPolicy_generateFilterRules(t *testing.T) {
 			name:    "no-policy",
 			field:   field{},
 			args:    args{},
-			want:    []tailcfg.FilterRule{},
+			want:    nil,
 			wantErr: false,
 		},
 		{
@@ -2896,7 +2896,7 @@ func Test_getFilteredByACLPeers(t *testing.T) {
 					User: types.User{Name: "marc"},
 				},
 			},
-			want: types.Nodes{},
+			want: nil,
 		},
 		{
 			// Investigating 699
@@ -3426,7 +3426,7 @@ func TestSSHRules(t *testing.T) {
 					},
 				},
 			},
-			want: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{}},
+			want: &tailcfg.SSHPolicy{Rules: nil},
 		},
 	}