From 18ae8feb2eec9f51cc8a0ab6f0183717991bcc47 Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Tue, 6 Apr 2021 15:06:14 -0700 Subject: [PATCH] Fix VM provider ID to match node ID --- cloud/converters/identity.go | 6 ++++-- cloud/converters/identity_test.go | 8 ++++---- cloud/defaults.go | 6 ++++++ cloud/scope/machinepool.go | 5 ++--- cloud/services/scalesets/scalesets.go | 4 ++-- cloud/services/scalesets/scalesets_test.go | 9 ++++----- cloud/services/virtualmachines/virtualmachines.go | 2 +- exp/controllers/azuremanagedmachinepool_reconciler.go | 2 +- 8 files changed, 24 insertions(+), 18 deletions(-) diff --git a/cloud/converters/identity.go b/cloud/converters/identity.go index 37b27bd6422..da4647d920a 100644 --- a/cloud/converters/identity.go +++ b/cloud/converters/identity.go @@ -19,6 +19,8 @@ package converters import ( "strings" + azure "sigs.k8s.io/cluster-api-provider-azure/cloud" + "github.com/Azure/azure-sdk-for-go/profiles/latest/compute/mgmt/compute" "github.com/pkg/errors" @@ -59,7 +61,7 @@ func UserAssignedIdentitiesToVMSSSDK(identities []infrav1.UserAssignedIdentity) return userIdentitiesMap, nil } -// sanitized removes "azure:///" prefix from the given id +// sanitized removes "azure://" prefix from the given id func sanitized(id string) string { - return strings.TrimPrefix(id, "azure:///") + return strings.TrimPrefix(id, azure.ProviderIDPrefix) } diff --git a/cloud/converters/identity_test.go b/cloud/converters/identity_test.go index cdb9ab37757..070d25379b9 100644 --- a/cloud/converters/identity_test.go +++ b/cloud/converters/identity_test.go @@ -38,14 +38,14 @@ var sampleSubjectFactory = []infrav1.UserAssignedIdentity{ } var expectedVMSDKObject = map[string]*compute.VirtualMachineIdentityUserAssignedIdentitiesValue{ - "foo": {}, - "bar": {}, + "/foo": {}, + "/bar": {}, "/without/prefix": {}, } var expectedVMSSSDKObject = map[string]*compute.VirtualMachineScaleSetIdentityUserAssignedIdentitiesValue{ - "foo": {}, - "bar": {}, + "/foo": {}, + "/bar": {}, "/without/prefix": {}, } diff --git a/cloud/defaults.go b/cloud/defaults.go index f7172d214e1..32b1ca87a9c 100644 --- a/cloud/defaults.go +++ b/cloud/defaults.go @@ -65,6 +65,12 @@ const ( ControlPlaneNodeGroup = "control-plane" ) +const ( + // ProviderIDPrefix will be appended to the beginning of Azure resource IDs to form the Kubernetes Provider ID. + // NOTE: this format matches the 2 slashes format used in cloud-provider and cluster-autoscaler. + ProviderIDPrefix = "azure://" +) + // GenerateBackendAddressPoolName generates a load balancer backend address pool name. func GenerateBackendAddressPoolName(lbName string) string { return fmt.Sprintf("%s-%s", lbName, "backendPool") diff --git a/cloud/scope/machinepool.go b/cloud/scope/machinepool.go index 6c56edb3939..9b4947ecfd6 100644 --- a/cloud/scope/machinepool.go +++ b/cloud/scope/machinepool.go @@ -19,7 +19,6 @@ package scope import ( "context" "encoding/base64" - "fmt" "github.com/Azure/go-autorest/autorest/to" "github.com/go-logr/logr" @@ -168,7 +167,7 @@ func (m *MachinePoolScope) UpdateInstanceStatuses(ctx context.Context, instances providerIDs := make([]string, len(instances)) for i, instance := range instances { - providerIDs[i] = fmt.Sprintf("azure://%s", instance.ID) + providerIDs[i] = azure.ProviderIDPrefix + instance.ID } nodeStatusByProviderID, err := m.getNodeStatusByProviderID(ctx, providerIDs) @@ -180,7 +179,7 @@ func (m *MachinePoolScope) UpdateInstanceStatuses(ctx context.Context, instances instanceStatuses := make([]*infrav1exp.AzureMachinePoolInstanceStatus, len(instances)) for i, instance := range instances { instanceStatuses[i] = &infrav1exp.AzureMachinePoolInstanceStatus{ - ProviderID: fmt.Sprintf("azure://%s", instance.ID), + ProviderID: azure.ProviderIDPrefix + instance.ID, InstanceID: instance.InstanceID, InstanceName: instance.Name, ProvisioningState: &instance.State, diff --git a/cloud/services/scalesets/scalesets.go b/cloud/services/scalesets/scalesets.go index 7592d5fc0bd..df594584353 100644 --- a/cloud/services/scalesets/scalesets.go +++ b/cloud/services/scalesets/scalesets.go @@ -117,7 +117,7 @@ func (s *Service) Reconcile(ctx context.Context) error { } default: // just in case, set the provider ID if the instance exists - s.Scope.SetProviderID(fmt.Sprintf("azure://%s", fetchedVMSS.ID)) + s.Scope.SetProviderID(azure.ProviderIDPrefix + fetchedVMSS.ID) } // Try to get the VMSS to update status if we have created a long running operation. If the VMSS is still in a long @@ -193,7 +193,7 @@ func (s *Service) patchVMSSIfNeeded(ctx context.Context, infraVMSS *infrav1exp.V ctx, span := tele.Tracer().Start(ctx, "scalesets.Service.patchVMSSIfNeeded") defer span.End() - s.Scope.SetProviderID(fmt.Sprintf("azure://%s", infraVMSS.ID)) + s.Scope.SetProviderID(azure.ProviderIDPrefix + infraVMSS.ID) spec := s.Scope.ScaleSetSpec() result, err := s.buildVMSSFromSpec(ctx, spec) diff --git a/cloud/services/scalesets/scalesets_test.go b/cloud/services/scalesets/scalesets_test.go index 8361635bbf7..b46b6697bf4 100644 --- a/cloud/services/scalesets/scalesets_test.go +++ b/cloud/services/scalesets/scalesets_test.go @@ -18,7 +18,6 @@ package scalesets import ( "context" - "fmt" "net/http" "testing" @@ -258,7 +257,7 @@ func TestReconcileVMSS(t *testing.T) { createdVMSS := newDefaultVMSS() instances := newDefaultInstances() createdVMSS = setupDefaultVMSSInProgressOperationDoneExpectations(g, s, m, createdVMSS, instances) - s.SetProviderID(fmt.Sprintf("azure://%s", *createdVMSS.ID)) + s.SetProviderID(azure.ProviderIDPrefix + *createdVMSS.ID) s.SetLongRunningOperationState(nil) s.SetProvisioningState(infrav1.VMStateSucceeded) s.NeedsK8sVersionUpdate().Return(false) @@ -279,7 +278,7 @@ func TestReconcileVMSS(t *testing.T) { vmss := newDefaultVMSS() instances := newDefaultInstances() vmss = setupDefaultVMSSInProgressOperationDoneExpectations(g, s, m, vmss, instances) - s.SetProviderID(fmt.Sprintf("azure://%s", *vmss.ID)) + s.SetProviderID(azure.ProviderIDPrefix + *vmss.ID) s.SetProvisioningState(infrav1.VMStateUpdating) // create a VMSS patch with an updated hash to match the spec @@ -387,7 +386,7 @@ func TestReconcileVMSS(t *testing.T) { spec.Identity = infrav1.VMIdentityUserAssigned spec.UserAssignedIdentities = []infrav1.UserAssignedIdentity{ { - ProviderID: "azure:////subscriptions/123/resourcegroups/456/providers/Microsoft.ManagedIdentity/userAssignedIdentities/id1", + ProviderID: "azure:///subscriptions/123/resourcegroups/456/providers/Microsoft.ManagedIdentity/userAssignedIdentities/id1", }, } s.ScaleSetSpec().Return(spec).AnyTimes() @@ -1009,7 +1008,7 @@ func setupDefaultVMSSExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorde func setupDefaultVMSSUpdateExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorder) { setupDefaultVMSSExpectations(s) - s.SetProviderID("azure://vmss-id") + s.SetProviderID(azure.ProviderIDPrefix + "vmss-id") s.SetProvisioningState(infrav1.VMStateUpdating) s.GetLongRunningOperationState().Return(nil) } diff --git a/cloud/services/virtualmachines/virtualmachines.go b/cloud/services/virtualmachines/virtualmachines.go index 0c25023aaab..828e6a622e8 100644 --- a/cloud/services/virtualmachines/virtualmachines.go +++ b/cloud/services/virtualmachines/virtualmachines.go @@ -93,7 +93,7 @@ func (s *Service) Reconcile(ctx context.Context) error { return errors.Wrapf(err, "failed to get VM %s", vmSpec.Name) case err == nil: // VM already exists, update the spec and skip creation. - s.Scope.SetProviderID(fmt.Sprintf("azure:///%s", existingVM.ID)) + s.Scope.SetProviderID(azure.ProviderIDPrefix + existingVM.ID) s.Scope.SetAnnotation("cluster-api-provider-azure", "true") s.Scope.SetAddresses(existingVM.Addresses) s.Scope.SetVMState(existingVM.State) diff --git a/exp/controllers/azuremanagedmachinepool_reconciler.go b/exp/controllers/azuremanagedmachinepool_reconciler.go index 5e278eaf8f8..1107a2197de 100644 --- a/exp/controllers/azuremanagedmachinepool_reconciler.go +++ b/exp/controllers/azuremanagedmachinepool_reconciler.go @@ -152,7 +152,7 @@ func (s *azureManagedMachinePoolService) Reconcile(ctx context.Context, scope *s var providerIDs = make([]string, len(instances)) for i := 0; i < len(instances); i++ { - providerIDs[i] = fmt.Sprintf("azure://%s", *instances[i].ID) + providerIDs[i] = azure.ProviderIDPrefix + *instances[i].ID } scope.InfraMachinePool.Spec.ProviderIDList = providerIDs