Repository: Azure/aad-pod-identity
Pull request #388 (2019-09-19 01:01:01)

Fix namespaced multiple binding/identity handling and verbose logs
In forcenamespaced configuration multiple bindings or identities could exists with the same name in different namespaces. The last binding which matched was used to create the assigned identity representing current state. This PR fixes the issue by using the right bindings and identities.

The map used for handling of identities did not consider the namespace. This PR changes the key of the map to a combination of namespace and name of the identity.

<!-- 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? -->


**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->

**Notes for Reviewers**:



Patch:
diff --git a/pkg/mic/mic.go b/pkg/mic/mic.go
index d2d8f5fc..028f5cf7 100644
--- a/pkg/mic/mic.go
+++ b/pkg/mic/mic.go
@@ -386,7 +386,7 @@ func (c *Client) createDesiredAssignedIdentityList(
 		var matchedBindings []aadpodid.AzureIdentityBinding
 		for _, allBinding := range *listBindings {
 			if allBinding.Spec.Selector == crdPodLabelVal {
-				glog.V(5).Infof("Found binding match for pod %s/%s with binding %s", pod.Namespace, pod.Name, allBinding.Name)
+				glog.V(5).Infof("Found binding match for pod %s/%s with binding %s/%s", pod.Namespace, pod.Name, allBinding.Namespace, allBinding.Name)
 				matchedBindings = append(matchedBindings, allBinding)
 				nodeRefs[pod.Spec.NodeName] = true
 			}
@@ -394,7 +394,7 @@ func (c *Client) createDesiredAssignedIdentityList(
 
 		for _, binding := range matchedBindings {
 			glog.V(5).Infof("Looking up id map: %v", binding.Spec.AzureIdentity)
-			if azureID, idPresent := idMap[binding.Spec.AzureIdentity]; idPresent {
+			if azureID, idPresent := idMap[getIDKey(binding.Namespace, binding.Spec.AzureIdentity)]; idPresent {
 				// working in Namespaced mode or this specific identity is namespaced
 				if c.IsNamespaced || aadpodid.IsNamespacedIdentity(&azureID) {
 					// They have to match all
@@ -404,8 +404,9 @@ func (c *Client) createDesiredAssignedIdentityList(
 						continue
 					}
 				}
-				glog.V(5).Infof("identity %s/%s assigned to %s/%s via %s/%s", azureID.Namespace, azureID.Name, pod.Namespace, pod.Name, binding.Namespace, binding.Namespace)
-				assignedID, err := c.makeAssignedIDs(&azureID, &binding, pod.Name, pod.Namespace, pod.Spec.NodeName)
+				glog.V(5).Infof("identity %s/%s assigned to %s/%s via %s/%s", azureID.Namespace, azureID.Name, pod.Namespace, pod.Name, binding.Namespace, binding.Name)
+
+				assignedID, err := c.makeAssignedIDs(azureID, binding, pod.Name, pod.Namespace, pod.Spec.NodeName)
 
 				if err != nil {
 					glog.Errorf("failed to create assignment for pod %s/%s with identity %s/%s with error %v", pod.Namespace, pod.Name, azureID.Namespace, azureID.Name, err.Error())
@@ -474,6 +475,15 @@ func (c *Client) matchAssignedID(x *aadpodid.AzureAssignedIdentity, y *aadpodid.
 	idX := x.Spec.AzureIdentityRef
 	idY := y.Spec.AzureIdentityRef
 
+	glog.V(6).Infof("assignedidX - %+v\n", x)
+	glog.V(6).Infof("assignedidY - %+v\n", y)
+
+	glog.V(6).Infof("bindingX - %+v\n", bindingX)
+	glog.V(6).Infof("bindingY - %+v\n", bindingY)
+
+	glog.V(6).Infof("idX - %+v\n", idX)
+	glog.V(6).Infof("idY - %+v\n", idY)
+
 	if bindingX.Name == bindingY.Name && bindingX.ResourceVersion == bindingY.ResourceVersion &&
 		idX.Name == idY.Name && idX.ResourceVersion == idY.ResourceVersion &&
 		x.Spec.Pod == y.Spec.Pod && x.Spec.PodNamespace == y.Spec.PodNamespace && x.Spec.NodeName == y.Spec.NodeName {
@@ -561,14 +571,17 @@ func (c *Client) getAzureAssignedIDsToDelete(old *[]aadpodid.AzureAssignedIdenti
 	return &delete, nil
 }
 
-func (c *Client) makeAssignedIDs(azID *aadpodid.AzureIdentity, azBinding *aadpodid.AzureIdentityBinding, podName, podNameSpace, nodeName string) (res *aadpodid.AzureAssignedIdentity, err error) {
+func (c *Client) makeAssignedIDs(azID aadpodid.AzureIdentity, azBinding aadpodid.AzureIdentityBinding, podName, podNameSpace, nodeName string) (res *aadpodid.AzureAssignedIdentity, err error) {
+	binding := azBinding
+	id := azID
+
 	assignedID := &aadpodid.AzureAssignedIdentity{
 		ObjectMeta: v1.ObjectMeta{
 			Name: c.getAssignedIDName(podName, podNameSpace, azID.Name),
 		},
 		Spec: aadpodid.AzureAssignedIdentitySpec{
-			AzureIdentityRef: azID,
-			AzureBindingRef:  azBinding,
+			AzureIdentityRef: &id,
+			AzureBindingRef:  &binding,
 			Pod:              podName,
 			PodNamespace:     podNameSpace,
 			NodeName:         nodeName,
@@ -578,7 +591,7 @@ func (c *Client) makeAssignedIDs(azID *aadpodid.AzureIdentity, azBinding *aadpod
 		},
 	}
 	// if we are in namespaced mode (or az identity is namespaced)
-	if c.IsNamespaced || aadpodid.IsNamespacedIdentity(azID) {
+	if c.IsNamespaced || aadpodid.IsNamespacedIdentity(&azID) {
 		assignedID.Namespace = azID.Namespace
 	} else {
 		// eventually this should be identity namespace
@@ -587,7 +600,8 @@ func (c *Client) makeAssignedIDs(azID *aadpodid.AzureIdentity, azBinding *aadpod
 		assignedID.Namespace = "default"
 	}
 
-	glog.V(5).Infof("Making assigned ID: %v", assignedID)
+	glog.V(6).Infof("Binding - %+v Identity - %+v", azBinding, azID)
+	glog.V(5).Infof("Making assigned ID: %+v", assignedID)
 	return assignedID, nil
 }
 
@@ -646,10 +660,14 @@ func (c *Client) getUserMSIListForNode(nodeOrVMSSName string, isvmss bool) ([]st
 	return c.CloudClient.GetUserMSIs(nodeOrVMSSName, isvmss)
 }
 
+func getIDKey(ns, name string) string {
+	return strings.Join([]string{ns, name}, "/")
+}
+
 func (c *Client) convertIDListToMap(arr []aadpodid.AzureIdentity) (m map[string]aadpodid.AzureIdentity, err error) {
 	m = make(map[string]aadpodid.AzureIdentity, len(arr))
 	for _, element := range arr {
-		m[element.Name] = element
+		m[getIDKey(element.Namespace, element.Name)] = element
 	}
 	return m, nil
 }


Test patch:
diff --git a/pkg/mic/mic_test.go b/pkg/mic/mic_test.go
index b33d17e6..f08c8387 100644
--- a/pkg/mic/mic_test.go
+++ b/pkg/mic/mic_test.go
@@ -598,42 +598,51 @@ func TestMapMICClient(t *testing.T) {
 	idList := make([]aadpodid.AzureIdentity, 0)
 
 	id := new(aadpodid.AzureIdentity)
+	id.Namespace = "default"
 	id.Name = "test-azure-identity"
 
 	idList = append(idList, *id)
 
+	id.Namespace = "newns"
 	id.Name = "test-akssvcrg-id"
+
 	idList = append(idList, *id)
 
 	idMap, _ := micClient.convertIDListToMap(idList)
 
+	namespace := "default"
 	name := "test-azure-identity"
 	count := 3
-	if azureID, idPresent := idMap[name]; idPresent {
+	if azureID, idPresent := idMap[getIDKey(namespace, name)]; idPresent {
 		if azureID.Name != name {
-			t.Errorf("id map id value mismatch")
+			t.Fatalf("id map id value mismatch")
 		}
 		count = count - 1
+	} else {
+		t.Fatalf("id %s not found", name)
 	}
 
+	namespace = "newns"
 	name = "test-akssvcrg-id"
-	if azureID, idPresent := idMap[name]; idPresent {
+	if azureID, idPresent := idMap[getIDKey(namespace, name)]; idPresent {
 		if azureID.Name != name {
-			t.Errorf("id map id value mismatch")
+			t.Fatalf("id map id value mismatch")
 		}
 		count = count - 1
+	} else {
+		t.Fatalf("id %s not found", name)
 	}
 
+	namespace = "default"
 	name = "test not there"
-	if _, idPresent := idMap[name]; idPresent {
-		t.Errorf("not present found")
+	if _, idPresent := idMap[getIDKey(namespace, name)]; idPresent {
+		t.Fatalf("not present found")
 	} else {
 		count = count - 1
 	}
 	if count != 0 {
-		t.Errorf("Test count mismatch")
+		t.Fatalf("Test count mismatch - count %d", count)
 	}
-
 }
 
 func (c *TestMICClient) testRunSync() func(t *testing.T) {
diff --git a/pkg/stats/stats_test.go b/pkg/stats/stats_test.go
index 365d6f51..c3c11fb2 100644
--- a/pkg/stats/stats_test.go
+++ b/pkg/stats/stats_test.go
@@ -1,7 +1,6 @@
 package stats_test
 
 import (
-	"fmt"
 	"reflect"
 	"sync"
 	"testing"
@@ -49,14 +48,14 @@ func TestConcurrency(t *testing.T) {
 		go func(c int) {
 			defer wg.Done()
 			startWg.Wait()
-			fmt.Printf("Updating %d \n", c)
+			//fmt.Printf("Updating %d \n", c)
 			stats.Update(stats.AssignedIDList, duration)
 		}(i)
 		wg.Add(1)
 		go func(c int) {
 			defer wg.Done()
 			startWg.Wait()
-			fmt.Printf("Getting %d\n", c)
+			//fmt.Printf("Getting %d\n", c)
 			stats.Get(stats.AssignedIDList)
 		}(i)
 	}