From 1eac5803ca37a2ea340809cc9cef8d3c854d36c6 Mon Sep 17 00:00:00 2001 From: Thibault Jamet Date: Fri, 29 Nov 2024 14:58:02 +0100 Subject: [PATCH] refactor: extract setting owner reference to a helper Change-Id: Iadd6a6ec887c6b70f7d931a4f0983f6e9b7d8d52 --- pkg/controllers/helpers.go | 24 +++++++++++++ pkg/controllers/helpers_internal_test.go | 40 ++++++++++++++++++++++ pkg/controllers/ingress_controller.go | 21 ++++-------- pkg/controllers/ingress_controller_test.go | 14 ++++---- 4 files changed, 78 insertions(+), 21 deletions(-) create mode 100644 pkg/controllers/helpers.go create mode 100644 pkg/controllers/helpers_internal_test.go diff --git a/pkg/controllers/helpers.go b/pkg/controllers/helpers.go new file mode 100644 index 0000000..dcfd045 --- /dev/null +++ b/pkg/controllers/helpers.go @@ -0,0 +1,24 @@ +package controllers + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func p[T any](v T) *T { + return &v +} + +func setOwnerRef(ownee, owner client.Object) { + ownee.SetOwnerReferences( + []metav1.OwnerReference{ + { + APIVersion: owner.GetObjectKind().GroupVersionKind().GroupVersion().String(), + Kind: owner.GetObjectKind().GroupVersionKind().Kind, + Name: owner.GetName(), + UID: owner.GetUID(), + Controller: p(true), + }, + }, + ) +} diff --git a/pkg/controllers/helpers_internal_test.go b/pkg/controllers/helpers_internal_test.go new file mode 100644 index 0000000..f48ec80 --- /dev/null +++ b/pkg/controllers/helpers_internal_test.go @@ -0,0 +1,40 @@ +package controllers + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + netv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestSetOwnerRef(t *testing.T) { + obj := v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test-namespace", + }, + } + ingress := &netv1.Ingress{ + TypeMeta: metav1.TypeMeta{ + Kind: "Ingress", + APIVersion: "networking.k8s.io/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ingress", + Namespace: "test-namespace", + UID: "test-uid", + }, + } + setOwnerRef(&obj, ingress) + + require.Len(t, obj.GetOwnerReferences(), 1) + assert.EqualValues(t, "Ingress", obj.GetOwnerReferences()[0].Kind) + assert.EqualValues(t, "networking.k8s.io/v1", obj.GetOwnerReferences()[0].APIVersion) + assert.EqualValues(t, "test-ingress", obj.GetOwnerReferences()[0].Name) + assert.EqualValues(t, "test-uid", obj.GetOwnerReferences()[0].UID) + require.NotNil(t, obj.GetOwnerReferences()[0].Controller) + assert.True(t, *obj.GetOwnerReferences()[0].Controller) +} diff --git a/pkg/controllers/ingress_controller.go b/pkg/controllers/ingress_controller.go index b6ad6bf..bae2ee1 100644 --- a/pkg/controllers/ingress_controller.go +++ b/pkg/controllers/ingress_controller.go @@ -133,13 +133,15 @@ func (r *IngressReconciler) calculateIngressWeight(ingress netv1.Ingress) (uint, return desiredWeight, err } -func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *externaldnsk8siov1alpha1.DNSEndpoint, target string, ingress netv1.Ingress, owner metav1.OwnerReference) { +func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *externaldnsk8siov1alpha1.DNSEndpoint, target string, ingress netv1.Ingress) { var desiredWeight uint var err error var healthCheckProperty *externaldnsk8siov1alpha1.ProviderSpecificProperty + + setOwnerRef(dnsEndpoint, &ingress) + dnsEndpoint.Name = ingress.ObjectMeta.Name dnsEndpoint.Namespace = ingress.ObjectMeta.Namespace - dnsEndpoint.SetOwnerReferences([]metav1.OwnerReference{owner}) desiredWeight = uint(trafficweight.Store.DesiredWeight) if trafficweight.Store.AWSHealthCheckID != "" { healthCheckProperty = &externaldnsk8siov1alpha1.ProviderSpecificProperty{ @@ -187,7 +189,7 @@ func (r *IngressReconciler) newDnsEndpoint(ctx context.Context, dnsEndpoint *ext } } -func (r *IngressReconciler) reconcileDNSEntries(ctx context.Context, ingress netv1.Ingress, ownerRef metav1.OwnerReference) error { +func (r *IngressReconciler) reconcileDNSEntries(ctx context.Context, ingress netv1.Ingress) error { log := r.Log.WithValues("IngressName", ingress.ObjectMeta.Name).WithValues("IngressNamespace", ingress.ObjectMeta.Namespace) if !r.ingressAnnotationMatchFilter(ingress) { @@ -210,7 +212,7 @@ func (r *IngressReconciler) reconcileDNSEntries(ctx context.Context, ingress net }, } var f controllerutil.MutateFn = func() error { - r.newDnsEndpoint(ctx, dnsEndpoint, target, ingress, ownerRef) + r.newDnsEndpoint(ctx, dnsEndpoint, target, ingress) return nil } _, err = ctrl.CreateOrUpdate(ctx, r.Client, dnsEndpoint, f) @@ -294,8 +296,6 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct trafficStoreMetrics.CurrentWeight.Set(float64(trafficweight.Store.CurrentWeight)) var ingress netv1.Ingress - controller := true - var ownerRef metav1.OwnerReference if err := r.Get(ctx, req.NamespacedName, &ingress); err != nil { if apierrors.IsNotFound(err) { @@ -322,13 +322,6 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // on deleted requests. return ctrl.Result{}, client.IgnoreNotFound(err) } - ownerRef = metav1.OwnerReference{ - APIVersion: ingress.APIVersion, - Kind: ingress.Kind, - Name: ingress.GetName(), - UID: ingress.GetUID(), - Controller: &controller, - } if ingress.ObjectMeta.DeletionTimestamp.IsZero() { // Not being deleted // There maybe situations in which we receive an reconciliation event while @@ -338,7 +331,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Info("DNS endpoint being removed, requeuing notification") return ctrl.Result{Requeue: true, RequeueAfter: 5 * time.Second}, nil } - err := r.reconcileDNSEntries(ctx, ingress, ownerRef) + err := r.reconcileDNSEntries(ctx, ingress) if err != nil { log.Error(err, "Could not reconcile DNS endpoints") return ctrl.Result{}, err diff --git a/pkg/controllers/ingress_controller_test.go b/pkg/controllers/ingress_controller_test.go index 3d118ea..3551c64 100644 --- a/pkg/controllers/ingress_controller_test.go +++ b/pkg/controllers/ingress_controller_test.go @@ -127,14 +127,14 @@ func TestIngressController(t *testing.T) { t.Run("Should forge DNS Endpoints", func(t *testing.T) { forged := &externaldnsk8siov1alpha1.DNSEndpoint{} - reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing, ownerRef) + reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing) assert.Equal(t, expected, *forged) }) t.Run("Should create a DNSentry that does not exist during reconcile", func(t *testing.T) { reconciler.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(&ing).Build() - err := reconciler.reconcileDNSEntries(context.Background(), ing, ownerRef) + err := reconciler.reconcileDNSEntries(context.Background(), ing) assert.NoError(t, err) ep := externaldnsk8siov1alpha1.DNSEndpoint{} @@ -150,7 +150,7 @@ func TestIngressController(t *testing.T) { oldEndpoint.Spec.Endpoints[0].DNSName = "randomChange" reconciler.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(&ing, oldEndpoint).Build() - err := reconciler.reconcileDNSEntries(context.Background(), ing, ownerRef) + err := reconciler.reconcileDNSEntries(context.Background(), ing) assert.NoError(t, err) ep := externaldnsk8siov1alpha1.DNSEndpoint{} @@ -165,7 +165,7 @@ func TestIngressController(t *testing.T) { reconciler.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(&ing).Build() reconciler.AnnotationFilter = NewAnnotationFilter("foo=bar") - err := reconciler.reconcileDNSEntries(context.Background(), ing, ownerRef) + err := reconciler.reconcileDNSEntries(context.Background(), ing) assert.NoError(t, err) ep := externaldnsk8siov1alpha1.DNSEndpoint{} @@ -180,7 +180,7 @@ func TestIngressController(t *testing.T) { reconciler.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(&ing).Build() reconciler.AnnotationFilter = NewAnnotationFilter("foo=notbar") - err := reconciler.reconcileDNSEntries(context.Background(), ing, ownerRef) + err := reconciler.reconcileDNSEntries(context.Background(), ing) ep := externaldnsk8siov1alpha1.DNSEndpoint{} err = reconciler.Client.Get(context.Background(), client.ObjectKeyFromObject(&expected), &ep) @@ -191,7 +191,7 @@ func TestIngressController(t *testing.T) { t.Run("if we dont set --aws-health-check-id ingress shouldnt have health property", func(t *testing.T) { trafficweight.Store.AWSHealthCheckID = "" forged := &externaldnsk8siov1alpha1.DNSEndpoint{} - reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing, ownerRef) + reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing) assert.Equal(t, expected, *forged) }) @@ -226,7 +226,7 @@ func TestIngressController(t *testing.T) { } forged := &externaldnsk8siov1alpha1.DNSEndpoint{} - reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing, ownerRef) + reconciler.newDnsEndpoint(context.Background(), forged, "bar-celona", ing) assert.Equal(t, expected, *forged) ing.Spec.Rules = oldRules })