Merge pull request #54191 from MrHohn/kube-proxy-metrics-flag-fix

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kube-proxy: Fix flag validation for healthz-bind-address and metrics-bind-address

**What this PR does / why we need it**: `--healthz-bind-address` and `--metrics-bind-address` are broken for kube-proxy as they do not allow `ip:port` format, though they claim to support it.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: Fixes #53754

**Special notes for your reviewer**:
cc @ncdc 

**Release note**:

```release-note
Fix kube-proxy flags validation for --healthz-bind-address and --metrics-bind-address to allow specifying ip:port.
```

cmd/kube-proxy/app/server.go

@@ -129,8 +129,8 @@ func AddFlags(options *Options, fs *pflag.FlagSet) {
 	fs.Var(componentconfig.IPVar{Val: &options.config.BindAddress}, "bind-address", "The IP address for the proxy server to serve on (set to `0.0.0.0` for all IPv4 interfaces and `::` for all IPv6 interfaces)")
 	fs.StringVar(&options.master, "master", options.master, "The address of the Kubernetes API server (overrides any value in kubeconfig)")
 	fs.Int32Var(&options.healthzPort, "healthz-port", options.healthzPort, "The port to bind the health check server. Use 0 to disable.")
-	fs.Var(componentconfig.IPVar{Val: &options.config.HealthzBindAddress}, "healthz-bind-address", "The IP address and port for the health check server to serve on (set to `0.0.0.0` for all IPv4 interfaces and `::` for all IPv6 interfaces)")
-	fs.Var(componentconfig.IPVar{Val: &options.config.MetricsBindAddress}, "metrics-bind-address", "The IP address and port for the metrics server to serve on (set to `0.0.0.0` for all IPv4 interfaces and `::` for all IPv6 interfaces)")
+	fs.Var(componentconfig.IPPortVar{Val: &options.config.HealthzBindAddress}, "healthz-bind-address", "The IP address and port for the health check server to serve on (set to `0.0.0.0` for all IPv4 interfaces and `::` for all IPv6 interfaces)")
+	fs.Var(componentconfig.IPPortVar{Val: &options.config.MetricsBindAddress}, "metrics-bind-address", "The IP address and port for the metrics server to serve on (set to `0.0.0.0` for all IPv4 interfaces and `::` for all IPv6 interfaces)")
 	fs.Int32Var(options.config.OOMScoreAdj, "oom-score-adj", utilpointer.Int32PtrDerefOr(options.config.OOMScoreAdj, int32(qos.KubeProxyOOMScoreAdj)), "The oom-score-adj value for kube-proxy process. Values must be within the range [-1000, 1000]")
 	fs.StringVar(&options.config.ResourceContainer, "resource-container", options.config.ResourceContainer, "Absolute name of the resource-only container to create and run the Kube-proxy in (Default: /kube-proxy).")
 	fs.MarkDeprecated("resource-container", "This feature will be removed in a later release.")

pkg/apis/componentconfig/helpers.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net"
+	"strconv"
 
 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -61,6 +62,55 @@ func (v IPVar) Type() string {
 	return "ip"
 }
 
+// IPPortVar allows IP or IP:port formats.
+type IPPortVar struct {
+	Val *string
+}
+
+func (v IPPortVar) Set(s string) error {
+	if len(s) == 0 {
+		v.Val = nil
+		return nil
+	}
+
+	if v.Val == nil {
+		// it's okay to panic here since this is programmer error
+		panic("the string pointer passed into IPPortVar should not be nil")
+	}
+
+	// Both IP and IP:port are valid.
+	// Attempt to parse into IP first.
+	if net.ParseIP(s) != nil {
+		*v.Val = s
+		return nil
+	}
+
+	// Can not parse into IP, now assume IP:port.
+	host, port, err := net.SplitHostPort(s)
+	if err != nil {
+		return fmt.Errorf("%q is not in a valid format (ip or ip:port): %v", s, err)
+	}
+	if net.ParseIP(host) == nil {
+		return fmt.Errorf("%q is not a valid IP address", host)
+	}
+	if _, err := strconv.Atoi(port); err != nil {
+		return fmt.Errorf("%q is not a valid number", port)
+	}
+	*v.Val = s
+	return nil
+}
+
+func (v IPPortVar) String() string {
+	if v.Val == nil {
+		return ""
+	}
+	return *v.Val
+}
+
+func (v IPPortVar) Type() string {
+	return "ipport"
+}
+
 type PortRangeVar struct {
 	Val *string
 }

pkg/apis/componentconfig/helpers_test.go

@@ -25,7 +25,7 @@ import (
 
 func TestIPVar(t *testing.T) {
 	defaultIP := "0.0.0.0"
-	cases := []struct {
+	testCases := []struct {
 		argc      string
 		expectErr bool
 		expectVal string
@@ -41,7 +41,7 @@ func TestIPVar(t *testing.T) {
 			expectVal: defaultIP,
 		},
 	}
-	for _, c := range cases {
+	for _, tc := range testCases {
 		fs := pflag.NewFlagSet("blah", pflag.PanicOnError)
 		ip := defaultIP
 		fs.Var(IPVar{&ip}, "ip", "the ip")
@@ -53,19 +53,113 @@ func TestIPVar(t *testing.T) {
 					err = r.(error)
 				}
 			}()
-			fs.Parse(strings.Split(c.argc, " "))
+			fs.Parse(strings.Split(tc.argc, " "))
 		}()
 
-		if c.expectErr && err == nil {
+		if tc.expectErr && err == nil {
 			t.Errorf("did not observe an expected error")
 			continue
 		}
-		if !c.expectErr && err != nil {
-			t.Errorf("observed an unexpected error")
+		if !tc.expectErr && err != nil {
+			t.Errorf("observed an unexpected error: %v", err)
 			continue
 		}
-		if c.expectVal != ip {
-			t.Errorf("unexpected ip: expected %q, saw %q", c.expectVal, ip)
+		if tc.expectVal != ip {
+			t.Errorf("unexpected ip: expected %q, saw %q", tc.expectVal, ip)
+		}
+	}
+}
+
+func TestIPPortVar(t *testing.T) {
+	defaultIPPort := "0.0.0.0:8080"
+	testCases := []struct {
+		desc      string
+		argc      string
+		expectErr bool
+		expectVal string
+	}{
+
+		{
+			desc:      "valid ipv4 1",
+			argc:      "blah --ipport=0.0.0.0",
+			expectVal: "0.0.0.0",
+		},
+		{
+			desc:      "valid ipv4 2",
+			argc:      "blah --ipport=127.0.0.1",
+			expectVal: "127.0.0.1",
+		},
+
+		{
+			desc:      "invalid IP",
+			argc:      "blah --ipport=invalidip",
+			expectErr: true,
+			expectVal: defaultIPPort,
+		},
+		{
+			desc:      "valid ipv4 with port",
+			argc:      "blah --ipport=0.0.0.0:8080",
+			expectVal: "0.0.0.0:8080",
+		},
+		{
+			desc:      "invalid ipv4 with invalid port",
+			argc:      "blah --ipport=0.0.0.0:invalidport",
+			expectErr: true,
+			expectVal: defaultIPPort,
+		},
+		{
+			desc:      "invalid IP with port",
+			argc:      "blah --ipport=invalidip:8080",
+			expectErr: true,
+			expectVal: defaultIPPort,
+		},
+		{
+			desc:      "valid ipv6 1",
+			argc:      "blah --ipport=::1",
+			expectVal: "::1",
+		},
+		{
+			desc:      "valid ipv6 2",
+			argc:      "blah --ipport=::",
+			expectVal: "::",
+		},
+		{
+			desc:      "valid ipv6 with port",
+			argc:      "blah --ipport=[::1]:8080",
+			expectVal: "[::1]:8080",
+		},
+		{
+			desc:      "invalid ipv6 with port without bracket",
+			argc:      "blah --ipport=fd00:f00d:600d:f00d:8080",
+			expectErr: true,
+			expectVal: defaultIPPort,
+		},
+	}
+	for _, tc := range testCases {
+		fs := pflag.NewFlagSet("blah", pflag.PanicOnError)
+		ipport := defaultIPPort
+		fs.Var(IPPortVar{&ipport}, "ipport", "the ip:port")
+
+		var err error
+		func() {
+			defer func() {
+				if r := recover(); r != nil {
+					err = r.(error)
+				}
+			}()
+			fs.Parse(strings.Split(tc.argc, " "))
+		}()
+
+		if tc.expectErr && err == nil {
+			t.Errorf("%q: Did not observe an expected error", tc.desc)
+			continue
+		}
+		if !tc.expectErr && err != nil {
+			t.Errorf("%q: Observed an unexpected error: %v", tc.desc, err)
+			continue
+		}
+		if tc.expectVal != ipport {
+			t.Errorf("%q: Unexpected ipport: expected %q, saw %q", tc.desc, tc.expectVal, ipport)
 		}
 	}
 }

pkg/apis/componentconfig/zz_generated.deepcopy.go

@@ -56,6 +56,31 @@ func (in *GroupResource) DeepCopy() *GroupResource {
 	return out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *IPPortVar) DeepCopyInto(out *IPPortVar) {
+	*out = *in
+	if in.Val != nil {
+		in, out := &in.Val, &out.Val
+		if *in == nil {
+			*out = nil
+		} else {
+			*out = new(string)
+			**out = **in
+		}
+	}
+	return
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new IPPortVar.
+func (in *IPPortVar) DeepCopy() *IPPortVar {
+	if in == nil {
+		return nil
+	}
+	out := new(IPPortVar)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *IPVar) DeepCopyInto(out *IPVar) {
 	*out = *in