Merge pull request #16567 from hakman/azure_storage_account

azure: Limit VMSS scope to specific storage account

go.mod

@@ -10,6 +10,7 @@ require (
 	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute v1.0.0
 	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork v1.1.0
 	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0
+	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage v1.5.0
 	github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v1.3.2
 	github.com/MakeNowJust/heredoc/v2 v2.0.1
 	github.com/Masterminds/sprig/v3 v3.2.3

k8s/crds/kops.k8s.io_clusters.yaml

@@ -410,6 +410,10 @@ spec:
                         description: RouteTableName is the name of the route table
                           attached to the subnet that the cluster is deployed in.
                         type: string
+                      storageAccountID:
+                        description: StorageAccountID specifies the storage account
+                          used for the cluster installation.
+                        type: string
                       subscriptionId:
                         description: SubscriptionID specifies the subscription used
                           for the cluster installation.

pkg/apis/kops/componentconfig.go

@@ -886,6 +886,8 @@ type OpenstackSpec struct {
 type AzureSpec struct {
 	// SubscriptionID specifies the subscription used for the cluster installation.
 	SubscriptionID string `json:"subscriptionID,omitempty"`
+	// StorageAccountID specifies the storage account used for the cluster installation.
+	StorageAccountID string `json:"storageAccountID,omitempty"`
 	// TenantID is the ID of the tenant that the cluster is deployed in.
 	TenantID string `json:"tenantID"`
 	// ResourceGroupName specifies the name of the resource group

pkg/apis/kops/v1alpha2/componentconfig.go

@@ -892,6 +892,8 @@ type OpenstackSpec struct {
 type AzureSpec struct {
 	// SubscriptionID specifies the subscription used for the cluster installation.
 	SubscriptionID string `json:"subscriptionId,omitempty"`
+	// StorageAccountID specifies the storage account used for the cluster installation.
+	StorageAccountID string `json:"storageAccountID,omitempty"`
 	// TenantID is the ID of the tenant that the cluster is deployed in.
 	TenantID string `json:"tenantId"`
 	// ResourceGroupName specifies the name of the resource group

pkg/apis/kops/v1alpha3/componentconfig.go

@@ -883,6 +883,8 @@ type OpenstackSpec struct {
 type AzureSpec struct {
 	// SubscriptionID specifies the subscription used for the cluster installation.
 	SubscriptionID string `json:"subscriptionID,omitempty"`
+	// StorageAccountID specifies the storage account used for the cluster installation.
+	StorageAccountID string `json:"storageAccountID,omitempty"`
 	// TenantID is the ID of the tenant that the cluster is deployed in.
 	TenantID string `json:"tenantID"`
 	// ResourceGroupName specifies the name of the resource group

pkg/model/azuremodel/vmscaleset.go

@@ -65,16 +65,26 @@ func (b *VMScaleSetModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 		if ig.IsControlPlane() || b.Cluster.UsesLegacyGossip() {
 			// Create tasks for assigning built-in roles to VM Scale Sets.
 			// See https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles
-			// for the ID definitions.
-			roleDefIDs := map[string]string{
+			resourceGroupID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s",
+				b.Cluster.Spec.CloudProvider.Azure.SubscriptionID,
+				b.Cluster.Spec.CloudProvider.Azure.ResourceGroupName,
+			)
+			c.AddTask(&azuretasks.RoleAssignment{
+				Name:       to.Ptr(fmt.Sprintf("%s-%s", *vmss.Name, "owner")),
+				Lifecycle:  b.Lifecycle,
+				Scope:      to.Ptr(resourceGroupID),
+				VMScaleSet: vmss,
 				// Owner
-				"owner": "8e3af657-a8ff-443c-a75c-2fe8c4bcb635",
+				RoleDefID: to.Ptr("8e3af657-a8ff-443c-a75c-2fe8c4bcb635"),
+			})
+			c.AddTask(&azuretasks.RoleAssignment{
+				Name:       to.Ptr(fmt.Sprintf("%s-%s", *vmss.Name, "blob")),
+				Lifecycle:  b.Lifecycle,
+				Scope:      to.Ptr(b.Cluster.Spec.CloudProvider.Azure.StorageAccountID),
+				VMScaleSet: vmss,
 				// Storage Blob Data Contributor
-				"blob": "ba92f5b4-2d11-453d-a403-e96b0029c9fe",
-			}
-			for k, roleDefID := range roleDefIDs {
-				c.AddTask(b.buildRoleAssignmentTask(vmss, k, roleDefID))
-			}
+				RoleDefID: to.Ptr("ba92f5b4-2d11-453d-a403-e96b0029c9fe"),
+			})
 		}
 	}
 
@@ -247,14 +257,3 @@ func parseImage(image string) (*compute.ImageReference, error) {
 		Version:   to.Ptr(l[3]),
 	}, nil
 }
-
-func (b *VMScaleSetModelBuilder) buildRoleAssignmentTask(vmss *azuretasks.VMScaleSet, roleKey, roleDefID string) *azuretasks.RoleAssignment {
-	name := fmt.Sprintf("%s-%s", *vmss.Name, roleKey)
-	return &azuretasks.RoleAssignment{
-		Name:          to.Ptr(name),
-		Lifecycle:     b.Lifecycle,
-		ResourceGroup: b.LinkToResourceGroup(),
-		VMScaleSet:    vmss,
-		RoleDefID:     to.Ptr(roleDefID),
-	}
-}

upup/pkg/fi/cloudup/azure/azure_cloud.go

@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
+	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage"
 	"k8s.io/klog/v2"
 	"k8s.io/kops/dnsprovider/pkg/dnsprovider"
 	"k8s.io/kops/pkg/apis/kops"
@@ -45,6 +46,7 @@ type AzureCloud interface {
 	fi.Cloud
 	AddClusterTags(tags map[string]*string)
 	FindVNetInfo(id, resourceGroup string) (*fi.VPCInfo, error)
+	FindStorageAccountInfo(name string) (*armstorage.Account, error)
 	SubscriptionID() string
 	ResourceGroup() ResourceGroupsClient
 	VirtualNetwork() VirtualNetworksClient
@@ -81,6 +83,7 @@ type azureCloudImplementation struct {
 	loadBalancersClient             LoadBalancersClient
 	publicIPAddressesClient         PublicIPAddressesClient
 	natGatewaysClient               NatGatewaysClient
+	storageAccountsClient           StorageAccountsClient
 }
 
 var _ fi.Cloud = &azureCloudImplementation{}
@@ -141,6 +144,9 @@ func NewAzureCloud(subscriptionID, resourceGroupName, location string, tags map[
 	if azureCloudImpl.natGatewaysClient, err = newNatGatewaysClientImpl(subscriptionID, cred); err != nil {
 		return nil, err
 	}
+	if azureCloudImpl.storageAccountsClient, err = newStorageAccountsClientImpl(subscriptionID, cred); err != nil {
+		return nil, err
+	}
 
 	return azureCloudImpl, nil
 }
@@ -158,7 +164,7 @@ func (c *azureCloudImplementation) DNS() (dnsprovider.Interface, error) {
 }
 
 func (c *azureCloudImplementation) FindVPCInfo(id string) (*fi.VPCInfo, error) {
-	return nil, errors.New("FindVPCInfo not implemented on azureCloud, use FindVNETInfo instead")
+	return nil, errors.New("FindVPCInfo not implemented on azureCloud, use FindVNetInfo instead")
 }
 
 func (c *azureCloudImplementation) FindVNetInfo(id, resourceGroup string) (*fi.VPCInfo, error) {
@@ -194,6 +200,19 @@ func (c *azureCloudImplementation) FindVNetInfo(id, resourceGroup string) (*fi.V
 	return nil, nil
 }
 
+func (c *azureCloudImplementation) FindStorageAccountInfo(name string) (*armstorage.Account, error) {
+	sas, err := c.storageAccountsClient.List(context.TODO())
+	if err != nil {
+		return nil, err
+	}
+	for _, sa := range sas {
+		if *sa.Name == name {
+			return sa, nil
+		}
+	}
+	return nil, nil
+}
+
 func (c *azureCloudImplementation) DeleteInstance(i *cloudinstances.CloudInstance) error {
 	vmssName := i.CloudInstanceGroup.HumanName
 	instanceID := strings.TrimPrefix(i.ID, vmssName+"_")

upup/pkg/fi/cloudup/azure/roleassignment.go

@@ -27,7 +27,7 @@ import (
 // RoleAssignmentsClient is a client for managing role assignments
 type RoleAssignmentsClient interface {
 	Create(ctx context.Context, scope, roleAssignmentName string, parameters authz.RoleAssignmentCreateParameters) (*authz.RoleAssignment, error)
-	List(ctx context.Context, resourceGroupName string) ([]*authz.RoleAssignment, error)
+	List(ctx context.Context, scope string) ([]*authz.RoleAssignment, error)
 	Delete(ctx context.Context, scope, raName string) error
 }
 
@@ -47,9 +47,9 @@ func (c *roleAssignmentsClientImpl) Create(
 	return &resp.RoleAssignment, err
 }
 
-func (c *roleAssignmentsClientImpl) List(ctx context.Context, resourceGroupName string) ([]*authz.RoleAssignment, error) {
+func (c *roleAssignmentsClientImpl) List(ctx context.Context, scope string) ([]*authz.RoleAssignment, error) {
 	var l []*authz.RoleAssignment
-	pager := c.c.NewListForResourceGroupPager(resourceGroupName, nil)
+	pager := c.c.NewListForScopePager(scope, nil)
 	for pager.More() {
 		resp, err := pager.NextPage(ctx)
 		if err != nil {

upup/pkg/fi/cloudup/azure/storageaccount.go

@@ -0,0 +1,59 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package azure
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
+	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage"
+)
+
+// StorageAccountsClient is a client for managing Network Interfaces.
+type StorageAccountsClient interface {
+	List(ctx context.Context) ([]*armstorage.Account, error)
+}
+
+type storageAccountsClientImpl struct {
+	c *armstorage.AccountsClient
+}
+
+var _ StorageAccountsClient = &storageAccountsClientImpl{}
+
+func (c *storageAccountsClientImpl) List(ctx context.Context) ([]*armstorage.Account, error) {
+	var l []*armstorage.Account
+	pager := c.c.NewListPager(nil)
+	for pager.More() {
+		resp, err := pager.NextPage(ctx)
+		if err != nil {
+			return nil, fmt.Errorf("listing storage accounts: %w", err)
+		}
+		l = append(l, resp.Value...)
+	}
+	return l, nil
+}
+
+func newStorageAccountsClientImpl(subscriptionID string, cred *azidentity.DefaultAzureCredential) (*storageAccountsClientImpl, error) {
+	c, err := armstorage.NewAccountsClient(subscriptionID, cred, nil)
+	if err != nil {
+		return nil, fmt.Errorf("creating storage accounts client: %w", err)
+	}
+	return &storageAccountsClientImpl{
+		c: c,
+	}, nil
+}

upup/pkg/fi/cloudup/azuretasks/roleassignment.go

@@ -43,10 +43,10 @@ type RoleAssignment struct {
 	Name      *string
 	Lifecycle fi.Lifecycle
 
-	ResourceGroup *ResourceGroup
-	VMScaleSet    *VMScaleSet
-	ID            *string
-	RoleDefID     *string
+	Scope      *string
+	VMScaleSet *VMScaleSet
+	ID         *string
+	RoleDefID  *string
 }
 
 var (
@@ -69,7 +69,7 @@ func (r *RoleAssignment) Find(c *fi.CloudupContext) (*RoleAssignment, error) {
 	}
 
 	cloud := c.T.Cloud.(azure.AzureCloud)
-	rs, err := cloud.RoleAssignment().List(context.TODO(), *r.ResourceGroup.Name)
+	rs, err := cloud.RoleAssignment().List(context.TODO(), *r.Scope)
 	if err != nil {
 		return nil, err
 	}
@@ -95,7 +95,7 @@ func (r *RoleAssignment) Find(c *fi.CloudupContext) (*RoleAssignment, error) {
 	}
 
 	// Query VM Scale Sets and find one that has matching Principal ID.
-	vs, err := cloud.VMScaleSet().List(context.TODO(), *r.ResourceGroup.Name)
+	vs, err := cloud.VMScaleSet().List(context.TODO(), *r.VMScaleSet.ResourceGroup.Name)
 	if err != nil {
 		return nil, err
 	}
@@ -117,9 +117,7 @@ func (r *RoleAssignment) Find(c *fi.CloudupContext) (*RoleAssignment, error) {
 	return &RoleAssignment{
 		Name:      r.Name,
 		Lifecycle: r.Lifecycle,
-		ResourceGroup: &ResourceGroup{
-			Name: r.ResourceGroup.Name,
-		},
+		Scope:     found.Properties.Scope,
 		VMScaleSet: &VMScaleSet{
 			Name: foundVMSS.Name,
 		},
@@ -165,9 +163,7 @@ func createNewRoleAssignment(t *azure.AzureAPITarget, e *RoleAssignment) error {
 	// We generate the name of Role Assignment here. It must be a valid GUID.
 	roleAssignmentName := uuid.New().String()
 
-	// TODO(kenji): Append additinoal scope ("providers/Microsoft.Storage/storageAccounts/<account-name>") when the role is for blob access so that
-	// the role is scoped to a specific storage account.
-	scope := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", t.Cloud.SubscriptionID(), *e.ResourceGroup.Name)
+	scope := *e.Scope
 	roleDefID := fmt.Sprintf("%s/providers/Microsoft.Authorization/roleDefinitions/%s", scope, *e.RoleDefID)
 	roleAssignment := authz.RoleAssignmentCreateParameters{
 		Properties: &authz.RoleAssignmentProperties{

upup/pkg/fi/cloudup/azuretasks/roleassignment_test.go

@@ -34,10 +34,8 @@ func TestRoleAssignmentRenderAzure(t *testing.T) {
 	apiTarget := azure.NewAzureAPITarget(cloud)
 	ra := &RoleAssignment{}
 	expected := &RoleAssignment{
-		Name: to.Ptr("ra"),
-		ResourceGroup: &ResourceGroup{
-			Name: to.Ptr("rg"),
-		},
+		Name:  to.Ptr("ra"),
+		Scope: to.Ptr("scope"),
 		VMScaleSet: &VMScaleSet{
 			Name:        to.Ptr("vmss"),
 			PrincipalID: to.Ptr("pid"),
@@ -75,16 +73,18 @@ func TestRoleAssignmentFind(t *testing.T) {
 		t.Fatalf("failed to create: %s", err)
 	}
 	vmss := &VMScaleSet{
-		Name:        to.Ptr(vmssName),
-		PrincipalID: resp.Identity.PrincipalID,
+		Name:          to.Ptr(vmssName),
+		PrincipalID:   resp.Identity.PrincipalID,
+		ResourceGroup: rg,
 	}
 
+	scope := "scope"
 	roleDefID := "rdid0"
 	ra := &RoleAssignment{
-		Name:          vmss.Name,
-		ResourceGroup: rg,
-		VMScaleSet:    vmss,
-		RoleDefID:     &roleDefID,
+		Name:       vmss.Name,
+		Scope:      &scope,
+		VMScaleSet: vmss,
+		RoleDefID:  &roleDefID,
 	}
 	// Find will return nothing if there is no Role Assignment created.
 	actual, err := ra.Find(ctx)
@@ -97,7 +97,6 @@ func TestRoleAssignmentFind(t *testing.T) {
 
 	// Create Role Assignments. One of them has irrelevant (different role definition ID).
 	roleAssignmentName := uuid.New().String()
-	scope := "s"
 	roleAssignment := authz.RoleAssignmentCreateParameters{
 		Properties: &authz.RoleAssignmentProperties{
 			RoleDefinitionID: to.Ptr(roleDefID),
@@ -163,9 +162,10 @@ func TestRoleAssignmentFind_NoPrincipalID(t *testing.T) {
 		t.Fatalf("failed to create Role Assignment: %s", err)
 	}
 
+	scope := "scope"
 	ra := &RoleAssignment{
-		Name:          to.Ptr(vmssName),
-		ResourceGroup: rg,
+		Name:  to.Ptr(vmssName),
+		Scope: to.Ptr(scope),
 		VMScaleSet: &VMScaleSet{
 			Name: to.Ptr(vmssName),
 			// Do not set principal ID.