Skip to content

Commit d37d921

Browse files
nojnhuhnojnhuh
committed
Check for VM assigned identities without API calls
1 parent 65d0034 commit d37d921

File tree

3 files changed

+19
-105
lines changed

3 files changed

+19
-105
lines changed

‎azure/converters/vm.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,10 @@ func SDKToVM(v armcompute.VirtualMachine) *VM {
6767
}
6868

6969
if v.Identity != nil {
70-
for _, identity := range v.Identity.UserAssignedIdentities {
71-
if identity != nil && identity.ClientID != nil {
72-
vm.UserAssignedIdentities = append(vm.UserAssignedIdentities, infrav1.UserAssignedIdentity{
73-
ProviderID: *identity.ClientID,
74-
})
75-
}
70+
for providerID := range v.Identity.UserAssignedIdentities {
71+
vm.UserAssignedIdentities = append(vm.UserAssignedIdentities, infrav1.UserAssignedIdentity{
72+
ProviderID: providerID,
73+
})
7674
}
7775
}
7876

‎azure/services/virtualmachines/virtualmachines.go

Lines changed: 6 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ import (
3232
"sigs.k8s.io/cluster-api-provider-azure/azure"
3333
"sigs.k8s.io/cluster-api-provider-azure/azure/converters"
3434
"sigs.k8s.io/cluster-api-provider-azure/azure/services/async"
35-
"sigs.k8s.io/cluster-api-provider-azure/azure/services/identities"
3635
"sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces"
3736
"sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips"
3837
azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure"
@@ -60,7 +59,6 @@ type Service struct {
6059
async.Reconciler
6160
interfacesGetter async.Getter
6261
publicIPsGetter async.Getter
63-
identitiesGetter identities.Client
6462
}
6563

6664
// New creates a new service.
@@ -69,10 +67,6 @@ func New(scope VMScope) (*Service, error) {
6967
if err != nil {
7068
return nil, err
7169
}
72-
identitiesSvc, err := identities.NewClient(scope)
73-
if err != nil {
74-
return nil, err
75-
}
7670
interfacesSvc, err := networkinterfaces.NewClient(scope, scope.DefaultedAzureCallTimeout())
7771
if err != nil {
7872
return nil, err
@@ -85,7 +79,6 @@ func New(scope VMScope) (*Service, error) {
8579
Scope: scope,
8680
interfacesGetter: interfacesSvc,
8781
publicIPsGetter: publicIPsSvc,
88-
identitiesGetter: identitiesSvc,
8982
Reconciler: async.New[armcompute.VirtualMachinesClientCreateOrUpdateResponse,
9083
armcompute.VirtualMachinesClientDeleteResponse](scope, Client, Client),
9184
}, nil
@@ -140,10 +133,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
140133
return errors.Errorf("%T is not a valid VM spec", vmSpec)
141134
}
142135

143-
err = s.checkUserAssignedIdentities(ctx, spec.UserAssignedIdentities, infraVM.UserAssignedIdentities)
144-
if err != nil {
145-
return errors.Wrap(err, "failed to check user assigned identities")
146-
}
136+
s.checkUserAssignedIdentities(spec.UserAssignedIdentities, infraVM.UserAssignedIdentities)
147137
}
148138
return err
149139
}
@@ -171,45 +161,22 @@ func (s *Service) Delete(ctx context.Context) error {
171161
return err
172162
}
173163

174-
func (s *Service) checkUserAssignedIdentities(ctx context.Context, specIdentities []infrav1.UserAssignedIdentity, vmIdentities []infrav1.UserAssignedIdentity) error {
175-
expectedMap := make(map[string]struct{})
164+
func (s *Service) checkUserAssignedIdentities(specIdentities []infrav1.UserAssignedIdentity, vmIdentities []infrav1.UserAssignedIdentity) {
176165
actualMap := make(map[string]struct{})
177166

178-
// Create a map of the expected identities. The ProviderID is converted to match the format of the VM identity.
179-
for _, expectedIdentity := range specIdentities {
180-
identitiesClient := s.identitiesGetter
181-
parsed, err := azureutil.ParseResourceID(expectedIdentity.ProviderID)
182-
if err != nil {
183-
return err
184-
}
185-
if parsed.SubscriptionID != s.Scope.SubscriptionID() {
186-
identitiesClient, err = identities.NewClientBySub(s.Scope, parsed.SubscriptionID)
187-
if err != nil {
188-
return errors.Wrapf(err, "failed to create identities client from subscription ID %s", parsed.SubscriptionID)
189-
}
190-
}
191-
expectedClientID, err := identitiesClient.GetClientID(ctx, expectedIdentity.ProviderID)
192-
if err != nil {
193-
return errors.Wrap(err, "failed to get client ID")
194-
}
195-
expectedMap[expectedClientID] = struct{}{}
196-
}
197-
198167
// Create a map of the actual identities from the vm.
199168
for _, actualIdentity := range vmIdentities {
200169
actualMap[actualIdentity.ProviderID] = struct{}{}
201170
}
202171

203172
// Check if the expected identities are present in the vm.
204-
for expectedKey := range expectedMap {
205-
_, exists := actualMap[expectedKey]
173+
for _, expectedIdentity := range specIdentities {
174+
_, exists := actualMap[expectedIdentity.ProviderID]
206175
if !exists {
207-
s.Scope.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+expectedKey)
208-
return nil
176+
s.Scope.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+expectedIdentity.ProviderID)
177+
return
209178
}
210179
}
211-
212-
return nil
213180
}
214181

215182
func (s *Service) getAddresses(ctx context.Context, vm armcompute.VirtualMachine, rgName string) ([]corev1.NodeAddress, error) {

‎azure/services/virtualmachines/virtualmachines_test.go

Lines changed: 9 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,13 @@ import (
2727
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5"
2828
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v4"
2929
. "github.com/onsi/gomega"
30-
"github.com/pkg/errors"
3130
"go.uber.org/mock/gomock"
3231
corev1 "k8s.io/api/core/v1"
3332
"k8s.io/utils/ptr"
3433
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3534

3635
infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
3736
"sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async"
38-
"sigs.k8s.io/cluster-api-provider-azure/azure/services/identities/mock_identities"
3937
"sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces"
4038
"sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips"
4139
"sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines/mock_virtualmachines"
@@ -317,106 +315,57 @@ func TestCheckUserAssignedIdentities(t *testing.T) {
317315
name string
318316
specIdentities []infrav1.UserAssignedIdentity
319317
actualIdentities []infrav1.UserAssignedIdentity
320-
expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder)
321-
expectedError string
318+
expectedKey string
319+
expect func(s *mock_virtualmachines.MockVMScopeMockRecorder)
322320
}{
323321
{
324322
name: "no user assigned identities",
325323
specIdentities: []infrav1.UserAssignedIdentity{},
326324
actualIdentities: []infrav1.UserAssignedIdentity{},
327-
expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) {
328-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil)
329-
},
330-
expectedError: "",
331325
},
332326
{
333327
name: "matching user assigned identities",
334328
specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
335329
actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
336-
expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) {
337-
s.SubscriptionID().Return("123")
338-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil)
339-
},
340-
expectedError: "",
341330
},
342331
{
343332
name: "less user assigned identities than expected",
344333
specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity, fakeUserAssignedIdentity2},
345334
actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
346-
expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) {
347-
s.SubscriptionID().AnyTimes().Return("123")
348-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil)
349-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity2.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity2.ProviderID, nil)
350-
s.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+fakeUserAssignedIdentity2.ProviderID).Times(1)
351-
},
352-
expectedError: "",
335+
expectedKey: fakeUserAssignedIdentity2.ProviderID,
353336
},
354337
{
355338
name: "more user assigned identities than expected",
356339
specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
357340
actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity, fakeUserAssignedIdentity2},
358-
expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) {
359-
s.SubscriptionID().Return("123")
360-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil)
361-
},
362-
expectedError: "",
363341
},
364342
{
365343
name: "mismatched user assigned identities by content",
366344
specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
367345
actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity2},
368-
expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) {
369-
s.SubscriptionID().Return("123")
370-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil)
371-
s.SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+fakeUserAssignedIdentity.ProviderID).Times(1)
372-
},
373-
expectedError: "",
346+
expectedKey: fakeUserAssignedIdentity.ProviderID,
374347
},
375348
{
376349
name: "duplicate user assigned identity in spec",
377350
specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity, fakeUserAssignedIdentity},
378351
actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
379-
expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) {
380-
s.SubscriptionID().AnyTimes().Return("123")
381-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return(fakeUserAssignedIdentity.ProviderID, nil)
382-
},
383-
expectedError: "",
384-
},
385-
{
386-
name: "invalid client id",
387-
specIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
388-
actualIdentities: []infrav1.UserAssignedIdentity{fakeUserAssignedIdentity},
389-
expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, i *mock_identities.MockClientMockRecorder) {
390-
s.SubscriptionID().Return("123")
391-
i.GetClientID(gomockinternal.AContext(), fakeUserAssignedIdentity.ProviderID).AnyTimes().Return("", errors.New("failed to get client id"))
392-
},
393-
expectedError: "failed to get client id",
394352
},
395353
}
396354
for _, tc := range testcases {
397355
t.Run(tc.name, func(t *testing.T) {
398-
g := NewWithT(t)
399356
t.Parallel()
400357
mockCtrl := gomock.NewController(t)
401358
defer mockCtrl.Finish()
402359
scopeMock := mock_virtualmachines.NewMockVMScope(mockCtrl)
403-
asyncMock := mock_async.NewMockReconciler(mockCtrl)
404-
identitiesMock := mock_identities.NewMockClient(mockCtrl)
405360

406-
tc.expect(scopeMock.EXPECT(), identitiesMock.EXPECT())
361+
if tc.expectedKey != "" {
362+
scopeMock.EXPECT().SetConditionFalse(infrav1.VMIdentitiesReadyCondition, infrav1.UserAssignedIdentityMissingReason, clusterv1.ConditionSeverityWarning, vmMissingUAI+tc.expectedKey).Times(1)
363+
}
407364
s := &Service{
408-
Scope: scopeMock,
409-
Reconciler: asyncMock,
410-
identitiesGetter: identitiesMock,
365+
Scope: scopeMock,
411366
}
412367

413-
err := s.checkUserAssignedIdentities(context.TODO(), tc.specIdentities, tc.actualIdentities)
414-
if tc.expectedError != "" {
415-
g.Expect(err).To(HaveOccurred())
416-
g.Expect(err.Error()).To(ContainSubstring(tc.expectedError))
417-
} else {
418-
g.Expect(err).NotTo(HaveOccurred())
419-
}
368+
s.checkUserAssignedIdentities(tc.specIdentities, tc.actualIdentities)
420369
})
421370
}
422371
}

0 commit comments

Comments
 (0)