Include node-external-ip in serving-kubelet.crt SANs (#4620)

* Include node-external-ip in serving-kubelet.crt SANs

Signed-off-by: Alexey Medvedchikov <alexeymedvedchikov@improbable.io>

pkg/agent/config/config.go

@@ -352,6 +352,11 @@ func get(ctx context.Context, envInfo *cmds.Agent, proxy proxy.Proxy) (*config.N
 		return nil, err
 	}
 
+	nodeExternalIPs, err := util.ParseStringSliceToIPs(envInfo.NodeExternalIP)
+	if err != nil {
+		return nil, fmt.Errorf("invalid node-external-ip: %w", err)
+	}
+
 	if envInfo.WithNodeID {
 		nodeID, err := ensureNodeID(filepath.Join(nodeConfigPath, "id"))
 		if err != nil {
@@ -362,7 +367,8 @@ func get(ctx context.Context, envInfo *cmds.Agent, proxy proxy.Proxy) (*config.N
 
 	os.Setenv("NODE_NAME", nodeName)
 
-	servingCert, err := getServingCert(nodeName, nodeIPs, servingKubeletCert, servingKubeletKey, newNodePasswordFile, info)
+	nodeExternalAndInternalIPs := append(nodeIPs, nodeExternalIPs...)
+	servingCert, err := getServingCert(nodeName, nodeExternalAndInternalIPs, servingKubeletCert, servingKubeletKey, newNodePasswordFile, info)
 	if err != nil {
 		return nil, err
 	}
@@ -464,16 +470,7 @@ func get(ctx context.Context, envInfo *cmds.Agent, proxy proxy.Proxy) (*config.N
 		return nil, errors.Wrap(err, "cannot configure IPv4 node-ip")
 	}
 	nodeConfig.AgentConfig.NodeIP = nodeIP.String()
-
-	for _, externalIP := range envInfo.NodeExternalIP {
-		for _, v := range strings.Split(externalIP, ",") {
-			ip := net.ParseIP(v)
-			if ip == nil {
-				return nil, fmt.Errorf("invalid node-external-ip %s", v)
-			}
-			nodeConfig.AgentConfig.NodeExternalIPs = append(nodeConfig.AgentConfig.NodeExternalIPs, ip)
-		}
-	}
+	nodeConfig.AgentConfig.NodeExternalIPs = nodeExternalIPs
 
 	// if configured, set NodeExternalIP to the first IPv4 address, for legacy clients
 	if len(nodeConfig.AgentConfig.NodeExternalIPs) > 0 {

pkg/util/net.go

@@ -103,14 +103,10 @@ func GetHostnameAndIPs(name string, nodeIPs cli.StringSlice) (string, []net.IP,
 		}
 		ips = append(ips, hostIP)
 	} else {
-		for _, hostIP := range nodeIPs {
-			for _, v := range strings.Split(hostIP, ",") {
-				ip := net.ParseIP(v)
-				if ip == nil {
-					return "", nil, fmt.Errorf("invalid node-ip %s", v)
-				}
-				ips = append(ips, ip)
-			}
+		var err error
+		ips, err = ParseStringSliceToIPs(nodeIPs)
+		if err != nil {
+			return "", nil, fmt.Errorf("invalid node-ip: %w", err)
 		}
 	}
 
@@ -128,3 +124,20 @@ func GetHostnameAndIPs(name string, nodeIPs cli.StringSlice) (string, []net.IP,
 
 	return name, ips, nil
 }
+
+// ParseStringSliceToIPs converts slice of strings that in turn can be lists of comma separated unparsed IP addresses
+// into a single slice of net.IP, it returns error if at any point parsing failed
+func ParseStringSliceToIPs(s cli.StringSlice) ([]net.IP, error) {
+	var ips []net.IP
+	for _, unparsedIP := range s {
+		for _, v := range strings.Split(unparsedIP, ",") {
+			ip := net.ParseIP(v)
+			if ip == nil {
+				return nil, fmt.Errorf("invalid ip format '%s'", v)
+			}
+			ips = append(ips, ip)
+		}
+	}
+
+	return ips, nil
+}

pkg/util/net_test.go

@@ -0,0 +1,84 @@
+package util
+
+import (
+	"net"
+	"reflect"
+	"testing"
+
+	"github.com/urfave/cli"
+)
+
+func Test_UnitParseStringSliceToIPs(t *testing.T) {
+	tests := []struct {
+		name    string
+		arg     cli.StringSlice
+		want    []net.IP
+		wantErr bool
+	}{
+		{
+			name: "nil string slice must return no errors",
+			arg:  nil,
+			want: nil,
+		},
+		{
+			name: "empty string slice must return no errors",
+			arg:  cli.StringSlice{},
+			want: nil,
+		},
+		{
+			name: "single element slice with correct IP must succeed",
+			arg:  cli.StringSlice{"10.10.10.10"},
+			want: []net.IP{net.ParseIP("10.10.10.10")},
+		},
+		{
+			name: "single element slice with correct IP list must succeed",
+			arg:  cli.StringSlice{"10.10.10.10,10.10.10.11"},
+			want: []net.IP{
+				net.ParseIP("10.10.10.10"),
+				net.ParseIP("10.10.10.11"),
+			},
+		},
+		{
+			name: "multi element slice with correct IP list must succeed",
+			arg:  cli.StringSlice{"10.10.10.10,10.10.10.11", "10.10.10.12,10.10.10.13"},
+			want: []net.IP{
+				net.ParseIP("10.10.10.10"),
+				net.ParseIP("10.10.10.11"),
+				net.ParseIP("10.10.10.12"),
+				net.ParseIP("10.10.10.13"),
+			},
+		},
+		{
+			name:    "single element slice with correct IP list with trailing comma must fail",
+			arg:     cli.StringSlice{"10.10.10.10,"},
+			want:    nil,
+			wantErr: true,
+		},
+		{
+			name:    "single element slice with incorrect IP (overflow) must fail",
+			arg:     cli.StringSlice{"10.10.10.256"},
+			want:    nil,
+			wantErr: true,
+		},
+		{
+			name:    "single element slice with incorrect IP (foreign symbols) must fail",
+			arg:     cli.StringSlice{"xxx.yyy.zzz.www"},
+			want:    nil,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(
+			tt.name, func(t *testing.T) {
+				got, err := ParseStringSliceToIPs(tt.arg)
+				if (err != nil) != tt.wantErr {
+					t.Errorf("ParseStringSliceToIPs() error = %v, wantErr %v", err, tt.wantErr)
+					return
+				}
+				if !reflect.DeepEqual(got, tt.want) {
+					t.Errorf("ParseStringSliceToIPs() = %v, want %v", got, tt.want)
+				}
+			},
+		)
+	}
+}