refactor(k8s): resolve node public IPs from the node object, restoring the old signatures

Cloud controller managers record an instance's public IP as an ExternalIP
address on the Node object, but findDefaultNodeHostnameE only ever matched
NodeHostName, so that address was never used. Both FindNodeHostname doc
comments have promised to prefer the external IP since 2018.

Preferring ExternalIP means the common case needs no cloud API call and no
configuration, which removes the reason to change the public signatures.
FindNodeHostnameContext and FindNodeHostnameContextE go back to their
original shape, so existing callers compile and behave as before, and the
ec2:DescribeInstances permission is no longer required.

NodePublicIPLookup stays as an escape hatch for clusters that do not
advertise an ExternalIP, reached through the new FindNodeHostnameWithOptions
functions. GetServiceEndpoint routes through those, so it keeps the lookup
when one is configured.

Author: james00012

modules/k8s/kubectl_options.go

@@ -15,8 +15,12 @@ const awsProviderIDScheme = "aws"
 // NodePublicIPLookup resolves the public IP addresses of the given cloud provider instance IDs in the given region,
 // returning a map of instance ID to public IP. Instances with no public IP may be omitted from the map.
 //
-// This exists so that k8s can resolve the externally reachable address of a node without depending on any cloud
-// provider module. It matches the signature of aws.GetPublicIpsOfEc2InstancesContextE, so on EKS you can wire it up
+// This is an escape hatch, not the usual path. Node addresses recorded by a cloud controller manager already carry
+// the instance's public IP as an ExternalIP, and FindNodeHostnameContextE prefers that, so most clusters need no
+// lookup at all. Set one only when your cluster does not advertise an ExternalIP.
+//
+// It exists so that k8s can resolve a node's externally reachable address without depending on any cloud provider
+// module. It matches the signature of aws.GetPublicIpsOfEc2InstancesContextE, so on EKS you can wire it up
 // directly:
 //
 //	options := k8s.NewKubectlOptions("", "", "default")
@@ -26,8 +30,9 @@ type NodePublicIPLookup func(t testing.TestingT, ctx context.Context, instanceID
 // KubectlOptions represents common options necessary to specify for all Kubectl calls
 type KubectlOptions struct {
 	Env map[string]string
-	// NodePublicIPLookup, if set, is used to resolve the public IP of a node whose provider ID identifies a cloud
-	// instance. If it is nil, node lookups fall back to the internal hostname recorded on the Kubernetes node object.
+	// NodePublicIPLookup, if set, resolves the public IP of a node whose provider ID identifies a cloud instance and
+	// whose node object carries no ExternalIP address. It is consulted only by the FindNodeHostnameWithOptions
+	// functions, and only after the node's own ExternalIP has been checked, so most callers can leave it nil.
 	// It is skipped when serializing options, since a function cannot be represented as JSON.
 	NodePublicIPLookup NodePublicIPLookup `json:"-"`
 	RestConfig         *rest.Config

modules/k8s/node_hostname_test.go

@@ -31,6 +31,17 @@ func nodeWithProviderID(providerID string) corev1.Node {
 	}
 }
 
+// nodeWithExternalIP models what a cloud controller manager records for an instance that has a public IP.
+func nodeWithExternalIP(providerID string, externalIP string) corev1.Node {
+	node := nodeWithProviderID(providerID)
+	node.Status.Addresses = append(
+		[]corev1.NodeAddress{{Type: corev1.NodeExternalIP, Address: externalIP}},
+		node.Status.Addresses...,
+	)
+
+	return node
+}
+
 func TestFindNodeHostnameUsesPublicIPLookup(t *testing.T) {
 	t.Parallel()
 
@@ -45,7 +56,7 @@ func TestFindNodeHostnameUsesPublicIPLookup(t *testing.T) {
 		return map[string]string{testInstanceID: testPublicIP}, nil
 	}
 
-	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
+	hostname, err := k8s.FindNodeHostnameWithOptionsContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
 	require.NoError(t, err)
 
 	assert.Equal(t, testPublicIP, hostname)
@@ -58,7 +69,7 @@ func TestFindNodeHostnameFallsBackWhenNoLookupConfigured(t *testing.T) {
 
 	options := k8s.NewKubectlOptions("", "", "default")
 
-	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
+	hostname, err := k8s.FindNodeHostnameWithOptionsContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
 	require.NoError(t, err)
 
 	assert.Equal(t, testHostname, hostname, "should fall back to the internal hostname when no lookup is configured")
@@ -72,7 +83,7 @@ func TestFindNodeHostnameFallsBackWhenInstanceHasNoPublicIP(t *testing.T) {
 		return map[string]string{}, nil
 	}
 
-	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
+	hostname, err := k8s.FindNodeHostnameWithOptionsContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
 	require.NoError(t, err)
 
 	assert.Equal(t, testHostname, hostname)
@@ -88,7 +99,7 @@ func TestFindNodeHostnameSkipsLookupForNonAWSProvider(t *testing.T) {
 		return nil, nil
 	}
 
-	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), options, nodeWithProviderID("gce://project/us-central1-a/instance-1"))
+	hostname, err := k8s.FindNodeHostnameWithOptionsContextE(t, t.Context(), options, nodeWithProviderID("gce://project/us-central1-a/instance-1"))
 	require.NoError(t, err)
 
 	assert.Equal(t, testHostname, hostname)
@@ -104,7 +115,7 @@ func TestFindNodeHostnamePropagatesLookupError(t *testing.T) {
 		return nil, expectedErr
 	}
 
-	_, err := k8s.FindNodeHostnameContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
+	_, err := k8s.FindNodeHostnameWithOptionsContextE(t, t.Context(), options, nodeWithProviderID(testProviderID))
 	require.ErrorIs(t, err, expectedErr)
 }
 
