Delete role assignments when deleting a VM

.gitignore

@@ -32,3 +32,4 @@ test/e2e/openshift/translations/
 # test outputs
 cmd/_test_output
 
+.idea

cmd/scale.go

@@ -267,7 +267,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error {
 				}
 			}
 
-			errList := operations.ScaleDownVMs(sc.client, sc.logger, sc.resourceGroupName, vmsToDelete...)
+			errList := operations.ScaleDownVMs(sc.client, sc.logger, sc.SubscriptionID.String(), sc.resourceGroupName, vmsToDelete...)
 			if errList != nil {
 				errorMessage := ""
 				for element := errList.Front(); element != nil; element = element.Next() {

pkg/armhelpers/graph.go

@@ -37,6 +37,17 @@ func (az *AzureClient) CreateRoleAssignment(scope string, roleAssignmentName str
 	return az.authorizationClient.Create(scope, roleAssignmentName, parameters)
 }
 
+// DeleteRoleAssignmentByID deletes a roleAssignment via its unique identifier
+func (az *AzureClient) DeleteRoleAssignmentByID(roleAssignmentID string) (authorization.RoleAssignment, error) {
+	return az.authorizationClient.DeleteByID(roleAssignmentID)
+}
+
+// ListRoleAssignmentsForPrincipal (e.g. a VM) via the scope and the unique identifier of the principal
+func (az *AzureClient) ListRoleAssignmentsForPrincipal(scope string, principalID string) (authorization.RoleAssignmentListResult, error) {
+	filter := fmt.Sprintf("principalId eq '%s'", principalID)
+	return az.authorizationClient.ListForScope(scope, filter)
+}
+
 // CreateApp is a simpler method for creating an application
 func (az *AzureClient) CreateApp(appName, appURL string, replyURLs *[]string, requiredResourceAccess *[]graphrbac.RequiredResourceAccess) (applicationID, servicePrincipalObjectID, servicePrincipalClientSecret string, err error) {
 	notBefore := time.Now()

pkg/armhelpers/interfaces.go

@@ -70,6 +70,8 @@ type ACSEngineClient interface {
 	// RBAC
 	CreateRoleAssignment(scope string, roleAssignmentName string, parameters authorization.RoleAssignmentCreateParameters) (authorization.RoleAssignment, error)
 	CreateRoleAssignmentSimple(applicationID, roleID string) error
+	DeleteRoleAssignmentByID(roleAssignmentNameID string) (authorization.RoleAssignment, error)
+	ListRoleAssignmentsForPrincipal(scope string, principalID string) (authorization.RoleAssignmentListResult, error)
 
 	// MANAGED DISKS
 	DeleteManagedDisk(resourceGroupName string, diskName string, cancel <-chan struct{}) (<-chan disk.OperationStatusResponse, <-chan error)

pkg/armhelpers/mockclients.go

@@ -274,6 +274,8 @@ func (mc *MockACSEngineClient) GetVirtualMachine(resourceGroup, name string) (co
 	resourceNameSuffix := "12345678"
 	poolname := "agentpool1"
 
+	principalID := "00000000-1111-2222-3333-444444444444"
+
 	tags := map[string]*string{
 		creationSourceString:     &creationSource,
 		orchestratorString:       &orchestrator,
@@ -284,6 +286,8 @@ func (mc *MockACSEngineClient) GetVirtualMachine(resourceGroup, name string) (co
 	return compute.VirtualMachine{
 		Name: &vm1Name,
 		Tags: &tags,
+		Identity: &compute.VirtualMachineIdentity{
+			PrincipalID: &principalID},
 		VirtualMachineProperties: &compute.VirtualMachineProperties{
 			StorageProfile: &compute.StorageProfile{
 				OsDisk: &compute.OSDisk{
@@ -462,3 +466,15 @@ func (mc *MockACSEngineClient) ListDeploymentOperations(resourceGroupName string
 func (mc *MockACSEngineClient) ListDeploymentOperationsNextResults(lastResults resources.DeploymentOperationsListResult) (result resources.DeploymentOperationsListResult, err error) {
 	return resources.DeploymentOperationsListResult{}, nil
 }
+
+// DeleteRoleAssignmentByID deletes a roleAssignment via its unique identifier
+func (mc *MockACSEngineClient) DeleteRoleAssignmentByID(roleAssignmentID string) (authorization.RoleAssignment, error) {
+	return authorization.RoleAssignment{}, nil
+}
+
+// ListRoleAssignmentsForPrincipal (e.g. a VM) via the scope and the unique identifier of the principal
+func (mc *MockACSEngineClient) ListRoleAssignmentsForPrincipal(scope string, principalID string) (authorization.RoleAssignmentListResult, error) {
+	roleAssignments := []authorization.RoleAssignment{}
+	return authorization.RoleAssignmentListResult{
+		Value: &roleAssignments}, nil
+}

pkg/operations/deletevm.go

@@ -8,8 +8,13 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
+const (
+	// AADRoleResourceGroupScopeTemplate is a template for a roleDefinition scope
+	AADRoleResourceGroupScopeTemplate = "/subscriptions/%s/resourceGroups/%s"
+)
+
 // CleanDeleteVirtualMachine deletes a VM and any associated OS disk
-func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry, resourceGroup, name string) error {
+func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry, subscriptionID, resourceGroup, name string) error {
 	logger.Infof("fetching VM: %s/%s", resourceGroup, name)
 	vm, err := az.GetVirtualMachine(resourceGroup, name)
 	if err != nil {
@@ -86,5 +91,25 @@ func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry,
 		}
 	}
 
+	// Role assignments are not deleted if the VM is destroyed, so we must cleanup ourselves!
+	// The role assignments should only be relevant if managed identities are used,
+	// but always cleaning them up is easier than adding rule based logic here and there.
+	scope := fmt.Sprintf(AADRoleResourceGroupScopeTemplate, subscriptionID, resourceGroup)
+	logger.Infof("fetching roleAssignments: %s with principal %s", scope, *vm.Identity.PrincipalID)
+	vmRoleAssignments, listRoleAssingmentsError := az.ListRoleAssignmentsForPrincipal(scope, *vm.Identity.PrincipalID)
+	if listRoleAssingmentsError != nil {
+		logger.Errorf("failed to list role assignments: %s/%s: %s", scope, *vm.Identity.PrincipalID, listRoleAssingmentsError.Error())
+		return listRoleAssingmentsError
+	}
+
+	for _, roleAssignment := range *vmRoleAssignments.Value {
+		logger.Infof("deleting role assignment: %s", *roleAssignment.ID)
+		_, deleteRoleAssignmentErr := az.DeleteRoleAssignmentByID(*roleAssignment.ID)
+		if deleteRoleAssignmentErr != nil {
+			logger.Errorf("failed to delete role assignment: %s: %s", *roleAssignment.ID, deleteRoleAssignmentErr.Error())
+			return deleteRoleAssignmentErr
+		}
+	}
+
 	return nil
 }

pkg/operations/kubernetesupgrade/upgradeagentnode.go

@@ -30,6 +30,7 @@ type UpgradeAgentNode struct {
 	TemplateMap             map[string]interface{}
 	ParametersMap           map[string]interface{}
 	UpgradeContainerService *api.ContainerService
+	SubscriptionID          string
 	ResourceGroup           string
 	Client                  armhelpers.ACSEngineClient
 	kubeConfig              string
@@ -62,7 +63,7 @@ func (kan *UpgradeAgentNode) DeleteNode(vmName *string, drain bool) error {
 		}
 	}
 	// Delete VM in ARM
-	if err := operations.CleanDeleteVirtualMachine(kan.Client, kan.logger, kan.ResourceGroup, *vmName); err != nil {
+	if err := operations.CleanDeleteVirtualMachine(kan.Client, kan.logger, kan.SubscriptionID, kan.ResourceGroup, *vmName); err != nil {
 		return err
 	}
 	// Delete VM in api server

pkg/operations/kubernetesupgrade/upgradecluster.go

@@ -19,10 +19,11 @@ import (
 // ClusterTopology contains resources of the cluster the upgrade operation
 // is targeting
 type ClusterTopology struct {
-	DataModel     *api.ContainerService
-	Location      string
-	ResourceGroup string
-	NameSuffix    string
+	DataModel      *api.ContainerService
+	SubscriptionID string
+	Location       string
+	ResourceGroup  string
+	NameSuffix     string
 
 	AgentPoolsToUpgrade map[string]bool
 	AgentPools          map[string]*AgentPoolTopology
@@ -59,6 +60,7 @@ const MasterPoolName = "master"
 func (uc *UpgradeCluster) UpgradeCluster(subscriptionID uuid.UUID, kubeConfig, resourceGroup string,
 	cs *api.ContainerService, nameSuffix string, agentPoolsToUpgrade []string, acsengineVersion string) error {
 	uc.ClusterTopology = ClusterTopology{}
+	uc.SubscriptionID = subscriptionID.String()
 	uc.ResourceGroup = resourceGroup
 	uc.DataModel = cs
 	uc.NameSuffix = nameSuffix

pkg/operations/kubernetesupgrade/upgrademasternode.go

@@ -24,6 +24,7 @@ type UpgradeMasterNode struct {
 	TemplateMap             map[string]interface{}
 	ParametersMap           map[string]interface{}
 	UpgradeContainerService *api.ContainerService
+	SubscriptionID          string
 	ResourceGroup           string
 	Client                  armhelpers.ACSEngineClient
 	kubeConfig              string
@@ -35,7 +36,7 @@ type UpgradeMasterNode struct {
 // the node.
 // The 'drain' flag is not used for deleting master nodes.
 func (kmn *UpgradeMasterNode) DeleteNode(vmName *string, drain bool) error {
-	return operations.CleanDeleteVirtualMachine(kmn.Client, kmn.logger, kmn.ResourceGroup, *vmName)
+	return operations.CleanDeleteVirtualMachine(kmn.Client, kmn.logger, kmn.SubscriptionID, kmn.ResourceGroup, *vmName)
 }
 
 // CreateNode creates a new master/agent node with the targeted version of Kubernetes

pkg/operations/kubernetesupgrade/upgrader.go

@@ -96,6 +96,7 @@ func (ku *Upgrader) upgradeMasterNodes() error {
 	upgradeMasterNode.ParametersMap = parametersMap
 	upgradeMasterNode.UpgradeContainerService = ku.ClusterTopology.DataModel
 	upgradeMasterNode.ResourceGroup = ku.ClusterTopology.ResourceGroup
+	upgradeMasterNode.SubscriptionID = ku.ClusterTopology.SubscriptionID
 	upgradeMasterNode.Client = ku.Client
 	upgradeMasterNode.kubeConfig = ku.kubeConfig
 	if ku.stepTimeout == nil {
@@ -242,6 +243,7 @@ func (ku *Upgrader) upgradeAgentPools() error {
 		upgradeAgentNode.TemplateMap = templateMap
 		upgradeAgentNode.ParametersMap = parametersMap
 		upgradeAgentNode.UpgradeContainerService = ku.ClusterTopology.DataModel
+		upgradeAgentNode.SubscriptionID = ku.ClusterTopology.SubscriptionID
 		upgradeAgentNode.ResourceGroup = ku.ClusterTopology.ResourceGroup
 		upgradeAgentNode.Client = ku.Client
 		upgradeAgentNode.kubeConfig = ku.kubeConfig

pkg/operations/scaledownagentpool.go

@@ -15,13 +15,13 @@ type VMScalingErrorDetails struct {
 
 // ScaleDownVMs removes the vms in the provided list. Returns a list with details on each failure.
 // all items in the list will always be of type *VMScalingErrorDetails
-func ScaleDownVMs(az armhelpers.ACSEngineClient, logger *log.Entry, resourceGroup string, vmNames ...string) *list.List {
+func ScaleDownVMs(az armhelpers.ACSEngineClient, logger *log.Entry, subscriptionID string, resourceGroup string, vmNames ...string) *list.List {
 	numVmsToDelete := len(vmNames)
 	errChan := make(chan *VMScalingErrorDetails, numVmsToDelete)
 	defer close(errChan)
 	for _, vmName := range vmNames {
 		go func(vmName string) {
-			err := CleanDeleteVirtualMachine(az, logger, resourceGroup, vmName)
+			err := CleanDeleteVirtualMachine(az, logger, subscriptionID, resourceGroup, vmName)
 			if err != nil {
 				errChan <- &VMScalingErrorDetails{Name: vmName, Error: err}
 				return

pkg/operations/scaledownagentpool_test.go

@@ -18,7 +18,7 @@ var _ = Describe("Scale down vms operation tests", func() {
 	It("Should return error messages for failing vms", func() {
 		mockClient := armhelpers.MockACSEngineClient{}
 		mockClient.FailGetVirtualMachine = true
-		errs := ScaleDownVMs(&mockClient, log.NewEntry(log.New()), "rg", "vm1", "vm2", "vm3", "vm5")
+		errs := ScaleDownVMs(&mockClient, log.NewEntry(log.New()), "sid", "rg", "vm1", "vm2", "vm3", "vm5")
 		Expect(errs.Len()).To(Equal(4))
 		for e := errs.Front(); e != nil; e = e.Next() {
 			output := e.Value.(*VMScalingErrorDetails)
@@ -28,7 +28,7 @@ var _ = Describe("Scale down vms operation tests", func() {
 	})
 	It("Should return nil for errors if all deletes successful", func() {
 		mockClient := armhelpers.MockACSEngineClient{}
-		errs := ScaleDownVMs(&mockClient, log.NewEntry(log.New()), "rg", "k8s-agent-F8EADCCF-0", "k8s-agent-F8EADCCF-3", "k8s-agent-F8EADCCF-2", "k8s-agent-F8EADCCF-4")
+		errs := ScaleDownVMs(&mockClient, log.NewEntry(log.New()), "sid", "rg", "k8s-agent-F8EADCCF-0", "k8s-agent-F8EADCCF-3", "k8s-agent-F8EADCCF-2", "k8s-agent-F8EADCCF-4")
 		Expect(errs).To(BeNil())
 	})
 })