From 8f8f469c0ac2ecbdfce79f14f055cbf1e1ff444d Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sun, 23 Jun 2024 22:06:50 +0200 Subject: [PATCH] Remove allocations of lists before use (#1989) * policy: remove allocs before appends in acls Signed-off-by: Kristoffer Dalby * notifier: make batcher tests stable/non-flaky Signed-off-by: Kristoffer Dalby * {db,derp,mapper}: dont allocate until append Signed-off-by: Kristoffer Dalby --------- Signed-off-by: Kristoffer Dalby --- hscontrol/db/node.go | 6 ++--- hscontrol/db/routes.go | 4 +-- hscontrol/derp/derp.go | 2 +- hscontrol/mapper/mapper.go | 2 +- hscontrol/notifier/notifier_test.go | 16 ++++++++++++ hscontrol/policy/acls.go | 40 ++++++++++++++--------------- hscontrol/policy/acls_test.go | 8 +++--- 7 files changed, 46 insertions(+), 32 deletions(-) diff --git a/hscontrol/db/node.go b/hscontrol/db/node.go index c675dc7c..e36d6ed1 100644 --- a/hscontrol/db/node.go +++ b/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)) } diff --git a/hscontrol/db/routes.go b/hscontrol/db/routes.go index 74b2b4b7..3b897190 100644 --- a/hscontrol/db/routes.go +++ b/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 { diff --git a/hscontrol/derp/derp.go b/hscontrol/derp/derp.go index 80ec520d..3afcb4ea 100644 --- a/hscontrol/derp/derp.go +++ b/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(). diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index d4f4392a..a6fa9ad6 100644 --- a/hscontrol/mapper/mapper.go +++ b/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 diff --git a/hscontrol/notifier/notifier_test.go b/hscontrol/notifier/notifier_test.go index 8841a46d..c41e0039 100644 --- a/hscontrol/notifier/notifier_test.go +++ b/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) } diff --git a/hscontrol/policy/acls.go b/hscontrol/policy/acls.go index 1196995d..9dde401b 100644 --- a/hscontrol/policy/acls.go +++ b/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{} - tags := []string{} + var out types.Nodes + 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) - invalidTags := make([]string, 0) + var validTags []string + 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 { diff --git a/hscontrol/policy/acls_test.go b/hscontrol/policy/acls_test.go index b0cafe10..c1e7ae08 100644 --- a/hscontrol/policy/acls_test.go +++ b/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}, }, }