validate ProviderID equality by comparing entire string

Author: jackfrancis

controllers/noderefutil/providerid.go

@@ -19,7 +19,6 @@ package noderefutil
 
 import (
 	"errors"
-	"fmt"
 	"regexp"
 	"strings"
 )
@@ -87,9 +86,9 @@ func (p *ProviderID) ID() string {
 	return p.id
 }
 
-// Equals returns true if both the CloudProvider and ID match.
+// Equals returns true if this ProviderID string matches another ProviderID string.
 func (p *ProviderID) Equals(o *ProviderID) bool {
-	return p.CloudProvider() == o.CloudProvider() && p.ID() == o.ID()
+	return p.String() == o.String()
 }
 
 // String returns the string representation of this object.
@@ -102,10 +101,8 @@ func (p *ProviderID) Validate() bool {
 	return p.CloudProvider() != "" && p.ID() != ""
 }
 
-// IndexKey returns a string concatenating the cloudProvider and the ID parts of the providerID.
-// E.g Format: cloudProvider://optional/segments/etc/id. IndexKey: cloudProvider/id
-// This is useful to use the providerID as a reliable index between nodes and machines
-// as it guarantees the infra Providers contract.
+// IndexKey returns the required level of uniqueness
+// to represent and index machines uniquely from their node providerID.
 func (p *ProviderID) IndexKey() string {
-	return fmt.Sprintf("%s/%s", p.CloudProvider(), p.ID())
+	return p.String()
 }

controllers/noderefutil/providerid_test.go

@@ -23,6 +23,7 @@ import (
 )
 
 const aws = "aws"
+const azure = "azure"
 
 func TestNewProviderID(t *testing.T) {
 	tests := []struct {
@@ -115,19 +116,53 @@ func TestInvalidProviderID(t *testing.T) {
 func TestProviderIDEquals(t *testing.T) {
 	g := NewWithT(t)
 
-	input1 := "aws:////instance-id1"
-	parsed1, err := NewProviderID(input1)
+	inputAWS1 := "aws:////instance-id1"
+	parsedAWS1, err := NewProviderID(inputAWS1)
 	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(parsed1.String()).To(Equal(input1))
-	g.Expect(parsed1.ID()).To(Equal("instance-id1"))
-	g.Expect(parsed1.CloudProvider()).To(Equal(aws))
+	g.Expect(parsedAWS1.String()).To(Equal(inputAWS1))
+	g.Expect(parsedAWS1.ID()).To(Equal("instance-id1"))
+	g.Expect(parsedAWS1.CloudProvider()).To(Equal(aws))
 
-	input2 := "aws:///us-west-1/instance-id1"
-	parsed2, err := NewProviderID(input2)
+	inputAWS2 := "aws:///us-west-1/instance-id1"
+	parsedAWS2, err := NewProviderID(inputAWS2)
 	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(parsed2.String()).To(Equal(input2))
-	g.Expect(parsed2.ID()).To(Equal("instance-id1"))
-	g.Expect(parsed2.CloudProvider()).To(Equal(aws))
+	g.Expect(parsedAWS2.String()).To(Equal(inputAWS2))
+	g.Expect(parsedAWS2.ID()).To(Equal("instance-id1"))
+	g.Expect(parsedAWS2.CloudProvider()).To(Equal(aws))
 
-	g.Expect(parsed1.Equals(parsed2)).To(BeTrue())
+	// Test for inequality
+	g.Expect(parsedAWS1.Equals(parsedAWS2)).To(BeFalse())
+
+	inputAzure1 := "azure:///subscriptions/4920076a-ba9f-11ec-8422-0242ac120002/resourceGroups/default-template/providers/Microsoft.Compute/virtualMachines/default-template-control-plane-fhrvh"
+	parsedAzure1, err := NewProviderID(inputAzure1)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(parsedAzure1.String()).To(Equal(inputAzure1))
+	g.Expect(parsedAzure1.ID()).To(Equal("default-template-control-plane-fhrvh"))
+	g.Expect(parsedAzure1.CloudProvider()).To(Equal(azure))
+
+	inputAzure2 := inputAzure1
+	parsedAzure2, err := NewProviderID(inputAzure2)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	// Test for equality
+	g.Expect(parsedAzure1.Equals(parsedAzure2)).To(BeTrue())
+
+	// Here we ensure that two different ProviderID strings that happen to have the same 'ID' are not equal
+	// We use Azure VMSS as an example, two different '0' VMs in different pools: k8s-pool1-vmss, and k8s-pool2-vmss
+	inputAzureVMFromOneVMSS := "azure:///subscriptions/4920076a-ba9f-11ec-8422-0242ac120002/resourceGroups/default-template/providers/Microsoft.Compute/virtualMachineScaleSets/k8s-pool1-vmss/virtualMachines/0"
+	inputAzureVMFromAnotherVMSS := "azure:///subscriptions/4920076a-ba9f-11ec-8422-0242ac120002/resourceGroups/default-template/providers/Microsoft.Compute/virtualMachineScaleSets/k8s-pool2-vmss/virtualMachines/0"
+	parsedAzureVMFromOneVMSS, err := NewProviderID(inputAzureVMFromOneVMSS)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(parsedAzureVMFromOneVMSS.String()).To(Equal(inputAzureVMFromOneVMSS))
+	g.Expect(parsedAzureVMFromOneVMSS.ID()).To(Equal("0"))
+	g.Expect(parsedAzureVMFromOneVMSS.CloudProvider()).To(Equal(azure))
+
+	parsedAzureVMFromAnotherVMSS, err := NewProviderID(inputAzureVMFromAnotherVMSS)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(parsedAzureVMFromAnotherVMSS.String()).To(Equal(inputAzureVMFromAnotherVMSS))
+	g.Expect(parsedAzureVMFromAnotherVMSS.ID()).To(Equal("0"))
+	g.Expect(parsedAzureVMFromAnotherVMSS.CloudProvider()).To(Equal(azure))
+
+	// Test for inequality
+	g.Expect(parsedAzureVMFromOneVMSS.Equals(parsedAzureVMFromAnotherVMSS)).To(BeFalse())
 }

docs/book/src/developer/providers/v1.2-to-v1.3.md

@@ -62,3 +62,4 @@ The default value is 0, meaning that the volume can be detached without any time
   * `COREDNS_VERSION_UPGRADE_TO`
   The variable `SkipUpgrade` could be set to revert to the old behaviour by making use of the `KUBERNETES_VERSION` variable and skipping the kubernetes upgrade.
 - cert-manager upgraded from v1.9.1 to v1.10.0.
+- Machine `providerID` is now being strictly checked for equality when compared against Kubernetes node `providerID` data. This is the expected criteria for correlating a Cluster API machine to its corresponding Kubernetes node, but historically this comparison was not strict, and instead compared only against the `ID` substring part of the full `providerID` string. Because different providers construct `providerID` strings differently, the `ID` substring is not uniformly defined and implemented across providers, and thus the existing `providerID` equality can not guarantee the correct Machine-Node correlation. It is very unlikely that this new behavior will break existing providers, but FYI: if strict `providerID` equality will degrade expected behaviors, you may need to update your provider implementation prior to adopting Cluster API v1.3.

internal/controllers/machine/machine_controller_noderef_test.go

@@ -81,7 +81,7 @@ func TestGetNode(t *testing.T) {
 					ProviderID: "aws://us-west-2/test-get-node-2",
 				},
 			},
-			providerIDInput: "aws:///test-get-node-2",
+			providerIDInput: "aws://us-west-2/test-get-node-2",
 		},
 		{
 			name: "gce prefix, cloudProvider and ID matches",
@@ -93,7 +93,7 @@ func TestGetNode(t *testing.T) {
 					ProviderID: "gce://us-central1/test-get-node-2",
 				},
 			},
-			providerIDInput: "gce:///test-get-node-2",
+			providerIDInput: "gce://us-central1/test-get-node-2",
 		},
 		{
 			name: "Node is not found",

internal/controllers/machine/machine_controller_test.go

@@ -100,7 +100,7 @@ func TestWatches(t *testing.T) {
 			Namespace: ns.Name,
 		},
 		Spec: corev1.NodeSpec{
-			ProviderID: "test:///id-1",
+			ProviderID: "test://id-1",
 		},
 	}
 
@@ -1800,7 +1800,7 @@ func TestNodeToMachine(t *testing.T) {
 			Name: "test-node-to-machine-1",
 		},
 		Spec: corev1.NodeSpec{
-			ProviderID: "test:///id-1",
+			ProviderID: "test://id-1",
 		},
 	}
 
@@ -1809,7 +1809,7 @@ func TestNodeToMachine(t *testing.T) {
 			Name: "test-node-to-machine-node-2",
 		},
 		Spec: corev1.NodeSpec{
-			ProviderID: "test:///id-2",
+			ProviderID: "test://id-2",
 		},
 	}