Normalize providerID values

We index on providerID but it turns out that those values on node and
machine are not always consistent. Some encode region, some do not,
for example.

This commit normalizes all values through the normalizedProviderString().

To ensure that we catch all places I've introduced a new type and made
the find() functions take this new type in lieu of a string. Unit
tests have also been adjusted to introduce a 'test:///' prefix on the
providerID value to further validate the change.

This change allows CAPI to work out-of-the-box, assuming v1alpha2.

It's also reasonable to assert that this consistency should be
enforced elsewhere and to make this behaviour easily revertable I'm
leaving this as a separate commit in this patch series.

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go

@@ -73,13 +73,13 @@ func indexMachineByProviderID(obj interface{}) ([]string, error) {
 		return nil, nil
 	}
 
-	return []string{providerID}, nil
+	return []string{string(normalizedProviderString(providerID))}, nil
 }
 
 func indexNodeByProviderID(obj interface{}) ([]string, error) {
 	if node, ok := obj.(*corev1.Node); ok {
 		if node.Spec.ProviderID != "" {
-			return []string{node.Spec.ProviderID}, nil
+			return []string{string(normalizedProviderString(node.Spec.ProviderID))}, nil
 		}
 		return []string{}, nil
 	}
@@ -191,8 +191,8 @@ func (c *machineController) run(stopCh <-chan struct{}) error {
 
 // findMachineByProviderID finds machine matching providerID. A
 // DeepCopy() of the object is returned on success.
-func (c *machineController) findMachineByProviderID(providerID string) (*Machine, error) {
-	objs, err := c.machineInformer.Informer().GetIndexer().ByIndex(machineProviderIDIndex, providerID)
+func (c *machineController) findMachineByProviderID(providerID normalizedProviderID) (*Machine, error) {
+	objs, err := c.machineInformer.Informer().GetIndexer().ByIndex(machineProviderIDIndex, string(providerID))
 	if err != nil {
 		return nil, err
 	}
@@ -447,7 +447,7 @@ func (c *machineController) nodeGroups() ([]*nodegroup, error) {
 }
 
 func (c *machineController) nodeGroupForNode(node *corev1.Node) (*nodegroup, error) {
-	machine, err := c.findMachineByProviderID(node.Spec.ProviderID)
+	machine, err := c.findMachineByProviderID(normalizedProviderString(node.Spec.ProviderID))
 	if err != nil {
 		return nil, err
 	}
@@ -504,8 +504,8 @@ func (c *machineController) nodeGroupForNode(node *corev1.Node) (*nodegroup, err
 // findNodeByProviderID find the Node object keyed by provideID.
 // Returns nil if it cannot be found. A DeepCopy() of the object is
 // returned on success.
-func (c *machineController) findNodeByProviderID(providerID string) (*corev1.Node, error) {
-	objs, err := c.nodeInformer.GetIndexer().ByIndex(nodeProviderIDIndex, providerID)
+func (c *machineController) findNodeByProviderID(providerID normalizedProviderID) (*corev1.Node, error) {
+	objs, err := c.nodeInformer.GetIndexer().ByIndex(nodeProviderIDIndex, string(providerID))
 	if err != nil {
 		return nil, err
 	}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go

@@ -205,7 +205,7 @@ func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference)
 			},
 		},
 		Spec: corev1.NodeSpec{
-			ProviderID: fmt.Sprintf("%s-%s-nodeid-%d", namespace, owner.Name, i),
+			ProviderID: fmt.Sprintf("test:////%s-%s-nodeid-%d", namespace, owner.Name, i),
 		},
 	}
 
@@ -224,7 +224,7 @@ func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference)
 			}},
 		},
 		Spec: MachineSpec{
-			ProviderID: pointer.StringPtr(fmt.Sprintf("%s-%s-nodeid-%d", namespace, owner.Name, i)),
+			ProviderID: pointer.StringPtr(fmt.Sprintf("test:////%s-%s-nodeid-%d", namespace, owner.Name, i)),
 		},
 		Status: MachineStatus{
 			NodeRef: &corev1.ObjectReference{
@@ -421,7 +421,7 @@ func TestControllerFindMachineByProviderID(t *testing.T) {
 	}
 
 	// Test #1: Verify underlying machine provider ID matches
-	machine, err := controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
+	machine, err := controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -440,7 +440,7 @@ func TestControllerFindMachineByProviderID(t *testing.T) {
 	if err := controller.machineInformer.Informer().GetStore().Update(machine); err != nil {
 		t.Fatalf("unexpected error updating machine, got %v", err)
 	}
-	machine, err = controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
+	machine, err = controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -824,7 +824,7 @@ func TestControllerFindMachineFromNodeAnnotation(t *testing.T) {
 	}
 
 	// Test #1: Verify machine can be found from node annotation
-	machine, err := controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
+	machine, err := controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -842,7 +842,7 @@ func TestControllerFindMachineFromNodeAnnotation(t *testing.T) {
 	if err := controller.nodeInformer.GetStore().Update(node); err != nil {
 		t.Fatalf("unexpected error updating node, got %v", err)
 	}
-	machine, err = controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
+	machine, err = controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -938,7 +938,7 @@ func TestControllerMachineSetNodeNamesUsingProviderID(t *testing.T) {
 	})
 
 	for i := range testConfig.nodes {
-		if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
+		if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
 			t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 		}
 	}
@@ -986,7 +986,7 @@ func TestControllerMachineSetNodeNamesUsingStatusNodeRefName(t *testing.T) {
 	})
 
 	for i := range testConfig.nodes {
-		if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
+		if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
 			t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 		}
 	}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go

