Merge pull request #5782 from k8s-infra-cherrypick-robot/cherry-pick-5780-to-release-1.20

[release-1.20] Check for VM assigned identities without API calls

Author: k8s-ci-robot

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,
+			})
 		}
 	}
 

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) {

azure/services/virtualmachines/virtualmachines_test.go

@@ -27,15 +27,13 @@ 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"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 
 	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)
 		})
 	}
 }