wgengine/filter/filtertype: make Match.IPProto a view

I noticed we were allocating these every time when they could just share the same memory. Rather than document ownership, just lock it down with a view. I was considering doing all of the fields but decided to just do this one first as test to see how infectious it became. Conclusion: not very. Updates #cleanup (while working towards tailscale/corp#20514) Change-Id: I8ce08519de0c9a53f20292adfbecd970fe362de0 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
tailscale · Jun 18, 2024 · bd93c30 · bd93c30
1 parent bfb775c
commit bd93c30
Show file tree

Hide file tree

Showing 8 changed files with 26 additions and 24 deletions.
diff --git a/net/tstun/wrap_test.go b/net/tstun/wrap_test.go
@@ -36,6 +36,7 @@ import (
 	"tailscale.com/types/logger"
 	"tailscale.com/types/netlogtype"
 	"tailscale.com/types/ptr"
+	"tailscale.com/types/views"
 	"tailscale.com/util/must"
 	"tailscale.com/wgengine/capture"
 	"tailscale.com/wgengine/filter"
@@ -156,10 +157,10 @@ func netports(netPorts ...string) (ret []filter.NetPortRange) {
 }
 
 func setfilter(logf logger.Logf, tun *Wrapper) {
-	protos := []ipproto.Proto{
+	protos := views.SliceOf([]ipproto.Proto{
 		ipproto.TCP,
 		ipproto.UDP,
-	}
+	})
 	matches := []filter.Match{
 		{IPProto: protos, Srcs: nets("5.6.7.8"), Dsts: netports("1.2.3.4:89-90")},
 		{IPProto: protos, Srcs: nets("1.2.3.4"), Dsts: netports("5.6.7.8:98")},

diff --git a/util/deephash/deephash_test.go b/util/deephash/deephash_test.go
@@ -27,6 +27,7 @@ import (
 	"tailscale.com/types/ipproto"
 	"tailscale.com/types/key"
 	"tailscale.com/types/ptr"
+	"tailscale.com/types/views"
 	"tailscale.com/util/deephash/testtype"
 	"tailscale.com/util/dnsname"
 	"tailscale.com/util/hashx"
@@ -353,7 +354,7 @@ func getVal() *tailscaleTypes {
 			},
 		},
 		filter.Match{
-			IPProto: []ipproto.Proto{1, 2, 3},
+			IPProto: views.SliceOf([]ipproto.Proto{1, 2, 3}),
 		},
 	}
 }

diff --git a/wgengine/filter/filter.go b/wgengine/filter/filter.go
@@ -126,7 +126,7 @@ func NewAllowAllForTest(logf logger.Logf) *Filter {
 	any6 := netip.PrefixFrom(netip.AddrFrom16([16]byte{}), 0)
 	ms := []Match{
 		{
-			IPProto: []ipproto.Proto{ipproto.TCP, ipproto.UDP, ipproto.ICMPv4},
+			IPProto: views.SliceOf([]ipproto.Proto{ipproto.TCP, ipproto.UDP, ipproto.ICMPv4}),
 			Srcs:    []netip.Prefix{any4},
 			Dsts: []NetPortRange{
 				{
@@ -139,7 +139,7 @@ func NewAllowAllForTest(logf logger.Logf) *Filter {
 			},
 		},
 		{
-			IPProto: []ipproto.Proto{ipproto.TCP, ipproto.UDP, ipproto.ICMPv6},
+			IPProto: views.SliceOf([]ipproto.Proto{ipproto.TCP, ipproto.UDP, ipproto.ICMPv6}),
 			Srcs:    []netip.Prefix{any6},
 			Dsts: []NetPortRange{
 				{

diff --git a/wgengine/filter/filter_test.go b/wgengine/filter/filter_test.go
@@ -45,7 +45,7 @@ func m(srcs []netip.Prefix, dsts []NetPortRange, protos ...ipproto.Proto) Match
 		protos = defaultProtos
 	}
 	return Match{
-		IPProto:      protos,
+		IPProto:      views.SliceOf(protos),
 		Srcs:         srcs,
 		SrcsContains: ipset.NewContainsIPFunc(views.SliceOf(srcs)),
 		Dsts:         dsts,
@@ -767,12 +767,7 @@ func TestMatchesFromFilterRules(t *testing.T) {
 			},
 			want: []Match{
 				{
-					IPProto: []ipproto.Proto{
-						ipproto.TCP,
-						ipproto.UDP,
-						ipproto.ICMPv4,
-						ipproto.ICMPv6,
-					},
+					IPProto: defaultProtosView,
 					Dsts: []NetPortRange{
 						{
 							Net:   netip.MustParsePrefix("0.0.0.0/0"),
@@ -804,9 +799,9 @@ func TestMatchesFromFilterRules(t *testing.T) {
 			},
 			want: []Match{
 				{
-					IPProto: []ipproto.Proto{
+					IPProto: views.SliceOf([]ipproto.Proto{
 						ipproto.TCP,
-					},
+					}),
 					Dsts: []NetPortRange{
 						{
 							Net:   netip.MustParsePrefix("1.2.0.0/16"),
@@ -830,6 +825,7 @@ func TestMatchesFromFilterRules(t *testing.T) {
 			cmpOpts := []cmp.Option{
 				cmp.Comparer(func(a, b netip.Addr) bool { return a == b }),
 				cmp.Comparer(func(a, b netip.Prefix) bool { return a == b }),
+				cmp.Comparer(func(a, b views.Slice[ipproto.Proto]) bool { return views.SliceEqual(a, b) }),
 				cmpopts.IgnoreFields(Match{}, ".SrcsContains"),
 			}
 			if diff := cmp.Diff(got, tt.want, cmpOpts...); diff != "" {

diff --git a/wgengine/filter/filtertype/filtertype.go b/wgengine/filter/filtertype/filtertype.go
@@ -11,6 +11,7 @@ import (
 
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/ipproto"
+	"tailscale.com/types/views"
 )
 
 //go:generate go run tailscale.com/cmd/cloner --type=Match,CapMatch
@@ -65,7 +66,7 @@ type CapMatch struct {
 // Match matches packets from any IP address in Srcs to any ip:port in
 // Dsts.
 type Match struct {
-	IPProto      []ipproto.Proto // required set (no default value at this layer)
+	IPProto      views.Slice[ipproto.Proto] // required set (no default value at this layer)
 	Srcs         []netip.Prefix
 	SrcsContains func(netip.Addr) bool `json:"-"` // report whether Addr is in Srcs
 	Dsts         []NetPortRange        // optional, if Srcs match

diff --git a/wgengine/filter/filtertype/filtertype_clone.go b/wgengine/filter/filtertype/filtertype_clone.go
diff --git a/wgengine/filter/match.go b/wgengine/filter/match.go
@@ -4,17 +4,16 @@
 package filter
 
 import (
-	"slices"
-
 	"tailscale.com/net/packet"
+	"tailscale.com/types/views"
 	"tailscale.com/wgengine/filter/filtertype"
 )
 
 type matches []filtertype.Match
 
 func (ms matches) match(q *packet.Parsed) bool {
 	for _, m := range ms {
-		if !slices.Contains(m.IPProto, q.IPProto) {
+		if !views.SliceContains(m.IPProto, q.IPProto) {
 			continue
 		}
 		if !m.SrcsContains(q.Src.Addr()) {
@@ -52,7 +51,7 @@ func (ms matches) matchIPsOnly(q *packet.Parsed) bool {
 // ignored, as long as the match is for the entire uint16 port range.
 func (ms matches) matchProtoAndIPsOnlyIfAllPorts(q *packet.Parsed) bool {
 	for _, m := range ms {
-		if !slices.Contains(m.IPProto, q.IPProto) {
+		if !views.SliceContains(m.IPProto, q.IPProto) {
 			continue
 		}
 		if !m.SrcsContains(q.Src.Addr()) {

diff --git a/wgengine/filter/tailcfg.go b/wgengine/filter/tailcfg.go
@@ -23,6 +23,8 @@ var defaultProtos = []ipproto.Proto{
 	ipproto.ICMPv6,
 }
 
+var defaultProtosView = views.SliceOf(defaultProtos)
+
 // MatchesFromFilterRules converts tailcfg FilterRules into Matches.
 // If an error is returned, the Matches result is still valid,
 // containing the rules that were successfully converted.
@@ -41,14 +43,15 @@ func MatchesFromFilterRules(pf []tailcfg.FilterRule) ([]Match, error) {
 		}
 
 		if len(r.IPProto) == 0 {
-			m.IPProto = append([]ipproto.Proto(nil), defaultProtos...)
+			m.IPProto = defaultProtosView
 		} else {
-			m.IPProto = make([]ipproto.Proto, 0, len(r.IPProto))
+			filtered := make([]ipproto.Proto, 0, len(r.IPProto))
 			for _, n := range r.IPProto {
 				if n >= 0 && n <= 0xff {
-					m.IPProto = append(m.IPProto, ipproto.Proto(n))
+					filtered = append(filtered, ipproto.Proto(n))
 				}
 			}
+			m.IPProto = views.SliceOf(filtered)
 		}
 
 		for i, s := range r.SrcIPs {