diff --git a/azure/converters/vm.go b/azure/converters/vm.go index 07a70d35220..26129e4b546 100644 --- a/azure/converters/vm.go +++ b/azure/converters/vm.go @@ -67,12 +67,10 @@ func SDKToVM(v armcompute.VirtualMachine) *VM { } if v.Identity != nil { - for _, identity := range v.Identity.UserAssignedIdentities { - if identity != nil && identity.ClientID != nil { - vm.UserAssignedIdentities = append(vm.UserAssignedIdentities, infrav1.UserAssignedIdentity{ - ProviderID: *identity.ClientID, - }) - } + for providerID := range v.Identity.UserAssignedIdentities { + vm.UserAssignedIdentities = append(vm.UserAssignedIdentities, infrav1.UserAssignedIdentity{ + ProviderID: providerID, + }) } } diff --git a/azure/services/virtualmachines/virtualmachines.go b/azure/services/virtualmachines/virtualmachines.go index 64623d494a3..dcf2633f40d 100644 --- a/azure/services/virtualmachines/virtualmachines.go +++ b/azure/services/virtualmachines/virtualmachines.go @@ -32,7 +32,6 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/converters" "sigs.k8s.io/cluster-api-provider-azure/azure/services/async" - "sigs.k8s.io/cluster-api-provider-azure/azure/services/identities" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure" @@ -60,7 +59,6 @@ type Service struct { async.Reconciler interfacesGetter async.Getter publicIPsGetter async.Getter - identitiesGetter identities.Client } // New creates a new service. @@ -69,10 +67,6 @@ func New(scope VMScope) (*Service, error) { if err != nil { return nil, err } - identitiesSvc, err := identities.NewClient(scope) - if err != nil { - return nil, err - } interfacesSvc, err := networkinterfaces.NewClient(scope, scope.DefaultedAzureCallTimeout()) if err != nil { return nil, err @@ -85,7 +79,6 @@ func New(scope VMScope) (*Service, error) { Scope: scope, interfacesGetter: interfacesSvc, publicIPsGetter: publicIPsSvc, - identitiesGetter: identitiesSvc, Reconciler: async.New[armcompute.VirtualMachinesClientCreateOrUpdateResponse, armcompute.VirtualMachinesClientDeleteResponse](scope, Client, Client), }, nil @@ -140,10 +133,7 @@ func (s *Service) Reconcile(ctx context.Context) error { return errors.Errorf("%T is not a valid VM spec", vmSpec) } - err = s.checkUserAssignedIdentities(ctx, spec.UserAssignedIdentities, infraVM.UserAssignedIdentities) - if err != nil { - return errors.Wrap(err, "failed to check user assigned identities") - } + s.checkUserAssignedIdentities(spec.UserAssignedIdentities, infraVM.UserAssignedIdentities) } return err } @@ -171,45 +161,22 @@ func (s *Service) Delete(ctx context.Context) error { return err } -func (s *Service) checkUserAssignedIdentities(ctx context.Context, specIdentities []infrav1.UserAssignedIdentity, vmIdentities []infrav1.UserAssignedIdentity) error { - expectedMap := make(map[string]struct{}) +func (s *Service) checkUserAssignedIdentities(specIdentities []infrav1.UserAssignedIdentity, vmIdentities []infrav1.UserAssignedIdentity) { actualMap := make(map[string]struct{}) - // Create a map of the expected identities. The ProviderID is converted to match the format of the VM identity. - for _, expectedIdentity := range specIdentities { - identitiesClient := s.identitiesGetter - parsed, err := azureutil.ParseResourceID(expectedIdentity.ProviderID) - if err != nil { - return err - } - if parsed.SubscriptionID != s.Scope.SubscriptionID() { - identitiesClient, err = identities.NewClientBySub(s.Scope, parsed.SubscriptionID) - if err != nil { - return errors.Wrapf(err, "failed to create identities client from subscription ID %s", parsed.SubscriptionID) - } - } - expectedClientID, err := identitiesClient.GetClientID(ctx, expectedIdentity.ProviderID) - if err != nil { - return errors.Wrap(err, "failed to get client ID") - } - expectedMap[expectedClientID] = struct{}{} - } - // Create a map of the actual identities from the vm. for _, actualIdentity := range vmIdentities { actualMap[actualIdentity.ProviderID] = struct{}{} } // Check if the expected identities are present in the vm. - for expectedKey := range expectedMap { - _, exists := actualMap[expectedKey] + for _, expectedIdentity := range specIdentities { + _, exists := actualMap[expectedIdentity.ProviderID] if !exists { - s.Scope.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+expectedKey) - return nil + s.Scope.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+expectedIdentity.ProviderID) + return } } - - return nil } func (s *Service) getAddresses(ctx context.Context, vm armcompute.VirtualMachine, rgName string) ([]corev1.NodeAddress, error) { diff --git a/azure/services/virtualmachines/virtualmachines_test.go b/azure/services/virtualmachines/virtualmachines_test.go index 4da42df90b2..a3981683afe 100644 --- a/azure/services/virtualmachines/virtualmachines_test.go +++ b/azure/services/virtualmachines/virtualmachines_test.go @@ -27,7 +27,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v4" . "github.com/onsi/gomega" - "github.com/pkg/errors" "go.uber.org/mock/gomock" corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" @@ -35,7 +34,6 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async" - "sigs.k8s.io/cluster-api-provider-azure/azure/services/identities/mock_identities" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines/mock_virtualmachines" @@ -317,106 +315,56 @@ func TestCheckUserAssignedIdentities(t *testing.T) { name string specIdentities []infrav1.UserAssignedIdentity actualIdentities []infrav1.UserAssignedIdentity - expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) - expectedError string + expectedKey string }{ { name: "no user assigned identities", specIdentities: []infrav1.UserAssignedIdentity{}, actualIdentities: []infrav1.UserAssignedIdentity{}, - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) { - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil) - }, - expectedError: "", }, { name: "matching user assigned identities", specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) { - s.SubscriptionID().Return("123") - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil) - }, - expectedError: "", }, { name: "less user assigned identities than expected", specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity, fakeUserAssignedIdentity2}, actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) { - s.SubscriptionID().AnyTimes().Return("123") - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil) - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity2.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity2.ProviderID, nil) - s.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+fakeUserAssignedIdentity2.ProviderID).Times(1) - }, - expectedError: "", + expectedKey: fakeUserAssignedIdentity2.ProviderID, }, { name: "more user assigned identities than expected", specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity, fakeUserAssignedIdentity2}, - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) { - s.SubscriptionID().Return("123") - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil) - }, - expectedError: "", }, { name: "mismatched user assigned identities by content", specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity2}, - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) { - s.SubscriptionID().Return("123") - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil) - s.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+fakeUserAssignedIdentity.ProviderID).Times(1) - }, - expectedError: "", + expectedKey: fakeUserAssignedIdentity.ProviderID, }, { name: "duplicate user assigned identity in spec", specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity, fakeUserAssignedIdentity}, actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) { - s.SubscriptionID().AnyTimes().Return("123") - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil) - }, - expectedError: "", - }, - { - name: "invalid client id", - specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, - actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity}, - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) { - s.SubscriptionID().Return("123") - i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return("", errors.New("failed to get client id")) - }, - expectedError: "failed to get client id", }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - g := NewWithT(t) t.Parallel() mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_virtualmachines.NewMockVMScope(mockCtrl) - asyncMock := mock_async.NewMockReconciler(mockCtrl) - identitiesMock := mock_identities.NewMockClient(mockCtrl) - tc.expect(scopeMock.EXPECT(), identitiesMock.EXPECT()) + if tc.expectedKey != "" { + scopeMock.EXPECT().SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+tc.expectedKey).Times(1) + } s := &Service{ - Scope: scopeMock, - Reconciler: asyncMock, - identitiesGetter: identitiesMock, + Scope: scopeMock, } - err := s.checkUserAssignedIdentities(context.TODO(), tc.specIdentities, tc.actualIdentities) - if tc.expectedError != "" { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tc.expectedError)) - } else { - g.Expect(err).NotTo(HaveOccurred()) - } + s.checkUserAssignedIdentities(tc.specIdentities, tc.actualIdentities) }) } }