Runtime checks around providerID

Author: Jeff Bornemann

pkg/oci/instances.go

@@ -97,7 +97,10 @@ func (cp *CloudProvider) NodeAddresses(ctx context.Context, name types.NodeName)
 // in this method to obtain nodeaddresses.
 func (cp *CloudProvider) NodeAddressesByProviderID(ctx context.Context, providerID string) ([]api.NodeAddress, error) {
 	glog.V(4).Infof("NodeAddressesByProviderID(%q) called", providerID)
-	instanceID := util.MapProviderIDToInstanceID(providerID)
+	instanceID, err := util.MapProviderIDToInstanceID(providerID)
+	if err != nil {
+		return nil, errors.Wrap(err, "MapProviderIDToInstanceID")
+	}
 	vnic, err := cp.client.Compute().GetPrimaryVNICForInstance(ctx, cp.config.CompartmentID, instanceID)
 	if err != nil {
 		return nil, errors.Wrap(err, "GetPrimaryVNICForInstance")
@@ -156,7 +159,10 @@ func (cp *CloudProvider) InstanceType(ctx context.Context, name types.NodeName)
 func (cp *CloudProvider) InstanceTypeByProviderID(ctx context.Context, providerID string) (string, error) {
 	glog.V(4).Infof("InstanceTypeByProviderID(%q) called", providerID)
 
-	instanceID := util.MapProviderIDToInstanceID(providerID)
+	instanceID, err := util.MapProviderIDToInstanceID(providerID)
+	if err != nil {
+		return "", errors.Wrap(err, "MapProviderIDToInstanceID")
+	}
 	inst, err := cp.client.Compute().GetInstance(ctx, instanceID)
 	if err != nil {
 		return "", errors.Wrap(err, "GetInstance")
@@ -183,7 +189,10 @@ func (cp *CloudProvider) CurrentNodeName(ctx context.Context, hostname string) (
 // instance will be immediately deleted by the cloud controller manager.
 func (cp *CloudProvider) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {
 	glog.V(4).Infof("InstanceExistsByProviderID(%q) called", providerID)
-	instanceID := util.MapProviderIDToInstanceID(providerID)
+	instanceID, err := util.MapProviderIDToInstanceID(providerID)
+	if err != nil {
+		return false, err
+	}
 	instance, err := cp.client.Compute().GetInstance(ctx, instanceID)
 	if client.IsNotFound(err) {
 		return false, nil

pkg/oci/load_balancer.go

@@ -148,7 +148,15 @@ func getSubnetsForNodes(ctx context.Context, nodes []*v1.Node, client client.Int
 			continue
 		}
 
-		id := util.MapProviderIDToInstanceID(node.Spec.ProviderID)
+		if node.Spec.ProviderID == "" {
+			return nil, errors.Errorf("ProviderID was not present on node %q", node.Name)
+		}
+
+		id, err := util.MapProviderIDToInstanceID(node.Spec.ProviderID)
+		if err != nil {
+			return nil, errors.Wrap(err, "MapProviderIDToInstanceID")
+		}
+
 		vnic, err := client.Compute().GetPrimaryVNICForInstance(ctx, compartmentID, id)
 		if err != nil {
 			return nil, err

pkg/oci/util/util.go

@@ -18,6 +18,7 @@ import (
 	"strings"
 
 	api "k8s.io/api/core/v1"
+	"github.com/pkg/errors"
 )
 
 const (
@@ -28,11 +29,14 @@ const (
 )
 
 // MapProviderIDToInstanceID parses the provider id and returns the instance ocid.
-func MapProviderIDToInstanceID(providerID string) string {
+func MapProviderIDToInstanceID(providerID string) (string, error) {
+	if providerID == "" {
+		return providerID, errors.New("provider ID is empty")
+	}
 	if strings.HasPrefix(providerID, providerPrefix) {
-		return strings.TrimPrefix(providerID, providerPrefix)
+		return strings.TrimPrefix(providerID, providerPrefix), nil
 	}
-	return providerID
+	return providerID, nil
 }
 
 // NodeInternalIP returns the nodes internal ip

pkg/oci/util/util_test.go

@@ -18,24 +18,35 @@ import "testing"
 
 func TestMapProviderIDToInstanceID(t *testing.T) {
 	testCases := map[string]struct {
-		providerID string
-		expected   string
+		providerID   string
+		instanceID   string
+		error		 bool
 	}{
 		"no cloud prefix": {
 			providerID: "testid",
-			expected:   "testid",
+			instanceID: "testid",
+			error: false,
 		},
 		"cloud prefix": {
 			providerID: providerPrefix + "testid",
-			expected:   "testid",
+			instanceID:  "testid",
+			error: false,
+		},
+		"empty string": {
+			providerID: "",
+			instanceID:  "",
+			error: true,
 		},
 	}
 
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
-			result := MapProviderIDToInstanceID(tc.providerID)
-			if result != tc.expected {
-				t.Errorf("Expected instance id %q, but got %q", tc.expected, result)
+			result, err := MapProviderIDToInstanceID(tc.providerID)
+			if result != tc.instanceID {
+				t.Errorf("Expected instance id %q, but got %q", tc.instanceID, result)
+			}
+			if (err == nil && tc.error) || (!tc.error && err != nil) {
+				t.Errorf("Expected an error condition for input %q, but did no receive one; or received one, when not expecting", tc.providerID)
 			}
 		})
 	}

pkg/oci/zones.go

@@ -47,7 +47,10 @@ func (cp *CloudProvider) GetZone(ctx context.Context) (cloudprovider.Zone, error
 // particularly used in the context of external cloud providers where node
 // initialization must be down outside the kubelets.
 func (cp *CloudProvider) GetZoneByProviderID(ctx context.Context, providerID string) (cloudprovider.Zone, error) {
-	instanceID := util.MapProviderIDToInstanceID(providerID)
+	instanceID, err := util.MapProviderIDToInstanceID(providerID)
+	if err != nil {
+		return cloudprovider.Zone{}, err
+	}
 	instance, err := cp.client.Compute().GetInstance(ctx, instanceID)
 	if err != nil {
 		return cloudprovider.Zone{}, err