From 4cdee5698425b2948409af4608cdfcaab05acb28 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 12 Feb 2024 16:37:56 -0500 Subject: [PATCH 01/36] moving changes from aamgayle/crdwatchreconciler to new verified branch --- api/v1alpha1/nginxingresscontroller_types.go | 4 + ...tes.azure.com_nginxingresscontrollers.yaml | 3 + pkg/controller/controller.go | 4 + .../keyvault/nginx_secret_provider_class.go | 220 ++++++++++ .../nginx_secret_provider_class_test.go | 397 ++++++++++++++++++ pkg/controller/keyvault/placeholder_pod.go | 119 ++++-- .../keyvault/placeholder_pod_test.go | 201 ++++++++- .../nginxingress/nginx_ingress_controller.go | 5 + 8 files changed, 911 insertions(+), 42 deletions(-) create mode 100644 pkg/controller/keyvault/nginx_secret_provider_class.go create mode 100644 pkg/controller/keyvault/nginx_secret_provider_class_test.go diff --git a/api/v1alpha1/nginxingresscontroller_types.go b/api/v1alpha1/nginxingresscontroller_types.go index df0336b9..7638a03f 100644 --- a/api/v1alpha1/nginxingresscontroller_types.go +++ b/api/v1alpha1/nginxingresscontroller_types.go @@ -61,6 +61,10 @@ type DefaultSSLCertificate struct { // Secret is a struct that holds the name and namespace fields used for the default ssl secret // +optional Secret *Secret `json:"secret,omitempty"` + + // Secret in the form of a Key Vault URI + // +optional + KeyVaultURI string `json:"keyVaultURI"` } // Secret is a struct that holds a name and namespace to be used in DefaultSSLCertificate diff --git a/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml b/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml index c52bc983..8f824b0c 100644 --- a/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml +++ b/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml @@ -66,6 +66,9 @@ spec: should use a certain SSL certificate by default. If this field is omitted, no default certificate will be used. properties: + keyVaultURI: + description: Secret in the form of a Key Vault URI + type: string secret: description: Secret is a struct that holds the name and namespace fields used for the default ssl secret diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 443e7ab5..5bac063a 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -184,6 +184,10 @@ func setupControllers(mgr ctrl.Manager, conf *config.Config, lgr logr.Logger, cl if err := keyvault.NewIngressSecretProviderClassReconciler(mgr, conf, ingressManager); err != nil { return fmt.Errorf("setting up ingress secret provider class reconciler: %w", err) } + lgr.Info("setting up nginx keyvault secret provider class reconciler") + if err := keyvault.NewNginxSecretProviderClassReconciler(mgr, conf, ingressManager); err != nil { + return fmt.Errorf("setting up crd secret provider class reconciler: %w", err) + } lgr.Info("setting up keyvault placeholder pod controller") if err := keyvault.NewPlaceholderPodController(mgr, conf, ingressManager); err != nil { return fmt.Errorf("setting up placeholder pod controller: %w", err) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go new file mode 100644 index 00000000..22998db2 --- /dev/null +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -0,0 +1,220 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package keyvault + +import ( + "context" + "encoding/json" + "fmt" + approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "net/url" + "strings" + + "github.com/go-logr/logr" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + 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" +) + +var ( + nginxSecretProviderControllerName = controllername.New("keyvault", "nginx", "secret", "provider") + NginxNamePrefix = "keyvault-nginx-" +) + +// NginxSecretProviderClassReconciler manages a SecretProviderClass for each nginx ingress controller that +// has a Keyvault URI in its DefaultSSLCertificate field. The SPC is used to mirror the Keyvault values into +// a k8s secret so that it can be used by the CRD controller. +type NginxSecretProviderClassReconciler struct { + client client.Client + events record.EventRecorder + config *config.Config +} + +func NewNginxSecretProviderClassReconciler(manager ctrl.Manager, conf *config.Config, ingressManager IngressManager) error { + metrics.InitControllerMetrics(nginxSecretProviderControllerName) + if conf.DisableKeyvault { + return nil + } + return nginxSecretProviderControllerName.AddToController( + ctrl. + NewControllerManagedBy(manager). + For(&approutingv1alpha1.NginxIngressController{}), manager.GetLogger(), + ).Complete(&NginxSecretProviderClassReconciler{ + client: manager.GetClient(), + events: manager.GetEventRecorderFor("aks-app-routing-operator"), + config: conf, + }) +} + +func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + var err error + result := ctrl.Result{} + + // do metrics + defer func() { + //placing this call inside a closure allows for result and err to be bound after Reconcile executes + //this makes sure they have the proper value + //just calling defer metrics.HandleControllerReconcileMetrics(controllerName, result, err) would bind + //the values of result and err to their zero values, since they were just instantiated + metrics.HandleControllerReconcileMetrics(nginxSecretProviderControllerName, result, err) + }() + + logger, err := logr.FromContext(ctx) + if err != nil { + return result, err + } + logger = nginxSecretProviderControllerName.AddToLogger(logger).WithValues("name", req.Name, "namespace", req.Namespace) + + logger.Info("getting Nginx Ingress") + nic := &approutingv1alpha1.NginxIngressController{} + err = i.client.Get(ctx, req.NamespacedName, nic) + if err != nil { + return result, client.IgnoreNotFound(err) + } + logger = logger.WithValues("name", nic.Name, "namespace", "approutingsystem", "generation", nic.Generation) + + spc := &secv1.SecretProviderClass{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "secrets-store.csi.x-k8s.io/v1", + Kind: "SecretProviderClass", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNginxCertName(nic), + Namespace: "approutingsystem", + Labels: manifests.GetTopLevelLabels(), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: nic.APIVersion, + Controller: util.BoolPtr(true), + Kind: nic.Kind, + Name: nic.Name, + UID: nic.UID, + }}, + }, + } + logger = logger.WithValues("spc", spc.Name) + ok, err := i.buildSPC(nic, spc) + if err != nil { + logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") + i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) + return result, nil + } + if ok { + logger.Info("reconciling secret provider class for ingress") + err = util.Upsert(ctx, i.client, spc) + if err != nil { + i.events.Eventf(nic, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err) + } + return result, err + } + + logger.Info("cleaning unused managed spc for ingress") + logger.Info("getting secret provider class for ingress") + + toCleanSPC := &secv1.SecretProviderClass{} + + err = i.client.Get(ctx, client.ObjectKeyFromObject(spc), toCleanSPC) + if err != nil { + return result, client.IgnoreNotFound(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 *NginxSecretProviderClassReconciler) buildSPC(nic *approutingv1alpha1.NginxIngressController, spc *secv1.SecretProviderClass) (bool, error) { + if nic.Spec.IngressClassName == "" { + return false, nil + } + + certURI := nic.Spec.DefaultSSLCertificate.KeyVaultURI + if certURI == "" { + return false, nil + } + + uri, err := url.Parse(certURI) + if err != nil { + return false, err + } + vaultName := strings.Split(uri.Host, ".")[0] + chunks := strings.Split(uri.Path, "/") + if len(chunks) < 3 { + return false, fmt.Errorf("invalid secret uri: %s", certURI) + } + secretName := chunks[2] + p := map[string]interface{}{ + "objectName": secretName, + "objectType": "secret", + } + if len(chunks) > 3 { + p["objectVersion"] = chunks[3] + } + + params, err := json.Marshal(p) + if err != nil { + return false, err + } + objects, err := json.Marshal(map[string]interface{}{"array": []string{string(params)}}) + if err != nil { + return false, err + } + + spc.Spec = secv1.SecretProviderClassSpec{ + Provider: secv1.Provider("azure"), + SecretObjects: []*secv1.SecretObject{{ + SecretName: DefaultNginxCertName(nic), + Type: "kubernetes.io/tls", + Data: []*secv1.SecretObjectData{ + { + ObjectName: secretName, + Key: "tls.key", + }, + { + ObjectName: secretName, + Key: "tls.crt", + }, + }, + }}, + // https://azure.github.io/secrets-store-csi-driver-provider-azure/docs/getting-started/usage/#create-your-own-secretproviderclass-object + Parameters: map[string]string{ + "keyvaultName": vaultName, + "useVMManagedIdentity": "true", + "userAssignedIdentityID": i.config.MSIClientID, + "tenantId": i.config.TenantID, + "objects": string(objects), + }, + } + + if i.config.Cloud != "" { + spc.Spec.Parameters[kvcsi.CloudNameParameter] = i.config.Cloud + } + + return true, nil +} + +// DefaultNginxCertName returns a default name for the nginx certificate name using the IngressClassName from the spec. +// Truncates characters in the IngressClassName passed the max secret length (255) if the IngressClassName and the default namespace are over the limit +func DefaultNginxCertName(nic *approutingv1alpha1.NginxIngressController) string { + secretMaxSize := 255 + certName := NginxNamePrefix + nic.Name + + if len(certName) > secretMaxSize { + return certName[0:secretMaxSize] + } + + return certName +} diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go new file mode 100644 index 00000000..ab252617 --- /dev/null +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -0,0 +1,397 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package keyvault + +import ( + "context" + "fmt" + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "k8s.io/apimachinery/pkg/runtime" + "net/url" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + 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" + kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" +) + +var ( + spcTestNginxIngressClassName = "webapprouting.kubernetes.azure.com" + spcTestNginxIngress = &v1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nic", + Namespace: "test-namespace", + }, + Spec: v1alpha1.NginxIngressControllerSpec{ + IngressClassName: spcTestNginxIngressClassName, + DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{ + Secret: nil, + KeyVaultURI: "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34"}, + }, + } +) + +func TestNginxSecretProviderClassReconcilerIntegration(t *testing.T) { + // Create the nic + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + nic := spcTestNginxIngress.DeepCopy() + + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + require.NoError(t, secv1.AddToScheme(scheme)) + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(nic).Build() + + recorder := record.NewFakeRecorder(10) + n := &NginxSecretProviderClassReconciler{ + client: c, + config: &config.Config{ + TenantID: "test-tenant-id", + MSIClientID: "test-msi-client-id", + }, + events: recorder, + } + + // Create the secret provider class + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: nic.Namespace, Name: nic.Name}} + beforeErrCount := testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + beforeRequestCount := testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err := n.Reconcile(ctx, req) + require.NoError(t, err) + + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + // Prove it exists + spc := &secv1.SecretProviderClass{} + spc.Name = DefaultNginxCertName(nic) + spc.Namespace = "approutingsystem" + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc)) + + expected := &secv1.SecretProviderClass{ + Spec: secv1.SecretProviderClassSpec{ + Provider: "azure", + Parameters: map[string]string{ + "keyvaultName": "testvault", + "objects": "{\"array\":[\"{\\\"objectName\\\":\\\"testcert\\\",\\\"objectType\\\":\\\"secret\\\",\\\"objectVersion\\\":\\\"f8982febc6894c0697b884f946fb1a34\\\"}\"]}", + "tenantId": n.config.TenantID, + "useVMManagedIdentity": "true", + "userAssignedIdentityID": n.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, nginxSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err = n.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + // Remove the cert's version from the nic + nic.Spec.DefaultSSLCertificate.KeyVaultURI = "https://testvault.vault.azure.net/certificates/testcert" + require.NoError(t, n.client.Update(ctx, nic)) + beforeErrCount = testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err = n.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + // Prove the objectVersion property was removed + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc)) + expected.Spec.Parameters["objects"] = "{\"array\":[\"{\\\"objectName\\\":\\\"testcert\\\",\\\"objectType\\\":\\\"secret\\\"}\"]}" + assert.Equal(t, expected.Spec, spc.Spec) + + // Remove the cert annotation from the nic + nic.Spec.DefaultSSLCertificate.KeyVaultURI = "" + require.NoError(t, n.client.Update(ctx, nic)) + beforeErrCount = testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err = n.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + // Prove secret class was removed + require.True(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(spc), spc))) + + // Check for idempotence + beforeErrCount = testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err = n.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) +} + +func TestNginxSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testing.T) { + // Create the nic + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + nic := spcTestNginxIngress.DeepCopy() + + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + require.NoError(t, secv1.AddToScheme(scheme)) + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(nic).Build() + + recorder := record.NewFakeRecorder(10) + n := &NginxSecretProviderClassReconciler{ + client: c, + config: &config.Config{ + TenantID: "test-tenant-id", + MSIClientID: "test-msi-client-id", + }, + events: recorder, + } + + // Create the secret provider class + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: nic.Namespace, Name: nic.Name}} + beforeErrCount := testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + beforeRequestCount := testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err := n.Reconcile(ctx, req) + require.NoError(t, err) + + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + spc := &secv1.SecretProviderClass{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "secrets-store.csi.x-k8s.io/v1", + Kind: "SecretProviderClass", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNginxCertName(nic), + Namespace: "approutingsystem", + Labels: manifests.GetTopLevelLabels(), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: nic.APIVersion, + Controller: util.BoolPtr(true), + Kind: nic.Kind, + Name: nic.Name, + UID: nic.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, n.client.Update(ctx, spc)) + assert.Equal(t, 0, len(spc.Labels)) + + // Remove the cert annotation from the nic + nic.Spec.DefaultSSLCertificate.KeyVaultURI = "" + require.NoError(t, n.client.Update(ctx, nic)) + + // Reconcile both changes + beforeErrCount = testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err = n.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, 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": n.config.TenantID, + "useVMManagedIdentity": "true", + "userAssignedIdentityID": n.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, nginxSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) + _, err = n.Reconcile(ctx, req) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, nginxSecretProviderControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) +} + +func TestNginxSecretProviderClassReconcilerInvalidURL(t *testing.T) { + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + // Create the nic + nic := spcTestNginxIngress.DeepCopy() + nic.Spec.DefaultSSLCertificate.KeyVaultURI = "inv@lid URL" + + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + require.NoError(t, secv1.AddToScheme(scheme)) + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(nic).Build() + recorder := record.NewFakeRecorder(10) + n := &NginxSecretProviderClassReconciler{ + client: c, + config: &config.Config{ + TenantID: "test-tenant-id", + MSIClientID: "test-msi-client-id", + }, + events: recorder, + } + + metrics.InitControllerMetrics(nginxSecretProviderControllerName) + + // get the before value of the error metrics + beforeErrCount := testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + beforeRequestCount := testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelError) + + req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: nic.Namespace, Name: nic.Name}} + _, err := n.Reconcile(ctx, req) + require.NoError(t, err) + + assert.Equal(t, "Warning InvalidInput error while processing Keyvault reference: invalid secret uri: inv@lid URL", <-recorder.Events) + //even though no error was returned, we should expect the error count to be incremented + afterErrCount := testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) + afterRequestCount := testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelError) + + assert.Greater(t, afterErrCount, beforeErrCount) + assert.Greater(t, afterRequestCount, beforeRequestCount) +} + +func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { + n := &NginxSecretProviderClassReconciler{} + + invalidURLIng := &v1alpha1.NginxIngressController{ + Spec: v1alpha1.NginxIngressControllerSpec{ + IngressClassName: spcTestNginxIngressClassName, + DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{KeyVaultURI: ""}, + }, + } + + t.Run("missing nic class name", func(t *testing.T) { + nic := invalidURLIng.DeepCopy() + nic.Spec.IngressClassName = "" + + ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + assert.False(t, ok) + require.NoError(t, err) + }) + + t.Run("empty key vault uri", func(t *testing.T) { + nic := invalidURLIng.DeepCopy() + + ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + assert.False(t, ok) + require.NoError(t, err) + }) + + t.Run("url with control character", func(t *testing.T) { + nic := invalidURLIng.DeepCopy() + cc := string([]byte{0x7f}) + nic.Spec.DefaultSSLCertificate.KeyVaultURI = cc + + ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + assert.False(t, ok) + _, expectedErr := url.Parse(cc) // the exact error depends on operating system + require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) + }) + + t.Run("url with one path segment", func(t *testing.T) { + nic := invalidURLIng.DeepCopy() + nic.Spec.DefaultSSLCertificate.KeyVaultURI = "http://test.com/foo" + + ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + assert.False(t, ok) + require.EqualError(t, err, "invalid secret uri: http://test.com/foo") + }) +} + +func TestNginxSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { + cases := []struct { + name, configCloud, spcCloud string + expected bool + }{ + { + name: "empty config cloud", + configCloud: "", + expected: false, + }, + { + name: "public cloud", + configCloud: "AzurePublicCloud", + spcCloud: "AzurePublicCloud", + expected: true, + }, + { + name: "sov cloud", + configCloud: "AzureUSGovernmentCloud", + spcCloud: "AzureUSGovernmentCloud", + expected: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + n := &NginxSecretProviderClassReconciler{ + config: &config.Config{ + Cloud: c.configCloud, + }, + } + + nic := spcTestNginxIngress.DeepCopy() + nic.Spec.DefaultSSLCertificate.KeyVaultURI = "https://test.vault.azure.net/secrets/test-secret" + + spc := &secv1.SecretProviderClass{} + ok, err := n.buildSPC(nic, spc) + require.NoError(t, err, "building SPC should not error") + require.True(t, ok, "SPC should be built") + + spcCloud, ok := spc.Spec.Parameters[kvcsi.CloudNameParameter] + require.Equal(t, c.expected, ok, "SPC cloud annotation unexpected") + require.Equal(t, c.spcCloud, spcCloud, "SPC cloud annotation doesn't match") + }) + } +} diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 723e104b..98ffab7d 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -6,8 +6,10 @@ package keyvault import ( "context" "fmt" + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "path" "strconv" + "strings" "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" @@ -108,49 +110,92 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } logger = logger.WithValues("deployment", dep.Name) - ing := &netv1.Ingress{} - ing.Name = util.FindOwnerKind(spc.OwnerReferences, "Ingress") - ing.Namespace = req.Namespace - logger = logger.WithValues("ingress", ing.Name) - if ing.Name != "" { - logger.Info("getting owner ingress") - if err = p.client.Get(ctx, client.ObjectKeyFromObject(ing), ing); err != nil { - return result, client.IgnoreNotFound(err) + if strings.HasPrefix(spc.Name, NginxNamePrefix) { + nic := &v1alpha1.NginxIngressController{} + nic.Name = util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController") + + if nic.Name != "" { + logger.Info("getting owner nginx ingress controller") + if err = p.client.Get(ctx, client.ObjectKeyFromObject(nic), nic); err != nil { + return result, client.IgnoreNotFound(err) + } } - } - managed, err := p.ingressManager.IsManaging(ing) - if err != nil { - return result, fmt.Errorf("determining if ingress is managed: %w", err) - } + if nic.Name == "" || nic.Spec.IngressClassName == "" || nic.Spec.DefaultSSLCertificate.KeyVaultURI == "" { + logger.Info("cleaning unused placeholder pod deployment") + logger.Info("getting placeholder deployment") + 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) + } + } + + // Manage a deployment resource + logger.Info("reconciling placeholder deployment for secret provider class") + if err = p.buildDeployment(ctx, dep, spc, nic.Name); err != nil { + err = fmt.Errorf("building deployment: %w", err) + p.events.Eventf(nic, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while building placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) + logger.Error(err, "failed to build placeholder deployment") + return result, err + } + + if err = util.Upsert(ctx, p.client, dep); err != nil { + p.events.Eventf(nic, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while creating or updating placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) + return result, err + } + } else { + if p.ingressManager == nil { + return result, fmt.Errorf("checking if ingressManager was not nil on non-nginx ingress: %w", err) + } + ing := &netv1.Ingress{} + ing.Name = util.FindOwnerKind(spc.OwnerReferences, "Ingress") + ing.Namespace = req.Namespace + logger = logger.WithValues("ingress", ing.Name) + if ing.Name != "" { + logger.Info("getting owner ingress") + if err = p.client.Get(ctx, client.ObjectKeyFromObject(ing), ing); err != nil { + return result, client.IgnoreNotFound(err) + } + } - if ing.Name == "" || ing.Spec.IngressClassName == nil || !managed { - logger.Info("cleaning unused placeholder pod deployment") + managed, err := p.ingressManager.IsManaging(ing) + if err != nil { + return result, fmt.Errorf("determining if ingress is managed: %w", err) + } - logger.Info("getting placeholder deployment") + if ing.Name == "" || ing.Spec.IngressClassName == nil || !managed { + logger.Info("cleaning unused placeholder pod deployment") - toCleanDeployment := &appsv1.Deployment{} - if err = p.client.Get(ctx, client.ObjectKeyFromObject(dep), toCleanDeployment); err != nil { - return result, client.IgnoreNotFound(err) + logger.Info("getting placeholder deployment") + + 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) + } } - if manifests.HasTopLevelLabels(toCleanDeployment.Labels) { - logger.Info("deleting placeholder deployment") - err = p.client.Delete(ctx, toCleanDeployment) - return result, client.IgnoreNotFound(err) + // Manage a deployment resource + logger.Info("reconciling placeholder deployment for secret provider class") + if err = p.buildDeployment(ctx, dep, spc, ing.Name); err != nil { + err = fmt.Errorf("building deployment: %w", err) + p.events.Eventf(ing, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while building placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) + logger.Error(err, "failed to build placeholder deployment") + return result, err } - } - // Manage a deployment resource - logger.Info("reconciling placeholder deployment for secret provider class") - if err = p.buildDeployment(ctx, dep, spc, ing); err != nil { - err = fmt.Errorf("building deployment: %w", err) - p.events.Eventf(ing, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while building placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) - logger.Error(err, "failed to build placeholder deployment") - return result, err - } - if err = util.Upsert(ctx, p.client, dep); err != nil { - p.events.Eventf(ing, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while creating or updating placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) - return result, err + if err = util.Upsert(ctx, p.client, dep); err != nil { + p.events.Eventf(ing, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while creating or updating placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) + return result, err + } } return result, nil @@ -167,7 +212,7 @@ func (p *PlaceholderPodController) getCurrentDeployment(ctx context.Context, nam return dep, nil } -func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *appsv1.Deployment, spc *secv1.SecretProviderClass, ing *netv1.Ingress) error { +func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *appsv1.Deployment, spc *secv1.SecretProviderClass, ingName string) error { old, err := p.getCurrentDeployment(ctx, client.ObjectKeyFromObject(dep)) if err != nil { return fmt.Errorf("getting current deployment: %w", err) @@ -189,7 +234,7 @@ func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *app Annotations: map[string]string{ "kubernetes.azure.com/observed-generation": strconv.FormatInt(spc.Generation, 10), "kubernetes.azure.com/purpose": "hold CSI mount to enable keyvault-to-k8s secret mirroring", - "kubernetes.azure.com/ingress-owner": ing.Name, + "kubernetes.azure.com/ingress-owner": ingName, "openservicemesh.io/sidecar-injection": "disabled", }, }, diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 54ae45cb..3649e584 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -5,6 +5,11 @@ package keyvault import ( "context" + "fmt" + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/log/zap" "testing" "k8s.io/apimachinery/pkg/api/errors" @@ -43,7 +48,18 @@ var ( IngressClassName: &placeholderTestIngClassName, }, } - + placeholderTestNginxIngress = &v1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nic", + }, + TypeMeta: metav1.TypeMeta{Kind: "NginxIngressController"}, + Spec: v1alpha1.NginxIngressControllerSpec{ + IngressClassName: spcTestNginxIngressClassName, + DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{ + Secret: nil, // nil Secret since SPC method for DefaultSSLCertificate using placeholder pods only uses the KeyVaultURI + KeyVaultURI: "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34"}, + }, + } placeholderSpc = &secv1.SecretProviderClass{ ObjectMeta: metav1.ObjectMeta{ Name: "test-spc", @@ -57,7 +73,7 @@ var ( } ) -func TestPlaceholderPodControllerIntegration(t *testing.T) { +func TestPlaceholderPodControllerIntegrationWithIng(t *testing.T) { ing := placeholderTestIng.DeepCopy() spc := placeholderSpc.DeepCopy() spc.Labels = manifests.GetTopLevelLabels() @@ -213,7 +229,158 @@ func TestPlaceholderPodControllerIntegration(t *testing.T) { assert.Equal(t, labels, updatedPlaceholder.Spec.Selector.MatchLabels, "selector labels should have been retained") } -func TestPlaceholderPodControllerNoManagedByLabels(t *testing.T) { +func TestPlaceholderPodControllerIntegrationWithNic(t *testing.T) { + ctx := context.Background() + ctx = logr.NewContext(ctx, zap.New()) + nic := placeholderTestNginxIngress.DeepCopy() + spc := getDefaultNginxSpc(nic) + spc.SetOwnerReferences(manifests.GetOwnerRefs(nic, true)) + + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + require.NoError(t, secv1.AddToScheme(scheme)) + require.NoError(t, appsv1.AddToScheme(scheme)) + + c := fake.NewClientBuilder().WithScheme(scheme).Build() + recorder := record.NewFakeRecorder(10) + require.NoError(t, c.Create(ctx, nic)) + require.NoError(t, c.Create(ctx, spc)) + + p := &PlaceholderPodController{ + client: c, + config: &config.Config{Registry: "test-registry"}, + ingressManager: nil, + events: recorder, + } + + // 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 := map[string]string{"app": spc.Name} + 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": nic.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}, + }, + }, + }}, + }), + }, + } + fmt.Printf("%v", dep) + 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) + + // Update the secret class generation + spc.Generation = 234 + expected.Template.Annotations["kubernetes.azure.com/observed-generation"] = "234" + require.NoError(t, c.Update(ctx, spc)) + + 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 generation annotation was updated + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(dep), dep)) + assert.Equal(t, expected, dep.Spec) + + // Change the ingress resource's class + nic.Spec.IngressClassName = "" + require.NoError(t, c.Update(ctx, nic)) + + 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 deleted + require.True(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(dep), dep))) + + // Prove idempotence + require.True(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(dep), dep))) + + // Prove that placeholder deployment retains immutable fields during updates + oldPlaceholder := &appsv1.Deployment{} + labels := map[string]string{"foo": "bar", "fizz": "buzz"} + oldPlaceholder.Spec.Selector = &metav1.LabelSelector{MatchLabels: labels} + oldPlaceholder.Name = "immutable-test" + require.NoError(t, c.Create(ctx, oldPlaceholder), "failed to create old placeholder deployment") + beforeErrCount = testutils.GetErrMetricCount(t, placeholderPodControllerName) + beforeReconcileCount = testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess) + _, err = p.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(oldPlaceholder)}) + require.NoError(t, err) + require.Equal(t, testutils.GetErrMetricCount(t, placeholderPodControllerName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, placeholderPodControllerName, metrics.LabelSuccess), beforeReconcileCount) + + updatedPlaceholder := &appsv1.Deployment{} + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(oldPlaceholder), updatedPlaceholder), "failed to get updated placeholder deployment") + assert.Equal(t, labels, updatedPlaceholder.Spec.Selector.MatchLabels, "selector labels should have been retained") +} + +func TestPlaceholderPodControllerNoManagedByLabelsWithIng(t *testing.T) { ing := placeholderTestIng.DeepCopy() spc := placeholderSpc.DeepCopy() spc.Labels = map[string]string{} @@ -333,7 +500,7 @@ func TestPlaceholderPodControllerNoManagedByLabels(t *testing.T) { require.False(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(dep), dep))) } -func TestNewPlaceholderPodController(t *testing.T) { +func TestNewPlaceholderPodControllerWithIng(t *testing.T) { m, err := manager.New(restConfig, manager.Options{Metrics: metricsserver.Options{BindAddress: ":0"}}) require.NoError(t, err) @@ -359,7 +526,7 @@ func TestNewPlaceholderPodController(t *testing.T) { require.NoError(t, err) } -func TestGetCurrentDeployment(t *testing.T) { +func TestGetCurrentDeploymentWithIng(t *testing.T) { dep := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: "test-deployment", @@ -379,3 +546,27 @@ func TestGetCurrentDeployment(t *testing.T) { require.NoError(t, err) require.Nil(t, dep) } + +func getDefaultNginxSpc(nic *v1alpha1.NginxIngressController) *secv1.SecretProviderClass { + spc := &secv1.SecretProviderClass{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "secrets-store.csi.x-k8s.io/v1", + Kind: "SecretProviderClass", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNginxCertName(nic), + Namespace: "approutingsystem", + Labels: manifests.GetTopLevelLabels(), + Generation: 123, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: nic.APIVersion, + Controller: util.BoolPtr(true), + Kind: "NginxIngressController", + Name: nic.Name, + UID: nic.UID, + }}, + }, + } + + return spc +} diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index cb7959a5..5950fbb2 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "github.com/Azure/aks-app-routing-operator/pkg/controller/keyvault" "net/url" "time" @@ -532,5 +533,9 @@ func ToNginxIngressConfig(nic *approutingv1alpha1.NginxIngressController, defaul nginxIng.DefaultSSLCertificate = nic.Spec.DefaultSSLCertificate.Secret.Namespace + "/" + nic.Spec.DefaultSSLCertificate.Secret.Name } + if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != "" { + defaultCert := "approutingsystem/" + keyvault.DefaultNginxCertName(nic) + nginxIng.DefaultSSLCertificate = defaultCert + } return nginxIng } From 2eff2510f712ae3116d373bea5ed98d6cab6df61 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Tue, 13 Feb 2024 15:09:36 -0500 Subject: [PATCH 02/36] unit tests for nginx_ingress_controller and parameter adjustments --- pkg/controller/controller.go | 2 +- .../keyvault/nginx_secret_provider_class.go | 2 +- .../keyvault/placeholder_pod_test.go | 6 +- .../nginxingress/nginx_ingress_controller.go | 14 ++-- .../nginx_ingress_controller_test.go | 80 +++++++++++++++++++ 5 files changed, 94 insertions(+), 10 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 5bac063a..411f5b60 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -185,7 +185,7 @@ func setupControllers(mgr ctrl.Manager, conf *config.Config, lgr logr.Logger, cl return fmt.Errorf("setting up ingress secret provider class reconciler: %w", err) } lgr.Info("setting up nginx keyvault secret provider class reconciler") - if err := keyvault.NewNginxSecretProviderClassReconciler(mgr, conf, ingressManager); err != nil { + if err := keyvault.NewNginxSecretProviderClassReconciler(mgr, conf); err != nil { return fmt.Errorf("setting up crd secret provider class reconciler: %w", err) } lgr.Info("setting up keyvault placeholder pod controller") diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 22998db2..8bf0d4ac 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -40,7 +40,7 @@ type NginxSecretProviderClassReconciler struct { config *config.Config } -func NewNginxSecretProviderClassReconciler(manager ctrl.Manager, conf *config.Config, ingressManager IngressManager) error { +func NewNginxSecretProviderClassReconciler(manager ctrl.Manager, conf *config.Config) error { metrics.InitControllerMetrics(nginxSecretProviderControllerName) if conf.DisableKeyvault { return nil diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 3649e584..11faeadd 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -500,7 +500,7 @@ func TestPlaceholderPodControllerNoManagedByLabelsWithIng(t *testing.T) { require.False(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(dep), dep))) } -func TestNewPlaceholderPodControllerWithIng(t *testing.T) { +func TestNewPlaceholderPodController(t *testing.T) { m, err := manager.New(restConfig, manager.Options{Metrics: metricsserver.Options{BindAddress: ":0"}}) require.NoError(t, err) @@ -524,6 +524,10 @@ func TestNewPlaceholderPodControllerWithIng(t *testing.T) { err = NewPlaceholderPodController(m, conf, ingressManager) require.NoError(t, err) + + // test nil ingress manager for nginx ingress controller + err = NewPlaceholderPodController(m, conf, nil) + require.NoError(t, err) } func TestGetCurrentDeploymentWithIng(t *testing.T) { diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 5950fbb2..4b8e9040 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -528,14 +528,14 @@ func ToNginxIngressConfig(nic *approutingv1alpha1.NginxIngressController, defaul }, } - if nic.Spec.DefaultSSLCertificate != nil && - nic.Spec.DefaultSSLCertificate.Secret.Name != "" && nic.Spec.DefaultSSLCertificate.Secret.Namespace != "" { - nginxIng.DefaultSSLCertificate = nic.Spec.DefaultSSLCertificate.Secret.Namespace + "/" + nic.Spec.DefaultSSLCertificate.Secret.Name + if nic.Spec.DefaultSSLCertificate != nil { + if nic.Spec.DefaultSSLCertificate.Secret != nil && nic.Spec.DefaultSSLCertificate.Secret.Name != "" && nic.Spec.DefaultSSLCertificate.Secret.Namespace != "" { + nginxIng.DefaultSSLCertificate = nic.Spec.DefaultSSLCertificate.Secret.Namespace + "/" + nic.Spec.DefaultSSLCertificate.Secret.Name + } + if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != "" { + nginxIng.DefaultSSLCertificate = "approutingsystem/" + keyvault.DefaultNginxCertName(nic) + } } - if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != "" { - defaultCert := "approutingsystem/" + keyvault.DefaultNginxCertName(nic) - nginxIng.DefaultSSLCertificate = defaultCert - } return nginxIng } diff --git a/pkg/controller/nginxingress/nginx_ingress_controller_test.go b/pkg/controller/nginxingress/nginx_ingress_controller_test.go index 01a2b8b6..94dad0c2 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller_test.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller_test.go @@ -73,6 +73,86 @@ func TestReconcileResources(t *testing.T) { } }) + t.Run("valid resources with defaultSSLCertificate Secret", func(t *testing.T) { + cl := fake.NewFakeClient() + events := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + conf: &config.Config{ + NS: "default", + }, + client: cl, + events: events, + } + + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "ingressClassName", + ControllerNamePrefix: "prefix", + DefaultSSLCertificate: &approutingv1alpha1.DefaultSSLCertificate{Secret: &approutingv1alpha1.Secret{Name: "test-name", Namespace: "test-namespace"}}, + }, + } + res := n.ManagedResources(nic) + + managed, err := n.ReconcileResource(context.Background(), nic, res) + require.NoError(t, err) + require.True(t, len(managed) == len(res.Objects())-1, "expected all resources to be returned as managed except the namespace") + + // prove objects were created + for _, obj := range res.Objects() { + require.NoError(t, cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)) + } + + // no events + select { + case <-events.Events: + require.Fail(t, "expected no events") + default: + } + }) + + t.Run("valid resources with defaultSSLCertificate key vault URI", func(t *testing.T) { + cl := fake.NewFakeClient() + events := record.NewFakeRecorder(10) + n := &nginxIngressControllerReconciler{ + conf: &config.Config{ + NS: "default", + }, + client: cl, + events: events, + } + + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nic", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "ingressClassName", + ControllerNamePrefix: "prefix", + DefaultSSLCertificate: &approutingv1alpha1.DefaultSSLCertificate{KeyVaultURI: "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34"}, + }, + } + res := n.ManagedResources(nic) + + managed, err := n.ReconcileResource(context.Background(), nic, res) + require.NoError(t, err) + require.True(t, len(managed) == len(res.Objects())-1, "expected all resources to be returned as managed except the namespace") + + // prove objects were created + for _, obj := range res.Objects() { + require.NoError(t, cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)) + } + + // no events + select { + case <-events.Events: + require.Fail(t, "expected no events") + default: + } + }) + t.Run("invalid resources", func(t *testing.T) { cl := fake.NewFakeClient() events := record.NewFakeRecorder(10) From 28103b02586d6487bf44659dc9928410e897c972 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Thu, 15 Feb 2024 09:52:55 -0500 Subject: [PATCH 03/36] tests and fix for app-routing-system namespace --- pkg/controller/keyvault/nginx_secret_provider_class.go | 7 +++++-- .../keyvault/nginx_secret_provider_class_test.go | 4 ++-- pkg/controller/keyvault/placeholder_pod_test.go | 4 ++-- pkg/controller/nginxingress/nginx_ingress_controller.go | 2 +- testing/e2e/suites/nginxIngressController.go | 9 +++++++++ 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 8bf0d4ac..1cea441c 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -81,7 +81,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req if err != nil { return result, client.IgnoreNotFound(err) } - logger = logger.WithValues("name", nic.Name, "namespace", "approutingsystem", "generation", nic.Generation) + logger = logger.WithValues("name", nic.Name, "namespace", "app-routing-system", "generation", nic.Generation) spc := &secv1.SecretProviderClass{ TypeMeta: metav1.TypeMeta{ @@ -90,7 +90,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: "approutingsystem", + Namespace: "app-routing-system", Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, @@ -140,6 +140,9 @@ func (i *NginxSecretProviderClassReconciler) buildSPC(nic *approutingv1alpha1.Ng if nic.Spec.IngressClassName == "" { return false, nil } + if nic.Spec.DefaultSSLCertificate == nil { + return false, nil + } certURI := nic.Spec.DefaultSSLCertificate.KeyVaultURI if certURI == "" { diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index ab252617..6d0fdc74 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -82,7 +82,7 @@ func TestNginxSecretProviderClassReconcilerIntegration(t *testing.T) { // Prove it exists spc := &secv1.SecretProviderClass{} spc.Name = DefaultNginxCertName(nic) - spc.Namespace = "approutingsystem" + spc.Namespace = "app-routing-system" require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc)) expected := &secv1.SecretProviderClass{ @@ -191,7 +191,7 @@ func TestNginxSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testin }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: "approutingsystem", + Namespace: "app-routing-system", Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 11faeadd..4863844d 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -380,7 +380,7 @@ func TestPlaceholderPodControllerIntegrationWithNic(t *testing.T) { assert.Equal(t, labels, updatedPlaceholder.Spec.Selector.MatchLabels, "selector labels should have been retained") } -func TestPlaceholderPodControllerNoManagedByLabelsWithIng(t *testing.T) { +func TestPlaceholderPodControllerNoManagedByLabels(t *testing.T) { ing := placeholderTestIng.DeepCopy() spc := placeholderSpc.DeepCopy() spc.Labels = map[string]string{} @@ -559,7 +559,7 @@ func getDefaultNginxSpc(nic *v1alpha1.NginxIngressController) *secv1.SecretProvi }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: "approutingsystem", + Namespace: "app-routing-system", Labels: manifests.GetTopLevelLabels(), Generation: 123, OwnerReferences: []metav1.OwnerReference{{ diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 4b8e9040..089c0e33 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -533,7 +533,7 @@ func ToNginxIngressConfig(nic *approutingv1alpha1.NginxIngressController, defaul nginxIng.DefaultSSLCertificate = nic.Spec.DefaultSSLCertificate.Secret.Namespace + "/" + nic.Spec.DefaultSSLCertificate.Secret.Name } if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != "" { - nginxIng.DefaultSSLCertificate = "approutingsystem/" + keyvault.DefaultNginxCertName(nic) + nginxIng.DefaultSSLCertificate = "app-routing-system/" + keyvault.DefaultNginxCertName(nic) } } diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 9b9b444d..27b6cd96 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -112,6 +112,15 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("able to create NginxIngressController despite missing Secret field'%s'", testNIC.Spec.ControllerNamePrefix) } + testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") + testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ + KeyVaultURI: "Invalid.URI", + } + lgr.Info("creating NginxIngressController with invalid Secret field") + if err := c.Create(ctx, testNIC); err == nil { + return fmt.Errorf("able to create NginxIngressController despite invalid key vault uri'%s'", testNIC.Spec.ControllerNamePrefix) + } + lgr.Info("finished testing") return nil }, From ca2b0e4dace9d0704b5f407dd17573ea61d98601 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Thu, 15 Feb 2024 10:47:46 -0500 Subject: [PATCH 04/36] keyVaultUri to pointer and associated changes --- api/v1alpha1/nginxingresscontroller_types.go | 2 +- pkg/controller/controller.go | 2 +- .../keyvault/nginx_secret_provider_class.go | 6 ++--- .../nginx_secret_provider_class_test.go | 25 +++++++++++-------- pkg/controller/keyvault/placeholder_pod.go | 2 +- .../keyvault/placeholder_pod_test.go | 3 ++- .../nginxingress/nginx_ingress_controller.go | 2 +- .../nginx_ingress_controller_test.go | 4 +-- testing/e2e/suites/nginxIngressController.go | 3 ++- 9 files changed, 28 insertions(+), 21 deletions(-) diff --git a/api/v1alpha1/nginxingresscontroller_types.go b/api/v1alpha1/nginxingresscontroller_types.go index 7638a03f..3ed8cd7b 100644 --- a/api/v1alpha1/nginxingresscontroller_types.go +++ b/api/v1alpha1/nginxingresscontroller_types.go @@ -64,7 +64,7 @@ type DefaultSSLCertificate struct { // Secret in the form of a Key Vault URI // +optional - KeyVaultURI string `json:"keyVaultURI"` + KeyVaultURI *string `json:"keyVaultURI"` } // Secret is a struct that holds a name and namespace to be used in DefaultSSLCertificate diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 411f5b60..be1d8e23 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -186,7 +186,7 @@ func setupControllers(mgr ctrl.Manager, conf *config.Config, lgr logr.Logger, cl } lgr.Info("setting up nginx keyvault secret provider class reconciler") if err := keyvault.NewNginxSecretProviderClassReconciler(mgr, conf); err != nil { - return fmt.Errorf("setting up crd secret provider class reconciler: %w", err) + return fmt.Errorf("setting up nginx secret provider class reconciler: %w", err) } lgr.Info("setting up keyvault placeholder pod controller") if err := keyvault.NewPlaceholderPodController(mgr, conf, ingressManager); err != nil { diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 1cea441c..0942cf24 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -140,11 +140,11 @@ func (i *NginxSecretProviderClassReconciler) buildSPC(nic *approutingv1alpha1.Ng if nic.Spec.IngressClassName == "" { return false, nil } - if nic.Spec.DefaultSSLCertificate == nil { + if nic.Spec.DefaultSSLCertificate == nil || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { return false, nil } - - certURI := nic.Spec.DefaultSSLCertificate.KeyVaultURI + + certURI := *nic.Spec.DefaultSSLCertificate.KeyVaultURI if certURI == "" { return false, nil } diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index 6d0fdc74..b1248a9c 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -32,6 +32,7 @@ import ( ) var ( + spcTestKeyVaultURI = "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34" spcTestNginxIngressClassName = "webapprouting.kubernetes.azure.com" spcTestNginxIngress = &v1alpha1.NginxIngressController{ ObjectMeta: metav1.ObjectMeta{ @@ -42,7 +43,7 @@ var ( IngressClassName: spcTestNginxIngressClassName, DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{ Secret: nil, - KeyVaultURI: "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34"}, + KeyVaultURI: &spcTestKeyVaultURI}, }, } ) @@ -116,7 +117,8 @@ func TestNginxSecretProviderClassReconcilerIntegration(t *testing.T) { require.Greater(t, testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) // Remove the cert's version from the nic - nic.Spec.DefaultSSLCertificate.KeyVaultURI = "https://testvault.vault.azure.net/certificates/testcert" + testKvUri := "https://testvault.vault.azure.net/certificates/testcert" + nic.Spec.DefaultSSLCertificate.KeyVaultURI = &testKvUri require.NoError(t, n.client.Update(ctx, nic)) beforeErrCount = testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) @@ -131,7 +133,7 @@ func TestNginxSecretProviderClassReconcilerIntegration(t *testing.T) { assert.Equal(t, expected.Spec, spc.Spec) // Remove the cert annotation from the nic - nic.Spec.DefaultSSLCertificate.KeyVaultURI = "" + nic.Spec.DefaultSSLCertificate.KeyVaultURI = nil require.NoError(t, n.client.Update(ctx, nic)) beforeErrCount = testutils.GetErrMetricCount(t, nginxSecretProviderControllerName) beforeRequestCount = testutils.GetReconcileMetricCount(t, nginxSecretProviderControllerName, metrics.LabelSuccess) @@ -212,8 +214,8 @@ func TestNginxSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testin require.NoError(t, n.client.Update(ctx, spc)) assert.Equal(t, 0, len(spc.Labels)) - // Remove the cert annotation from the nic - nic.Spec.DefaultSSLCertificate.KeyVaultURI = "" + // Remove the cert uri from the nic + nic.Spec.DefaultSSLCertificate.KeyVaultURI = nil require.NoError(t, n.client.Update(ctx, nic)) // Reconcile both changes @@ -265,8 +267,9 @@ func TestNginxSecretProviderClassReconcilerInvalidURL(t *testing.T) { ctx := context.Background() ctx = logr.NewContext(ctx, logr.Discard()) // Create the nic + invalidUri := "inv@lid URL" nic := spcTestNginxIngress.DeepCopy() - nic.Spec.DefaultSSLCertificate.KeyVaultURI = "inv@lid URL" + nic.Spec.DefaultSSLCertificate.KeyVaultURI = &invalidUri scheme := runtime.NewScheme() require.NoError(t, v1alpha1.AddToScheme(scheme)) @@ -307,7 +310,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { invalidURLIng := &v1alpha1.NginxIngressController{ Spec: v1alpha1.NginxIngressControllerSpec{ IngressClassName: spcTestNginxIngressClassName, - DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{KeyVaultURI: ""}, + DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{KeyVaultURI: nil}, }, } @@ -331,7 +334,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { t.Run("url with control character", func(t *testing.T) { nic := invalidURLIng.DeepCopy() cc := string([]byte{0x7f}) - nic.Spec.DefaultSSLCertificate.KeyVaultURI = cc + nic.Spec.DefaultSSLCertificate.KeyVaultURI = &cc ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) assert.False(t, ok) @@ -341,7 +344,8 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { t.Run("url with one path segment", func(t *testing.T) { nic := invalidURLIng.DeepCopy() - nic.Spec.DefaultSSLCertificate.KeyVaultURI = "http://test.com/foo" + fooTestUri := "http://test.com/foo" + nic.Spec.DefaultSSLCertificate.KeyVaultURI = &fooTestUri ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) assert.False(t, ok) @@ -382,7 +386,8 @@ func TestNginxSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { } nic := spcTestNginxIngress.DeepCopy() - nic.Spec.DefaultSSLCertificate.KeyVaultURI = "https://test.vault.azure.net/secrets/test-secret" + testSecretUri := "https://test.vault.azure.net/secrets/test-secret" + nic.Spec.DefaultSSLCertificate.KeyVaultURI = &testSecretUri spc := &secv1.SecretProviderClass{} ok, err := n.buildSPC(nic, spc) diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 98ffab7d..c9fa5ea9 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -121,7 +121,7 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } } - if nic.Name == "" || nic.Spec.IngressClassName == "" || nic.Spec.DefaultSSLCertificate.KeyVaultURI == "" { + if nic.Name == "" || nic.Spec.IngressClassName == "" || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { logger.Info("cleaning unused placeholder pod deployment") logger.Info("getting placeholder deployment") toCleanDeployment := &appsv1.Deployment{} diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 4863844d..20910d4a 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -48,6 +48,7 @@ var ( IngressClassName: &placeholderTestIngClassName, }, } + placeholderTestUri = "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34" placeholderTestNginxIngress = &v1alpha1.NginxIngressController{ ObjectMeta: metav1.ObjectMeta{ Name: "test-nic", @@ -57,7 +58,7 @@ var ( IngressClassName: spcTestNginxIngressClassName, DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{ Secret: nil, // nil Secret since SPC method for DefaultSSLCertificate using placeholder pods only uses the KeyVaultURI - KeyVaultURI: "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34"}, + KeyVaultURI: &placeholderTestUri}, }, } placeholderSpc = &secv1.SecretProviderClass{ diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 089c0e33..08853bc0 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -532,7 +532,7 @@ func ToNginxIngressConfig(nic *approutingv1alpha1.NginxIngressController, defaul if nic.Spec.DefaultSSLCertificate.Secret != nil && nic.Spec.DefaultSSLCertificate.Secret.Name != "" && nic.Spec.DefaultSSLCertificate.Secret.Namespace != "" { nginxIng.DefaultSSLCertificate = nic.Spec.DefaultSSLCertificate.Secret.Namespace + "/" + nic.Spec.DefaultSSLCertificate.Secret.Name } - if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != "" { + if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != nil { nginxIng.DefaultSSLCertificate = "app-routing-system/" + keyvault.DefaultNginxCertName(nic) } } diff --git a/pkg/controller/nginxingress/nginx_ingress_controller_test.go b/pkg/controller/nginxingress/nginx_ingress_controller_test.go index 94dad0c2..761618a2 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller_test.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller_test.go @@ -123,7 +123,7 @@ func TestReconcileResources(t *testing.T) { client: cl, events: events, } - + kvUri := "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34" nic := &approutingv1alpha1.NginxIngressController{ ObjectMeta: metav1.ObjectMeta{ Name: "nic", @@ -131,7 +131,7 @@ func TestReconcileResources(t *testing.T) { Spec: approutingv1alpha1.NginxIngressControllerSpec{ IngressClassName: "ingressClassName", ControllerNamePrefix: "prefix", - DefaultSSLCertificate: &approutingv1alpha1.DefaultSSLCertificate{KeyVaultURI: "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34"}, + DefaultSSLCertificate: &approutingv1alpha1.DefaultSSLCertificate{KeyVaultURI: &kvUri}, }, } res := n.ManagedResources(nic) diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 27b6cd96..42ae1d1f 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -112,9 +112,10 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("able to create NginxIngressController despite missing Secret field'%s'", testNIC.Spec.ControllerNamePrefix) } + invalidUri := "Invalid.URI" testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - KeyVaultURI: "Invalid.URI", + KeyVaultURI: &invalidUri, } lgr.Info("creating NginxIngressController with invalid Secret field") if err := c.Create(ctx, testNIC); err == nil { From 506e83626c05d45c1c79a047e3045d6a143d4e94 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Thu, 15 Feb 2024 14:31:31 -0500 Subject: [PATCH 05/36] Generic BuildSPC function and namespace fixes --- .../keyvault/ingress_secret_provider_class.go | 108 +++------------- .../ingress_secret_provider_class_test.go | 84 ++++--------- pkg/controller/keyvault/kv_util.go | 115 ++++++++++++++++++ .../keyvault/nginx_secret_provider_class.go | 99 +-------------- .../nginx_secret_provider_class_test.go | 22 ++-- .../keyvault/placeholder_pod_test.go | 5 +- .../nginxingress/nginx_ingress_controller.go | 2 +- 7 files changed, 170 insertions(+), 265 deletions(-) create mode 100644 pkg/controller/keyvault/kv_util.go diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index 7952840e..4856394c 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -5,11 +5,12 @@ package keyvault import ( "context" - "encoding/json" "fmt" - "net/url" - "strings" - + "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" "github.com/go-logr/logr" netv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -17,13 +18,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" 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" ) var ( @@ -103,7 +97,19 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re }, } logger = logger.WithValues("spc", spc.Name) - ok, err := i.buildSPC(ing, spc) + + // Checking if we manage the ingress. All false cases without an error are assumed that we don't manage it + ok, err := i.ingressManager.IsManaging(ing) + + if err != nil { + ok = false + err = fmt.Errorf("determining if ingress is managed: %w", err) + } + + if ok { + ok, err = BuildSPC(ing, spc, i.config) + } + if err != nil { logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) @@ -136,81 +142,3 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re return result, nil } - -func (i *IngressSecretProviderClassReconciler) buildSPC(ing *netv1.Ingress, spc *secv1.SecretProviderClass) (bool, error) { - if ing.Spec.IngressClassName == nil || ing.Annotations == nil { - return false, nil - } - - managed, err := i.ingressManager.IsManaging(ing) - if err != nil { - return false, fmt.Errorf("determining if ingress is managed: %w", err) - } - if !managed { - return false, nil - } - - certURI := ing.Annotations["kubernetes.azure.com/tls-cert-keyvault-uri"] - if certURI == "" { - return false, nil - } - - uri, err := url.Parse(certURI) - if err != nil { - return false, err - } - vaultName := strings.Split(uri.Host, ".")[0] - chunks := strings.Split(uri.Path, "/") - if len(chunks) < 3 { - return false, fmt.Errorf("invalid secret uri: %s", certURI) - } - secretName := chunks[2] - p := map[string]interface{}{ - "objectName": secretName, - "objectType": "secret", - } - if len(chunks) > 3 { - p["objectVersion"] = chunks[3] - } - - params, err := json.Marshal(p) - if err != nil { - return false, err - } - objects, err := json.Marshal(map[string]interface{}{"array": []string{string(params)}}) - if err != nil { - return false, err - } - - spc.Spec = secv1.SecretProviderClassSpec{ - Provider: secv1.Provider("azure"), - SecretObjects: []*secv1.SecretObject{{ - SecretName: fmt.Sprintf("keyvault-%s", ing.Name), - Type: "kubernetes.io/tls", - Data: []*secv1.SecretObjectData{ - { - ObjectName: secretName, - Key: "tls.key", - }, - { - ObjectName: secretName, - Key: "tls.crt", - }, - }, - }}, - // https://azure.github.io/secrets-store-csi-driver-provider-azure/docs/getting-started/usage/#create-your-own-secretproviderclass-object - Parameters: map[string]string{ - "keyvaultName": vaultName, - "useVMManagedIdentity": "true", - "userAssignedIdentityID": i.config.MSIClientID, - "tenantId": i.config.TenantID, - "objects": string(objects), - }, - } - - if i.config.Cloud != "" { - spc.Spec.Parameters[kvcsi.CloudNameParameter] = i.config.Cloud - } - - return true, nil -} diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index 90ded194..568824cd 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -44,6 +44,7 @@ var ( IngressClassName: &spcTestIngressClassName, }, } + spcTestDefaultConf = BuildTestSpcConfig("test-msi", "test-tenant", "test-cloud") ) func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { @@ -316,16 +317,6 @@ func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) { } func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { - i := &IngressSecretProviderClassReconciler{ - ingressManager: NewIngressManagerFromFn(func(ing *netv1.Ingress) (bool, error) { - if *ing.Spec.IngressClassName == spcTestIngressClassName { - return true, nil - } - - return false, nil - }), - } - invalidURLIng := &netv1.Ingress{ Spec: netv1.IngressSpec{ IngressClassName: &spcTestIngressClassName, @@ -337,26 +328,26 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { ing.Spec.IngressClassName = nil ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL"} - ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) - t.Run("incorrect ingress class", func(t *testing.T) { - 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"} - - ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) - assert.False(t, ok) - require.NoError(t, err) - }) + //t.Run("incorrect ingress class", func(t *testing.T) { + // 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"} + // + // ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + // assert.False(t, ok) + // require.NoError(t, err) + //}) t.Run("nil annotations", func(t *testing.T) { ing := invalidURLIng.DeepCopy() - ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -365,7 +356,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { ing := invalidURLIng.DeepCopy() ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": ""} - ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -375,7 +366,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { cc := string([]byte{0x7f}) ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": cc} - ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) _, expectedErr := url.Parse(cc) // the exact error depends on operating system require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) @@ -385,7 +376,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { 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{}) + ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.EqualError(t, err, "invalid secret uri: http://test.com/foo") }) @@ -417,26 +408,13 @@ func TestIngressSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - i := &IngressSecretProviderClassReconciler{ - config: &config.Config{ - Cloud: c.configCloud, - }, - ingressManager: NewIngressManagerFromFn(func(ing *netv1.Ingress) (bool, error) { - if *ing.Spec.IngressClassName == spcTestIngressClassName { - return true, nil - } - - return false, nil - }), - } - 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{} - ok, err := i.buildSPC(ing, spc) + ok, err := BuildSPC(ing, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) require.NoError(t, err, "building SPC should not error") require.True(t, ok, "SPC should be built") @@ -447,26 +425,12 @@ func TestIngressSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { } } -func TestIngressSecretProviderClassReconcilerBuildSPCFailedIsManaging(t *testing.T) { - i := &IngressSecretProviderClassReconciler{ - ingressManager: NewIngressManagerFromFn(func(ing *netv1.Ingress) (bool, error) { - return false, fmt.Errorf("failed to get ingress") - }), - } - - ing := &netv1.Ingress{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-ingress", - Annotations: map[string]string{}, - }, - Spec: netv1.IngressSpec{ - IngressClassName: &spcTestIngressClassName, - }, +func BuildTestSpcConfig(msiClient, tenantID, cloud string) *config.Config { + spcTestConf := config.Config{ + MSIClientID: msiClient, + TenantID: tenantID, + Cloud: cloud, } - spc := &secv1.SecretProviderClass{} - ok, err := i.buildSPC(ing, spc) - require.False(t, ok) - require.Error(t, err) - require.ErrorContains(t, err, "determining if ingress is managed") -} \ No newline at end of file + return &spcTestConf +} diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go new file mode 100644 index 00000000..f7439df3 --- /dev/null +++ b/pkg/controller/keyvault/kv_util.go @@ -0,0 +1,115 @@ +package keyvault + +import ( + "encoding/json" + "fmt" + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/config" + kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" + v1 "k8s.io/api/networking/v1" + "net/url" + secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" + "strings" +) + +var NginxNamePrefix = "keyvault-nginx-" + +func BuildSPC(ic interface{}, spc *secv1.SecretProviderClass, config *config.Config) (bool, error) { + var certURI, specSecretName string + + if nic, ok := ic.(*v1alpha1.NginxIngressController); ok { + if nic.Spec.IngressClassName == "" { + return false, nil + } + if nic.Spec.DefaultSSLCertificate == nil || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { + return false, nil + } + certURI = *nic.Spec.DefaultSSLCertificate.KeyVaultURI + specSecretName = DefaultNginxCertName(nic) + } + + if ing, ok := ic.(*v1.Ingress); ok { + if ing.Spec.IngressClassName == nil || ing.Annotations == nil { + return false, nil + } + + certURI = ing.Annotations["kubernetes.azure.com/tls-cert-keyvault-uri"] + specSecretName = fmt.Sprintf("keyvault-%s", ing.Name) + } + + if certURI == "" { + return false, nil + } + + uri, err := url.Parse(certURI) + if err != nil { + return false, err + } + vaultName := strings.Split(uri.Host, ".")[0] + chunks := strings.Split(uri.Path, "/") + if len(chunks) < 3 { + return false, fmt.Errorf("invalid secret uri: %s", certURI) + } + secretName := chunks[2] + p := map[string]interface{}{ + "objectName": secretName, + "objectType": "secret", + } + if len(chunks) > 3 { + p["objectVersion"] = chunks[3] + } + + params, err := json.Marshal(p) + if err != nil { + return false, err + } + objects, err := json.Marshal(map[string]interface{}{"array": []string{string(params)}}) + if err != nil { + return false, err + } + + spc.Spec = secv1.SecretProviderClassSpec{ + Provider: secv1.Provider("azure"), + SecretObjects: []*secv1.SecretObject{{ + SecretName: specSecretName, + Type: "kubernetes.io/tls", + Data: []*secv1.SecretObjectData{ + { + ObjectName: secretName, + Key: "tls.key", + }, + { + ObjectName: secretName, + Key: "tls.crt", + }, + }, + }}, + // https://azure.github.io/secrets-store-csi-driver-provider-azure/docs/getting-started/usage/#create-your-own-secretproviderclass-object + Parameters: map[string]string{ + "keyvaultName": vaultName, + "useVMManagedIdentity": "true", + "userAssignedIdentityID": config.MSIClientID, + "tenantId": config.TenantID, + "objects": string(objects), + }, + } + + if config.Cloud != "" { + spc.Spec.Parameters[kvcsi.CloudNameParameter] = config.Cloud + } + + return true, nil +} + +// DefaultNginxCertName returns a default name for the nginx certificate name using the IngressClassName from the spec. +// Truncates characters in the IngressClassName passed the max secret length (255) if the IngressClassName and the default namespace are over the limit +func DefaultNginxCertName(nic *v1alpha1.NginxIngressController) string { + secretMaxSize := 255 + certName := NginxNamePrefix + nic.Name + + if len(certName) > secretMaxSize { + return certName[0:secretMaxSize] + } + + return certName +} diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 0942cf24..ee1a1f01 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -5,12 +5,7 @@ package keyvault import ( "context" - "encoding/json" - "fmt" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" - "net/url" - "strings" - "github.com/go-logr/logr" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" @@ -23,12 +18,10 @@ import ( "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" ) var ( nginxSecretProviderControllerName = controllername.New("keyvault", "nginx", "secret", "provider") - NginxNamePrefix = "keyvault-nginx-" ) // NginxSecretProviderClassReconciler manages a SecretProviderClass for each nginx ingress controller that @@ -81,7 +74,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req if err != nil { return result, client.IgnoreNotFound(err) } - logger = logger.WithValues("name", nic.Name, "namespace", "app-routing-system", "generation", nic.Generation) + logger = logger.WithValues("name", nic.Name, "namespace", config.DefaultNs, "generation", nic.Generation) spc := &secv1.SecretProviderClass{ TypeMeta: metav1.TypeMeta{ @@ -90,7 +83,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: "app-routing-system", + Namespace: config.DefaultNs, Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, @@ -102,7 +95,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req }, } logger = logger.WithValues("spc", spc.Name) - ok, err := i.buildSPC(nic, spc) + ok, err := BuildSPC(nic, spc, i.config) if err != nil { logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) @@ -135,89 +128,3 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req return result, nil } - -func (i *NginxSecretProviderClassReconciler) buildSPC(nic *approutingv1alpha1.NginxIngressController, spc *secv1.SecretProviderClass) (bool, error) { - if nic.Spec.IngressClassName == "" { - return false, nil - } - if nic.Spec.DefaultSSLCertificate == nil || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { - return false, nil - } - - certURI := *nic.Spec.DefaultSSLCertificate.KeyVaultURI - if certURI == "" { - return false, nil - } - - uri, err := url.Parse(certURI) - if err != nil { - return false, err - } - vaultName := strings.Split(uri.Host, ".")[0] - chunks := strings.Split(uri.Path, "/") - if len(chunks) < 3 { - return false, fmt.Errorf("invalid secret uri: %s", certURI) - } - secretName := chunks[2] - p := map[string]interface{}{ - "objectName": secretName, - "objectType": "secret", - } - if len(chunks) > 3 { - p["objectVersion"] = chunks[3] - } - - params, err := json.Marshal(p) - if err != nil { - return false, err - } - objects, err := json.Marshal(map[string]interface{}{"array": []string{string(params)}}) - if err != nil { - return false, err - } - - spc.Spec = secv1.SecretProviderClassSpec{ - Provider: secv1.Provider("azure"), - SecretObjects: []*secv1.SecretObject{{ - SecretName: DefaultNginxCertName(nic), - Type: "kubernetes.io/tls", - Data: []*secv1.SecretObjectData{ - { - ObjectName: secretName, - Key: "tls.key", - }, - { - ObjectName: secretName, - Key: "tls.crt", - }, - }, - }}, - // https://azure.github.io/secrets-store-csi-driver-provider-azure/docs/getting-started/usage/#create-your-own-secretproviderclass-object - Parameters: map[string]string{ - "keyvaultName": vaultName, - "useVMManagedIdentity": "true", - "userAssignedIdentityID": i.config.MSIClientID, - "tenantId": i.config.TenantID, - "objects": string(objects), - }, - } - - if i.config.Cloud != "" { - spc.Spec.Parameters[kvcsi.CloudNameParameter] = i.config.Cloud - } - - return true, nil -} - -// DefaultNginxCertName returns a default name for the nginx certificate name using the IngressClassName from the spec. -// Truncates characters in the IngressClassName passed the max secret length (255) if the IngressClassName and the default namespace are over the limit -func DefaultNginxCertName(nic *approutingv1alpha1.NginxIngressController) string { - secretMaxSize := 255 - certName := NginxNamePrefix + nic.Name - - if len(certName) > secretMaxSize { - return certName[0:secretMaxSize] - } - - return certName -} diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index b1248a9c..de624cf1 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -83,7 +83,7 @@ func TestNginxSecretProviderClassReconcilerIntegration(t *testing.T) { // Prove it exists spc := &secv1.SecretProviderClass{} spc.Name = DefaultNginxCertName(nic) - spc.Namespace = "app-routing-system" + spc.Namespace = config.DefaultNs require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc)) expected := &secv1.SecretProviderClass{ @@ -193,7 +193,7 @@ func TestNginxSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testin }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: "app-routing-system", + Namespace: config.DefaultNs, Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, @@ -305,8 +305,6 @@ func TestNginxSecretProviderClassReconcilerInvalidURL(t *testing.T) { } func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { - n := &NginxSecretProviderClassReconciler{} - invalidURLIng := &v1alpha1.NginxIngressController{ Spec: v1alpha1.NginxIngressControllerSpec{ IngressClassName: spcTestNginxIngressClassName, @@ -318,7 +316,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { nic := invalidURLIng.DeepCopy() nic.Spec.IngressClassName = "" - ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -326,7 +324,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { t.Run("empty key vault uri", func(t *testing.T) { nic := invalidURLIng.DeepCopy() - ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -336,7 +334,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { cc := string([]byte{0x7f}) nic.Spec.DefaultSSLCertificate.KeyVaultURI = &cc - ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) _, expectedErr := url.Parse(cc) // the exact error depends on operating system require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) @@ -347,7 +345,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { fooTestUri := "http://test.com/foo" nic.Spec.DefaultSSLCertificate.KeyVaultURI = &fooTestUri - ok, err := n.buildSPC(nic, &secv1.SecretProviderClass{}) + ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.EqualError(t, err, "invalid secret uri: http://test.com/foo") }) @@ -379,18 +377,12 @@ func TestNginxSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - n := &NginxSecretProviderClassReconciler{ - config: &config.Config{ - Cloud: c.configCloud, - }, - } - nic := spcTestNginxIngress.DeepCopy() testSecretUri := "https://test.vault.azure.net/secrets/test-secret" nic.Spec.DefaultSSLCertificate.KeyVaultURI = &testSecretUri spc := &secv1.SecretProviderClass{} - ok, err := n.buildSPC(nic, spc) + ok, err := BuildSPC(nic, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) require.NoError(t, err, "building SPC should not error") require.True(t, ok, "SPC should be built") diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 20910d4a..0b1e1294 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -9,7 +9,6 @@ import ( "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" - "sigs.k8s.io/controller-runtime/pkg/log/zap" "testing" "k8s.io/apimachinery/pkg/api/errors" @@ -232,7 +231,7 @@ func TestPlaceholderPodControllerIntegrationWithIng(t *testing.T) { func TestPlaceholderPodControllerIntegrationWithNic(t *testing.T) { ctx := context.Background() - ctx = logr.NewContext(ctx, zap.New()) + ctx = logr.NewContext(ctx, logr.Discard()) nic := placeholderTestNginxIngress.DeepCopy() spc := getDefaultNginxSpc(nic) spc.SetOwnerReferences(manifests.GetOwnerRefs(nic, true)) @@ -560,7 +559,7 @@ func getDefaultNginxSpc(nic *v1alpha1.NginxIngressController) *secv1.SecretProvi }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: "app-routing-system", + Namespace: config.DefaultNs, Labels: manifests.GetTopLevelLabels(), Generation: 123, OwnerReferences: []metav1.OwnerReference{{ diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 08853bc0..2516bbdb 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -533,7 +533,7 @@ func ToNginxIngressConfig(nic *approutingv1alpha1.NginxIngressController, defaul nginxIng.DefaultSSLCertificate = nic.Spec.DefaultSSLCertificate.Secret.Namespace + "/" + nic.Spec.DefaultSSLCertificate.Secret.Name } if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != nil { - nginxIng.DefaultSSLCertificate = "app-routing-system/" + keyvault.DefaultNginxCertName(nic) + nginxIng.DefaultSSLCertificate = config.DefaultNs + "/" + keyvault.DefaultNginxCertName(nic) } } From 0882918e4e28b472328b682e01743f790c4c9ddf Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 21 Feb 2024 15:22:36 -0500 Subject: [PATCH 06/36] placeholder_pod refactor and additional nic kv uri e2e test --- api/v1alpha1/zz_generated.deepcopy.go | 5 + pkg/controller/keyvault/placeholder_pod.go | 156 ++++++++++--------- testing/e2e/suites/nginxIngressController.go | 12 +- 3 files changed, 97 insertions(+), 76 deletions(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 0fc273d2..2f064c3b 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -17,6 +17,11 @@ func (in *DefaultSSLCertificate) DeepCopyInto(out *DefaultSSLCertificate) { *out = new(Secret) **out = **in } + if in.KeyVaultURI != nil { + in, out := &in.KeyVaultURI, &out.KeyVaultURI + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DefaultSSLCertificate. diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index c9fa5ea9..9ddfa713 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -110,94 +110,94 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } logger = logger.WithValues("deployment", dep.Name) - if strings.HasPrefix(spc.Name, NginxNamePrefix) { - nic := &v1alpha1.NginxIngressController{} - nic.Name = util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController") + return p.upsertObjectDeployment(dep, spc, req, ctx, logger) +} - if nic.Name != "" { - logger.Info("getting owner nginx ingress controller") - if err = p.client.Get(ctx, client.ObjectKeyFromObject(nic), nic); err != nil { - return result, client.IgnoreNotFound(err) - } +func (p *PlaceholderPodController) placeholderPodCleanCheck(obj client.Object) (bool, error) { + nic, ok := obj.(*v1alpha1.NginxIngressController) + if ok { + if nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { + return true, nil } - - if nic.Name == "" || nic.Spec.IngressClassName == "" || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { - logger.Info("cleaning unused placeholder pod deployment") - logger.Info("getting placeholder deployment") - 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) - } + return false, nil + } + ing, ok := obj.(*netv1.Ingress) + if ok { + managed, err := p.ingressManager.IsManaging(ing) + if err != nil { + return false, fmt.Errorf("determining if ingress is managed: %w", err) } - - // Manage a deployment resource - logger.Info("reconciling placeholder deployment for secret provider class") - if err = p.buildDeployment(ctx, dep, spc, nic.Name); err != nil { - err = fmt.Errorf("building deployment: %w", err) - p.events.Eventf(nic, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while building placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) - logger.Error(err, "failed to build placeholder deployment") - return result, err + if ing.Name == "" || ing.Spec.IngressClassName == nil || !managed { + return true, nil } + return false, nil + } - if err = util.Upsert(ctx, p.client, dep); err != nil { - p.events.Eventf(nic, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while creating or updating placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) - return result, err - } - } else { - if p.ingressManager == nil { - return result, fmt.Errorf("checking if ingressManager was not nil on non-nginx ingress: %w", err) - } - ing := &netv1.Ingress{} - ing.Name = util.FindOwnerKind(spc.OwnerReferences, "Ingress") - ing.Namespace = req.Namespace - logger = logger.WithValues("ingress", ing.Name) - if ing.Name != "" { - logger.Info("getting owner ingress") - if err = p.client.Get(ctx, client.ObjectKeyFromObject(ing), ing); err != nil { - return result, client.IgnoreNotFound(err) - } - } + return false, nil +} - managed, err := p.ingressManager.IsManaging(ing) - if err != nil { - return result, fmt.Errorf("determining if ingress is managed: %w", err) - } +func (p *PlaceholderPodController) upsertObjectDeployment(dep *appsv1.Deployment, spc *secv1.SecretProviderClass, req ctrl.Request, ctx context.Context, logger logr.Logger) (ctrl.Result, error) { + var ( + err error + obj client.Object + ) - if ing.Name == "" || ing.Spec.IngressClassName == nil || !managed { - logger.Info("cleaning unused placeholder pod deployment") + result := ctrl.Result{} - logger.Info("getting placeholder deployment") + objName := util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController") + if objName != "" { + obj := &v1alpha1.NginxIngressController{} + obj.Name = objName + } - 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) - } - } - // Manage a deployment resource - logger.Info("reconciling placeholder deployment for secret provider class") - if err = p.buildDeployment(ctx, dep, spc, ing.Name); err != nil { - err = fmt.Errorf("building deployment: %w", err) - p.events.Eventf(ing, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while building placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) - logger.Error(err, "failed to build placeholder deployment") - return result, err + objName = util.FindOwnerKind(spc.OwnerReferences, "Ingress") + if objName != "" { + obj := &netv1.Ingress{} + obj.Name = objName + obj.Namespace = req.Namespace + } + + if obj.GetName() != "" { + logger.Info(fmt.Sprintf("getting owner %s", obj.GetObjectKind().GroupVersionKind().Kind)) + if err = p.client.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { + return result, client.IgnoreNotFound(err) } + } - if err = util.Upsert(ctx, p.client, dep); err != nil { - p.events.Eventf(ing, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while creating or updating placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) - return result, err + cleanPod, err := p.placeholderPodCleanCheck(obj) + if err != nil { + return result, err + } + + if cleanPod { + logger.Info("cleaning unused placeholder pod deployment") + logger.Info("getting placeholder deployment") + 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) } } + // Manage a deployment resource + logger.Info("reconciling placeholder deployment for secret provider class") + if err = p.buildDeployment(ctx, dep, spc, obj.GetName()); err != nil { + err = fmt.Errorf("building deployment: %w", err) + p.events.Eventf(obj, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while building placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) + logger.Error(err, "failed to build placeholder deployment") + return result, err + } + + if err = util.Upsert(ctx, p.client, dep); err != nil { + p.events.Eventf(obj, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while creating or updating placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) + logger.Error(err, "failed to upsert placeholder deployment") + return result, err + } + return result, nil } @@ -223,6 +223,12 @@ func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *app if old != nil { // we need to ensure that immutable fields are not changed labels = old.Spec.Selector.MatchLabels } + var ingAnnotation string + if strings.HasPrefix(spc.Name, NginxNamePrefix) { + ingAnnotation = "kubernetes.azure.com/ingress-owner" + } else { + ingAnnotation = "kubernetes.azure.com/nginx-ingress-controller-owner" + } dep.Spec = appsv1.DeploymentSpec{ Replicas: util.Int32Ptr(1), @@ -234,7 +240,7 @@ func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *app Annotations: map[string]string{ "kubernetes.azure.com/observed-generation": strconv.FormatInt(spc.Generation, 10), "kubernetes.azure.com/purpose": "hold CSI mount to enable keyvault-to-k8s secret mirroring", - "kubernetes.azure.com/ingress-owner": ingName, + ingAnnotation: ingName, "openservicemesh.io/sidecar-injection": "disabled", }, }, diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 42ae1d1f..1985f27d 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -112,12 +112,22 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("able to create NginxIngressController despite missing Secret field'%s'", testNIC.Spec.ControllerNamePrefix) } + validUri := "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34" + testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") + testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ + KeyVaultURI: &validUri, + } + lgr.Info("creating NginxIngressController with valid keyvault uri field") + if err := c.Create(ctx, testNIC); err != nil { + return fmt.Errorf("unable to create NginxIngressController despite valid key vault uri'%s'", testNIC.Spec.ControllerNamePrefix) + } + invalidUri := "Invalid.URI" testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ KeyVaultURI: &invalidUri, } - lgr.Info("creating NginxIngressController with invalid Secret field") + lgr.Info("creating NginxIngressController with invalid keyvault uri field") if err := c.Create(ctx, testNIC); err == nil { return fmt.Errorf("able to create NginxIngressController despite invalid key vault uri'%s'", testNIC.Spec.ControllerNamePrefix) } From 78889dc8c72d315e97711a1aa022e0aca87405ea Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Thu, 22 Feb 2024 14:24:34 -0500 Subject: [PATCH 07/36] obj to nic bug fix, ing annotation change and unit test adjustment --- pkg/controller/keyvault/placeholder_pod.go | 38 +++++++++---------- .../keyvault/placeholder_pod_test.go | 16 ++++---- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 9ddfa713..816669c3 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -7,10 +7,6 @@ import ( "context" "fmt" "github.com/Azure/aks-app-routing-operator/api/v1alpha1" - "path" - "strconv" - "strings" - "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -19,9 +15,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + "path" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" + "strconv" "github.com/Azure/aks-app-routing-operator/pkg/config" "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" @@ -144,17 +142,19 @@ func (p *PlaceholderPodController) upsertObjectDeployment(dep *appsv1.Deployment result := ctrl.Result{} - objName := util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController") - if objName != "" { - obj := &v1alpha1.NginxIngressController{} - obj.Name = objName + if objName := util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController"); objName != "" { + obj = &v1alpha1.NginxIngressController{} + obj.SetName(objName) } - objName = util.FindOwnerKind(spc.OwnerReferences, "Ingress") - if objName != "" { - obj := &netv1.Ingress{} - obj.Name = objName - obj.Namespace = req.Namespace + if objName := util.FindOwnerKind(spc.OwnerReferences, "Ingress"); objName != "" { + obj = &netv1.Ingress{} + obj.SetName(objName) + obj.SetNamespace(req.Namespace) + } + + if obj.GetName() == "" { + return result, nil } if obj.GetName() != "" { @@ -185,7 +185,7 @@ func (p *PlaceholderPodController) upsertObjectDeployment(dep *appsv1.Deployment // Manage a deployment resource logger.Info("reconciling placeholder deployment for secret provider class") - if err = p.buildDeployment(ctx, dep, spc, obj.GetName()); err != nil { + if err = p.buildDeployment(ctx, dep, spc, obj); err != nil { err = fmt.Errorf("building deployment: %w", err) p.events.Eventf(obj, "Warning", "FailedUpdateOrCreatePlaceholderPodDeployment", "error while building placeholder pod Deployment needed to pull Keyvault reference: %s", err.Error()) logger.Error(err, "failed to build placeholder deployment") @@ -212,7 +212,7 @@ func (p *PlaceholderPodController) getCurrentDeployment(ctx context.Context, nam return dep, nil } -func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *appsv1.Deployment, spc *secv1.SecretProviderClass, ingName string) error { +func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *appsv1.Deployment, spc *secv1.SecretProviderClass, obj client.Object) error { old, err := p.getCurrentDeployment(ctx, client.ObjectKeyFromObject(dep)) if err != nil { return fmt.Errorf("getting current deployment: %w", err) @@ -224,10 +224,10 @@ func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *app labels = old.Spec.Selector.MatchLabels } var ingAnnotation string - if strings.HasPrefix(spc.Name, NginxNamePrefix) { - ingAnnotation = "kubernetes.azure.com/ingress-owner" - } else { + if _, ok := obj.(*v1alpha1.NginxIngressController); ok { ingAnnotation = "kubernetes.azure.com/nginx-ingress-controller-owner" + } else { + ingAnnotation = "kubernetes.azure.com/ingress-owner" } dep.Spec = appsv1.DeploymentSpec{ @@ -240,7 +240,7 @@ func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *app Annotations: map[string]string{ "kubernetes.azure.com/observed-generation": strconv.FormatInt(spc.Generation, 10), "kubernetes.azure.com/purpose": "hold CSI mount to enable keyvault-to-k8s secret mirroring", - ingAnnotation: ingName, + ingAnnotation: obj.GetName(), "openservicemesh.io/sidecar-injection": "disabled", }, }, diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 0b1e1294..b3e60287 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -56,8 +56,8 @@ var ( Spec: v1alpha1.NginxIngressControllerSpec{ IngressClassName: spcTestNginxIngressClassName, DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{ - Secret: nil, // nil Secret since SPC method for DefaultSSLCertificate using placeholder pods only uses the KeyVaultURI - KeyVaultURI: &placeholderTestUri}, + KeyVaultURI: &placeholderTestUri, + }, }, } placeholderSpc = &secv1.SecretProviderClass{ @@ -282,10 +282,10 @@ func TestPlaceholderPodControllerIntegrationWithNic(t *testing.T) { 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": nic.Name, - "openservicemesh.io/sidecar-injection": "disabled", + "kubernetes.azure.com/observed-generation": "123", + "kubernetes.azure.com/purpose": "hold CSI mount to enable keyvault-to-k8s secret mirroring", + "kubernetes.azure.com/nginx-ingress-controller-owner": nic.Name, + "openservicemesh.io/sidecar-injection": "disabled", }, }, Spec: *manifests.WithPreferSystemNodes(&corev1.PodSpec{ @@ -345,8 +345,8 @@ func TestPlaceholderPodControllerIntegrationWithNic(t *testing.T) { require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(dep), dep)) assert.Equal(t, expected, dep.Spec) - // Change the ingress resource's class - nic.Spec.IngressClassName = "" + // Remove Key Vault URI + nic.Spec.DefaultSSLCertificate.KeyVaultURI = nil require.NoError(t, c.Update(ctx, nic)) beforeErrCount = testutils.GetErrMetricCount(t, placeholderPodControllerName) From 5c7fc53c008c8a3dd0df70c92e053864f210eca8 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Thu, 22 Feb 2024 15:42:02 -0500 Subject: [PATCH 08/36] changed nginx_secret_provider_class namespace to come from config --- pkg/controller/keyvault/nginx_secret_provider_class.go | 2 +- pkg/controller/keyvault/nginx_secret_provider_class_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index ee1a1f01..0ef939a4 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -83,7 +83,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: config.DefaultNs, + Namespace: i.config.NS, Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index de624cf1..66430f49 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -83,7 +83,7 @@ func TestNginxSecretProviderClassReconcilerIntegration(t *testing.T) { // Prove it exists spc := &secv1.SecretProviderClass{} spc.Name = DefaultNginxCertName(nic) - spc.Namespace = config.DefaultNs + spc.Namespace = n.config.NS require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc)) expected := &secv1.SecretProviderClass{ @@ -193,7 +193,7 @@ func TestNginxSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testin }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultNginxCertName(nic), - Namespace: config.DefaultNs, + Namespace: n.config.NS, Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, From fc0513629f18ce227ea5b08fae5156dc8a54f404 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 26 Feb 2024 12:07:49 -0500 Subject: [PATCH 09/36] Added CEL validation for keyvault fields and properties size on DefaultSSLCertificate --- api/v1alpha1/nginxingresscontroller_types.go | 3 +++ ...pprouting.kubernetes.azure.com_nginxingresscontrollers.yaml | 3 +++ 2 files changed, 6 insertions(+) diff --git a/api/v1alpha1/nginxingresscontroller_types.go b/api/v1alpha1/nginxingresscontroller_types.go index 3ed8cd7b..238f4384 100644 --- a/api/v1alpha1/nginxingresscontroller_types.go +++ b/api/v1alpha1/nginxingresscontroller_types.go @@ -57,6 +57,9 @@ type NginxIngressControllerSpec struct { DefaultSSLCertificate *DefaultSSLCertificate `json:"defaultSSLCertificate,omitempty"` } +// DefaultSSLCertificate holds a secret in the form of a secret struct ith name and namespace properties or a key vault uri +// +kubebuilder:validation:MaxProperties=1 +// +kubebuilder:validation:XValidation:rule="(isURL(self.keyVaultURI) || !has(self.keyVaultURI))" type DefaultSSLCertificate struct { // Secret is a struct that holds the name and namespace fields used for the default ssl secret // +optional diff --git a/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml b/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml index 8f824b0c..35280b8c 100644 --- a/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml +++ b/config/crd/bases/approuting.kubernetes.azure.com_nginxingresscontrollers.yaml @@ -65,6 +65,7 @@ spec: description: DefaultSSLCertificate defines whether the NginxIngressController should use a certain SSL certificate by default. If this field is omitted, no default certificate will be used. + maxProperties: 1 properties: keyVaultURI: description: Secret in the form of a Key Vault URI @@ -88,6 +89,8 @@ spec: - namespace type: object type: object + x-kubernetes-validations: + - rule: (isURL(self.keyVaultURI) || !has(self.keyVaultURI)) ingressClassName: default: nginx.approuting.kubernetes.azure.com description: IngressClassName is the name of the IngressClass that From 5d1a82d5f4bfbf931de1c89b5d40dc41dd31189e Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 28 Feb 2024 10:25:34 -0500 Subject: [PATCH 10/36] addressing comments and added placeholder pod check test --- .../keyvault/ingress_secret_provider_class.go | 5 +- .../ingress_secret_provider_class_test.go | 33 ++------ pkg/controller/keyvault/kv_util.go | 24 +++--- pkg/controller/keyvault/kv_util_test.go | 23 +++++ .../keyvault/nginx_secret_provider_class.go | 2 +- .../nginx_secret_provider_class_test.go | 14 ++-- pkg/controller/keyvault/placeholder_pod.go | 45 +++++----- .../keyvault/placeholder_pod_test.go | 83 +++++++++++++++++++ testing/e2e/suites/nginxIngressController.go | 25 +++--- testing/e2e/suites/utils.go | 9 ++ 10 files changed, 180 insertions(+), 83 deletions(-) create mode 100644 pkg/controller/keyvault/kv_util_test.go diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index 4856394c..84391bc0 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -102,12 +102,11 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re ok, err := i.ingressManager.IsManaging(ing) if err != nil { - ok = false - err = fmt.Errorf("determining if ingress is managed: %w", err) + return result, fmt.Errorf("determining if ingress is managed: %w", err) } if ok { - ok, err = BuildSPC(ing, spc, i.config) + ok, err = buildSPC(ing, spc, i.config) } if err != nil { diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index 568824cd..fd22957c 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -323,31 +323,10 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { }, } - t.Run("missing ingress class", func(t *testing.T) { - ing := invalidURLIng.DeepCopy() - ing.Spec.IngressClassName = nil - ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL"} - - ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - require.NoError(t, err) - }) - - //t.Run("incorrect ingress class", func(t *testing.T) { - // 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"} - // - // ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) - // assert.False(t, ok) - // require.NoError(t, err) - //}) - t.Run("nil annotations", func(t *testing.T) { ing := invalidURLIng.DeepCopy() - ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -356,7 +335,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { ing := invalidURLIng.DeepCopy() ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": ""} - ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -366,7 +345,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { cc := string([]byte{0x7f}) ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": cc} - ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) _, expectedErr := url.Parse(cc) // the exact error depends on operating system require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) @@ -376,13 +355,13 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { ing := invalidURLIng.DeepCopy() ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "http://test.com/foo"} - ok, err := BuildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.EqualError(t, err, "invalid secret uri: http://test.com/foo") }) } -func TestIngressSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { +func TestIngressSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { cases := []struct { name, configCloud, spcCloud string expected bool @@ -414,7 +393,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { } spc := &secv1.SecretProviderClass{} - ok, err := BuildSPC(ing, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) + ok, err := buildSPC(ing, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) require.NoError(t, err, "building SPC should not error") require.True(t, ok, "SPC should be built") diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go index f7439df3..51554843 100644 --- a/pkg/controller/keyvault/kv_util.go +++ b/pkg/controller/keyvault/kv_util.go @@ -8,33 +8,35 @@ import ( kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" v1 "k8s.io/api/networking/v1" "net/url" + "reflect" + "sigs.k8s.io/controller-runtime/pkg/client" secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" "strings" ) -var NginxNamePrefix = "keyvault-nginx-" +var nginxNamePrefix = "keyvault-nginx-" -func BuildSPC(ic interface{}, spc *secv1.SecretProviderClass, config *config.Config) (bool, error) { +func buildSPC(obj client.Object, spc *secv1.SecretProviderClass, config *config.Config) (bool, error) { var certURI, specSecretName string - if nic, ok := ic.(*v1alpha1.NginxIngressController); ok { - if nic.Spec.IngressClassName == "" { - return false, nil - } + switch objType := reflect.TypeOf(obj).String(); objType { + case "*v1alpha1.NginxIngressController": + nic, _ := obj.(*v1alpha1.NginxIngressController) if nic.Spec.DefaultSSLCertificate == nil || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { return false, nil } certURI = *nic.Spec.DefaultSSLCertificate.KeyVaultURI specSecretName = DefaultNginxCertName(nic) - } - - if ing, ok := ic.(*v1.Ingress); ok { - if ing.Spec.IngressClassName == nil || ing.Annotations == nil { + case "*v1.Ingress": + ing, _ := obj.(*v1.Ingress) + if ing.Annotations == nil { return false, nil } certURI = ing.Annotations["kubernetes.azure.com/tls-cert-keyvault-uri"] specSecretName = fmt.Sprintf("keyvault-%s", ing.Name) + default: + return false, fmt.Errorf("incorrect object type: %s", objType) } if certURI == "" { @@ -105,7 +107,7 @@ func BuildSPC(ic interface{}, spc *secv1.SecretProviderClass, config *config.Con // Truncates characters in the IngressClassName passed the max secret length (255) if the IngressClassName and the default namespace are over the limit func DefaultNginxCertName(nic *v1alpha1.NginxIngressController) string { secretMaxSize := 255 - certName := NginxNamePrefix + nic.Name + certName := nginxNamePrefix + nic.Name if len(certName) > secretMaxSize { return certName[0:secretMaxSize] diff --git a/pkg/controller/keyvault/kv_util_test.go b/pkg/controller/keyvault/kv_util_test.go new file mode 100644 index 00000000..1ca1f167 --- /dev/null +++ b/pkg/controller/keyvault/kv_util_test.go @@ -0,0 +1,23 @@ +package keyvault + +import ( + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "testing" +) + +var ( + maxString = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbb" +) + +func TestDefaultNginxCertName(t *testing.T) { + testStr := DefaultNginxCertName(&v1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: maxString, + }, + }) + + require.NotContains(t, testStr, "b") + require.Contains(t, testStr, nginxNamePrefix) +} diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 0ef939a4..a10cdb2b 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -95,7 +95,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req }, } logger = logger.WithValues("spc", spc.Name) - ok, err := BuildSPC(nic, spc, i.config) + ok, err := buildSPC(nic, spc, i.config) if err != nil { logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index 66430f49..ea91d613 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -304,7 +304,7 @@ func TestNginxSecretProviderClassReconcilerInvalidURL(t *testing.T) { assert.Greater(t, afterRequestCount, beforeRequestCount) } -func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { +func TestNginxSecretProviderClassReconcilerbuildSPCInvalidURLs(t *testing.T) { invalidURLIng := &v1alpha1.NginxIngressController{ Spec: v1alpha1.NginxIngressControllerSpec{ IngressClassName: spcTestNginxIngressClassName, @@ -316,7 +316,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { nic := invalidURLIng.DeepCopy() nic.Spec.IngressClassName = "" - ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -324,7 +324,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { t.Run("empty key vault uri", func(t *testing.T) { nic := invalidURLIng.DeepCopy() - ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.NoError(t, err) }) @@ -334,7 +334,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { cc := string([]byte{0x7f}) nic.Spec.DefaultSSLCertificate.KeyVaultURI = &cc - ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) _, expectedErr := url.Parse(cc) // the exact error depends on operating system require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) @@ -345,13 +345,13 @@ func TestNginxSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { fooTestUri := "http://test.com/foo" nic.Spec.DefaultSSLCertificate.KeyVaultURI = &fooTestUri - ok, err := BuildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) + ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) require.EqualError(t, err, "invalid secret uri: http://test.com/foo") }) } -func TestNginxSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { +func TestNginxSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { cases := []struct { name, configCloud, spcCloud string expected bool @@ -382,7 +382,7 @@ func TestNginxSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) { nic.Spec.DefaultSSLCertificate.KeyVaultURI = &testSecretUri spc := &secv1.SecretProviderClass{} - ok, err := BuildSPC(nic, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) + ok, err := buildSPC(nic, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) require.NoError(t, err, "building SPC should not error") require.True(t, ok, "SPC should be built") diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 816669c3..cd0ee0c2 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -108,19 +108,18 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } logger = logger.WithValues("deployment", dep.Name) - return p.upsertObjectDeployment(dep, spc, req, ctx, logger) + return p.reconcileObjectDeployment(dep, spc, req, ctx, logger) } func (p *PlaceholderPodController) placeholderPodCleanCheck(obj client.Object) (bool, error) { - nic, ok := obj.(*v1alpha1.NginxIngressController) - if ok { - if nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { + switch obj.GetObjectKind().GroupVersionKind().Kind { + case "NginxIngressController": + nic, _ := obj.(*v1alpha1.NginxIngressController) + if nic.Spec.DefaultSSLCertificate == nil || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { return true, nil } - return false, nil - } - ing, ok := obj.(*netv1.Ingress) - if ok { + case "Ingress": + ing, _ := obj.(*netv1.Ingress) managed, err := p.ingressManager.IsManaging(ing) if err != nil { return false, fmt.Errorf("determining if ingress is managed: %w", err) @@ -128,13 +127,12 @@ func (p *PlaceholderPodController) placeholderPodCleanCheck(obj client.Object) ( if ing.Name == "" || ing.Spec.IngressClassName == nil || !managed { return true, nil } - return false, nil } return false, nil } -func (p *PlaceholderPodController) upsertObjectDeployment(dep *appsv1.Deployment, spc *secv1.SecretProviderClass, req ctrl.Request, ctx context.Context, logger logr.Logger) (ctrl.Result, error) { +func (p *PlaceholderPodController) reconcileObjectDeployment(dep *appsv1.Deployment, spc *secv1.SecretProviderClass, req ctrl.Request, ctx context.Context, logger logr.Logger) (ctrl.Result, error) { var ( err error obj client.Object @@ -142,19 +140,18 @@ func (p *PlaceholderPodController) upsertObjectDeployment(dep *appsv1.Deployment result := ctrl.Result{} - if objName := util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController"); objName != "" { + switch { + case util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController") != "": obj = &v1alpha1.NginxIngressController{} - obj.SetName(objName) - } - - if objName := util.FindOwnerKind(spc.OwnerReferences, "Ingress"); objName != "" { + obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController")) + break + case util.FindOwnerKind(spc.OwnerReferences, "Ingress") != "": obj = &netv1.Ingress{} - obj.SetName(objName) + obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "Ingress")) obj.SetNamespace(req.Namespace) - } - - if obj.GetName() == "" { - return result, nil + break + default: + return result, fmt.Errorf("failed to reconcile object deployment: object type not nginxIngressController or ingress") } if obj.GetName() != "" { @@ -223,11 +220,15 @@ func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *app if old != nil { // we need to ensure that immutable fields are not changed labels = old.Spec.Selector.MatchLabels } + var ingAnnotation string - if _, ok := obj.(*v1alpha1.NginxIngressController); ok { + switch obj.GetObjectKind().GroupVersionKind().Kind { + case "NginxIngressController": ingAnnotation = "kubernetes.azure.com/nginx-ingress-controller-owner" - } else { + case "Ingress": ingAnnotation = "kubernetes.azure.com/ingress-owner" + default: + return fmt.Errorf("failed to build deployment: object type not ingress or nginxingresscontroller") } dep.Spec = appsv1.DeploymentSpec{ diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index b3e60287..1ebd1343 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -551,6 +551,89 @@ func TestGetCurrentDeploymentWithIng(t *testing.T) { require.Nil(t, dep) } +func TestPlaceholderPodCleanCheck(t *testing.T) { + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + require.NoError(t, netv1.AddToScheme(scheme)) + require.NoError(t, appsv1.AddToScheme(scheme)) + + nic := placeholderTestNginxIngress.DeepCopy() + ing := placeholderTestIng.DeepCopy() + ing.TypeMeta.Kind = "Ingress" + unmanagedIngClassName := "unmanagedClassName" + errorIngClassName := "errorClassName" + + c := fake.NewClientBuilder().WithScheme(scheme).Build() + require.NoError(t, c.Create(ctx, nic)) + require.NoError(t, c.Create(ctx, ing)) + p := &PlaceholderPodController{ + client: c, + config: &config.Config{Registry: "test-registry"}, + ingressManager: NewIngressManagerFromFn(func(ing *netv1.Ingress) (bool, error) { + if ing.Spec.IngressClassName != nil { + if *ing.Spec.IngressClassName == unmanagedIngClassName { + return false, nil + } + + if *ing.Spec.IngressClassName == errorIngClassName { + return false, fmt.Errorf("an error has occured checking if ingress is managed") + } + } + return true, nil + }), + } + + // Default scenarios for ingress and nginxingresscontroller. Does not clean when spc required fields are there + cleanPod, err := p.placeholderPodCleanCheck(nic) + require.NoError(t, err) + require.Equal(t, false, cleanPod) + + cleanPod, err = p.placeholderPodCleanCheck(ing) + require.NoError(t, err) + require.Equal(t, false, cleanPod) + + // Clean Placeholder Pod scenarios + + // nic without key vault uri + nic.Spec.DefaultSSLCertificate.KeyVaultURI = nil + cleanPod, err = p.placeholderPodCleanCheck(nic) + require.NoError(t, err) + require.Equal(t, true, cleanPod) + + // nic without DefaultSSLCertificate + nic.Spec.DefaultSSLCertificate = nil + cleanPod, err = p.placeholderPodCleanCheck(nic) + require.NoError(t, err) + require.Equal(t, true, cleanPod) + + // ingress without IngressClassName + ing.Spec.IngressClassName = nil + cleanPod, err = p.placeholderPodCleanCheck(ing) + require.NoError(t, err) + require.Equal(t, true, cleanPod) + + ing.Spec.IngressClassName = placeholderTestIng.Spec.IngressClassName //returning value to IngressClassName to test individual fields triggering clean + + // ingress with empty Name field + ing.Name = "" + cleanPod, err = p.placeholderPodCleanCheck(ing) + require.NoError(t, err) + require.Equal(t, true, cleanPod) + + // unmanaged ingress + ing.Spec.IngressClassName = &unmanagedIngClassName + cleanPod, err = p.placeholderPodCleanCheck(ing) + require.NoError(t, err) + require.Equal(t, true, cleanPod) + + // ingress that hits an error while checking if it's managed + ing.Spec.IngressClassName = &errorIngClassName + cleanPod, err = p.placeholderPodCleanCheck(ing) + require.Error(t, err, "determining if ingress is managed: an error has occured checking if ingress is managed") + require.Equal(t, false, cleanPod) +} func getDefaultNginxSpc(nic *v1alpha1.NginxIngressController) *secv1.SecretProviderClass { spc := &secv1.SecretProviderClass{ TypeMeta: metav1.TypeMeta{ diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 1985f27d..b84bab23 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -78,10 +78,7 @@ func nicTests(in infra.Provisioned) []test { testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - Secret: &v1alpha1.Secret{ - Name: "Invalid+@Name", - Namespace: "validnamespace", - }, + Secret: getTestSecret("Invalid+@Name", "validnamespace"), } lgr.Info("creating NginxIngressController with invalid Secret field") if err := c.Create(ctx, testNIC); err == nil { @@ -90,10 +87,7 @@ func nicTests(in infra.Provisioned) []test { testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - Secret: &v1alpha1.Secret{ - Name: "validname", - Namespace: "Invalid+@Namespace", - }, + Secret: getTestSecret("validname", "Invalid+@Namespace"), } lgr.Info("creating NginxIngressController with invalid Secret field") if err := c.Create(ctx, testNIC); err == nil { @@ -102,10 +96,7 @@ func nicTests(in infra.Provisioned) []test { testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - Secret: &v1alpha1.Secret{ - Name: "validname", - Namespace: "", - }, + Secret: getTestSecret("validname", ""), } lgr.Info("creating NginxIngressController with empty Secret field") if err := c.Create(ctx, testNIC); err == nil { @@ -132,6 +123,16 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("able to create NginxIngressController despite invalid key vault uri'%s'", testNIC.Spec.ControllerNamePrefix) } + testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") + testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ + KeyVaultURI: &validUri, + Secret: getTestSecret("validname", "validnamespace"), + } + lgr.Info("creating NginxIngressController with both keyvault uri field and secret field") + if err := c.Create(ctx, testNIC); err == nil { + return fmt.Errorf("able to create NginxIngressController despite having both keyvaulturi and secret fields'%s'", testNIC.Spec.ControllerNamePrefix) + } + lgr.Info("finished testing") return nil }, diff --git a/testing/e2e/suites/utils.go b/testing/e2e/suites/utils.go index 50481c0f..d7805b40 100644 --- a/testing/e2e/suites/utils.go +++ b/testing/e2e/suites/utils.go @@ -3,6 +3,7 @@ package suites import ( "context" "fmt" + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "time" "github.com/Azure/aks-app-routing-operator/testing/e2e/logger" @@ -62,3 +63,11 @@ func upsert(ctx context.Context, c client.Client, obj client.Object) error { lgr.Info("object updated") return nil } + +func getTestSecret(name, namespace string) *v1alpha1.Secret { + sec := &v1alpha1.Secret{ + Name: name, + Namespace: namespace, + } + return sec +} From 3d7564b4f1eb0b7d3d998431671a7c7ba63da518 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 4 Mar 2024 14:16:22 -0500 Subject: [PATCH 11/36] Added e2e tests to confirm pulling secret from keyvault --- testing/e2e/go.mod | 14 +++ testing/e2e/go.sum | 25 ++++++ testing/e2e/suites/nginxIngressController.go | 95 ++++++++++++++++++-- 3 files changed, 126 insertions(+), 8 deletions(-) diff --git a/testing/e2e/go.mod b/testing/e2e/go.mod index 9e722597..c4950f64 100644 --- a/testing/e2e/go.mod +++ b/testing/e2e/go.mod @@ -29,6 +29,7 @@ require ( k8s.io/apimachinery v0.28.1 k8s.io/client-go v0.28.1 sigs.k8s.io/controller-runtime v0.16.1 + sigs.k8s.io/secrets-store-csi-driver v1.3.4 ) require ( @@ -37,27 +38,35 @@ require ( github.com/Azure/go-autorest v14.2.0+incompatible // indirect github.com/Azure/go-autorest/autorest/adal v0.9.23 // indirect github.com/Azure/go-autorest/autorest/date v0.3.0 // indirect + github.com/Azure/go-autorest/autorest/to v0.4.0 // indirect github.com/Azure/go-autorest/logger v0.2.1 // indirect github.com/Azure/go-autorest/tracing v0.6.0 // indirect + github.com/Azure/secrets-store-csi-driver-provider-azure v1.4.1 // indirect github.com/AzureAD/microsoft-authentication-library-for-go v1.0.0 // indirect + github.com/beorn7/perks v1.0.1 // indirect + github.com/cespare/xxhash/v2 v2.2.0 // indirect github.com/cjlapao/common-go v0.0.39 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/emicklei/go-restful/v3 v3.11.0 // indirect github.com/evanphx/json-patch/v5 v5.6.0 // indirect + github.com/fsnotify/fsnotify v1.6.0 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-openapi/jsonpointer v0.20.0 // indirect github.com/go-openapi/jsonreference v0.20.2 // indirect github.com/go-openapi/swag v0.22.4 // indirect github.com/gogo/protobuf v1.3.2 // indirect + github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/google/gnostic-models v0.6.8 // indirect github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.2.0 // indirect + github.com/imdario/mergo v0.3.16 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/kylelemons/godebug v1.1.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect + github.com/matttproud/golang_protobuf_extensions v1.0.4 // indirect github.com/microsoft/kiota-abstractions-go v1.2.2 // indirect github.com/microsoft/kiota-authentication-azure-go v1.0.0 // indirect github.com/microsoft/kiota-http-go v1.1.0 // indirect @@ -72,7 +81,9 @@ require ( github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect + github.com/prometheus/client_model v0.4.0 // indirect github.com/prometheus/common v0.44.0 // indirect + github.com/prometheus/procfs v0.11.1 // indirect github.com/sethvargo/go-envconfig v0.8.0 // indirect github.com/spf13/pflag v1.0.5 // indirect github.com/std-uritemplate/std-uritemplate/go v0.0.42 // indirect @@ -87,11 +98,14 @@ require ( golang.org/x/term v0.15.0 // indirect golang.org/x/text v0.14.0 // indirect golang.org/x/time v0.3.0 // indirect + gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect google.golang.org/appengine v1.6.7 // indirect google.golang.org/protobuf v1.31.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + k8s.io/apiextensions-apiserver v0.28.1 // indirect + k8s.io/component-base v0.28.1 // indirect k8s.io/klog/v2 v2.100.1 // indirect k8s.io/kube-openapi v0.0.0-20230816210353-14e408962443 // indirect k8s.io/utils v0.0.0-20230726121419-3b25d923346b // indirect diff --git a/testing/e2e/go.sum b/testing/e2e/go.sum index 12055a5c..fb4c5cd3 100644 --- a/testing/e2e/go.sum +++ b/testing/e2e/go.sum @@ -1,3 +1,4 @@ +github.com/Azure/azure-sdk-for-go v66.0.0+incompatible h1:bmmC38SlE8/E81nNADlgmVGurPWMHDX2YNXVQMrBpEE= github.com/Azure/azure-sdk-for-go/sdk/azcore v1.7.2 h1:t5+QXLCK9SVi0PPdaY0PrFvYUo24KwA0QwxnaHRSVd4= github.com/Azure/azure-sdk-for-go/sdk/azcore v1.7.2/go.mod h1:bjGvMhVMb+EEm3VRNQawDMUyMMjo+S5ewNjflkep/0Q= github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.3.0 h1:vcYCAze6p19qBW7MhZybIsqD8sMV8js0NyQM8JDnVtg= @@ -38,14 +39,20 @@ github.com/Azure/go-autorest/autorest/date v0.3.0/go.mod h1:BI0uouVdmngYNUzGWeSY github.com/Azure/go-autorest/autorest/mocks v0.4.1/go.mod h1:LTp+uSrOhSkaKrUy935gNZuuIPPVsHlr9DSOxSayd+k= github.com/Azure/go-autorest/autorest/mocks v0.4.2 h1:PGN4EDXnuQbojHbU0UWoNvmu9AGVwYHG9/fkDYhtAfw= github.com/Azure/go-autorest/autorest/mocks v0.4.2/go.mod h1:Vy7OitM9Kei0i1Oj+LvyAWMXJHeKH1MVlzFugfVrmyU= +github.com/Azure/go-autorest/autorest/to v0.4.0 h1:oXVqrxakqqV1UZdSazDOPOLvOIz+XA683u8EctwboHk= +github.com/Azure/go-autorest/autorest/to v0.4.0/go.mod h1:fE8iZBn7LQR7zH/9XU2NcPR4o9jEImooCeWJcYV/zLE= github.com/Azure/go-autorest/logger v0.2.1 h1:IG7i4p/mDa2Ce4TRyAO8IHnVhAVF3RFU+ZtXWSmf4Tg= github.com/Azure/go-autorest/logger v0.2.1/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZmbF5NWuPV8+WeEW8= github.com/Azure/go-autorest/tracing v0.6.0 h1:TYi4+3m5t6K48TGI9AUdb+IzbnSxvnvUMfuitfgcfuo= github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU= +github.com/Azure/secrets-store-csi-driver-provider-azure v1.4.1 h1:24/mZ06Uzu8EkdgJj8zcy5C3Ipsg1doM62hx0WPscNU= +github.com/Azure/secrets-store-csi-driver-provider-azure v1.4.1/go.mod h1:xUXKV8vOut59vIrFhyEY+4PgiK2LXkP10BtI+2y8VXM= github.com/AzureAD/microsoft-authentication-library-for-go v1.0.0 h1:OBhqkivkhkMqLPymWEppkm7vgPQY2XsHoEkaMQ0AdZY= github.com/AzureAD/microsoft-authentication-library-for-go v1.0.0/go.mod h1:kgDmCTgBzIEPFElEF+FK0SdjAor06dRq2Go927dnQ6o= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= +github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= +github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/cjlapao/common-go v0.0.39 h1:bAAUrj2B9v0kMzbAOhzjSmiyDy+rd56r2sy7oEiQLlA= github.com/cjlapao/common-go v0.0.39/go.mod h1:M3dzazLjTjEtZJbbxoA5ZDiGCiHmpwqW9l4UWaddwOA= github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= @@ -61,6 +68,7 @@ github.com/evanphx/json-patch v5.6.0+incompatible h1:jBYDEEiFBPxA0v50tFdvOzQQTCv github.com/evanphx/json-patch/v5 v5.6.0 h1:b91NhWfaz02IuVxO9faSllyAtNXHMPkC5J8sJCLunww= github.com/evanphx/json-patch/v5 v5.6.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY= +github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbSClcnxKAGw= github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.2.4 h1:g01GSCwiDw2xSZfjJ2/T9M+S6pFdcNtFYsp+Y43HYDQ= @@ -82,6 +90,9 @@ github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69 github.com/golang-jwt/jwt/v4 v4.0.0/go.mod h1:/xlHOz8bRuivTWchD4jCa+NbatV+wEUSzwAxVc6locg= github.com/golang-jwt/jwt/v4 v4.5.0 h1:7cYmW1XlMY7h7ii7UhUyChSgS5wUJEnm9uZVTGqOWzg= github.com/golang-jwt/jwt/v4 v4.5.0/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0= +github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE= +github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= +github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg= @@ -98,6 +109,7 @@ github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 h1:K6RDEckDVWvDI9JAJY github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4= github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/imdario/mergo v0.3.16 h1:wwQJbIsHYGMUyLSPrEq1CT16AhnhNJQ51+4fdHUnCl4= +github.com/imdario/mergo v0.3.16/go.mod h1:WBLT9ZmE3lPoWsEzCh9LPo3TiwVN+ZKEjmz+hD27ysY= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= @@ -119,6 +131,7 @@ github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+ github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0= github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= github.com/matttproud/golang_protobuf_extensions v1.0.4 h1:mmDVorXM7PCGKw94cs5zkfA9PSy5pEvNWRP0ET0TIVo= +github.com/matttproud/golang_protobuf_extensions v1.0.4/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4= github.com/microsoft/kiota-abstractions-go v1.2.2 h1:2i/zHAxjbONp7Q5+qN3NEF1zuf5YWMKCLxbkVqPm6uQ= github.com/microsoft/kiota-abstractions-go v1.2.2/go.mod h1:mWUfljJB88owshlQl6LhnqlEHmWyutTPK7A/L7AGnNw= github.com/microsoft/kiota-authentication-azure-go v1.0.0 h1:29FNZZ/4nnCOwFcGWlB/sxPvWz487HA2bXH8jR5k2Rk= @@ -145,6 +158,7 @@ github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjY github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f h1:KUppIJq7/+SVif2QVs3tOP0zanoHgBEVAwHxUSIzRqU= +github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE= github.com/onsi/ginkgo/v2 v2.11.0 h1:WgqUCUt/lT6yXoQ8Wef0fsNn5cAuMK7+KT9UFRz2tcU= github.com/onsi/gomega v1.27.10 h1:naR28SdDFlqrG6kScpT8VWpu1xWY5nJRCF3XaYyBjhI= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU= @@ -158,9 +172,11 @@ github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH github.com/prometheus/client_golang v1.16.0 h1:yk/hx9hDbrGHovbci4BY+pRMfSuuat626eFsHb7tmT8= github.com/prometheus/client_golang v1.16.0/go.mod h1:Zsulrv/L9oM40tJ7T815tM89lFEugiJ9HzIqaAx4LKc= github.com/prometheus/client_model v0.4.0 h1:5lQXD3cAg1OXBf4Wq03gTrXHeaV0TQvGfUooCfx1yqY= +github.com/prometheus/client_model v0.4.0/go.mod h1:oMQmHW1/JoDwqLtg57MGgP/Fb1CJEYF2imWWhWtMkYU= github.com/prometheus/common v0.44.0 h1:+5BrQJwiBB9xsMygAB3TNvpQKOwlkc25LbISbrdOOfY= github.com/prometheus/common v0.44.0/go.mod h1:ofAIvZbQ1e/nugmZGz4/qCb9Ap1VoSTIO7x0VV9VvuY= github.com/prometheus/procfs v0.11.1 h1:xRC8Iq1yyca5ypa9n1EZnWZkt7dwcoRPQwX/5gwaUuI= +github.com/prometheus/procfs v0.11.1/go.mod h1:eesXgaPo1q7lBpVMoMy0ZOFTth9hBn4W/y0/p/ScXhY= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/sethvargo/go-envconfig v0.8.0 h1:AcmdAewSFAc7pQ1Ghz+vhZkilUtxX559QlDuLLiSkdI= @@ -192,6 +208,7 @@ go.opentelemetry.io/otel/metric v1.18.0 h1:JwVzw94UYmbx3ej++CwLUQZxEODDj/pOuTCvz go.opentelemetry.io/otel/metric v1.18.0/go.mod h1:nNSpsVDjWGfb7chbRLUNW+PBNdcSTHD4Uu5pfFMOI0k= go.opentelemetry.io/otel/trace v1.18.0 h1:NY+czwbHbmndxojTEKiSMHkG2ClNH2PwmcHrdo0JY10= go.opentelemetry.io/otel/trace v1.18.0/go.mod h1:T2+SGJGuYZY3bjj5rgh/hN7KIrlpWC5nS8Mjvzckz+0= +go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= go.uber.org/zap v1.25.0 h1:4Hvk6GtkucQ790dqmj7l1eEnRdKm3k3ZUrUMS2d5+5c= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= @@ -220,6 +237,7 @@ golang.org/x/net v0.14.0 h1:BONx9s002vGdD9umnlX1Po8vOZmrgH34qlHcD1MfK14= golang.org/x/net v0.14.0/go.mod h1:PpSgVXXLK0OxS0F31C1/tv6XNguvCrnXIDrFMspZIUI= golang.org/x/oauth2 v0.11.0 h1:vPL4xzxBM4niKCW6g9whtaWVXTJf1U5e4aZxxFx/gbU= golang.org/x/oauth2 v0.11.0/go.mod h1:LdF7O/8bLR/qWK9DrpXmbHLTouvRHK0SgJl0GmDBchk= +golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -235,6 +253,7 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20210616045830-e2b7044e8c71/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.15.0 h1:h48lPFYpsTvQJZF4EKyI4aLHaev3CxivZmv7yZig9pc= golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= @@ -264,6 +283,7 @@ golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8T golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gomodules.xyz/jsonpatch/v2 v2.4.0 h1:Ci3iUJyx9UeRx7CeFN8ARgGbkESwJK+KB9lLcWxY/Zw= +gomodules.xyz/jsonpatch/v2 v2.4.0/go.mod h1:AH3dM2RI6uoBZxn3LVrfvJ3E0/9dG4cSrbuBJT4moAY= google.golang.org/appengine v1.6.7 h1:FZR1q0exgwxzPzp/aF+VccGrSfxfPpkBqjIIEq3ru6c= google.golang.org/appengine v1.6.7/go.mod h1:8WjMMxjGQR8xUklV/ARdw2HLXBOI7O7uCIDZVag1xfc= google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw= @@ -284,10 +304,13 @@ gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= k8s.io/api v0.28.1 h1:i+0O8k2NPBCPYaMB+uCkseEbawEt/eFaiRqUx8aB108= k8s.io/api v0.28.1/go.mod h1:uBYwID+66wiL28Kn2tBjBYQdEU0Xk0z5qF8bIBqk/Dg= k8s.io/apiextensions-apiserver v0.28.1 h1:l2ThkBRjrWpw4f24uq0Da2HaEgqJZ7pcgiEUTKSmQZw= +k8s.io/apiextensions-apiserver v0.28.1/go.mod h1:sVvrI+P4vxh2YBBcm8n2ThjNyzU4BQGilCQ/JAY5kGs= k8s.io/apimachinery v0.28.1 h1:EJD40og3GizBSV3mkIoXQBsws32okPOy+MkRyzh6nPY= k8s.io/apimachinery v0.28.1/go.mod h1:X0xh/chESs2hP9koe+SdIAcXWcQ+RM5hy0ZynB+yEvw= k8s.io/client-go v0.28.1 h1:pRhMzB8HyLfVwpngWKE8hDcXRqifh1ga2Z/PU9SXVK8= k8s.io/client-go v0.28.1/go.mod h1:pEZA3FqOsVkCc07pFVzK076R+P/eXqsgx5zuuRWukNE= +k8s.io/component-base v0.28.1 h1:LA4AujMlK2mr0tZbQDZkjWbdhTV5bRyEyAFe0TJxlWg= +k8s.io/component-base v0.28.1/go.mod h1:jI11OyhbX21Qtbav7JkhehyBsIRfnO8oEgoAR12ArIU= k8s.io/klog/v2 v2.100.1 h1:7WCHKK6K8fNhTqfBhISHQ97KrnJNFZMcQvKp7gP/tmg= k8s.io/klog/v2 v2.100.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= k8s.io/kube-openapi v0.0.0-20230816210353-14e408962443 h1:CAIciCnJnSOQxPd0xvpV6JU3D4AJvnYbImPpFpO9Hnw= @@ -298,6 +321,8 @@ sigs.k8s.io/controller-runtime v0.16.1 h1:+15lzrmHsE0s2kNl0Dl8cTchI5Cs8qofo5PGcP sigs.k8s.io/controller-runtime v0.16.1/go.mod h1:vpMu3LpI5sYWtujJOa2uPK61nB5rbwlN7BAB8aSLvGU= sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo= sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= +sigs.k8s.io/secrets-store-csi-driver v1.3.4 h1:rCMOb2I4lJaN6sw0CjT6YHA8ts2yscWAOBGu0EaCIWk= +sigs.k8s.io/secrets-store-csi-driver v1.3.4/go.mod h1:jh6wML45aTbxT2YZtU4khzSm8JYxwVrQbhsum+WR6j8= sigs.k8s.io/structured-merge-diff/v4 v4.3.0 h1:UZbZAZfX0wV2zr7YZorDz6GXROfDFj6LvqCRm4VUVKk= sigs.k8s.io/structured-merge-diff/v4 v4.3.0/go.mod h1:N8hJocpFajUSSeSJ9bOZ77VzejKZaXsTtZo4/u7Io08= sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo= diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index b84bab23..afafb398 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -6,6 +6,9 @@ import ( "fmt" "time" + "github.com/Azure/aks-app-routing-operator/pkg/controller/keyvault" + secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "github.com/Azure/aks-app-routing-operator/testing/e2e/infra" "github.com/Azure/aks-app-routing-operator/testing/e2e/logger" @@ -104,14 +107,6 @@ func nicTests(in infra.Provisioned) []test { } validUri := "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34" - testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") - testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - KeyVaultURI: &validUri, - } - lgr.Info("creating NginxIngressController with valid keyvault uri field") - if err := c.Create(ctx, testNIC); err != nil { - return fmt.Errorf("unable to create NginxIngressController despite valid key vault uri'%s'", testNIC.Spec.ControllerNamePrefix) - } invalidUri := "Invalid.URI" testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") @@ -208,6 +203,90 @@ func nicTests(in infra.Provisioned) []test { return err } + lgr.Info("finished testing") + return nil + }, + }, + { + name: "testing DefaultSSLCertificate validity", + cfgs: builderFromInfra(in). + withOsm(in, false, true). + withVersions(manifests.OperatorVersionLatest). + withZones([]manifests.DnsZoneCount{manifests.DnsZoneCountNone}, []manifests.DnsZoneCount{manifests.DnsZoneCountNone}). + build(), + run: func(ctx context.Context, config *rest.Config, operator manifests.OperatorConfig) error { + lgr := logger.FromContext(ctx) + lgr.Info("starting test") + + c, err := client.New(config, client.Options{ + Scheme: scheme, + }) + if err != nil { + return fmt.Errorf("creating client: %w") + } + + // get keyvault uri + kvuri := in.Cert.GetId() + + // create defaultSSLCert + defaultSSLCert := v1alpha1.DefaultSSLCertificate{ + KeyVaultURI: &kvuri, + } + + // create nic and upsert + testNIC := manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") + testNIC.Spec.DefaultSSLCertificate = &defaultSSLCert + if err := upsert(ctx, c, testNIC); err != nil { + return fmt.Errorf("ensuring private NIC: %w", err) + } + + // create and validate that spc was created + spc := &secv1.SecretProviderClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: keyvault.DefaultNginxCertName(testNIC), + Namespace: "app-routing-system", + }, + } + blankSpc := &secv1.SecretProviderClass{} + c.Get(ctx, client.ObjectKeyFromObject(spc), blankSpc) + if err := upsert(ctx, c, testNIC); err != nil { + return fmt.Errorf("upserting NIC: %w", err) + } + + var service v1alpha1.ManagedObjectReference + lgr.Info("waiting for service associated with private NIC to be ready") + if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + lgr.Info("checking if private NIC service is ready") + var nic v1alpha1.NginxIngressController + if err := c.Get(ctx, client.ObjectKeyFromObject(testNIC), &nic); err != nil { + return false, fmt.Errorf("get private nic: %w", err) + } + + if nic.Status.ManagedResourceRefs == nil { + return false, nil + } + + for _, ref := range nic.Status.ManagedResourceRefs { + if ref.Kind == "Service" { + lgr.Info("found service") + service = ref + return true, nil + } + } + + lgr.Info("service not found") + return false, nil + }); err != nil { + return fmt.Errorf("waiting for test NIC to be ready: %w", err) + } + + if err := clientServerTest(ctx, config, operator, nil, in, func(ingress *netv1.Ingress, service *corev1.Service, z zoner) error { + ingress.Spec.IngressClassName = to.Ptr(testNIC.Spec.IngressClassName) + return nil + }, to.Ptr(service.Name)); err != nil { + return err + } + lgr.Info("finished testing") return nil }, From 9276555727bb6e169904934b0d14aa4687c48b67 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 4 Mar 2024 14:21:45 -0500 Subject: [PATCH 12/36] error checking for spc get --- testing/e2e/suites/nginxIngressController.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index afafb398..b751c65b 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -248,7 +248,10 @@ func nicTests(in infra.Provisioned) []test { }, } blankSpc := &secv1.SecretProviderClass{} - c.Get(ctx, client.ObjectKeyFromObject(spc), blankSpc) + if err := c.Get(ctx, client.ObjectKeyFromObject(spc), blankSpc); err != nil { + return fmt.Errorf("getting SPC: %w", err) + } + if err := upsert(ctx, c, testNIC); err != nil { return fmt.Errorf("upserting NIC: %w", err) } From 5420ca3952cd0e7f150114ada64e469773bce48d Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 4 Mar 2024 14:23:43 -0500 Subject: [PATCH 13/36] removing unneeded upsert of nic --- testing/e2e/suites/nginxIngressController.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index b751c65b..7a085d8d 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -237,7 +237,7 @@ func nicTests(in infra.Provisioned) []test { testNIC := manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &defaultSSLCert if err := upsert(ctx, c, testNIC); err != nil { - return fmt.Errorf("ensuring private NIC: %w", err) + return fmt.Errorf("upserting NIC: %w", err) } // create and validate that spc was created @@ -252,10 +252,6 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("getting SPC: %w", err) } - if err := upsert(ctx, c, testNIC); err != nil { - return fmt.Errorf("upserting NIC: %w", err) - } - var service v1alpha1.ManagedObjectReference lgr.Info("waiting for service associated with private NIC to be ready") if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { From 1af06d78c890a02321d50fc612ec5ed2027cef7b Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 4 Mar 2024 15:12:43 -0500 Subject: [PATCH 14/36] added secretproviderclass to scheme --- testing/e2e/suites/nginxIngressController.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 7a085d8d..fbd70f9c 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -43,6 +43,7 @@ func init() { appsv1.AddToScheme(scheme) policyv1.AddToScheme(scheme) rbacv1.AddToScheme(scheme) + secv1.AddToScheme(scheme) } func nicTests(in infra.Provisioned) []test { @@ -252,6 +253,10 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("getting SPC: %w", err) } + if err := upsert(ctx, c, testNIC); err != nil { + return fmt.Errorf("upserting NIC: %w", err) + } + var service v1alpha1.ManagedObjectReference lgr.Info("waiting for service associated with private NIC to be ready") if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { From 2aab9480322059a13137564c2c67daab59fd0b71 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 4 Mar 2024 17:24:33 -0500 Subject: [PATCH 15/36] added wait time to ensure spc is found after nic is created --- testing/e2e/suites/nginxIngressController.go | 40 ++++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index fbd70f9c..7b24c582 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -241,22 +241,6 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("upserting NIC: %w", err) } - // create and validate that spc was created - spc := &secv1.SecretProviderClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: keyvault.DefaultNginxCertName(testNIC), - Namespace: "app-routing-system", - }, - } - blankSpc := &secv1.SecretProviderClass{} - if err := c.Get(ctx, client.ObjectKeyFromObject(spc), blankSpc); err != nil { - return fmt.Errorf("getting SPC: %w", err) - } - - if err := upsert(ctx, c, testNIC); err != nil { - return fmt.Errorf("upserting NIC: %w", err) - } - var service v1alpha1.ManagedObjectReference lgr.Info("waiting for service associated with private NIC to be ready") if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { @@ -284,6 +268,30 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("waiting for test NIC to be ready: %w", err) } + lgr.Info("waiting for spc to be created after NIC") + if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + lgr.Info("checking if associated SPC is created") + lgr.Info("checking if spc is created") + spc := &secv1.SecretProviderClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: keyvault.DefaultNginxCertName(testNIC), + Namespace: "app-routing-system", + }, + } + cleanSPC := &secv1.SecretProviderClass{} + + if err := c.Get(ctx, client.ObjectKeyFromObject(spc), cleanSPC); err != nil { + return false, fmt.Errorf("getting SPC: %w", err) + } else { + return true, nil + } + + lgr.Info("spc not found") + return false, nil + }); err != nil { + return fmt.Errorf("waiting for SPC to be ready: %w", err) + } + if err := clientServerTest(ctx, config, operator, nil, in, func(ingress *netv1.Ingress, service *corev1.Service, z zoner) error { ingress.Spec.IngressClassName = to.Ptr(testNIC.Spec.IngressClassName) return nil From faa7670dc5bfa3e32df1d402fd29a53972259dc1 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 6 Mar 2024 12:33:26 -0500 Subject: [PATCH 16/36] addressing comments --- .../keyvault/ingress_secret_provider_class.go | 37 +++++++++---------- .../ingress_secret_provider_class_test.go | 9 +++-- pkg/controller/keyvault/kv_util.go | 29 +++++++-------- pkg/controller/keyvault/kv_util_test.go | 13 ++++++- .../nginx_secret_provider_class_test.go | 2 +- testing/e2e/suites/nginxIngressController.go | 26 +++++++++---- testing/e2e/suites/utils.go | 9 ----- 7 files changed, 67 insertions(+), 58 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index 84391bc0..78ae6187 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -6,6 +6,7 @@ package keyvault import ( "context" "fmt" + "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" @@ -99,28 +100,27 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re logger = logger.WithValues("spc", spc.Name) // Checking if we manage the ingress. All false cases without an error are assumed that we don't manage it - ok, err := i.ingressManager.IsManaging(ing) - - if err != nil { + var isManaged bool + if isManaged, err = i.ingressManager.IsManaging(ing); err != nil { return result, fmt.Errorf("determining if ingress is managed: %w", err) } - if ok { - ok, err = buildSPC(ing, spc, i.config) - } + if isManaged { + var upsertSPC bool - if err != nil { - logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") - i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) - return result, nil - } - if ok { - logger.Info("reconciling secret provider class for ingress") - err = util.Upsert(ctx, i.client, spc) - if err != nil { - i.events.Eventf(ing, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err) + if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { + logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") + i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) + return result, nil + } + + if upsertSPC { + logger.Info("reconciling secret provider class for ingress") + if err = util.Upsert(ctx, i.client, spc); err != nil { + i.events.Eventf(ing, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err) + } + return result, err } - return result, err } logger.Info("cleaning unused managed spc for ingress") @@ -128,8 +128,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re toCleanSPC := &secv1.SecretProviderClass{} - err = i.client.Get(ctx, client.ObjectKeyFromObject(spc), toCleanSPC) - if err != nil { + if err = i.client.Get(ctx, client.ObjectKeyFromObject(spc), toCleanSPC); err != nil { return result, client.IgnoreNotFound(err) } diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index fd22957c..e8be2868 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "net/url" + "sigs.k8s.io/controller-runtime/pkg/log/zap" "testing" "github.com/go-logr/logr" @@ -44,7 +45,7 @@ var ( IngressClassName: &spcTestIngressClassName, }, } - spcTestDefaultConf = BuildTestSpcConfig("test-msi", "test-tenant", "test-cloud") + spcTestDefaultConf = buildTestSpcConfig("test-msi", "test-tenant", "test-cloud") ) func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { @@ -69,7 +70,7 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { } ctx := context.Background() - ctx = logr.NewContext(ctx, logr.Discard()) + ctx = logr.NewContext(ctx, zap.New()) // Create the secret provider class req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: ing.Namespace, Name: ing.Name}} @@ -393,7 +394,7 @@ func TestIngressSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { } spc := &secv1.SecretProviderClass{} - ok, err := buildSPC(ing, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) + ok, err := buildSPC(ing, spc, buildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) require.NoError(t, err, "building SPC should not error") require.True(t, ok, "SPC should be built") @@ -404,7 +405,7 @@ func TestIngressSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { } } -func BuildTestSpcConfig(msiClient, tenantID, cloud string) *config.Config { +func buildTestSpcConfig(msiClient, tenantID, cloud string) *config.Config { spcTestConf := config.Config{ MSIClientID: msiClient, TenantID: tenantID, diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go index 51554843..54db7ee0 100644 --- a/pkg/controller/keyvault/kv_util.go +++ b/pkg/controller/keyvault/kv_util.go @@ -3,15 +3,16 @@ package keyvault import ( "encoding/json" "fmt" + "net/url" + "reflect" + "strings" + "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "github.com/Azure/aks-app-routing-operator/pkg/config" kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" v1 "k8s.io/api/networking/v1" - "net/url" - "reflect" "sigs.k8s.io/controller-runtime/pkg/client" secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" - "strings" ) var nginxNamePrefix = "keyvault-nginx-" @@ -19,24 +20,22 @@ var nginxNamePrefix = "keyvault-nginx-" func buildSPC(obj client.Object, spc *secv1.SecretProviderClass, config *config.Config) (bool, error) { var certURI, specSecretName string - switch objType := reflect.TypeOf(obj).String(); objType { - case "*v1alpha1.NginxIngressController": - nic, _ := obj.(*v1alpha1.NginxIngressController) - if nic.Spec.DefaultSSLCertificate == nil || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { + switch t := obj.(type) { + case *v1alpha1.NginxIngressController: + if t.Spec.DefaultSSLCertificate == nil || t.Spec.DefaultSSLCertificate.KeyVaultURI == nil { return false, nil } - certURI = *nic.Spec.DefaultSSLCertificate.KeyVaultURI - specSecretName = DefaultNginxCertName(nic) - case "*v1.Ingress": - ing, _ := obj.(*v1.Ingress) - if ing.Annotations == nil { + certURI = *t.Spec.DefaultSSLCertificate.KeyVaultURI + specSecretName = DefaultNginxCertName(t) + case *v1.Ingress: + if t.Annotations == nil { return false, nil } - certURI = ing.Annotations["kubernetes.azure.com/tls-cert-keyvault-uri"] - specSecretName = fmt.Sprintf("keyvault-%s", ing.Name) + certURI = t.Annotations["kubernetes.azure.com/tls-cert-keyvault-uri"] + specSecretName = fmt.Sprintf("keyvault-%s", t.Name) default: - return false, fmt.Errorf("incorrect object type: %s", objType) + return false, fmt.Errorf("incorrect object type: %s", reflect.TypeOf(obj).String()) } if certURI == "" { diff --git a/pkg/controller/keyvault/kv_util_test.go b/pkg/controller/keyvault/kv_util_test.go index 1ca1f167..794f1533 100644 --- a/pkg/controller/keyvault/kv_util_test.go +++ b/pkg/controller/keyvault/kv_util_test.go @@ -8,13 +8,22 @@ import ( ) var ( - maxString = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbb" + testName = "testName" + maxSizeString = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbb" ) func TestDefaultNginxCertName(t *testing.T) { + testStr := DefaultNginxCertName(&v1alpha1.NginxIngressController{ ObjectMeta: metav1.ObjectMeta{ - Name: maxString, + Name: testName, + }, + }) + require.Equal(t, testStr, nginxNamePrefix+testName) + + testStr = DefaultNginxCertName(&v1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: maxSizeString, }, }) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index ea91d613..1de6f58a 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -382,7 +382,7 @@ func TestNginxSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { nic.Spec.DefaultSSLCertificate.KeyVaultURI = &testSecretUri spc := &secv1.SecretProviderClass{} - ok, err := buildSPC(nic, spc, BuildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) + ok, err := buildSPC(nic, spc, buildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) require.NoError(t, err, "building SPC should not error") require.True(t, ok, "SPC should be built") diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 7b24c582..05266a7e 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -82,7 +82,10 @@ func nicTests(in infra.Provisioned) []test { testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - Secret: getTestSecret("Invalid+@Name", "validnamespace"), + Secret: &v1alpha1.Secret{ + Name: "Invalid+@Name", + Namespace: "validnamespace", + }, } lgr.Info("creating NginxIngressController with invalid Secret field") if err := c.Create(ctx, testNIC); err == nil { @@ -91,7 +94,10 @@ func nicTests(in infra.Provisioned) []test { testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - Secret: getTestSecret("validname", "Invalid+@Namespace"), + Secret: &v1alpha1.Secret{ + Name: "validname", + Namespace: "Invalid+@Namespace", + }, } lgr.Info("creating NginxIngressController with invalid Secret field") if err := c.Create(ctx, testNIC); err == nil { @@ -100,7 +106,10 @@ func nicTests(in infra.Provisioned) []test { testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ - Secret: getTestSecret("validname", ""), + Secret: &v1alpha1.Secret{ + Name: "validname", + Namespace: "", + }, } lgr.Info("creating NginxIngressController with empty Secret field") if err := c.Create(ctx, testNIC); err == nil { @@ -122,7 +131,10 @@ func nicTests(in infra.Provisioned) []test { testNIC = manifests.NewNginxIngressController("nginx-ingress-controller", "nginxingressclass") testNIC.Spec.DefaultSSLCertificate = &v1alpha1.DefaultSSLCertificate{ KeyVaultURI: &validUri, - Secret: getTestSecret("validname", "validnamespace"), + Secret: &v1alpha1.Secret{ + Name: "validname", + Namespace: "validnamespace", + }, } lgr.Info("creating NginxIngressController with both keyvault uri field and secret field") if err := c.Create(ctx, testNIC); err == nil { @@ -280,14 +292,12 @@ func nicTests(in infra.Provisioned) []test { } cleanSPC := &secv1.SecretProviderClass{} - if err := c.Get(ctx, client.ObjectKeyFromObject(spc), cleanSPC); err != nil { - return false, fmt.Errorf("getting SPC: %w", err) - } else { + if err := c.Get(ctx, client.ObjectKeyFromObject(spc), cleanSPC); err == nil { return true, nil } lgr.Info("spc not found") - return false, nil + return false, fmt.Errorf("getting SPC: %w", err) }); err != nil { return fmt.Errorf("waiting for SPC to be ready: %w", err) } diff --git a/testing/e2e/suites/utils.go b/testing/e2e/suites/utils.go index d7805b40..50481c0f 100644 --- a/testing/e2e/suites/utils.go +++ b/testing/e2e/suites/utils.go @@ -3,7 +3,6 @@ package suites import ( "context" "fmt" - "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "time" "github.com/Azure/aks-app-routing-operator/testing/e2e/logger" @@ -63,11 +62,3 @@ func upsert(ctx context.Context, c client.Client, obj client.Object) error { lgr.Info("object updated") return nil } - -func getTestSecret(name, namespace string) *v1alpha1.Secret { - sec := &v1alpha1.Secret{ - Name: name, - Namespace: namespace, - } - return sec -} From ade7a2260c5b374736af478212e10b35ba7c184c Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 11 Mar 2024 13:57:13 -0400 Subject: [PATCH 17/36] removed waits for spc and service in e2e test --- .../keyvault/nginx_secret_provider_class.go | 4 +- testing/e2e/suites/nginxIngressController.go | 73 ++++++++++--------- 2 files changed, 40 insertions(+), 37 deletions(-) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index a10cdb2b..d8a1f1e6 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -95,13 +95,13 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req }, } logger = logger.WithValues("spc", spc.Name) - ok, err := buildSPC(nic, spc, i.config) + upsertSPC, err := buildSPC(nic, spc, i.config) if err != nil { logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) return result, nil } - if ok { + if upsertSPC { logger.Info("reconciling secret provider class for ingress") err = util.Upsert(ctx, i.client, spc) if err != nil { diff --git a/testing/e2e/suites/nginxIngressController.go b/testing/e2e/suites/nginxIngressController.go index 05266a7e..aae34147 100644 --- a/testing/e2e/suites/nginxIngressController.go +++ b/testing/e2e/suites/nginxIngressController.go @@ -4,9 +4,9 @@ import ( "context" "errors" "fmt" + "github.com/Azure/aks-app-routing-operator/pkg/controller/keyvault" "time" - "github.com/Azure/aks-app-routing-operator/pkg/controller/keyvault" secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" "github.com/Azure/aks-app-routing-operator/api/v1alpha1" @@ -253,53 +253,56 @@ func nicTests(in infra.Provisioned) []test { return fmt.Errorf("upserting NIC: %w", err) } - var service v1alpha1.ManagedObjectReference - lgr.Info("waiting for service associated with private NIC to be ready") - if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { - lgr.Info("checking if private NIC service is ready") - var nic v1alpha1.NginxIngressController + var service *v1alpha1.ManagedObjectReference + var nic v1alpha1.NginxIngressController + lgr.Info("waiting for NIC to be available") + if err = wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + lgr.Info("checking if NIC is available") if err := c.Get(ctx, client.ObjectKeyFromObject(testNIC), &nic); err != nil { - return false, fmt.Errorf("get private nic: %w", err) + return false, fmt.Errorf("get nic: %w", err) } - if nic.Status.ManagedResourceRefs == nil { - return false, nil - } - - for _, ref := range nic.Status.ManagedResourceRefs { - if ref.Kind == "Service" { - lgr.Info("found service") - service = ref + for _, cond := range nic.Status.Conditions { + if cond.Type == v1alpha1.ConditionTypeAvailable { + lgr.Info("found nic") + if len(nic.Status.ManagedResourceRefs) == 0 { + lgr.Info("nic has no ManagedResourceRefs") + return false, nil + } return true, nil } } - - lgr.Info("service not found") + lgr.Info("nic not available") return false, nil }); err != nil { - return fmt.Errorf("waiting for test NIC to be ready: %w", err) + return fmt.Errorf("waiting for test NIC to be available: %w", err) } - lgr.Info("waiting for spc to be created after NIC") - if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { - lgr.Info("checking if associated SPC is created") - lgr.Info("checking if spc is created") - spc := &secv1.SecretProviderClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: keyvault.DefaultNginxCertName(testNIC), - Namespace: "app-routing-system", - }, - } - cleanSPC := &secv1.SecretProviderClass{} + lgr.Info("checking if associated SPC is created") + spc := &secv1.SecretProviderClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: keyvault.DefaultNginxCertName(testNIC), + Namespace: "app-routing-system", + }, + } + cleanSPC := &secv1.SecretProviderClass{} - if err := c.Get(ctx, client.ObjectKeyFromObject(spc), cleanSPC); err == nil { - return true, nil + if err := c.Get(ctx, client.ObjectKeyFromObject(spc), cleanSPC); err != nil { + lgr.Info("spc not found") + return err + } + lgr.Info("found spc") + + lgr.Info("checking for service in managed resource refs") + for _, ref := range nic.Status.ManagedResourceRefs { + if ref.Kind == "Service" { + lgr.Info("found service") + service = &ref } + } - lgr.Info("spc not found") - return false, fmt.Errorf("getting SPC: %w", err) - }); err != nil { - return fmt.Errorf("waiting for SPC to be ready: %w", err) + if service == nil { + return fmt.Errorf("no service available in resource refs") } if err := clientServerTest(ctx, config, operator, nil, in, func(ingress *netv1.Ingress, service *corev1.Service, z zoner) error { From cda8f475d18069702676f2ef22db3a3fa52dc668 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Tue, 12 Mar 2024 15:38:35 -0400 Subject: [PATCH 18/36] Reverted from zap to logr.Discard --- pkg/controller/keyvault/ingress_secret_provider_class_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index e8be2868..e21ad4a4 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "net/url" - "sigs.k8s.io/controller-runtime/pkg/log/zap" "testing" "github.com/go-logr/logr" @@ -70,7 +69,7 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { } ctx := context.Background() - ctx = logr.NewContext(ctx, zap.New()) + ctx = logr.NewContext(ctx, logr.Discard()) // Create the secret provider class req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: ing.Namespace, Name: ing.Name}} From 02933287d06c974f553b1dcdcc4728b407168b31 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 20 Mar 2024 13:14:06 -0400 Subject: [PATCH 19/36] Addressing comments --- .../keyvault/nginx_secret_provider_class.go | 9 ++++++-- pkg/controller/keyvault/placeholder_pod.go | 22 ++++++++----------- .../nginxingress/nginx_ingress_controller.go | 8 +++---- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index d8a1f1e6..ed48e187 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -74,7 +74,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req if err != nil { return result, client.IgnoreNotFound(err) } - logger = logger.WithValues("name", nic.Name, "namespace", config.DefaultNs, "generation", nic.Generation) + logger = logger.WithValues("name", nic.Name, "generation", nic.Generation) spc := &secv1.SecretProviderClass{ TypeMeta: metav1.TypeMeta{ @@ -95,6 +95,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req }, } logger = logger.WithValues("spc", spc.Name) + logger.Info("building spc and upserting if managed with labels") upsertSPC, err := buildSPC(nic, spc, i.config) if err != nil { logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") @@ -108,9 +109,11 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req i.events.Eventf(nic, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err) } return result, err + } else { + logger.Info("spc is either not managed or key vault uri was removed") } - logger.Info("cleaning unused managed spc for ingress") + logger.Info("cleaning unused spc if ingress is managed") logger.Info("getting secret provider class for ingress") toCleanSPC := &secv1.SecretProviderClass{} @@ -124,6 +127,8 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req logger.Info("removing secret provider class for ingress") err = i.client.Delete(ctx, toCleanSPC) return result, client.IgnoreNotFound(err) + } else { + logger.Info("spc was not managed so spc was not removed") } return result, nil diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index cd0ee0c2..6d309a24 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -112,19 +112,17 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } func (p *PlaceholderPodController) placeholderPodCleanCheck(obj client.Object) (bool, error) { - switch obj.GetObjectKind().GroupVersionKind().Kind { - case "NginxIngressController": - nic, _ := obj.(*v1alpha1.NginxIngressController) - if nic.Spec.DefaultSSLCertificate == nil || nic.Spec.DefaultSSLCertificate.KeyVaultURI == nil { + switch t := obj.(type) { + case *v1alpha1.NginxIngressController: + if t.Spec.DefaultSSLCertificate == nil || t.Spec.DefaultSSLCertificate.KeyVaultURI == nil { return true, nil } - case "Ingress": - ing, _ := obj.(*netv1.Ingress) - managed, err := p.ingressManager.IsManaging(ing) + case *netv1.Ingress: + managed, err := p.ingressManager.IsManaging(t) if err != nil { return false, fmt.Errorf("determining if ingress is managed: %w", err) } - if ing.Name == "" || ing.Spec.IngressClassName == nil || !managed { + if t.Name == "" || t.Spec.IngressClassName == nil || !managed { return true, nil } } @@ -144,12 +142,10 @@ func (p *PlaceholderPodController) reconcileObjectDeployment(dep *appsv1.Deploym case util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController") != "": obj = &v1alpha1.NginxIngressController{} obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController")) - break case util.FindOwnerKind(spc.OwnerReferences, "Ingress") != "": obj = &netv1.Ingress{} obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "Ingress")) obj.SetNamespace(req.Namespace) - break default: return result, fmt.Errorf("failed to reconcile object deployment: object type not nginxIngressController or ingress") } @@ -222,10 +218,10 @@ func (p *PlaceholderPodController) buildDeployment(ctx context.Context, dep *app } var ingAnnotation string - switch obj.GetObjectKind().GroupVersionKind().Kind { - case "NginxIngressController": + switch obj.(type) { + case *v1alpha1.NginxIngressController: ingAnnotation = "kubernetes.azure.com/nginx-ingress-controller-owner" - case "Ingress": + case *netv1.Ingress: ingAnnotation = "kubernetes.azure.com/ingress-owner" default: return fmt.Errorf("failed to build deployment: object type not ingress or nginxingresscontroller") diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index 2516bbdb..e05e153a 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -528,11 +528,11 @@ func ToNginxIngressConfig(nic *approutingv1alpha1.NginxIngressController, defaul }, } - if nic.Spec.DefaultSSLCertificate != nil { - if nic.Spec.DefaultSSLCertificate.Secret != nil && nic.Spec.DefaultSSLCertificate.Secret.Name != "" && nic.Spec.DefaultSSLCertificate.Secret.Namespace != "" { - nginxIng.DefaultSSLCertificate = nic.Spec.DefaultSSLCertificate.Secret.Namespace + "/" + nic.Spec.DefaultSSLCertificate.Secret.Name + if cert := nic.Spec.DefaultSSLCertificate; cert != nil { + if cert.Secret != nil && cert.Secret.Name != "" && cert.Secret.Namespace != "" { + nginxIng.DefaultSSLCertificate = cert.Secret.Namespace + "/" + cert.Secret.Name } - if nic.Spec.DefaultSSLCertificate != nil && nic.Spec.DefaultSSLCertificate.KeyVaultURI != nil { + if cert != nil && cert.KeyVaultURI != nil { nginxIng.DefaultSSLCertificate = config.DefaultNs + "/" + keyvault.DefaultNginxCertName(nic) } } From e6a9959a35e726298d12ad8fca479bffe1dd7c2d Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 20 Mar 2024 14:18:46 -0400 Subject: [PATCH 20/36] typo fix --- api/v1alpha1/nginxingresscontroller_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/v1alpha1/nginxingresscontroller_types.go b/api/v1alpha1/nginxingresscontroller_types.go index 59106cd6..0347d7f6 100644 --- a/api/v1alpha1/nginxingresscontroller_types.go +++ b/api/v1alpha1/nginxingresscontroller_types.go @@ -61,7 +61,7 @@ type NginxIngressControllerSpec struct { Scaling *Scaling `json:"scaling,omitempty"` } -// DefaultSSLCertificate holds a secret in the form of a secret struct ith name and namespace properties or a key vault uri +// DefaultSSLCertificate holds a secret in the form of a secret struct with name and namespace properties or a key vault uri // +kubebuilder:validation:MaxProperties=1 // +kubebuilder:validation:XValidation:rule="(isURL(self.keyVaultURI) || !has(self.keyVaultURI))" type DefaultSSLCertificate struct { From 48e6789b47ba1eb3053d16f57df47b461b2a2422 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Fri, 22 Mar 2024 14:57:45 -0400 Subject: [PATCH 21/36] Addressing comments --- .../keyvault/ingress_secret_provider_class.go | 9 ++++++--- pkg/controller/keyvault/kv_util.go | 11 +++++++---- .../keyvault/nginx_secret_provider_class.go | 10 +++++++--- pkg/controller/keyvault/placeholder_pod.go | 3 ++- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index 78ae6187..632e8eec 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -109,9 +109,12 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re var upsertSPC bool if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { - logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") - i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) - return result, nil + if userErr := err.(buildSPCUserError); userErr != nil { + logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") + i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.Error()) + return result, nil + } + return result, err } if upsertSPC { diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go index 54db7ee0..ab3d3577 100644 --- a/pkg/controller/keyvault/kv_util.go +++ b/pkg/controller/keyvault/kv_util.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "net/url" - "reflect" "strings" "github.com/Azure/aks-app-routing-operator/api/v1alpha1" @@ -35,7 +34,7 @@ func buildSPC(obj client.Object, spc *secv1.SecretProviderClass, config *config. certURI = t.Annotations["kubernetes.azure.com/tls-cert-keyvault-uri"] specSecretName = fmt.Sprintf("keyvault-%s", t.Name) default: - return false, fmt.Errorf("incorrect object type: %s", reflect.TypeOf(obj).String()) + return false, fmt.Errorf("incorrect object type: %s", t) } if certURI == "" { @@ -44,12 +43,12 @@ func buildSPC(obj client.Object, spc *secv1.SecretProviderClass, config *config. uri, err := url.Parse(certURI) if err != nil { - return false, err + return false, buildSPCUserError(err) } vaultName := strings.Split(uri.Host, ".")[0] chunks := strings.Split(uri.Path, "/") if len(chunks) < 3 { - return false, fmt.Errorf("invalid secret uri: %s", certURI) + return false, buildSPCUserError(fmt.Errorf("invalid secret uri: %s", certURI)) } secretName := chunks[2] p := map[string]interface{}{ @@ -114,3 +113,7 @@ func DefaultNginxCertName(nic *v1alpha1.NginxIngressController) string { return certName } + +type buildSPCUserError interface { + error +} diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index ed48e187..da715d9e 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -97,11 +97,15 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req logger = logger.WithValues("spc", spc.Name) logger.Info("building spc and upserting if managed with labels") upsertSPC, err := buildSPC(nic, spc, i.config) + if err != nil { - logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") - i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err) - return result, nil + if userErr := err.(buildSPCUserError); userErr != nil { + i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.Error()) + return result, nil + } + return result, err } + if upsertSPC { logger.Info("reconciling secret provider class for ingress") err = util.Upsert(ctx, i.client, spc) diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 6d309a24..963bbb33 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -147,7 +147,8 @@ func (p *PlaceholderPodController) reconcileObjectDeployment(dep *appsv1.Deploym obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "Ingress")) obj.SetNamespace(req.Namespace) default: - return result, fmt.Errorf("failed to reconcile object deployment: object type not nginxIngressController or ingress") + p.events.Eventf(obj, "Waring", "FailedReconcileObjectDeployment", "failed to reconcile object deployment: object type not nginxIngressController or ingress") + return result, nil } if obj.GetName() != "" { From 71a87729ab31e700fdbadd3c45af95cbc278c95c Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Fri, 22 Mar 2024 17:16:32 -0400 Subject: [PATCH 22/36] Added tests for buildDeployment and moved buildSPC tests to kv_util_test --- .../ingress_secret_provider_class_test.go | 90 --------- pkg/controller/keyvault/kv_util_test.go | 171 ++++++++++++++++++ .../nginx_secret_provider_class_test.go | 92 ---------- .../keyvault/placeholder_pod_test.go | 43 +++++ 4 files changed, 214 insertions(+), 182 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index e21ad4a4..9a7884c9 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -6,7 +6,6 @@ package keyvault import ( "context" "fmt" - "net/url" "testing" "github.com/go-logr/logr" @@ -27,7 +26,6 @@ import ( "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" ) var ( @@ -316,94 +314,6 @@ func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) { assert.Greater(t, afterRequestCount, beforeRequestCount) } -func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { - invalidURLIng := &netv1.Ingress{ - Spec: netv1.IngressSpec{ - IngressClassName: &spcTestIngressClassName, - }, - } - - t.Run("nil annotations", func(t *testing.T) { - ing := invalidURLIng.DeepCopy() - - ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - require.NoError(t, err) - }) - - t.Run("empty url", func(t *testing.T) { - ing := invalidURLIng.DeepCopy() - ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": ""} - - ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - require.NoError(t, err) - }) - - t.Run("url with control character", func(t *testing.T) { - ing := invalidURLIng.DeepCopy() - cc := string([]byte{0x7f}) - ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": cc} - - ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - _, expectedErr := url.Parse(cc) // the exact error depends on operating system - require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) - }) - - t.Run("url with one path segment", func(t *testing.T) { - ing := invalidURLIng.DeepCopy() - ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "http://test.com/foo"} - - ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - require.EqualError(t, err, "invalid secret uri: http://test.com/foo") - }) -} - -func TestIngressSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { - cases := []struct { - name, configCloud, spcCloud string - expected bool - }{ - { - name: "empty config cloud", - configCloud: "", - expected: false, - }, - { - name: "public cloud", - configCloud: "AzurePublicCloud", - spcCloud: "AzurePublicCloud", - expected: true, - }, - { - name: "sov cloud", - configCloud: "AzureUSGovernmentCloud", - spcCloud: "AzureUSGovernmentCloud", - expected: true, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - 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{} - ok, err := buildSPC(ing, spc, buildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) - require.NoError(t, err, "building SPC should not error") - require.True(t, ok, "SPC should be built") - - spcCloud, ok := spc.Spec.Parameters[kvcsi.CloudNameParameter] - require.Equal(t, c.expected, ok, "SPC cloud annotation unexpected") - require.Equal(t, c.spcCloud, spcCloud, "SPC cloud annotation doesn't match") - }) - } -} - func buildTestSpcConfig(msiClient, tenantID, cloud string) *config.Config { spcTestConf := config.Config{ MSIClientID: msiClient, diff --git a/pkg/controller/keyvault/kv_util_test.go b/pkg/controller/keyvault/kv_util_test.go index 1b2706dd..8002a861 100644 --- a/pkg/controller/keyvault/kv_util_test.go +++ b/pkg/controller/keyvault/kv_util_test.go @@ -1,13 +1,46 @@ package keyvault import ( + "fmt" "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + netv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "net/url" + "sigs.k8s.io/controller-runtime/pkg/client" + secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" "testing" ) var ( + buildSpcTestIngress = &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, + }, + } + + buildSpcTestNginxIngress = &v1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nic", + Namespace: "test-namespace", + }, + Spec: v1alpha1.NginxIngressControllerSpec{ + IngressClassName: spcTestNginxIngressClassName, + DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{ + Secret: nil, + KeyVaultURI: &spcTestKeyVaultURI}, + }, + } + testName = "testName" maxSizeString = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbb" ) @@ -35,3 +68,141 @@ func TestCertSecretName(t *testing.T) { require.Equal(t, "keyvault-ingressname", certSecretName("ingressname")) require.Equal(t, "keyvault-anotheringressname", certSecretName("anotheringressname")) } + +func TestIngressSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { + cases := []struct { + name, configCloud, spcCloud string + expected bool + }{ + { + name: "empty config cloud", + configCloud: "", + expected: false, + }, + { + name: "public cloud", + configCloud: "AzurePublicCloud", + spcCloud: "AzurePublicCloud", + expected: true, + }, + { + name: "sov cloud", + configCloud: "AzureUSGovernmentCloud", + spcCloud: "AzureUSGovernmentCloud", + expected: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ing := buildSpcTestIngress.DeepCopy() + ing.Annotations = map[string]string{ + "kubernetes.azure.com/tls-cert-keyvault-uri": "https://test.vault.azure.net/secrets/test-secret", + } + + spc := &secv1.SecretProviderClass{} + ok, err := buildSPC(ing, spc, buildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) + require.NoError(t, err, "building SPC should not error") + require.True(t, ok, "SPC should be built") + + spcCloud, ok := spc.Spec.Parameters[kvcsi.CloudNameParameter] + require.Equal(t, c.expected, ok, "SPC cloud annotation unexpected") + require.Equal(t, c.spcCloud, spcCloud, "SPC cloud annotation doesn't match") + }) + } +} + +func TestNginxSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { + cases := []struct { + name, configCloud, spcCloud string + expected bool + }{ + { + name: "empty config cloud", + configCloud: "", + expected: false, + }, + { + name: "public cloud", + configCloud: "AzurePublicCloud", + spcCloud: "AzurePublicCloud", + expected: true, + }, + { + name: "sov cloud", + configCloud: "AzureUSGovernmentCloud", + spcCloud: "AzureUSGovernmentCloud", + expected: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + nic := buildSpcTestNginxIngress.DeepCopy() + testSecretUri := "https://test.vault.azure.net/secrets/test-secret" + nic.Spec.DefaultSSLCertificate.KeyVaultURI = &testSecretUri + + spc := &secv1.SecretProviderClass{} + ok, err := buildSPC(nic, spc, buildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) + require.NoError(t, err, "building SPC should not error") + require.True(t, ok, "SPC should be built") + + spcCloud, ok := spc.Spec.Parameters[kvcsi.CloudNameParameter] + require.Equal(t, c.expected, ok, "SPC cloud annotation unexpected") + require.Equal(t, c.spcCloud, spcCloud, "SPC cloud annotation doesn't match") + }) + } +} + +func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { + invalidURLIng := &netv1.Ingress{ + Spec: netv1.IngressSpec{ + IngressClassName: &spcTestIngressClassName, + }, + } + + t.Run("nil annotations", func(t *testing.T) { + ing := invalidURLIng.DeepCopy() + + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + assert.False(t, ok) + require.NoError(t, err) + }) + + t.Run("empty url", func(t *testing.T) { + ing := invalidURLIng.DeepCopy() + ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": ""} + + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + assert.False(t, ok) + require.NoError(t, err) + }) + + t.Run("url with control character", func(t *testing.T) { + ing := invalidURLIng.DeepCopy() + cc := string([]byte{0x7f}) + ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": cc} + + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + assert.False(t, ok) + _, expectedErr := url.Parse(cc) // the exact error depends on operating system + require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) + }) + + t.Run("url with one path segment", func(t *testing.T) { + ing := invalidURLIng.DeepCopy() + ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "http://test.com/foo"} + + ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) + assert.False(t, ok) + require.EqualError(t, err, "invalid secret uri: http://test.com/foo") + }) +} + +func TestBuildSPCWithWrongObject(t *testing.T) { + var obj client.Object + + ok, err := buildSPC(obj, &secv1.SecretProviderClass{}, spcTestDefaultConf) + assert.False(t, ok) + require.EqualError(t, err, fmt.Sprintf("incorrect object type: %s", obj)) +} diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index 1de6f58a..f91b2f7a 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -5,10 +5,8 @@ package keyvault import ( "context" - "fmt" "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "k8s.io/apimachinery/pkg/runtime" - "net/url" "testing" "github.com/go-logr/logr" @@ -28,7 +26,6 @@ import ( "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" ) var ( @@ -303,92 +300,3 @@ func TestNginxSecretProviderClassReconcilerInvalidURL(t *testing.T) { assert.Greater(t, afterErrCount, beforeErrCount) assert.Greater(t, afterRequestCount, beforeRequestCount) } - -func TestNginxSecretProviderClassReconcilerbuildSPCInvalidURLs(t *testing.T) { - invalidURLIng := &v1alpha1.NginxIngressController{ - Spec: v1alpha1.NginxIngressControllerSpec{ - IngressClassName: spcTestNginxIngressClassName, - DefaultSSLCertificate: &v1alpha1.DefaultSSLCertificate{KeyVaultURI: nil}, - }, - } - - t.Run("missing nic class name", func(t *testing.T) { - nic := invalidURLIng.DeepCopy() - nic.Spec.IngressClassName = "" - - ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - require.NoError(t, err) - }) - - t.Run("empty key vault uri", func(t *testing.T) { - nic := invalidURLIng.DeepCopy() - - ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - require.NoError(t, err) - }) - - t.Run("url with control character", func(t *testing.T) { - nic := invalidURLIng.DeepCopy() - cc := string([]byte{0x7f}) - nic.Spec.DefaultSSLCertificate.KeyVaultURI = &cc - - ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - _, expectedErr := url.Parse(cc) // the exact error depends on operating system - require.EqualError(t, err, fmt.Sprintf("%s", expectedErr)) - }) - - t.Run("url with one path segment", func(t *testing.T) { - nic := invalidURLIng.DeepCopy() - fooTestUri := "http://test.com/foo" - nic.Spec.DefaultSSLCertificate.KeyVaultURI = &fooTestUri - - ok, err := buildSPC(nic, &secv1.SecretProviderClass{}, spcTestDefaultConf) - assert.False(t, ok) - require.EqualError(t, err, "invalid secret uri: http://test.com/foo") - }) -} - -func TestNginxSecretProviderClassReconcilerbuildSPCCloud(t *testing.T) { - cases := []struct { - name, configCloud, spcCloud string - expected bool - }{ - { - name: "empty config cloud", - configCloud: "", - expected: false, - }, - { - name: "public cloud", - configCloud: "AzurePublicCloud", - spcCloud: "AzurePublicCloud", - expected: true, - }, - { - name: "sov cloud", - configCloud: "AzureUSGovernmentCloud", - spcCloud: "AzureUSGovernmentCloud", - expected: true, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - nic := spcTestNginxIngress.DeepCopy() - testSecretUri := "https://test.vault.azure.net/secrets/test-secret" - nic.Spec.DefaultSSLCertificate.KeyVaultURI = &testSecretUri - - spc := &secv1.SecretProviderClass{} - ok, err := buildSPC(nic, spc, buildTestSpcConfig("test-msi", "test-tenant", c.configCloud)) - require.NoError(t, err, "building SPC should not error") - require.True(t, ok, "SPC should be built") - - spcCloud, ok := spc.Spec.Parameters[kvcsi.CloudNameParameter] - require.Equal(t, c.expected, ok, "SPC cloud annotation unexpected") - require.Equal(t, c.spcCloud, spcCloud, "SPC cloud annotation doesn't match") - }) - } -} diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 1ebd1343..a546f619 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -71,6 +71,16 @@ var ( }}, }, } + placeholderDeployment = &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "default", + }, + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + } ) func TestPlaceholderPodControllerIntegrationWithIng(t *testing.T) { @@ -634,6 +644,39 @@ func TestPlaceholderPodCleanCheck(t *testing.T) { require.Error(t, err, "determining if ingress is managed: an error has occured checking if ingress is managed") require.Equal(t, false, cleanPod) } + +func TestBuildDeployment(t *testing.T) { + ctx := context.Background() + ctx = logr.NewContext(ctx, logr.Discard()) + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + require.NoError(t, netv1.AddToScheme(scheme)) + require.NoError(t, appsv1.AddToScheme(scheme)) + + spc := placeholderSpc.DeepCopy() + + c := fake.NewClientBuilder().WithScheme(scheme).Build() + p := &PlaceholderPodController{ + client: c, + config: &config.Config{Registry: "test-registry"}, + ingressManager: NewIngressManagerFromFn(func(ing *netv1.Ingress) (bool, error) { + return true, nil + }), + } + + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: spc.Name, + Namespace: spc.Namespace, + }, + } + + var emptyObj client.Object + + err := p.buildDeployment(ctx, dep, spc, emptyObj) + require.EqualError(t, err, "failed to build deployment: object type not ingress or nginxingresscontroller") +} + func getDefaultNginxSpc(nic *v1alpha1.NginxIngressController) *secv1.SecretProviderClass { spc := &secv1.SecretProviderClass{ TypeMeta: metav1.TypeMeta{ From 9e3f751a7b8dc8a3aae4cc1dec6f000d0bf3e3ab Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 25 Mar 2024 13:00:44 -0400 Subject: [PATCH 23/36] ingressClassName error check test --- .../ingress_secret_provider_class_test.go | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index 9a7884c9..36b9ad6e 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -61,7 +61,9 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { if *ing.Spec.IngressClassName == spcTestIngressClassName { return true, nil } - + if *ing.Spec.IngressClassName == "error" { + return false, fmt.Errorf("ingressClassNameError") + } return false, nil }), } @@ -116,6 +118,28 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { require.Equal(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount) require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + // Error check for isManaged function + ingClassName := ing.Spec.IngressClassName + errorName := "error" + ing.Spec.IngressClassName = &errorName + + require.NoError(t, i.client.Update(ctx, ing)) + beforeErrCount = testutils.GetErrMetricCount(t, ingressSecretProviderControllerName) + beforeRequestCount = testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess) + _, err = i.Reconcile(ctx, req) + require.Errorf(t, err, fmt.Sprintf("determining if ingress is managed: %w", "ingressClassNameError")) + require.Greater(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount) + require.Equal(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) + + ing.Spec.IngressClassName = ingClassName + require.NoError(t, i.client.Update(ctx, ing)) + 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) + // Remove the cert's version from the ingress ing.Annotations = map[string]string{ "kubernetes.azure.com/tls-cert-keyvault-uri": "https://testvault.vault.azure.net/certificates/testcert", From c054fb1c67369fe1474e964f210368620a8c67ff Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Mon, 25 Mar 2024 13:05:37 -0400 Subject: [PATCH 24/36] string fix in test --- pkg/controller/keyvault/ingress_secret_provider_class_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class_test.go b/pkg/controller/keyvault/ingress_secret_provider_class_test.go index 36b9ad6e..4706dfb3 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class_test.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class_test.go @@ -127,7 +127,7 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) { beforeErrCount = testutils.GetErrMetricCount(t, ingressSecretProviderControllerName) beforeRequestCount = testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess) _, err = i.Reconcile(ctx, req) - require.Errorf(t, err, fmt.Sprintf("determining if ingress is managed: %w", "ingressClassNameError")) + require.Errorf(t, err, fmt.Sprintf("determining if ingress is managed: %s", "ingressClassNameError")) require.Greater(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount) require.Equal(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount) From affa015480efb7499545befc34e45088380e7278 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Tue, 26 Mar 2024 15:01:53 -0400 Subject: [PATCH 25/36] Error() and ToPtr fix --- pkg/controller/keyvault/ingress_secret_provider_class.go | 2 +- pkg/controller/keyvault/nginx_secret_provider_class.go | 4 ++-- pkg/controller/keyvault/nginx_secret_provider_class_test.go | 2 +- pkg/controller/keyvault/placeholder_pod_test.go | 6 +++--- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index cfb08013..f566bd25 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -120,7 +120,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re if upsertSPC { logger.Info("reconciling secret provider class for ingress") if err = util.Upsert(ctx, i.client, spc); err != nil { - i.events.Eventf(ing, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err) + i.events.Eventf(ing, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err.Error()) } return result, err } diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index da715d9e..d20e2468 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -87,7 +87,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, - Controller: util.BoolPtr(true), + Controller: util.ToPtr(true), Kind: nic.Kind, Name: nic.Name, UID: nic.UID, @@ -110,7 +110,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req logger.Info("reconciling secret provider class for ingress") err = util.Upsert(ctx, i.client, spc) if err != nil { - i.events.Eventf(nic, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err) + i.events.Eventf(nic, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err.Error()) } return result, err } else { diff --git a/pkg/controller/keyvault/nginx_secret_provider_class_test.go b/pkg/controller/keyvault/nginx_secret_provider_class_test.go index f91b2f7a..864c8848 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class_test.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class_test.go @@ -194,7 +194,7 @@ func TestNginxSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testin Labels: manifests.GetTopLevelLabels(), OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, - Controller: util.BoolPtr(true), + Controller: util.ToPtr(true), Kind: nic.Kind, Name: nic.Name, UID: nic.UID, diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 34f0d54f..90246517 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -299,7 +299,7 @@ func TestPlaceholderPodControllerIntegrationWithNic(t *testing.T) { }, }, Spec: *manifests.WithPreferSystemNodes(&corev1.PodSpec{ - AutomountServiceAccountToken: util.BoolPtr(false), + AutomountServiceAccountToken: util.ToPtr(false), Containers: []corev1.Container{{ Name: "placeholder", Image: "test-registry/oss/kubernetes/pause:3.6-hotfix.20220114", @@ -320,7 +320,7 @@ func TestPlaceholderPodControllerIntegrationWithNic(t *testing.T) { VolumeSource: corev1.VolumeSource{ CSI: &corev1.CSIVolumeSource{ Driver: "secrets-store.csi.k8s.io", - ReadOnly: util.BoolPtr(true), + ReadOnly: util.ToPtr(true), VolumeAttributes: map[string]string{"secretProviderClass": spc.Name}, }, }, @@ -690,7 +690,7 @@ func getDefaultNginxSpc(nic *v1alpha1.NginxIngressController) *secv1.SecretProvi Generation: 123, OwnerReferences: []metav1.OwnerReference{{ APIVersion: nic.APIVersion, - Controller: util.BoolPtr(true), + Controller: util.ToPtr(true), Kind: "NginxIngressController", Name: nic.Name, UID: nic.UID, From 53cb9e89fae62c8e19159e3ef55011bef04516fa Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Tue, 26 Mar 2024 15:59:37 -0400 Subject: [PATCH 26/36] UserError function and better error checking --- .../keyvault/ingress_secret_provider_class.go | 5 ++-- pkg/controller/keyvault/kv_util.go | 27 +++++++++++++++---- pkg/controller/keyvault/kv_util_test.go | 11 ++++++++ .../keyvault/nginx_secret_provider_class.go | 5 ++-- 4 files changed, 39 insertions(+), 9 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index f566bd25..93375c70 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -5,6 +5,7 @@ package keyvault import ( "context" + "errors" "fmt" "github.com/Azure/aks-app-routing-operator/pkg/config" @@ -109,9 +110,9 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re var upsertSPC bool if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { - if userErr := err.(buildSPCUserError); userErr != nil { + if errors.As(err, &buildSPCUserError{}) { logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") - i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.Error()) + i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err.Error()) return result, nil } return result, err diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go index 86fd7613..56d72764 100644 --- a/pkg/controller/keyvault/kv_util.go +++ b/pkg/controller/keyvault/kv_util.go @@ -43,12 +43,12 @@ func buildSPC(obj client.Object, spc *secv1.SecretProviderClass, config *config. uri, err := url.Parse(certURI) if err != nil { - return false, buildSPCUserError(err) + return false, newBuildSPCUserError(err.Error()) } vaultName := strings.Split(uri.Host, ".")[0] chunks := strings.Split(uri.Path, "/") if len(chunks) < 3 { - return false, buildSPCUserError(fmt.Errorf("invalid secret uri: %s", certURI)) + return false, newBuildSPCUserError(fmt.Sprintf("invalid secret uri: %s", certURI)) } secretName := chunks[2] p := map[string]interface{}{ @@ -114,10 +114,27 @@ func DefaultNginxCertName(nic *v1alpha1.NginxIngressController) string { return certName } -type buildSPCUserError interface { +func certSecretName(ingressName string) string { + return fmt.Sprintf("keyvault-%s", ingressName) +} + +type userError interface { error + UserError() string } -func certSecretName(ingressName string) string { - return fmt.Sprintf("keyvault-%s", ingressName) +type buildSPCUserError struct { + errMessage string +} + +func (b buildSPCUserError) Error() string { + return b.UserError() +} + +func (b buildSPCUserError) UserError() string { + return b.errMessage +} + +func newBuildSPCUserError(msg string) buildSPCUserError { + return buildSPCUserError{msg} } diff --git a/pkg/controller/keyvault/kv_util_test.go b/pkg/controller/keyvault/kv_util_test.go index 8002a861..fac99821 100644 --- a/pkg/controller/keyvault/kv_util_test.go +++ b/pkg/controller/keyvault/kv_util_test.go @@ -1,6 +1,7 @@ package keyvault import ( + "errors" "fmt" "github.com/Azure/aks-app-routing-operator/api/v1alpha1" kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types" @@ -206,3 +207,13 @@ func TestBuildSPCWithWrongObject(t *testing.T) { assert.False(t, ok) require.EqualError(t, err, fmt.Sprintf("incorrect object type: %s", obj)) } + +func TestUserErrors(t *testing.T) { + testMsg := "test error message" + testError := newBuildSPCUserError(testMsg) + + assert.True(t, testError.UserError() == testMsg) + assert.True(t, testError.UserError() == testError.Error()) + assert.True(t, errors.As(testError, &buildSPCUserError{})) + assert.True(t, errors.As(testError, &buildSPCUserError{})) +} diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index d20e2468..5933f203 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -5,6 +5,7 @@ package keyvault import ( "context" + "errors" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "github.com/go-logr/logr" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -99,8 +100,8 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req upsertSPC, err := buildSPC(nic, spc, i.config) if err != nil { - if userErr := err.(buildSPCUserError); userErr != nil { - i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.Error()) + if errors.As(err, &buildSPCUserError{}) { + i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err.Error()) return result, nil } return result, err From 4bc5095c901279d3f68739e0119b06789c424723 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 12:15:41 -0400 Subject: [PATCH 27/36] changes for UserError --- .../keyvault/ingress_secret_provider_class.go | 4 ++-- pkg/controller/keyvault/kv_util.go | 23 +++++++++++++------ pkg/controller/keyvault/kv_util_test.go | 9 ++++---- .../keyvault/nginx_secret_provider_class.go | 4 ++-- 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index 93375c70..a162d932 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -110,9 +110,9 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re var upsertSPC bool if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { - if errors.As(err, &buildSPCUserError{}) { + if errors.As(err, &UserError) { logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") - i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err.Error()) + i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", UserError.UserError()) return result, nil } return result, err diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go index 56d72764..a3f7304e 100644 --- a/pkg/controller/keyvault/kv_util.go +++ b/pkg/controller/keyvault/kv_util.go @@ -43,12 +43,13 @@ func buildSPC(obj client.Object, spc *secv1.SecretProviderClass, config *config. uri, err := url.Parse(certURI) if err != nil { - return false, newBuildSPCUserError(err.Error()) + return false, newBuildSPCUserError(err, fmt.Sprintf("unable to parse certificate uri: %s", certURI)) } vaultName := strings.Split(uri.Host, ".")[0] chunks := strings.Split(uri.Path, "/") + if len(chunks) < 3 { - return false, newBuildSPCUserError(fmt.Sprintf("invalid secret uri: %s", certURI)) + return false, newBuildSPCUserError(fmt.Errorf("uri Path contains too few segments: has: %d requires greater than: %d uri path: %s", len(chunks), 3, uri.Path), fmt.Sprintf("invalid secret uri: %s", certURI)) } secretName := chunks[2] p := map[string]interface{}{ @@ -124,17 +125,25 @@ type userError interface { } type buildSPCUserError struct { - errMessage string + err error + userMessage string } +// for internal use func (b buildSPCUserError) Error() string { - return b.UserError() + return b.err.Error() } +// for user facing messages func (b buildSPCUserError) UserError() string { - return b.errMessage + return b.userMessage } -func newBuildSPCUserError(msg string) buildSPCUserError { - return buildSPCUserError{msg} +func newBuildSPCUserError(err error, msg string) buildSPCUserError { + return buildSPCUserError{err, msg} } + +var ( + UserError userError + BuildSPCUserError buildSPCUserError +) diff --git a/pkg/controller/keyvault/kv_util_test.go b/pkg/controller/keyvault/kv_util_test.go index fac99821..2c3d9c33 100644 --- a/pkg/controller/keyvault/kv_util_test.go +++ b/pkg/controller/keyvault/kv_util_test.go @@ -196,7 +196,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) { ok, err := buildSPC(ing, &secv1.SecretProviderClass{}, spcTestDefaultConf) assert.False(t, ok) - require.EqualError(t, err, "invalid secret uri: http://test.com/foo") + require.EqualError(t, err, "uri Path contains too few segments: has: 2 requires greater than: 3 uri path: /foo") }) } @@ -210,10 +210,9 @@ func TestBuildSPCWithWrongObject(t *testing.T) { func TestUserErrors(t *testing.T) { testMsg := "test error message" - testError := newBuildSPCUserError(testMsg) + testError := newBuildSPCUserError(errors.New("test"), testMsg) assert.True(t, testError.UserError() == testMsg) - assert.True(t, testError.UserError() == testError.Error()) - assert.True(t, errors.As(testError, &buildSPCUserError{})) - assert.True(t, errors.As(testError, &buildSPCUserError{})) + assert.True(t, errors.As(testError, &UserError)) + assert.True(t, errors.As(testError, &BuildSPCUserError)) } diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 5933f203..e6be1ebe 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -100,8 +100,8 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req upsertSPC, err := buildSPC(nic, spc, i.config) if err != nil { - if errors.As(err, &buildSPCUserError{}) { - i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", err.Error()) + if errors.As(err, &UserError) { + i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", UserError.UserError()) return result, nil } return result, err From 35879802fb1f093942e716affc9166253329c8ac Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 14:40:41 -0400 Subject: [PATCH 28/36] addressing comments --- pkg/controller/keyvault/ingress_secret_provider_class.go | 7 ++++--- pkg/controller/keyvault/kv_util.go | 5 ----- pkg/controller/keyvault/nginx_secret_provider_class.go | 7 +++++-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index a162d932..c74730e7 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -110,9 +110,10 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re var upsertSPC bool if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { - if errors.As(err, &UserError) { - logger.Info("failed to build secret provider class for ingress, user input invalid. sending warning event") - i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", UserError.UserError()) + var userErr userError + if errors.As(err, &userErr) { + logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event"), userErr.Error()) + i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } return result, err diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go index a3f7304e..60d99eb3 100644 --- a/pkg/controller/keyvault/kv_util.go +++ b/pkg/controller/keyvault/kv_util.go @@ -142,8 +142,3 @@ func (b buildSPCUserError) UserError() string { func newBuildSPCUserError(err error, msg string) buildSPCUserError { return buildSPCUserError{err, msg} } - -var ( - UserError userError - BuildSPCUserError buildSPCUserError -) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index e6be1ebe..8f412daf 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -6,6 +6,7 @@ package keyvault import ( "context" "errors" + "fmt" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "github.com/go-logr/logr" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -100,8 +101,10 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req upsertSPC, err := buildSPC(nic, spc, i.config) if err != nil { - if errors.As(err, &UserError) { - i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", UserError.UserError()) + var userErr userError + if errors.As(err, &userErr) { + logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event"), userErr.Error()) + i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } return result, err From e33cf0972530777a45973e41a9c1b60f01e88680 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 14:43:09 -0400 Subject: [PATCH 29/36] typo fix --- pkg/controller/keyvault/nginx_secret_provider_class.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 8f412daf..3b91bf2d 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -103,7 +103,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req if err != nil { var userErr userError if errors.As(err, &userErr) { - logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event"), userErr.Error()) + logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s. sending warning event"), userErr.Error()) i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } From edf6c6e76ca1fb3c3c10137313b58ef1f24929fd Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 14:45:11 -0400 Subject: [PATCH 30/36] test fixes --- pkg/controller/keyvault/kv_util_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/controller/keyvault/kv_util_test.go b/pkg/controller/keyvault/kv_util_test.go index 2c3d9c33..4c1514bc 100644 --- a/pkg/controller/keyvault/kv_util_test.go +++ b/pkg/controller/keyvault/kv_util_test.go @@ -211,8 +211,10 @@ func TestBuildSPCWithWrongObject(t *testing.T) { func TestUserErrors(t *testing.T) { testMsg := "test error message" testError := newBuildSPCUserError(errors.New("test"), testMsg) + var userErr userError + var buildSPCUserErr buildSPCUserError assert.True(t, testError.UserError() == testMsg) - assert.True(t, errors.As(testError, &UserError)) - assert.True(t, errors.As(testError, &BuildSPCUserError)) + assert.True(t, errors.As(testError, &userErr)) + assert.True(t, errors.As(testError, &buildSPCUserErr)) } From 97ca6291e11f47c265ff03b887860c697ae7be30 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 14:47:32 -0400 Subject: [PATCH 31/36] added logging for buildSPC error --- pkg/controller/keyvault/ingress_secret_provider_class.go | 2 ++ pkg/controller/keyvault/nginx_secret_provider_class.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index c74730e7..6833b7da 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -116,6 +116,8 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } + + logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s."), err.Error()) return result, err } diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 3b91bf2d..3ea04293 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -107,6 +107,8 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } + + logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s."), err.Error()) return result, err } From de6f98614d218677693ad03f14f996376a1d7068 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 14:55:23 -0400 Subject: [PATCH 32/36] Added logging --- pkg/controller/keyvault/ingress_secret_provider_class.go | 6 ++++-- pkg/controller/keyvault/nginx_secret_provider_class.go | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index 6833b7da..df77e307 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -103,6 +103,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re // Checking if we manage the ingress. All false cases without an error are assumed that we don't manage it var isManaged bool if isManaged, err = i.ingressManager.IsManaging(ing); err != nil { + logger.Info(fmt.Sprintf("failed while checking if ingress was managed with error: %s.", err.Error())) return result, fmt.Errorf("determining if ingress is managed: %w", err) } @@ -112,12 +113,12 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { var userErr userError if errors.As(err, &userErr) { - logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event"), userErr.Error()) + logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event", userErr.Error())) i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } - logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s."), err.Error()) + logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s.", err.Error())) return result, err } @@ -125,6 +126,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re logger.Info("reconciling secret provider class for ingress") if err = util.Upsert(ctx, i.client, spc); err != nil { i.events.Eventf(ing, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err.Error()) + logger.Info(fmt.Sprintf("failed to upsert secret provider class for ingress with error: %s.", err.Error())) } return result, err } diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 3ea04293..d06e719d 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -103,12 +103,12 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req if err != nil { var userErr userError if errors.As(err, &userErr) { - logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s. sending warning event"), userErr.Error()) + logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s. sending warning event", userErr.Error())) i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } - logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s."), err.Error()) + logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s.", err.Error())) return result, err } @@ -117,6 +117,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req err = util.Upsert(ctx, i.client, spc) if err != nil { i.events.Eventf(nic, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err.Error()) + logger.Info(fmt.Sprintf("failed to upsert secret provider class for nginx ingress class with error: %s.", err.Error())) } return result, err } else { From d16cefc9a36e824a33944b74e1b5c9663c67104b Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 15:39:50 -0400 Subject: [PATCH 33/36] Error level logs --- pkg/controller/keyvault/ingress_secret_provider_class.go | 6 +++--- pkg/controller/keyvault/nginx_secret_provider_class.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index df77e307..d017b685 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -103,7 +103,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re // Checking if we manage the ingress. All false cases without an error are assumed that we don't manage it var isManaged bool if isManaged, err = i.ingressManager.IsManaging(ing); err != nil { - logger.Info(fmt.Sprintf("failed while checking if ingress was managed with error: %s.", err.Error())) + logger.Error(err, fmt.Sprintf("failed while checking if ingress was managed with error: %s.", err.Error())) return result, fmt.Errorf("determining if ingress is managed: %w", err) } @@ -113,7 +113,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { var userErr userError if errors.As(err, &userErr) { - logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event", userErr.Error())) + logger.Error(err, fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event", userErr.Error())) i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } @@ -126,7 +126,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re logger.Info("reconciling secret provider class for ingress") if err = util.Upsert(ctx, i.client, spc); err != nil { i.events.Eventf(ing, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err.Error()) - logger.Info(fmt.Sprintf("failed to upsert secret provider class for ingress with error: %s.", err.Error())) + logger.Error(err, fmt.Sprintf("failed to upsert secret provider class for ingress with error: %s.", err.Error())) } return result, err } diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index d06e719d..6e6b2fd9 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -103,7 +103,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req if err != nil { var userErr userError if errors.As(err, &userErr) { - logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s. sending warning event", userErr.Error())) + logger.Error(err, fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s. sending warning event", userErr.Error())) i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } @@ -117,7 +117,7 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req err = util.Upsert(ctx, i.client, spc) if err != nil { i.events.Eventf(nic, "Warning", "FailedUpdateOrCreateSPC", "error while creating or updating SecretProviderClass needed to pull Keyvault reference: %s", err.Error()) - logger.Info(fmt.Sprintf("failed to upsert secret provider class for nginx ingress class with error: %s.", err.Error())) + logger.Error(err, fmt.Sprintf("failed to upsert secret provider class for nginx ingress class with error: %s.", err.Error())) } return result, err } else { From ee2f6204a6c11a6cd66264d6a691101a1d28464f Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 15:54:56 -0400 Subject: [PATCH 34/36] Addressing comments --- .../keyvault/ingress_secret_provider_class.go | 4 ++-- pkg/controller/keyvault/kv_util.go | 19 +++++++------------ pkg/controller/keyvault/kv_util_test.go | 4 +--- .../keyvault/nginx_secret_provider_class.go | 4 ++-- 4 files changed, 12 insertions(+), 19 deletions(-) diff --git a/pkg/controller/keyvault/ingress_secret_provider_class.go b/pkg/controller/keyvault/ingress_secret_provider_class.go index d017b685..3edb82bc 100644 --- a/pkg/controller/keyvault/ingress_secret_provider_class.go +++ b/pkg/controller/keyvault/ingress_secret_provider_class.go @@ -113,12 +113,12 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re if upsertSPC, err = buildSPC(ing, spc, i.config); err != nil { var userErr userError if errors.As(err, &userErr) { - logger.Error(err, fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event", userErr.Error())) + logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s. sending warning event", userErr.Error())) i.events.Eventf(ing, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } - logger.Info(fmt.Sprintf("failed to build secret provider class for ingress with error: %s.", err.Error())) + logger.Error(err, fmt.Sprintf("failed to build secret provider class for ingress with error: %s.", err.Error())) return result, err } diff --git a/pkg/controller/keyvault/kv_util.go b/pkg/controller/keyvault/kv_util.go index 60d99eb3..48125bbb 100644 --- a/pkg/controller/keyvault/kv_util.go +++ b/pkg/controller/keyvault/kv_util.go @@ -43,13 +43,13 @@ func buildSPC(obj client.Object, spc *secv1.SecretProviderClass, config *config. uri, err := url.Parse(certURI) if err != nil { - return false, newBuildSPCUserError(err, fmt.Sprintf("unable to parse certificate uri: %s", certURI)) + return false, newUserError(err, fmt.Sprintf("unable to parse certificate uri: %s", certURI)) } vaultName := strings.Split(uri.Host, ".")[0] chunks := strings.Split(uri.Path, "/") if len(chunks) < 3 { - return false, newBuildSPCUserError(fmt.Errorf("uri Path contains too few segments: has: %d requires greater than: %d uri path: %s", len(chunks), 3, uri.Path), fmt.Sprintf("invalid secret uri: %s", certURI)) + return false, newUserError(fmt.Errorf("uri Path contains too few segments: has: %d requires greater than: %d uri path: %s", len(chunks), 3, uri.Path), fmt.Sprintf("invalid secret uri: %s", certURI)) } secretName := chunks[2] p := map[string]interface{}{ @@ -119,26 +119,21 @@ func certSecretName(ingressName string) string { return fmt.Sprintf("keyvault-%s", ingressName) } -type userError interface { - error - UserError() string -} - -type buildSPCUserError struct { +type userError struct { err error userMessage string } // for internal use -func (b buildSPCUserError) Error() string { +func (b userError) Error() string { return b.err.Error() } // for user facing messages -func (b buildSPCUserError) UserError() string { +func (b userError) UserError() string { return b.userMessage } -func newBuildSPCUserError(err error, msg string) buildSPCUserError { - return buildSPCUserError{err, msg} +func newUserError(err error, msg string) userError { + return userError{err, msg} } diff --git a/pkg/controller/keyvault/kv_util_test.go b/pkg/controller/keyvault/kv_util_test.go index 4c1514bc..3ee1d023 100644 --- a/pkg/controller/keyvault/kv_util_test.go +++ b/pkg/controller/keyvault/kv_util_test.go @@ -210,11 +210,9 @@ func TestBuildSPCWithWrongObject(t *testing.T) { func TestUserErrors(t *testing.T) { testMsg := "test error message" - testError := newBuildSPCUserError(errors.New("test"), testMsg) + testError := newUserError(errors.New("test"), testMsg) var userErr userError - var buildSPCUserErr buildSPCUserError assert.True(t, testError.UserError() == testMsg) assert.True(t, errors.As(testError, &userErr)) - assert.True(t, errors.As(testError, &buildSPCUserErr)) } diff --git a/pkg/controller/keyvault/nginx_secret_provider_class.go b/pkg/controller/keyvault/nginx_secret_provider_class.go index 6e6b2fd9..466718fe 100644 --- a/pkg/controller/keyvault/nginx_secret_provider_class.go +++ b/pkg/controller/keyvault/nginx_secret_provider_class.go @@ -103,12 +103,12 @@ func (i *NginxSecretProviderClassReconciler) Reconcile(ctx context.Context, req if err != nil { var userErr userError if errors.As(err, &userErr) { - logger.Error(err, fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s. sending warning event", userErr.Error())) + logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s. sending warning event", userErr.Error())) i.events.Eventf(nic, "Warning", "InvalidInput", "error while processing Keyvault reference: %s", userErr.UserError()) return result, nil } - logger.Info(fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s.", err.Error())) + logger.Error(err, fmt.Sprintf("failed to build secret provider class for nginx ingress controller with error: %s.", err.Error())) return result, err } From dc90a06ad6545360c65015169c33fe1fec1635cc Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 16:03:47 -0400 Subject: [PATCH 35/36] moved logging for placeholder_pod to switch cases --- pkg/controller/keyvault/placeholder_pod.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 62d7a888..7e4bcc46 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -142,17 +142,15 @@ func (p *PlaceholderPodController) reconcileObjectDeployment(dep *appsv1.Deploym case util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController") != "": obj = &v1alpha1.NginxIngressController{} obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "NginxIngressController")) + logger.Info(fmt.Sprint("getting owner NginxIngressController")) case util.FindOwnerKind(spc.OwnerReferences, "Ingress") != "": obj = &netv1.Ingress{} obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "Ingress")) obj.SetNamespace(req.Namespace) - default: - p.events.Eventf(obj, "Waring", "FailedReconcileObjectDeployment", "failed to reconcile object deployment: object type not nginxIngressController or ingress") - return result, nil + logger.Info(fmt.Sprint("getting owner Ingress")) } if obj.GetName() != "" { - logger.Info(fmt.Sprintf("getting owner %s", obj.GetObjectKind().GroupVersionKind().Kind)) if err = p.client.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { return result, client.IgnoreNotFound(err) } From 3e66697779f89f597695b96c9b7d56a8c93df789 Mon Sep 17 00:00:00 2001 From: Asa Gayle Date: Wed, 27 Mar 2024 16:09:58 -0400 Subject: [PATCH 36/36] default case for placeholder_pod --- pkg/controller/keyvault/placeholder_pod.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 7e4bcc46..0001ca87 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -148,6 +148,9 @@ func (p *PlaceholderPodController) reconcileObjectDeployment(dep *appsv1.Deploym obj.SetName(util.FindOwnerKind(spc.OwnerReferences, "Ingress")) obj.SetNamespace(req.Namespace) logger.Info(fmt.Sprint("getting owner Ingress")) + default: + logger.Info("owner type not found") + return result, nil } if obj.GetName() != "" {