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] 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 {