Repository: Azure/aad-pod-identity
Pull request #318 (2019-07-29 23:36:46)

update e2e test + cleanup
<!-- 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? -->
- Update whitelist e2e test to ensure mixed scenario works as expected
- Add unit tests
- Cleanup
- Add comments 

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->

**Notes for Reviewers**:
Addressing comments from previous PR

aramase: ```bash
Ran 15 of 15 Specs in 2664.251 seconds
SUCCESS! -- 15 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAADPodIdentity (2664.25s)
PASS
ok  	github.com/Azure/aad-pod-identity/test/e2e	2664.280s
```

Patch:
diff --git a/pkg/nmi/server/server.go b/pkg/nmi/server/server.go
index ecae44fa..bf7a5359 100644
--- a/pkg/nmi/server/server.go
+++ b/pkg/nmi/server/server.go
@@ -320,7 +320,7 @@ func (s *Server) msiHandler(logger *log.Entry, w http.ResponseWriter, r *http.Re
 	}
 
 	// If its mic, then just directly get the token and pass back.
-	if pod.IsPodExcepted(selectors.MatchLabels, exceptionList) || s.isMIC(podns, rsName) {
+	if pod.IsPodExcepted(selectors.MatchLabels, *exceptionList) || s.isMIC(podns, rsName) {
 		logger.Infof("Exception pod %s/%s token handling", podns, podname)
 		response, errorCode, err := s.getTokenForExceptedPod(logger, rqClientID, rqResource)
 		if err != nil {
diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go
index 4caa9451..44456a40 100644
--- a/pkg/pod/pod.go
+++ b/pkg/pod/pod.go
@@ -98,16 +98,16 @@ func (c *Client) GetPods() (pods []*corev1.Pod, err error) {
 }
 
 // IsPodExcepted returns true if pod label is part of exception crd
-func IsPodExcepted(podLabels map[string]string, exceptionList *[]aadpodid.AzurePodIdentityException) bool {
-	return exceptionList != nil && len(*exceptionList) > 0 && labelInExceptionList(podLabels, *exceptionList)
+func IsPodExcepted(podLabels map[string]string, exceptionList []aadpodid.AzurePodIdentityException) bool {
+	return len(exceptionList) > 0 && labelInExceptionList(podLabels, exceptionList)
 }
 
 // labelInExceptionList checks if the labels defined in azurepodidentityexception match label defined in pods
 func labelInExceptionList(podLabels map[string]string, exceptionList []aadpodid.AzurePodIdentityException) bool {
-	for _, podIdentityException := range exceptionList {
-		for exceptionLabelKey := range podIdentityException.Spec.PodLabels {
+	for _, exception := range exceptionList {
+		for exceptionLabelKey, exceptionLabelValue := range exception.Spec.PodLabels {
 			if val, ok := podLabels[exceptionLabelKey]; ok {
-				if strings.EqualFold(val, podLabels[exceptionLabelKey]) {
+				if strings.EqualFold(val, exceptionLabelValue) {
 					return true
 				}
 			}


Test patch:
diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go
index b3fb3dfd..93961929 100644
--- a/pkg/pod/pod_test.go
+++ b/pkg/pod/pod_test.go
@@ -1,11 +1,13 @@
 package pod
 
 import (
+	"testing"
+
 	aadpodid "github.com/Azure/aad-pod-identity/pkg/apis/aadpodidentity/v1"
 
 	"github.com/golang/glog"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 type TestPodClient struct {
@@ -59,3 +61,60 @@ func (c *TestPodClient) DeletePod(podName string, podNs string) {
 		c.pods = newPods
 	}
 }
+
+func TestIsPodExcepted(t *testing.T) {
+	cases := []struct {
+		podLabels        map[string]string
+		exceptionList    []aadpodid.AzurePodIdentityException
+		shouldBeExcepted bool
+	}{
+		{
+			podLabels:        map[string]string{"foo": "bar"},
+			exceptionList:    nil,
+			shouldBeExcepted: false,
+		},
+		{
+			podLabels: map[string]string{"foo": "except"},
+			exceptionList: []aadpodid.AzurePodIdentityException{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "exception1",
+					},
+					Spec: aadpodid.AzurePodIdentityExceptionSpec{
+						PodLabels: map[string]string{"foo": "notexcept"},
+					},
+				},
+			},
+			shouldBeExcepted: false,
+		},
+		{
+			podLabels: map[string]string{"foo": "except"},
+			exceptionList: []aadpodid.AzurePodIdentityException{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "exception1",
+					},
+					Spec: aadpodid.AzurePodIdentityExceptionSpec{
+						PodLabels: map[string]string{"foo": "notexcept"},
+					},
+				},
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name: "exception2",
+					},
+					Spec: aadpodid.AzurePodIdentityExceptionSpec{
+						PodLabels: map[string]string{"foo": "except"},
+					},
+				},
+			},
+			shouldBeExcepted: true,
+		},
+	}
+
+	for _, tc := range cases {
+		isExcepted := IsPodExcepted(tc.podLabels, tc.exceptionList)
+		if isExcepted != tc.shouldBeExcepted {
+			t.Fatalf("expected: %v, got %v", tc.shouldBeExcepted, isExcepted)
+		}
+	}
+}
diff --git a/test/common/k8s/infra/infra.go b/test/common/k8s/infra/infra.go
index ded19719..c1ffd70f 100644
--- a/test/common/k8s/infra/infra.go
+++ b/test/common/k8s/infra/infra.go
@@ -73,6 +73,7 @@ type IdentityValidatorTemplateData struct {
 	IdentityValidatorVersion string
 	NodeName                 string
 	Replicas                 string
+	DeploymentLabels         map[string]string
 }
 
 // CreateIdentityValidator will create an identity validator deployment on a Kubernetes cluster
diff --git a/test/e2e/aadpodidentity_test.go b/test/e2e/aadpodidentity_test.go
index 2bdcbf82..33498c90 100644
--- a/test/e2e/aadpodidentity_test.go
+++ b/test/e2e/aadpodidentity_test.go
@@ -708,42 +708,94 @@ var _ = Describe("Kubernetes cluster using aad-pod-identity", func() {
 	})
 
 	It("should pass validation by bypassing nmi using azurepodidentityexception crd", func() {
+		// Creates 2 pods with labels defined in AzurePodIdentityException
+		// Creates 1 pod that needs to go through pod-identity
+		// Validates the mixed scenario works as expected.
+
 		// Assign user assigned identity to every node
 		nodeList, err := node.GetAll()
 		Expect(err).NotTo(HaveOccurred())
 
-		enableUserAssignedIdentityOnCluster(nodeList, keyvaultIdentity)
+		enableUserAssignedIdentityOnCluster(nodeList, fmt.Sprintf("%s-%d", keyvaultIdentity, 1))
+		enableUserAssignedIdentityOnCluster(nodeList, fmt.Sprintf("%s-%d", keyvaultIdentity, 2))
+
+		enableSystemAssignedIdentityOnCluster(nodeList)
+
+		err = azurepodidentityexception.Create(fmt.Sprintf("%s-%d", identityValidator, 1), templateOutputPath, map[string]string{"thispod": "shouldexcept"})
+		Expect(err).NotTo(HaveOccurred())
 
-		err = azurepodidentityexception.Create(identityValidator, templateOutputPath, map[string]string{"app": "identity-validator"})
+		err = azurepodidentityexception.Create(fmt.Sprintf("%s-%d", identityValidator, 2), templateOutputPath, map[string]string{"thispod": "alsoshouldexcept"})
 		Expect(err).NotTo(HaveOccurred())
 
 		data := infra.IdentityValidatorTemplateData{
-			Name:                     identityValidator,
+			Name:                     fmt.Sprintf("%s-%d", identityValidator, 1),
+			IdentityBinding:          "random",
+			Registry:                 cfg.Registry,
+			IdentityValidatorVersion: cfg.IdentityValidatorVersion,
+			Replicas:                 "1",
+			DeploymentLabels:         map[string]string{"thispod": "shouldexcept"},
+		}
+
+		err = infra.CreateIdentityValidator(cfg.SubscriptionID, cfg.ResourceGroup, templateOutputPath, data)
+		Expect(err).NotTo(HaveOccurred())
+
+		data = infra.IdentityValidatorTemplateData{
+			Name:                     fmt.Sprintf("%s-%d", identityValidator, 2),
 			IdentityBinding:          "random",
 			Registry:                 cfg.Registry,
 			IdentityValidatorVersion: cfg.IdentityValidatorVersion,
 			Replicas:                 "1",
+			DeploymentLabels:         map[string]string{"thispod": "alsoshouldexcept"},
 		}
 
 		err = infra.CreateIdentityValidator(cfg.SubscriptionID, cfg.ResourceGroup, templateOutputPath, data)
 		Expect(err).NotTo(HaveOccurred())
 
-		ok, err := deploy.WaitOnReady(identityValidator)
+		ok, err := deploy.WaitOnReady(fmt.Sprintf("%s-%d", identityValidator, 1))
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ok).To(Equal(true))
 
-		ok, err = azureassignedidentity.WaitOnLengthMatched(0)
+		ok, err = deploy.WaitOnReady(fmt.Sprintf("%s-%d", identityValidator, 2))
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ok).To(Equal(true))
 
-		podName, err := pod.GetNameByPrefix(identityValidator)
+		setUpIdentityAndDeployment(keyvaultIdentity, "0", "1")
+
+		ok, err = azureassignedidentity.WaitOnLengthMatched(1)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(podName).NotTo(Equal(""))
+		Expect(ok).To(Equal(true))
 
-		identityClientID, err := azure.GetIdentityClientID(cfg.ResourceGroup, keyvaultIdentity)
+		// This pod should go through the nmi as it has the aadpodidbinding label and doesn't contain exception crd
+		podName0, err := pod.GetNameByPrefix(fmt.Sprintf("%s-%d", identityValidator, 0))
 		Expect(err).NotTo(HaveOccurred())
+		Expect(podName0).NotTo(Equal(""))
 
-		cmdOutput, err := validateUserAssignedIdentityOnPod(podName, identityClientID)
+		identityClientID, err := azure.GetIdentityClientID(cfg.ResourceGroup, fmt.Sprintf("%s-%d", keyvaultIdentity, 0))
+		Expect(err).NotTo(HaveOccurred())
+
+		cmdOutput, err := validateUserAssignedIdentityOnPod(podName0, identityClientID)
+		Expect(errors.Wrap(err, string(cmdOutput))).NotTo(HaveOccurred())
+
+		// Pod1 and Pod2 have labels matching labels defined in exception crds. So NMI should proxy the request as is
+		// and send the token back without any validation.
+		podName1, err := pod.GetNameByPrefix(fmt.Sprintf("%s-%d", identityValidator, 1))
+		Expect(err).NotTo(HaveOccurred())
+		Expect(podName1).NotTo(Equal(""))
+
+		identityClientID, err = azure.GetIdentityClientID(cfg.ResourceGroup, fmt.Sprintf("%s-%d", keyvaultIdentity, 1))
+		Expect(err).NotTo(HaveOccurred())
+
+		cmdOutput, err = validateUserAssignedIdentityOnPod(podName1, identityClientID)
+		Expect(errors.Wrap(err, string(cmdOutput))).NotTo(HaveOccurred())
+
+		podName2, err := pod.GetNameByPrefix(fmt.Sprintf("%s-%d", identityValidator, 2))
+		Expect(err).NotTo(HaveOccurred())
+		Expect(podName2).NotTo(Equal(""))
+
+		identityClientID, err = azure.GetIdentityClientID(cfg.ResourceGroup, fmt.Sprintf("%s-%d", keyvaultIdentity, 2))
+		Expect(err).NotTo(HaveOccurred())
+
+		cmdOutput, err = validateUserAssignedIdentityOnPod(podName2, identityClientID)
 		Expect(errors.Wrap(err, string(cmdOutput))).NotTo(HaveOccurred())
 	})
 
diff --git a/test/e2e/identityvalidator/identityvalidator.go b/test/e2e/identityvalidator/identityvalidator.go
index 66450eb0..1a34d2bf 100644
--- a/test/e2e/identityvalidator/identityvalidator.go
+++ b/test/e2e/identityvalidator/identityvalidator.go
@@ -87,8 +87,12 @@ func testClusterWideUserAssignedIdentity(logger *log.Entry, msiEndpoint, subscri
 
 // testUserAssignedIdentityOnPod will verify whether a pod identity is working properly
 func testUserAssignedIdentityOnPod(logger *log.Entry, msiEndpoint, identityClientID, keyvaultName, keyvaultSecretName, keyvaultSecretVersion string) error {
+	// When new authorizer is created, azure-sdk-for-go  tries to create dataplane authorizer using MSI. It checks the AZURE_CLIENT_ID to get the client id
+	// for the user assigned identity. If client id not found, then NewServicePrincipalTokenFromMSI is invoked instead of using the actual
+	// user assigned identity. Setting this env var ensures we validate GetSecret using the desired user assigned identity.
 	os.Setenv("AZURE_CLIENT_ID", identityClientID)
 	defer os.Unsetenv("AZURE_CLIENT_ID")
+
 	keyClient := keyvault.New()
 	authorizer, err := auth.NewAuthorizerFromEnvironment()
 	if err == nil {
diff --git a/test/e2e/template/deployment.yaml b/test/e2e/template/deployment.yaml
index 68107797..5ae8a75d 100644
--- a/test/e2e/template/deployment.yaml
+++ b/test/e2e/template/deployment.yaml
@@ -4,6 +4,9 @@ metadata:
   labels:
     app: identity-validator
     aadpodidbinding: {{.IdentityBinding}}
+    {{- range $key, $value := $.DeploymentLabels }}
+    {{ $key }}: {{ $value -}}
+    {{ end }}
   name: {{.Name}}
   namespace: default
 spec:
@@ -13,6 +16,9 @@ spec:
       labels:
         app: identity-validator
         aadpodidbinding: {{.IdentityBinding}}
+        {{- range $key, $value := $.DeploymentLabels }}
+        {{ $key }}: {{ $value -}}
+        {{ end }}
     spec:
       {{ if ne .NodeName "" -}}
       nodeName: {{.NodeName}}