Repository: linode/linode-cloud-controller-manager
Pull request #123 (2023-03-03 13:21:54)

Use InstancesV2 to limit API calls (and simplify the code)
## Rationale

There's a new-ish interface, `InstancesV2`, which is a) a lot simpler than `Instances`, b) calls the cloud API much less often, so we end up with a lot less code that should be also more efficient.

I tried to split the implementation into individual commits while keeping the tests green, so at first I implemented both interfaces at once and then deleted the old one once I had tests in place for the new one.

I also removed the Zones implementation, because it's no longer called within `InstancesV2` - I did move its tests into `instances_test.go`, so its functionality is preserved.

### General:

* [x] Have you removed all sensitive information, including but not limited to access keys and passwords?
* [x] Have you checked to ensure there aren't other open or closed [Pull Requests](../../pulls) for the same bug/feature/question?

### Pull Request Guidelines:

1. [x] Does your submission pass tests?
1. [x] Have you added tests? 
1. [x] Are you addressing a single feature in this PR? 
1. [x] Are your commits atomic, addressing one change per commit?
1. [x] Are you following the conventions of the language? 
1. [x] Have you saved your large formatting changes for a different PR, so we can focus on your work?
1. [x] Have you explained your rationale for why this feature is needed? 
1. [ ] Have you linked your PR to an [open issue](https://blog.github.com/2013-05-14-closing-issues-via-pull-requests/)




Patch:
diff --git a/cloud/linode/cloud.go b/cloud/linode/cloud.go
index b061cd9..c9c19c5 100644
--- a/cloud/linode/cloud.go
+++ b/cloud/linode/cloud.go
@@ -28,8 +28,7 @@ var Options struct {
 
 type linodeCloud struct {
 	client        Client
-	instances     cloudprovider.Instances
-	zones         cloudprovider.Zones
+	instances     cloudprovider.InstancesV2
 	loadbalancers cloudprovider.LoadBalancer
 }
 
@@ -64,7 +63,6 @@ func newCloud() (cloudprovider.Interface, error) {
 	return &linodeCloud{
 		client:        &linodeClient,
 		instances:     newInstances(&linodeClient),
-		zones:         newZones(&linodeClient, region),
 		loadbalancers: newLoadbalancers(&linodeClient, region),
 	}, nil
 }
@@ -83,15 +81,15 @@ func (c *linodeCloud) LoadBalancer() (cloudprovider.LoadBalancer, bool) {
 }
 
 func (c *linodeCloud) Instances() (cloudprovider.Instances, bool) {
-	return c.instances, true
+	return nil, false
 }
 
 func (c *linodeCloud) InstancesV2() (cloudprovider.InstancesV2, bool) {
-	return nil, false
+	return c.instances, true
 }
 
 func (c *linodeCloud) Zones() (cloudprovider.Zones, bool) {
-	return c.zones, true
+	return nil, false
 }
 
 func (c *linodeCloud) Clusters() (cloudprovider.Clusters, bool) {
diff --git a/cloud/linode/instances.go b/cloud/linode/instances.go
index f39a11c..3402159 100644
--- a/cloud/linode/instances.go
+++ b/cloud/linode/instances.go
@@ -17,7 +17,7 @@ type instances struct {
 	client Client
 }
 
-func newInstances(client Client) cloudprovider.Instances {
+func newInstances(client Client) cloudprovider.InstancesV2 {
 	return &instances{client}
 }
 
@@ -29,176 +29,89 @@ func (e instanceNoIPAddressesError) Error() string {
 	return fmt.Sprintf("instance %d has no IP addresses", e.id)
 }
 
-func (i *instances) NodeAddresses(ctx context.Context, name types.NodeName) ([]v1.NodeAddress, error) {
-	ctx = sentry.SetHubOnContext(ctx)
-	sentry.SetTag(ctx, "node_name", string(name))
-
-	linode, err := linodeByName(ctx, i.client, name)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return nil, err
-	}
-
-	addresses, err := i.nodeAddresses(ctx, linode)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return nil, err
-	}
-
-	return addresses, nil
-}
+func (i *instances) lookupLinode(ctx context.Context, node *v1.Node) (*linodego.Instance, error) {
+	providerID := node.Spec.ProviderID
+	nodeName := types.NodeName(node.Name)
 
-func (i *instances) NodeAddressesByProviderID(ctx context.Context, providerID string) ([]v1.NodeAddress, error) {
-	ctx = sentry.SetHubOnContext(ctx)
 	sentry.SetTag(ctx, "provider_id", providerID)
+	sentry.SetTag(ctx, "node_name", node.Name)
 
-	id, err := parseProviderID(providerID)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return nil, err
-	}
-
-	linode, err := linodeByID(ctx, i.client, id)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return nil, err
-	}
-
-	addresses, err := i.nodeAddresses(ctx, linode)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return nil, err
-	}
-
-	return addresses, nil
-}
-
-func (i *instances) nodeAddresses(ctx context.Context, linode *linodego.Instance) ([]v1.NodeAddress, error) {
-	var addresses []v1.NodeAddress
-	addresses = append(addresses, v1.NodeAddress{Type: v1.NodeHostName, Address: linode.Label})
-
-	ips, err := i.client.GetInstanceIPAddresses(ctx, linode.ID)
-	if err != nil {
-		return nil, err
-	}
-
-	if (len(ips.IPv4.Public) == 0) && (len(ips.IPv4.Private) == 0) {
-		return nil, instanceNoIPAddressesError{linode.ID}
-	}
-
-	if len(ips.IPv4.Public) > 0 {
-		addresses = append(addresses, v1.NodeAddress{Type: v1.NodeExternalIP, Address: ips.IPv4.Public[0].Address})
-	}
-	// Allow Nodes to not have an ExternalIP, if this proves problematic this will be reverted
+	if providerID != "" {
+		id, err := parseProviderID(providerID)
+		if err != nil {
+			sentry.CaptureError(ctx, err)
+			return nil, err
+		}
+		sentry.SetTag(ctx, "linode_id", strconv.Itoa(id))
 
-	if len(ips.IPv4.Private) > 0 {
-		addresses = append(addresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: ips.IPv4.Private[0].Address})
+		return linodeByID(ctx, i.client, id)
 	}
-	// Allow Nodes to not have an InternalIP, if this proves problematic this will be reverted
 
-	return addresses, nil
+	return linodeByName(ctx, i.client, nodeName)
 }
 