@@ -144,10 +155,70 @@ func TestFindNodeHostnameRejectsMalformedAwsProviderIDs(t *testing.T) {
 			// The guard must fire before the region slice, with and without a lookup configured.
 			for _, opts := range []*k8s.KubectlOptions{nil, options} {
 				require.NotPanics(t, func() {
-					_, err := k8s.FindNodeHostnameContextE(t, t.Context(), opts, node)
+					_, err := k8s.FindNodeHostnameWithOptionsContextE(t, t.Context(), opts, node)
 					require.Error(t, err)
 				})
 			}
 		})
 	}
 }
+
+// TestFindNodeHostnamePrefersExternalIPFromNode is the primary path. Cloud controller managers record an
+// instance's public IP as an ExternalIP address on the node object, so no cloud API call is needed and no lookup
+// has to be configured.
+func TestFindNodeHostnamePrefersExternalIPFromNode(t *testing.T) {
+	t.Parallel()
+
+	node := nodeWithExternalIP(testProviderID, testPublicIP)
+
+	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), node)
+	require.NoError(t, err)
+	assert.Equal(t, testPublicIP, hostname, "the node's ExternalIP must win without any options")
+}
+
+// TestFindNodeHostnameExternalIPSkipsTheLookup pins that the cloud API call is not made when the node already
+// advertises an external IP, even if a lookup is configured.
+func TestFindNodeHostnameExternalIPSkipsTheLookup(t *testing.T) {
+	t.Parallel()
+
+	options := k8s.NewKubectlOptions("", "", "default")
+	options.NodePublicIPLookup = func(_ gotesting.TestingT, _ context.Context, _ []string, _ string) (map[string]string, error) {
+		t.Fatal("lookup must not be called when the node advertises an ExternalIP")
+
+		return nil, nil
+	}
+
+	hostname, err := k8s.FindNodeHostnameWithOptionsContextE(t, t.Context(), options, nodeWithExternalIP(testProviderID, testPublicIP))
+	require.NoError(t, err)
+	assert.Equal(t, testPublicIP, hostname)
+}
+
+// TestFindNodeHostnameExternalIPWorksForNonAwsProviders confirms the preference is provider agnostic, so GKE and
+// other clusters that advertise an ExternalIP get it too.
+func TestFindNodeHostnameExternalIPWorksForNonAwsProviders(t *testing.T) {
+	t.Parallel()
+
+	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), nodeWithExternalIP("gce://project/zone/instance", testPublicIP))
+	require.NoError(t, err)
+	assert.Equal(t, testPublicIP, hostname)
+}
+
+// TestFindNodeHostnameWithoutOptionsFallsBackToHostname covers an AWS node with no ExternalIP reached through the
+// options-free entry point: there is nothing to query with, so it degrades to the internal hostname.
+func TestFindNodeHostnameWithoutOptionsFallsBackToHostname(t *testing.T) {
+	t.Parallel()
+
+	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), nodeWithProviderID(testProviderID))
+	require.NoError(t, err)
+	assert.Equal(t, testHostname, hostname)
+}
+
+// TestFindNodeHostnameIgnoresEmptyExternalIP guards against an ExternalIP entry with a blank address shadowing the
+// rest of the resolution chain.
+func TestFindNodeHostnameIgnoresEmptyExternalIP(t *testing.T) {
+	t.Parallel()
+
+	hostname, err := k8s.FindNodeHostnameContextE(t, t.Context(), nodeWithExternalIP(testProviderID, ""))
+	require.NoError(t, err)
+	assert.Equal(t, testHostname, hostname, "a blank ExternalIP must not shadow the hostname")
+}

modules/k8s/service.go

@@ -205,7 +205,7 @@ func findEndpointForNodePortServiceContext(
 		return "", err
 	}
 
-	nodeHostname, err := FindNodeHostnameContextE(t, ctx, options, node)
+	nodeHostname, err := FindNodeHostnameWithOptionsContextE(t, ctx, options, node)
 	if err != nil {
 		return "", err
 	}
