Skip to content

Commit d9e3197

Browse files
frobwarefrobware
committed
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.
1 parent c5fa2b4 commit d9e3197

File tree

6 files changed

+62
-22
lines changed

6 files changed

+62
-22
lines changed

‎cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,13 @@ func indexMachineByProviderID(obj interface{}) ([]string, error) {
7474
return nil, nil
7575
}
7676

77-
return []string{providerID}, nil
77+
return []string{string(normalizedProviderString(providerID))}, nil
7878
}
7979

8080
func indexNodeByProviderID(obj interface{}) ([]string, error) {
8181
if node, ok := obj.(*corev1.Node); ok {
8282
if node.Spec.ProviderID != "" {
83-
return []string{node.Spec.ProviderID}, nil
83+
return []string{string(normalizedProviderString(node.Spec.ProviderID))}, nil
8484
}
8585
return []string{}, nil
8686
}
@@ -192,8 +192,8 @@ func (c *machineController) run(stopCh <-chan struct{}) error {
192192

193193
// findMachineByProviderID finds machine matching providerID. A
194194
// DeepCopy() of the object is returned on success.
195-
func (c *machineController) findMachineByProviderID(providerID string) (*Machine, error) {
196-
objs, err := c.machineInformer.Informer().GetIndexer().ByIndex(machineProviderIDIndex, providerID)
195+
func (c *machineController) findMachineByProviderID(providerID normalizedProviderID) (*Machine, error) {
196+
objs, err := c.machineInformer.Informer().GetIndexer().ByIndex(machineProviderIDIndex, string(providerID))
197197
if err != nil {
198198
return nil, err
199199
}
@@ -448,7 +448,7 @@ func (c *machineController) nodeGroups() ([]*nodegroup, error) {
448448
}
449449

450450
func (c *machineController) nodeGroupForNode(node *corev1.Node) (*nodegroup, error) {
451-
machine, err := c.findMachineByProviderID(node.Spec.ProviderID)
451+
machine, err := c.findMachineByProviderID(normalizedProviderString(node.Spec.ProviderID))
452452
if err != nil {
453453
return nil, err
454454
}
@@ -505,8 +505,8 @@ func (c *machineController) nodeGroupForNode(node *corev1.Node) (*nodegroup, err
505505
// findNodeByProviderID find the Node object keyed by provideID.
506506
// Returns nil if it cannot be found. A DeepCopy() of the object is
507507
// returned on success.
508-
func (c *machineController) findNodeByProviderID(providerID string) (*corev1.Node, error) {
509-
objs, err := c.nodeInformer.GetIndexer().ByIndex(nodeProviderIDIndex, providerID)
508+
func (c *machineController) findNodeByProviderID(providerID normalizedProviderID) (*corev1.Node, error) {
509+
objs, err := c.nodeInformer.GetIndexer().ByIndex(nodeProviderIDIndex, string(providerID))
510510
if err != nil {
511511
return nil, err
512512
}

‎cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference)
205205
},
206206
},
207207
Spec: corev1.NodeSpec{
208-
ProviderID: fmt.Sprintf("%s-%s-nodeid-%d", namespace, owner.Name, i),
208+
ProviderID: fmt.Sprintf("test:////%s-%s-nodeid-%d", namespace, owner.Name, i),
209209
},
210210
}
211211

@@ -224,7 +224,7 @@ func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference)
224224
}},
225225
},
226226
Spec: MachineSpec{
227-
ProviderID: pointer.StringPtr(fmt.Sprintf("%s-%s-nodeid-%d", namespace, owner.Name, i)),
227+
ProviderID: pointer.StringPtr(fmt.Sprintf("test:////%s-%s-nodeid-%d", namespace, owner.Name, i)),
228228
},
229229
Status: MachineStatus{
230230
NodeRef: &corev1.ObjectReference{
@@ -421,7 +421,7 @@ func TestControllerFindMachineByProviderID(t *testing.T) {
421421
}
422422

423423
// Test #1: Verify underlying machine provider ID matches
424-
machine, err := controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
424+
machine, err := controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
425425
if err != nil {
426426
t.Fatalf("unexpected error: %v", err)
427427
}
@@ -440,7 +440,7 @@ func TestControllerFindMachineByProviderID(t *testing.T) {
440440
if err := controller.machineInformer.Informer().GetStore().Update(machine); err != nil {
441441
t.Fatalf("unexpected error updating machine, got %v", err)
442442
}
443-
machine, err = controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
443+
machine, err = controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
444444
if err != nil {
445445
t.Fatalf("unexpected error: %v", err)
446446
}
@@ -824,7 +824,7 @@ func TestControllerFindMachineFromNodeAnnotation(t *testing.T) {
824824
}
825825

826826
// Test #1: Verify machine can be found from node annotation
827-
machine, err := controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
827+
machine, err := controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
828828
if err != nil {
829829
t.Fatalf("unexpected error: %v", err)
830830
}
@@ -842,7 +842,7 @@ func TestControllerFindMachineFromNodeAnnotation(t *testing.T) {
842842
if err := controller.nodeInformer.GetStore().Update(node); err != nil {
843843
t.Fatalf("unexpected error updating node, got %v", err)
844844
}
845-
machine, err = controller.findMachineByProviderID(testConfig.nodes[0].Spec.ProviderID)
845+
machine, err = controller.findMachineByProviderID(normalizedProviderString(testConfig.nodes[0].Spec.ProviderID))
846846
if err != nil {
847847
t.Fatalf("unexpected error: %v", err)
848848
}
@@ -938,7 +938,7 @@ func TestControllerMachineSetNodeNamesUsingProviderID(t *testing.T) {
938938
})
939939

940940
for i := range testConfig.nodes {
941-
if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
941+
if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
942942
t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
943943
}
944944
}
@@ -986,7 +986,7 @@ func TestControllerMachineSetNodeNamesUsingStatusNodeRefName(t *testing.T) {
986986
})
987987

988988
for i := range testConfig.nodes {
989-
if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
989+
if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
990990
t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
991991
}
992992
}

