Repository: Azure/aad-pod-identity
Pull request #272 (2019-07-02 22:43:18)

NMI retries and ticker for periodic sync reconcile
<!-- 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? -->
- Wrap list pod ids call with retries
- Filter assigned ids based on state in NMI
- Change the status code to `404` instead of `403` when no valid assigned ids found after retries. 
- Add a ticker to periodically run sync cycle to reconcile any crds that are in error state. This is required because if no pod events are generated, then the sync loop will never get triggered to resync.
- Make the sync retry interval configurable

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->
fixes https://github.com/Azure/aad-pod-identity/issues/257

**Notes for Reviewers**:


aramase: Ran 10 of 10 Specs in 4998.515 seconds
SUCCESS! -- 10 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAADPodIdentity (4998.52s)
PASS
ok  	github.com/Azure/aad-pod-identity/test/e2e	4998.532s
aramase: ```bash
Ran 11 of 11 Specs in 3839.814 seconds
SUCCESS! -- 11 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAADPodIdentity (3839.81s)
PASS
ok  	github.com/Azure/aad-pod-identity/test/e2e	3839.837s
```

Patch:
diff --git a/Makefile b/Makefile
index 6b3ef468..413755fd 100644
--- a/Makefile
+++ b/Makefile
@@ -141,7 +141,7 @@ e2e:
 
 .PHONY: unit-test
 unit-test:
-	go test $(shell go list ./... | grep -v /test/e2e) -v
+	go test -count=1 $(shell go list ./... | grep -v /test/e2e) -v
 
 .PHONY: validate-version
 validate-version: validate-version-NMI validate-version-MIC validate-version-IDENTITY_VALIDATOR validate-version-DEMO
