Delete role assignments when deleting a VM (#2934)

.gitignore

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

cmd/scale.go

@@ -277,7 +277,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

@@ -32,6 +32,8 @@ type MockACSEngineClient struct {
 	FailDeleteNetworkInterface      bool
 	FailGetKubernetesClient         bool
 	FailListProviders               bool
+	ShouldSupportVMIdentity         bool
+	FailDeleteRoleAssignment        bool
 	MockKubernetesClient            *MockKubernetesClient
 }
 
@@ -274,16 +276,24 @@ 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,
 		resourceNameSuffixString: &resourceNameSuffix,
 		poolnameString:           &poolname,
 	}
 
+	var vmIdentity *compute.VirtualMachineIdentity
+	if mc.ShouldSupportVMIdentity {
+		vmIdentity = &compute.VirtualMachineIdentity{PrincipalID: &principalID}
+	}
+
 	return compute.VirtualMachine{
-		Name: &vm1Name,
-		Tags: &tags,
+		Name:     &vm1Name,
+		Tags:     &tags,
+		Identity: vmIdentity,
 		VirtualMachineProperties: &compute.VirtualMachineProperties{
 			StorageProfile: &compute.StorageProfile{
 				OsDisk: &compute.OSDisk{
@@ -462,3 +472,27 @@ 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) {
+	if mc.FailDeleteRoleAssignment {
+		return authorization.RoleAssignment{}, fmt.Errorf("DeleteRoleAssignmentByID failed")
+	}
+
+	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{}
+
+	if mc.ShouldSupportVMIdentity {
+		var assignmentID = "role-assignment-id"
+		var assignment = authorization.RoleAssignment{
+			ID: &assignmentID}
+		roleAssignments = append(roleAssignments, assignment)
+	}
+
+	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,27 @@ func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry,
 		}
 	}
 
+	if vm.Identity != nil {
+		// 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

@@ -29,6 +29,7 @@ type UpgradeAgentNode struct {
 	TemplateMap             map[string]interface{}
 	ParametersMap           map[string]interface{}
 	UpgradeContainerService *api.ContainerService
+	SubscriptionID          string
 	ResourceGroup           string
 	Client                  armhelpers.ACSEngineClient
 	kubeConfig              string
@@ -61,7 +62,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/upgradecluster_test.go

@@ -164,6 +164,47 @@ var _ = Describe("Upgrade Kubernetes cluster tests", func() {
 		Expect(err).NotTo(BeNil())
 		Expect(err.Error()).To(Equal("Error while querying ARM for resources: Kubernetes:1.6.8 cannot be upgraded to 1.8.6"))
 	})
+
+	It("Should return error message when failing to delete role assignment during upgrade operation", func() {
+		cs := createContainerService("testcluster", "1.6.9", 3, 2)
+		cs.Properties.OrchestratorProfile.OrchestratorVersion = "1.7.14"
+		cs.Properties.OrchestratorProfile.KubernetesConfig = &api.KubernetesConfig{}
+		cs.Properties.OrchestratorProfile.KubernetesConfig.UseManagedIdentity = true
+		uc := UpgradeCluster{
+			Translator: &i18n.Translator{},
+			Logger:     log.NewEntry(log.New()),
+		}
+
+		mockClient := armhelpers.MockACSEngineClient{}
+		mockClient.FailDeleteRoleAssignment = true
+		mockClient.ShouldSupportVMIdentity = true
+		uc.Client = &mockClient
+
+		subID, _ := uuid.FromString("DEC923E3-1EF1-4745-9516-37906D56DEC4")
+
+		err := uc.UpgradeCluster(subID, "kubeConfig", "TestRg", cs, "12345678", []string{"agentpool1"}, TestACSEngineVersion)
+		Expect(err).NotTo(BeNil())
+		Expect(err.Error()).To(Equal("DeleteRoleAssignmentByID failed"))
+	})
+
+	It("Should not fail if no managed identity is returned by azure during upgrade operation", func() {
+		cs := createContainerService("testcluster", "1.6.9", 3, 2)
+		cs.Properties.OrchestratorProfile.OrchestratorVersion = "1.7.14"
+		cs.Properties.OrchestratorProfile.KubernetesConfig = &api.KubernetesConfig{}
+		cs.Properties.OrchestratorProfile.KubernetesConfig.UseManagedIdentity = true
+		uc := UpgradeCluster{
+			Translator: &i18n.Translator{},
+			Logger:     log.NewEntry(log.New()),
+		}
+
+		mockClient := armhelpers.MockACSEngineClient{}
+		uc.Client = &mockClient
+
+		subID, _ := uuid.FromString("DEC923E3-1EF1-4745-9516-37906D56DEC4")
+
+		err := uc.UpgradeCluster(subID, "kubeConfig", "TestRg", cs, "12345678", []string{"agentpool1"}, TestACSEngineVersion)
+		Expect(err).To(BeNil())
+	})
 })
 
 func createContainerService(containerServiceName string, orchestratorVersion string, masterCount int, agentCount int) *api.ContainerService {

pkg/operations/kubernetesupgrade/upgrademasternode.go

@@ -22,6 +22,7 @@ type UpgradeMasterNode struct {
 	TemplateMap             map[string]interface{}
 	ParametersMap           map[string]interface{}
 	UpgradeContainerService *api.ContainerService
+	SubscriptionID          string
 	ResourceGroup           string
 	Client                  armhelpers.ACSEngineClient
 	kubeConfig              string
@@ -33,7 +34,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

@@ -93,6 +93,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 {
@@ -239,6 +240,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())
 	})
 })