From 5bde9404a09e1dcfb19131e582dba32af631b4b8 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 15 Jun 2023 05:22:11 -0400 Subject: [PATCH 1/2] Remove unused error return value from internal function --- pkg/proxy/util/utils.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index b0bfc2b625c..2135b912563 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -227,7 +227,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 +249,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 +258,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 From bb0c3a08189ea523e0045bc021c9cdf60c037d45 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 18 May 2023 17:52:02 -0400 Subject: [PATCH 2/2] Remove proxyutil.IsProxyableIP / IsProxyableHostname These don't belong in pkg/proxy/util; they involve a completely unrelated definition of proxying. Since each is only used from one place, just inline them at the callers. --- pkg/proxy/util/utils.go | 44 ------------ pkg/proxy/util/utils_test.go | 71 ------------------- .../core/node/storage/storage_test.go | 24 +++---- pkg/registry/core/node/strategy.go | 21 +++++- pkg/registry/core/pod/strategy.go | 6 +- 5 files changed, 31 insertions(+), 135 deletions(-) diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index 2135b912563..bda706ce657 100644 --- a/pkg/proxy/util/utils.go +++ b/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 diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index 49c3528cf20..4209d113130 100644 --- a/pkg/proxy/util/utils_test.go +++ b/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 diff --git a/pkg/registry/core/node/storage/storage_test.go b/pkg/registry/core/node/storage/storage_test.go index e32abea17bf..52887c68b82 100644 --- a/pkg/registry/core/node/storage/storage_test.go +++ b/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.Error() != testCase.err.Error() { - t.Fatalf("Unexpected error: %v, expected: %v", err, testCase.err) + if err != nil { + if !testCase.err { + t.Fatalf("Unexpected error: %v", err) } - return - } - - 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) + } else if testCase.err { + t.Fatalf("Expected error but got none") } if location == nil { diff --git a/pkg/registry/core/node/strategy.go b/pkg/registry/core/node/strategy.go index b2492d74281..a6900429ff8 100644 --- a/pkg/registry/core/node/strategy.go +++ b/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 diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index ec5e2195c6d..63bc0b52432 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/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 { - return nil, nil, errors.NewBadRequest(err.Error()) + if ip := netutils.ParseIPSloppy(podIP); ip == nil || !ip.IsGlobalUnicast() { + return nil, nil, errors.NewBadRequest("address not allowed") } loc := &url.URL{