Repository: Azure/aad-pod-identity (go)

Issue:
Generate warning when multiple bindings with same name exists in non namespaced mode
**Describe the request**

When running in non namespaced mode, there is a possibility that `AzureIdentity` and `AzureIdentityBinding` with same name are created across multiple namespaces. We need to generate a warning when this scenario happens and also bind always to 1 of the identities.

**Explain why AAD Pod Identity needs it**

**Describe the solution you'd like**

**Describe alternatives you've considered**

**Additional context**


Pull request:
fix: prevent overwriting of AzureAssignedIdentity when creating it
Signed-off-by: Ernest Wong <chuwon@microsoft.com>

<!-- Thank you for helping AAD Pod Identity with a pull request! Please make sure you read the [contributing guidelines](https://github.com/Azure/aad-pod-identity/blob/master/CONTRIBUTING.md). -->

**Reason for Change**:
<!-- What does this PR improve or fix in AAD Pod Identity? Why is it needed? -->

Fixes #1065 

Deployed two AzureIdentity and AzureIdentityBinding in the default and test namespace with the same name and selector. Unable to repro the issue with this PR:

```
I0623 17:08:49.859982       1 main.go:114] starting mic process. Version: v0.0.0-dev. Build date: 2021-06-23-17:07
W0623 17:08:49.860054       1 main.go:119] --kubeconfig not passed will use InClusterConfig
I0623 17:08:49.860063       1 main.go:136] kubeconfig () cloudconfig (/etc/kubernetes/azure.json)
I0623 17:08:49.860240       1 main.go:144] running MIC in namespaced mode: false
I0623 17:08:49.860250       1 main.go:148] client QPS set to: 5. Burst to: 5
I0623 17:08:49.860269       1 mic.go:138] starting to create the pod identity client. Version: v0.0.0-dev. Build date: 2021-06-23-17:07
I0623 17:08:49.881027       1 mic.go:144] Kubernetes server version: v1.19.9
I0623 17:08:49.881249       1 cloudprovider.go:122] MIC using user assigned identity: af91##### REDACTED #####a608 for authentication.
I0623 17:08:49.947979       1 probes.go:41] initialized health probe on port 8080
I0623 17:08:49.947997       1 probes.go:44] started health probe
I0623 17:08:49.948050       1 metrics.go:341] registered views for metric
I0623 17:08:49.948085       1 prometheus_exporter.go:21] starting Prometheus exporter
I0623 17:08:49.948091       1 metrics.go:347] registered and exported metrics on port 8888
I0623 17:08:49.948094       1 mic.go:242] initiating MIC Leader election
I0623 17:08:49.948101       1 leaderelection.go:243] attempting to acquire leader lease  default/aad-pod-identity-mic...
I0623 17:09:11.179664       1 leaderelection.go:253] successfully acquired lease default/aad-pod-identity-mic
I0623 17:09:11.185024       1 mic.go:330] type upgrade status configmap found from version: v1.8.0. Skipping type upgrade!
I0623 17:09:11.286169       1 crd.go:457] CRD informers started
I0623 17:09:11.585690       1 pod.go:72] pod cache synchronized. Took 400.618104ms
I0623 17:09:11.585783       1 pod.go:79] pod watcher started !!
codespace ➜ /workspaces/aad-pod-identity (azure-identity-duplicated-name ✗) $ k logs aad-pod-identity-mic-5667dbc6cc-x5z2c | head -n 30
I0623 17:08:49.859982       1 main.go:114] starting mic process. Version: v0.0.0-dev. Build date: 2021-06-23-17:07
W0623 17:08:49.860054       1 main.go:119] --kubeconfig not passed will use InClusterConfig
I0623 17:08:49.860063       1 main.go:136] kubeconfig () cloudconfig (/etc/kubernetes/azure.json)
I0623 17:08:49.860240       1 main.go:144] running MIC in namespaced mode: false
I0623 17:08:49.860250       1 main.go:148] client QPS set to: 5. Burst to: 5
I0623 17:08:49.860269       1 mic.go:138] starting to create the pod identity client. Version: v0.0.0-dev. Build date: 2021-06-23-17:07
I0623 17:08:49.881027       1 mic.go:144] Kubernetes server version: v1.19.9
I0623 17:08:49.881249       1 cloudprovider.go:122] MIC using user assigned identity: af91##### REDACTED #####a608 for authentication.
I0623 17:08:49.947979       1 probes.go:41] initialized health probe on port 8080
I0623 17:08:49.947997       1 probes.go:44] started health probe
I0623 17:08:49.948050       1 metrics.go:341] registered views for metric
I0623 17:08:49.948085       1 prometheus_exporter.go:21] starting Prometheus exporter
I0623 17:08:49.948091       1 metrics.go:347] registered and exported metrics on port 8888
I0623 17:08:49.948094       1 mic.go:242] initiating MIC Leader election
I0623 17:08:49.948101       1 leaderelection.go:243] attempting to acquire leader lease  default/aad-pod-identity-mic...
I0623 17:09:11.179664       1 leaderelection.go:253] successfully acquired lease default/aad-pod-identity-mic
I0623 17:09:11.185024       1 mic.go:330] type upgrade status configmap found from version: v1.8.0. Skipping type upgrade!
I0623 17:09:11.286169       1 crd.go:457] CRD informers started
I0623 17:09:11.585690       1 pod.go:72] pod cache synchronized. Took 400.618104ms
I0623 17:09:11.585783       1 pod.go:79] pod watcher started !!
I0623 17:09:11.785220       1 mic.go:400] sync thread started.
W0623 17:09:11.785434       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.785574       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.785671       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.785778       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.785867       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.785995       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.786123       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.786227       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
W0623 17:09:11.786387       1 mic.go:638] AzureIdentity demo exists in both default and test namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)
```

<!--
**Is this a chart or deployment yaml update?**
If yes, please update the yamls in the [manifest_staging/](https://github.com/Azure/aad-pod-identity/tree/master/manifest_staging/) folder, where we host the staging charts and deployment yamls. All the yaml changes will then be promoted into the released charts folder with the next release. Please also add the new configurable values to the configuration [table](https://github.com/Azure/aad-pod-identity/tree/master/manifest_staging/charts/aad-pod-identity#configuration). 
-->

**Requirements**

- [x] squashed commits
- [x] included documentation
- [x] added unit tests and e2e tests (if applicable). See [test standard](https://github.com/Azure/aad-pod-identity/blob/master/CONTRIBUTING.md#test-standard) for more details.
- [x] ran `make precommit`

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->

**Please answer the following questions with yes/no**:

Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?

- [ ] yes
- [x] no

**Notes for Reviewers**:


Patch:
diff --git a/pkg/apis/aadpodidentity/sort.go b/pkg/apis/aadpodidentity/sort.go
new file mode 100644
index 00000000..b45bb513
--- /dev/null
+++ b/pkg/apis/aadpodidentity/sort.go
@@ -0,0 +1,18 @@
+package aadpodidentity
+
+type AzureIdentityBindings []AzureIdentityBinding
+
+func (a AzureIdentityBindings) Len() int {
+	return len(a)
+}
+
+func (a AzureIdentityBindings) Swap(i, j int) {
+	a[i], a[j] = a[j], a[i]
+}
+
+func (a AzureIdentityBindings) Less(i, j int) bool {
+	if a[i].Namespace == a[j].Namespace {
+		return a[i].Name < a[j].Name
+	}
+	return a[i].Namespace < a[j].Namespace
+}
diff --git a/pkg/mic/mic.go b/pkg/mic/mic.go
index 232b74ed..ae57f6a8 100644
--- a/pkg/mic/mic.go
+++ b/pkg/mic/mic.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"reflect"
+	"sort"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -608,6 +609,10 @@ func (c *Client) createDesiredAssignedIdentityList(
 			continue
 		}
 
+		// sort all matching bindings so we can iterate the slice
+		// in an deterministic fashion in different sync cycles
+		sort.Sort(aadpodid.AzureIdentityBindings(matchedBindings))
+
 		for _, binding := range matchedBindings {
 			klog.V(5).Infof("looking up id map: %s/%s", binding.Namespace, binding.Spec.AzureIdentity)
 			if azureID, idPresent := idMap[getIDKey(binding.Namespace, binding.Spec.AzureIdentity)]; idPresent {
@@ -627,7 +632,14 @@ func (c *Client) createDesiredAssignedIdentityList(
 					klog.Errorf("failed to create an AzureAssignedIdentity between pod %s/%s and AzureIdentity %s/%s, error: %+v", pod.Namespace, pod.Name, azureID.Namespace, azureID.Name, err)
 					continue
 				}
-				newAssignedIDs[assignedID.Name] = *assignedID
+
+				if a, ok := newAssignedIDs[assignedID.Name]; ok {
+					// see https://github.com/Azure/aad-pod-identity/issues/1065
+					klog.Warningf("AzureIdentity %s exists in both %s and %s namespace. Considering renaming it or enabling Namespace mode (https://azure.github.io/aad-pod-identity/docs/configure/match_pods_in_namespace)",
+						azureID.Name, a.Spec.AzureIdentityRef.Namespace, azureID.Namespace)
+				} else {
+					newAssignedIDs[assignedID.Name] = *assignedID
+				}
 			} else {
 				// This is the case where the identity has been deleted.
 				// In such a case, we will skip it from matching binding.


Test patch:
diff --git a/.pipelines/templates/e2e-test.yml b/.pipelines/templates/e2e-test.yml
index ed57772a..877eb5ea 100644
--- a/.pipelines/templates/e2e-test.yml
+++ b/.pipelines/templates/e2e-test.yml
@@ -35,14 +35,6 @@ jobs:
         - ${{ if eq(clusterConfig, 'aks') }}:
           - template: deploy-aks-cluster.yml
 
-          - script: |
-              if [[ -n "${GINKGO_SKIP:-}" ]]; then
-                GINKGO_SKIP="${GINKGO_SKIP}|"
-              fi
-              GINKGO_SKIP="${GINKGO_SKIP}should.be.backward.compatible.with.old.and.new.version.of.MIC.and.NMI"
-              echo "##vso[task.setvariable variable=GINKGO_SKIP]${GINKGO_SKIP}"
-            displayName: "Disable backward compatibility test case"
-
         - ${{ if not(eq(clusterConfig, 'aks')) }}:
           - template: deploy-aks-engine-cluster.yml
 
diff --git a/pkg/apis/aadpodidentity/sort_test.go b/pkg/apis/aadpodidentity/sort_test.go
new file mode 100644
index 00000000..c14d85df
--- /dev/null
+++ b/pkg/apis/aadpodidentity/sort_test.go
@@ -0,0 +1,66 @@
+package aadpodidentity
+
+import (
+	"sort"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestSort(t *testing.T) {
+	slice := []AzureIdentityBinding{{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test2",
+			Namespace: "test",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test1",
+			Namespace: "default",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test3",
+			Namespace: "default",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test1",
+			Namespace: "test",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test2",
+			Namespace: "default",
+		},
+	}}
+	expected := []AzureIdentityBinding{{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test1",
+			Namespace: "default",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test2",
+			Namespace: "default",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test3",
+			Namespace: "default",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test1",
+			Namespace: "test",
+		},
+	}, {
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "test2",
+			Namespace: "test",
+		},
+	}}
+	sort.Sort(AzureIdentityBindings(slice))
+	assert.Equal(t, slice, expected)
+}
diff --git a/test/e2e/framework/azureassignedidentity/azureassignedidentity_helpers.go b/test/e2e/framework/azureassignedidentity/azureassignedidentity_helpers.go
index 40437ade..943189c1 100644
--- a/test/e2e/framework/azureassignedidentity/azureassignedidentity_helpers.go
+++ b/test/e2e/framework/azureassignedidentity/azureassignedidentity_helpers.go
@@ -25,7 +25,7 @@ type WaitInput struct {
 }
 
 // Wait waits for an AzureAssignedIdentity to reach a desired state.
-func Wait(input WaitInput) {
+func Wait(input WaitInput) *aadpodv1.AzureAssignedIdentity {
 	Expect(input.Getter).NotTo(BeNil(), "input.Getter is required for AzureAssignedIdentity.Wait")
 	Expect(input.PodName).NotTo(BeEmpty(), "input.PodName is required for AzureAssignedIdentity.Wait")
 	Expect(input.Namespace).NotTo(BeEmpty(), "input.Namespace is required for AzureAssignedIdentity.Wait")
@@ -36,8 +36,8 @@ func Wait(input WaitInput) {
 
 	By(fmt.Sprintf("Ensuring that AzureAssignedIdentity \"%s\" is in %s state", name, input.StateToWaitFor))
 
+	azureAssignedIdentity := &aadpodv1.AzureAssignedIdentity{}
 	Eventually(func() bool {
-		azureAssignedIdentity := &aadpodv1.AzureAssignedIdentity{}
 
 		// AzureAssignedIdentity is always in default namespace unless MIC is in namespaced mode
 		if err := input.Getter.Get(context.TODO(), client.ObjectKey{Name: name, Namespace: corev1.NamespaceDefault}, azureAssignedIdentity); err != nil {
@@ -48,6 +48,8 @@ func Wait(input WaitInput) {
 		}
 		return false
 	}, framework.Timeout, framework.Polling).Should(BeTrue())
+
+	return azureAssignedIdentity
 }
 
 // WaitForLenInput is the input for WaitForLen.
diff --git a/test/e2e/name_conflict_test.go b/test/e2e/name_conflict_test.go
new file mode 100644
index 00000000..029b075b
--- /dev/null
+++ b/test/e2e/name_conflict_test.go
@@ -0,0 +1,119 @@
+// +build e2e
+
+package e2e
+
+import (
+	aadpodv1 "github.com/Azure/aad-pod-identity/pkg/apis/aadpodidentity/v1"
+	"github.com/Azure/aad-pod-identity/test/e2e/framework/azureassignedidentity"
+	"github.com/Azure/aad-pod-identity/test/e2e/framework/azureidentity"
+	"github.com/Azure/aad-pod-identity/test/e2e/framework/azureidentitybinding"
+	"github.com/Azure/aad-pod-identity/test/e2e/framework/identityvalidator"
+	"github.com/Azure/aad-pod-identity/test/e2e/framework/namespace"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+)
+
+// e2e test for regression https://github.com/Azure/aad-pod-identity/issues/1065.
+var _ = Describe("When AAD Pod Identity operations are disrupted", func() {
+	var (
+		specName = "name-conflict"
+		ns1      *corev1.Namespace
+		ns2      *corev1.Namespace
+	)
+
+	BeforeEach(func() {
+		ns1 = namespace.Create(namespace.CreateInput{
+			Creator: kubeClient,
+			Name:    specName,
+		})
+
+		ns2 = namespace.Create(namespace.CreateInput{
+			Creator: kubeClient,
+			Name:    specName,
+		})
+
+		// assume that ns1's name is alphabetically smaller than ns2's name
+		if ns1.Name >= ns2.Name {
+			ns1, ns2 = ns2, ns1
+		}
+	})
+
+	AfterEach(func() {
+		for _, ns := range []*corev1.Namespace{ns1, ns2} {
+			Cleanup(CleanupInput{
+				Namespace: ns,
+				Getter:    kubeClient,
+				Lister:    kubeClient,
+				Deleter:   kubeClient,
+			})
+		}
+	})
+
+	It("should pass the identity validation even when two AzureIdentities and AzureIdentityBindings with the same name are deployed across different namespaces", func() {
+		azureIdentity := azureidentity.Create(azureidentity.CreateInput{
+			Creator:      kubeClient,
+			Config:       config,
+			AzureClient:  azureClient,
+			Name:         keyvaultIdentity,
+			Namespace:    ns1.Name,
+			IdentityType: aadpodv1.UserAssignedMSI,
+			IdentityName: keyvaultIdentity,
+		})
+		azureIdentityBinding := azureidentitybinding.Create(azureidentitybinding.CreateInput{
+			Creator:           kubeClient,
+			Name:              keyvaultIdentityBinding,
+			Namespace:         ns1.Name,
+			AzureIdentityName: azureIdentity.Name,
+			Selector:          keyvaultIdentitySelector,
+		})
+
+		// Create AzureIdentity and AzureIdentityBiniding in ns2 with the same name
+		_ = azureidentity.Create(azureidentity.CreateInput{
+			Creator:      kubeClient,
+			Config:       config,
+			AzureClient:  azureClient,
+			Name:         keyvaultIdentity,
+			Namespace:    ns2.Name,
+			IdentityType: aadpodv1.UserAssignedMSI,
+			IdentityName: keyvaultIdentity,
+		})
+		_ = azureidentitybinding.Create(azureidentitybinding.CreateInput{
+			Creator:           kubeClient,
+			Name:              keyvaultIdentityBinding,
+			Namespace:         ns2.Name,
+			AzureIdentityName: azureIdentity.Name,
+			Selector:          keyvaultIdentitySelector,
+		})
+
+		identityValidator := identityvalidator.Create(identityvalidator.CreateInput{
+			Creator:         kubeClient,
+			Config:          config,
+			Namespace:       ns1.Name,
+			IdentityBinding: azureIdentityBinding.Spec.Selector,
+		})
+
+		azureAssignedIdentity := azureassignedidentity.Wait(azureassignedidentity.WaitInput{
+			Getter:            kubeClient,
+			PodName:           identityValidator.Name,
+			Namespace:         ns1.Name,
+			AzureIdentityName: azureIdentity.Name,
+			StateToWaitFor:    aadpodv1.AssignedIDAssigned,
+		})
+
+		identityvalidator.Validate(identityvalidator.ValidateInput{
+			Getter:             kubeClient,
+			Config:             config,
+			KubeconfigPath:     kubeconfigPath,
+			PodName:            identityValidator.Name,
+			Namespace:          ns1.Name,
+			IdentityClientID:   azureIdentity.Spec.ClientID,
+			IdentityResourceID: azureIdentity.Spec.ResourceID,
+		})
+
+		// ensure that the AzureAssignedIdentity is referencing the AzureIdentity and AzureIdentityBinding from ns1
+		Expect(azureAssignedIdentity.Spec.AzureIdentityRef.Namespace).Should(Equal(ns1.Name))
+		Expect(azureAssignedIdentity.Spec.AzureBindingRef.Namespace).Should(Equal(ns1.Name))
+	})
+})