From 4db42d5d53b9e744cc8744793b8f7b48246623c7 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Thu, 26 Oct 2023 16:47:32 -0400 Subject: [PATCH] add managed-by label checks for spc, placeholder pods and ingress backend (#111) --- .../keyvault/ingress_secret_provider_class.go | 19 +- .../ingress_secret_provider_class_test.go | 186 ++++++++++++++---- pkg/controller/keyvault/placeholder_pod.go | 21 +- .../keyvault/placeholder_pod_test.go | 178 ++++++++++++++--- .../osm/ingress_backend_reconciler.go | 37 ++-- .../osm/ingress_backend_reconciler_test.go | 86 +++++++- pkg/manifests/common.go | 21 +- pkg/manifests/common_test.go | 19 ++ 8 files changed, 468 insertions(+), 99 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index df2e24b4..ca34ae81 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -10,7 +10,6 @@ import ( "net/url" "strings" - "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" "github.com/go-logr/logr" netv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -20,7 +19,9 @@ import ( secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" + "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/Azure/aks-app-routing-operator/pkg/util" kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" ) @@ -91,6 +92,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("keyvault-%s", ing.Name), Namespace: ing.Namespace, + Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: ing.APIVersion, Controller: util.BoolPtr(true), @@ -115,14 +117,21 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re logger.Info("cleaning unused managed spc for ingress") logger.Info("getting secret provider class for ingress") - err = i.client.Get(ctx, client.ObjectKeyFromObject(spc), spc) + + toCleanSPC := &secv1.SecretProviderClass{} + + err = i.client.Get(ctx, client.ObjectKeyFromObject(spc), toCleanSPC) if err != nil { return result, client.IgnoreNotFound(err) } - logger.Info("removing secret provider class for ingress") - err = i.client.Delete(ctx, spc) - return result, err + if manifests.HasTopLevelLabels(toCleanSPC.Labels) { + logger.Info("removing secret provider class for ingress") + err = i.client.Delete(ctx, toCleanSPC) + return result, client.IgnoreNotFound(err) + } + + return result, nil } func (i *IngressSecretProviderClassReconciler) buildSPC(ing *netv1.Ingress, spc *secv1.SecretProviderClass) (bool, error) { diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index 2d38b6c8..d386fd5d 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -9,8 +9,6 @@ import ( "net/url" "testing" - "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" - "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -26,18 +24,31 @@ import ( "github.com/Azure/aks-app-routing-operator/pkg/config" "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" + "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" + "github.com/Azure/aks-app-routing-operator/pkg/manifests" + "github.com/Azure/aks-app-routing-operator/pkg/util" kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" ) -func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { - ing := &netv1.Ingress{} - ing.Name = "test-ingress" - ing.Namespace = "default" - ingressClass := "webapprouting.kubernetes.azure.com" - ing.Spec.IngressClassName = &ingressClass - ing.Annotations = map[string]string{ - "kubernetes.azure.com/tls-cert-keyvault-uri": "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34", +var ( + spcTestIngressClassName = "webapprouting.kubernetes.azure.com" + spcTestIngress = &netv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ingress", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.azure.com/tls-cert-keyvault-uri": "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34", + }, + }, + Spec: netv1.IngressSpec{ + IngressClassName: &spcTestIngressClassName, + }, } +) + +func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { + // Create the ingress + ing := spcTestIngress.DeepCopy() c := fake.NewClientBuilder().WithObjects(ing).Build() require.NoError(t, secv1.AddToScheme(c.Scheme())) @@ -47,7 +58,7 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { TenantID: "test-tenant-id", MSIClientID: "test-msi-client-id", }, - ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}), + ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}), } ctx := context.Background() @@ -67,6 +78,7 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { spc := &secv1.SecretProviderClass{} spc.Name = "keyvault-" + ing.Name spc.Namespace = ing.Namespace + spc.Labels = manifests.GetTopLevelLabels() require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc)) expected := &secv1.SecretProviderClass{ @@ -138,12 +150,114 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) } +func TestIngressSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testing.T) { + // Create the ingress + ing := spcTestIngress.DeepCopy() + + c := fake.NewClientBuilder().WithObjects(ing).Build() + require.NoError(t, secv1.AddToScheme(c.Scheme())) + i := &IngressSecretProviderClassReconciler{ + client: c, + config: &config.Config{ + TenantID: "test-tenant-id", + MSIClientID: "test-msi-client-id", + }, + ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}), + } + + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + + // Create the secret provider class + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: ing.Namespace, Name: ing.Name}} + beforeErrCount := testutils.GetErrMetricCount(t, ingressSecretProviderControllerName) + beforeRequestCount := testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess) + _, err := i.Reconcile(ctx, req) + require.NoError(t, err) + + require.Equal(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + spc := &secv1.SecretProviderClass{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "secrets-store.csi.x-k8s.io/v1", + Kind: "SecretProviderClass", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("keyvault-%s", ing.Name), + Namespace: ing.Namespace, + Labels: manifests.GetTopLevelLabels(), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: ing.APIVersion, + Controller: util.BoolPtr(true), + Kind: ing.Kind, + Name: ing.Name, + UID: ing.UID, + }}, + }, + } + + // Get secret provider class + require.False(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(spc), spc))) + assert.Equal(t, len(manifests.GetTopLevelLabels()), len(spc.Labels)) + + // Remove the labels + spc.Labels = map[string]string{} + require.NoError(t, i.client.Update(ctx, spc)) + assert.Equal(t, 0, len(spc.Labels)) + + // Remove the cert annotation from the ingress + ing.Annotations = map[string]string{} + require.NoError(t, i.client.Update(ctx, ing)) + + // Reconcile both changes + beforeErrCount = testutils.GetErrMetricCount(t, ingressSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess) + _, err = i.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + // Prove secret class was not removed + require.False(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(spc), spc))) + assert.Equal(t, 0, len(spc.Labels)) + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc)) + + // Check secret provider class Spec after Reconcile + expected := &secv1.SecretProviderClass{ + Spec: secv1.SecretProviderClassSpec{ + Provider: "azure", + Parameters: map[string]string{ + "keyvaultName": "testvault", + "objects": "{\"array\":[\"{\\\"objectName\\\":\\\"testcert\\\",\\\"objectType\\\":\\\"secret\\\",\\\"objectVersion\\\":\\\"f8982febc6894c0697b884f946fb1a34\\\"}\"]}", + "tenantId": i.config.TenantID, + "useVMManagedIdentity": "true", + "userAssignedIdentityID": i.config.MSIClientID, + }, + SecretObjects: []*secv1.SecretObject{{ + SecretName: spc.Name, + Type: "kubernetes.io/tls", + Data: []*secv1.SecretObjectData{ + {ObjectName: "testcert", Key: "tls.key"}, + {ObjectName: "testcert", Key: "tls.crt"}, + }, + }}, + }, + } + assert.Equal(t, expected.Spec, spc.Spec) + + // Check for idempotence + beforeErrCount = testutils.GetErrMetricCount(t, ingressSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess) + _, err = i.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) +} + func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) { - ing := &netv1.Ingress{} - ing.Name = "test-ingress" - ing.Namespace = "default" - ingressClass := "webapprouting.kubernetes.azure.com" - ing.Spec.IngressClassName = &ingressClass + // Create the ingress + ing := spcTestIngress.DeepCopy() ing.Annotations = map[string]string{ "kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL", } @@ -158,7 +272,7 @@ func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) { MSIClientID: "test-msi-client-id", }, events: recorder, - ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}), + ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}), } ctx := context.Background() @@ -184,17 +298,18 @@ func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) { } func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { - ingressClass := "webapprouting.kubernetes.azure.com" - i := &IngressSecretProviderClassReconciler{ - ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}), + ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}), } - ing := &netv1.Ingress{} - ing.Spec.IngressClassName = &ingressClass + invalidURLIng := &netv1.Ingress{ + Spec: netv1.IngressSpec{ + IngressClassName: &spcTestIngressClassName, + }, + } t.Run("missing ingress class", func(t *testing.T) { - ing := ing.DeepCopy() + ing := invalidURLIng.DeepCopy() ing.Spec.IngressClassName = nil ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL"} @@ -204,7 +319,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { }) t.Run("incorrect ingress class", func(t *testing.T) { - ing := ing.DeepCopy() + ing := invalidURLIng.DeepCopy() incorrect := "some-other-ingress-class" ing.Spec.IngressClassName = &incorrect ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL"} @@ -215,7 +330,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { }) t.Run("nil annotations", func(t *testing.T) { - ing := ing.DeepCopy() + ing := invalidURLIng.DeepCopy() ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) assert.False(t, ok) @@ -223,7 +338,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { }) t.Run("empty url", func(t *testing.T) { - ing := ing.DeepCopy() + ing := invalidURLIng.DeepCopy() ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": ""} ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) @@ -232,7 +347,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { }) t.Run("url with control character", func(t *testing.T) { - ing := ing.DeepCopy() + ing := invalidURLIng.DeepCopy() cc := string([]byte{0x7f}) ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": cc} @@ -243,7 +358,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { }) t.Run("url with one path segment", func(t *testing.T) { - ing := ing.DeepCopy() + ing := invalidURLIng.DeepCopy() ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "http://test.com/foo"} ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) @@ -278,23 +393,16 @@ func TestIngressSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - ingressClass := "webapprouting.kubernetes.azure.com" i := &IngressSecretProviderClassReconciler{ config: &config.Config{ Cloud: c.configCloud, }, - ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}), + ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}), } - ing := &netv1.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "kubernetes.azure.com/tls-cert-keyvault-uri": "https://test.vault.azure.net/secrets/test-secret", - }, - }, - Spec: netv1.IngressSpec{ - IngressClassName: &ingressClass, - }, + ing := spcTestIngress.DeepCopy() + ing.Annotations = map[string]string{ + "kubernetes.azure.com/tls-cert-keyvault-uri": "https://test.vault.azure.net/secrets/test-secret", } spc := &secv1.SecretProviderClass{} diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index f251a81c..c0e5e28b 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -8,7 +8,6 @@ import ( "path" "strconv" - "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -20,6 +19,7 @@ import ( secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/Azure/aks-app-routing-operator/pkg/util" @@ -87,6 +87,7 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque ObjectMeta: metav1.ObjectMeta{ Name: spc.Name, Namespace: spc.Namespace, + Labels: spc.Labels, OwnerReferences: []metav1.OwnerReference{{ APIVersion: spc.APIVersion, Controller: util.BoolPtr(true), @@ -110,19 +111,22 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } managed := p.ingressManager.IsManaging(ing) + if ing.Name == "" || ing.Spec.IngressClassName == nil || !managed { logger.Info("cleaning unused placeholder pod deployment") logger.Info("getting placeholder deployment") - if err = p.client.Get(ctx, client.ObjectKeyFromObject(dep), dep); err != nil { + + toCleanDeployment := &appsv1.Deployment{} + if err = p.client.Get(ctx, client.ObjectKeyFromObject(dep), toCleanDeployment); err != nil { + return result, client.IgnoreNotFound(err) + } + if manifests.HasTopLevelLabels(toCleanDeployment.Labels) { + logger.Info("deleting placeholder deployment") + err = p.client.Delete(ctx, toCleanDeployment) return result, client.IgnoreNotFound(err) } - - logger.Info("deleting placeholder deployment") - err = p.client.Delete(ctx, dep) - return result, client.IgnoreNotFound(err) } - // Manage a deployment resource logger.Info("reconciling placeholder deployment for secret provider class") p.buildDeployment(dep, spc, ing) @@ -134,7 +138,8 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } func (p *PlaceholderPodController) buildDeployment(dep *appsv1.Deployment, spc *secv1.SecretProviderClass, ing *netv1.Ingress) { - labels := map[string]string{"app": spc.Name} + labels := util.MergeMaps(map[string]string{"app": spc.Name}, manifests.GetTopLevelLabels()) + dep.Spec = appsv1.DeploymentSpec{ Replicas: util.Int32Ptr(1), RevisionHistoryLimit: util.Int32Ptr(2), diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 8b2db6bf..d4a861ae 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -5,20 +5,15 @@ package keyvault import ( "context" + "k8s.io/apimachinery/pkg/api/errors" "testing" - "github.com/Azure/aks-app-routing-operator/pkg/config" - "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" - "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" - "github.com/Azure/aks-app-routing-operator/pkg/manifests" - "github.com/Azure/aks-app-routing-operator/pkg/util" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" netv1 "k8s.io/api/networking/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -28,30 +23,50 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" + + "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" + "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" + "github.com/Azure/aks-app-routing-operator/pkg/manifests" + "github.com/Azure/aks-app-routing-operator/pkg/util" +) + +var ( + placeholderTestIngClassName = "webapprouting.kubernetes.azure.com" + placeholderTestIng = &netv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ing", + Namespace: "default", + }, + Spec: netv1.IngressSpec{ + IngressClassName: &placeholderTestIngClassName, + }, + } + + placeholderSpc = &secv1.SecretProviderClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-spc", + Namespace: placeholderTestIng.Namespace, + Generation: 123, + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Ingress", + Name: placeholderTestIng.Name, + }}, + }, + } ) func TestPlaceholderPodControllerIntegration(t *testing.T) { - ing := &netv1.Ingress{} - ing.Name = "test-ing" - ing.Namespace = "default" - ingressClass := "webapprouting.kubernetes.azure.com" - ing.Spec.IngressClassName = &ingressClass - - spc := &secv1.SecretProviderClass{} - spc.Name = "test-spc" - spc.Namespace = ing.Namespace - spc.Generation = 123 - spc.OwnerReferences = []metav1.OwnerReference{{ - Kind: "Ingress", - Name: ing.Name, - }} + ing := placeholderTestIng.DeepCopy() + spc := placeholderSpc.DeepCopy() + spc.Labels = manifests.GetTopLevelLabels() c := fake.NewClientBuilder().WithObjects(spc, ing).Build() require.NoError(t, secv1.AddToScheme(c.Scheme())) p := &PlaceholderPodController{ client: c, config: &config.Config{Registry: "test-registry"}, - ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}), + ingressManager: NewIngressManager(map[string]struct{}{placeholderTestIngClassName: {}}), } ctx := context.Background() @@ -66,20 +81,25 @@ func TestPlaceholderPodControllerIntegration(t *testing.T) { require.Equal(t, testutils.GetErrMetricCount(t, placeholderPodControllerName), beforeErrCount) require.Greater(t, testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess), beforeReconcileCount) - dep := &appsv1.Deployment{} - dep.Name = spc.Name - dep.Namespace = spc.Namespace + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: spc.Name, + Namespace: spc.Namespace, + }, + } require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(dep), dep)) replicas := int32(1) historyLimit := int32(2) + + expectedLabels := util.MergeMaps(spc.Labels, map[string]string{"app": spc.Name}) expected := appsv1.DeploymentSpec{ Replicas: &replicas, RevisionHistoryLimit: &historyLimit, - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": spc.Name}}, + Selector: &metav1.LabelSelector{MatchLabels: expectedLabels}, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"app": spc.Name}, + Labels: expectedLabels, Annotations: map[string]string{ "kubernetes.azure.com/observed-generation": "123", "kubernetes.azure.com/purpose": "hold CSI mount to enable keyvault-to-k8s secret mirroring", @@ -161,6 +181,112 @@ func TestPlaceholderPodControllerIntegration(t *testing.T) { require.True(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(dep), dep))) } +func TestPlaceholderPodControllerNoManagedByLabels(t *testing.T) { + ing := placeholderTestIng.DeepCopy() + spc := placeholderSpc.DeepCopy() + spc.Labels = map[string]string{} + + c := fake.NewClientBuilder().WithObjects(spc, ing).Build() + require.NoError(t, secv1.AddToScheme(c.Scheme())) + p := &PlaceholderPodController{ + client: c, + config: &config.Config{Registry: "test-registry"}, + ingressManager: NewIngressManager(map[string]struct{}{placeholderTestIngClassName: {}}), + } + + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + + // Create placeholder pod deployment + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: spc.Namespace, Name: spc.Name}} + beforeErrCount := testutils.GetErrMetricCount(t, placeholderPodControllerName) + beforeReconcileCount := testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess) + _, err := p.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, placeholderPodControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess), beforeReconcileCount) + + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: spc.Name, + Namespace: spc.Namespace, + }, + } + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(dep), dep)) + + replicas := int32(1) + historyLimit := int32(2) + + expectedLabels := util.MergeMaps(map[string]string{"app": spc.Name}, manifests.GetTopLevelLabels()) + expected := appsv1.DeploymentSpec{ + Replicas: &replicas, + RevisionHistoryLimit: &historyLimit, + Selector: &metav1.LabelSelector{MatchLabels: expectedLabels}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: expectedLabels, + Annotations: map[string]string{ + "kubernetes.azure.com/observed-generation": "123", + "kubernetes.azure.com/purpose": "hold CSI mount to enable keyvault-to-k8s secret mirroring", + "kubernetes.azure.com/ingress-owner": ing.Name, + "openservicemesh.io/sidecar-injection": "disabled", + }, + }, + Spec: *manifests.WithPreferSystemNodes(&corev1.PodSpec{ + AutomountServiceAccountToken: util.BoolPtr(false), + Containers: []corev1.Container{{ + Name: "placeholder", + Image: "test-registry/oss/kubernetes/pause:3.6-hotfix.20220114", + VolumeMounts: []corev1.VolumeMount{{ + Name: "secrets", + MountPath: "/mnt/secrets", + ReadOnly: true, + }}, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("20m"), + corev1.ResourceMemory: resource.MustParse("24Mi"), + }, + }, + }}, + Volumes: []corev1.Volume{{ + Name: "secrets", + VolumeSource: corev1.VolumeSource{ + CSI: &corev1.CSIVolumeSource{ + Driver: "secrets-store.csi.k8s.io", + ReadOnly: util.BoolPtr(true), + VolumeAttributes: map[string]string{"secretProviderClass": spc.Name}, + }, + }, + }}, + }), + }, + } + assert.Equal(t, expected, dep.Spec) + + // Prove idempotence + beforeErrCount = testutils.GetErrMetricCount(t, placeholderPodControllerName) + beforeReconcileCount = testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess) + _, err = p.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, placeholderPodControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess), beforeReconcileCount) + + // Change the ingress resource's class + ing.Spec.IngressClassName = nil + require.NoError(t, c.Update(ctx, ing)) + + beforeErrCount = testutils.GetErrMetricCount(t, placeholderPodControllerName) + beforeReconcileCount = testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess) + _, err = p.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, placeholderPodControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess), beforeReconcileCount) + + // Prove the deployment was not deleted + require.False(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(dep), dep))) +} + func TestNewPlaceholderPodController(t *testing.T) { m, err := manager.New(restConfig, manager.Options{Metrics: metricsserver.Options{BindAddress: ":0"}}) require.NoError(t, err) diff --git a/pkg/controller/osm/ingress_backend_reconciler.go b/pkg/controller/osm/ingress_backend_reconciler.go index 5e3975e2..d9ead313 100644 --- a/pkg/controller/osm/ingress_backend_reconciler.go +++ b/pkg/controller/osm/ingress_backend_reconciler.go @@ -6,7 +6,6 @@ package osm import ( "context" - "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" "github.com/go-logr/logr" policyv1alpha1 "github.com/openservicemesh/osm/pkg/apis/policy/v1alpha1" netv1 "k8s.io/api/networking/v1" @@ -15,7 +14,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" + "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/Azure/aks-app-routing-operator/pkg/util" ) @@ -104,8 +105,6 @@ func (i *IngressBackendReconciler) Reconcile(ctx context.Context, req ctrl.Reque } logger = logger.WithValues("name", ing.Name, "namespace", ing.Namespace, "generation", ing.Generation) - // TODO: add label and check for label before cleanup - backend := &policyv1alpha1.IngressBackend{ TypeMeta: metav1.TypeMeta{ Kind: "IngressBackend", @@ -114,6 +113,7 @@ func (i *IngressBackendReconciler) Reconcile(ctx context.Context, req ctrl.Reque ObjectMeta: metav1.ObjectMeta{ Name: ing.Name, Namespace: ing.Namespace, + Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: ing.APIVersion, Controller: util.BoolPtr(true), @@ -127,19 +127,6 @@ func (i *IngressBackendReconciler) Reconcile(ctx context.Context, req ctrl.Reque controllerName, ok := i.ingressControllerNamer.IngressControllerName(ing) logger = logger.WithValues("ingressController", controllerName) - if ing.Annotations == nil || ing.Annotations["kubernetes.azure.com/use-osm-mtls"] == "" || !ok { - logger.Info("Ingress does not have osm mtls annotation, cleaning up managed IngressBackend") - - logger.Info("getting IngressBackend") - err = i.client.Get(ctx, client.ObjectKeyFromObject(backend), backend) - if err != nil { - return result, client.IgnoreNotFound(err) - } - - logger.Info("deleting IngressBackend") - err = i.client.Delete(ctx, backend) - return result, err - } backend.Spec = policyv1alpha1.IngressBackendSpec{ Backends: []policyv1alpha1.BackendSpec{}, @@ -174,6 +161,24 @@ func (i *IngressBackendReconciler) Reconcile(ctx context.Context, req ctrl.Reque } } + if ing.Annotations == nil || ing.Annotations["kubernetes.azure.com/use-osm-mtls"] == "" || !ok { + logger.Info("Ingress does not have osm mtls annotation, cleaning up managed IngressBackend") + logger.Info("getting IngressBackend") + + toCleanBackend := &policyv1alpha1.IngressBackend{} + err = i.client.Get(ctx, client.ObjectKeyFromObject(backend), toCleanBackend) + if err != nil { + return result, client.IgnoreNotFound(err) + } + + if manifests.HasTopLevelLabels(toCleanBackend.Labels) { + logger.Info("deleting IngressBackend") + err = i.client.Delete(ctx, toCleanBackend) + return result, client.IgnoreNotFound(err) + } + return result, nil + } + logger.Info("reconciling OSM ingress backend for ingress") err = util.Upsert(ctx, i.client, backend) return result, err diff --git a/pkg/controller/osm/ingress_backend_reconciler_test.go b/pkg/controller/osm/ingress_backend_reconciler_test.go index ef0a4170..f00b4dba 100644 --- a/pkg/controller/osm/ingress_backend_reconciler_test.go +++ b/pkg/controller/osm/ingress_backend_reconciler_test.go @@ -27,14 +27,15 @@ import ( "github.com/Azure/aks-app-routing-operator/pkg/config" "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" + "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/Azure/aks-app-routing-operator/pkg/util" ) var ( - env *envtest.Environment - restConfig *rest.Config - err error - ing = &netv1.Ingress{ + env *envtest.Environment + restConfig *rest.Config + err error + backendTestIng = &netv1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "test-ingress", Namespace: "test-ns", @@ -73,6 +74,7 @@ func TestMain(m *testing.M) { } func TestIngressBackendReconcilerIntegration(t *testing.T) { + ing := backendTestIng.DeepCopy() c := fake.NewClientBuilder().WithObjects(ing).Build() require.NoError(t, policyv1alpha1.AddToScheme(c.Scheme())) @@ -136,7 +138,83 @@ func TestIngressBackendReconcilerIntegration(t *testing.T) { require.Greater(t, testutils.GetReconcileMetricCount(t, ingressBackendControllerName, metrics.LabelSuccess), beforeReconcileCount) } +func TestIngressBackendReconcilerIntegrationNoLabels(t *testing.T) { + ing := backendTestIng.DeepCopy() + c := fake.NewClientBuilder().WithObjects(ing).Build() + require.NoError(t, policyv1alpha1.AddToScheme(c.Scheme())) + + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: ing.Namespace, Name: ing.Name}} + e := &IngressBackendReconciler{ + client: c, + config: &config.Config{NS: "test-config-ns"}, + ingressControllerNamer: NewIngressControllerNamer(map[string]string{*ing.Spec.IngressClassName: "test-name"}), + } + + // Initial reconcile + beforeErrCount := testutils.GetErrMetricCount(t, ingressBackendControllerName) + beforeReconcileCount := testutils.GetReconcileMetricCount(t, ingressBackendControllerName, metrics.LabelSuccess) + _, err := e.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, ingressBackendControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, ingressBackendControllerName, metrics.LabelSuccess), beforeReconcileCount) + + // Prove config is correct + backend := &policyv1alpha1.IngressBackend{ + ObjectMeta: metav1.ObjectMeta{ + Name: ing.Name, + Namespace: ing.Namespace, + }, + } + require.NoError(t, e.client.Get(ctx, client.ObjectKeyFromObject(backend), backend)) + require.Len(t, backend.Spec.Backends, 1) + assert.Equal(t, policyv1alpha1.BackendSpec{ + Name: "test-service", + Port: policyv1alpha1.PortSpec{Number: 123, Protocol: "https"}, + }, backend.Spec.Backends[0]) + + // Cover no-op updates + beforeErrCount = testutils.GetErrMetricCount(t, ingressBackendControllerName) + beforeReconcileCount = testutils.GetReconcileMetricCount(t, ingressBackendControllerName, metrics.LabelSuccess) + _, err = e.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, ingressBackendControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, ingressBackendControllerName, metrics.LabelSuccess), beforeReconcileCount) + + // Get updated backend + require.False(t, errors.IsNotFound(e.client.Get(ctx, client.ObjectKeyFromObject(backend), backend))) + assert.Equal(t, len(manifests.GetTopLevelLabels()), len(backend.Labels)) + + // Remove the labels + backend.Labels = map[string]string{} + require.NoError(t, e.client.Update(ctx, backend)) + assert.Equal(t, 0, len(backend.Labels)) + + // Remove the annotation + ing.Annotations = map[string]string{} + require.NoError(t, c.Update(ctx, ing)) + + // Reconcile + beforeErrCount = testutils.GetErrMetricCount(t, ingressBackendControllerName) + beforeReconcileCount = testutils.GetReconcileMetricCount(t, ingressBackendControllerName, metrics.LabelSuccess) + _, err = e.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, ingressBackendControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, ingressBackendControllerName, metrics.LabelSuccess), beforeReconcileCount) + + require.False(t, errors.IsNotFound(e.client.Get(ctx, client.ObjectKeyFromObject(backend), backend))) + assert.Equal(t, 0, len(backend.Labels)) + _, err = e.Reconcile(ctx, req) + require.NoError(t, err) + assert.Equal(t, 0, len(backend.Labels)) + + // Prove the ingress backend was not cleaned up + require.False(t, errors.IsNotFound(e.client.Get(ctx, client.ObjectKeyFromObject(backend), backend))) +} + func TestNewIngressBackendReconciler(t *testing.T) { + ing := backendTestIng.DeepCopy() m, err := manager.New(restConfig, manager.Options{Metrics: metricsserver.Options{BindAddress: ":0"}}) require.NoError(t, err) diff --git a/pkg/manifests/common.go b/pkg/manifests/common.go index afe90fa9..f57767a1 100644 --- a/pkg/manifests/common.go +++ b/pkg/manifests/common.go @@ -1,11 +1,12 @@ package manifests import ( - "github.com/Azure/aks-app-routing-operator/pkg/config" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/Azure/aks-app-routing-operator/pkg/config" ) const operatorName = "aks-app-routing-operator" @@ -15,6 +16,24 @@ func GetTopLevelLabels() map[string]string { // this is a function to avoid any return map[string]string{"app.kubernetes.io/managed-by": operatorName} } +// Checks the first set of labels has the labels of the other passed in sets +func HasTopLevelLabels(objLabels map[string]string) bool { + if len(objLabels) == 0 { + return false + } + + for label, val := range GetTopLevelLabels() { + spcVal, ok := objLabels[label] + if !ok { + return false + } + if spcVal != val { + return false + } + } + return true +} + func getOwnerRefs(deploy *appsv1.Deployment) []metav1.OwnerReference { if deploy == nil { return nil diff --git a/pkg/manifests/common_test.go b/pkg/manifests/common_test.go index 34c8e0ef..53e58ad5 100644 --- a/pkg/manifests/common_test.go +++ b/pkg/manifests/common_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/Azure/aks-app-routing-operator/pkg/config" + "github.com/Azure/aks-app-routing-operator/pkg/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sigs.k8s.io/controller-runtime/pkg/client" @@ -39,6 +40,24 @@ func TestNamespaceResources(t *testing.T) { } } +func TestHasTopLevelLabels(t *testing.T) { + cases := []struct { + Labels map[string]string + ReqLabels []map[string]string + Outcome bool + }{ + {Labels: map[string]string{}, Outcome: false}, // Blank labels + {Labels: map[string]string{"fake": "fake"}, Outcome: false}, // Only fake labels + {Labels: map[string]string{"app.kubernetes.io/managed-by": "false-operator-name"}, Outcome: false}, // Correct key, incorrect value + {Labels: GetTopLevelLabels(), Outcome: true}, // Correct labels + {Labels: util.MergeMaps(GetTopLevelLabels(), map[string]string{"fakeLabel1": "fakeValue1"}), Outcome: true}, // Additional labels + } + + for _, c := range cases { + require.Equal(t, HasTopLevelLabels(c.Labels), c.Outcome) + } +} + // AssertFixture checks the fixture path and compares it to the provided objects, failing if they are not equal func AssertFixture(t *testing.T, fixturePath string, objs []client.Object) { t.Logf("Testing fixture %s", fixturePath)