From 1f0e14ade7965deef23ef2241eadbb97785d2deb Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 22:28:32 -0500 Subject: [PATCH 1/7] add default nic controller --- pkg/controller/controller.go | 34 ++++++-- pkg/controller/nginxingress/default.go | 77 +++++++++++++++++++ .../nginxingress/nginx_ingress_controller.go | 40 ++++++++-- pkg/webhook/nginxingress.go | 8 +- 4 files changed, 141 insertions(+), 18 deletions(-) create mode 100644 pkg/controller/nginxingress/default.go diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index dc5f4847..761aaeaa 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -115,9 +115,12 @@ func NewManagerForRestConfig(conf *config.Config, rc *rest.Config) (ctrl.Manager return nil, fmt.Errorf("loading CRDs: %w", err) } - if err := setupControllers(m, conf); err != nil { - return nil, fmt.Errorf("setting up controllers: %w", err) - } + go func() { + if err := setupControllers(m, conf, webhooksReady, setupLog); err != nil { + setupLog.Error(err, "unable to setup controllers") + os.Exit(1) + } + }() webhookCfg, err := webhook.New(conf) if err != nil { @@ -147,8 +150,8 @@ func NewManagerForRestConfig(conf *config.Config, rc *rest.Config) (ctrl.Manager setupLog.Error(err, "failed to setup webhooks") os.Exit(1) } - setupLog.Info("webhooks are ready") + setupLog.Info("webhooks are ready") close(webhooksReady) }() @@ -163,18 +166,27 @@ func setupWebhooks(mgr ctrl.Manager, addWebhooksFn func(mgr ctrl.Manager) error) return nil } -func setupControllers(mgr ctrl.Manager, conf *config.Config) error { - var selfDeploy *appsv1.Deployment = nil // self deploy doesn't work because operator isn't in same resources as child resources - +func setupControllers(mgr ctrl.Manager, conf *config.Config, webhooksReady <-chan struct{}, lgr logr.Logger) error { + lgr.Info("settup up ExternalDNS controller") if err := dns.NewExternalDns(mgr, conf); err != nil { return fmt.Errorf("setting up external dns controller: %w", err) } + lgr.Info("setting up Nginx Ingress Controller reconciler") if err := nginxingress.NewReconciler(conf, mgr); err != nil { return fmt.Errorf("setting up nginx ingress controller reconciler: %w", err) } - nginxConfigs, err := nginx.New(mgr, conf, selfDeploy) + lgr.Info("waiting for webhooks to be ready") + <-webhooksReady // some controllers need to wait for webhooks to be ready because they interact directly with our CRDs + lgr.Info("finished waiting for webhooks to be ready") + + lgr.Info("setting up default Nginx Ingress Controller reconciler") + if err := nginxingress.NewDefaultReconciler(mgr); err != nil { + return fmt.Errorf("setting up nginx ingress default controller reconciler: %w", err) + } + + nginxConfigs, err := nginx.New(mgr, conf, nil) if err != nil { return fmt.Errorf("getting nginx configs: %w", err) } @@ -187,6 +199,7 @@ func setupControllers(mgr ctrl.Manager, conf *config.Config) error { }) } + lgr.Info("setting up ingress concurrency watchdog") if err := ingress.NewConcurrencyWatchdog(mgr, conf, watchdogTargets); err != nil { return fmt.Errorf("setting up ingress concurrency watchdog: %w", err) } @@ -201,20 +214,25 @@ func setupControllers(mgr ctrl.Manager, conf *config.Config) error { } ingressManager := keyvault.NewIngressManager(ics) + lgr.Info("setting up keyvault secret provider class reconciler") 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 keyvault placeholder pod controller") if err := keyvault.NewPlaceholderPodController(mgr, conf, ingressManager); err != nil { return fmt.Errorf("setting up placeholder pod controller: %w", err) } + lgr.Info("setting up keyvault event mirror") if err = keyvault.NewEventMirror(mgr, conf); err != nil { return fmt.Errorf("setting up event mirror: %w", err) } ingressControllerNamer := osm.NewIngressControllerNamer(icToController) + lgr.Info("setting up ingress backend reconciler") if err := osm.NewIngressBackendReconciler(mgr, conf, ingressControllerNamer); err != nil { return fmt.Errorf("setting up ingress backend reconciler: %w", err) } + lgr.Info("setting up ingress cert config reconciler") if err = osm.NewIngressCertConfigReconciler(mgr, conf); err != nil { return fmt.Errorf("setting up ingress cert config reconciler: %w", err) } diff --git a/pkg/controller/nginxingress/default.go b/pkg/controller/nginxingress/default.go new file mode 100644 index 00000000..eb2628a2 --- /dev/null +++ b/pkg/controller/nginxingress/default.go @@ -0,0 +1,77 @@ +package nginxingress + +import ( + "context" + "fmt" + "time" + + approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/controller/common" + "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" + netv1 "k8s.io/api/networking/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + // DefaultcName is the default Ingress class name + DefaultIcName = "webapprouting.kubernetes.azure.com" + // DefaultNicName is the default Nginx Ingress Controller resource name + DefaultNicName = "default" + DefaultNicResourceName = "nginx" + reconcileInterval = time.Minute * 3 +) + +func NewDefaultReconciler(mgr ctrl.Manager) error { + nic := &approutingv1alpha1.NginxIngressController{ + TypeMeta: metav1.TypeMeta{ + APIVersion: approutingv1alpha1.GroupVersion.String(), + Kind: "NginxIngressController", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNicName, + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + ControllerNamePrefix: "nginx", + IngressClassName: DefaultIcName, + }, + Status: approutingv1alpha1.NginxIngressControllerStatus{}, + } + + if err := common.NewResourceReconciler(mgr, controllername.New("default", "nginx", "ingress", "controller", "reconciler"), []client.Object{nic}, reconcileInterval); err != nil { + return fmt.Errorf("creating default nginx ingress controller: %w", err) + } + + return nil +} + +func getDefaultIngressClassControllerClass(cl client.Client) (string, error) { + defaultNicCc := "webapprouting.kubernetes.azure.com/nginx" + + defaultIc := &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultIcName, + }, + } + err := cl.Get(context.Background(), types.NamespacedName{Name: DefaultIcName}, defaultIc) + if err == nil { + defaultNicCc = defaultIc.Spec.Controller + } + if err != nil && !k8serrors.IsNotFound(err) { + return "", fmt.Errorf("getting default ingress class: %w", err) + } + + return defaultNicCc, nil +} + +// IsDefaultNic returns true if the given NginxIngressController is the default one +func IsDefaultNic(nic *approutingv1alpha1.NginxIngressController) bool { + if nic == nil { + return false + } + + return nic.Name == DefaultNicName && nic.Spec.IngressClassName == DefaultIcName +} diff --git a/pkg/controller/nginxingress/nginx_ingress_controller.go b/pkg/controller/nginxingress/nginx_ingress_controller.go index a5166ef4..3462703e 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller.go @@ -47,19 +47,32 @@ var ( // nginxIngressControllerReconciler reconciles a NginxIngressController object type nginxIngressControllerReconciler struct { - client client.Client - conf *config.Config - events record.EventRecorder + client client.Client + conf *config.Config + events record.EventRecorder + defaultNicControllerClass string } // NewReconciler sets up the controller with the Manager. func NewReconciler(conf *config.Config, mgr ctrl.Manager) error { metrics.InitControllerMetrics(nginxIngressControllerReconcilerName) + // use non caching client since it's before manager is started + nonCachingCl, err := client.New(mgr.GetConfig(), client.Options{}) + if err != nil { + return fmt.Errorf("creating non caching client: %w", err) + } + + defaultNicCc, err := getDefaultIngressClassControllerClass(nonCachingCl) + if err != nil { + return fmt.Errorf("getting default ingress class controller class: %w", err) + } + reconciler := &nginxIngressControllerReconciler{ - client: mgr.GetClient(), - conf: conf, - events: mgr.GetEventRecorderFor("aks-app-routing-operator"), + client: mgr.GetClient(), + conf: conf, + events: mgr.GetEventRecorderFor("aks-app-routing-operator"), + defaultNicControllerClass: defaultNicCc, } if err := nginxIngressControllerReconcilerName.AddToController( @@ -198,15 +211,21 @@ func (n *nginxIngressControllerReconciler) ManagedResources(nic *approutingv1alp } cc := "webapprouting.kubernetes.azure.com/nginx/" + url.PathEscape(nic.Name) - // it's impossible for this to happen because we enforce nic.Name to be less than 101 characters in validating webhooks if len(cc) > controllerClassMaxLen { cc = cc[:controllerClassMaxLen] } + resourceName := fmt.Sprintf("%s-%d", nic.Spec.ControllerNamePrefix, nic.Status.CollisionCount) + + if IsDefaultNic(nic) { + cc = n.defaultNicControllerClass + resourceName = DefaultNicResourceName + } + nginxIngressCfg := &manifests.NginxIngressConfig{ ControllerClass: cc, - ResourceName: fmt.Sprintf("%s-%d", nic.Spec.ControllerNamePrefix, nic.Status.CollisionCount), + ResourceName: resourceName, IcName: nic.Spec.IngressClassName, ServiceConfig: &manifests.ServiceConfig{ Annotations: nic.Spec.LoadBalancerAnnotations, @@ -267,6 +286,11 @@ func (n *nginxIngressControllerReconciler) GetCollisionCount(ctx context.Context func (n *nginxIngressControllerReconciler) collides(ctx context.Context, nic *approutingv1alpha1.NginxIngressController) (collision, error) { lgr := log.FromContext(ctx) + // ignore any collisions on default nic for migration story. Need to acquire ownership of old app routing resources + if IsDefaultNic(nic) { + return collisionNone, nil + } + res := n.ManagedResources(nic) if res == nil { return collisionNone, fmt.Errorf("getting managed objects") diff --git a/pkg/webhook/nginxingress.go b/pkg/webhook/nginxingress.go index 66ecd2eb..7ab35ca0 100644 --- a/pkg/webhook/nginxingress.go +++ b/pkg/webhook/nginxingress.go @@ -9,6 +9,7 @@ import ( approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "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/controller/nginxingress" "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/Azure/aks-app-routing-operator/pkg/util" "github.com/go-logr/logr" @@ -202,9 +203,12 @@ func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admissio return admission.Denied(invalidReason) } - // TODO: record metrics, will add later - if req.Operation == admissionv1.Create { + if nginxingress.IsDefaultNic(&nginxIngressController) { + // need to allow for ic to exist already for the default migration case (migrating from non-crd versions of app routing to crd versions) + return admission.Allowed("") + } + lgr.Info("checking if IngressClass already exists") ic := &netv1.IngressClass{ ObjectMeta: metav1.ObjectMeta{ From fc6f14cc812f85b8e4c18e67bb7684168620616f Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 23:00:41 -0500 Subject: [PATCH 2/7] add some unit tests --- pkg/controller/nginxingress/default_test.go | 104 ++++++++++++++++++++ pkg/webhook/nginxingress_test.go | 44 +++++++++ 2 files changed, 148 insertions(+) create mode 100644 pkg/controller/nginxingress/default_test.go diff --git a/pkg/controller/nginxingress/default_test.go b/pkg/controller/nginxingress/default_test.go new file mode 100644 index 00000000..a5fde36f --- /dev/null +++ b/pkg/controller/nginxingress/default_test.go @@ -0,0 +1,104 @@ +package nginxingress + +import ( + "context" + "testing" + + approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/stretchr/testify/require" + netv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestGetDefaultIngressClassControllerClass(t *testing.T) { + cl := fake.NewClientBuilder().Build() + + // when default IngressClass doesn't exist in cluster it defaults to webapprouting.kubernetes.azure.com/nginx + cc, err := getDefaultIngressClassControllerClass(cl) + require.NoError(t, err) + require.Equal(t, "webapprouting.kubernetes.azure.com/nginx", cc) + + // when IngressClass exists in cluster we take whatever is in the Spec.Controller field + controller := "controllerField" + ic := &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultIcName, + }, + Spec: netv1.IngressClassSpec{ + Controller: controller, + }, + } + require.NoError(t, cl.Create(context.Background(), ic)) + cc, err = getDefaultIngressClassControllerClass(cl) + require.NoError(t, err) + require.Equal(t, controller, cc) +} + +func TestIsDefaultNic(t *testing.T) { + cases := []struct { + Name string + Nic *approutingv1alpha1.NginxIngressController + Expected bool + }{ + { + Name: "nil nic", + Nic: nil, + Expected: false, + }, + { + Name: "default name, default IngressClassName", + Nic: &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNicName, + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: DefaultIcName, + }, + }, + Expected: true, + }, + { + Name: "default name, non default IngressClassName", + Nic: &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNicName, + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "non-default", + }, + }, + Expected: false, + }, + { + Name: "non default name, default IngressClassName", + Nic: &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-default", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: DefaultIcName, + }, + }, + Expected: false, + }, + { + Name: "non default name, non default IngressClassName", + Nic: &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-default", + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: "non-default", + }, + }, + Expected: false, + }, + } + + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + require.Equal(t, c.Expected, IsDefaultNic(c.Nic)) + }) + } +} \ No newline at end of file diff --git a/pkg/webhook/nginxingress_test.go b/pkg/webhook/nginxingress_test.go index 35ac360d..b41c5101 100644 --- a/pkg/webhook/nginxingress_test.go +++ b/pkg/webhook/nginxingress_test.go @@ -12,6 +12,7 @@ import ( approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" + "github.com/Azure/aks-app-routing-operator/pkg/controller/nginxingress" "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" "github.com/go-logr/logr" "github.com/stretchr/testify/require" @@ -91,6 +92,13 @@ func TestNginxIngressResourceValidator(t *testing.T) { } require.NoError(t, cl.Create(context.Background(), existingNic)) + defaultIc := &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: nginxingress.DefaultIcName, + }, + } + require.NoError(t, cl.Create(context.Background(), defaultIc)) + cases := []struct { name string req admission.Request @@ -238,6 +246,42 @@ func TestNginxIngressResourceValidator(t *testing.T) { authenticator: validUser, expected: admission.Allowed(""), }, + { + name: "valid nginx ingress controller, valid user, default nic", + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { + copy := validNginxIngressController.DeepCopy() + copy.Spec.IngressClassName = nginxingress.DefaultIcName + copy.Name = nginxingress.DefaultNicName + return copy + }()), + }, + }, + }, + authenticator: validUser, + expected: admission.Allowed(""), + }, + { + name: "valid nginx ingress controller, invalid user, default nic", + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: toRaw(func() *approutingv1alpha1.NginxIngressController { + copy := validNginxIngressController.DeepCopy() + copy.Spec.IngressClassName = nginxingress.DefaultIcName + copy.Name = nginxingress.DefaultNicName + return copy + }()), + }, + }, + }, + authenticator: invalidUser, + expected: admission.Denied("invalid user"), + }, } metrics.InitControllerMetrics(nginxResourceValidationName) From 67586c29e3384ab4553a5cd86aed39eb05abdd2b Mon Sep 17 00:00:00 2001 From: Oliver King Date: Fri, 3 Nov 2023 23:16:56 -0500 Subject: [PATCH 3/7] add more unit tests --- .../nginx_ingress_controller_test.go | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/pkg/controller/nginxingress/nginx_ingress_controller_test.go b/pkg/controller/nginxingress/nginx_ingress_controller_test.go index 44eaf76b..10b9e531 100644 --- a/pkg/controller/nginxingress/nginx_ingress_controller_test.go +++ b/pkg/controller/nginxingress/nginx_ingress_controller_test.go @@ -171,6 +171,36 @@ func TestManagedResources(t *testing.T) { require.Equal(t, v, resources.Service.Annotations[k]) } }) + + t.Run("default nic", func(t *testing.T) { + defaultNicControllerClass := "defaultNicControllerClass" + n := &nginxIngressControllerReconciler{ + conf: &config.Config{ + NS: "default", + }, + defaultNicControllerClass: defaultNicControllerClass, + } + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNicName, + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: DefaultIcName, + ControllerNamePrefix: "nginx", + }, + } + + resources := n.ManagedResources(nic) + require.NotNil(t, resources) + require.Equal(t, nic.Spec.IngressClassName, resources.IngressClass.Name) + require.Equal(t, defaultNicControllerClass, resources.IngressClass.Spec.Controller) + require.Equal(t, nic.Spec.ControllerNamePrefix, resources.Deployment.Name) + + // check that we are only putting owner references on managed resources + ownerRef := manifests.GetOwnerRefs(nic, true) + require.Equal(t, ownerRef, resources.Deployment.OwnerReferences) + require.NotEqual(t, ownerRef, resources.Namespace.OwnerReferences) + }) } func TestGetCollisionCount(t *testing.T) { @@ -307,6 +337,31 @@ func TestCollides(t *testing.T) { got, err = n.collides(ctx, nic3) require.NoError(t, err) require.Equal(t, collisionNone, got) + + // no collision for default nic + defaultNic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNicName, + }, + Spec: approutingv1alpha1.NginxIngressControllerSpec{ + IngressClassName: DefaultIcName, + }, + } + defaultIc := &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaultNic.Spec.IngressClassName, + }, + } + require.NoError(t, cl.Create(ctx, defaultIc)) + defaultSa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNicResourceName, + }, + } + require.NoError(t, cl.Create(ctx, defaultSa)) + got, err = n.collides(ctx, defaultNic) + require.NoError(t, err) + require.Equal(t, collisionNone, got) } func TestUpdateStatusManagedResourceRefs(t *testing.T) { From 6a78b7dc7d3db4112877336fcf610b41659d68d7 Mon Sep 17 00:00:00 2001 From: Oliver King Date: Sat, 4 Nov 2023 11:28:36 -0500 Subject: [PATCH 4/7] fix placeholder pod upgrade --- pkg/controller/keyvault/placeholder_pod.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index c0e5e28b..24dbae16 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -138,12 +138,13 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } func (p *PlaceholderPodController) buildDeployment(dep *appsv1.Deployment, spc *secv1.SecretProviderClass, ing *netv1.Ingress) { - labels := util.MergeMaps(map[string]string{"app": spc.Name}, manifests.GetTopLevelLabels()) + selectorLabels := map[string]string{"app": spc.Name} + labels := util.MergeMaps(selectorLabels, manifests.GetTopLevelLabels()) dep.Spec = appsv1.DeploymentSpec{ Replicas: util.Int32Ptr(1), RevisionHistoryLimit: util.Int32Ptr(2), - Selector: &metav1.LabelSelector{MatchLabels: labels}, + Selector: &metav1.LabelSelector{MatchLabels: selectorLabels}, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: labels, From b5e02872425a9a77fb1b6564b116eed0918f3fa5 Mon Sep 17 00:00:00 2001 From: Oliver King Date: Sat, 4 Nov 2023 15:42:42 -0500 Subject: [PATCH 5/7] swap default strategy to allow for default nic cleanup --- go.mod | 4 +- pkg/controller/keyvault/placeholder_pod.go | 6 +- .../keyvault/placeholder_pod_test.go | 7 +- pkg/controller/nginxingress/default.go | 72 +++++++++++++++++-- 4 files changed, 76 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index edd5b32e..cb673f70 100644 --- a/go.mod +++ b/go.mod @@ -16,11 +16,13 @@ require ( github.com/prometheus/common v0.44.0 github.com/stretchr/testify v1.8.4 go.uber.org/zap v1.25.0 + gomodules.xyz/jsonpatch/v2 v2.4.0 k8s.io/api v0.28.1 k8s.io/apiextensions-apiserver v0.28.1 k8s.io/apimachinery v0.28.1 k8s.io/client-go v0.28.1 k8s.io/klog/v2 v2.100.1 + k8s.io/utils v0.0.0-20230726121419-3b25d923346b sigs.k8s.io/controller-runtime v0.16.1 sigs.k8s.io/secrets-store-csi-driver v1.3.4 sigs.k8s.io/yaml v1.3.0 @@ -73,7 +75,6 @@ require ( golang.org/x/term v0.11.0 // indirect golang.org/x/text v0.12.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 @@ -81,7 +82,6 @@ require ( gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/component-base v0.28.1 // indirect k8s.io/kube-openapi v0.0.0-20230816210353-14e408962443 // indirect - k8s.io/utils v0.0.0-20230726121419-3b25d923346b // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.3.0 // indirect ) diff --git a/pkg/controller/keyvault/placeholder_pod.go b/pkg/controller/keyvault/placeholder_pod.go index 24dbae16..a6678d49 100644 --- a/pkg/controller/keyvault/placeholder_pod.go +++ b/pkg/controller/keyvault/placeholder_pod.go @@ -138,13 +138,11 @@ func (p *PlaceholderPodController) Reconcile(ctx context.Context, req ctrl.Reque } func (p *PlaceholderPodController) buildDeployment(dep *appsv1.Deployment, spc *secv1.SecretProviderClass, ing *netv1.Ingress) { - selectorLabels := map[string]string{"app": spc.Name} - labels := util.MergeMaps(selectorLabels, manifests.GetTopLevelLabels()) - + labels := map[string]string{"app": spc.Name} dep.Spec = appsv1.DeploymentSpec{ Replicas: util.Int32Ptr(1), RevisionHistoryLimit: util.Int32Ptr(2), - Selector: &metav1.LabelSelector{MatchLabels: selectorLabels}, + Selector: &metav1.LabelSelector{MatchLabels: labels}, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: labels, diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index d4a861ae..8be8b773 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -5,9 +5,10 @@ package keyvault import ( "context" - "k8s.io/apimachinery/pkg/api/errors" "testing" + "k8s.io/apimachinery/pkg/api/errors" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -92,7 +93,7 @@ func TestPlaceholderPodControllerIntegration(t *testing.T) { replicas := int32(1) historyLimit := int32(2) - expectedLabels := util.MergeMaps(spc.Labels, map[string]string{"app": spc.Name}) + expectedLabels := map[string]string{"app": spc.Name} expected := appsv1.DeploymentSpec{ Replicas: &replicas, RevisionHistoryLimit: &historyLimit, @@ -217,7 +218,7 @@ func TestPlaceholderPodControllerNoManagedByLabels(t *testing.T) { replicas := int32(1) historyLimit := int32(2) - expectedLabels := util.MergeMaps(map[string]string{"app": spc.Name}, manifests.GetTopLevelLabels()) + expectedLabels := .map[string]string{"app": spc.Name} expected := appsv1.DeploymentSpec{ Replicas: &replicas, RevisionHistoryLimit: &historyLimit, diff --git a/pkg/controller/nginxingress/default.go b/pkg/controller/nginxingress/default.go index eb2628a2..204d2e17 100644 --- a/pkg/controller/nginxingress/default.go +++ b/pkg/controller/nginxingress/default.go @@ -6,8 +6,10 @@ import ( "time" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" - "github.com/Azure/aks-app-routing-operator/pkg/controller/common" "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/util" + "github.com/go-logr/logr" netv1 "k8s.io/api/networking/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -22,10 +24,35 @@ const ( // DefaultNicName is the default Nginx Ingress Controller resource name DefaultNicName = "default" DefaultNicResourceName = "nginx" - reconcileInterval = time.Minute * 3 ) func NewDefaultReconciler(mgr ctrl.Manager) error { + name := controllername.New("default", "nginx", "ingress", "controller", "reconciler") + if err := mgr.Add(&defaultNicReconciler{ + name: name, + lgr: name.AddToLogger(mgr.GetLogger()), + client: mgr.GetClient(), + }); err != nil { + return fmt.Errorf("adding default nginx ingress controller: %w", err) + } + + return nil +} + +type defaultNicReconciler struct { + name controllername.ControllerNamer + client client.Client + lgr logr.Logger +} + +func (d *defaultNicReconciler) Start(ctx context.Context) (err error) { + start := time.Now() + d.lgr.Info("starting to reconcile default nginx ingress controller") + defer func() { + d.lgr.Info("finished reconciling default nginx ingress controller", "latencySec", time.Since(start).Seconds()) + metrics.HandleControllerReconcileMetrics(d.name, ctrl.Result{}, err) + }() + nic := &approutingv1alpha1.NginxIngressController{ TypeMeta: metav1.TypeMeta{ APIVersion: approutingv1alpha1.GroupVersion.String(), @@ -41,11 +68,48 @@ func NewDefaultReconciler(mgr ctrl.Manager) error { Status: approutingv1alpha1.NginxIngressControllerStatus{}, } - if err := common.NewResourceReconciler(mgr, controllername.New("default", "nginx", "ingress", "controller", "reconciler"), []client.Object{nic}, reconcileInterval); err != nil { - return fmt.Errorf("creating default nginx ingress controller: %w", err) + d.lgr.Info("determining if default nginx ingress controller should be created") + shouldCreate, err := shouldCreateDefaultNic(d.client) + if err != nil { + d.lgr.Error(err, "checking if default nginx ingress controller should be created") + return fmt.Errorf("checking if default nginx ingress controller should be created: %w", err) + } + + if !shouldCreate { + d.lgr.Info("default nginx ingress controller should not be created") + return nil + } + + d.lgr.Info("upserting default nginx ingress controller") + if err := util.Upsert(ctx, d.client, nic); err != nil { + d.lgr.Error(err, "upserting default nginx ingress controller") + return fmt.Errorf("upserting default nginx ingress controller: %w", err) } return nil + +} + +func shouldCreateDefaultNic(cl client.Client) (bool, error) { + defaultIc := &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultIcName, + }, + } + + err := cl.Get(context.Background(), types.NamespacedName{Name: DefaultIcName}, defaultIc) + switch { + case err == nil: // default IngressClass exists, we must create default nic for upgrade story + return true, nil + case k8serrors.IsNotFound(err): + // default IngressClass does not exist, we don't need to create default nic. We aren't upgrading from older App Routing versions for the first time. + // this is either a new user or an existing user that deleted their default nic + return false, nil + case err != nil: + return false, fmt.Errorf("getting default ingress class: %w", err) + } + + return false, nil } func getDefaultIngressClassControllerClass(cl client.Client) (string, error) { From da83f9ea5f47b1ab12bd44f745d5fda3e09af2e3 Mon Sep 17 00:00:00 2001 From: Oliver King Date: Sat, 4 Nov 2023 16:00:09 -0500 Subject: [PATCH 6/7] add unit tests --- pkg/controller/nginxingress/default.go | 1 - pkg/controller/nginxingress/default_test.go | 60 ++++++++++++++++++++- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/pkg/controller/nginxingress/default.go b/pkg/controller/nginxingress/default.go index 204d2e17..7fe701e1 100644 --- a/pkg/controller/nginxingress/default.go +++ b/pkg/controller/nginxingress/default.go @@ -65,7 +65,6 @@ func (d *defaultNicReconciler) Start(ctx context.Context) (err error) { ControllerNamePrefix: "nginx", IngressClassName: DefaultIcName, }, - Status: approutingv1alpha1.NginxIngressControllerStatus{}, } d.lgr.Info("determining if default nginx ingress controller should be created") diff --git a/pkg/controller/nginxingress/default_test.go b/pkg/controller/nginxingress/default_test.go index a5fde36f..f9819c2a 100644 --- a/pkg/controller/nginxingress/default_test.go +++ b/pkg/controller/nginxingress/default_test.go @@ -5,12 +5,70 @@ import ( "testing" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" + "github.com/go-logr/logr" "github.com/stretchr/testify/require" netv1 "k8s.io/api/networking/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +func TestDefaultNicReconciler(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, approutingv1alpha1.AddToScheme(scheme)) + require.NoError(t, netv1.AddToScheme(scheme)) + cl := fake.NewClientBuilder().WithScheme(scheme).Build() + + // when default nic doesn't exist in cluster we don't create the default nic + d := &defaultNicReconciler{ + client: cl, + lgr: logr.Discard(), + name: controllername.New("testing"), + } + require.NoError(t, d.Start(context.Background())) + + nic := &approutingv1alpha1.NginxIngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultNicName, + }, + } + require.True(t, k8serrors.IsNotFound(d.client.Get(context.Background(), types.NamespacedName{Name: nic.Name}, nic))) + + // when default nic exists in cluster we create the default nic + require.NoError(t, cl.Create(context.Background(), &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultIcName, + }, + })) + require.NoError(t, d.Start(context.Background())) + require.NoError(t, d.client.Get(context.Background(), types.NamespacedName{Name: nic.Name}, nic)) + require.Equal(t, "nginx", nic.Spec.ControllerNamePrefix) + require.Equal(t, DefaultIcName, nic.Spec.IngressClassName) + +} + +func TestShouldCreateDefaultNic(t *testing.T) { + cl := fake.NewClientBuilder().Build() + + // when default ic doesn't exist in cluster + shouldCreate, err := shouldCreateDefaultNic(cl) + require.NoError(t, err) + require.False(t, shouldCreate) + + // when default ic exists in cluster + require.NoError(t, cl.Create(context.Background(), &netv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultIcName, + }, + })) + shouldCreate, err = shouldCreateDefaultNic(cl) + require.NoError(t, err) + require.True(t, shouldCreate) +} + func TestGetDefaultIngressClassControllerClass(t *testing.T) { cl := fake.NewClientBuilder().Build() @@ -101,4 +159,4 @@ func TestIsDefaultNic(t *testing.T) { require.Equal(t, c.Expected, IsDefaultNic(c.Nic)) }) } -} \ No newline at end of file +} From ff7caf1ddae3089a3acf04930e3a74a8e6a041cc Mon Sep 17 00:00:00 2001 From: Oliver King Date: Sat, 4 Nov 2023 17:06:19 -0500 Subject: [PATCH 7/7] fix typo --- pkg/controller/keyvault/placeholder_pod_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/keyvault/placeholder_pod_test.go b/pkg/controller/keyvault/placeholder_pod_test.go index 8be8b773..08d28366 100644 --- a/pkg/controller/keyvault/placeholder_pod_test.go +++ b/pkg/controller/keyvault/placeholder_pod_test.go @@ -218,7 +218,7 @@ func TestPlaceholderPodControllerNoManagedByLabels(t *testing.T) { replicas := int32(1) historyLimit := int32(2) - expectedLabels := .map[string]string{"app": spc.Name} + expectedLabels := map[string]string{"app": spc.Name} expected := appsv1.DeploymentSpec{ Replicas: &replicas, RevisionHistoryLimit: &historyLimit,