Repository: Azure/aad-pod-identity
Pull request #441 (2019-11-21 19:38:36)

fix identity removal
<!-- Thank you for helping AAD Pod Identity with a pull request! -->

**Reason for Change**:
<!-- What does this PR improve or fix in AAD Pod Identity? Why is it needed? -->
With the change for whitelisting identities - https://github.com/Azure/aad-pod-identity/pull/431, non-immutable identities are no longer being cleaned up as expected. 

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->
This PR fixes the identity removal from non-immutable identities and also performs the check for immutable/non-immutable right at the start.

**Notes for Reviewers**:


kkmsft: /azp run
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/cmd/mic/main.go b/cmd/mic/main.go
index 019e64aa..acdb67ef 100644
--- a/cmd/mic/main.go
+++ b/cmd/mic/main.go
@@ -103,7 +103,10 @@ func main() {
 	config.Burst = int(clientQPS)
 	glog.Infof("Client QPS set to: %v. Burst to: %v", config.QPS, config.Burst)
 
-	immutableUserMSIsList := strings.Split(immutableUserMSIs, ",")
+	var immutableUserMSIsList []string
+	if immutableUserMSIs != "" {
+		immutableUserMSIsList = strings.Split(immutableUserMSIs, ",")
+	}
 	glog.Infof("immutable identities are %v", immutableUserMSIsList)
 
 	micClient, err := mic.NewMICClient(cloudconfig, config, forceNamespaced, syncRetryDuration, &leaderElectionCfg, enableScaleFeatures, createDeleteBatch, immutableUserMSIsList)
diff --git a/pkg/mic/mic.go b/pkg/mic/mic.go
index 1a0ee10f..88f6d2d9 100644
--- a/pkg/mic/mic.go
+++ b/pkg/mic/mic.go
@@ -51,18 +51,18 @@ type LeaderElectionConfig struct {
 // Client has the required pointers to talk to the api server
 // and interact with the CRD related datastructure.
 type Client struct {
-	CRDClient           crd.ClientInt
-	CloudClient         cloudprovider.ClientInt
-	PodClient           pod.ClientInt
-	EventRecorder       record.EventRecorder
-	EventChannel        chan aadpodid.EventType
-	NodeClient          NodeGetter
-	IsNamespaced        bool
-	SyncLoopStarted     bool
-	syncRetryInterval   time.Duration
-	enableScaleFeatures bool
-	createDeleteBatch   int64
-	ImmutableUserMSIs   map[string]bool
+	CRDClient            crd.ClientInt
+	CloudClient          cloudprovider.ClientInt
+	PodClient            pod.ClientInt
+	EventRecorder        record.EventRecorder
+	EventChannel         chan aadpodid.EventType
+	NodeClient           NodeGetter
+	IsNamespaced         bool
+	SyncLoopStarted      bool
+	syncRetryInterval    time.Duration
+	enableScaleFeatures  bool
+	createDeleteBatch    int64
+	ImmutableUserMSIsMap map[string]bool
 
 	syncing int32 // protect against conucrrent sync's
 
@@ -120,7 +120,8 @@ func NewMICClient(cloudconfig string, config *rest.Config, isNamespaced bool, sy
 
 	var immutableUserMSIsMap map[string]bool
 
-	if immutableUserMSIsList != nil {
+	if len(immutableUserMSIsList) > 0 {
+		// this map contains list of identities that are configured by user as immutable.
 		immutableUserMSIsMap = make(map[string]bool)
 		for _, item := range immutableUserMSIsList {
 			immutableUserMSIsMap[strings.ToLower(item)] = true
@@ -128,17 +129,17 @@ func NewMICClient(cloudconfig string, config *rest.Config, isNamespaced bool, sy
 	}
 
 	c := &Client{
-		CRDClient:           crdClient,
-		CloudClient:         cloudClient,
-		PodClient:           podClient,
-		EventRecorder:       recorder,
-		EventChannel:        eventCh,
-		NodeClient:          &NodeClient{informer.Core().V1().Nodes()},
-		IsNamespaced:        isNamespaced,
-		syncRetryInterval:   syncRetryInterval,
-		enableScaleFeatures: enableScaleFeatures,
-		createDeleteBatch:   createDeleteBatch,
-		ImmutableUserMSIs:   immutableUserMSIsMap,
+		CRDClient:            crdClient,
+		CloudClient:          cloudClient,
+		PodClient:            podClient,
+		EventRecorder:        recorder,
+		EventChannel:         eventCh,
+		NodeClient:           &NodeClient{informer.Core().V1().Nodes()},
+		IsNamespaced:         isNamespaced,
+		syncRetryInterval:    syncRetryInterval,
+		enableScaleFeatures:  enableScaleFeatures,
+		createDeleteBatch:    createDeleteBatch,
+		ImmutableUserMSIsMap: immutableUserMSIsMap,
 	}
 	leaderElector, err := c.NewLeaderElector(clientSet, recorder, leaderElectionConfig)
 	if err != nil {
@@ -470,12 +471,16 @@ func (c *Client) getListOfIdsToDelete(deleteList map[string]aadpodid.AzureAssign
 			glog.Error(err)
 			continue
 		}
+
 		id := delID.Spec.AzureIdentityRef
 		isUserAssignedMSI := c.checkIfUserAssignedMSI(id)
+		isImmutableIdentity := c.checkIfIdentityImmutable(id.Spec.ResourceID)
 
 		// this case includes Assigned state and empty state to ensure backward compatability
 		if delID.Status.Status == aadpodid.AssignedIDAssigned || delID.Status.Status == "" {
-			if !inUse && isUserAssignedMSI {
+			// only user assigned identities that are not in use and are not defined as
+			// immutable will be removed from underlying node/vmss
+			if !inUse && isUserAssignedMSI && !isImmutableIdentity {
 				c.appendToRemoveListForNode(id.Spec.ResourceID, delID.Spec.NodeName, nodeMap)
 			}
 		}
@@ -805,20 +810,7 @@ func (c *Client) updateUserMSI(newAssignedIDs map[string]aadpodid.AzureAssignedI
 	addUserAssignedMSIIDs := c.getUniqueIDs(nodeTrackList.addUserAssignedMSIIDs)
 	removeUserAssignedMSIIDs := c.getUniqueIDs(nodeTrackList.removeUserAssignedMSIIDs)
 
-	// The the list of identities (user-defined managed identities) shouldn't be deleted from VM/VMSS, but only from Kubernetes
-	var approvedRemoveUserAssignedMSIIDs []string
-	if c.ImmutableUserMSIs != nil {
-		approvedRemoveUserAssignedMSIIDs := make([]string, 0)
-		for _, item := range removeUserAssignedMSIIDs {
-			if _, ok := c.ImmutableUserMSIs[item]; !ok {
-				approvedRemoveUserAssignedMSIIDs = append(approvedRemoveUserAssignedMSIIDs, item)
-			}
-		}
-	} else {
-		approvedRemoveUserAssignedMSIIDs = removeUserAssignedMSIIDs
-	}
-
-	err := c.CloudClient.UpdateUserMSI(addUserAssignedMSIIDs, approvedRemoveUserAssignedMSIIDs, nodeOrVMSSName, nodeTrackList.isvmss)
+	err := c.CloudClient.UpdateUserMSI(addUserAssignedMSIIDs, removeUserAssignedMSIIDs, nodeOrVMSSName, nodeTrackList.isvmss)
 	if err != nil {
 		glog.Errorf("Updating msis on node %s, add [%d], del [%d] failed with error %v", nodeOrVMSSName, len(nodeTrackList.assignedIDsToCreate), len(nodeTrackList.assignedIDsToDelete), err)
 		idList, getErr := c.getUserMSIListForNode(nodeOrVMSSName, nodeTrackList.isvmss)
@@ -1044,3 +1036,18 @@ func (c *Client) consolidateVMSSNodes(nodeMap map[string]trackUserAssignedMSIIds
 		}
 	}
 }
+
+// checkIfIdentityImmutable checks if the identity is immutable
+// if identity is immutable, then it will not be removed from underlying node/vmss
+// returns true if identity is immutable
+func (c *Client) checkIfIdentityImmutable(id string) bool {
+	// no immutable identity list defined, then identity is not immutable and can be safely removed
+	if c.ImmutableUserMSIsMap == nil {
+		return false
+	}
+	// identity is immutable, so should not be deleted from the underlying node/vmss
+	if _, exists := c.ImmutableUserMSIsMap[id]; exists {
+		return true
+	}
+	return false
+}


Test patch:
diff --git a/pkg/mic/mic_test.go b/pkg/mic/mic_test.go
index df33e6c6..2bb0a146 100644
--- a/pkg/mic/mic_test.go
+++ b/pkg/mic/mic_test.go
@@ -605,16 +605,16 @@ func NewMICTestClient(eventCh chan aadpodid.EventType,
 	immutableUserMSIs map[string]bool) *TestMICClient {
 
 	realMICClient := &Client{
-		CloudClient:       cpClient,
-		CRDClient:         crdClient,
-		EventRecorder:     eventRecorder,
-		PodClient:         podClient,
-		EventChannel:      eventCh,
-		NodeClient:        nodeClient,
-		syncRetryInterval: 120 * time.Second,
-		IsNamespaced:      isNamespaced,
-		createDeleteBatch: createDeleteBatch,
-		ImmutableUserMSIs: immutableUserMSIs,
+		CloudClient:          cpClient,
+		CRDClient:            crdClient,
+		EventRecorder:        eventRecorder,
+		PodClient:            podClient,
+		EventChannel:         eventCh,
+		NodeClient:           nodeClient,
+		syncRetryInterval:    120 * time.Second,
+		IsNamespaced:         isNamespaced,
+		createDeleteBatch:    createDeleteBatch,
+		ImmutableUserMSIsMap: immutableUserMSIs,
 	}
 
 	return &TestMICClient{
diff --git a/test/e2e/aadpodidentity_test.go b/test/e2e/aadpodidentity_test.go
index 3b05f7e4..f53d6be8 100644
--- a/test/e2e/aadpodidentity_test.go
+++ b/test/e2e/aadpodidentity_test.go
@@ -639,7 +639,6 @@ var _ = Describe("Kubernetes cluster using aad-pod-identity", func() {
 			return
 		}
 
-
 		setUpIdentityAndDeployment(keyvaultIdentity, "", "1", func(d *infra.IdentityValidatorTemplateData) {
 			d.NodeName = vmss[0].Name
 		})