kube/egressservices: improve egress ports config readability (#13722)

Instead of converting our PortMap struct to a string during marshalling
for use as a key, convert the whole collection of PortMaps to a list of
PortMap objects, which improves the readability of the JSON config while
still keeping the data structure we need in the code.

Updates #13406

Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>

kube/egressservices/egressservices.go

@@ -9,11 +9,8 @@
 package egressservices
 
 import (
-	"encoding"
+	"encoding/json"
-	"fmt"
 	"net/netip"
-	"strconv"
-	"strings"
 )
 
 // KeyEgressServices is name of the proxy state Secret field that contains the
@@ -31,7 +28,7 @@ type Config struct {
 	// should be proxied.
 	TailnetTarget TailnetTarget `json:"tailnetTarget"`
 	// Ports contains mappings for ports that can be accessed on the tailnet target.
-	Ports map[PortMap]struct{} `json:"ports"`
+	Ports PortMaps `json:"ports"`
 }
 
 // TailnetTarget is the tailnet target to which traffic for the egress service
@@ -52,35 +49,38 @@ type PortMap struct {
 	TargetPort uint16 `json:"targetPort"`
 }
 
-// PortMap is used as a Config.Ports map key. Config needs to be serialized/deserialized to/from JSON. JSON only
+type PortMaps map[PortMap]struct{}
-// supports string map keys, so we need to implement TextMarshaler/TextUnmarshaler to convert PortMap to string and
-// back.
-var _ encoding.TextMarshaler = PortMap{}
-var _ encoding.TextUnmarshaler = &PortMap{}
 
-func (pm *PortMap) UnmarshalText(t []byte) error {
+// PortMaps is a list of PortMap structs, however, we want to use it as a set
-	tt := string(t)
+// with efficient lookups in code. It implements custom JSON marshalling
-	ss := strings.Split(tt, ":")
+// methods to convert between being a list in JSON and a set (map with empty
-	if len(ss) != 3 {
+// values) in code.
-		return fmt.Errorf("error unmarshalling portmap from JSON, wants a portmap in form <protocol>:<matchPort>:<targetPor>, got %q", tt)
+var _ json.Marshaler = &PortMaps{}
+var _ json.Marshaler = PortMaps{}
+var _ json.Unmarshaler = &PortMaps{}
+
+func (p *PortMaps) UnmarshalJSON(data []byte) error {
+	*p = make(map[PortMap]struct{})
+
+	var l []PortMap
+	if err := json.Unmarshal(data, &l); err != nil {
+		return err
 	}
-	pm.Protocol = ss[0]
+
-	matchPort, err := strconv.ParseUint(ss[1], 10, 16)
+	for _, pm := range l {
-	if err != nil {
+		(*p)[pm] = struct{}{}
-		return fmt.Errorf("error converting match port %q to uint16: %w", ss[1], err)
 	}
-	pm.MatchPort = uint16(matchPort)
+
-	targetPort, err := strconv.ParseUint(ss[2], 10, 16)
-	if err != nil {
-		return fmt.Errorf("error converting target port %q to uint16: %w", ss[2], err)
-	}
-	pm.TargetPort = uint16(targetPort)
 	return nil
 }
 
-func (pm PortMap) MarshalText() ([]byte, error) {
+func (p PortMaps) MarshalJSON() ([]byte, error) {
-	s := fmt.Sprintf("%s:%d:%d", pm.Protocol, pm.MatchPort, pm.TargetPort)
+	l := make([]PortMap, 0, len(p))
-	return []byte(s), nil
+	for pm := range p {
+		l = append(l, pm)
+	}
+
+	return json.Marshal(l)
 }
 
 // Status represents the currently configured firewall rules for all egress
@@ -94,7 +94,7 @@ type Status struct {
 // ServiceStatus is the currently configured firewall rules for an egress
 // service.
 type ServiceStatus struct {
-	Ports map[PortMap]struct{} `json:"ports"`
+	Ports PortMaps `json:"ports"`
 	// TailnetTargetIPs are the tailnet target IPs that were used to
 	// configure these firewall rules. For a TailnetTarget with IP set, this
 	// is the same as IP.

kube/egressservices/egressservices_test.go

@@ -5,8 +5,9 @@ package egressservices
 
 import (
 	"encoding/json"
-	"reflect"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func Test_jsonUnmarshalConfig(t *testing.T) {
@@ -18,7 +19,7 @@ func Test_jsonUnmarshalConfig(t *testing.T) {
 	}{
 		{
 			name:     "success",
-			bs:       []byte(`{"ports":{"tcp:4003:80":{}}}`),
+			bs:       []byte(`{"ports":[{"protocol":"tcp","matchPort":4003,"targetPort":80}]}`),
 			wantsCfg: Config{Ports: map[PortMap]struct{}{{Protocol: "tcp", MatchPort: 4003, TargetPort: 80}: {}}},
 		},
 		{
@@ -34,8 +35,8 @@ func Test_jsonUnmarshalConfig(t *testing.T) {
 			if gotErr := json.Unmarshal(tt.bs, &cfg); (gotErr != nil) != tt.wantsErr {
 				t.Errorf("json.Unmarshal returned error %v, wants error %v", gotErr, tt.wantsErr)
 			}
-			if !reflect.DeepEqual(cfg, tt.wantsCfg) {
+			if diff := cmp.Diff(cfg, tt.wantsCfg); diff != "" {
-				t.Errorf("json.Unmarshal produced Config %v, wants Config %v", cfg, tt.wantsCfg)
+				t.Errorf("unexpected secrets (-got +want):\n%s", diff)
 			}
 		})
 	}
@@ -54,12 +55,12 @@ func Test_jsonMarshalConfig(t *testing.T) {
 			protocol:   "tcp",
 			matchPort:  4003,
 			targetPort: 80,
-			wantsBs:    []byte(`{"tailnetTarget":{"ip":"","fqdn":""},"ports":{"tcp:4003:80":{}}}`),
+			wantsBs:    []byte(`{"tailnetTarget":{"ip":"","fqdn":""},"ports":[{"protocol":"tcp","matchPort":4003,"targetPort":80}]}`),
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			cfg := Config{Ports: map[PortMap]struct{}{{
+			cfg := Config{Ports: PortMaps{{
 				Protocol:   tt.protocol,
 				MatchPort:  tt.matchPort,
 				TargetPort: tt.targetPort}: {}}}
@@ -68,8 +69,8 @@ func Test_jsonMarshalConfig(t *testing.T) {
 			if gotErr != nil {
 				t.Errorf("json.Marshal(%+#v) returned unexpected error %v", cfg, gotErr)
 			}
-			if !reflect.DeepEqual(gotBs, tt.wantsBs) {
+			if diff := cmp.Diff(gotBs, tt.wantsBs); diff != "" {
-				t.Errorf("json.Marshal(%+#v) returned '%v', wants '%v'", cfg, string(gotBs), string(tt.wantsBs))
+				t.Errorf("unexpected secrets (-got +want):\n%s", diff)
 			}
 		})
 	}