Merge pull request #2965 from willie-yao/identity-scope

Allow for configurable scope in system assigned identities

Author: k8s-ci-robot

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
 

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
 }
 

api/v1alpha3/zz_generated.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
 }
 

api/v1alpha4/azuremachine_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
 }
 

api/v1alpha4/azuremachinetemplate_conversion.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 a generated UUID.
+			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()
 }

api/v1alpha4/zz_generated.conversion.go

@@ -17,13 +17,17 @@ limitations under the License.
 package v1beta1
 
 import (
+	"context"
 	"encoding/json"
+	"fmt"
 	"reflect"
 	"testing"
 
 	"github.com/google/uuid"
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
 	"k8s.io/utils/pointer"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func TestAzureMachineSpec_SetDefaultSSHPublicKey(t *testing.T) {
@@ -53,29 +57,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) {
@@ -367,6 +392,65 @@ func TestAzureMachineSpec_SetNetworkInterfacesDefaults(t *testing.T) {
 	}
 }
 
+func TestAzureMachineSpec_GetSubscriptionID(t *testing.T) {
+	g := NewWithT(t)
+
+	tests := []struct {
+		name        string
+		maxAttempts int
+		want        string
+		wantErr     bool
+	}{
+		{
+			name:        "subscription ID is returned",
+			maxAttempts: 1,
+			want:        "test-subscription-id",
+			wantErr:     false,
+		},
+		{
+			name:        "subscription ID is returned after 2 attempts",
+			maxAttempts: 2,
+			want:        "test-subscription-id",
+			wantErr:     false,
+		},
+		{
+			name:        "subscription ID is not returned after 5 attempts",
+			maxAttempts: 5,
+			want:        "",
+			wantErr:     true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			client := mockClient{ReturnError: tc.wantErr}
+			result, err := GetSubscriptionID(client, "test-cluster", "default", tc.maxAttempts)
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(result).To(Equal(tc.want))
+			}
+		})
+	}
+}
+
+type mockClient struct {
+	client.Client
+	ReturnError bool
+}
+
+func (m mockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	if m.ReturnError {
+		return errors.New("AzureCluster not found: failed to find owner cluster for test-cluster")
+	}
+	ac := &AzureCluster{}
+	ac.Spec.SubscriptionID = "test-subscription-id"
+	obj.(*AzureCluster).Spec.SubscriptionID = ac.Spec.SubscriptionID
+
+	return nil
+}
+
 func createMachineWithSSHPublicKey(sshPublicKey string) *AzureMachine {
 	machine := hardcodedAzureMachineWithSSHKey(sshPublicKey)
 	return machine

api/v1beta1/azuremachine_default.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 UUID.
+	// If not specified, a random UUID 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.