-func (i *instances) InstanceID(ctx context.Context, nodeName types.NodeName) (string, error) {
+func (i *instances) InstanceExists(ctx context.Context, node *v1.Node) (bool, error) {
 	ctx = sentry.SetHubOnContext(ctx)
-	sentry.SetTag(ctx, "node_name", string(nodeName))
-
-	linode, err := linodeByName(ctx, i.client, nodeName)
-	if err != nil {
+	if _, err := i.lookupLinode(ctx, node); err != nil {
+		if apiError, ok := err.(*linodego.Error); ok && apiError.Code == http.StatusNotFound {
+			return false, nil
+		}
 		sentry.CaptureError(ctx, err)
-		return "", err
+		return false, err
 	}
-	return strconv.Itoa(linode.ID), nil
-}
 
-func (i *instances) InstanceType(ctx context.Context, nodeName types.NodeName) (string, error) {
-	ctx = sentry.SetHubOnContext(ctx)
-	sentry.SetTag(ctx, "node_name", string(nodeName))
-
-	linode, err := linodeByName(ctx, i.client, nodeName)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return "", err
-	}
-	return linode.Type, nil
+	return true, nil
 }
 
-func (i *instances) InstanceTypeByProviderID(ctx context.Context, providerID string) (string, error) {
+func (i *instances) InstanceShutdown(ctx context.Context, node *v1.Node) (bool, error) {
 	ctx = sentry.SetHubOnContext(ctx)
-	sentry.SetTag(ctx, "provider_id", providerID)
-
-	id, err := parseProviderID(providerID)
+	instance, err := i.lookupLinode(ctx, node)
 	if err != nil {
 		sentry.CaptureError(ctx, err)
-		return "", err
+		return false, err
 	}
 
-	sentry.SetTag(ctx, "linode_id", strconv.Itoa(id))
-
-	linode, err := linodeByID(ctx, i.client, id)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return "", err
+	// An instance is considered to be "shutdown" when it is
+	// in the process of shutting down, or already offline.
+	if instance.Status == linodego.InstanceOffline ||
+		instance.Status == linodego.InstanceShuttingDown {
+		return true, nil
 	}
-	return linode.Type, nil
-}
-
-func (i *instances) AddSSHKeyToAllInstances(_ context.Context, user string, keyData []byte) error {
-	return cloudprovider.NotImplemented
-}
 
-func (i *instances) CurrentNodeName(_ context.Context, hostname string) (types.NodeName, error) {
-	return types.NodeName(hostname), nil
+	return false, nil
 }
 
-func (i *instances) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {
+func (i *instances) InstanceMetadata(ctx context.Context, node *v1.Node) (*cloudprovider.InstanceMetadata, error) {
 	ctx = sentry.SetHubOnContext(ctx)
-	sentry.SetTag(ctx, "provider_id", providerID)
-
-	id, err := parseProviderID(providerID)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return false, err
-	}
-
-	sentry.SetTag(ctx, "linode_id", strconv.Itoa(id))
-
-	_, err = linodeByID(ctx, i.client, id)
+	linode, err := i.lookupLinode(ctx, node)
 	if err != nil {
-		if apiError, ok := err.(*linodego.Error); ok && apiError.Code == http.StatusNotFound {
-			return false, nil
-		}
 		sentry.CaptureError(ctx, err)
-		return false, err
+		return nil, err
 	}
 
-	return true, nil
-}
-
-func (i *instances) InstanceShutdownByProviderID(ctx context.Context, providerID string) (bool, error) {
-	ctx = sentry.SetHubOnContext(ctx)
-	sentry.SetTag(ctx, "provider_id", providerID)
-
-	id, err := parseProviderID(providerID)
-	if err != nil {
+	if len(linode.IPv4) == 0 {
+		err := instanceNoIPAddressesError{linode.ID}
 		sentry.CaptureError(ctx, err)
-		return false, err
+		return nil, err
 	}
 
-	sentry.SetTag(ctx, "linode_id", strconv.Itoa(id))
+	addresses := []v1.NodeAddress{{Type: v1.NodeHostName, Address: linode.Label}}
 
-	instance, err := linodeByID(ctx, i.client, id)
-	if err != nil {
-		sentry.CaptureError(ctx, err)
-		return false, err
+	for _, ip := range linode.IPv4 {
+		ipType := v1.NodeExternalIP
+		if ip.IsPrivate() {
+			ipType = v1.NodeInternalIP
+		}
+		addresses = append(addresses, v1.NodeAddress{Type: ipType, Address: ip.String()})
 	}
 
-	// An instance is considered to be "shutdown" when it is
-	// in the process of shutting down, or already offline.
-	if instance.Status == linodego.InstanceOffline ||
-		instance.Status == linodego.InstanceShuttingDown {
-		return true, nil
+	// note that Zone is omitted as it's not a thing in Linode
+	meta := &cloudprovider.InstanceMetadata{
+		ProviderID:    fmt.Sprintf("%v%v", providerIDPrefix, linode.ID),
+		NodeAddresses: addresses,
+		InstanceType:  linode.Type,
+		Region:        linode.Region,
 	}
 
-	return false, nil
+	return meta, nil
 }
diff --git a/cloud/linode/zones.go b/cloud/linode/zones.go
deleted file mode 100644
index 272369c..0000000
--- a/cloud/linode/zones.go
+++ /dev/null
@@ -1,42 +0,0 @@
-package linode
-
-import (
-	"context"
-
-	"k8s.io/apimachinery/pkg/types"
-	cloudprovider "k8s.io/cloud-provider"
-)
-
-type zones struct {
-	client Client
-	region string
-}
-
-func newZones(client Client, zone string) cloudprovider.Zones {
-	return zones{client, zone}
-}
-
-func (z zones) GetZone(_ context.Context) (cloudprovider.Zone, error) {
-	return cloudprovider.Zone{Region: z.region}, nil
-}
-
-func (z zones) GetZoneByProviderID(ctx context.Context, providerID string) (cloudprovider.Zone, error) {
-	id, err := parseProviderID(providerID)
-	if err != nil {
-		return cloudprovider.Zone{}, err
-	}
-	linode, err := linodeByID(ctx, z.client, id)
-	if err != nil {
-		return cloudprovider.Zone{}, err
-	}
-
-	return cloudprovider.Zone{Region: linode.Region}, nil
-}
-
-func (z zones) GetZoneByNodeName(ctx context.Context, nodeName types.NodeName) (cloudprovider.Zone, error) {
-	linode, err := linodeByName(ctx, z.client, nodeName)
-	if err != nil {
-		return cloudprovider.Zone{}, err
-	}
-	return cloudprovider.Zone{Region: linode.Region}, nil
-}


Test patch:
diff --git a/cloud/linode/instances_test.go b/cloud/linode/instances_test.go
index 7b9cbd7..f42068a 100644
--- a/cloud/linode/instances_test.go
+++ b/cloud/linode/instances_test.go
@@ -2,20 +2,32 @@ package linode
 
 import (
 	"context"
+	"errors"
+	"fmt"
+	"net"
 	"net/http"
 	"strconv"
 	"testing"
 
 	"github.com/golang/mock/gomock"
 	"github.com/linode/linodego"
-	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/types"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	cloudprovider "k8s.io/cloud-provider"
 )
 
-func TestInstanceExistsByProviderID(t *testing.T) {
+func nodeWithProviderID(providerID string) *v1.Node {
+	return &v1.Node{Spec: v1.NodeSpec{
+		ProviderID: providerID,
+	}}
+}
+
+func nodeWithName(name string) *v1.Node {
+	return &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: name}}
+}
+
+func TestInstanceExists(t *testing.T) {
 	ctx := context.TODO()
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -24,28 +36,42 @@ func TestInstanceExistsByProviderID(t *testing.T) {
 	instances := newInstances(client)
 
 	t.Run("should propagate generic api error", func(t *testing.T) {
-		providerID := providerIDPrefix + "123"
+		node := nodeWithProviderID(providerIDPrefix + "123")
 		expectedErr := errors.New("some error")
 		client.EXPECT().GetInstance(gomock.Any(), 123).Times(1).Return(nil, expectedErr)
 
-		exists, err := instances.InstanceExistsByProviderID(ctx, providerID)
+		exists, err := instances.InstanceExists(ctx, node)
 		assert.ErrorIs(t, err, expectedErr)
 		assert.False(t, exists)
 	})
 
-	t.Run("should return false if linode does not exist", func(t *testing.T) {
-		providerID := providerIDPrefix + "123"
+	t.Run("should return false if linode does not exist (by providerID)", func(t *testing.T) {
+		node := nodeWithProviderID(providerIDPrefix + "123")
 		client.EXPECT().GetInstance(gomock.Any(), 123).Times(1).Return(nil, &linodego.Error{
 			Code: http.StatusNotFound,
 		})
 
-		exists, err := instances.InstanceExistsByProviderID(ctx, providerID)
+		exists, err := instances.InstanceExists(ctx, node)
 		assert.NoError(t, err)
 		assert.False(t, exists)
 	})
 
-	t.Run("should return true if linode exists", func(t *testing.T) {
-		providerID := providerIDPrefix + "123"
+	t.Run("should return false if linode does not exist (by name)", func(t *testing.T) {
+		name := "some-name"
+		node := nodeWithName(name)
+		notFound := &linodego.Error{
+			Code: http.StatusNotFound,
+		}
+		filter := linodeFilterListOptions(name)
+		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return(nil, notFound)
+
+		exists, err := instances.InstanceExists(ctx, node)
+		assert.NoError(t, err)
+		assert.False(t, exists)
+	})
+
+	t.Run("should return true if linode exists (by provider)", func(t *testing.T) {
+		node := nodeWithProviderID(providerIDPrefix + "123")
 		client.EXPECT().GetInstance(gomock.Any(), 123).Times(1).Return(&linodego.Instance{
 			ID:     123,
 			Label:  "mock",
@@ -53,13 +79,26 @@ func TestInstanceExistsByProviderID(t *testing.T) {
 			Type:   "g6-standard-2",
 		}, nil)
 
-		exists, err := instances.InstanceExistsByProviderID(ctx, providerID)
+		exists, err := instances.InstanceExists(ctx, node)
+		assert.NoError(t, err)
+		assert.True(t, exists)
+	})
+
+	t.Run("should return true if linode exists (by name)", func(t *testing.T) {
+		name := "some-name"
+		node := nodeWithName(name)
+
+		client.EXPECT().ListInstances(gomock.Any(), linodeFilterListOptions(name)).Times(1).Return([]linodego.Instance{
+			{ID: 123, Label: name},
+		}, nil)
+
+		exists, err := instances.InstanceExists(ctx, node)
 		assert.NoError(t, err)
 		assert.True(t, exists)
 	})
 }
 
-func TestNodeAddresses(t *testing.T) {
+func TestMetadataRetrieval(t *testing.T) {
 	ctx := context.TODO()
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -67,71 +106,120 @@ func TestNodeAddresses(t *testing.T) {
 	client := NewMockClient(ctrl)
 	instances := newInstances(client)
 
-	t.Run("errors when linode does not exist", func(t *testing.T) {
+	t.Run("errors when linode does not exist (by name)", func(t *testing.T) {
 		name := "does-not-exist"
+		node := nodeWithName(name)
 		filter := linodeFilterListOptions(name)
 		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{}, nil)
 
-		addrs, err := instances.NodeAddresses(ctx, types.NodeName(name))
+		meta, err := instances.InstanceMetadata(ctx, node)
 		assert.ErrorIs(t, err, cloudprovider.InstanceNotFound)
-		assert.Nil(t, addrs)
+		assert.Nil(t, meta)
 	})
 
-	t.Run("errors when linode does not have any ips", func(t *testing.T) {
-		id := 29392
-		name := "an-instance"
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{
-			{ID: id, Label: name},
-		}, nil)
-		client.EXPECT().GetInstanceIPAddresses(gomock.Any(), id).Times(1).Return(&linodego.InstanceIPAddressResponse{
-			IPv4: &linodego.InstanceIPv4Response{
-				Public:  []*linodego.InstanceIP{},
-				Private: []*linodego.InstanceIP{},
-			},
-		}, nil)
+	t.Run("fails when linode does not exist (by provider)", func(t *testing.T) {
+		id := 456302
+		providerID := providerIDPrefix + strconv.Itoa(id)
+		node := nodeWithProviderID(providerID)
+		getInstanceErr := &linodego.Error{Code: http.StatusNotFound}
+		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(nil, getInstanceErr)
+		meta, err := instances.InstanceMetadata(ctx, node)
 
-		addrs, err := instances.NodeAddresses(ctx, types.NodeName(name))
-		assert.Error(t, err, instanceNoIPAddressesError{id})
-		assert.Nil(t, addrs)
+		assert.ErrorIs(t, err, getInstanceErr)
+		assert.Nil(t, meta)
 	})
 
-	t.Run("should return addresses when linode is found", func(t *testing.T) {
+	t.Run("should return data when linode is found (by name)", func(t *testing.T) {
 		id := 123
 		name := "mock-instance"
-		publicIPv4 := "45.76.101.25"
-		privateIPv4 := "192.168.133.65"
+		node := nodeWithName(name)
+		publicIPv4 := net.ParseIP("45.76.101.25")
+		privateIPv4 := net.ParseIP("192.168.133.65")
+		linodeType := "g6-standard-1"
+		region := "us-east"
 		filter := linodeFilterListOptions(name)
 		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{
-			{ID: id, Label: name},
-		}, nil)
-		client.EXPECT().GetInstanceIPAddresses(gomock.Any(), id).Times(1).Return(&linodego.InstanceIPAddressResponse{
-			IPv4: &linodego.InstanceIPv4Response{
-				Public:  []*linodego.InstanceIP{{Address: publicIPv4}},
-				Private: []*linodego.InstanceIP{{Address: privateIPv4}},
-			},
+			{ID: id, Label: name, Type: linodeType, Region: region, IPv4: []*net.IP{&publicIPv4, &privateIPv4}},
 		}, nil)
 
-		addrs, err := instances.NodeAddresses(ctx, types.NodeName(name))
+		meta, err := instances.InstanceMetadata(ctx, node)
 		assert.NoError(t, err)
-		assert.Equal(t, addrs, []v1.NodeAddress{
+		assert.Equal(t, providerIDPrefix+strconv.Itoa(id), meta.ProviderID)
+		assert.Equal(t, region, meta.Region)
+		assert.Equal(t, linodeType, meta.InstanceType)
+		assert.Equal(t, meta.NodeAddresses, []v1.NodeAddress{
 			{
 				Type:    v1.NodeHostName,
 				Address: name,
 			},
 			{
 				Type:    v1.NodeExternalIP,
-				Address: publicIPv4,
+				Address: publicIPv4.String(),
 			},
 			{
 				Type:    v1.NodeInternalIP,
-				Address: privateIPv4,
+				Address: privateIPv4.String(),
 			},
 		})
 	})
+
+	ipTests := []struct {
+		name            string
+		inputIPs        []string
+		outputAddresses []v1.NodeAddress
+		expectedErr     error
+	}{
+		{"no IPs", nil, nil, instanceNoIPAddressesError{192910}},
+		{"one public, one private", []string{"32.74.121.25", "192.168.121.42"},
+			[]v1.NodeAddress{{Type: v1.NodeExternalIP, Address: "32.74.121.25"}, {Type: v1.NodeInternalIP, Address: "192.168.121.42"}}, nil},
+		{"one public, no private", []string{"32.74.121.25"},
+			[]v1.NodeAddress{{Type: v1.NodeExternalIP, Address: "32.74.121.25"}}, nil},
+		{"one private, no public", []string{"192.168.121.42"},
+			[]v1.NodeAddress{{Type: v1.NodeInternalIP, Address: "192.168.121.42"}}, nil},
+		{"two public addresses", []string{"32.74.121.25", "32.74.121.22"},
+			[]v1.NodeAddress{{Type: v1.NodeExternalIP, Address: "32.74.121.25"}, {Type: v1.NodeExternalIP, Address: "32.74.121.22"}}, nil},
+		{"two private addresses", []string{"192.168.121.42", "10.0.2.15"},
+			[]v1.NodeAddress{{Type: v1.NodeInternalIP, Address: "192.168.121.42"}, {Type: v1.NodeInternalIP, Address: "10.0.2.15"}}, nil},
+	}
+
+	for _, test := range ipTests {
+		t.Run(fmt.Sprintf("addresses are retrieved - %s", test.name), func(t *testing.T) {
+			id := 192910
+			name := "my-instance"
+			providerID := providerIDPrefix + strconv.Itoa(id)
+			node := nodeWithProviderID(providerID)
+
+			ips := make([]*net.IP, 0, len(test.inputIPs))
+			for _, ip := range test.inputIPs {
+				parsed := net.ParseIP(ip)
+				if parsed == nil {
+					t.Fatalf("cannot parse %v as an ipv4", ip)
+				}
+				ips = append(ips, &parsed)
+			}
+
+			linodeType := "g6-standard-1"
+			region := "us-east"
+			client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
+				ID: id, Label: name, Type: linodeType, Region: region, IPv4: ips,
+			}, nil)
+
+			meta, err := instances.InstanceMetadata(ctx, node)
+
+			assert.Equal(t, err, test.expectedErr)
+			if test.expectedErr == nil {
+				assert.Equal(t, region, meta.Region)
+				assert.Equal(t, linodeType, meta.InstanceType)
+				addresses := append([]v1.NodeAddress{
+					{Type: v1.NodeHostName, Address: name},
+				}, test.outputAddresses...)
+				assert.Equal(t, meta.NodeAddresses, addresses)
+			}
+		})
+	}
 }
 
-func TestNodeAddressesByProviderID(t *testing.T) {
+func TestMalformedProviders(t *testing.T) {
 	ctx := context.TODO()
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -141,133 +229,23 @@ func TestNodeAddressesByProviderID(t *testing.T) {
 
 	t.Run("fails on malformed providerID", func(t *testing.T) {
 		providerID := "bogus://bogus"
-		addrs, err := instances.NodeAddressesByProviderID(ctx, providerID)
-
+		node := nodeWithProviderID(providerID)
+		meta, err := instances.InstanceMetadata(ctx, node)
 		assert.ErrorIs(t, err, invalidProviderIDError{providerID})
-		assert.Nil(t, addrs)
+		assert.Nil(t, meta)
 	})
 
 	t.Run("fails on non-numeric providerID", func(t *testing.T) {
 		providerID := providerIDPrefix + "abc"
-		addrs, err := instances.NodeAddressesByProviderID(ctx, providerID)
+		node := nodeWithProviderID(providerID)
+		meta, err := instances.InstanceMetadata(ctx, node)
 
 		assert.ErrorIs(t, err, invalidProviderIDError{providerID})
-		assert.Nil(t, addrs)
-	})
-
-	t.Run("fails when linode does not exist", func(t *testing.T) {
-		id := 456302
-		providerID := providerIDPrefix + strconv.Itoa(id)
-		getInstanceErr := &linodego.Error{Code: http.StatusNotFound}
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(nil, getInstanceErr)
-		addrs, err := instances.NodeAddressesByProviderID(ctx, providerID)
-
-		assert.ErrorIs(t, err, getInstanceErr)
-		assert.Nil(t, addrs)
-	})
-
-	t.Run("should return addresses when linode is found", func(t *testing.T) {
-		id := 192910
-		name := "my-instance"
-		providerID := providerIDPrefix + strconv.Itoa(id)
-		publicIPv4 := "32.74.121.25"
-		privateIPv4 := "192.168.121.42"
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
-			ID: id, Label: name,
-		}, nil)
-		client.EXPECT().GetInstanceIPAddresses(gomock.Any(), id).Times(1).Return(&linodego.InstanceIPAddressResponse{
-			IPv4: &linodego.InstanceIPv4Response{
-				Public:  []*linodego.InstanceIP{{Address: publicIPv4}},
-				Private: []*linodego.InstanceIP{{Address: privateIPv4}},
-			},
-		}, nil)
-
-		addrs, err := instances.NodeAddressesByProviderID(ctx, providerID)
-
-		assert.NoError(t, err)
-		assert.Equal(t, addrs, []v1.NodeAddress{
-			{
-				Type:    v1.NodeHostName,
-				Address: name,
-			},
-			{
-				Type:    v1.NodeExternalIP,
-				Address: publicIPv4,
-			},
-			{
-				Type:    v1.NodeInternalIP,
-				Address: privateIPv4,
-			},
-		})
-	})
-}
-
-func TestInstanceID(t *testing.T) {
-	ctx := context.TODO()
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	client := NewMockClient(ctrl)
-	instances := newInstances(client)
-
-	t.Run("fails when instance not found", func(t *testing.T) {
-		name := "non-existant-instance"
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{}, nil)
-		instanceID, err := instances.InstanceID(ctx, types.NodeName(name))
-
-		assert.ErrorIs(t, err, cloudprovider.InstanceNotFound)
-		assert.Zero(t, instanceID)
-	})
-
-	t.Run("succeeds when instance exists", func(t *testing.T) {
-		id := 129289
-		name := "existing-instance"
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{
-			{ID: id, Label: name},
-		}, nil)
-		instanceID, err := instances.InstanceID(ctx, types.NodeName(name))
-
-		assert.NoError(t, err)
-		assert.Equal(t, strconv.Itoa(id), instanceID)
-	})
-}
-
-func TestInstanceType(t *testing.T) {
-	ctx := context.TODO()
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	client := NewMockClient(ctrl)
-	instances := newInstances(client)
-
-	t.Run("fails when instance not found", func(t *testing.T) {
-		name := "bogus"
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{}, nil)
-		instanceType, err := instances.InstanceType(ctx, types.NodeName(name))
-
-		assert.ErrorIs(t, err, cloudprovider.InstanceNotFound)
-		assert.Empty(t, instanceType)
-	})
-
-	t.Run("succeeds when instance exists", func(t *testing.T) {
-		id := 39399
-		name := "my-nanode"
-		linodeType := "g6-standard-1"
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{
-			{ID: id, Label: name, Type: linodeType},
-		}, nil)
-		instanceType, err := instances.InstanceType(ctx, types.NodeName(name))
-
-		assert.NoError(t, err)
-		assert.Equal(t, linodeType, instanceType)
+		assert.Nil(t, meta)
 	})
 }
 
-func TestInstanceTypeByProviderID(t *testing.T) {
+func TestInstanceShutdown(t *testing.T) {
 	ctx := context.TODO()
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -275,57 +253,25 @@ func TestInstanceTypeByProviderID(t *testing.T) {
 	client := NewMockClient(ctrl)
 	instances := newInstances(client)
 
-	t.Run("fails when instance not found", func(t *testing.T) {
-		id := 39281
-		providerID := providerIDPrefix + strconv.Itoa(id)
+	t.Run("fails when instance not found (by provider)", func(t *testing.T) {
+		id := 12345
+		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
 		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(nil, linodego.Error{Code: http.StatusNotFound})
-		instanceType, err := instances.InstanceTypeByProviderID(ctx, providerID)
+		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.Error(t, err)
-		assert.Empty(t, instanceType)
-	})
-
-	t.Run("succeeds when instance exists", func(t *testing.T) {
-		id := 39281
-		providerID := providerIDPrefix + strconv.Itoa(id)
-		linodeType := "g6-standard-2"
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
-			ID: id, Label: "some-linode", Type: linodeType,
-		}, nil)
-		instanceType, err := instances.InstanceTypeByProviderID(ctx, providerID)
-
-		assert.NoError(t, err)
-		assert.Equal(t, linodeType, instanceType)
+		assert.False(t, shutdown)
 	})
-}
-
-func TestCurrentNodeName(t *testing.T) {
-	instances := newInstances(nil)
-	hostname := "lke2901-2920fa-3838f-28a21"
-	nodeName, err := instances.CurrentNodeName(context.TODO(), hostname)
-	assert.NoError(t, err)
-	assert.Equal(t, types.NodeName(hostname), nodeName)
-}
 
-func TestAddSSHKeyToAllInstances(t *testing.T) {
-	instances := newInstances(nil)
-	err := instances.AddSSHKeyToAllInstances(context.TODO(), "root", []byte{})
-	assert.ErrorIs(t, err, cloudprovider.NotImplemented)
-}
-
-func TestInstanceShutdownByProviderID(t *testing.T) {
-	ctx := context.TODO()
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	client := NewMockClient(ctrl)
-	instances := newInstances(client)
-
-	t.Run("fails when instance not found", func(t *testing.T) {
-		id := 12345
-		providerID := providerIDPrefix + strconv.Itoa(id)
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(nil, linodego.Error{Code: http.StatusNotFound})
-		shutdown, err := instances.InstanceShutdownByProviderID(ctx, providerID)
+	t.Run("fails when instance not found (by name)", func(t *testing.T) {
+		name := "some-name"
+		node := nodeWithName(name)
+		notFound := &linodego.Error{
+			Code: http.StatusNotFound,
+		}
+		filter := linodeFilterListOptions(name)
+		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return(nil, notFound)
+		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.Error(t, err)
 		assert.False(t, shutdown)
@@ -333,11 +279,11 @@ func TestInstanceShutdownByProviderID(t *testing.T) {
 
 	t.Run("returns true when instance is shut down", func(t *testing.T) {
 		id := 12345
-		providerID := providerIDPrefix + strconv.Itoa(id)
+		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
 		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
 			ID: id, Label: "offline-linode", Status: linodego.InstanceOffline,
 		}, nil)
-		shutdown, err := instances.InstanceShutdownByProviderID(ctx, providerID)
+		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.NoError(t, err)
 		assert.True(t, shutdown)
@@ -345,11 +291,11 @@ func TestInstanceShutdownByProviderID(t *testing.T) {
 
 	t.Run("returns true when instance is shutting down", func(t *testing.T) {
 		id := 12345
-		providerID := providerIDPrefix + strconv.Itoa(id)
+		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
 		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
 			ID: id, Label: "shutting-down-linode", Status: linodego.InstanceShuttingDown,
 		}, nil)
-		shutdown, err := instances.InstanceShutdownByProviderID(ctx, providerID)
+		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.NoError(t, err)
 		assert.True(t, shutdown)
@@ -357,11 +303,11 @@ func TestInstanceShutdownByProviderID(t *testing.T) {
 
 	t.Run("returns false when instance is running", func(t *testing.T) {
 		id := 12345
-		providerID := providerIDPrefix + strconv.Itoa(id)
+		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
 		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
 			ID: id, Label: "running-linode", Status: linodego.InstanceRunning,
 		}, nil)
-		shutdown, err := instances.InstanceShutdownByProviderID(ctx, providerID)
+		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.NoError(t, err)
 		assert.False(t, shutdown)
diff --git a/cloud/linode/zones_test.go b/cloud/linode/zones_test.go
deleted file mode 100644
index 9d03046..0000000
--- a/cloud/linode/zones_test.go
+++ /dev/null
@@ -1,95 +0,0 @@
-package linode
-
-import (
-	"context"
-	"net/http"
-	"strconv"
-	"testing"
-
-	"github.com/golang/mock/gomock"
-	"github.com/linode/linodego"
-	"github.com/stretchr/testify/assert"
-	"k8s.io/apimachinery/pkg/types"
-	cloudprovider "k8s.io/cloud-provider"
-)
-
-func TestGetZone(t *testing.T) {
-	region := "us-east"
-	zones := newZones(nil, region)
-	zone, err := zones.GetZone(context.TODO())
-
-	assert.NoError(t, err)
-	assert.Equal(t, zone, cloudprovider.Zone{Region: region})
-}
-
-func TestGetZoneByProviderID(t *testing.T) {
-	ctx := context.TODO()
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	client := NewMockClient(ctrl)
-	zones := newZones(client, "us-east")
-
-	t.Run("fail when providerID is malformed", func(t *testing.T) {
-		providerID := "bogus://123"
-		zone, err := zones.GetZoneByProviderID(ctx, providerID)
-
-		assert.Error(t, err, invalidProviderIDError{providerID}.Error())
-		assert.Equal(t, cloudprovider.Zone{}, zone)
-	})
-
-	t.Run("fail on api error", func(t *testing.T) {
-		id := 29182
-		providerID := providerIDPrefix + strconv.Itoa(id)
-		getErr := &linodego.Error{Code: http.StatusServiceUnavailable}
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(nil, getErr)
-		zone, err := zones.GetZoneByProviderID(ctx, providerID)
-
-		assert.ErrorIs(t, err, getErr)
-		assert.Equal(t, cloudprovider.Zone{}, zone)
-	})
-
-	t.Run("get region when linode exists", func(t *testing.T) {
-		id := 29818
-		region := "eu-west"
-		providerID := providerIDPrefix + strconv.Itoa(id)
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
-			ID: id, Region: region,
-		}, nil)
-		zone, err := zones.GetZoneByProviderID(ctx, providerID)
-
-		assert.NoError(t, err)
-		assert.Equal(t, cloudprovider.Zone{Region: region}, zone)
-	})
-}
-
-func TestGetZoneByNodeName(t *testing.T) {
-	ctx := context.TODO()
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	client := NewMockClient(ctrl)
-	zones := newZones(client, "us-east")
-
-	t.Run("fail on api error", func(t *testing.T) {
-		name := "a-very-nice-linode"
-		listErr := &linodego.Error{Code: http.StatusInternalServerError}
-		client.EXPECT().ListInstances(gomock.Any(), linodeFilterListOptions(name)).Times(1).Return(nil, listErr)
-		zone, err := zones.GetZoneByNodeName(ctx, types.NodeName(name))
-
-		assert.ErrorIs(t, err, listErr)
-		assert.Equal(t, cloudprovider.Zone{}, zone)
-	})
-
-	t.Run("get region when linode exists", func(t *testing.T) {
-		name := "some-linode"
-		region := "eu-west"
-		client.EXPECT().ListInstances(gomock.Any(), linodeFilterListOptions(name)).Times(1).Return([]linodego.Instance{
-			{ID: 291828, Label: name, Region: region},
-		}, nil)
-		zone, err := zones.GetZoneByNodeName(ctx, types.NodeName(name))
-
-		assert.NoError(t, err)
-		assert.Equal(t, cloudprovider.Zone{Region: region}, zone)
-	})
-}