Merge pull request #118680 from danwinship/not-that-kind-of-proxying

move an apiserver helper function out of pkg/proxy

pkg/proxy/util/utils.go

@@ -19,7 +19,6 @@ package util
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"net"
 	"strconv"
@@ -45,14 +44,6 @@ const (
 	IPv6ZeroCIDR = "::/0"
 )
 
-var (
-	// ErrAddressNotAllowed indicates the address is not allowed
-	ErrAddressNotAllowed = errors.New("address not allowed")
-
-	// ErrNoAddresses indicates there are no addresses for the hostname
-	ErrNoAddresses = errors.New("no addresses for hostname")
-)
-
 // isValidEndpoint checks that the given host / port pair are valid endpoint
 func isValidEndpoint(host string, port int) bool {
 	return host != "" && port > 0
@@ -95,46 +86,11 @@ func IsLoopBack(ip string) bool {
 	return false
 }
 
-// IsProxyableIP checks if a given IP address is permitted to be proxied
-func IsProxyableIP(ip string) error {
-	netIP := netutils.ParseIPSloppy(ip)
-	if netIP == nil {
-		return ErrAddressNotAllowed
-	}
-	return isProxyableIP(netIP)
-}
-
-func isProxyableIP(ip net.IP) error {
-	if !ip.IsGlobalUnicast() {
-		return ErrAddressNotAllowed
-	}
-	return nil
-}
-
 // Resolver is an interface for net.Resolver
 type Resolver interface {
 	LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, error)
 }
 
