diff --git a/azure/defaults.go b/azure/defaults.go index 84c24ab1f64..de2017bde91 100644 --- a/azure/defaults.go +++ b/azure/defaults.go @@ -82,6 +82,9 @@ 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://" + // azureBuiltInContributorID the ID of the Contributor role in Azure + // Ref: https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles + azureBuiltInContributorID = "b24988ac-6180-42a0-ab88-20f7382dd24c" ) const ( @@ -105,6 +108,16 @@ func GenerateBackendAddressPoolName(lbName string) string { return fmt.Sprintf("%s-%s", lbName, "backendPool") } +// GenerateSubscriptionScope generates a role assignment scope that applies to all resources in the subscription. +func GenerateSubscriptionScope(subscriptionID string) string { + return fmt.Sprintf("/subscriptions/%s/", subscriptionID) +} + +// GenerateContributorRoleDefinitionID generates the contributor role definition ID. +func GenerateContributorRoleDefinitionID(subscriptionID string) string { + return fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", subscriptionID, azureBuiltInContributorID) +} + // GenerateOutboundBackendAddressPoolName generates a load balancer outbound backend address pool name. func GenerateOutboundBackendAddressPoolName(lbName string) string { return fmt.Sprintf("%s-%s", lbName, "outboundBackendPool") diff --git a/azure/scope/machine.go b/azure/scope/machine.go index 5dcb09a4b66..318cafe4e49 100644 --- a/azure/scope/machine.go +++ b/azure/scope/machine.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines" "sigs.k8s.io/cluster-api-provider-azure/util/futures" "sigs.k8s.io/cluster-api-provider-azure/util/tele" @@ -288,17 +289,32 @@ func (m *MachineScope) DiskSpecs() []azure.ResourceSpecGetter { } // RoleAssignmentSpecs returns the role assignment specs. -func (m *MachineScope) RoleAssignmentSpecs() []azure.RoleAssignmentSpec { - if m.AzureMachine.Spec.Identity == infrav1.VMIdentitySystemAssigned { - return []azure.RoleAssignmentSpec{ - { - MachineName: m.Name(), - Name: m.AzureMachine.Spec.RoleAssignmentName, - ResourceType: azure.VirtualMachine, - }, +func (m *MachineScope) RoleAssignmentSpecs(principalID *string) []azure.ResourceSpecGetter { + roles := make([]azure.ResourceSpecGetter, 1) + if m.HasSystemAssignedIdentity() { + roles[0] = &roleassignments.RoleAssignmentSpec{ + Name: m.AzureMachine.Spec.RoleAssignmentName, + MachineName: m.Name(), + ResourceType: azure.VirtualMachine, + ResourceGroup: m.ResourceGroup(), + Scope: azure.GenerateSubscriptionScope(m.SubscriptionID()), + RoleDefinitionID: azure.GenerateContributorRoleDefinitionID(m.SubscriptionID()), + PrincipalID: principalID, } + return roles } - return []azure.RoleAssignmentSpec{} + return []azure.ResourceSpecGetter{} +} + +// RoleAssignmentResourceType returns the role assignment resource type. +func (m *MachineScope) RoleAssignmentResourceType() string { + return azure.VirtualMachine +} + +// HasSystemAssignedIdentity returns true if the azure machine has +// system assigned identity. +func (m *MachineScope) HasSystemAssignedIdentity() bool { + return m.AzureMachine.Spec.Identity == infrav1.VMIdentitySystemAssigned } // VMExtensionSpecs returns the vm extension specs. diff --git a/azure/scope/machine_test.go b/azure/scope/machine_test.go index fe4505a64b5..2126ad5a22c 100644 --- a/azure/scope/machine_test.go +++ b/azure/scope/machine_test.go @@ -35,6 +35,7 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) @@ -381,7 +382,7 @@ func TestMachineScope_RoleAssignmentSpecs(t *testing.T) { tests := []struct { name string machineScope MachineScope - want []azure.RoleAssignmentSpec + want []azure.ResourceSpecGetter }{ { name: "returns empty if VM identity is system assigned", @@ -393,7 +394,7 @@ func TestMachineScope_RoleAssignmentSpecs(t *testing.T) { }, }, }, - want: []azure.RoleAssignmentSpec{}, + want: []azure.ResourceSpecGetter{}, }, { name: "returns RoleAssignmentSpec if VM identity is not system assigned", @@ -408,19 +409,37 @@ func TestMachineScope_RoleAssignmentSpecs(t *testing.T) { RoleAssignmentName: "azure-role-assignment-name", }, }, + ClusterScoper: &ClusterScope{ + AzureClients: AzureClients{ + EnvironmentSettings: auth.EnvironmentSettings{ + Values: map[string]string{ + auth.SubscriptionID: "123", + }, + }, + }, + AzureCluster: &infrav1.AzureCluster{ + Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", + }, + }, + }, }, - want: []azure.RoleAssignmentSpec{ - { - MachineName: "machine-name", - Name: "azure-role-assignment-name", - ResourceType: azure.VirtualMachine, + want: []azure.ResourceSpecGetter{ + &roleassignments.RoleAssignmentSpec{ + ResourceType: azure.VirtualMachine, + MachineName: "machine-name", + Name: "azure-role-assignment-name", + ResourceGroup: "my-rg", + Scope: azure.GenerateSubscriptionScope("123"), + RoleDefinitionID: azure.GenerateContributorRoleDefinitionID("123"), + PrincipalID: to.StringPtr("fakePrincipalID"), }, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := tt.machineScope.RoleAssignmentSpecs(); !reflect.DeepEqual(got, tt.want) { + if got := tt.machineScope.RoleAssignmentSpecs(to.StringPtr("fakePrincipalID")); !reflect.DeepEqual(got, tt.want) { t.Errorf("RoleAssignmentSpecs() = %v, want %v", got, tt.want) } }) diff --git a/azure/scope/machinepool.go b/azure/scope/machinepool.go index e302ed1892e..145dae64f1b 100644 --- a/azure/scope/machinepool.go +++ b/azure/scope/machinepool.go @@ -31,6 +31,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" machinepool "sigs.k8s.io/cluster-api-provider-azure/azure/scope/strategies/machinepool_deployments" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/util/futures" "sigs.k8s.io/cluster-api-provider-azure/util/tele" @@ -569,17 +570,30 @@ func (m *MachinePoolScope) SaveVMImageToStatus(image *infrav1.Image) { } // RoleAssignmentSpecs returns the role assignment specs. -func (m *MachinePoolScope) RoleAssignmentSpecs() []azure.RoleAssignmentSpec { - if m.AzureMachinePool.Spec.Identity == infrav1.VMIdentitySystemAssigned { - return []azure.RoleAssignmentSpec{ - { - MachineName: m.Name(), - Name: m.AzureMachinePool.Spec.RoleAssignmentName, - ResourceType: azure.VirtualMachineScaleSet, - }, +func (m *MachinePoolScope) RoleAssignmentSpecs(principalID *string) []azure.ResourceSpecGetter { + roles := make([]azure.ResourceSpecGetter, 1) + if m.HasSystemAssignedIdentity() { + roles[0] = &roleassignments.RoleAssignmentSpec{ + Name: m.AzureMachinePool.Spec.RoleAssignmentName, + MachineName: m.Name(), + ResourceGroup: m.ResourceGroup(), + ResourceType: azure.VirtualMachineScaleSet, + PrincipalID: principalID, } + return roles } - return []azure.RoleAssignmentSpec{} + return []azure.ResourceSpecGetter{} +} + +// RoleAssignmentResourceType returns the role assignment resource type. +func (m *MachinePoolScope) RoleAssignmentResourceType() string { + return azure.VirtualMachineScaleSet +} + +// HasSystemAssignedIdentity returns true if the azure machine pool has system +// assigned identity. +func (m *MachinePoolScope) HasSystemAssignedIdentity() bool { + return m.AzureMachinePool.Spec.Identity == infrav1.VMIdentitySystemAssigned } // VMSSExtensionSpecs returns the vmss extension specs. diff --git a/azure/services/roleassignments/client.go b/azure/services/roleassignments/client.go index a084d2a3a1e..620e019ae4d 100644 --- a/azure/services/roleassignments/client.go +++ b/azure/services/roleassignments/client.go @@ -21,22 +21,17 @@ import ( "github.com/Azure/azure-sdk-for-go/profiles/2019-03-01/authorization/mgmt/authorization" "github.com/Azure/go-autorest/autorest" + azureautorest "github.com/Azure/go-autorest/autorest/azure" + "github.com/pkg/errors" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) -// client wraps go-sdk. -type client interface { - Create(context.Context, string, string, authorization.RoleAssignmentCreateParameters) (authorization.RoleAssignment, error) -} - // azureClient contains the Azure go-sdk Client. type azureClient struct { roleassignments authorization.RoleAssignmentsClient } -var _ client = (*azureClient)(nil) - // newClient creates a new role assignment client from subscription ID. func newClient(auth azure.Authorizer) *azureClient { c := newRoleAssignmentClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer()) @@ -50,18 +45,45 @@ func newRoleAssignmentClient(subscriptionID string, baseURI string, authorizer a return roleClient } -// Create creates a role assignment. -// Parameters: -// scope - the scope of the role assignment to create. The scope can be any REST resource instance. For -// example, use '/subscriptions/{subscription-id}/' for a subscription, -// '/subscriptions/{subscription-id}/resourceGroups/{resource-group-name}' for a resource group, and -// '/subscriptions/{subscription-id}/resourceGroups/{resource-group-name}/providers/{resource-provider}/{resource-type}/{resource-name}' -// for a resource. -// roleAssignmentName - the name of the role assignment to create. It can be any valid GUID. -// parameters - parameters for the role assignment. -func (ac *azureClient) Create(ctx context.Context, scope string, roleAssignmentName string, parameters authorization.RoleAssignmentCreateParameters) (authorization.RoleAssignment, error) { - ctx, _, done := tele.StartSpanWithLogger(ctx, "roleassignments.AzureClient.Create") +// Get gets the specified role assignment by the role assignment name. +func (ac *azureClient) Get(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, error) { + ctx, span := tele.Tracer().Start(ctx, "roleassignments.AzureClient.Get") + defer span.End() + return ac.roleassignments.Get(ctx, spec.OwnerResourceName(), spec.ResourceName()) +} + +// CreateOrUpdateAsync creates a roleassignment. +// Creating a roleassignment is not a long running operation, so we don't ever return a future. +func (ac *azureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (interface{}, azureautorest.FutureAPI, error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "groups.AzureClient.CreateOrUpdate") + defer done() + createParams, ok := parameters.(authorization.RoleAssignmentCreateParameters) + if !ok { + return nil, nil, errors.Errorf("%T is not a authorization.RoleAssignmentCreateParameters", parameters) + } + result, err := ac.roleassignments.Create(ctx, spec.OwnerResourceName(), spec.ResourceName(), createParams) + return result, nil, err +} + +// IsDone returns true if the long-running operation has completed. +func (ac *azureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (bool, error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "roleassignments.AzureClient.IsDone") defer done() - return ac.roleassignments.Create(ctx, scope, roleAssignmentName, parameters) + isDone, err := future.DoneWithContext(ctx, ac.roleassignments) + if err != nil { + return false, errors.Wrap(err, "failed checking if the operation was complete") + } + return isDone, nil +} + +// Result fetches the result of a long-running operation future. +func (ac *azureClient) Result(ctx context.Context, futureData azureautorest.FutureAPI, futureType string) (interface{}, error) { + // Result is a no-op for role assignment as only Delete operations return a future. + return nil, nil +} + +// DeleteAsync is no-op for role assignments. It gets deleted as part of the VM deletion. +func (ac *azureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) { + return nil, nil } diff --git a/azure/services/roleassignments/mock_roleassignments/client_mock.go b/azure/services/roleassignments/mock_roleassignments/client_mock.go index 5d02825bdce..053f04293c0 100644 --- a/azure/services/roleassignments/mock_roleassignments/client_mock.go +++ b/azure/services/roleassignments/mock_roleassignments/client_mock.go @@ -19,49 +19,3 @@ limitations under the License. // Package mock_roleassignments is a generated GoMock package. package mock_roleassignments - -import ( - context "context" - reflect "reflect" - - authorization "github.com/Azure/azure-sdk-for-go/profiles/2019-03-01/authorization/mgmt/authorization" - gomock "github.com/golang/mock/gomock" -) - -// Mockclient is a mock of client interface. -type Mockclient struct { - ctrl *gomock.Controller - recorder *MockclientMockRecorder -} - -// MockclientMockRecorder is the mock recorder for Mockclient. -type MockclientMockRecorder struct { - mock *Mockclient -} - -// NewMockclient creates a new mock instance. -func NewMockclient(ctrl *gomock.Controller) *Mockclient { - mock := &Mockclient{ctrl: ctrl} - mock.recorder = &MockclientMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *Mockclient) EXPECT() *MockclientMockRecorder { - return m.recorder -} - -// Create mocks base method. -func (m *Mockclient) Create(arg0 context.Context, arg1, arg2 string, arg3 authorization.RoleAssignmentCreateParameters) (authorization.RoleAssignment, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Create", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(authorization.RoleAssignment) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Create indicates an expected call of Create. -func (mr *MockclientMockRecorder) Create(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Create", reflect.TypeOf((*Mockclient)(nil).Create), arg0, arg1, arg2, arg3) -} diff --git a/azure/services/roleassignments/mock_roleassignments/roleassignments_mock.go b/azure/services/roleassignments/mock_roleassignments/roleassignments_mock.go index 5f2b86d03f4..e4fabeab353 100644 --- a/azure/services/roleassignments/mock_roleassignments/roleassignments_mock.go +++ b/azure/services/roleassignments/mock_roleassignments/roleassignments_mock.go @@ -27,6 +27,7 @@ import ( gomock "github.com/golang/mock/gomock" v1beta1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" azure "sigs.k8s.io/cluster-api-provider-azure/azure" + v1beta10 "sigs.k8s.io/cluster-api/api/v1beta1" ) // MockRoleAssignmentScope is a mock of RoleAssignmentScope interface. @@ -52,20 +53,6 @@ func (m *MockRoleAssignmentScope) EXPECT() *MockRoleAssignmentScopeMockRecorder return m.recorder } -// AdditionalTags mocks base method. -func (m *MockRoleAssignmentScope) AdditionalTags() v1beta1.Tags { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AdditionalTags") - ret0, _ := ret[0].(v1beta1.Tags) - return ret0 -} - -// AdditionalTags indicates an expected call of AdditionalTags. -func (mr *MockRoleAssignmentScopeMockRecorder) AdditionalTags() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AdditionalTags", reflect.TypeOf((*MockRoleAssignmentScope)(nil).AdditionalTags)) -} - // Authorizer mocks base method. func (m *MockRoleAssignmentScope) Authorizer() autorest.Authorizer { m.ctrl.T.Helper() @@ -80,20 +67,6 @@ func (mr *MockRoleAssignmentScopeMockRecorder) Authorizer() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authorizer", reflect.TypeOf((*MockRoleAssignmentScope)(nil).Authorizer)) } -// AvailabilitySetEnabled mocks base method. -func (m *MockRoleAssignmentScope) AvailabilitySetEnabled() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AvailabilitySetEnabled") - ret0, _ := ret[0].(bool) - return ret0 -} - -// AvailabilitySetEnabled indicates an expected call of AvailabilitySetEnabled. -func (mr *MockRoleAssignmentScopeMockRecorder) AvailabilitySetEnabled() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AvailabilitySetEnabled", reflect.TypeOf((*MockRoleAssignmentScope)(nil).AvailabilitySetEnabled)) -} - // BaseURI mocks base method. func (m *MockRoleAssignmentScope) BaseURI() string { m.ctrl.T.Helper() @@ -150,46 +123,44 @@ func (mr *MockRoleAssignmentScopeMockRecorder) CloudEnvironment() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudEnvironment", reflect.TypeOf((*MockRoleAssignmentScope)(nil).CloudEnvironment)) } -// CloudProviderConfigOverrides mocks base method. -func (m *MockRoleAssignmentScope) CloudProviderConfigOverrides() *v1beta1.CloudProviderConfigOverrides { +// DeleteLongRunningOperationState mocks base method. +func (m *MockRoleAssignmentScope) DeleteLongRunningOperationState(arg0, arg1 string) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CloudProviderConfigOverrides") - ret0, _ := ret[0].(*v1beta1.CloudProviderConfigOverrides) - return ret0 + m.ctrl.Call(m, "DeleteLongRunningOperationState", arg0, arg1) } -// CloudProviderConfigOverrides indicates an expected call of CloudProviderConfigOverrides. -func (mr *MockRoleAssignmentScopeMockRecorder) CloudProviderConfigOverrides() *gomock.Call { +// DeleteLongRunningOperationState indicates an expected call of DeleteLongRunningOperationState. +func (mr *MockRoleAssignmentScopeMockRecorder) DeleteLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudProviderConfigOverrides", reflect.TypeOf((*MockRoleAssignmentScope)(nil).CloudProviderConfigOverrides)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLongRunningOperationState", reflect.TypeOf((*MockRoleAssignmentScope)(nil).DeleteLongRunningOperationState), arg0, arg1) } -// ClusterName mocks base method. -func (m *MockRoleAssignmentScope) ClusterName() string { +// GetLongRunningOperationState mocks base method. +func (m *MockRoleAssignmentScope) GetLongRunningOperationState(arg0, arg1 string) *v1beta1.Future { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ClusterName") - ret0, _ := ret[0].(string) + ret := m.ctrl.Call(m, "GetLongRunningOperationState", arg0, arg1) + ret0, _ := ret[0].(*v1beta1.Future) return ret0 } -// ClusterName indicates an expected call of ClusterName. -func (mr *MockRoleAssignmentScopeMockRecorder) ClusterName() *gomock.Call { +// GetLongRunningOperationState indicates an expected call of GetLongRunningOperationState. +func (mr *MockRoleAssignmentScopeMockRecorder) GetLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClusterName", reflect.TypeOf((*MockRoleAssignmentScope)(nil).ClusterName)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLongRunningOperationState", reflect.TypeOf((*MockRoleAssignmentScope)(nil).GetLongRunningOperationState), arg0, arg1) } -// FailureDomains mocks base method. -func (m *MockRoleAssignmentScope) FailureDomains() []string { +// HasSystemAssignedIdentity mocks base method. +func (m *MockRoleAssignmentScope) HasSystemAssignedIdentity() bool { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "FailureDomains") - ret0, _ := ret[0].([]string) + ret := m.ctrl.Call(m, "HasSystemAssignedIdentity") + ret0, _ := ret[0].(bool) return ret0 } -// FailureDomains indicates an expected call of FailureDomains. -func (mr *MockRoleAssignmentScopeMockRecorder) FailureDomains() *gomock.Call { +// HasSystemAssignedIdentity indicates an expected call of HasSystemAssignedIdentity. +func (mr *MockRoleAssignmentScopeMockRecorder) HasSystemAssignedIdentity() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FailureDomains", reflect.TypeOf((*MockRoleAssignmentScope)(nil).FailureDomains)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasSystemAssignedIdentity", reflect.TypeOf((*MockRoleAssignmentScope)(nil).HasSystemAssignedIdentity)) } // HashKey mocks base method. @@ -206,18 +177,18 @@ func (mr *MockRoleAssignmentScopeMockRecorder) HashKey() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HashKey", reflect.TypeOf((*MockRoleAssignmentScope)(nil).HashKey)) } -// Location mocks base method. -func (m *MockRoleAssignmentScope) Location() string { +// Name mocks base method. +func (m *MockRoleAssignmentScope) Name() string { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Location") + ret := m.ctrl.Call(m, "Name") ret0, _ := ret[0].(string) return ret0 } -// Location indicates an expected call of Location. -func (mr *MockRoleAssignmentScopeMockRecorder) Location() *gomock.Call { +// Name indicates an expected call of Name. +func (mr *MockRoleAssignmentScopeMockRecorder) Name() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Location", reflect.TypeOf((*MockRoleAssignmentScope)(nil).Location)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockRoleAssignmentScope)(nil).Name)) } // ResourceGroup mocks base method. @@ -234,18 +205,44 @@ func (mr *MockRoleAssignmentScopeMockRecorder) ResourceGroup() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockRoleAssignmentScope)(nil).ResourceGroup)) } +// RoleAssignmentResourceType mocks base method. +func (m *MockRoleAssignmentScope) RoleAssignmentResourceType() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RoleAssignmentResourceType") + ret0, _ := ret[0].(string) + return ret0 +} + +// RoleAssignmentResourceType indicates an expected call of RoleAssignmentResourceType. +func (mr *MockRoleAssignmentScopeMockRecorder) RoleAssignmentResourceType() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RoleAssignmentResourceType", reflect.TypeOf((*MockRoleAssignmentScope)(nil).RoleAssignmentResourceType)) +} + // RoleAssignmentSpecs mocks base method. -func (m *MockRoleAssignmentScope) RoleAssignmentSpecs() []azure.RoleAssignmentSpec { +func (m *MockRoleAssignmentScope) RoleAssignmentSpecs(principalID *string) []azure.ResourceSpecGetter { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RoleAssignmentSpecs") - ret0, _ := ret[0].([]azure.RoleAssignmentSpec) + ret := m.ctrl.Call(m, "RoleAssignmentSpecs", principalID) + ret0, _ := ret[0].([]azure.ResourceSpecGetter) return ret0 } // RoleAssignmentSpecs indicates an expected call of RoleAssignmentSpecs. -func (mr *MockRoleAssignmentScopeMockRecorder) RoleAssignmentSpecs() *gomock.Call { +func (mr *MockRoleAssignmentScopeMockRecorder) RoleAssignmentSpecs(principalID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RoleAssignmentSpecs", reflect.TypeOf((*MockRoleAssignmentScope)(nil).RoleAssignmentSpecs), principalID) +} + +// SetLongRunningOperationState mocks base method. +func (m *MockRoleAssignmentScope) SetLongRunningOperationState(arg0 *v1beta1.Future) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetLongRunningOperationState", arg0) +} + +// SetLongRunningOperationState indicates an expected call of SetLongRunningOperationState. +func (mr *MockRoleAssignmentScopeMockRecorder) SetLongRunningOperationState(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RoleAssignmentSpecs", reflect.TypeOf((*MockRoleAssignmentScope)(nil).RoleAssignmentSpecs)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLongRunningOperationState", reflect.TypeOf((*MockRoleAssignmentScope)(nil).SetLongRunningOperationState), arg0) } // SubscriptionID mocks base method. @@ -275,3 +272,39 @@ func (mr *MockRoleAssignmentScopeMockRecorder) TenantID() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockRoleAssignmentScope)(nil).TenantID)) } + +// UpdateDeleteStatus mocks base method. +func (m *MockRoleAssignmentScope) UpdateDeleteStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdateDeleteStatus", arg0, arg1, arg2) +} + +// UpdateDeleteStatus indicates an expected call of UpdateDeleteStatus. +func (mr *MockRoleAssignmentScopeMockRecorder) UpdateDeleteStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateDeleteStatus", reflect.TypeOf((*MockRoleAssignmentScope)(nil).UpdateDeleteStatus), arg0, arg1, arg2) +} + +// UpdatePatchStatus mocks base method. +func (m *MockRoleAssignmentScope) UpdatePatchStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePatchStatus", arg0, arg1, arg2) +} + +// UpdatePatchStatus indicates an expected call of UpdatePatchStatus. +func (mr *MockRoleAssignmentScopeMockRecorder) UpdatePatchStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePatchStatus", reflect.TypeOf((*MockRoleAssignmentScope)(nil).UpdatePatchStatus), arg0, arg1, arg2) +} + +// UpdatePutStatus mocks base method. +func (m *MockRoleAssignmentScope) UpdatePutStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePutStatus", arg0, arg1, arg2) +} + +// UpdatePutStatus indicates an expected call of UpdatePutStatus. +func (mr *MockRoleAssignmentScopeMockRecorder) UpdatePutStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePutStatus", reflect.TypeOf((*MockRoleAssignmentScope)(nil).UpdatePutStatus), arg0, arg1, arg2) +} diff --git a/azure/services/roleassignments/roleassignments.go b/azure/services/roleassignments/roleassignments.go index 3e52934335e..d4773a8cffa 100644 --- a/azure/services/roleassignments/roleassignments.go +++ b/azure/services/roleassignments/roleassignments.go @@ -18,45 +18,46 @@ package roleassignments import ( "context" - "fmt" - "github.com/Azure/azure-sdk-for-go/profiles/2019-03-01/authorization/mgmt/authorization" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute" - "github.com/Azure/go-autorest/autorest/to" "github.com/pkg/errors" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/services/async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines" + "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) -const ( - azureBuiltInContributorID = "b24988ac-6180-42a0-ab88-20f7382dd24c" - serviceName = "roleassignments" -) +const serviceName = "roleassignments" // RoleAssignmentScope defines the scope interface for a role assignment service. type RoleAssignmentScope interface { - azure.ClusterDescriber - RoleAssignmentSpecs() []azure.RoleAssignmentSpec + azure.AsyncStatusUpdater + azure.Authorizer + RoleAssignmentSpecs(principalID *string) []azure.ResourceSpecGetter + HasSystemAssignedIdentity() bool + RoleAssignmentResourceType() string + Name() string + ResourceGroup() string } // Service provides operations on Azure resources. type Service struct { - Scope RoleAssignmentScope - client - virtualMachinesGetter async.Getter + Scope RoleAssignmentScope + virtualMachinesGetter async.Getter + async.Reconciler virtualMachineScaleSetClient scalesets.Client } // New creates a new service. func New(scope RoleAssignmentScope) *Service { + client := newClient(scope) return &Service{ Scope: scope, - client: newClient(scope), virtualMachinesGetter: virtualmachines.NewClient(scope), virtualMachineScaleSetClient: scalesets.NewClient(scope), + Reconciler: async.New(scope, client, client), } } @@ -67,92 +68,86 @@ func (s *Service) Name() string { // Reconcile creates a role assignment. func (s *Service) Reconcile(ctx context.Context) error { - ctx, _, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.Reconcile") + ctx, log, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.Reconcile") defer done() + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout) + defer cancel() + log.V(2).Info("reconciling role assignment") + + // Return early if the identity is not system assigned as there will be no + // role assignment spec in this case. + if !s.Scope.HasSystemAssignedIdentity() { + log.V(2).Info("no role assignment spec to reconcile") + return nil + } - for _, roleSpec := range s.Scope.RoleAssignmentSpecs() { - switch roleSpec.ResourceType { - case azure.VirtualMachine: - return s.reconcileVM(ctx, roleSpec) - case azure.VirtualMachineScaleSet: - return s.reconcileVMSS(ctx, roleSpec) - default: - return errors.Errorf("unexpected resource type %q. Expected one of [%s, %s]", roleSpec.ResourceType, - azure.VirtualMachine, azure.VirtualMachineScaleSet) + var principalID *string + resourceType := s.Scope.RoleAssignmentResourceType() + switch resourceType { + case azure.VirtualMachine: + ID, err := s.getVMPrincipalID(ctx) + if err != nil { + return errors.Wrap(err, "failed to assign role to system assigned identity") + } + principalID = ID + case azure.VirtualMachineScaleSet: + ID, err := s.getVMSSPrincipalID(ctx) + if err != nil { + return errors.Wrap(err, "failed to assign role to system assigned identity") + } + principalID = ID + default: + return errors.Errorf("unexpected resource type %q. Expected one of [%s, %s]", resourceType, + azure.VirtualMachine, azure.VirtualMachineScaleSet) + } + + for _, roleAssignmentSpec := range s.Scope.RoleAssignmentSpecs(principalID) { + log.V(2).Info("Creating role assignment") + _, err := s.CreateResource(ctx, roleAssignmentSpec, serviceName) + if err != nil { + return errors.Wrapf(err, "cannot assign role to %s system assigned identity", resourceType) } } return nil } -func (s *Service) reconcileVM(ctx context.Context, roleSpec azure.RoleAssignmentSpec) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.reconcileVM") +// getVMPrincipalID returns the VM principal ID. +func (s *Service) getVMPrincipalID(ctx context.Context) (*string, error) { + ctx, log, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.getVMPrincipalID") defer done() - + log.V(2).Info("fetching principal ID for VM") spec := &virtualmachines.VMSpec{ - Name: roleSpec.MachineName, + Name: s.Scope.Name(), ResourceGroup: s.Scope.ResourceGroup(), } resultVMIface, err := s.virtualMachinesGetter.Get(ctx, spec) if err != nil { - return errors.Wrap(err, "cannot get VM to assign role to system assigned identity") + return nil, errors.Wrap(err, "failed to get principal ID for VM") } resultVM, ok := resultVMIface.(compute.VirtualMachine) if !ok { - return errors.Errorf("%T is not a compute.VirtualMachine", resultVMIface) + return nil, errors.Errorf("%T is not a compute.VirtualMachine", resultVMIface) } - - err = s.assignRole(ctx, roleSpec.Name, resultVM.Identity.PrincipalID) - if err != nil { - return errors.Wrap(err, "cannot assign role to VM system assigned identity") - } - - log.V(2).Info("successfully created role assignment for generated Identity for VM", "virtual machine", roleSpec.MachineName) - - return nil + return resultVM.Identity.PrincipalID, nil } -func (s *Service) reconcileVMSS(ctx context.Context, roleSpec azure.RoleAssignmentSpec) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.reconcileVMSS") +// getVMSSPrincipalID returns the VMSS principal ID. +func (s *Service) getVMSSPrincipalID(ctx context.Context) (*string, error) { + ctx, log, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.getVMPrincipalID") defer done() - - resultVMSS, err := s.virtualMachineScaleSetClient.Get(ctx, s.Scope.ResourceGroup(), roleSpec.MachineName) - if err != nil { - return errors.Wrap(err, "cannot get VMSS to assign role to system assigned identity") - } - - err = s.assignRole(ctx, roleSpec.Name, resultVMSS.Identity.PrincipalID) + log.V(2).Info("fetching principal ID for VMSS") + resultVMSS, err := s.virtualMachineScaleSetClient.Get(ctx, s.Scope.ResourceGroup(), s.Scope.Name()) if err != nil { - return errors.Wrap(err, "cannot assign role to VMSS system assigned identity") + return nil, errors.Wrap(err, "failed to get principal ID for VMSS") } - - log.V(2).Info("successfully created role assignment for generated Identity for VMSS", "virtual machine scale set", roleSpec.MachineName) - - return nil -} - -func (s *Service) assignRole(ctx context.Context, roleAssignmentName string, principalID *string) error { - ctx, _, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.assignRole") - defer done() - - scope := fmt.Sprintf("/subscriptions/%s/", s.Scope.SubscriptionID()) - // Azure built-in roles https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles - contributorRoleDefinitionID := fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", s.Scope.SubscriptionID(), azureBuiltInContributorID) - params := authorization.RoleAssignmentCreateParameters{ - Properties: &authorization.RoleAssignmentProperties{ - RoleDefinitionID: to.StringPtr(contributorRoleDefinitionID), - PrincipalID: principalID, - }, - } - _, err := s.client.Create(ctx, scope, roleAssignmentName, params) - return err + return resultVMSS.Identity.PrincipalID, nil } // Delete is a no-op as the role assignments get deleted as part of VM deletion. func (s *Service) Delete(ctx context.Context) error { _, _, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.Delete") defer done() - return nil } diff --git a/azure/services/roleassignments/roleassignments_test.go b/azure/services/roleassignments/roleassignments_test.go index dd5190df09c..8f6468d5b9f 100644 --- a/azure/services/roleassignments/roleassignments_test.go +++ b/azure/services/roleassignments/roleassignments_test.go @@ -18,10 +18,10 @@ package roleassignments import ( "context" + "fmt" "net/http" "testing" - "github.com/Azure/azure-sdk-for-go/profiles/2019-03-01/authorization/mgmt/authorization" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute" "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/to" @@ -40,72 +40,83 @@ var ( Name: "test-vm", ResourceGroup: "my-rg", } + fakePrincipalID = "fake-p-id" + fakeRoleAssignment1 = RoleAssignmentSpec{ + MachineName: "test-vm", + ResourceGroup: "my-rg", + ResourceType: azure.VirtualMachine, + PrincipalID: to.StringPtr("fake-principal-id"), + } + fakeRoleAssignment2 = RoleAssignmentSpec{ + MachineName: "test-vmss", + ResourceGroup: "my-rg", + ResourceType: azure.VirtualMachineScaleSet, + } + + emptyRoleAssignmentSpec = RoleAssignmentSpec{} + fakeRoleAssignmentSpecs = []azure.ResourceSpecGetter{&fakeRoleAssignment1, &fakeRoleAssignment2, &emptyRoleAssignmentSpec} ) func TestReconcileRoleAssignmentsVM(t *testing.T) { testcases := []struct { name string - expect func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_async.MockGetterMockRecorder) + expect func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) expectedError string }{ { name: "create a role assignment", expectedError: "", - expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_async.MockGetterMockRecorder) { + + expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, + m *mock_async.MockGetterMockRecorder, + r *mock_async.MockReconcilerMockRecorder) { s.SubscriptionID().AnyTimes().Return("12345") s.ResourceGroup().Return("my-rg") - s.RoleAssignmentSpecs().Return([]azure.RoleAssignmentSpec{ - { - MachineName: "test-vm", - ResourceType: azure.VirtualMachine, - }, - }) - v.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ + s.Name().Return(fakeRoleAssignment1.MachineName) + s.HasSystemAssignedIdentity().Return(true) + s.RoleAssignmentResourceType().Return("VirtualMachine") + s.RoleAssignmentSpecs(&fakePrincipalID).Return(fakeRoleAssignmentSpecs[:1]) + m.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ Identity: &compute.VirtualMachineIdentity{ - PrincipalID: to.StringPtr("000"), + PrincipalID: &fakePrincipalID, }, }, nil) - m.Create(gomockinternal.AContext(), "/subscriptions/12345/", gomock.AssignableToTypeOf("uuid"), gomock.AssignableToTypeOf(authorization.RoleAssignmentCreateParameters{ - Properties: &authorization.RoleAssignmentProperties{ - RoleDefinitionID: to.StringPtr("/subscriptions/12345/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c"), - PrincipalID: to.StringPtr("000"), - }, - })) + r.CreateResource(gomockinternal.AContext(), &fakeRoleAssignment1, serviceName).Return(&fakeRoleAssignment1, nil) }, }, { name: "error getting VM", - expectedError: "cannot get VM to assign role to system assigned identity: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_async.MockGetterMockRecorder) { + expectedError: "failed to assign role to system assigned identity: failed to get principal ID for VM: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, + m *mock_async.MockGetterMockRecorder, + r *mock_async.MockReconcilerMockRecorder) { s.SubscriptionID().AnyTimes().Return("12345") s.ResourceGroup().Return("my-rg") - s.RoleAssignmentSpecs().Return([]azure.RoleAssignmentSpec{ - { - MachineName: "test-vm", - ResourceType: azure.VirtualMachine, - }, - }) - v.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + s.Name().Return(fakeRoleAssignment1.MachineName) + s.HasSystemAssignedIdentity().Return(true) + s.RoleAssignmentResourceType().Return("VirtualMachine") + m.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) }, }, { name: "return error when creating a role assignment", - expectedError: "cannot assign role to VM system assigned identity: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_async.MockGetterMockRecorder) { + expectedError: "cannot assign role to VirtualMachine system assigned identity: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, + m *mock_async.MockGetterMockRecorder, + r *mock_async.MockReconcilerMockRecorder) { s.SubscriptionID().AnyTimes().Return("12345") s.ResourceGroup().Return("my-rg") - s.RoleAssignmentSpecs().Return([]azure.RoleAssignmentSpec{ - { - MachineName: "test-vm", - ResourceType: azure.VirtualMachine, - }, - }) - v.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ + s.Name().Return(fakeRoleAssignment1.MachineName) + s.RoleAssignmentResourceType().Return("VirtualMachine") + s.HasSystemAssignedIdentity().Return(true) + s.RoleAssignmentSpecs(&fakePrincipalID).Return(fakeRoleAssignmentSpecs[0:1]) + m.Get(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ Identity: &compute.VirtualMachineIdentity{ - PrincipalID: to.StringPtr("000"), + PrincipalID: &fakePrincipalID, }, }, nil) - m.Create(gomockinternal.AContext(), "/subscriptions/12345/", gomock.AssignableToTypeOf("uuid"), gomock.AssignableToTypeOf(authorization.RoleAssignmentCreateParameters{})).Return(authorization.RoleAssignment{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + r.CreateResource(gomockinternal.AContext(), &fakeRoleAssignment1, serviceName).Return(&RoleAssignmentSpec{}, + autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) }, }, } @@ -118,15 +129,15 @@ func TestReconcileRoleAssignmentsVM(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_roleassignments.NewMockRoleAssignmentScope(mockCtrl) - clientMock := mock_roleassignments.NewMockclient(mockCtrl) vmGetterMock := mock_async.NewMockGetter(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), vmGetterMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), vmGetterMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ Scope: scopeMock, - client: clientMock, virtualMachinesGetter: vmGetterMock, + Reconciler: asyncMock, } err := s.Reconcile(context.TODO()) @@ -139,70 +150,65 @@ func TestReconcileRoleAssignmentsVM(t *testing.T) { }) } } + func TestReconcileRoleAssignmentsVMSS(t *testing.T) { testcases := []struct { - name string - expect func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_scalesets.MockClientMockRecorder) + name string + expect func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, + mvmss *mock_scalesets.MockClientMockRecorder) expectedError string }{ { name: "create a role assignment", expectedError: "", - expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_scalesets.MockClientMockRecorder) { - s.SubscriptionID().AnyTimes().Return("12345") + expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, + r *mock_async.MockReconcilerMockRecorder, + mvmss *mock_scalesets.MockClientMockRecorder) { + s.HasSystemAssignedIdentity().Return(true) + s.RoleAssignmentSpecs(&fakePrincipalID).Return(fakeRoleAssignmentSpecs[1:2]) + s.RoleAssignmentResourceType().Return(azure.VirtualMachineScaleSet) s.ResourceGroup().Return("my-rg") - s.RoleAssignmentSpecs().Return([]azure.RoleAssignmentSpec{ - { - MachineName: "test-vmss", - ResourceType: azure.VirtualMachineScaleSet, - }, - }) - v.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{ + s.Name().Return("test-vmss") + mvmss.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{ Identity: &compute.VirtualMachineScaleSetIdentity{ - PrincipalID: to.StringPtr("000"), + PrincipalID: &fakePrincipalID, }, }, nil) - m.Create(gomockinternal.AContext(), "/subscriptions/12345/", gomock.AssignableToTypeOf("uuid"), gomock.AssignableToTypeOf(authorization.RoleAssignmentCreateParameters{ - Properties: &authorization.RoleAssignmentProperties{ - RoleDefinitionID: to.StringPtr("/subscriptions/12345/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c"), - PrincipalID: to.StringPtr("000"), - }, - })) + r.CreateResource(gomockinternal.AContext(), &fakeRoleAssignment2, serviceName).Return(&fakeRoleAssignment2, nil) }, }, { name: "error getting VMSS", - expectedError: "cannot get VMSS to assign role to system assigned identity: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_scalesets.MockClientMockRecorder) { - s.SubscriptionID().AnyTimes().Return("12345") + expectedError: "failed to assign role to system assigned identity: failed to get principal ID for VMSS: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, + r *mock_async.MockReconcilerMockRecorder, + mvmss *mock_scalesets.MockClientMockRecorder) { + s.RoleAssignmentResourceType().Return(azure.VirtualMachineScaleSet) s.ResourceGroup().Return("my-rg") - s.RoleAssignmentSpecs().Return([]azure.RoleAssignmentSpec{ - { - MachineName: "test-vmss", - ResourceType: azure.VirtualMachineScaleSet, - }, - }) - v.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + s.Name().Return("test-vmss") + s.HasSystemAssignedIdentity().Return(true) + mvmss.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{}, + autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) }, }, { name: "return error when creating a role assignment", - expectedError: "cannot assign role to VMSS system assigned identity: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, m *mock_roleassignments.MockclientMockRecorder, v *mock_scalesets.MockClientMockRecorder) { - s.SubscriptionID().AnyTimes().Return("12345") + expectedError: fmt.Sprintf("cannot assign role to %s system assigned identity: #: Internal Server Error: StatusCode=500", azure.VirtualMachineScaleSet), + expect: func(s *mock_roleassignments.MockRoleAssignmentScopeMockRecorder, + r *mock_async.MockReconcilerMockRecorder, + mvmss *mock_scalesets.MockClientMockRecorder) { + s.HasSystemAssignedIdentity().Return(true) + s.RoleAssignmentSpecs(&fakePrincipalID).Return(fakeRoleAssignmentSpecs[1:2]) + s.RoleAssignmentResourceType().Return(azure.VirtualMachineScaleSet) s.ResourceGroup().Return("my-rg") - s.RoleAssignmentSpecs().Return([]azure.RoleAssignmentSpec{ - { - MachineName: "test-vmss", - ResourceType: azure.VirtualMachineScaleSet, - }, - }) - v.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{ + s.Name().Return("test-vmss") + mvmss.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{ Identity: &compute.VirtualMachineScaleSetIdentity{ - PrincipalID: to.StringPtr("000"), + PrincipalID: &fakePrincipalID, }, }, nil) - m.Create(gomockinternal.AContext(), "/subscriptions/12345/", gomock.AssignableToTypeOf("uuid"), gomock.AssignableToTypeOf(authorization.RoleAssignmentCreateParameters{})).Return(authorization.RoleAssignment{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + r.CreateResource(gomockinternal.AContext(), &fakeRoleAssignment2, serviceName).Return(&RoleAssignmentSpec{}, + autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) }, }, } @@ -215,15 +221,15 @@ func TestReconcileRoleAssignmentsVMSS(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_roleassignments.NewMockRoleAssignmentScope(mockCtrl) - clientMock := mock_roleassignments.NewMockclient(mockCtrl) - vmssMock := mock_scalesets.NewMockClient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) + vmMock := mock_scalesets.NewMockClient(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), vmssMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), asyncMock.EXPECT(), vmMock.EXPECT()) s := &Service{ Scope: scopeMock, - client: clientMock, - virtualMachineScaleSetClient: vmssMock, + Reconciler: asyncMock, + virtualMachineScaleSetClient: vmMock, } err := s.Reconcile(context.TODO()) diff --git a/azure/services/roleassignments/spec.go b/azure/services/roleassignments/spec.go new file mode 100644 index 00000000000..19904edcc9c --- /dev/null +++ b/azure/services/roleassignments/spec.go @@ -0,0 +1,67 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package roleassignments + +import ( + "github.com/Azure/azure-sdk-for-go/profiles/2019-03-01/authorization/mgmt/authorization" + "github.com/Azure/go-autorest/autorest/to" + "github.com/pkg/errors" +) + +// RoleAssignmentSpec defines the specification for a role assignment. +type RoleAssignmentSpec struct { + Name string + MachineName string + ResourceGroup string + ResourceType string + PrincipalID *string + RoleDefinitionID string + Scope string +} + +// ResourceName returns the name of the role assignment. +func (s *RoleAssignmentSpec) ResourceName() string { + return s.Name +} + +// ResourceGroupName returns the name of the resource group. +func (s *RoleAssignmentSpec) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName returns the scope for role assignment. +// TODO: Consider renaming the function for better readability (@sonasingh46). +func (s *RoleAssignmentSpec) OwnerResourceName() string { + return s.Scope +} + +// Parameters returns the parameters for the RoleAssignmentSpec. +func (s *RoleAssignmentSpec) Parameters(existing interface{}) (interface{}, error) { + if existing != nil { + if _, ok := existing.(authorization.RoleAssignment); !ok { + return nil, errors.Errorf("%T is not a authorization.RoleAssignment", existing) + } + // RoleAssignmentSpec already exists + return nil, nil + } + return authorization.RoleAssignmentCreateParameters{ + Properties: &authorization.RoleAssignmentProperties{ + PrincipalID: s.PrincipalID, + RoleDefinitionID: to.StringPtr(s.RoleDefinitionID), + }, + }, nil +} diff --git a/azure/services/scalesets/client.go b/azure/services/scalesets/client.go index 22f907d8da5..352ac897fe2 100644 --- a/azure/services/scalesets/client.go +++ b/azure/services/scalesets/client.go @@ -31,7 +31,6 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "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/scope" "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) @@ -162,7 +161,7 @@ func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, resourceGroupNam if err != nil { // if an error occurs, return the future. // this means the long-running operation didn't finish in the specified timeout. - return converters.SDKToFuture(&future, infrav1.PutFuture, scope.ScalesetsServiceName, vmssName, resourceGroupName) + return converters.SDKToFuture(&future, infrav1.PutFuture, serviceName, vmssName, resourceGroupName) } // todo: this returns the result VMSS, we should use it @@ -195,7 +194,7 @@ func (ac *AzureClient) UpdateAsync(ctx context.Context, resourceGroupName, vmssN if err != nil { // if an error occurs, return the future. // this means the long-running operation didn't finish in the specified timeout. - return converters.SDKToFuture(&future, infrav1.PatchFuture, scope.ScalesetsServiceName, vmssName, resourceGroupName) + return converters.SDKToFuture(&future, infrav1.PatchFuture, serviceName, vmssName, resourceGroupName) } // todo: this returns the result VMSS, we should use it _, err = future.Result(ac.scalesets) @@ -306,7 +305,7 @@ func (ac *AzureClient) DeleteAsync(ctx context.Context, resourceGroupName, vmssN if err != nil { // if an error occurs, return the future. // this means the long-running operation didn't finish in the specified timeout. - return converters.SDKToFuture(&future, infrav1.DeleteFuture, scope.ScalesetsServiceName, vmssName, resourceGroupName) + return converters.SDKToFuture(&future, infrav1.DeleteFuture, serviceName, vmssName, resourceGroupName) } _, err = future.Result(ac.scalesets) diff --git a/azure/services/scalesets/scalesets.go b/azure/services/scalesets/scalesets.go index fc7ee6a2de0..55df787a2ec 100644 --- a/azure/services/scalesets/scalesets.go +++ b/azure/services/scalesets/scalesets.go @@ -28,7 +28,6 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "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/scope" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" "sigs.k8s.io/cluster-api-provider-azure/util/generators" "sigs.k8s.io/cluster-api-provider-azure/util/slice" @@ -91,7 +90,7 @@ func (s *Service) Reconcile(ctx context.Context) (retErr error) { // check if there is an ongoing long running operation var ( - future = s.Scope.GetLongRunningOperationState(s.Scope.ScaleSetSpec().Name, scope.ScalesetsServiceName) + future = s.Scope.GetLongRunningOperationState(s.Scope.ScaleSetSpec().Name, serviceName) fetchedVMSS *azure.VMSS ) @@ -146,7 +145,7 @@ func (s *Service) Reconcile(ctx context.Context) (retErr error) { } // if we get to here, we have completed any long running VMSS operations (creates / updates) - s.Scope.DeleteLongRunningOperationState(s.Scope.ScaleSetSpec().Name, scope.ScalesetsServiceName) + s.Scope.DeleteLongRunningOperationState(s.Scope.ScaleSetSpec().Name, serviceName) return nil } @@ -173,7 +172,7 @@ func (s *Service) Delete(ctx context.Context) error { }() // check if there is an ongoing long running operation - future := s.Scope.GetLongRunningOperationState(vmssSpec.Name, scope.ScalesetsServiceName) + future := s.Scope.GetLongRunningOperationState(vmssSpec.Name, serviceName) if future != nil { // if the operation is not complete this will return an error _, err := s.GetResultIfDone(ctx, future) @@ -182,7 +181,7 @@ func (s *Service) Delete(ctx context.Context) error { } // ScaleSet has been deleted - s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, scope.ScalesetsServiceName) + s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, serviceName) return nil } @@ -206,7 +205,7 @@ func (s *Service) Delete(ctx context.Context) error { } // future is either nil, or the result of the future is complete - s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, scope.ScalesetsServiceName) + s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, serviceName) return nil } diff --git a/azure/services/scalesets/scalesets_test.go b/azure/services/scalesets/scalesets_test.go index 2bb1e5de371..db14ccc8663 100644 --- a/azure/services/scalesets/scalesets_test.go +++ b/azure/services/scalesets/scalesets_test.go @@ -27,20 +27,14 @@ import ( "github.com/golang/mock/gomock" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/resource" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/pointer" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" - "sigs.k8s.io/cluster-api-provider-azure/azure/scope" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets/mock_scalesets" - infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - clusterv1exp "sigs.k8s.io/cluster-api/exp/api/v1beta1" - "sigs.k8s.io/controller-runtime/pkg/client/fake" ) const ( @@ -53,49 +47,6 @@ func init() { _ = clusterv1.AddToScheme(scheme.Scheme) } -func TestNewService(t *testing.T) { - g := NewGomegaWithT(t) - scheme := runtime.NewScheme() - _ = clusterv1.AddToScheme(scheme) - _ = infrav1.AddToScheme(scheme) - _ = infrav1exp.AddToScheme(scheme) - - cluster := &clusterv1.Cluster{ - ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"}, - } - client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster).Build() - s, err := scope.NewClusterScope(context.Background(), scope.ClusterScopeParams{ - AzureClients: scope.AzureClients{ - Authorizer: autorest.NullAuthorizer{}, - }, - Client: client, - Cluster: cluster, - AzureCluster: &infrav1.AzureCluster{ - Spec: infrav1.AzureClusterSpec{ - AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ - Location: "test-location", - SubscriptionID: "123", - }, - ResourceGroup: "my-rg", - NetworkSpec: infrav1.NetworkSpec{ - Vnet: infrav1.VnetSpec{Name: "my-vnet", ResourceGroup: "my-rg"}, - }, - }, - }, - }) - g.Expect(err).NotTo(HaveOccurred()) - - mps, err := scope.NewMachinePoolScope(scope.MachinePoolScopeParams{ - Client: client, - MachinePool: new(clusterv1exp.MachinePool), - AzureMachinePool: new(infrav1exp.AzureMachinePool), - ClusterScope: s, - }) - g.Expect(err).NotTo(HaveOccurred()) - actual := New(mps, resourceskus.NewStaticCache(nil, "")) - g.Expect(actual).ToNot(BeNil()) -} - func TestGetExistingVMSS(t *testing.T) { testcases := []struct { name string @@ -267,8 +218,9 @@ func TestReconcileVMSS(t *testing.T) { s.ScaleSetSpec().Return(defaultSpec).AnyTimes() createdVMSS := newDefaultVMSS("VM_SIZE") instances := newDefaultInstances() + setupDefaultVMSSInProgressOperationDoneExpectations(s, m, createdVMSS, instances) - s.DeleteLongRunningOperationState(defaultSpec.Name, scope.ScalesetsServiceName) + s.DeleteLongRunningOperationState(defaultSpec.Name, serviceName) }, }, { @@ -279,8 +231,9 @@ func TestReconcileVMSS(t *testing.T) { s.ScaleSetSpec().Return(defaultSpec).AnyTimes() createdVMSS := newDefaultWindowsVMSS() instances := newDefaultInstances() + setupDefaultVMSSInProgressOperationDoneExpectations(s, m, createdVMSS, instances) - s.DeleteLongRunningOperationState(defaultSpec.Name, scope.ScalesetsServiceName) + s.DeleteLongRunningOperationState(defaultSpec.Name, serviceName) }, }, { @@ -618,11 +571,11 @@ func TestDeleteVMSS(t *testing.T) { }).AnyTimes() s.ResourceGroup().AnyTimes().Return("my-existing-rg") future := &infrav1.Future{} - s.GetLongRunningOperationState("my-existing-vmss", scope.ScalesetsServiceName).Return(future) + s.GetLongRunningOperationState("my-existing-vmss", serviceName).Return(future) m.GetResultIfDone(gomockinternal.AContext(), future).Return(compute.VirtualMachineScaleSet{}, nil) m.Get(gomockinternal.AContext(), "my-existing-rg", "my-existing-vmss"). Return(compute.VirtualMachineScaleSet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) - s.DeleteLongRunningOperationState("my-existing-vmss", scope.ScalesetsServiceName) + s.DeleteLongRunningOperationState("my-existing-vmss", serviceName) }, }, { @@ -635,7 +588,7 @@ func TestDeleteVMSS(t *testing.T) { Capacity: 3, }).AnyTimes() s.ResourceGroup().AnyTimes().Return(resourceGroup) - s.GetLongRunningOperationState(name, scope.ScalesetsServiceName).Return(nil) + s.GetLongRunningOperationState(name, serviceName).Return(nil) m.DeleteAsync(gomockinternal.AContext(), resourceGroup, name). Return(nil, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) m.Get(gomockinternal.AContext(), resourceGroup, name). @@ -652,7 +605,7 @@ func TestDeleteVMSS(t *testing.T) { Capacity: 3, }).AnyTimes() s.ResourceGroup().AnyTimes().Return(resourceGroup) - s.GetLongRunningOperationState(name, scope.ScalesetsServiceName).Return(nil) + s.GetLongRunningOperationState(name, serviceName).Return(nil) m.DeleteAsync(gomockinternal.AContext(), resourceGroup, name). Return(nil, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) m.Get(gomockinternal.AContext(), resourceGroup, name). @@ -1206,7 +1159,7 @@ func setupDefaultVMSSInProgressOperationDoneExpectations(s *mock_scalesets.MockS Name: defaultVMSSName, Data: "", } - s.GetLongRunningOperationState(defaultVMSSName, scope.ScalesetsServiceName).Return(future) + s.GetLongRunningOperationState(defaultVMSSName, serviceName).Return(future) m.GetResultIfDone(gomockinternal.AContext(), future).Return(createdVMSS, nil).AnyTimes() m.ListInstances(gomockinternal.AContext(), defaultResourceGroup, defaultVMSSName).Return(instances, nil).AnyTimes() s.MaxSurge().Return(1, nil) @@ -1216,7 +1169,7 @@ func setupDefaultVMSSInProgressOperationDoneExpectations(s *mock_scalesets.MockS func setupDefaultVMSSStartCreatingExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorder, m *mock_scalesets.MockClientMockRecorder) { setupDefaultVMSSExpectations(s) - s.GetLongRunningOperationState(defaultVMSSName, scope.ScalesetsServiceName).Return(nil) + s.GetLongRunningOperationState(defaultVMSSName, serviceName).Return(nil) m.Get(gomockinternal.AContext(), defaultResourceGroup, defaultVMSSName). Return(compute.VirtualMachineScaleSet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found")) } @@ -1281,7 +1234,7 @@ func setupVMSSExpectationsWithoutVMImage(s *mock_scalesets.MockScaleSetScopeMock func setupDefaultVMSSUpdateExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorder) { setupUpdateVMSSExpectations(s) s.SetProviderID(azure.ProviderIDPrefix + "vmss-id") - s.GetLongRunningOperationState(defaultVMSSName, scope.ScalesetsServiceName).Return(nil) + s.GetLongRunningOperationState(defaultVMSSName, serviceName).Return(nil) s.MaxSurge().Return(1, nil) s.SetVMSSState(gomock.Any()) }