From 055f2b568bbff2a0821626319eef27c6a3b88d18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cedric=20R=C3=B6ck?= Date: Thu, 24 May 2018 01:42:03 +0200 Subject: [PATCH] Delete role assignments when deleting a VM (#2934) --- .gitignore | 1 + cmd/scale.go | 2 +- pkg/armhelpers/graph.go | 11 +++++ pkg/armhelpers/interfaces.go | 2 + pkg/armhelpers/mockclients.go | 38 ++++++++++++++++- pkg/operations/deletevm.go | 29 ++++++++++++- .../kubernetesupgrade/upgradeagentnode.go | 3 +- .../kubernetesupgrade/upgradecluster.go | 10 +++-- .../kubernetesupgrade/upgradecluster_test.go | 41 +++++++++++++++++++ .../kubernetesupgrade/upgrademasternode.go | 3 +- pkg/operations/kubernetesupgrade/upgrader.go | 2 + pkg/operations/scaledownagentpool.go | 4 +- pkg/operations/scaledownagentpool_test.go | 4 +- 13 files changed, 136 insertions(+), 14 deletions(-) diff --git a/.gitignore b/.gitignore index bdffcb61b6..c6d69b2645 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,4 @@ test/e2e/openshift/translations/ # test outputs cmd/_test_output +.idea diff --git a/cmd/scale.go b/cmd/scale.go index c10a89f8fd..f7247ae4be 100644 --- a/cmd/scale.go +++ b/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() { diff --git a/pkg/armhelpers/graph.go b/pkg/armhelpers/graph.go index 3c87299126..158668718a 100644 --- a/pkg/armhelpers/graph.go +++ b/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() diff --git a/pkg/armhelpers/interfaces.go b/pkg/armhelpers/interfaces.go index 622c630b5c..0f6e812914 100644 --- a/pkg/armhelpers/interfaces.go +++ b/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) diff --git a/pkg/armhelpers/mockclients.go b/pkg/armhelpers/mockclients.go index d9f23255ca..ac9c9253a4 100644 --- a/pkg/armhelpers/mockclients.go +++ b/pkg/armhelpers/mockclients.go @@ -32,6 +32,8 @@ type MockACSEngineClient struct { FailDeleteNetworkInterface bool FailGetKubernetesClient bool FailListProviders bool + ShouldSupportVMIdentity bool + FailDeleteRoleAssignment bool MockKubernetesClient *MockKubernetesClient } @@ -274,6 +276,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, @@ -281,9 +285,15 @@ func (mc *MockACSEngineClient) GetVirtualMachine(resourceGroup, name string) (co 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 +} diff --git a/pkg/operations/deletevm.go b/pkg/operations/deletevm.go index b5b593ca26..e7be1cfa14 100644 --- a/pkg/operations/deletevm.go +++ b/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 } diff --git a/pkg/operations/kubernetesupgrade/upgradeagentnode.go b/pkg/operations/kubernetesupgrade/upgradeagentnode.go index e24695fcf9..000b4c66a3 100644 --- a/pkg/operations/kubernetesupgrade/upgradeagentnode.go +++ b/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 diff --git a/pkg/operations/kubernetesupgrade/upgradecluster.go b/pkg/operations/kubernetesupgrade/upgradecluster.go index 26f11fa31d..94e64ed330 100644 --- a/pkg/operations/kubernetesupgrade/upgradecluster.go +++ b/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 diff --git a/pkg/operations/kubernetesupgrade/upgradecluster_test.go b/pkg/operations/kubernetesupgrade/upgradecluster_test.go index 793d49f9a7..cbc75466b1 100644 --- a/pkg/operations/kubernetesupgrade/upgradecluster_test.go +++ b/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 { diff --git a/pkg/operations/kubernetesupgrade/upgrademasternode.go b/pkg/operations/kubernetesupgrade/upgrademasternode.go index e2c41a15ba..10209f7127 100644 --- a/pkg/operations/kubernetesupgrade/upgrademasternode.go +++ b/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 diff --git a/pkg/operations/kubernetesupgrade/upgrader.go b/pkg/operations/kubernetesupgrade/upgrader.go index 6e85be80e7..8cb4c0b0e9 100644 --- a/pkg/operations/kubernetesupgrade/upgrader.go +++ b/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 diff --git a/pkg/operations/scaledownagentpool.go b/pkg/operations/scaledownagentpool.go index a9b2424f01..83f892d3bc 100644 --- a/pkg/operations/scaledownagentpool.go +++ b/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 diff --git a/pkg/operations/scaledownagentpool_test.go b/pkg/operations/scaledownagentpool_test.go index 3b1052b92c..5aa92be2b0 100644 --- a/pkg/operations/scaledownagentpool_test.go +++ b/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()) }) })