-// IsProxyableHostname checks if the IP addresses for a given hostname are permitted to be proxied
-func IsProxyableHostname(ctx context.Context, resolv Resolver, hostname string) error {
-	resp, err := resolv.LookupIPAddr(ctx, hostname)
-	if err != nil {
-		return err
-	}
-
-	if len(resp) == 0 {
-		return ErrNoAddresses
-	}
-
-	for _, host := range resp {
-		if err := isProxyableIP(host.IP); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // GetLocalAddrs returns a list of all network addresses on the local system
 func GetLocalAddrs() ([]net.IP, error) {
 	var localAddrs []net.IP
@@ -227,7 +183,7 @@ func MapIPsByIPFamily(ipStrings []string) map[v1.IPFamily][]string {
 	ipFamilyMap := map[v1.IPFamily][]string{}
 	for _, ip := range ipStrings {
 		// Handle only the valid IPs
-		if ipFamily, err := getIPFamilyFromIP(ip); err == nil {
+		if ipFamily := getIPFamilyFromIP(ip); ipFamily != "" {
 			ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ip)
 		} else {
 			// this function is called in multiple places. All of which
@@ -249,7 +205,7 @@ func MapCIDRsByIPFamily(cidrStrings []string) map[v1.IPFamily][]string {
 	ipFamilyMap := map[v1.IPFamily][]string{}
 	for _, cidr := range cidrStrings {
 		// Handle only the valid CIDRs
-		if ipFamily, err := getIPFamilyFromCIDR(cidr); err == nil {
+		if ipFamily := getIPFamilyFromCIDR(cidr); ipFamily != "" {
 			ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], cidr)
 		} else {
 			klog.ErrorS(nil, "Skipping invalid CIDR", "cidr", cidr)
@@ -258,27 +214,29 @@ func MapCIDRsByIPFamily(cidrStrings []string) map[v1.IPFamily][]string {
 	return ipFamilyMap
 }
 
-func getIPFamilyFromIP(ipStr string) (v1.IPFamily, error) {
+// Returns the IP family of ipStr, or "" if ipStr can't be parsed as an IP
+func getIPFamilyFromIP(ipStr string) v1.IPFamily {
 	netIP := netutils.ParseIPSloppy(ipStr)
 	if netIP == nil {
-		return "", ErrAddressNotAllowed
+		return ""
 	}
 
 	if netutils.IsIPv6(netIP) {
-		return v1.IPv6Protocol, nil
+		return v1.IPv6Protocol
 	}
-	return v1.IPv4Protocol, nil
+	return v1.IPv4Protocol
 }
 
-func getIPFamilyFromCIDR(cidrStr string) (v1.IPFamily, error) {
+// Returns the IP family of cidrStr, or "" if cidrStr can't be parsed as a CIDR
+func getIPFamilyFromCIDR(cidrStr string) v1.IPFamily {
 	_, netCIDR, err := netutils.ParseCIDRSloppy(cidrStr)
 	if err != nil {
-		return "", ErrAddressNotAllowed
+		return ""
 	}
 	if netutils.IsIPv6CIDR(netCIDR) {
-		return v1.IPv6Protocol, nil
+		return v1.IPv6Protocol
 	}
-	return v1.IPv4Protocol, nil
+	return v1.IPv4Protocol
 }
 
 // OtherIPFamily returns the other ip family

pkg/proxy/util/utils_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package util
 
 import (
-	"context"
 	"math/rand"
 	"net"
 	"reflect"
@@ -96,76 +95,6 @@ func TestBuildPortsToEndpointsMap(t *testing.T) {
 	}
 }
 
-func TestIsProxyableIP(t *testing.T) {
-	testCases := []struct {
-		ip   string
-		want error
-	}{
-		{"0.0.0.0", ErrAddressNotAllowed},
-		{"127.0.0.1", ErrAddressNotAllowed},
-		{"127.0.0.2", ErrAddressNotAllowed},
-		{"169.254.169.254", ErrAddressNotAllowed},
-		{"169.254.1.1", ErrAddressNotAllowed},
-		{"224.0.0.0", ErrAddressNotAllowed},
-		{"10.0.0.1", nil},
-		{"192.168.0.1", nil},
-		{"172.16.0.1", nil},
-		{"8.8.8.8", nil},
-		{"::", ErrAddressNotAllowed},
-		{"::1", ErrAddressNotAllowed},
-		{"fe80::", ErrAddressNotAllowed},
-		{"ff02::", ErrAddressNotAllowed},
-		{"ff01::", ErrAddressNotAllowed},
-		{"2600::", nil},
-		{"1", ErrAddressNotAllowed},
-		{"", ErrAddressNotAllowed},
-	}
-
-	for i := range testCases {
-		got := IsProxyableIP(testCases[i].ip)
-		if testCases[i].want != got {
-			t.Errorf("case %d: expected %v, got %v", i, testCases[i].want, got)
-		}
-	}
-}
-
-type dummyResolver struct {
-	ips []string
-	err error
-}
-
-func (r *dummyResolver) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, error) {
-	if r.err != nil {
-		return nil, r.err
-	}
-	resp := []net.IPAddr{}
-	for _, ipString := range r.ips {
-		resp = append(resp, net.IPAddr{IP: netutils.ParseIPSloppy(ipString)})
-	}
-	return resp, nil
-}
-
-func TestIsProxyableHostname(t *testing.T) {
-	testCases := []struct {
-		hostname string
-		ips      []string
-		want     error
-	}{
-		{"k8s.io", []string{}, ErrNoAddresses},
-		{"k8s.io", []string{"8.8.8.8"}, nil},
-		{"k8s.io", []string{"169.254.169.254"}, ErrAddressNotAllowed},
-		{"k8s.io", []string{"127.0.0.1", "8.8.8.8"}, ErrAddressNotAllowed},
-	}
-
-	for i := range testCases {
-		resolv := dummyResolver{ips: testCases[i].ips}
-		got := IsProxyableHostname(context.Background(), &resolv, testCases[i].hostname)
-		if testCases[i].want != got {
-			t.Errorf("case %d: expected %v, got %v", i, testCases[i].want, got)
-		}
-	}
-}
-
 func TestShouldSkipService(t *testing.T) {
 	testCases := []struct {
 		service    *v1.Service

pkg/registry/core/node/storage/storage_test.go

@@ -32,7 +32,6 @@ import (
 	etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	kubeletclient "k8s.io/kubernetes/pkg/kubelet/client"
-	proxyutil "k8s.io/kubernetes/pkg/proxy/util"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
 )
 
@@ -192,7 +191,7 @@ func TestResourceLocation(t *testing.T) {
 		node  *api.Node
 		query string
 		host  string
-		err   error
+		err   bool
 	}
 
 	testCases := []testCase{{
@@ -215,19 +214,19 @@ func TestResourceLocation(t *testing.T) {
 		node:  newNode("node0", setNodeIPAddress("127.0.0.1")),
 		query: "node0",
 		host:  "",
-		err:   proxyutil.ErrAddressNotAllowed,
+		err:   true,
 	}, {
 		name:  "non-proxyable hostname with kubelet port in query",
 		node:  newNode("node0", setNodeIPAddress("127.0.0.1")),
 		query: "node0:5000",
 		host:  "",
-		err:   proxyutil.ErrAddressNotAllowed,
+		err:   true,
 	}, {
 		name:  "non-proxyable hostname with kubelet port in status",
 		node:  newNode("node0", setNodeIPAddress("127.0.0.1"), setNodeDaemonEndpoint(443)),
 		query: "node0",
 		host:  "",
-		err:   proxyutil.ErrAddressNotAllowed,
+		err:   true,
 	}}
 
 	for _, testCase := range testCases {
@@ -245,18 +244,13 @@ func TestResourceLocation(t *testing.T) {
 			redirector := rest.Redirector(storage)
 			location, _, err := redirector.ResourceLocation(ctx, testCase.query)
 
-			if err != nil && testCase.err != nil {
+			if err != nil {
-				if err.Error() != testCase.err.Error() {
+				if !testCase.err {
-					t.Fatalf("Unexpected error: %v, expected: %v", err, testCase.err)
+					t.Fatalf("Unexpected error: %v", err)
 				}
-
 				return
-			}
+			} else if testCase.err {
-
+				t.Fatalf("Expected error but got none")
-			if err != nil && testCase.err == nil {
-				t.Fatalf("Unexpected error: %v, expected: %v", err, testCase.err)
-			} else if err == nil && testCase.err != nil {
-				t.Fatalf("Expected error but got none, err: %v", testCase.err)
 			}
 
 			if location == nil {

pkg/registry/core/node/strategy.go

@@ -38,7 +38,6 @@ import (
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/kubelet/client"
-	proxyutil "k8s.io/kubernetes/pkg/proxy/util"
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 )
 
@@ -241,7 +240,7 @@ func ResourceLocation(getter ResourceGetter, connection client.ConnectionInfoGet
 		return nil, nil, err
 	}
 
-	if err := proxyutil.IsProxyableHostname(ctx, &net.Resolver{}, info.Hostname); err != nil {
+	if err := isProxyableHostname(ctx, info.Hostname); err != nil {
 		return nil, nil, errors.NewBadRequest(err.Error())
 	}
 
@@ -261,6 +260,24 @@ func ResourceLocation(getter ResourceGetter, connection client.ConnectionInfoGet
 	return &url.URL{Scheme: schemeReq, Host: net.JoinHostPort(info.Hostname, portReq)}, proxyTransport, nil
 }
 
+func isProxyableHostname(ctx context.Context, hostname string) error {
+	resp, err := net.DefaultResolver.LookupIPAddr(ctx, hostname)
+	if err != nil {
+		return err
+	}
+
+	if len(resp) == 0 {
+		return fmt.Errorf("no addresses for hostname")
+	}
+	for _, host := range resp {
+		if !host.IP.IsGlobalUnicast() {
+			return fmt.Errorf("address not allowed")
+		}
+	}
+
+	return nil
+}
+
 func fieldIsDeprecatedWarnings(obj runtime.Object) []string {
 	newNode := obj.(*api.Node)
 	var warnings []string

pkg/registry/core/pod/strategy.go

@@ -47,7 +47,7 @@ import (
 	corevalidation "k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/kubelet/client"
-	proxyutil "k8s.io/kubernetes/pkg/proxy/util"
+	netutils "k8s.io/utils/net"
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 )
 
@@ -398,8 +398,8 @@ func ResourceLocation(ctx context.Context, getter ResourceGetter, rt http.RoundT
 		}
 	}
 	podIP := getPodIP(pod)
-	if err := proxyutil.IsProxyableIP(podIP); err != nil {
+	if ip := netutils.ParseIPSloppy(podIP); ip == nil || !ip.IsGlobalUnicast() {
-		return nil, nil, errors.NewBadRequest(err.Error())
+		return nil, nil, errors.NewBadRequest("address not allowed")
 	}
 
 	loc := &url.URL{