From b9fae864e2eab60ad02295789d917e186d2f6272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sun, 13 May 2018 16:31:31 +0200 Subject: [PATCH 1/8] Delete vm related role assignments when deleting a VM --- cmd/scale.go | 2 +- pkg/armhelpers/graph.go | 9 +++++++ pkg/armhelpers/interfaces.go | 2 ++ pkg/operations/deletevm.go | 27 ++++++++++++++++++- .../kubernetesupgrade/upgradeagentnode.go | 3 ++- .../kubernetesupgrade/upgradecluster.go | 10 ++++--- .../kubernetesupgrade/upgrademasternode.go | 3 ++- pkg/operations/kubernetesupgrade/upgrader.go | 2 ++ pkg/operations/scaledownagentpool.go | 4 +-- 9 files changed, 52 insertions(+), 10 deletions(-) diff --git a/cmd/scale.go b/cmd/scale.go index 5c5adbb319..0fb903d036 100644 --- a/cmd/scale.go +++ b/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() { diff --git a/pkg/armhelpers/graph.go b/pkg/armhelpers/graph.go index 3c87299126..053c2a282e 100644 --- a/pkg/armhelpers/graph.go +++ b/pkg/armhelpers/graph.go @@ -37,6 +37,15 @@ func (az *AzureClient) CreateRoleAssignment(scope string, roleAssignmentName str return az.authorizationClient.Create(scope, roleAssignmentName, parameters) } +func (az *AzureClient) DeleteRoleAssignmentByID(roleAssignmentID string) (authorization.RoleAssignment, error) { + return az.authorizationClient.DeleteByID(roleAssignmentID) +} + +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 45c70198a7..dfaad71850 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/operations/deletevm.go b/pkg/operations/deletevm.go index b5b593ca26..d38282fda0 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,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", *roleAssignment.ID, deleteRoleAssignmentErr.Error()) + return deleteRoleAssignmentErr + } + } + return nil } diff --git a/pkg/operations/kubernetesupgrade/upgradeagentnode.go b/pkg/operations/kubernetesupgrade/upgradeagentnode.go index 2f822613b9..dafe3aa8c5 100644 --- a/pkg/operations/kubernetesupgrade/upgradeagentnode.go +++ b/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 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/upgrademasternode.go b/pkg/operations/kubernetesupgrade/upgrademasternode.go index 8b883a7f3c..f9e97e9a94 100644 --- a/pkg/operations/kubernetesupgrade/upgrademasternode.go +++ b/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 diff --git a/pkg/operations/kubernetesupgrade/upgrader.go b/pkg/operations/kubernetesupgrade/upgrader.go index 547834513f..1c75f91b5f 100644 --- a/pkg/operations/kubernetesupgrade/upgrader.go +++ b/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 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 From 9d367623eb580d562f0d4b4d0dc1b7269d09cfb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sun, 13 May 2018 16:32:29 +0200 Subject: [PATCH 2/8] Ignore .idea generated files --- .gitignore | 1 + 1 file changed, 1 insertion(+) 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 From 1e78d43199f3b7454000d0a4cc7532aa57066462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sun, 13 May 2018 16:32:12 +0200 Subject: [PATCH 3/8] Adapted tests to work with the new method argument and interface functions --- pkg/armhelpers/mockclients.go | 14 ++++++++++++++ pkg/operations/scaledownagentpool_test.go | 4 ++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/pkg/armhelpers/mockclients.go b/pkg/armhelpers/mockclients.go index ca0ecc213d..3184ac1394 100644 --- a/pkg/armhelpers/mockclients.go +++ b/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,13 @@ func (mc *MockACSEngineClient) ListDeploymentOperations(resourceGroupName string func (mc *MockACSEngineClient) ListDeploymentOperationsNextResults(lastResults resources.DeploymentOperationsListResult) (result resources.DeploymentOperationsListResult, err error) { return resources.DeploymentOperationsListResult{}, nil } + +func (mc *MockACSEngineClient) DeleteRoleAssignmentByID(roleAssignmentID string) (authorization.RoleAssignment, error) { + return authorization.RoleAssignment{}, nil +} + +func (mc *MockACSEngineClient) ListRoleAssignmentsForPrincipal(scope string, principalID string) (authorization.RoleAssignmentListResult, error) { + roleAssignments := []authorization.RoleAssignment{} + return authorization.RoleAssignmentListResult{ + Value: &roleAssignments}, nil +} diff --git a/pkg/operations/scaledownagentpool_test.go b/pkg/operations/scaledownagentpool_test.go index 3b1052b92c..4f3384ff43 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()) }) }) From 463e2039b08d71507252310f1f842eddae92d1ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sun, 13 May 2018 17:35:14 +0200 Subject: [PATCH 4/8] circle-ci feedback / gofmt --- pkg/operations/deletevm.go | 2 +- pkg/operations/scaledownagentpool_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/operations/deletevm.go b/pkg/operations/deletevm.go index d38282fda0..c6f6564b39 100644 --- a/pkg/operations/deletevm.go +++ b/pkg/operations/deletevm.go @@ -106,7 +106,7 @@ func CleanDeleteVirtualMachine(az armhelpers.ACSEngineClient, logger *log.Entry, logger.Infof("deleting role assignment: %s", *roleAssignment.ID) _, deleteRoleAssignmentErr := az.DeleteRoleAssignmentByID(*roleAssignment.ID) if deleteRoleAssignmentErr != nil { - logger.Errorf("failed to delete role assignment: %s", *roleAssignment.ID, deleteRoleAssignmentErr.Error()) + logger.Errorf("failed to delete role assignment: %s: %s", *roleAssignment.ID, deleteRoleAssignmentErr.Error()) return deleteRoleAssignmentErr } } diff --git a/pkg/operations/scaledownagentpool_test.go b/pkg/operations/scaledownagentpool_test.go index 4f3384ff43..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()), "sid","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) From fdc833880ec7814b77053fe446a057dbdf490ed7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sun, 13 May 2018 17:38:23 +0200 Subject: [PATCH 5/8] circle-ci feedback / gofmt --- pkg/armhelpers/graph.go | 2 ++ pkg/armhelpers/mockclients.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/pkg/armhelpers/graph.go b/pkg/armhelpers/graph.go index 053c2a282e..ff664cd544 100644 --- a/pkg/armhelpers/graph.go +++ b/pkg/armhelpers/graph.go @@ -37,10 +37,12 @@ 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) } +// List all role assignments for a principal (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) diff --git a/pkg/armhelpers/mockclients.go b/pkg/armhelpers/mockclients.go index 3184ac1394..332839fccd 100644 --- a/pkg/armhelpers/mockclients.go +++ b/pkg/armhelpers/mockclients.go @@ -467,10 +467,12 @@ func (mc *MockACSEngineClient) ListDeploymentOperationsNextResults(lastResults r 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 } +// List all role assignments for a principal (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{ From 45ff3dffa3275388e578a547b65ea6598cb2842b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sun, 13 May 2018 17:41:10 +0200 Subject: [PATCH 6/8] circle-ci feedback / gofmt --- pkg/armhelpers/graph.go | 2 +- pkg/armhelpers/mockclients.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/armhelpers/graph.go b/pkg/armhelpers/graph.go index ff664cd544..158668718a 100644 --- a/pkg/armhelpers/graph.go +++ b/pkg/armhelpers/graph.go @@ -42,7 +42,7 @@ func (az *AzureClient) DeleteRoleAssignmentByID(roleAssignmentID string) (author return az.authorizationClient.DeleteByID(roleAssignmentID) } -// List all role assignments for a principal (e.g. a VM) via the scope and the unique identifier of the principal +// 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) diff --git a/pkg/armhelpers/mockclients.go b/pkg/armhelpers/mockclients.go index 332839fccd..047d184c7f 100644 --- a/pkg/armhelpers/mockclients.go +++ b/pkg/armhelpers/mockclients.go @@ -472,7 +472,7 @@ func (mc *MockACSEngineClient) DeleteRoleAssignmentByID(roleAssignmentID string) return authorization.RoleAssignment{}, nil } -// List all role assignments for a principal (e.g. a VM) via the scope and the unique identifier of the principal +// 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{ From 1829cfe08d729cb05a0fa42721c8c4e111326f85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sat, 19 May 2018 21:42:53 +0200 Subject: [PATCH 7/8] Added test cases for role assignment deletion and added a fix to handle if no managed identity is returned by azure --- pkg/armhelpers/mockclients.go | 26 ++++++++++-- pkg/operations/deletevm.go | 34 +++++++-------- .../kubernetesupgrade/upgradecluster_test.go | 41 +++++++++++++++++++ 3 files changed, 81 insertions(+), 20 deletions(-) diff --git a/pkg/armhelpers/mockclients.go b/pkg/armhelpers/mockclients.go index 047d184c7f..12d798505c 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 } @@ -283,11 +285,15 @@ func (mc *MockACSEngineClient) GetVirtualMachine(resourceGroup, name string) (co poolnameString: &poolname, } + var vmIdentity *compute.VirtualMachineIdentity = nil + if mc.ShouldSupportVMIdentity { + vmIdentity = &compute.VirtualMachineIdentity{PrincipalID: &principalID} + } + return compute.VirtualMachine{ - Name: &vm1Name, - Tags: &tags, - Identity: &compute.VirtualMachineIdentity{ - PrincipalID: &principalID}, + Name: &vm1Name, + Tags: &tags, + Identity: vmIdentity, VirtualMachineProperties: &compute.VirtualMachineProperties{ StorageProfile: &compute.StorageProfile{ OsDisk: &compute.OSDisk{ @@ -469,12 +475,24 @@ func (mc *MockACSEngineClient) ListDeploymentOperationsNextResults(lastResults r // 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 c6f6564b39..e7be1cfa14 100644 --- a/pkg/operations/deletevm.go +++ b/pkg/operations/deletevm.go @@ -91,23 +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 - } + 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 + 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 + } } } 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 { From 47e03a66672901973eb8b8b0024e6b2510704e52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ro=CC=88ck=2C=20Cedric?= Date: Sat, 19 May 2018 21:51:47 +0200 Subject: [PATCH 8/8] Fixed linting errors --- pkg/armhelpers/mockclients.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/armhelpers/mockclients.go b/pkg/armhelpers/mockclients.go index 12d798505c..d568df4c56 100644 --- a/pkg/armhelpers/mockclients.go +++ b/pkg/armhelpers/mockclients.go @@ -285,7 +285,7 @@ func (mc *MockACSEngineClient) GetVirtualMachine(resourceGroup, name string) (co poolnameString: &poolname, } - var vmIdentity *compute.VirtualMachineIdentity = nil + var vmIdentity *compute.VirtualMachineIdentity if mc.ShouldSupportVMIdentity { vmIdentity = &compute.VirtualMachineIdentity{PrincipalID: &principalID} } @@ -487,9 +487,9 @@ func (mc *MockACSEngineClient) ListRoleAssignmentsForPrincipal(scope string, pri roleAssignments := []authorization.RoleAssignment{} if mc.ShouldSupportVMIdentity { - var assignmentId = "role-assignment-id" + var assignmentID = "role-assignment-id" var assignment = authorization.RoleAssignment{ - ID: &assignmentId} + ID: &assignmentID} roleAssignments = append(roleAssignments, assignment) }