Repository: Azure/aad-pod-identity
Pull request #271 (2019-07-01 21:20:18)

Handle case sensitive id check
<!-- 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? -->
MIC fails when trying to compare same identity but with different cases for resource groups `resourceGroups` | `resourcegroups` in the resourceID format. This results in error when trying to assign the identity which already exists. This change will ensure we validate the identities case insensitively.

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->
https://github.com/Azure/aad-pod-identity/issues/269

**Notes for Reviewers**:



Patch:
diff --git a/pkg/cloudprovider/identity.go b/pkg/cloudprovider/identity.go
index 1b0f7349..44e98523 100644
--- a/pkg/cloudprovider/identity.go
+++ b/pkg/cloudprovider/identity.go
@@ -2,6 +2,7 @@ package cloudprovider
 
 import (
 	"errors"
+	"strings"
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-04-01/compute"
 )
@@ -65,7 +66,7 @@ func filter(ls *[]string, filter string) {
 	}
 
 	for i, v := range *ls {
-		if v == filter {
+		if strings.EqualFold(v, filter) {
 			copy((*ls)[i:], (*ls)[i+1:])
 			*ls = (*ls)[:len(*ls)-1]
 			return
@@ -94,7 +95,7 @@ func appendUserIdentity(idType *compute.ResourceIdentityType, idList *[]string,
 
 func checkIfIDInList(idList []string, desiredID string) bool {
 	for _, id := range idList {
-		if id == desiredID {
+		if strings.EqualFold(id, desiredID) {
 			return true
 		}
 	}
diff --git a/pkg/k8s/client.go b/pkg/k8s/client.go
index f14bd426..89361549 100644
--- a/pkg/k8s/client.go
+++ b/pkg/k8s/client.go
@@ -15,11 +15,12 @@ import (
 
 	aadpodid "github.com/Azure/aad-pod-identity/pkg/apis/aadpodidentity/v1"
 	crd "github.com/Azure/aad-pod-identity/pkg/crd"
+	log "github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
-	getPodListTries                 = 5
+	getPodListRetries               = 4
 	getPodListSleepTimeMilliseconds = 300
 )
 
@@ -87,7 +88,7 @@ func (c *KubeClient) GetPodName(podip string) (podns, poddname string, err error
 		return "", "", fmt.Errorf("podip is empty")
 	}
 
-	podList, err := c.getPodListWithTries(podip, getPodListTries, getPodListSleepTimeMilliseconds)
+	podList, err := c.getPodListRetry(podip, getPodListRetries, getPodListSleepTimeMilliseconds)
 
 	if err != nil {
 		return "", "", err
@@ -100,19 +101,53 @@ func (c *KubeClient) GetPodName(podip string) (podns, poddname string, err error
 	return "", "", fmt.Errorf("match failed, ip:%s matching pods:%v", podip, podList)
 }
 
-func (c *KubeClient) getPodListWithTries(podip string, tries int, sleeptime time.Duration) (*v1.PodList, error) {
-	podList, err := c.PodListWatch.List(metav1.ListOptions{
+func (c *KubeClient) getPodList(podip string) (*v1.PodList, error) {
+	listObject, err := c.PodListWatch.List(metav1.ListOptions{
 		FieldSelector: "status.podIP==" + podip + phaseStatusFilter,
 	})
 
-	if err != nil || len(podList.(*v1.PodList).Items) == 0 {
-		if tries > 1 {
-			time.Sleep(sleeptime * time.Millisecond)
-			return c.getPodListWithTries(podip, tries-1, sleeptime)
-		}
+	if err != nil {
+		return nil, err
+	}
+
+	// Confirm that we are able to cast properly.
+	podList, ok := listObject.(*v1.PodList)
+	if !ok {
+		return nil, fmt.Errorf("list object could not be converted to podlist")
+	}
+
+	if podList == nil {
+		return nil, fmt.Errorf("pod list nil")
 	}
 
-	return podList.(*v1.PodList), err
+	if len(podList.Items) == 0 {
+		return nil, fmt.Errorf("pod list empty")
+	}
+
+	return podList, nil
+}
+
+func (c *KubeClient) getPodListRetry(podip string, retries int, sleeptime time.Duration) (*v1.PodList, error) {
+	var podList *v1.PodList
+	var err error
+	i := 0
+
+	for {
+		// Atleast run the getpodlist once.
+		podList, err = c.getPodList(podip)
+		if err == nil {
+			return podList, nil
+		}
+		if i >= retries {
+			break
+		}
+		i++
+		log.Warningf("List pod error: %+v. Retrying, attempt number: %d", err, i)
+		time.Sleep(sleeptime * time.Millisecond)
+	}
+	// We reach here only if there is an error and we have exhausted all retries.
+	// Return the last error
+	return nil, err
 }
 
 // GetLocalIP returns the non loopback local IP of the host


Test patch:
diff --git a/pkg/cloudprovider/identity_test.go b/pkg/cloudprovider/identity_test.go
index 9296bda1..28d0bc72 100644
--- a/pkg/cloudprovider/identity_test.go
+++ b/pkg/cloudprovider/identity_test.go
@@ -120,6 +120,27 @@ func TestAppendUserIdentity(t *testing.T) {
 	if idType != compute.ResourceIdentityTypeUserAssigned {
 		t.Fatalf("expected type %s, got: %s", compute.ResourceIdentityTypeUserAssigned, idType)
 	}
+
+	// test case sensitivity
+	idList = []string{}
+	expect = []string{"/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/devcluster/providers/Microsoft.ManagedIdentity/userAssignedIdentities/keyvault-identity"}
+	append = appendUserIdentity(&idType, &idList, "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/devcluster/providers/Microsoft.ManagedIdentity/userAssignedIdentities/keyvault-identity")
+	if !append {
+		t.Fatalf("Expecting the id to be not present. But present returned by Append.")
+	}
+	checkIDList(t, expect, idList)
+	if idType != compute.ResourceIdentityTypeUserAssigned {
+		t.Fatalf("expected type %s, got: %s", compute.ResourceIdentityTypeUserAssigned, idType)
+	}
+	// append same id with non camel case resourcegroups
+	append = appendUserIdentity(&idType, &idList, "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/devcluster/providers/Microsoft.ManagedIdentity/userAssignedIdentities/keyvault-identity")
+	if append {
+		t.Fatalf("Expecting the id to be present. But not present returned by Append.")
+	}
+	checkIDList(t, expect, idList)
+	if idType != compute.ResourceIdentityTypeUserAssigned {
+		t.Fatalf("expected type %s, got: %s", compute.ResourceIdentityTypeUserAssigned, idType)
+	}
 }
 
 func checkIDList(t *testing.T, expect, actual []string) {