@@ -256,12 +256,10 @@ func pickRandomNodeE(t testing.TestingT, options *KubectlOptions) (corev1.Node,
 // FindNodeHostnameContext returns the hostname or IP address of the given node using the provided context, preferring
 // the external IP when available. This will fail the test if there is an error.
 //
-// Resolving the external IP of a cloud instance requires options.NodePublicIPLookup to be set. See NodePublicIPLookup.
-//
 //nolint:gocritic // hugeParam: cannot change public function signature
-func FindNodeHostnameContext(t testing.TestingT, ctx context.Context, options *KubectlOptions, node corev1.Node) string {
+func FindNodeHostnameContext(t testing.TestingT, ctx context.Context, node corev1.Node) string {
 	t.Helper()
-	hostname, err := FindNodeHostnameContextE(t, ctx, options, node)
+	hostname, err := FindNodeHostnameContextE(t, ctx, node)
 	require.NoError(t, err)
 
 	return hostname
@@ -270,10 +268,52 @@ func FindNodeHostnameContext(t testing.TestingT, ctx context.Context, options *K
 // FindNodeHostnameContextE returns the hostname or IP address of the given node using the provided context, preferring
 // the external IP when available.
 //
-// Resolving the external IP of a cloud instance requires options.NodePublicIPLookup to be set. See NodePublicIPLookup.
+// The address is read from the node object itself: cloud controller managers record an instance's public IP as an
+// ExternalIP address on the node. For the rare cluster that does not advertise one, see
+// FindNodeHostnameWithOptionsContextE, which can fall back to querying the cloud provider directly.
+//
+//nolint:gocritic // hugeParam: cannot change public function signature
+func FindNodeHostnameContextE(t testing.TestingT, ctx context.Context, node corev1.Node) (string, error) {
+	return FindNodeHostnameWithOptionsContextE(t, ctx, nil, node)
+}
+
+// FindNodeHostnameWithOptionsContext behaves like FindNodeHostnameContext, but consults
+// options.NodePublicIPLookup when the node itself does not advertise an external IP. This will fail the test if
+// there is an error.
 //
 //nolint:gocritic // hugeParam: cannot change public function signature
-func FindNodeHostnameContextE(t testing.TestingT, ctx context.Context, options *KubectlOptions, node corev1.Node) (string, error) {
+func FindNodeHostnameWithOptionsContext(
+	t testing.TestingT,
+	ctx context.Context,
+	options *KubectlOptions,
+	node corev1.Node,
+) string {
+	t.Helper()
+	hostname, err := FindNodeHostnameWithOptionsContextE(t, ctx, options, node)
+	require.NoError(t, err)
+
+	return hostname
+}
+
+// FindNodeHostnameWithOptionsContextE behaves like FindNodeHostnameContextE, but consults
+// options.NodePublicIPLookup when the node itself does not advertise an external IP.
+//
+// Almost all callers want FindNodeHostnameContextE instead. A lookup is only needed on clusters whose cloud
+// controller manager does not record the instance's public IP as an ExternalIP on the node object. See
+// NodePublicIPLookup for the wiring.
+//
+//nolint:gocritic // hugeParam: cannot change public function signature
+func FindNodeHostnameWithOptionsContextE(
+	t testing.TestingT,
+	ctx context.Context,
+	options *KubectlOptions,
+	node corev1.Node,
+) (string, error) {
+	// An external IP recorded on the node is authoritative and costs no API call, so prefer it for every provider.
+	if externalIP, ok := findNodeAddress(&node, corev1.NodeExternalIP); ok {
+		return externalIP, nil
+	}
+
 	nodeIDUri, err := url.Parse(node.Spec.ProviderID)
 	if err != nil {
 		return "", err
@@ -287,6 +327,17 @@ func FindNodeHostnameContextE(t testing.TestingT, ctx context.Context, options *
 	}
 }
 
+// findNodeAddress returns the first address of the given type recorded on the node, and whether one was found.
+func findNodeAddress(node *corev1.Node, addressType corev1.NodeAddressType) (string, bool) {
+	for _, address := range node.Status.Addresses {
+		if address.Type == addressType && address.Address != "" {
+			return address.Address, true
+		}
+	}
+
+	return "", false
+}
+
 // findAwsNodeHostname will return the public ip of the node, assuming the node is an AWS EC2 instance.
 // If the instance does not have a public IP, will return the internal hostname as recorded on the Kubernetes node
 // object.
@@ -339,10 +390,8 @@ func findAwsNodeHostnameContextE(
 
 // findDefaultNodeHostname returns the hostname recorded on the Kubernetes node object.
 func findDefaultNodeHostnameE(node *corev1.Node) (string, error) {
-	for _, address := range node.Status.Addresses {
-		if address.Type == corev1.NodeHostName {
-			return address.Address, nil
-		}
+	if hostname, ok := findNodeAddress(node, corev1.NodeHostName); ok {
+		return hostname, nil
 	}
 
 	return "", NewNodeHasNoHostnameError(node)