@@ -111,7 +111,7 @@ func (ng *nodegroup) DeleteNodes(nodes []*corev1.Node) error {
 	// suitable candidate for deletion and drop the replica count
 	// by 1. Fail fast on any error.
 	for _, node := range nodes {
-		machine, err := ng.machineController.findMachineByProviderID(node.Spec.ProviderID)
+		machine, err := ng.machineController.findMachineByProviderID(normalizedProviderString(node.Spec.ProviderID))
 		if err != nil {
 			return err
 		}
@@ -198,7 +198,7 @@ func (ng *nodegroup) Nodes() ([]cloudprovider.Instance, error) {
 	instances := make([]cloudprovider.Instance, len(nodes))
 	for i := range nodes {
 		instances[i] = cloudprovider.Instance{
-			Id: nodes[i],
+			Id: string(normalizedProviderString(nodes[i])),
 		}
 	}
 

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

@@ -659,7 +659,7 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
 		})
 
 		for i := 0; i < len(nodeNames); i++ {
-			if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
+			if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
 				t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 			}
 		}
@@ -722,7 +722,6 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
 
 func TestNodeGroupMachineSetDeleteNodesWithMismatchedNodes(t *testing.T) {
 	test := func(t *testing.T, expected int, testConfigs []*testConfig) {
-		t.Helper()
 		testConfig0, testConfig1 := testConfigs[0], testConfigs[1]
 		controller, stop := mustCreateTestController(t, testConfigs...)
 		defer stop()
@@ -756,7 +755,7 @@ func TestNodeGroupMachineSetDeleteNodesWithMismatchedNodes(t *testing.T) {
 			expectedErr0 = `node "test-namespace1-machineset-0-nodeid-0" doesn't belong to node group "test-namespace0/machinedeployment-0"`
 		}
 
-		if !strings.Contains(err0.Error(), expectedErr0) {
+		if !strings.Contains(err0.Error(), string(normalizedProviderString(expectedErr0))) {
 			t.Errorf("expected: %q, got: %q", expectedErr0, err0.Error())
 		}
 
@@ -771,7 +770,7 @@ func TestNodeGroupMachineSetDeleteNodesWithMismatchedNodes(t *testing.T) {
 			expectedErr1 = `node "test-namespace0-machineset-0-nodeid-0" doesn't belong to node group "test-namespace1/machinedeployment-0"`
 		}
 
-		if !strings.Contains(err1.Error(), expectedErr1) {
+		if !strings.Contains(err1.Error(), string(normalizedProviderString(expectedErr1))) {
 			t.Errorf("expected: %q, got: %q", expectedErr1, err1.Error())
 		}
 
@@ -845,7 +844,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {
 		})
 
 		for i := 0; i < len(nodeNames); i++ {
-			if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
+			if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
 				t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
 			}
 		}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go

@@ -18,6 +18,7 @@ package clusterapi
 
 import (
 	"strconv"
+	"strings"
 
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -48,6 +49,8 @@ var (
 	errInvalidMaxAnnotation = errors.New("invalid max annotation")
 )
 
+type normalizedProviderID string
+
 // minSize returns the minimum value encoded in the annotations keyed
 // by nodeGroupMinSizeAnnotationKey. Returns errMissingMinAnnotation
 // if the annotation doesn't exist or errInvalidMinAnnotation if the
@@ -143,3 +146,10 @@ func machineSetIsOwnedByMachineDeployment(machineSet *MachineSet, machineDeploym
 	}
 	return false
 }
+
+// normalizedProviderString splits s on '/' returning everything after
+// the last '/'.
+func normalizedProviderString(s string) normalizedProviderID {
+	split := strings.Split(s, "/")
+	return normalizedProviderID(split[len(split)-1])
+}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go

@@ -369,3 +369,34 @@ func TestUtilMachineSetMachineDeploymentOwnerRef(t *testing.T) {
 		})
 	}
 }
+
+func TestUtilNormalizedProviderID(t *testing.T) {
+	for _, tc := range []struct {
+		description string
+		providerID  string
+		expectedID  normalizedProviderID
+	}{{
+		description: "nil string yields empty string",
+		providerID:  "",
+		expectedID:  "",
+	}, {
+		description: "empty string",
+		providerID:  "",
+		expectedID:  "",
+	}, {
+		description: "id without / characters",
+		providerID:  "i-12345678",
+		expectedID:  "i-12345678",
+	}, {
+		description: "id with / characters",
+		providerID:  "aws:////i-12345678",
+		expectedID:  "i-12345678",
+	}} {
+		t.Run(tc.description, func(t *testing.T) {
+			actualID := normalizedProviderString(tc.providerID)
+			if actualID != tc.expectedID {
+				t.Errorf("expected %v, got %v", tc.expectedID, actualID)
+			}
+		})
+	}
+}