Runtime checks around providerID (#228)

* Runtime checks around providerID

* Added additional required authorization for security lists to example

Author: jbornemann

manifests/cloud-provider-example.yaml

@@ -35,6 +35,9 @@ loadBalancer:
   subnet2: ocid1.subnet.oc1.phx.aaaaaaaahuxrgvs65iwdz7ekwgg3l5gyah7ww5klkwjcso74u3e4i64hvtvq
 
   # SecurityListManagementMode configures how security lists are managed by the CCM.
+  # If you choose to have security lists managed by the CCM, ensure you have setup the following additional OCI policy:
+  # Allow dynamic-group [your dynamic group name] to manage security-lists in compartment [your compartment name]
+  #
   #   "All" (default): Manage all required security list rules for load balancer services.
   #   "Frontend":      Manage only security list rules for ingress to the load
   #                    balancer. Requires that the user has setup a rule that

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) {
 	cp.logger.With("instanceID", providerID).Debug("Getting node addresses by provider id")
-	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")
@@ -157,7 +160,10 @@ func (cp *CloudProvider) InstanceType(ctx context.Context, name types.NodeName)
 func (cp *CloudProvider) InstanceTypeByProviderID(ctx context.Context, providerID string) (string, error) {
 	cp.logger.With("instanceID", providerID).Debug("Getting instance type by provider id")
 
-	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) {
 	cp.logger.With("instanceID", providerID).Debug("Checking instance exists by provider id")
-	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

@@ -149,7 +149,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(".spec.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

@@ -17,6 +17,7 @@ package util
 import (
 	"strings"
 
+	"github.com/pkg/errors"
 	api "k8s.io/api/core/v1"
 )
 
@@ -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

@@ -19,23 +19,34 @@ import "testing"
 func TestMapProviderIDToInstanceID(t *testing.T) {
 	testCases := map[string]struct {
 		providerID string
-		expected   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