Added test cases for role assignment deletion and added a fix to hand…

…le if no managed identity is returned by azure

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
 }

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
+			}
 		}
 	}
 

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 {