‎cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ func (ng *nodegroup) DeleteNodes(nodes []*corev1.Node) error {
111111
// suitable candidate for deletion and drop the replica count
112112
// by 1. Fail fast on any error.
113113
for _, node := range nodes {
114-
machine, err := ng.machineController.findMachineByProviderID(node.Spec.ProviderID)
114+
machine, err := ng.machineController.findMachineByProviderID(normalizedProviderString(node.Spec.ProviderID))
115115
if err != nil {
116116
return err
117117
}
@@ -198,7 +198,7 @@ func (ng *nodegroup) Nodes() ([]cloudprovider.Instance, error) {
198198
instances := make([]cloudprovider.Instance, len(nodes))
199199
for i := range nodes {
200200
instances[i] = cloudprovider.Instance{
201-
Id: nodes[i],
201+
Id: string(normalizedProviderString(nodes[i])),
202202
}
203203
}
204204

‎cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,7 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
659659
})
660660

661661
for i := 0; i < len(nodeNames); i++ {
662-
if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
662+
if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
663663
t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
664664
}
665665
}
@@ -722,7 +722,6 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
722722

723723
func TestNodeGroupMachineSetDeleteNodesWithMismatchedNodes(t *testing.T) {
724724
test := func(t *testing.T, expected int, testConfigs []*testConfig) {
725-
t.Helper()
726725
testConfig0, testConfig1 := testConfigs[0], testConfigs[1]
727726
controller, stop := mustCreateTestController(t, testConfigs...)
728727
defer stop()
@@ -756,7 +755,7 @@ func TestNodeGroupMachineSetDeleteNodesWithMismatchedNodes(t *testing.T) {
756755
expectedErr0 = `node "test-namespace1-machineset-0-nodeid-0" doesn't belong to node group "test-namespace0/machinedeployment-0"`
757756
}
758757

759-
if !strings.Contains(err0.Error(), expectedErr0) {
758+
if !strings.Contains(err0.Error(), string(normalizedProviderString(expectedErr0))) {
760759
t.Errorf("expected: %q, got: %q", expectedErr0, err0.Error())
761760
}
762761

@@ -771,7 +770,7 @@ func TestNodeGroupMachineSetDeleteNodesWithMismatchedNodes(t *testing.T) {
771770
expectedErr1 = `node "test-namespace0-machineset-0-nodeid-0" doesn't belong to node group "test-namespace1/machinedeployment-0"`
772771
}
773772

774-
if !strings.Contains(err1.Error(), expectedErr1) {
773+
if !strings.Contains(err1.Error(), string(normalizedProviderString(expectedErr1))) {
775774
t.Errorf("expected: %q, got: %q", expectedErr1, err1.Error())
776775
}
777776

@@ -845,7 +844,7 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) {
845844
})
846845

847846
for i := 0; i < len(nodeNames); i++ {
848-
if nodeNames[i].Id != testConfig.nodes[i].Spec.ProviderID {
847+
if nodeNames[i].Id != string(normalizedProviderString(testConfig.nodes[i].Spec.ProviderID)) {
849848
t.Fatalf("expected %q, got %q", testConfig.nodes[i].Spec.ProviderID, nodeNames[i].Id)
850849
}
851850
}

‎cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package clusterapi
1818

1919
import (
2020
"strconv"
21+
"strings"
2122

2223
"github.com/pkg/errors"
2324
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -48,6 +49,8 @@ var (
4849
errInvalidMaxAnnotation = errors.New("invalid max annotation")
4950
)
5051

52+
type normalizedProviderID string
53+
5154
// minSize returns the minimum value encoded in the annotations keyed
5255
// by nodeGroupMinSizeAnnotationKey. Returns errMissingMinAnnotation
5356
// if the annotation doesn't exist or errInvalidMinAnnotation if the
@@ -143,3 +146,10 @@ func machineSetIsOwnedByMachineDeployment(machineSet *MachineSet, machineDeploym
143146
}
144147
return false
145148
}
149+
150+
// normalizedProviderString splits s on '/' returning everything after
151+
// the last '/'.
152+
func normalizedProviderString(s string) normalizedProviderID {
153+
split := strings.Split(s, "/")
154+
return normalizedProviderID(split[len(split)-1])
155+
}

‎cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,3 +369,34 @@ func TestUtilMachineSetMachineDeploymentOwnerRef(t *testing.T) {
369369
})
370370
}
371371
}
372+
373+
func TestUtilNormalizedProviderID(t *testing.T) {
374+
for _, tc := range []struct {
375+
description string
376+
providerID string
377+
expectedID normalizedProviderID
378+
}{{
379+
description: "nil string yields empty string",
380+
providerID: "",
381+
expectedID: "",
382+
}, {
383+
description: "empty string",
384+
providerID: "",
385+
expectedID: "",
386+
}, {
387+
description: "id without / characters",
388+
providerID: "i-12345678",
389+
expectedID: "i-12345678",
390+
}, {
391+
description: "id with / characters",
392+
providerID: "aws:////i-12345678",
393+
expectedID: "i-12345678",
394+
}} {
395+
t.Run(tc.description, func(t *testing.T) {
396+
actualID := normalizedProviderString(tc.providerID)
397+
if actualID != tc.expectedID {
398+
t.Errorf("expected %v, got %v", tc.expectedID, actualID)
399+
}
400+
})
401+
}
402+
}

0 commit comments

Comments
 (0)