From 8985b372269df93368463e21c0d15eab808c861c Mon Sep 17 00:00:00 2001 From: Ernest Wong Date: Thu, 24 Jun 2021 11:07:19 -0700 Subject: [PATCH] fix: prevent overwriting of AzureAssignedIdentity when creating it (#1100) Signed-off-by: GitHub --- .pipelines/templates/e2e-test.yml | 8 -- pkg/apis/aadpodidentity/sort.go | 18 +++ pkg/apis/aadpodidentity/sort_test.go | 66 ++++++++++ pkg/mic/mic.go | 14 ++- .../azureassignedidentity_helpers.go | 6 +- test/e2e/name_conflict_test.go | 119 ++++++++++++++++++ 6 files changed, 220 insertions(+), 11 deletions(-) create mode 100644 pkg/apis/aadpodidentity/sort.go create mode 100644 pkg/apis/aadpodidentity/sort_test.go create mode 100644 test/e2e/name_conflict_test.go 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.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/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/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. 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)) + }) +})