Merge pull request #36 from andrewsykim/15

Handle correct providerID format

do/droplets.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"strings"
 
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -54,8 +55,12 @@ func (i *instances) NodeAddresses(nodeName types.NodeName) ([]v1.NodeAddress, er
 // NodeAddressesByProviderID returns all the valid addresses of the specified
 // node by providerId. For DO this is the public/private ipv4 addresses for now.
 func (i *instances) NodeAddressesByProviderID(providerId string) ([]v1.NodeAddress, error) {
-	// we can technically get all the required data from metadata service
-	droplet, err := dropletByID(context.TODO(), i.client, providerId)
+	id, err := dropletIDFromProviderID(providerId)
+	if err != nil {
+		return nil, err
+	}
+
+	droplet, err := dropletByID(context.TODO(), i.client, id)
 	if err != nil {
 		return nil, err
 	}
@@ -111,7 +116,12 @@ func (i *instances) InstanceType(name types.NodeName) (string, error) {
 
 // InstanceTypeByProviderID returns the type of the specified instance.
 func (i *instances) InstanceTypeByProviderID(providerId string) (string, error) {
-	droplet, err := dropletByID(context.TODO(), i.client, providerId)
+	id, err := dropletIDFromProviderID(providerId)
+	if err != nil {
+		return "", err
+	}
+
+	droplet, err := dropletByID(context.TODO(), i.client, id)
 	if err != nil {
 		return "", err
 	}
@@ -174,3 +184,24 @@ func dropletByName(ctx context.Context, client *godo.Client, nodeName types.Node
 
 	return nil, cloudprovider.InstanceNotFound
 }
+
+// dropletIDFromProviderID returns a droplet's ID extracted from the node's
+// providerID spec. The providerID spec should be retrievable from the Kubernetes
+// node object. The expected format is: digitalocean://droplet-id
+func dropletIDFromProviderID(providerID string) (string, error) {
+	if providerID == "" {
+		return "", errors.New("providerID cannot be empty string")
+	}
+
+	split := strings.Split(providerID, "/")
+	if len(split) != 3 {
+		return "", fmt.Errorf("unexpected providerID format: %s, format should be: digitalocean://12345", providerID)
+	}
+
+	// since split[0] is actually "digitalocean:"
+	if strings.TrimSuffix(split[0], ":") != providerName {
+		return "", fmt.Errorf("provider name from providerID should be digitalocean: %s", providerID)
+	}
+
+	return split[2], nil
+}

do/droplets_test.go

@@ -18,6 +18,7 @@ package do
 
 import (
 	"bytes"
+	"errors"
 	"io/ioutil"
 	"net/http"
 	"reflect"
@@ -205,7 +206,7 @@ func TestNodeAddressesByProviderID(t *testing.T) {
 		},
 	}
 
-	addresses, err := instances.NodeAddressesByProviderID("123")
+	addresses, err := instances.NodeAddressesByProviderID("digitalocean://123")
 
 	if !reflect.DeepEqual(addresses, expectedAddresses) {
 		t.Errorf("unexpected node addresses. got: %v want: %v", addresses, expectedAddresses)
@@ -261,3 +262,55 @@ func TestInstanceType(t *testing.T) {
 		t.Errorf("expected type 2gb, got: %s", instanceType)
 	}
 }
+
+func Test_dropletIDFromProviderID(t *testing.T) {
+	testcases := []struct {
+		name       string
+		providerID string
+		dropletID  string
+		err        error
+	}{
+		{
+			"valid providerID",
+			"digitalocean://12345",
+			"12345",
+			nil,
+		},
+		{
+			"invalid providerID - empty string",
+			"",
+			"",
+			errors.New("providerID cannot be empty string"),
+		},
+		{
+			"invalid providerID - wrong format",
+			"digitalocean:/12345",
+			"",
+			errors.New("unexpected providerID format: digitalocean:/12345, format should be: digitalocean://12345"),
+		},
+		{
+			"invalid providerID - wrong provider name",
+			"do://12345",
+			"",
+			errors.New("provider name from providerID should be digitalocean: do://12345"),
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			dropletID, err := dropletIDFromProviderID(testcase.providerID)
+			if dropletID != testcase.dropletID {
+				t.Errorf("actual droplet ID: %s", dropletID)
+				t.Errorf("expected droplet ID: %s", testcase.dropletID)
+				t.Error("unexpected droplet ID")
+			}
+
+			if !reflect.DeepEqual(err, testcase.err) {
+				t.Errorf("actual err: %v", err)
+				t.Errorf("expected err: %v", testcase.err)
+				t.Error("unexpected err")
+			}
+		})
+	}
+
+}

do/zones.go

@@ -42,7 +42,12 @@ func (z zones) GetZone() (cloudprovider.Zone, error) {
 // locality region of the node specified by providerId. GetZoneByProviderID
 // will only fill the Region field of cloudprovider.Zone for DO.
 func (z zones) GetZoneByProviderID(providerID string) (cloudprovider.Zone, error) {
-	d, err := dropletByID(context.Background(), z.client, providerID)
+	id, err := dropletIDFromProviderID(providerID)
+	if err != nil {
+		return cloudprovider.Zone{}, err
+	}
+
+	d, err := dropletByID(context.Background(), z.client, id)
 	if err != nil {
 		return cloudprovider.Zone{}, err
 	}

do/zones_test.go

@@ -50,7 +50,7 @@ func TestZones_GetZoneByProviderID(t *testing.T) {
 
 	expected := cloudprovider.Zone{Region: "test1"}
 
-	actual, err := zones.GetZoneByProviderID("123")
+	actual, err := zones.GetZoneByProviderID("digitalocean://123")
 
 	if !reflect.DeepEqual(actual, expected) {
 		t.Errorf("unexpected region. got: %+v want: %+v", actual, expected)