From 364a7bf4d279f4f3f4ab0b6b611a3d63dbc76ed3 Mon Sep 17 00:00:00 2001 From: Jack Francis Date: Tue, 12 Apr 2022 13:34:19 -0700 Subject: [PATCH] validate ProviderID equality by comparing entire string --- controllers/noderefutil/providerid.go | 11 ++---- controllers/noderefutil/providerid_test.go | 38 +++++++++++++------ .../machine_controller_noderef_test.go | 4 +- .../machine/machine_controller_test.go | 6 +-- 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/controllers/noderefutil/providerid.go b/controllers/noderefutil/providerid.go index 4137d5de1de7..99d8a6162582 100644 --- a/controllers/noderefutil/providerid.go +++ b/controllers/noderefutil/providerid.go @@ -19,7 +19,6 @@ package noderefutil import ( "errors" - "fmt" "regexp" "strings" ) @@ -89,7 +88,7 @@ func (p *ProviderID) ID() string { // Equals returns true if both the CloudProvider and ID match. 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..51ae31f9384b 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,34 @@ 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 := "azure:///subscriptions/4920076a-ba9f-11ec-8422-0242ac120002/resourceGroups/default-template/providers/Microsoft.Compute/virtualMachines/default-template-control-plane-fhrvh" + parsedAzure2, err := NewProviderID(inputAzure2) + g.Expect(err).NotTo(HaveOccurred()) + + // Test for equalitey + g.Expect(parsedAzure1.Equals(parsedAzure2)).To(BeTrue()) } 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 3d9f7c38a729..957a28db88d2 100644 --- a/internal/controllers/machine/machine_controller_test.go +++ b/internal/controllers/machine/machine_controller_test.go @@ -96,7 +96,7 @@ func TestWatches(t *testing.T) { Namespace: ns.Name, }, Spec: corev1.NodeSpec{ - ProviderID: "test:///id-1", + ProviderID: "test://id-1", }, } @@ -1671,7 +1671,7 @@ func TestNodeToMachine(t *testing.T) { Name: "test-node-to-machine-1", }, Spec: corev1.NodeSpec{ - ProviderID: "test:///id-1", + ProviderID: "test://id-1", }, } @@ -1680,7 +1680,7 @@ func TestNodeToMachine(t *testing.T) { Name: "test-node-to-machine-node-2", }, Spec: corev1.NodeSpec{ - ProviderID: "test:///id-2", + ProviderID: "test://id-2", }, }