From 1a26e6622976af497eb68bd50c606eba1d0e2066 Mon Sep 17 00:00:00 2001 From: willie-yao Date: Wed, 21 Dec 2022 20:36:53 +0000 Subject: [PATCH] Allow for configurable scope in system assigned identities --- api/v1alpha3/azuremachine_conversion.go | 4 + .../azuremachinetemplate_conversion.go | 4 + api/v1alpha3/zz_generated.conversion.go | 1 + api/v1alpha4/azuremachine_conversion.go | 4 + .../azuremachinetemplate_conversion.go | 4 + api/v1alpha4/zz_generated.conversion.go | 1 + api/v1beta1/azuremachine_default.go | 94 +++++++++-- api/v1beta1/azuremachine_default_test.go | 52 ++++-- api/v1beta1/azuremachine_types.go | 25 ++- api/v1beta1/azuremachine_validation.go | 28 +++- api/v1beta1/azuremachine_validation_test.go | 81 +++++++++ api/v1beta1/azuremachine_webhook.go | 38 +++-- api/v1beta1/azuremachine_webhook_test.go | 86 ++++++++-- api/v1beta1/azuremachinetemplate_webhook.go | 11 +- api/v1beta1/zz_generated.deepcopy.go | 20 +++ azure/defaults.go | 13 -- azure/scope/machine.go | 30 +++- azure/scope/machine_test.go | 59 ++++++- azure/scope/machinepool.go | 36 +++- azure/scope/machinepool_test.go | 118 ++++++++++++++ ...re.cluster.x-k8s.io_azuremachinepools.yaml | 25 ++- ...ucture.cluster.x-k8s.io_azuremachines.yaml | 25 ++- ...luster.x-k8s.io_azuremachinetemplates.yaml | 28 +++- docs/book/src/topics/vm-identity.md | 18 ++ .../v1alpha3/azuremachinepool_conversion.go | 4 + exp/api/v1alpha3/zz_generated.conversion.go | 1 + .../v1alpha4/azuremachinepool_conversion.go | 4 + exp/api/v1alpha4/zz_generated.conversion.go | 1 + exp/api/v1beta1/azuremachinepool_default.go | 51 +++++- .../v1beta1/azuremachinepool_default_test.go | 53 ++++-- exp/api/v1beta1/azuremachinepool_types.go | 7 +- exp/api/v1beta1/azuremachinepool_webhook.go | 44 ++++- .../v1beta1/azuremachinepool_webhook_test.go | 154 ++++++++++++++---- exp/api/v1beta1/zz_generated.deepcopy.go | 5 + main.go | 11 +- 35 files changed, 980 insertions(+), 160 deletions(-) diff --git a/api/v1alpha3/azuremachine_conversion.go b/api/v1alpha3/azuremachine_conversion.go index 601290a64fd..8920037cb71 100644 --- a/api/v1alpha3/azuremachine_conversion.go +++ b/api/v1alpha3/azuremachine_conversion.go @@ -78,6 +78,10 @@ func (src *AzureMachine) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.NetworkInterfaces = restored.Spec.NetworkInterfaces } + if restored.Spec.SystemAssignedIdentityRole != nil { + dst.Spec.SystemAssignedIdentityRole = restored.Spec.SystemAssignedIdentityRole + } + //nolint:staticcheck // SubnetName is now deprecated, but the v1beta1 defaulting webhook will migrate it to the networkInterfaces field dst.Spec.SubnetName = restored.Spec.SubnetName diff --git a/api/v1alpha3/azuremachinetemplate_conversion.go b/api/v1alpha3/azuremachinetemplate_conversion.go index 32c9a8d9f8b..a182eb2e2c7 100644 --- a/api/v1alpha3/azuremachinetemplate_conversion.go +++ b/api/v1alpha3/azuremachinetemplate_conversion.go @@ -82,6 +82,10 @@ func (src *AzureMachineTemplate) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.Template.Spec.NetworkInterfaces = restored.Spec.Template.Spec.NetworkInterfaces } + if restored.Spec.Template.Spec.SystemAssignedIdentityRole != nil { + dst.Spec.Template.Spec.SystemAssignedIdentityRole = restored.Spec.Template.Spec.SystemAssignedIdentityRole + } + return nil } diff --git a/api/v1alpha3/zz_generated.conversion.go b/api/v1alpha3/zz_generated.conversion.go index 76b8ac86ac1..2342b14eb06 100644 --- a/api/v1alpha3/zz_generated.conversion.go +++ b/api/v1alpha3/zz_generated.conversion.go @@ -1062,6 +1062,7 @@ func autoConvert_v1beta1_AzureMachineSpec_To_v1alpha3_AzureMachineSpec(in *v1bet } out.Identity = VMIdentity(in.Identity) out.UserAssignedIdentities = *(*[]UserAssignedIdentity)(unsafe.Pointer(&in.UserAssignedIdentities)) + // WARNING: in.SystemAssignedIdentityRole requires manual conversion: does not exist in peer-type out.RoleAssignmentName = in.RoleAssignmentName if err := Convert_v1beta1_OSDisk_To_v1alpha3_OSDisk(&in.OSDisk, &out.OSDisk, s); err != nil { return err diff --git a/api/v1alpha4/azuremachine_conversion.go b/api/v1alpha4/azuremachine_conversion.go index 0d5aa517fb7..d292b3e1c3f 100644 --- a/api/v1alpha4/azuremachine_conversion.go +++ b/api/v1alpha4/azuremachine_conversion.go @@ -64,6 +64,10 @@ func (src *AzureMachine) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.NetworkInterfaces = restored.Spec.NetworkInterfaces } + if restored.Spec.SystemAssignedIdentityRole != nil { + dst.Spec.SystemAssignedIdentityRole = restored.Spec.SystemAssignedIdentityRole + } + return nil } diff --git a/api/v1alpha4/azuremachinetemplate_conversion.go b/api/v1alpha4/azuremachinetemplate_conversion.go index a6ecc51dc24..204f359074e 100644 --- a/api/v1alpha4/azuremachinetemplate_conversion.go +++ b/api/v1alpha4/azuremachinetemplate_conversion.go @@ -66,6 +66,10 @@ func (src *AzureMachineTemplate) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.Template.Spec.NetworkInterfaces = restored.Spec.Template.Spec.NetworkInterfaces } + if restored.Spec.Template.Spec.SystemAssignedIdentityRole != nil { + dst.Spec.Template.Spec.SystemAssignedIdentityRole = restored.Spec.Template.Spec.SystemAssignedIdentityRole + } + return nil } diff --git a/api/v1alpha4/zz_generated.conversion.go b/api/v1alpha4/zz_generated.conversion.go index aa031337403..769a103c415 100644 --- a/api/v1alpha4/zz_generated.conversion.go +++ b/api/v1alpha4/zz_generated.conversion.go @@ -1295,6 +1295,7 @@ func autoConvert_v1beta1_AzureMachineSpec_To_v1alpha4_AzureMachineSpec(in *v1bet } out.Identity = VMIdentity(in.Identity) out.UserAssignedIdentities = *(*[]UserAssignedIdentity)(unsafe.Pointer(&in.UserAssignedIdentities)) + // WARNING: in.SystemAssignedIdentityRole requires manual conversion: does not exist in peer-type out.RoleAssignmentName = in.RoleAssignmentName if err := Convert_v1beta1_OSDisk_To_v1alpha4_OSDisk(&in.OSDisk, &out.OSDisk, s); err != nil { return err diff --git a/api/v1beta1/azuremachine_default.go b/api/v1beta1/azuremachine_default.go index 6a838d3972a..69ab7694571 100644 --- a/api/v1beta1/azuremachine_default.go +++ b/api/v1beta1/azuremachine_default.go @@ -17,15 +17,24 @@ limitations under the License. package v1beta1 import ( + "context" "encoding/base64" + "fmt" + "time" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute" + "github.com/pkg/errors" "golang.org/x/crypto/ssh" "k8s.io/apimachinery/pkg/util/uuid" utilSSH "sigs.k8s.io/cluster-api-provider-azure/util/ssh" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" ) +// ContributorRoleID is the ID of the built-in "Contributor" role. +const ContributorRoleID = "b24988ac-6180-42a0-ab88-20f7382dd24c" + // SetDefaultSSHPublicKey sets the default SSHPublicKey for an AzureMachine. func (s *AzureMachineSpec) SetDefaultSSHPublicKey() error { if sshKeyData := s.SSHPublicKey; sshKeyData == "" { @@ -79,10 +88,32 @@ func (s *AzureMachineSpec) SetDataDisksDefaults() { } // SetIdentityDefaults sets the defaults for VM Identity. -func (s *AzureMachineSpec) SetIdentityDefaults() { +func (s *AzureMachineSpec) SetIdentityDefaults(subscriptionID string) { + // Ensure the deprecated fields and new fields are not populated simultaneously + if s.RoleAssignmentName != "" && s.SystemAssignedIdentityRole != nil && s.SystemAssignedIdentityRole.Name != "" { + // Both the deprecated and the new fields are both set, return without changes + // and reject the request in the validating webhook which runs later. + return + } if s.Identity == VMIdentitySystemAssigned { - if s.RoleAssignmentName == "" { - s.RoleAssignmentName = string(uuid.NewUUID()) + if s.SystemAssignedIdentityRole == nil { + s.SystemAssignedIdentityRole = &SystemAssignedIdentityRole{} + } + if s.RoleAssignmentName != "" { + // Move the existing value from the deprecated RoleAssignmentName field. + s.SystemAssignedIdentityRole.Name = s.RoleAssignmentName + s.RoleAssignmentName = "" + } else if s.SystemAssignedIdentityRole.Name == "" { + // Default role name to the role assignment name. + s.SystemAssignedIdentityRole.Name = string(uuid.NewUUID()) + } + if s.SystemAssignedIdentityRole.Scope == "" && subscriptionID != "" { + // Default scope to the subscription. + s.SystemAssignedIdentityRole.Scope = fmt.Sprintf("/subscriptions/%s/", subscriptionID) + } + if s.SystemAssignedIdentityRole.DefinitionID == "" && subscriptionID != "" { + // Default role definition ID to Contributor role. + s.SystemAssignedIdentityRole.DefinitionID = fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", subscriptionID, ContributorRoleID) } } } @@ -143,15 +174,52 @@ func (s *AzureMachineSpec) SetNetworkInterfacesDefaults() { } } +// GetSubscriptionID returns the subscription ID for the given cluster name and namespace. +func GetSubscriptionID(cli client.Client, clusterName string, namespace string, maxAttempts int) (string, error) { + ctx := context.Background() + + ownerCluster := &AzureCluster{} + key := client.ObjectKey{ + Namespace: namespace, + Name: clusterName, + } + + for i := 1; ; i++ { + err := cli.Get(ctx, key, ownerCluster) + if err != nil { + if i > maxAttempts { + return "", errors.Wrapf(err, "failed to find owner cluster for %s/%s", namespace, clusterName) + } + time.Sleep(1 * time.Second) + continue + } + break + } + + return ownerCluster.Spec.SubscriptionID, nil +} + // SetDefaults sets to the defaults for the AzureMachineSpec. -func (s *AzureMachineSpec) SetDefaults() { - if err := s.SetDefaultSSHPublicKey(); err != nil { - ctrl.Log.WithName("SetDefault").Error(err, "SetDefaultSshPublicKey failed") - } - s.SetDefaultCachingType() - s.SetDataDisksDefaults() - s.SetIdentityDefaults() - s.SetSpotEvictionPolicyDefaults() - s.SetDiagnosticsDefaults() - s.SetNetworkInterfacesDefaults() +func (m *AzureMachine) SetDefaults(client client.Client) { + if err := m.Spec.SetDefaultSSHPublicKey(); err != nil { + ctrl.Log.WithName("SetDefault").Error(err, "failed to set default SSH public key") + } + + // Fetch the Cluster. + clusterName, ok := m.Labels[clusterv1.ClusterLabelName] + if !ok { + ctrl.Log.WithName("SetDefault").Error(errors.Errorf("failed to fetch owner ClusterName for AzureMachine %s/%s", m.Namespace, m.Name), "failed to fetch ClusterName") + } + + subscriptionID, err := GetSubscriptionID(client, clusterName, m.Namespace, 5) + if err != nil { + ctrl.Log.WithName("SetDefault").Error(err, "failed to fetch subscription ID for AzureMachine %s/%s", m.Namespace, m.Name) + } + + m.Spec.SetDefaultCachingType() + m.Spec.SetDataDisksDefaults() + m.Spec.SetIdentityDefaults(subscriptionID) + m.Spec.SetSpotEvictionPolicyDefaults() + m.Spec.SetDiagnosticsDefaults() + m.Spec.SetNetworkInterfacesDefaults() } diff --git a/api/v1beta1/azuremachine_default_test.go b/api/v1beta1/azuremachine_default_test.go index b0a860249a0..137a2669bce 100644 --- a/api/v1beta1/azuremachine_default_test.go +++ b/api/v1beta1/azuremachine_default_test.go @@ -18,6 +18,7 @@ package v1beta1 import ( "encoding/json" + "fmt" "reflect" "testing" @@ -53,29 +54,50 @@ func TestAzureMachineSpec_SetIdentityDefaults(t *testing.T) { machine *AzureMachine } + fakeSubscriptionID := uuid.New().String() + fakeClusterName := "testcluster" + fakeRoleDefinitionID := "testroledefinitionid" + fakeScope := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", fakeSubscriptionID, fakeClusterName) existingRoleAssignmentName := "42862306-e485-4319-9bf0-35dbc6f6fe9c" roleAssignmentExistTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{ - Identity: VMIdentitySystemAssigned, - RoleAssignmentName: existingRoleAssignmentName, - }}} - roleAssignmentEmptyTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{ - Identity: VMIdentitySystemAssigned, - RoleAssignmentName: "", + Identity: VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Name: existingRoleAssignmentName, + }, }}} notSystemAssignedTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{ - Identity: VMIdentityUserAssigned, + Identity: VMIdentityUserAssigned, + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{}, + }}} + systemAssignedIdentityRoleExistTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{ + Identity: VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Scope: fakeScope, + DefinitionID: fakeRoleDefinitionID, + }, }}} - roleAssignmentExistTest.machine.Spec.SetIdentityDefaults() - g.Expect(roleAssignmentExistTest.machine.Spec.RoleAssignmentName).To(Equal(existingRoleAssignmentName)) + emptyTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{ + Identity: VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{}, + }}} - roleAssignmentEmptyTest.machine.Spec.SetIdentityDefaults() - g.Expect(roleAssignmentEmptyTest.machine.Spec.RoleAssignmentName).To(Not(BeEmpty())) - _, err := uuid.Parse(roleAssignmentEmptyTest.machine.Spec.RoleAssignmentName) - g.Expect(err).To(Not(HaveOccurred())) + roleAssignmentExistTest.machine.Spec.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(roleAssignmentExistTest.machine.Spec.SystemAssignedIdentityRole.Name).To(Equal(existingRoleAssignmentName)) + + notSystemAssignedTest.machine.Spec.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(notSystemAssignedTest.machine.Spec.SystemAssignedIdentityRole.Name).To(BeEmpty()) - notSystemAssignedTest.machine.Spec.SetIdentityDefaults() - g.Expect(notSystemAssignedTest.machine.Spec.RoleAssignmentName).To(BeEmpty()) + systemAssignedIdentityRoleExistTest.machine.Spec.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(systemAssignedIdentityRoleExistTest.machine.Spec.SystemAssignedIdentityRole.Scope).To(Equal(fakeScope)) + g.Expect(systemAssignedIdentityRoleExistTest.machine.Spec.SystemAssignedIdentityRole.DefinitionID).To(Equal(fakeRoleDefinitionID)) + + emptyTest.machine.Spec.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(emptyTest.machine.Spec.SystemAssignedIdentityRole.Name).To(Not(BeEmpty())) + _, err := uuid.Parse(emptyTest.machine.Spec.SystemAssignedIdentityRole.Name) + g.Expect(err).To(Not(HaveOccurred())) + g.Expect(emptyTest.machine.Spec.SystemAssignedIdentityRole.Scope).To(Equal(fmt.Sprintf("/subscriptions/%s/", fakeSubscriptionID))) + g.Expect(emptyTest.machine.Spec.SystemAssignedIdentityRole.DefinitionID).To(Equal(fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", fakeSubscriptionID, ContributorRoleID))) } func TestAzureMachineSpec_SetDataDisksDefaults(t *testing.T) { diff --git a/api/v1beta1/azuremachine_types.go b/api/v1beta1/azuremachine_types.go index 46348b50907..42fe4c0627b 100644 --- a/api/v1beta1/azuremachine_types.go +++ b/api/v1beta1/azuremachine_types.go @@ -66,8 +66,11 @@ type AzureMachineSpec struct { // +optional UserAssignedIdentities []UserAssignedIdentity `json:"userAssignedIdentities,omitempty"` - // RoleAssignmentName is the name of the role assignment to create for a system assigned identity. It can be any valid GUID. - // If not specified, a random GUID will be generated. + // SystemAssignedIdentityRole defines the role and scope to assign to the system assigned identity. + // +optional + SystemAssignedIdentityRole *SystemAssignedIdentityRole `json:"systemAssignedIdentityRole,omitempty"` + + // Deprecated: RoleAssignmentName should be set in the systemAssignedIdentityRole field. // +optional RoleAssignmentName string `json:"roleAssignmentName,omitempty"` @@ -149,6 +152,24 @@ type SpotVMOptions struct { EvictionPolicy *SpotEvictionPolicy `json:"evictionPolicy,omitempty"` } +// SystemAssignedIdentityRole defines the role and scope to assign to the system assigned identity. +type SystemAssignedIdentityRole struct { + // Name is the name of the role assignment to create for a system assigned identity. It can be any valid GUID. + // If not specified, a random GUID will be generated. + // +optional + Name string `json:"name,omitempty"` + + // DefinitionID is the ID of the role definition to create for a system assigned identity. It can be an Azure built-in role or a custom role. + // Refer to built-in roles: https://learn.microsoft.com/en-us/azure/role-based-access-control/built-in-roles + // +optional + DefinitionID string `json:"definitionID,omitempty"` + + // Scope is the scope that the role assignment or definition applies to. The scope can be any REST resource instance. + // If not specified, the scope will be the subscription. + // +optional + Scope string `json:"scope,omitempty"` +} + // AzureMachineStatus defines the observed state of AzureMachine. type AzureMachineStatus struct { // Ready is true when the provider resource is ready. diff --git a/api/v1beta1/azuremachine_validation.go b/api/v1beta1/azuremachine_validation.go index 894664c858f..b42370bd0dc 100644 --- a/api/v1beta1/azuremachine_validation.go +++ b/api/v1beta1/azuremachine_validation.go @@ -58,6 +58,10 @@ func ValidateAzureMachineSpec(spec AzureMachineSpec) field.ErrorList { allErrs = append(allErrs, errs...) } + if errs := ValidateSystemAssignedIdentityRole(spec.Identity, spec.RoleAssignmentName, spec.SystemAssignedIdentityRole, field.NewPath("systemAssignedIdentityRole")); len(errs) > 0 { + allErrs = append(allErrs, errs...) + } + return allErrs } @@ -117,15 +121,35 @@ func ValidateSystemAssignedIdentity(identityType VMIdentity, oldIdentity, newIde } // ValidateUserAssignedIdentity validates the user-assigned identities list. -func ValidateUserAssignedIdentity(identityType VMIdentity, userAssignedIdenteties []UserAssignedIdentity, fldPath *field.Path) field.ErrorList { +func ValidateUserAssignedIdentity(identityType VMIdentity, userAssignedIdentities []UserAssignedIdentity, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if identityType == VMIdentityUserAssigned && len(userAssignedIdenteties) == 0 { + if identityType == VMIdentityUserAssigned && len(userAssignedIdentities) == 0 { allErrs = append(allErrs, field.Required(fldPath, "must be specified for the 'UserAssigned' identity type")) } return allErrs } +// ValidateSystemAssignedIdentityRole validates the system-assigned identity role. +func ValidateSystemAssignedIdentityRole(identityType VMIdentity, roleAssignmentName string, role *SystemAssignedIdentityRole, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + if roleAssignmentName != "" && role != nil && role.Name != "" { + allErrs = append(allErrs, field.Invalid(fldPath, role.Name, "cannot set both roleAssignmentName and systemAssignedIdentityRole.name")) + } + if identityType == VMIdentitySystemAssigned { + if role.DefinitionID == "" { + allErrs = append(allErrs, field.Invalid(field.NewPath("Spec", "SystemAssignedIdentityRole", "DefinitionID"), role.DefinitionID, "the roleDefinitionID field cannot be empty")) + } + if role.Scope == "" { + allErrs = append(allErrs, field.Invalid(field.NewPath("Spec", "SystemAssignedIdentityRole", "Scope"), role.Scope, "the scope field cannot be empty")) + } + } + if identityType != VMIdentitySystemAssigned && role != nil { + allErrs = append(allErrs, field.Forbidden(field.NewPath("Spec", "Role"), "systemAssignedIdentityRole can only be set when identity is set to SystemAssigned")) + } + return allErrs +} + // ValidateDataDisks validates a list of data disks. func ValidateDataDisks(dataDisks []DataDisk, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} diff --git a/api/v1beta1/azuremachine_validation_test.go b/api/v1beta1/azuremachine_validation_test.go index aae846e3416..cefc5d20be9 100644 --- a/api/v1beta1/azuremachine_validation_test.go +++ b/api/v1beta1/azuremachine_validation_test.go @@ -499,6 +499,87 @@ func TestAzureMachine_ValidateSystemAssignedIdentity(t *testing.T) { } } +func TestAzureMachine_ValidateSystemAssignedIdentityRole(t *testing.T) { + g := NewWithT(t) + + tests := []struct { + name string + Identity VMIdentity + roleAssignmentName string + role *SystemAssignedIdentityRole + wantErr bool + }{ + { + name: "valid role", + Identity: VMIdentitySystemAssigned, + role: &SystemAssignedIdentityRole{ + Name: uuid.New().String(), + Scope: "fake-scope", + DefinitionID: "fake-definition-id", + }, + }, + { + name: "valid role using deprecated role assignment name", + Identity: VMIdentitySystemAssigned, + roleAssignmentName: uuid.New().String(), + role: &SystemAssignedIdentityRole{ + Scope: "fake-scope", + DefinitionID: "fake-definition-id", + }, + }, + { + name: "set both system assigned identity role and role assignment name", + Identity: VMIdentitySystemAssigned, + roleAssignmentName: uuid.New().String(), + role: &SystemAssignedIdentityRole{ + Name: uuid.New().String(), + Scope: "fake-scope", + DefinitionID: "fake-definition-id", + }, + wantErr: true, + }, + { + name: "wrong Identity type", + Identity: VMIdentityUserAssigned, + role: &SystemAssignedIdentityRole{ + Name: uuid.New().String(), + Scope: "fake-scope", + DefinitionID: "fake-definition-id", + }, + wantErr: true, + }, + { + name: "missing scope", + Identity: VMIdentitySystemAssigned, + role: &SystemAssignedIdentityRole{ + Name: uuid.New().String(), + DefinitionID: "fake-definition-id", + }, + wantErr: true, + }, + { + name: "missing definition id", + Identity: VMIdentitySystemAssigned, + role: &SystemAssignedIdentityRole{ + Name: uuid.New().String(), + Scope: "fake-scope", + }, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := ValidateSystemAssignedIdentityRole(tc.Identity, tc.roleAssignmentName, tc.role, field.NewPath("systemAssignedIdentityRole")) + if tc.wantErr { + g.Expect(err).NotTo(BeEmpty()) + } else { + g.Expect(err).To(BeEmpty()) + } + }) + } +} + func TestAzureMachine_ValidateDataDisksUpdate(t *testing.T) { g := NewWithT(t) diff --git a/api/v1beta1/azuremachine_webhook.go b/api/v1beta1/azuremachine_webhook.go index 8f7f5f1700d..10bdf61b5d0 100644 --- a/api/v1beta1/azuremachine_webhook.go +++ b/api/v1beta1/azuremachine_webhook.go @@ -23,29 +23,24 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" webhookutils "sigs.k8s.io/cluster-api-provider-azure/util/webhook" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/client" ) -// SetupWebhookWithManager sets up and registers the webhook with the manager. -func (m *AzureMachine) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(m). - Complete() -} - // +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=azuremachines,versions=v1beta1,name=validation.azuremachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 // +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachine,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=azuremachines,versions=v1beta1,name=default.azuremachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 -var _ webhook.Validator = &AzureMachine{} - // ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (m *AzureMachine) ValidateCreate() error { +func (m *AzureMachine) ValidateCreate(client client.Client) error { spec := m.Spec allErrs := ValidateAzureMachineSpec(spec) - if errs := ValidateSystemAssignedIdentity(spec.Identity, "", spec.RoleAssignmentName, field.NewPath("roleAssignmentName")); len(errs) > 0 { + roleAssignmentName := "" + if spec.SystemAssignedIdentityRole != nil { + roleAssignmentName = spec.SystemAssignedIdentityRole.Name + } + + if errs := ValidateSystemAssignedIdentity(spec.Identity, "", roleAssignmentName, field.NewPath("roleAssignmentName")); len(errs) > 0 { allErrs = append(allErrs, errs...) } @@ -57,7 +52,7 @@ func (m *AzureMachine) ValidateCreate() error { } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object) error { +func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object, client client.Client) error { var allErrs field.ErrorList old := oldRaw.(*AzureMachine) @@ -75,6 +70,13 @@ func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object) error { allErrs = append(allErrs, err) } + if err := webhookutils.ValidateImmutable( + field.NewPath("Spec", "SystemAssignedIdentityRole"), + old.Spec.SystemAssignedIdentityRole, + m.Spec.SystemAssignedIdentityRole); err != nil { + allErrs = append(allErrs, err) + } + if err := webhookutils.ValidateImmutable( field.NewPath("Spec", "UserAssignedIdentities"), old.Spec.UserAssignedIdentities, @@ -179,11 +181,11 @@ func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object) error { } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (m *AzureMachine) ValidateDelete() error { +func (m *AzureMachine) ValidateDelete(client client.Client) error { return nil } -// Default implements webhookutil.defaulter so a webhook will be registered for the type. -func (m *AzureMachine) Default() { - m.Spec.SetDefaults() +// Default implements webhook.Defaulter so a webhook will be registered for the type. +func (m *AzureMachine) Default(client client.Client) { + m.SetDefaults(client) } diff --git a/api/v1beta1/azuremachine_webhook_test.go b/api/v1beta1/azuremachine_webhook_test.go index 10c50986cec..2d87c01ca6d 100644 --- a/api/v1beta1/azuremachine_webhook_test.go +++ b/api/v1beta1/azuremachine_webhook_test.go @@ -17,12 +17,14 @@ limitations under the License. package v1beta1 import ( + "context" "testing" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" ) var ( @@ -146,7 +148,7 @@ func TestAzureMachine_ValidateCreate(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - err := tc.machine.ValidateCreate() + err := tc.machine.ValidateCreate(nil) if tc.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -293,6 +295,50 @@ func TestAzureMachine_ValidateUpdate(t *testing.T) { }, wantErr: false, }, + { + name: "invalidTest: azuremachine.spec.RoleAssignmentName is immutable", + oldMachine: &AzureMachine{ + Spec: AzureMachineSpec{ + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Name: "role", + Scope: "scope", + DefinitionID: "definitionID", + }, + }, + }, + newMachine: &AzureMachine{ + Spec: AzureMachineSpec{ + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Name: "not-role", + Scope: "scope", + DefinitionID: "definitionID", + }, + }, + }, + wantErr: true, + }, + { + name: "validTest: azuremachine.spec.SystemAssignedIdentityRole is immutable", + oldMachine: &AzureMachine{ + Spec: AzureMachineSpec{ + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Name: "role", + Scope: "scope", + DefinitionID: "definitionID", + }, + }, + }, + newMachine: &AzureMachine{ + Spec: AzureMachineSpec{ + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Name: "role", + Scope: "scope", + DefinitionID: "definitionID", + }, + }, + }, + wantErr: false, + }, { name: "invalidTest: azuremachine.spec.OSDisk is immutable", oldMachine: &AzureMachine{ @@ -650,7 +696,7 @@ func TestAzureMachine_ValidateUpdate(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - err := tc.newMachine.ValidateUpdate(tc.oldMachine) + err := tc.newMachine.ValidateUpdate(tc.oldMachine, nil) if tc.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -660,6 +706,16 @@ func TestAzureMachine_ValidateUpdate(t *testing.T) { } } +type mockDefaultClient struct { + client.Client + SubscriptionID string +} + +func (m mockDefaultClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + obj.(*AzureCluster).Spec.SubscriptionID = m.SubscriptionID + return nil +} + func TestAzureMachine_Default(t *testing.T) { g := NewWithT(t) @@ -667,23 +723,25 @@ func TestAzureMachine_Default(t *testing.T) { machine *AzureMachine } + testSubscriptionID := "test-subscription-id" + mockClient := mockDefaultClient{SubscriptionID: testSubscriptionID} existingPublicKey := validSSHPublicKey publicKeyExistTest := test{machine: createMachineWithSSHPublicKey(existingPublicKey)} publicKeyNotExistTest := test{machine: createMachineWithSSHPublicKey("")} - publicKeyExistTest.machine.Default() + publicKeyExistTest.machine.Default(mockClient) g.Expect(publicKeyExistTest.machine.Spec.SSHPublicKey).To(Equal(existingPublicKey)) - publicKeyNotExistTest.machine.Default() + publicKeyNotExistTest.machine.Default(mockClient) g.Expect(publicKeyNotExistTest.machine.Spec.SSHPublicKey).To(Not(BeEmpty())) cacheTypeNotSpecifiedTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: ""}}}} - cacheTypeNotSpecifiedTest.machine.Default() + cacheTypeNotSpecifiedTest.machine.Default(mockClient) g.Expect(cacheTypeNotSpecifiedTest.machine.Spec.OSDisk.CachingType).To(Equal("None")) for _, possibleCachingType := range compute.PossibleCachingTypesValues() { cacheTypeSpecifiedTest := test{machine: &AzureMachine{Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: string(possibleCachingType)}}}} - cacheTypeSpecifiedTest.machine.Default() + cacheTypeSpecifiedTest.machine.Default(mockClient) g.Expect(cacheTypeSpecifiedTest.machine.Spec.OSDisk.CachingType).To(Equal(string(possibleCachingType))) } } @@ -769,10 +827,14 @@ func createMachineWithOsDiskCacheType(cacheType string) *AzureMachine { func createMachineWithRoleAssignmentName() *AzureMachine { machine := &AzureMachine{ Spec: AzureMachineSpec{ - SSHPublicKey: validSSHPublicKey, - OSDisk: validOSDisk, - Identity: VMIdentitySystemAssigned, - RoleAssignmentName: "c6e3443d-bc11-4335-8819-ab6637b10586", + SSHPublicKey: validSSHPublicKey, + OSDisk: validOSDisk, + Identity: VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Name: "c6e3443d-bc11-4335-8819-ab6637b10586", + Scope: "test-scope", + DefinitionID: "test-definition-id", + }, }, } return machine @@ -784,6 +846,10 @@ func createMachineWithoutRoleAssignmentName() *AzureMachine { SSHPublicKey: validSSHPublicKey, OSDisk: validOSDisk, Identity: VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &SystemAssignedIdentityRole{ + Scope: "test-scope", + DefinitionID: "test-definition-id", + }, }, } return machine diff --git a/api/v1beta1/azuremachinetemplate_webhook.go b/api/v1beta1/azuremachinetemplate_webhook.go index 8202a9013fe..f69c28c45bd 100644 --- a/api/v1beta1/azuremachinetemplate_webhook.go +++ b/api/v1beta1/azuremachinetemplate_webhook.go @@ -32,8 +32,9 @@ import ( // AzureMachineTemplateImmutableMsg ... const ( - AzureMachineTemplateImmutableMsg = "AzureMachineTemplate spec.template.spec field is immutable. Please create new resource instead. ref doc: https://cluster-api.sigs.k8s.io/tasks/updating-machine-templates.html" - AzureMachineTemplateRoleAssignmentNameMsg = "AzureMachineTemplate spec.template.spec.roleAssignmentName field can't be set" + AzureMachineTemplateImmutableMsg = "AzureMachineTemplate spec.template.spec field is immutable. Please create new resource instead. ref doc: https://cluster-api.sigs.k8s.io/tasks/updating-machine-templates.html" + AzureMachineTemplateRoleAssignmentNameMsg = "AzureMachineTemplate spec.template.spec.roleAssignmentName field can't be set" + AzureMachineTemplateSystemAssignedIdentityRoleNameMsg = "AzureMachineTemplate spec.template.spec.systemAssignedIdentityRole.name field can't be set" ) // SetupWebhookWithManager sets up and registers the webhook with the manager. @@ -64,6 +65,12 @@ func (r *AzureMachineTemplate) ValidateCreate(ctx context.Context, obj runtime.O ) } + if spec.SystemAssignedIdentityRole != nil && spec.SystemAssignedIdentityRole.Name != "" { + allErrs = append(allErrs, + field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec", "systemAssignedIdentityRole", "name"), t, AzureMachineTemplateSystemAssignedIdentityRoleNameMsg), + ) + } + if (r.Spec.Template.Spec.NetworkInterfaces != nil) && len(r.Spec.Template.Spec.NetworkInterfaces) > 0 && r.Spec.Template.Spec.SubnetName != "" { allErrs = append(allErrs, field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec", "networkInterfaces"), r.Spec.Template.Spec.NetworkInterfaces, "cannot set both NetworkInterfaces and machine SubnetName")) } diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 4a594b56405..1ee35b20679 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -780,6 +780,11 @@ func (in *AzureMachineSpec) DeepCopyInto(out *AzureMachineSpec) { *out = make([]UserAssignedIdentity, len(*in)) copy(*out, *in) } + if in.SystemAssignedIdentityRole != nil { + in, out := &in.SystemAssignedIdentityRole, &out.SystemAssignedIdentityRole + *out = new(SystemAssignedIdentityRole) + **out = **in + } in.OSDisk.DeepCopyInto(&out.OSDisk) if in.DataDisks != nil { in, out := &in.DataDisks, &out.DataDisks @@ -2640,6 +2645,21 @@ func (in Subnets) DeepCopy() Subnets { return *out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *SystemAssignedIdentityRole) DeepCopyInto(out *SystemAssignedIdentityRole) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SystemAssignedIdentityRole. +func (in *SystemAssignedIdentityRole) DeepCopy() *SystemAssignedIdentityRole { + if in == nil { + return nil + } + out := new(SystemAssignedIdentityRole) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in Tags) DeepCopyInto(out *Tags) { { diff --git a/azure/defaults.go b/azure/defaults.go index de2827cdb8c..3f186f3b737 100644 --- a/azure/defaults.go +++ b/azure/defaults.go @@ -87,9 +87,6 @@ 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 ( @@ -113,16 +110,6 @@ 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 82a4e9c1c57..961eae0412b 100644 --- a/azure/scope/machine.go +++ b/azure/scope/machine.go @@ -335,12 +335,12 @@ func (m *MachineScope) RoleAssignmentSpecs(principalID *string) []azure.Resource roles := make([]azure.ResourceSpecGetter, 1) if m.HasSystemAssignedIdentity() { roles[0] = &roleassignments.RoleAssignmentSpec{ - Name: m.AzureMachine.Spec.RoleAssignmentName, + Name: m.SystemAssignedIdentityName(), MachineName: m.Name(), ResourceType: azure.VirtualMachine, ResourceGroup: m.ResourceGroup(), - Scope: azure.GenerateSubscriptionScope(m.SubscriptionID()), - RoleDefinitionID: azure.GenerateContributorRoleDefinitionID(m.SubscriptionID()), + Scope: m.SystemAssignedIdentityScope(), + RoleDefinitionID: m.SystemAssignedIdentityDefinitionID(), PrincipalID: principalID, } return roles @@ -521,6 +521,30 @@ func (m *MachineScope) AvailabilitySetID() string { return asID } +// SystemAssignedIdentityName returns the role assignment name for the system assigned identity. +func (m *MachineScope) SystemAssignedIdentityName() string { + if m.AzureMachine.Spec.SystemAssignedIdentityRole != nil { + return m.AzureMachine.Spec.SystemAssignedIdentityRole.Name + } + return "" +} + +// SystemAssignedIdentityScope returns the scope for the system assigned identity. +func (m *MachineScope) SystemAssignedIdentityScope() string { + if m.AzureMachine.Spec.SystemAssignedIdentityRole != nil { + return m.AzureMachine.Spec.SystemAssignedIdentityRole.Scope + } + return "" +} + +// SystemAssignedIdentityDefinitionID returns the role definition id for the system assigned identity. +func (m *MachineScope) SystemAssignedIdentityDefinitionID() string { + if m.AzureMachine.Spec.SystemAssignedIdentityRole != nil { + return m.AzureMachine.Spec.SystemAssignedIdentityRole.DefinitionID + } + return "" +} + // SetProviderID sets the AzureMachine providerID in spec. func (m *MachineScope) SetProviderID(v string) { m.AzureMachine.Spec.ProviderID = pointer.String(v) diff --git a/azure/scope/machine_test.go b/azure/scope/machine_test.go index 8b0dfbcc432..11fce00def1 100644 --- a/azure/scope/machine_test.go +++ b/azure/scope/machine_test.go @@ -425,7 +425,7 @@ func TestMachineScope_RoleAssignmentSpecs(t *testing.T) { want []azure.ResourceSpecGetter }{ { - name: "returns empty if VM identity is system assigned", + name: "returns empty if VM identity is not system assigned", machineScope: MachineScope{ Machine: &clusterv1.Machine{}, AzureMachine: &infrav1.AzureMachine{ @@ -437,7 +437,7 @@ func TestMachineScope_RoleAssignmentSpecs(t *testing.T) { want: []azure.ResourceSpecGetter{}, }, { - name: "returns RoleAssignmentSpec if VM identity is not system assigned", + name: "returns RoleAssignmentSpec if VM identity is system assigned", machineScope: MachineScope{ Machine: &clusterv1.Machine{}, AzureMachine: &infrav1.AzureMachine{ @@ -445,8 +445,55 @@ func TestMachineScope_RoleAssignmentSpecs(t *testing.T) { Name: "machine-name", }, Spec: infrav1.AzureMachineSpec{ - Identity: infrav1.VMIdentitySystemAssigned, - RoleAssignmentName: "azure-role-assignment-name", + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: "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", + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + Location: "westus", + }, + }, + }, + }, + }, + want: []azure.ResourceSpecGetter{ + &roleassignments.RoleAssignmentSpec{ + ResourceType: azure.VirtualMachine, + MachineName: "machine-name", + Name: "azure-role-assignment-name", + ResourceGroup: "my-rg", + PrincipalID: pointer.String("fakePrincipalID"), + }, + }, + }, + { + name: "returns RoleAssignmentSpec with specified scope and role assignment id", + machineScope: MachineScope{ + Machine: &clusterv1.Machine{}, + AzureMachine: &infrav1.AzureMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-name", + }, + Spec: infrav1.AzureMachineSpec{ + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: "azure-role-assignment-name", + Scope: "/subscriptions/123/resourceGroups/my-rg", + DefinitionID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Authorization/roleAssignments/123", + }, }, }, ClusterScoper: &ClusterScope{ @@ -473,8 +520,8 @@ func TestMachineScope_RoleAssignmentSpecs(t *testing.T) { MachineName: "machine-name", Name: "azure-role-assignment-name", ResourceGroup: "my-rg", - Scope: azure.GenerateSubscriptionScope("123"), - RoleDefinitionID: azure.GenerateContributorRoleDefinitionID("123"), + Scope: "/subscriptions/123/resourceGroups/my-rg", + RoleDefinitionID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Authorization/roleAssignments/123", PrincipalID: pointer.String("fakePrincipalID"), }, }, diff --git a/azure/scope/machinepool.go b/azure/scope/machinepool.go index 490b2bfa8fb..e427d4f086d 100644 --- a/azure/scope/machinepool.go +++ b/azure/scope/machinepool.go @@ -165,6 +165,30 @@ func (m *MachinePoolScope) SetProviderID(v string) { m.AzureMachinePool.Spec.ProviderID = v } +// SystemAssignedIdentityName returns the scope for the system assigned identity. +func (m *MachinePoolScope) SystemAssignedIdentityName() string { + if m.AzureMachinePool.Spec.SystemAssignedIdentityRole != nil { + return m.AzureMachinePool.Spec.SystemAssignedIdentityRole.Name + } + return "" +} + +// SystemAssignedIdentityScope returns the scope for the system assigned identity. +func (m *MachinePoolScope) SystemAssignedIdentityScope() string { + if m.AzureMachinePool.Spec.SystemAssignedIdentityRole != nil { + return m.AzureMachinePool.Spec.SystemAssignedIdentityRole.Scope + } + return "" +} + +// SystemAssignedIdentityDefinitionID returns the role definition ID for the system assigned identity. +func (m *MachinePoolScope) SystemAssignedIdentityDefinitionID() string { + if m.AzureMachinePool.Spec.SystemAssignedIdentityRole != nil { + return m.AzureMachinePool.Spec.SystemAssignedIdentityRole.DefinitionID + } + return "" +} + // ProvisioningState returns the AzureMachinePool provisioning state. func (m *MachinePoolScope) ProvisioningState() infrav1.ProvisioningState { if m.AzureMachinePool.Status.ProvisioningState != nil { @@ -586,11 +610,13 @@ func (m *MachinePoolScope) RoleAssignmentSpecs(principalID *string) []azure.Reso 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, + Name: m.SystemAssignedIdentityName(), + MachineName: m.Name(), + ResourceGroup: m.ResourceGroup(), + ResourceType: azure.VirtualMachineScaleSet, + Scope: m.SystemAssignedIdentityScope(), + RoleDefinitionID: m.SystemAssignedIdentityDefinitionID(), + PrincipalID: principalID, } return roles } diff --git a/azure/scope/machinepool_test.go b/azure/scope/machinepool_test.go index 03fc324491d..149b7c4042b 100644 --- a/azure/scope/machinepool_test.go +++ b/azure/scope/machinepool_test.go @@ -34,6 +34,7 @@ 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/mock_azure" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments" "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -657,6 +658,123 @@ func TestMachinePoolScope_updateReplicasAndProviderIDs(t *testing.T) { } } +func TestMachinePoolScope_RoleAssignmentSpecs(t *testing.T) { + tests := []struct { + name string + machinePoolScope MachinePoolScope + want []azure.ResourceSpecGetter + }{ + { + name: "returns empty if VM identity is not system assigned", + machinePoolScope: MachinePoolScope{ + AzureMachinePool: &infrav1exp.AzureMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-name", + }, + }, + }, + want: []azure.ResourceSpecGetter{}, + }, + { + name: "returns role assignment spec if VM identity is system assigned", + machinePoolScope: MachinePoolScope{ + MachinePool: &expv1.MachinePool{}, + AzureMachinePool: &infrav1exp.AzureMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-name", + }, + Spec: infrav1exp.AzureMachinePoolSpec{ + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: "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", + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + Location: "westus", + }, + }, + }, + }, + }, + want: []azure.ResourceSpecGetter{ + &roleassignments.RoleAssignmentSpec{ + ResourceType: azure.VirtualMachineScaleSet, + MachineName: "machine-name", + Name: "role-assignment-name", + ResourceGroup: "my-rg", + PrincipalID: pointer.String("fakePrincipalID"), + }, + }, + }, + { + name: "returns role assignment spec if scope and role definition ID are set", + machinePoolScope: MachinePoolScope{ + MachinePool: &expv1.MachinePool{}, + AzureMachinePool: &infrav1exp.AzureMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-name", + }, + Spec: infrav1exp.AzureMachinePoolSpec{ + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: "role-assignment-name", + Scope: "scope", + DefinitionID: "role-definition-id", + }, + }, + }, + ClusterScoper: &ClusterScope{ + AzureClients: AzureClients{ + EnvironmentSettings: auth.EnvironmentSettings{ + Values: map[string]string{ + auth.SubscriptionID: "123", + }, + }, + }, + AzureCluster: &infrav1.AzureCluster{ + Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", + AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ + Location: "westus", + }, + }, + }, + }, + }, + want: []azure.ResourceSpecGetter{ + &roleassignments.RoleAssignmentSpec{ + ResourceType: azure.VirtualMachineScaleSet, + MachineName: "machine-name", + Name: "role-assignment-name", + ResourceGroup: "my-rg", + Scope: "scope", + RoleDefinitionID: "role-definition-id", + PrincipalID: pointer.String("fakePrincipalID"), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.machinePoolScope.RoleAssignmentSpecs(pointer.String("fakePrincipalID")); !reflect.DeepEqual(got, tt.want) { + t.Errorf("RoleAssignmentSpecs() = %v, want %v", got, tt.want) + } + }) + } +} + func TestMachinePoolScope_VMSSExtensionSpecs(t *testing.T) { tests := []struct { name string diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml index 7098135d1dd..66b659c2900 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml @@ -1401,9 +1401,8 @@ spec: type: string type: array roleAssignmentName: - description: RoleAssignmentName is the name of the role assignment - to create for a system assigned identity. It can be any valid GUID. - If not specified, a random GUID will be generated. + description: 'Deprecated: RoleAssignmentName should be set in the + systemAssignedIdentityRole field.' type: string strategy: default: @@ -1475,6 +1474,26 @@ spec: - RollingUpdate type: string type: object + systemAssignedIdentityRole: + description: SystemAssignedIdentityRole defines the role and scope + to assign to the system assigned identity. + properties: + definitionID: + description: 'DefinitionID is the ID of the role definition to + create for a system assigned identity. It can be an Azure built-in + role or a custom role. Refer to built-in roles: https://learn.microsoft.com/en-us/azure/role-based-access-control/built-in-roles' + type: string + name: + description: Name is the name of the role assignment to create + for a system assigned identity. It can be any valid GUID. If + not specified, a random GUID will be generated. + type: string + scope: + description: Scope is the scope that the role assignment or definition + applies to. The scope can be any REST resource instance. If + not specified, the scope will be the subscription. + type: string + type: object template: description: Template contains the details used to build a replica virtual machine within the Machine Pool diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml index 4b9761b7594..c64ae4b8033 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml @@ -1458,9 +1458,8 @@ spec: cloud provider. type: string roleAssignmentName: - description: RoleAssignmentName is the name of the role assignment - to create for a system assigned identity. It can be any valid GUID. - If not specified, a random GUID will be generated. + description: 'Deprecated: RoleAssignmentName should be set in the + systemAssignedIdentityRole field.' type: string securityProfile: description: SecurityProfile specifies the Security profile settings @@ -1498,6 +1497,26 @@ spec: description: 'Deprecated: SubnetName should be set in the networkInterfaces field.' type: string + systemAssignedIdentityRole: + description: SystemAssignedIdentityRole defines the role and scope + to assign to the system assigned identity. + properties: + definitionID: + description: 'DefinitionID is the ID of the role definition to + create for a system assigned identity. It can be an Azure built-in + role or a custom role. Refer to built-in roles: https://learn.microsoft.com/en-us/azure/role-based-access-control/built-in-roles' + type: string + name: + description: Name is the name of the role assignment to create + for a system assigned identity. It can be any valid GUID. If + not specified, a random GUID will be generated. + type: string + scope: + description: Scope is the scope that the role assignment or definition + applies to. The scope can be any REST resource instance. If + not specified, the scope will be the subscription. + type: string + type: object userAssignedIdentities: description: UserAssignedIdentities is a list of standalone Azure identities provided by the user The lifecycle of a user-assigned diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml index afe99143e37..0d806c6d11d 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml @@ -1241,9 +1241,8 @@ spec: by the cloud provider. type: string roleAssignmentName: - description: RoleAssignmentName is the name of the role assignment - to create for a system assigned identity. It can be any - valid GUID. If not specified, a random GUID will be generated. + description: 'Deprecated: RoleAssignmentName should be set + in the systemAssignedIdentityRole field.' type: string securityProfile: description: SecurityProfile specifies the Security profile @@ -1282,6 +1281,29 @@ spec: description: 'Deprecated: SubnetName should be set in the networkInterfaces field.' type: string + systemAssignedIdentityRole: + description: SystemAssignedIdentityRole defines the role and + scope to assign to the system assigned identity. + properties: + definitionID: + description: 'DefinitionID is the ID of the role definition + to create for a system assigned identity. It can be + an Azure built-in role or a custom role. Refer to built-in + roles: https://learn.microsoft.com/en-us/azure/role-based-access-control/built-in-roles' + type: string + name: + description: Name is the name of the role assignment to + create for a system assigned identity. It can be any + valid GUID. If not specified, a random GUID will be + generated. + type: string + scope: + description: Scope is the scope that the role assignment + or definition applies to. The scope can be any REST + resource instance. If not specified, the scope will + be the subscription. + type: string + type: object userAssignedIdentities: description: UserAssignedIdentities is a list of standalone Azure identities provided by the user The lifecycle of a diff --git a/docs/book/src/topics/vm-identity.md b/docs/book/src/topics/vm-identity.md index 93ef4776991..fe938ba1d62 100644 --- a/docs/book/src/topics/vm-identity.md +++ b/docs/book/src/topics/vm-identity.md @@ -104,6 +104,24 @@ spec: The CAPZ controller will look for `SystemAssigned` value in `identity` field under `AzureMachineTemplate`, and enable system-assigned managed identity in the virtual machine. +For more granularity regarding permissions, you can specify the scope and the role assignment of the system-assigned managed identity by setting the `scope` and `definitionID` fields inside the `systemAssignedIdentityRole` struct. In the following example, we assign the `Owner` role to the system-assigned managed identity on the resource group. IDs for the role assignments can be found in the [Azure docs](https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles). + +```yaml +apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 +kind: AzureMachineTemplate +metadata: + name: ${CLUSTER_NAME}-md-0 + namespace: default +spec: + template: + spec: + identity: SystemAssigned + systemAssignedIdentityRole: + scope: /subscriptions/${AZURE_SUBSCRIPTION_ID}/resourceGroups/${RESOURCE_GROUP_NAME} + definitionID: $/subscriptions/${AZURE_SUBSCRIPTION_ID}/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635 + ... +``` + * In Machine Pool ```yaml diff --git a/exp/api/v1alpha3/azuremachinepool_conversion.go b/exp/api/v1alpha3/azuremachinepool_conversion.go index ad8d8f663b6..32550eebad1 100644 --- a/exp/api/v1alpha3/azuremachinepool_conversion.go +++ b/exp/api/v1alpha3/azuremachinepool_conversion.go @@ -108,6 +108,10 @@ func (src *AzureMachinePool) ConvertTo(dstRaw conversion.Hub) error { // Restore orchestration mode dst.Spec.OrchestrationMode = restored.Spec.OrchestrationMode + if restored.Spec.SystemAssignedIdentityRole != nil { + dst.Spec.SystemAssignedIdentityRole = restored.Spec.SystemAssignedIdentityRole + } + return nil } diff --git a/exp/api/v1alpha3/zz_generated.conversion.go b/exp/api/v1alpha3/zz_generated.conversion.go index a3eb54a79e7..bea4d377864 100644 --- a/exp/api/v1alpha3/zz_generated.conversion.go +++ b/exp/api/v1alpha3/zz_generated.conversion.go @@ -347,6 +347,7 @@ func autoConvert_v1beta1_AzureMachinePoolSpec_To_v1alpha3_AzureMachinePoolSpec(i out.ProviderID = in.ProviderID out.ProviderIDList = *(*[]string)(unsafe.Pointer(&in.ProviderIDList)) out.Identity = clusterapiproviderazureapiv1alpha3.VMIdentity(in.Identity) + // WARNING: in.SystemAssignedIdentityRole requires manual conversion: does not exist in peer-type out.UserAssignedIdentities = *(*[]clusterapiproviderazureapiv1alpha3.UserAssignedIdentity)(unsafe.Pointer(&in.UserAssignedIdentities)) out.RoleAssignmentName = in.RoleAssignmentName // WARNING: in.Strategy requires manual conversion: does not exist in peer-type diff --git a/exp/api/v1alpha4/azuremachinepool_conversion.go b/exp/api/v1alpha4/azuremachinepool_conversion.go index 163a4ec1f52..582640bd94e 100644 --- a/exp/api/v1alpha4/azuremachinepool_conversion.go +++ b/exp/api/v1alpha4/azuremachinepool_conversion.go @@ -74,6 +74,10 @@ func (src *AzureMachinePool) ConvertTo(dstRaw conversion.Hub) error { // Restore orchestration mode dst.Spec.OrchestrationMode = restored.Spec.OrchestrationMode + if restored.Spec.SystemAssignedIdentityRole != nil { + dst.Spec.SystemAssignedIdentityRole = restored.Spec.SystemAssignedIdentityRole + } + return nil } diff --git a/exp/api/v1alpha4/zz_generated.conversion.go b/exp/api/v1alpha4/zz_generated.conversion.go index a79a31cd724..e15b1205d3d 100644 --- a/exp/api/v1alpha4/zz_generated.conversion.go +++ b/exp/api/v1alpha4/zz_generated.conversion.go @@ -551,6 +551,7 @@ func autoConvert_v1beta1_AzureMachinePoolSpec_To_v1alpha4_AzureMachinePoolSpec(i out.ProviderID = in.ProviderID out.ProviderIDList = *(*[]string)(unsafe.Pointer(&in.ProviderIDList)) out.Identity = clusterapiproviderazureapiv1alpha4.VMIdentity(in.Identity) + // WARNING: in.SystemAssignedIdentityRole requires manual conversion: does not exist in peer-type out.UserAssignedIdentities = *(*[]clusterapiproviderazureapiv1alpha4.UserAssignedIdentity)(unsafe.Pointer(&in.UserAssignedIdentities)) out.RoleAssignmentName = in.RoleAssignmentName if err := Convert_v1beta1_AzureMachinePoolDeploymentStrategy_To_v1alpha4_AzureMachinePoolDeploymentStrategy(&in.Strategy, &out.Strategy, s); err != nil { diff --git a/exp/api/v1beta1/azuremachinepool_default.go b/exp/api/v1beta1/azuremachinepool_default.go index ece0b4ac026..3a37befbde9 100644 --- a/exp/api/v1beta1/azuremachinepool_default.go +++ b/exp/api/v1beta1/azuremachinepool_default.go @@ -18,13 +18,38 @@ package v1beta1 import ( "encoding/base64" + "fmt" "golang.org/x/crypto/ssh" "k8s.io/apimachinery/pkg/util/uuid" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure" utilSSH "sigs.k8s.io/cluster-api-provider-azure/util/ssh" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" ) +// SetDefaults sets the default values for an AzureMachinePool. +func (amp *AzureMachinePool) SetDefaults(client client.Client) { + if err := amp.SetDefaultSSHPublicKey(); err != nil { + ctrl.Log.WithName("AzureMachinePoolLogger").Error(err, "SetDefaultSshPublicKey failed") + } + + machinePool, err := azureutil.FindParentMachinePoolWithRetry(amp.Name, client, 5) + if err != nil { + ctrl.Log.WithName("AzureMachinePoolLogger").Error(err, "findParentMachinePool failed") + } + + subscriptionID, err := infrav1.GetSubscriptionID(client, machinePool.Spec.ClusterName, machinePool.Namespace, 5) + if err != nil { + ctrl.Log.WithName("AzureMachinePoolLogger").Error(err, "getSubscriptionID failed") + } + + amp.SetIdentityDefaults(subscriptionID) + amp.SetDiagnosticsDefaults() + amp.SetNetworkInterfacesDefaults() +} + // SetDefaultSSHPublicKey sets the default SSHPublicKey for an AzureMachinePool. func (amp *AzureMachinePool) SetDefaultSSHPublicKey() error { if sshKeyData := amp.Spec.Template.SSHPublicKey; sshKeyData == "" { @@ -39,11 +64,35 @@ func (amp *AzureMachinePool) SetDefaultSSHPublicKey() error { } // SetIdentityDefaults sets the defaults for VMSS Identity. -func (amp *AzureMachinePool) SetIdentityDefaults() { +func (amp *AzureMachinePool) SetIdentityDefaults(subscriptionID string) { + // Ensure the deprecated fields and new fields are not populated simultaneously + if amp.Spec.RoleAssignmentName != "" && amp.Spec.SystemAssignedIdentityRole != nil && amp.Spec.SystemAssignedIdentityRole.Name != "" { + // Both the deprecated and the new fields are both set, return without changes + // and reject the request in the validating webhook which runs later. + return + } if amp.Spec.Identity == infrav1.VMIdentitySystemAssigned { if amp.Spec.RoleAssignmentName == "" { amp.Spec.RoleAssignmentName = string(uuid.NewUUID()) } + if amp.Spec.SystemAssignedIdentityRole == nil { + amp.Spec.SystemAssignedIdentityRole = &infrav1.SystemAssignedIdentityRole{} + } + if amp.Spec.SystemAssignedIdentityRole.Name == "" { + amp.Spec.SystemAssignedIdentityRole.Name = string(uuid.NewUUID()) + if amp.Spec.RoleAssignmentName != "" { + amp.Spec.SystemAssignedIdentityRole.Name = amp.Spec.RoleAssignmentName + amp.Spec.RoleAssignmentName = "" + } + } + if amp.Spec.SystemAssignedIdentityRole.Scope == "" { + // Default scope to the subscription. + amp.Spec.SystemAssignedIdentityRole.Scope = fmt.Sprintf("/subscriptions/%s/", subscriptionID) + } + if amp.Spec.SystemAssignedIdentityRole.DefinitionID == "" { + // Default role definition ID to Contributor role. + amp.Spec.SystemAssignedIdentityRole.DefinitionID = fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", subscriptionID, infrav1.ContributorRoleID) + } } } diff --git a/exp/api/v1beta1/azuremachinepool_default_test.go b/exp/api/v1beta1/azuremachinepool_default_test.go index 3f401b4d5f0..08985ea3f5b 100644 --- a/exp/api/v1beta1/azuremachinepool_default_test.go +++ b/exp/api/v1beta1/azuremachinepool_default_test.go @@ -17,10 +17,12 @@ limitations under the License. package v1beta1 import ( + "fmt" "testing" "github.com/google/uuid" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" ) @@ -52,29 +54,49 @@ func TestAzureMachinePool_SetIdentityDefaults(t *testing.T) { machinePool *AzureMachinePool } + fakeSubscriptionID := uuid.New().String() + fakeClusterName := "testcluster" + fakeRoleDefinitionID := "testroledefinitionid" + fakeScope := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", fakeSubscriptionID, fakeClusterName) existingRoleAssignmentName := "42862306-e485-4319-9bf0-35dbc6f6fe9c" roleAssignmentExistTest := test{machinePool: &AzureMachinePool{Spec: AzureMachinePoolSpec{ - Identity: infrav1.VMIdentitySystemAssigned, - RoleAssignmentName: existingRoleAssignmentName, - }}} - roleAssignmentEmptyTest := test{machinePool: &AzureMachinePool{Spec: AzureMachinePoolSpec{ - Identity: infrav1.VMIdentitySystemAssigned, - RoleAssignmentName: "", + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: existingRoleAssignmentName, + }, }}} notSystemAssignedTest := test{machinePool: &AzureMachinePool{Spec: AzureMachinePoolSpec{ Identity: infrav1.VMIdentityUserAssigned, }}} + systemAssignedIdentityRoleExistTest := test{machinePool: &AzureMachinePool{Spec: AzureMachinePoolSpec{ + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + DefinitionID: fakeRoleDefinitionID, + Scope: fakeScope, + }, + }}} + emptyTest := test{machinePool: &AzureMachinePool{Spec: AzureMachinePoolSpec{ + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{}, + }}} - roleAssignmentExistTest.machinePool.SetIdentityDefaults() - g.Expect(roleAssignmentExistTest.machinePool.Spec.RoleAssignmentName).To(Equal(existingRoleAssignmentName)) + roleAssignmentExistTest.machinePool.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(roleAssignmentExistTest.machinePool.Spec.SystemAssignedIdentityRole.Name).To(Equal(existingRoleAssignmentName)) - roleAssignmentEmptyTest.machinePool.SetIdentityDefaults() - g.Expect(roleAssignmentEmptyTest.machinePool.Spec.RoleAssignmentName).To(Not(BeEmpty())) - _, err := uuid.Parse(roleAssignmentEmptyTest.machinePool.Spec.RoleAssignmentName) - g.Expect(err).To(Not(HaveOccurred())) + notSystemAssignedTest.machinePool.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(notSystemAssignedTest.machinePool.Spec.SystemAssignedIdentityRole).To(BeNil()) + + systemAssignedIdentityRoleExistTest.machinePool.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(systemAssignedIdentityRoleExistTest.machinePool.Spec.SystemAssignedIdentityRole.Scope).To(Equal(fakeScope)) + g.Expect(systemAssignedIdentityRoleExistTest.machinePool.Spec.SystemAssignedIdentityRole.DefinitionID).To(Equal(fakeRoleDefinitionID)) - notSystemAssignedTest.machinePool.SetIdentityDefaults() - g.Expect(notSystemAssignedTest.machinePool.Spec.RoleAssignmentName).To(BeEmpty()) + emptyTest.machinePool.SetIdentityDefaults(fakeSubscriptionID) + g.Expect(emptyTest.machinePool.Spec.SystemAssignedIdentityRole.Name).To(Not(BeEmpty())) + _, err := uuid.Parse(emptyTest.machinePool.Spec.SystemAssignedIdentityRole.Name) + g.Expect(err).To(Not(HaveOccurred())) + g.Expect(emptyTest.machinePool.Spec.SystemAssignedIdentityRole).To(Not(BeNil())) + g.Expect(emptyTest.machinePool.Spec.SystemAssignedIdentityRole.Scope).To(Equal(fmt.Sprintf("/subscriptions/%s/", fakeSubscriptionID))) + g.Expect(emptyTest.machinePool.Spec.SystemAssignedIdentityRole.DefinitionID).To(Equal(fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", fakeSubscriptionID, infrav1.ContributorRoleID))) } func TestAzureMachinePool_SetDiagnosticsDefaults(t *testing.T) { @@ -248,5 +270,8 @@ func hardcodedAzureMachinePoolWithSSHKey(sshPublicKey string) *AzureMachinePool SSHPublicKey: sshPublicKey, }, }, + ObjectMeta: metav1.ObjectMeta{ + Name: "testmachinepool", + }, } } diff --git a/exp/api/v1beta1/azuremachinepool_types.go b/exp/api/v1beta1/azuremachinepool_types.go index 3ed3e787041..3b12008fffd 100644 --- a/exp/api/v1beta1/azuremachinepool_types.go +++ b/exp/api/v1beta1/azuremachinepool_types.go @@ -135,6 +135,10 @@ type ( // +optional Identity infrav1.VMIdentity `json:"identity,omitempty"` + // SystemAssignedIdentityRole defines the role and scope to assign to the system assigned identity. + // +optional + SystemAssignedIdentityRole *infrav1.SystemAssignedIdentityRole `json:"systemAssignedIdentityRole,omitempty"` + // UserAssignedIdentities is a list of standalone Azure identities provided by the user // The lifecycle of a user-assigned identity is managed separately from the lifecycle of // the AzureMachinePool. @@ -142,8 +146,7 @@ type ( // +optional UserAssignedIdentities []infrav1.UserAssignedIdentity `json:"userAssignedIdentities,omitempty"` - // RoleAssignmentName is the name of the role assignment to create for a system assigned identity. It can be any valid GUID. - // If not specified, a random GUID will be generated. + // Deprecated: RoleAssignmentName should be set in the systemAssignedIdentityRole field. // +optional RoleAssignmentName string `json:"roleAssignmentName,omitempty"` diff --git a/exp/api/v1beta1/azuremachinepool_webhook.go b/exp/api/v1beta1/azuremachinepool_webhook.go index a26c84e532e..d661311d93b 100644 --- a/exp/api/v1beta1/azuremachinepool_webhook.go +++ b/exp/api/v1beta1/azuremachinepool_webhook.go @@ -46,12 +46,7 @@ func (amp *AzureMachinePool) SetupWebhookWithManager(mgr ctrl.Manager) error { // Default implements webhook.Defaulter so a webhook will be registered for the type. func (amp *AzureMachinePool) Default(client client.Client) { - if err := amp.SetDefaultSSHPublicKey(); err != nil { - ctrl.Log.WithName("AzureMachinePoolLogger").Error(err, "SetDefaultSshPublicKey failed") - } - amp.SetIdentityDefaults() - amp.SetDiagnosticsDefaults() - amp.SetNetworkInterfacesDefaults() + amp.SetDefaults(client) } // +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachinepool,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=azuremachinepools,versions=v1beta1,name=validation.azuremachinepool.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 @@ -90,6 +85,7 @@ func (amp *AzureMachinePool) Validate(old runtime.Object, client client.Client) amp.ValidateOrchestrationMode(client), amp.ValidateStrategy(), amp.ValidateSystemAssignedIdentity(old), + amp.ValidateSystemAssignedIdentityRole, amp.ValidateNetwork, } @@ -190,11 +186,18 @@ func (amp *AzureMachinePool) ValidateSystemAssignedIdentity(old runtime.Object) return fmt.Errorf("unexpected type for old azure machine pool object. Expected: %q, Got: %q", "AzureMachinePool", reflect.TypeOf(old)) } - oldRole = oldMachinePool.Spec.RoleAssignmentName + if amp.Spec.SystemAssignedIdentityRole != nil { + oldRole = oldMachinePool.Spec.SystemAssignedIdentityRole.Name + } + } + + roleAssignmentName := "" + if amp.Spec.SystemAssignedIdentityRole != nil { + roleAssignmentName = amp.Spec.SystemAssignedIdentityRole.Name } fldPath := field.NewPath("roleAssignmentName") - if errs := infrav1.ValidateSystemAssignedIdentity(amp.Spec.Identity, oldRole, amp.Spec.RoleAssignmentName, fldPath); len(errs) > 0 { + if errs := infrav1.ValidateSystemAssignedIdentity(amp.Spec.Identity, oldRole, roleAssignmentName, fldPath); len(errs) > 0 { return kerrors.NewAggregate(errs.ToAggregate().Errors()) } @@ -202,6 +205,31 @@ func (amp *AzureMachinePool) ValidateSystemAssignedIdentity(old runtime.Object) } } +// ValidateSystemAssignedIdentityRole validates the scope and roleDefinitionID for the system-assigned identity. +func (amp *AzureMachinePool) ValidateSystemAssignedIdentityRole() error { + var allErrs field.ErrorList + if amp.Spec.RoleAssignmentName != "" && amp.Spec.SystemAssignedIdentityRole.Name != "" { + allErrs = append(allErrs, field.Invalid(field.NewPath("systemAssignedIdentityRole"), amp.Spec.SystemAssignedIdentityRole.Name, "cannot set both roleAssignmentName and systemAssignedIdentityRole.name")) + } + if amp.Spec.Identity == infrav1.VMIdentitySystemAssigned { + if amp.Spec.SystemAssignedIdentityRole.DefinitionID == "" { + allErrs = append(allErrs, field.Invalid(field.NewPath("systemAssignedIdentityRole", "DefinitionID"), amp.Spec.SystemAssignedIdentityRole.DefinitionID, "the roleDefinitionID field cannot be empty")) + } + if amp.Spec.SystemAssignedIdentityRole.Scope == "" { + allErrs = append(allErrs, field.Invalid(field.NewPath("systemAssignedIdentityRole", "Scope"), amp.Spec.SystemAssignedIdentityRole.Scope, "the scope field cannot be empty")) + } + } + if amp.Spec.Identity != infrav1.VMIdentitySystemAssigned && amp.Spec.SystemAssignedIdentityRole != nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("systemAssignedIdentityRole"), amp.Spec.SystemAssignedIdentityRole, "systemAssignedIdentityRole can only be set when identity is set to 'SystemAssigned'")) + } + + if len(allErrs) > 0 { + return kerrors.NewAggregate(allErrs.ToAggregate().Errors()) + } + + return nil +} + // ValidateDiagnostics validates the Diagnostic spec. func (amp *AzureMachinePool) ValidateDiagnostics() error { var allErrs field.ErrorList diff --git a/exp/api/v1beta1/azuremachinepool_webhook_test.go b/exp/api/v1beta1/azuremachinepool_webhook_test.go index 40c3e8a9527..7ac35a846d1 100644 --- a/exp/api/v1beta1/azuremachinepool_webhook_test.go +++ b/exp/api/v1beta1/azuremachinepool_webhook_test.go @@ -21,6 +21,7 @@ import ( "crypto/rand" "crypto/rsa" "encoding/base64" + "fmt" "testing" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute" @@ -28,12 +29,15 @@ import ( . "github.com/onsi/gomega" "github.com/pkg/errors" "golang.org/x/crypto/ssh" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/uuid" utilfeature "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/feature" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" capifeature "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/controller-runtime/pkg/client" @@ -45,6 +49,28 @@ var ( one = intstr.FromInt(1) ) +type mockClient struct { + client.Client + Version string + ReturnError bool +} + +func (m mockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + obj.(*expv1.MachinePool).Spec.Template.Spec.Version = &m.Version + return nil +} + +func (m mockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if m.ReturnError { + return errors.New("MachinePool.cluster.x-k8s.io \"mock-machinepool-mp-0\" not found") + } + mp := &expv1.MachinePool{} + mp.Spec.Template.Spec.Version = &m.Version + list.(*expv1.MachinePoolList).Items = []expv1.MachinePool{*mp} + + return nil +} + func TestAzureMachinePool_ValidateCreate(t *testing.T) { // NOTE: AzureMachinePool is behind MachinePool feature gate flag; the webhook // must prevent creating new objects in case the feature flag is disabled. @@ -225,19 +251,35 @@ func TestAzureMachinePool_ValidateCreate(t *testing.T) { } } -type mockClient struct { +type mockDefaultClient struct { client.Client - Version string - ReturnError bool + Name string + ClusterName string + SubscriptionID string + Version string + ReturnError bool } -func (m mockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - if m.ReturnError { - return errors.New("MachinePool.cluster.x-k8s.io \"mock-machinepool-mp-0\" not found") +func (m mockDefaultClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + obj.(*infrav1.AzureCluster).Spec.SubscriptionID = m.SubscriptionID + return nil +} + +func (m mockDefaultClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + list.(*expv1.MachinePoolList).Items = []expv1.MachinePool{ + { + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + InfrastructureRef: corev1.ObjectReference{ + Name: m.Name, + }, + }, + }, + ClusterName: m.ClusterName, + }, + }, } - mp := &expv1.MachinePool{} - mp.Spec.Template.Spec.Version = &m.Version - list.(*expv1.MachinePoolList).Items = []expv1.MachinePool{*mp} return nil } @@ -355,29 +397,69 @@ func TestAzureMachinePool_Default(t *testing.T) { publicKeyNotExistTest := test{amp: createMachinePoolWithSSHPublicKey("")} existingRoleAssignmentName := "42862306-e485-4319-9bf0-35dbc6f6fe9c" - roleAssignmentExistTest := test{amp: &AzureMachinePool{Spec: AzureMachinePoolSpec{ - Identity: "SystemAssigned", - RoleAssignmentName: existingRoleAssignmentName, - }}} - - roleAssignmentEmptyTest := test{amp: &AzureMachinePool{Spec: AzureMachinePoolSpec{ - Identity: "SystemAssigned", - RoleAssignmentName: "", - }}} - - roleAssignmentExistTest.amp.Default(nil) - g.Expect(roleAssignmentExistTest.amp.Spec.RoleAssignmentName).To(Equal(existingRoleAssignmentName)) - - roleAssignmentEmptyTest.amp.Default(nil) - g.Expect(roleAssignmentEmptyTest.amp.Spec.RoleAssignmentName).To(Not(BeEmpty())) - _, err := guuid.Parse(roleAssignmentEmptyTest.amp.Spec.RoleAssignmentName) - g.Expect(err).To(Not(HaveOccurred())) - publicKeyExistTest.amp.Default(nil) + fakeSubscriptionID := guuid.New().String() + fakeClusterName := "testcluster" + fakeMachinePoolName := "testmachinepool" + mockClient := mockDefaultClient{Name: fakeMachinePoolName, ClusterName: fakeClusterName, SubscriptionID: fakeSubscriptionID} + + roleAssignmentExistTest := test{amp: &AzureMachinePool{ + Spec: AzureMachinePoolSpec{ + Identity: "SystemAssigned", + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: existingRoleAssignmentName, + Scope: "scope", + DefinitionID: "definitionID", + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fakeMachinePoolName, + }, + }} + + emptyTest := test{amp: &AzureMachinePool{ + Spec: AzureMachinePoolSpec{ + Identity: "SystemAssigned", + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{}, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fakeMachinePoolName, + }, + }} + + systemAssignedIdentityRoleExistTest := test{amp: &AzureMachinePool{ + Spec: AzureMachinePoolSpec{ + Identity: "SystemAssigned", + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + DefinitionID: "testroledefinitionid", + Scope: "testscope", + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fakeMachinePoolName, + }, + }} + + roleAssignmentExistTest.amp.Default(mockClient) + g.Expect(roleAssignmentExistTest.amp.Spec.SystemAssignedIdentityRole.Name).To(Equal(existingRoleAssignmentName)) + + publicKeyExistTest.amp.Default(mockClient) g.Expect(publicKeyExistTest.amp.Spec.Template.SSHPublicKey).To(Equal(existingPublicKey)) - publicKeyNotExistTest.amp.Default(nil) + publicKeyNotExistTest.amp.Default(mockClient) g.Expect(publicKeyNotExistTest.amp.Spec.Template.SSHPublicKey).NotTo(BeEmpty()) + + systemAssignedIdentityRoleExistTest.amp.Default(mockClient) + g.Expect(systemAssignedIdentityRoleExistTest.amp.Spec.SystemAssignedIdentityRole.DefinitionID).To(Equal("testroledefinitionid")) + g.Expect(systemAssignedIdentityRoleExistTest.amp.Spec.SystemAssignedIdentityRole.Scope).To(Equal("testscope")) + + emptyTest.amp.Default(mockClient) + g.Expect(emptyTest.amp.Spec.SystemAssignedIdentityRole.Name).To(Not(BeEmpty())) + _, err := guuid.Parse(emptyTest.amp.Spec.SystemAssignedIdentityRole.Name) + g.Expect(err).To(Not(HaveOccurred())) + g.Expect(emptyTest.amp.Spec.SystemAssignedIdentityRole).To(Not(BeNil())) + g.Expect(emptyTest.amp.Spec.SystemAssignedIdentityRole.Scope).To(Equal(fmt.Sprintf("/subscriptions/%s/", fakeSubscriptionID))) + g.Expect(emptyTest.amp.Spec.SystemAssignedIdentityRole.DefinitionID).To(Equal(fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", fakeSubscriptionID, infrav1.ContributorRoleID))) } func createMachinePoolWithMarketPlaceImage(publisher, offer, sku, version string, terminateNotificationTimeout *int) *AzureMachinePool { @@ -455,8 +537,12 @@ func createMachinePoolWithImageByID(imageID string, terminateNotificationTimeout func createMachinePoolWithSystemAssignedIdentity(role string) *AzureMachinePool { return &AzureMachinePool{ Spec: AzureMachinePoolSpec{ - Identity: infrav1.VMIdentitySystemAssigned, - RoleAssignmentName: role, + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: role, + Scope: "scope", + DefinitionID: "definitionID", + }, }, } } @@ -573,8 +659,12 @@ func getKnownValidAzureMachinePool() *AzureMachinePool { SSHPublicKey: validSSHPublicKey, TerminateNotificationTimeout: pointer.Int(10), }, - Identity: infrav1.VMIdentitySystemAssigned, - RoleAssignmentName: string(uuid.NewUUID()), + Identity: infrav1.VMIdentitySystemAssigned, + SystemAssignedIdentityRole: &infrav1.SystemAssignedIdentityRole{ + Name: string(uuid.NewUUID()), + Scope: "scope", + DefinitionID: "definitionID", + }, Strategy: AzureMachinePoolDeploymentStrategy{ Type: RollingUpdateAzureMachinePoolDeploymentStrategyType, RollingUpdate: &MachineRollingUpdateDeployment{ diff --git a/exp/api/v1beta1/zz_generated.deepcopy.go b/exp/api/v1beta1/zz_generated.deepcopy.go index d5e5f243093..0e53ab98298 100644 --- a/exp/api/v1beta1/zz_generated.deepcopy.go +++ b/exp/api/v1beta1/zz_generated.deepcopy.go @@ -334,6 +334,11 @@ func (in *AzureMachinePoolSpec) DeepCopyInto(out *AzureMachinePoolSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.SystemAssignedIdentityRole != nil { + in, out := &in.SystemAssignedIdentityRole, &out.SystemAssignedIdentityRole + *out = new(apiv1beta1.SystemAssignedIdentityRole) + **out = **in + } if in.UserAssignedIdentities != nil { in, out := &in.UserAssignedIdentities, &out.UserAssignedIdentities *out = make([]apiv1beta1.UserAssignedIdentity, len(*in)) diff --git a/main.go b/main.go index 7f5844f45bc..eaced819303 100644 --- a/main.go +++ b/main.go @@ -488,11 +488,6 @@ func registerWebhooks(mgr manager.Manager) { os.Exit(1) } - if err := (&infrav1.AzureMachine{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "AzureMachine") - os.Exit(1) - } - if err := (&infrav1.AzureMachineTemplate{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "AzureMachineTemplate") os.Exit(1) @@ -522,6 +517,12 @@ func registerWebhooks(mgr manager.Manager) { hookServer.Register("/validate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachinepool", webhookutils.NewValidatingWebhook( &infrav1exp.AzureMachinePool{}, mgr.GetClient(), )) + hookServer.Register("/mutate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachine", webhookutils.NewMutatingWebhook( + &infrav1.AzureMachine{}, mgr.GetClient(), + )) + hookServer.Register("/validate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachine", webhookutils.NewValidatingWebhook( + &infrav1.AzureMachine{}, mgr.GetClient(), + )) hookServer.Register("/mutate-infrastructure-cluster-x-k8s-io-v1beta1-azuremanagedmachinepool", webhookutils.NewMutatingWebhook( &infrav1.AzureManagedMachinePool{}, mgr.GetClient(), ))