diff --git a/cmd/mic/main.go b/cmd/mic/main.go
index 2ff283cb..b5777766 100644
--- a/cmd/mic/main.go
+++ b/cmd/mic/main.go
@@ -3,6 +3,7 @@ package main
 import (
 	"flag"
 	"os"
+	"time"
 
 	"github.com/Azure/aad-pod-identity/pkg/mic"
 	"github.com/Azure/aad-pod-identity/version"
@@ -12,10 +13,11 @@ import (
 )
 
 var (
-	kubeconfig      string
-	cloudconfig     string
-	forceNamespaced bool
-	versionInfo     bool
+	kubeconfig        string
+	cloudconfig       string
+	forceNamespaced   bool
+	versionInfo       bool
+	syncRetryDuration time.Duration
 )
 
 func main() {
@@ -24,6 +26,8 @@ func main() {
 	flag.StringVar(&cloudconfig, "cloudconfig", "", "Path to cloud config e.g. Azure.json file")
 	flag.BoolVar(&forceNamespaced, "forceNamespaced", false, "Forces namespaced identities, binding, and assignment")
 	flag.BoolVar(&versionInfo, "version", false, "Prints the version information")
+	flag.DurationVar(&syncRetryDuration, "syncRetryDuration", 3600*time.Second, "The interval in seconds at which sync loop should periodically check for errors and reconcile.")
+
 	flag.Parse()
 	if versionInfo {
 		version.PrintVersionAndExit()
@@ -43,7 +47,8 @@ func main() {
 	}
 
 	forceNamespaced = forceNamespaced || "true" == os.Getenv("FORCENAMESPACED")
-	micClient, err := mic.NewMICClient(cloudconfig, config, forceNamespaced)
+
+	micClient, err := mic.NewMICClient(cloudconfig, config, forceNamespaced, syncRetryDuration)
 	if err != nil {
 		glog.Fatalf("Could not get the MIC client: %+v", err)
 	}
diff --git a/pkg/apis/aadpodidentity/v1/types.go b/pkg/apis/aadpodidentity/v1/types.go
index 7619aadf..89a92adf 100644
--- a/pkg/apis/aadpodidentity/v1/types.go
+++ b/pkg/apis/aadpodidentity/v1/types.go
@@ -26,8 +26,14 @@ const (
 	CRDLabelKey = "aadpodidbinding"
 
 	BehaviorKey = "aadpodidentity.k8s.io/Behavior"
-	// Namespaced Behavior
+	// BehaviorNamespaced ...
 	BehaviorNamespaced = "namespaced"
+	// AssignedIDCreated status indicates azure assigned identity is created
+	AssignedIDCreated = "Created"
+	// AssignedIDAssigned status indicates identity has been assigned to the node
+	AssignedIDAssigned = "Assigned"
+	// AssignedIDUnAssigned status indicates identity has been unassigned from the node
+	AssignedIDUnAssigned = "Unassigned"
 )
 
 /*** Global data structures ***/
diff --git a/pkg/crd/crd.go b/pkg/crd/crd.go
index 7a792ddf..a0c9c4cd 100644
--- a/pkg/crd/crd.go
+++ b/pkg/crd/crd.go
@@ -38,7 +38,7 @@ type ClientInt interface {
 	ListBindings() (res *[]aadpodid.AzureIdentityBinding, err error)
 	ListAssignedIDs() (res *[]aadpodid.AzureAssignedIdentity, err error)
 	ListIds() (res *[]aadpodid.AzureIdentity, err error)
-	ListPodIds(podns, podname string) (*[]aadpodid.AzureIdentity, error)
+	ListPodIds(podns, podname string) (map[string][]aadpodid.AzureIdentity, error)
 }
 
 func NewCRDClientLite(config *rest.Config) (crdClient *Client, err error) {
@@ -257,22 +257,22 @@ func (c *Client) ListIds() (res *[]aadpodid.AzureIdentity, err error) {
 	return &ret.(*aadpodid.AzureIdentityList).Items, nil
 }
 
-//ListPodIds - given a pod with pod name space
-func (c *Client) ListPodIds(podns, podname string) (*[]aadpodid.AzureIdentity, error) {
+// ListPodIds - given a pod with pod name space
+// returns a map with list of azure identities in each state
+func (c *Client) ListPodIds(podns, podname string) (map[string][]aadpodid.AzureIdentity, error) {
 	azAssignedIDList, err := c.AssignedIDListWatch.List(v1.ListOptions{})
 	if err != nil {
 		glog.Error(err)
 		return nil, err
 	}
 
-	var matchedIds []aadpodid.AzureIdentity
+	idStateMap := make(map[string][]aadpodid.AzureIdentity)
 	for _, v := range azAssignedIDList.(*aadpodid.AzureAssignedIdentityList).Items {
 		if v.Spec.Pod == podname && v.Spec.PodNamespace == podns {
-			matchedIds = append(matchedIds, *v.Spec.AzureIdentityRef)
+			idStateMap[v.Status.Status] = append(idStateMap[v.Status.Status], *v.Spec.AzureIdentityRef)
 		}
 	}
-
-	return &matchedIds, nil
+	return idStateMap, nil
 }
 
 type patchStatusOps struct {
diff --git a/pkg/k8s/client.go b/pkg/k8s/client.go
index 89361549..f7068034 100644
--- a/pkg/k8s/client.go
+++ b/pkg/k8s/client.go
@@ -39,7 +39,7 @@ type Client interface {
 	// GetPodName return the matching azure identity or nil
 	GetPodName(podip string) (podns, podname string, err error)
 	// ListPodIds pod matching azure identity or nil
-	ListPodIds(podns, podname string) (*[]aadpodid.AzureIdentity, error)
+	ListPodIds(podns, podname string) (map[string][]aadpodid.AzureIdentity, error)
 	// GetSecret returns secret the secretRef represents
 	GetSecret(secretRef *v1.SecretReference) (*v1.Secret, error)
 }
@@ -168,7 +168,7 @@ func GetLocalIP() (string, error) {
 }
 
 // ListPodIds lists matching ids for pod or error
-func (c *KubeClient) ListPodIds(podns, podname string) (*[]aadpodid.AzureIdentity, error) {
+func (c *KubeClient) ListPodIds(podns, podname string) (map[string][]aadpodid.AzureIdentity, error) {
 	return c.CrdClient.ListPodIds(podns, podname)
 }
 
diff --git a/pkg/k8s/fake.go b/pkg/k8s/fake.go
index 236d042d..221d47a8 100644
--- a/pkg/k8s/fake.go
+++ b/pkg/k8s/fake.go
@@ -2,7 +2,7 @@ package k8s
 
 import (
 	aadpodid "github.com/Azure/aad-pod-identity/pkg/apis/aadpodidentity/v1"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 )
 
 // FakeClient implements Interface
@@ -23,7 +23,7 @@ func (c *FakeClient) GetPodName(podip string) (podns, podname string, err error)
 }
 
 // ListPodIds for pod
-func (c *FakeClient) ListPodIds(podns string, podname string) (*[]aadpodid.AzureIdentity, error) {
+func (c *FakeClient) ListPodIds(podns, podname string) (map[string][]aadpodid.AzureIdentity, error) {
 	return nil, nil
 }
 
diff --git a/pkg/mic/mic.go b/pkg/mic/mic.go
index 3825d7ab..3d967e13 100644
--- a/pkg/mic/mic.go
+++ b/pkg/mic/mic.go
@@ -26,12 +26,6 @@ import (
 const (
 	stopped = int32(0)
 	running = int32(1)
-	// IdentityCreated status indicates azure assigned identity is created
-	IdentityCreated = "Created"
-	// IdentityAssigned status indicates identity has been assigned to the node
-	IdentityAssigned = "Assigned"
-	// IdentityUnassigned status indicates identity has been unassigned from the node
-	IdentityUnassigned = "Unassigned"
 )
 
 // NodeGetter ...
@@ -43,13 +37,14 @@ type NodeGetter interface {
 // 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
+	CRDClient         crd.ClientInt
+	CloudClient       cloudprovider.ClientInt
+	PodClient         pod.ClientInt
+	EventRecorder     record.EventRecorder
+	EventChannel      chan aadpodid.EventType
+	NodeClient        NodeGetter
+	IsNamespaced      bool
+	syncRetryInterval time.Duration
 
 	syncing int32 // protect against conucrrent sync's
 }
@@ -68,7 +63,7 @@ type trackUserAssignedMSIIds struct {
 }
 
 // NewMICClient returnes new mic client
-func NewMICClient(cloudconfig string, config *rest.Config, isNamespaced bool) (*Client, error) {
+func NewMICClient(cloudconfig string, config *rest.Config, isNamespaced bool, syncRetryInterval time.Duration) (*Client, error) {
 	glog.Infof("Starting to create the pod identity client. Version: %v. Build date: %v", version.MICVersion, version.BuildDate)
 
 	clientSet := kubernetes.NewForConfigOrDie(config)
@@ -96,13 +91,14 @@ func NewMICClient(cloudconfig string, config *rest.Config, isNamespaced bool) (*
 	recorder := eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: aadpodid.CRDGroup})
 
 	return &Client{
-		CRDClient:     crdClient,
-		CloudClient:   cloudClient,
-		PodClient:     podClient,
-		EventRecorder: recorder,
-		EventChannel:  eventCh,
-		NodeClient:    &NodeClient{informer.Core().V1().Nodes()},
-		IsNamespaced:  isNamespaced,
+		CRDClient:         crdClient,
+		CloudClient:       cloudClient,
+		PodClient:         podClient,
+		EventRecorder:     recorder,
+		EventChannel:      eventCh,
+		NodeClient:        &NodeClient{informer.Core().V1().Nodes()},
+		IsNamespaced:      isNamespaced,
+		syncRetryInterval: syncRetryInterval,
 	}, nil
 }
 
@@ -152,6 +148,9 @@ func (c *Client) Sync(exit <-chan struct{}) {
 	}
 	defer c.setStopped()
 
+	ticker := time.NewTicker(c.syncRetryInterval)
+	defer ticker.Stop()
+
 	glog.Info("Sync thread started.")
 	var event aadpodid.EventType
 	for {
@@ -159,13 +158,16 @@ func (c *Client) Sync(exit <-chan struct{}) {
 		case <-exit:
 			return
 		case event = <-c.EventChannel:
+			glog.V(6).Infof("Received event: %v", event)
+		case <-ticker.C:
+			glog.V(6).Infof("Running periodic sync loop")
 		}
 
 		stats.Init()
 		// This is the only place where the AzureAssignedIdentity creation is initiated.
 		begin := time.Now()
 		workDone := false
-		glog.V(6).Infof("Received event: %v", event)
+
 		// List all pods in all namespaces
 		systemTime := time.Now()
 		listPods, err := c.PodClient.GetPods()
@@ -355,7 +357,7 @@ func (c *Client) getListOfIdsToDelete(deleteList, newAssignedIDs []aadpodid.Azur
 		isUserAssignedMSI := c.checkIfUserAssignedMSI(id)
 
 		// this case includes Assigned state and empty state to ensure backward compatability
-		if delID.Status.Status == IdentityAssigned || delID.Status.Status == "" {
+		if delID.Status.Status == aadpodid.AssignedIDAssigned || delID.Status.Status == "" {
 			if !inUse && isUserAssignedMSI {
 				c.appendToRemoveListForNode(id.Spec.ResourceID, delID.Spec.NodeName, nodeMap)
 			}
@@ -370,7 +372,7 @@ func (c *Client) getListOfIdsToAssign(addList []aadpodid.AzureAssignedIdentity,
 		id := createID.Spec.AzureIdentityRef
 		isUserAssignedMSI := c.checkIfUserAssignedMSI(id)
 
-		if createID.Status.Status == "" || createID.Status.Status == IdentityCreated {
+		if createID.Status.Status == "" || createID.Status.Status == aadpodid.AssignedIDCreated {
 			if isUserAssignedMSI {
 				c.appendToAddListForNode(id.Spec.ResourceID, createID.Spec.NodeName, nodeMap)
 			}
@@ -417,7 +419,7 @@ func (c *Client) getAzureAssignedIDsToCreate(old, new []aadpodid.AzureAssignedId
 				// if the old assigned id is in created state, then the identity assignment to the node
 				// is not done. Adding to the list will ensure we retry identity assignment to node for
 				// this assigned identity.
-				if oldAssignedID.Status.Status == IdentityCreated {
+				if oldAssignedID.Status.Status == aadpodid.AssignedIDCreated {
 					create = append(create, oldAssignedID)
 				}
 				break
@@ -641,7 +643,7 @@ func (c *Client) updateUserMSI(newAssignedIDs []aadpodid.AzureAssignedIdentity,
 			// this is the state when the azure assigned identity is yet to be created
 			glog.V(5).Infof("Initiating assigned id creation for pod - %s, binding - %s", createID.Spec.Pod, binding.Name)
 
-			createID.Status.Status = IdentityCreated
+			createID.Status.Status = aadpodid.AssignedIDCreated
 			err := c.createAssignedIdentity(&createID)
 			if err != nil {
 				c.EventRecorder.Event(binding, corev1.EventTypeWarning, "binding apply error",
@@ -686,8 +688,8 @@ func (c *Client) updateUserMSI(newAssignedIDs []aadpodid.AzureAssignedIdentity,
 				fmt.Sprintf("Binding %s applied on node %s for pod %s", binding.Name, createID.Spec.NodeName, createID.Name))
 
 			// Identity is successfully assigned to node, so update the status of assigned identity to assigned
-			if updateErr := c.updateAssignedIdentityStatus(&createID, IdentityAssigned); updateErr != nil {
-				message := fmt.Sprintf("Updating assigned identity %s status to %s for pod %s failed with error %v", createID.Name, IdentityAssigned, createID.Spec.Pod, updateErr.Error())
+			if updateErr := c.updateAssignedIdentityStatus(&createID, aadpodid.AssignedIDAssigned); updateErr != nil {
+				message := fmt.Sprintf("Updating assigned identity %s status to %s for pod %s failed with error %v", createID.Name, aadpodid.AssignedIDAssigned, createID.Spec.Pod, updateErr.Error())
 				c.EventRecorder.Event(&createID, corev1.EventTypeWarning, "status update error", message)
 				glog.Error(message)
 			}
@@ -734,9 +736,9 @@ func (c *Client) updateUserMSI(newAssignedIDs []aadpodid.AzureAssignedIdentity,
 	for _, createID := range nodeTrackList.assignedIDsToCreate {
 		binding := createID.Spec.AzureBindingRef
 		// update the status to assigned for assigned identity as identity was successfully assigned to node.
-		err = c.updateAssignedIdentityStatus(&createID, IdentityAssigned)
+		err = c.updateAssignedIdentityStatus(&createID, aadpodid.AssignedIDAssigned)
 		if err != nil {
-			message := fmt.Sprintf("Updating assigned identity %s status to %s for pod %s failed with error %v", createID.Name, IdentityAssigned, createID.Spec.Pod, err.Error())
+			message := fmt.Sprintf("Updating assigned identity %s status to %s for pod %s failed with error %v", createID.Name, aadpodid.AssignedIDAssigned, createID.Spec.Pod, err.Error())
 			c.EventRecorder.Event(&createID, corev1.EventTypeWarning, "status update error", message)
 			glog.Error(message)
 			continue
@@ -750,9 +752,9 @@ func (c *Client) updateUserMSI(newAssignedIDs []aadpodid.AzureAssignedIdentity,
 
 		// update the status for the assigned identity to Unassigned as the identity has been successfully removed from node.
 		// this will ensure on next sync loop we only try to delete the assigned identity instead of doing everything.
-		err = c.updateAssignedIdentityStatus(&delID, IdentityUnassigned)
+		err = c.updateAssignedIdentityStatus(&delID, aadpodid.AssignedIDUnAssigned)
 		if err != nil {
-			message := fmt.Sprintf("Updating assigned identity %s status to %s for pod %s failed with error %v", delID.Name, IdentityUnassigned, delID.Spec.Pod, err.Error())
+			message := fmt.Sprintf("Updating assigned identity %s status to %s for pod %s failed with error %v", delID.Name, aadpodid.AssignedIDUnAssigned, delID.Spec.Pod, err.Error())
 			c.EventRecorder.Event(&delID, corev1.EventTypeWarning, "status update error", message)
 			glog.Error(message)
 			continue
diff --git a/pkg/nmi/server/server.go b/pkg/nmi/server/server.go
index 001a016d..a1705e42 100644
--- a/pkg/nmi/server/server.go
+++ b/pkg/nmi/server/server.go
@@ -1,6 +1,7 @@
 package server
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -27,6 +28,8 @@ import (
 const (
 	iptableUpdateTimeIntervalInSeconds = 60
 	localhost                          = "127.0.0.1"
+	listPodIDsRetryAttempts            = 5
+	listPodIDsRetryIntervalInSeconds   = 6
 )
 
 // Server encapsulates all of the parameters necessary for starting up
@@ -42,6 +45,7 @@ type Server struct {
 	IsNamespaced                       bool
 }
 
+// NMIResponse is the response returned to caller
 type NMIResponse struct {
 	Token    adal.Token `json:"token"`
 	ClientID string     `json:"clientid"`
@@ -153,31 +157,31 @@ func (fn appHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 func (s *Server) hostHandler(logger *log.Entry, w http.ResponseWriter, r *http.Request) {
 	hostIP := parseRemoteAddr(r.RemoteAddr)
+	rqClientID, rqResource := parseRequestClientIDAndResource(r)
+
 	if hostIP != localhost {
 		msg := "request remote address is not from a host"
 		logger.Error(msg)
 		http.Error(w, msg, http.StatusInternalServerError)
 		return
 	}
-
 	podns, podname := parseRequestHeader(r)
 	if podns == "" || podname == "" {
 		logger.Errorf("missing podname and podns from request")
 		http.Error(w, "missing 'podname' and 'podns' from request header", http.StatusBadRequest)
 		return
 	}
-
-	podIDs, err := s.KubeClient.ListPodIds(podns, podname)
-	if err != nil || len(*podIDs) == 0 {
-		msg := fmt.Sprintf("no AzureAssignedIdentity found for pod:%s/%s", podns, podname)
+	podIDs, err := listPodIDsWithRetry(r.Context(), s.KubeClient, logger, podns, podname, rqClientID, listPodIDsRetryAttempts)
+	if err != nil {
+		msg := fmt.Sprintf("no AzureAssignedIdentity found for pod:%s/%s in assigned state", podns, podname)
 		logger.Errorf("%s, %+v", msg, err)
-		http.Error(w, msg, http.StatusForbidden)
+		http.Error(w, msg, http.StatusNotFound)
 		return
 	}
 
-	// filter out if we are in namesoaced mode
+	// filter out if we are in namespaced mode
 	filterPodIdentities := []aadpodid.AzureIdentity{}
-	for _, val := range *(podIDs) {
+	for _, val := range podIDs {
 		if s.IsNamespaced || aadpodid.IsNamespacedIdentity(&val) {
 			// namespaced mode
 			if val.Namespace == podns {
@@ -192,8 +196,7 @@ func (s *Server) hostHandler(logger *log.Entry, w http.ResponseWriter, r *http.R
 			filterPodIdentities = append(filterPodIdentities, val)
 		}
 	}
-	podIDs = &filterPodIdentities
-	rqClientID, rqResource := parseRequestClientIDAndResource(r)
+	podIDs = filterPodIdentities
 	token, clientID, err := getTokenForMatchingID(s.KubeClient, logger, rqClientID, rqResource, podIDs)
 	if err != nil {
 		logger.Errorf("failed to get service principal token for pod:%s/%s, %+v", podns, podname, err)
@@ -220,6 +223,8 @@ func (s *Server) hostHandler(logger *log.Entry, w http.ResponseWriter, r *http.R
 // configured id.
 func (s *Server) msiHandler(logger *log.Entry, w http.ResponseWriter, r *http.Request) {
 	podIP := parseRemoteAddr(r.RemoteAddr)
+	rqClientID, rqResource := parseRequestClientIDAndResource(r)
+
 	if podIP == "" {
 		msg := "request remote address is empty"
 		logger.Error(msg)
@@ -232,14 +237,13 @@ func (s *Server) msiHandler(logger *log.Entry, w http.ResponseWriter, r *http.Re
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}
-	podIDs, err := s.KubeClient.ListPodIds(podns, podname)
-	if err != nil || len(*podIDs) == 0 {
-		msg := fmt.Sprintf("no AzureAssignedIdentity found for pod:%s/%s", podns, podname)
+	podIDs, err := listPodIDsWithRetry(r.Context(), s.KubeClient, logger, podns, podname, rqClientID, listPodIDsRetryAttempts)
+	if err != nil {
+		msg := fmt.Sprintf("no AzureAssignedIdentity found for pod:%s/%s in assigned state", podns, podname)
 		logger.Errorf("%s, %+v", msg, err)
-		http.Error(w, msg, http.StatusForbidden)
+		http.Error(w, msg, http.StatusNotFound)
 		return
 	}
-	rqClientID, rqResource := parseRequestClientIDAndResource(r)
 	token, _, err := getTokenForMatchingID(s.KubeClient, logger, rqClientID, rqResource, podIDs)
 	if err != nil {
 		logger.Errorf("failed to get service principal token for pod:%s/%s, %+v", podns, podname, err)
@@ -255,9 +259,9 @@ func (s *Server) msiHandler(logger *log.Entry, w http.ResponseWriter, r *http.Re
 	w.Write(response)
 }
 
-func getTokenForMatchingID(kubeClient k8s.Client, logger *log.Entry, rqClientID string, rqResource string, podIDs *[]aadpodid.AzureIdentity) (token *adal.Token, clientID string, err error) {
+func getTokenForMatchingID(kubeClient k8s.Client, logger *log.Entry, rqClientID string, rqResource string, podIDs []aadpodid.AzureIdentity) (token *adal.Token, clientID string, err error) {
 	rqHasClientID := len(rqClientID) != 0
-	for _, v := range *podIDs {
+	for _, v := range podIDs {
 		clientID := v.Spec.ClientID
 		if rqHasClientID && !strings.EqualFold(rqClientID, clientID) {
 			logger.Warningf("clientid mismatch, requested:%s available:%s", rqClientID, clientID)
@@ -380,3 +384,57 @@ func handleTermination() {
 	log.Infof("Exiting with %v", exitCode)
 	os.Exit(exitCode)
 }
+
+func listPodIDsWithRetry(ctx context.Context, kubeClient k8s.Client, logger *log.Entry, podns, podname, rqClientID string, maxAttemptsPerCheck int) ([]aadpodid.AzureIdentity, error) {
+	attempt := 0
+	var err error
+	var idStateMap map[string][]aadpodid.AzureIdentity
+
+	// this loop will run to ensure we have assigned identities before we return. If there are no assigned identities in created state within 30s, then we return an error.
+	// If we get an assigned identity in created state within 30s, then loop will continue for another 30s to find assigned identity in assigned state.
+	for attempt < 2*maxAttemptsPerCheck {
+		idStateMap, err = kubeClient.ListPodIds(podns, podname)
+		if err == nil {
+			if len(rqClientID) == 0 {
+				// check to ensure backward compatability with assignedIDs that have no state
+				if len(idStateMap[""]) != 0 {
+					logger.Warningf("found assignedIDs with no state for pod:%s/%s. AssignedIDs created with old version of mic.", podns, podname)
+					return idStateMap[""], nil
+				}
+				if len(idStateMap[aadpodid.AssignedIDAssigned]) != 0 {
+					return idStateMap[aadpodid.AssignedIDAssigned], nil
+				}
+			}
+
+			// if client id exists in request, we need to ensure the identity with this client
+			// exists and is in Assigned state
+			if len(rqClientID) != 0 {
+				// check to ensure backward compatability with assignedIDs that have no state
+				for _, podID := range idStateMap[""] {
+					if strings.EqualFold(rqClientID, podID.Spec.ClientID) {
+						logger.Warningf("found assignedIDs with no state for pod:%s/%s. AssignedIDs created with old version of mic.", podns, podname)
+						return idStateMap[""], nil
+					}
+				}
+				for _, podID := range idStateMap[aadpodid.AssignedIDAssigned] {
+					if strings.EqualFold(rqClientID, podID.Spec.ClientID) {
+						return idStateMap[aadpodid.AssignedIDAssigned], nil
+					}
+				}
+			}
+			if len(idStateMap[aadpodid.AssignedIDCreated]) == 0 && attempt >= maxAttemptsPerCheck {
+				return nil, fmt.Errorf("getting assigned identities for pod %s/%s in CREATED state failed after %d attempts. Error: %v", podns, podname, maxAttemptsPerCheck, err)
+			}
+		}
+		attempt++
+
+		select {
+		case <-time.After(listPodIDsRetryIntervalInSeconds * time.Second):
+		case <-ctx.Done():
+			err = ctx.Err()
+			return nil, err
+		}
+		logger.Warningf("failed to get assigned ids for pod:%s/%s in ASSIGNED state, retrying attempt: %d", podns, podname, attempt)
+	}
+	return nil, fmt.Errorf("getting assigned identities for pod %s/%s in ASSIGNED state failed after %d attempts. Error: %v", podns, podname, 2*maxAttemptsPerCheck, err)
+}


Test patch:
diff --git a/pkg/mic/mic_test.go b/pkg/mic/mic_test.go
index bf3ae042..cc7d5ace 100644
--- a/pkg/mic/mic_test.go
+++ b/pkg/mic/mic_test.go
@@ -441,8 +441,8 @@ func (c *TestCrdClient) ListAssignedIDs() (res *[]aadpodid.AzureAssignedIdentity
 	return &assignedIDList, nil
 }
 
-func (c *Client) ListPodIds(podns, podname string) (*[]aadpodid.AzureIdentity, error) {
-	return &[]aadpodid.AzureIdentity{}, nil
+func (c *Client) ListPodIds(podns, podname string) (map[string][]aadpodid.AzureIdentity, error) {
+	return map[string][]aadpodid.AzureIdentity{}, nil
 }
 
 func (c *TestCrdClient) SetError(err error) {
@@ -556,12 +556,13 @@ func (c *TestEventRecorder) AnnotatedEventf(object runtime.Object, annotations m
 func NewMICTestClient(eventCh chan aadpodid.EventType, cpClient *TestCloudClient, crdClient *TestCrdClient, podClient *TestPodClient, nodeClient *TestNodeClient, eventRecorder *TestEventRecorder) *TestMICClient {
 
 	realMICClient := &Client{
-		CloudClient:   cpClient,
-		CRDClient:     crdClient,
-		EventRecorder: eventRecorder,
-		PodClient:     podClient,
-		EventChannel:  eventCh,
-		NodeClient:    nodeClient,
+		CloudClient:       cpClient,
+		CRDClient:         crdClient,
+		EventRecorder:     eventRecorder,
+		PodClient:         podClient,
+		EventChannel:      eventCh,
+		NodeClient:        nodeClient,
+		syncRetryInterval: 120 * time.Second,
 	}
 
 	return &TestMICClient{
@@ -741,8 +742,8 @@ func TestSimpleMICClient(t *testing.T) {
 		t.Fatalf("list assigned failed")
 	}
 
-	if (*listAssignedIDs)[0].Status.Status != IdentityCreated {
-		t.Fatalf("expected status to be %s, got: %s", IdentityCreated, (*listAssignedIDs)[0].Status.Status)
+	if (*listAssignedIDs)[0].Status.Status != aadpodid.AssignedIDCreated {
+		t.Fatalf("expected status to be %s, got: %s", aadpodid.AssignedIDCreated, (*listAssignedIDs)[0].Status.Status)
 	}
 
 	/*
@@ -1003,8 +1004,8 @@ func TestMICStateFlow(t *testing.T) {
 	if !(len(*listAssignedIDs) == 1) {
 		t.Fatalf("expected assigned identities len: %d, got: %d", 1, len(*listAssignedIDs))
 	}
-	if !((*listAssignedIDs)[0].Status.Status == IdentityAssigned) {
-		t.Fatalf("expected status to be %s, got: %s", IdentityCreated, (*listAssignedIDs)[0].Status.Status)
+	if !((*listAssignedIDs)[0].Status.Status == aadpodid.AssignedIDAssigned) {
+		t.Fatalf("expected status to be %s, got: %s", aadpodid.AssignedIDAssigned, (*listAssignedIDs)[0].Status.Status)
 	}
 
 	// delete the pod, simulate failure in cloud calls on trying to un-assign identity from node
@@ -1022,8 +1023,8 @@ func TestMICStateFlow(t *testing.T) {
 	if !(len(*listAssignedIDs) == 1) {
 		t.Fatalf("expected assigned identities len: %d, got: %d", 1, len(*listAssignedIDs))
 	}
-	if !((*listAssignedIDs)[0].Status.Status == IdentityAssigned) {
-		t.Fatalf("expected status to be %s, got: %s", IdentityAssigned, (*listAssignedIDs)[0].Status.Status)
+	if !((*listAssignedIDs)[0].Status.Status == aadpodid.AssignedIDAssigned) {
+		t.Fatalf("expected status to be %s, got: %s", aadpodid.AssignedIDAssigned, (*listAssignedIDs)[0].Status.Status)
 	}
 
 	cloudClient.UnSetError()
@@ -1051,13 +1052,13 @@ func TestMICStateFlow(t *testing.T) {
 	}
 	for _, assignedID := range *listAssignedIDs {
 		if assignedID.Spec.Pod == "test-pod1" {
-			if assignedID.Status.Status != IdentityUnassigned {
-				t.Fatalf("Expected status to be: %s. Got: %s", IdentityUnassigned, assignedID.Status.Status)
+			if assignedID.Status.Status != aadpodid.AssignedIDUnAssigned {
+				t.Fatalf("Expected status to be: %s. Got: %s", aadpodid.AssignedIDUnAssigned, assignedID.Status.Status)
 			}
 		}
 		if assignedID.Spec.Pod == "test-pod2" {
-			if assignedID.Status.Status != IdentityAssigned {
-				t.Fatalf("Expected status to be: %s. Got: %s", IdentityAssigned, assignedID.Status.Status)
+			if assignedID.Status.Status != aadpodid.AssignedIDAssigned {
+				t.Fatalf("Expected status to be: %s. Got: %s", aadpodid.AssignedIDAssigned, assignedID.Status.Status)
 			}
 		}
 	}
@@ -1079,6 +1080,85 @@ func TestMICStateFlow(t *testing.T) {
 	}
 }
 
+func TestSyncRetryLoop(t *testing.T) {
+	eventCh := make(chan aadpodid.EventType, 100)
+	cloudClient := NewTestCloudClient(config.AzureConfig{})
+	crdClient := NewTestCrdClient(nil)
+	podClient := NewTestPodClient()
+	nodeClient := NewTestNodeClient()
+	var evtRecorder TestEventRecorder
+	evtRecorder.lastEvent = new(LastEvent)
+	evtRecorder.eventChannel = make(chan bool, 100)
+
+	micClient := NewMICTestClient(eventCh, cloudClient, crdClient, podClient, nodeClient, &evtRecorder)
+	syncRetryInterval, err := time.ParseDuration("10s")
+	if err != nil {
+		t.Errorf("error parsing duration: %v", err)
+	}
+	micClient.syncRetryInterval = syncRetryInterval
+
+	// Add a pod, identity and binding.
+	crdClient.CreateID("test-id1", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
+	crdClient.CreateBinding("testbinding1", "test-id1", "test-select1")
+
+	nodeClient.AddNode("test-node1")
+	podClient.AddPod("test-pod1", "default", "test-node1", "test-select1")
+
+	eventCh <- aadpodid.PodCreated
+	defer micClient.testRunSync()(t)
+
+	if !evtRecorder.WaitForEvents(1) {
+		t.Fatalf("Timeout waiting for mic sync cycles")
+	}
+	listAssignedIDs, err := crdClient.ListAssignedIDs()
+	if err != nil {
+		glog.Error(err)
+		t.Errorf("list assigned failed")
+	}
+	if !(len(*listAssignedIDs) == 1) {
+		t.Fatalf("expected assigned identities len: %d, got: %d", 1, len(*listAssignedIDs))
+	}
+	if !((*listAssignedIDs)[0].Status.Status == aadpodid.AssignedIDAssigned) {
+		t.Fatalf("expected status to be %s, got: %s", aadpodid.AssignedIDAssigned, (*listAssignedIDs)[0].Status.Status)
+	}
+
+	// delete the pod, simulate failure in cloud calls on trying to un-assign identity from node
+	podClient.DeletePod("test-pod1", "default")
+	cloudClient.SetError(errors.New("error removing identity from node"))
+	cloudClient.testVMClient.identity = &compute.VirtualMachineIdentity{IdentityIds: &[]string{"test-user-msi-resourceid"}}
+
+	eventCh <- aadpodid.PodDeleted
+	if !evtRecorder.WaitForEvents(1) {
+		t.Fatalf("Timeout waiting for mic sync cycles")
+	}
+
+	listAssignedIDs, err = crdClient.ListAssignedIDs()
+	if err != nil {
+		glog.Error(err)
+		t.Errorf("list assigned failed")
+	}
+	if !(len(*listAssignedIDs) == 1) {
+		t.Fatalf("expected assigned identities len: %d, got: %d", 1, len(*listAssignedIDs))
+	}
+	if !((*listAssignedIDs)[0].Status.Status == aadpodid.AssignedIDAssigned) {
+		t.Fatalf("expected status to be %s, got: %s", aadpodid.AssignedIDAssigned, (*listAssignedIDs)[0].Status.Status)
+	}
+	cloudClient.UnSetError()
+
+	if !evtRecorder.WaitForEvents(1) {
+		t.Fatalf("Timeout waiting for mic sync retry cycle")
+	}
+
+	listAssignedIDs, err = crdClient.ListAssignedIDs()
+	if err != nil {
+		glog.Error(err)
+		t.Errorf("list assigned failed")
+	}
+	if !(len(*listAssignedIDs) == 0) {
+		t.Fatalf("expected assigned identities len: %d, got: %d", 0, len(*listAssignedIDs))
+	}
+}
+
 func TestSyncExit(t *testing.T) {
 	eventCh := make(chan aadpodid.EventType)
 	cloudClient := NewTestCloudClient(config.AzureConfig{VMType: "vmss"})
diff --git a/pkg/nmi/server/server_test.go b/pkg/nmi/server/server_test.go
index 772ff1d2..f836ab27 100644
--- a/pkg/nmi/server/server_test.go
+++ b/pkg/nmi/server/server_test.go
@@ -7,7 +7,7 @@ import (
 	aadpodid "github.com/Azure/aad-pod-identity/pkg/apis/aadpodidentity/v1"
 	"github.com/Azure/aad-pod-identity/pkg/k8s"
 	log "github.com/sirupsen/logrus"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
 )
@@ -37,5 +37,5 @@ func TestGetTokenForMatchingIDBySP(t *testing.T) {
 	}
 	podIDs := []aadpodid.AzureIdentity{podID}
 	logger := log.WithError(nil)
-	getTokenForMatchingID(kubeClient, logger, podID.Spec.ClientID, "https://management.azure.com/", &podIDs)
+	getTokenForMatchingID(kubeClient, logger, podID.Spec.ClientID, "https://management.azure.com/", podIDs)
 }
diff --git a/test/e2e/aadpodidentity_test.go b/test/e2e/aadpodidentity_test.go
index fb383dfe..9dfe303f 100644
--- a/test/e2e/aadpodidentity_test.go
+++ b/test/e2e/aadpodidentity_test.go
@@ -52,7 +52,7 @@ var _ = BeforeSuite(func() {
 	Expect(err).NotTo(HaveOccurred())
 	cfg = *c
 
-	setupInfra()
+	setupInfra(cfg.Registry, cfg.NMIVersion, cfg.MICVersion)
 })
 
 var _ = AfterSuite(func() {
@@ -111,7 +111,7 @@ var _ = Describe("Kubernetes cluster using aad-pod-identity", func() {
 		fmt.Println("\nTearing down the test environment...")
 
 		// Ensure a clean cluster after the end of each test
-		cmd := exec.Command("kubectl", "delete", "AzureIdentity,AzureIdentityBinding,AzureAssignedIdentity", "--all")
+		cmd := exec.Command("kubectl", "delete", "AzureIdentity,AzureIdentityBinding", "--all")
 		util.PrintCommand(cmd)
 		_, err := cmd.CombinedOutput()
 		Expect(err).NotTo(HaveOccurred())
@@ -482,7 +482,7 @@ var _ = Describe("Kubernetes cluster using aad-pod-identity", func() {
 		}
 
 		// reset the infra to previous state
-		setupInfra()
+		setupInfra(cfg.Registry, cfg.NMIVersion, cfg.MICVersion)
 	})
 
 	It("should not alter the system assigned identity after creating and deleting pod identity", func() {
@@ -556,12 +556,47 @@ var _ = Describe("Kubernetes cluster using aad-pod-identity", func() {
 			}
 		}
 	})
+
+	It("should be backward compatible with old and new version of mic and nmi", func() {
+		// Uninstall CRDs and delete MIC and NMI
+		err := deploy.Delete("default", templateOutputPath)
+		Expect(err).NotTo(HaveOccurred())
+
+		ok, err := pod.WaitOnDeletion("nmi")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ok).To(Equal(true))
+
+		// setup mic and nmi with old releases
+		setupInfra("mcr.microsoft.com/k8s/aad-pod-identity", "1.4", "1.3")
+
+		setUpIdentityAndDeployment(keyvaultIdentity, "", "1")
+
+		ok, err = azureassignedidentity.WaitOnLengthMatched(1)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ok).To(Equal(true))
+
+		// update the infra to use latest mic and nmi images
+		setupInfra(cfg.Registry, cfg.NMIVersion, cfg.MICVersion)
+
+		ok, err = daemonset.WaitOnReady(nmiDaemonSet)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ok).To(Equal(true))
+
+		ok, err = waitForIPTableRulesToExist("busybox")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ok).To(Equal(true))
+
+		azureAssignedIdentity, err := azureassignedidentity.GetByPrefix(identityValidator)
+		Expect(err).NotTo(HaveOccurred())
+
+		validateAzureAssignedIdentity(azureAssignedIdentity, keyvaultIdentity)
+	})
 })
 
 // setupInfra creates the crds, mic, nmi and blocks until iptable entries exist
-func setupInfra() {
+func setupInfra(registry, nmiVersion, micVersion string) {
 	// Install CRDs and deploy MIC and NMI
-	err := infra.CreateInfra("default", cfg.Registry, cfg.NMIVersion, cfg.MICVersion, templateOutputPath)
+	err := infra.CreateInfra("default", registry, nmiVersion, micVersion, templateOutputPath)
 
 	Expect(err).NotTo(HaveOccurred())
 
@@ -626,8 +661,6 @@ func setUpIdentityAndDeployment(azureIdentityName, suffix, replicas string) {
 	ok, err = daemonset.WaitOnReady(nmiDaemonSet)
 	Expect(err).NotTo(HaveOccurred())
 	Expect(ok).To(Equal(true))
-
-	time.Sleep(30 * time.Second)
 }
 
 // validateAzureAssignedIdentity will make sure a given AzureAssignedIdentity has the correct properties
diff --git a/test/e2e/template/deployment-rbac.yaml b/test/e2e/template/deployment-rbac.yaml
index 6aee394a..a33f75d7 100644
--- a/test/e2e/template/deployment-rbac.yaml
+++ b/test/e2e/template/deployment-rbac.yaml
@@ -85,6 +85,8 @@ metadata:
   name: nmi
   namespace: {{.Namespace}}
 spec:
+  updateStrategy:
+    type: RollingUpdate
   template:
     metadata:
       labels: