From 626b4f4d970f92a4844323629e07e944ab7b9928 Mon Sep 17 00:00:00 2001 From: Jack Francis Date: Tue, 18 Oct 2022 09:17:19 -0700 Subject: [PATCH] validate ProviderID equality by comparing entire string --- controllers/noderefutil/providerid.go | 13 ++--- controllers/noderefutil/providerid_test.go | 57 +++++++++++++++---- .../src/developer/providers/v1.2-to-v1.3.md | 1 + .../machine_controller_noderef_test.go | 4 +- .../machine/machine_controller_test.go | 6 +- 5 files changed, 57 insertions(+), 24 deletions(-) diff --git a/controllers/noderefutil/providerid.go b/controllers/noderefutil/providerid.go index 8056331eed07..6c6dede690b8 100644 --- a/controllers/noderefutil/providerid.go +++ b/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() } diff --git a/controllers/noderefutil/providerid_test.go b/controllers/noderefutil/providerid_test.go index d5ae29478713..77a77a831a4f 100644 --- a/controllers/noderefutil/providerid_test.go +++ b/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()) } diff --git a/docs/book/src/developer/providers/v1.2-to-v1.3.md b/docs/book/src/developer/providers/v1.2-to-v1.3.md index 62dd28f9f34e..1f0a24559b17 100644 --- a/docs/book/src/developer/providers/v1.2-to-v1.3.md +++ b/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. diff --git a/internal/controllers/machine/machine_controller_noderef_test.go b/internal/controllers/machine/machine_controller_noderef_test.go index 2cf369dac81f..6eedb805bf64 100644 --- a/internal/controllers/machine/machine_controller_noderef_test.go +++ b/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", diff --git a/internal/controllers/machine/machine_controller_test.go b/internal/controllers/machine/machine_controller_test.go index 56b644044130..203e817476d6 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/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", }, }