fix: prevent overwriting of AzureAssignedIdentity when creating it (#1100)

Signed-off-by: GitHub <noreply@github.com>

.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
 

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
+}

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)
+}

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/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.

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))
+	})
+})