CFDomain and CFRoute controllers pass logger in the ctx

[#1935]
cloudfoundry · Jun 23, 2023 · 5914feb · 5914feb
1 parent 6665d02
commit 5914feb
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 29 deletions.
diff --git a/controllers/controllers/networking/cfdomain_controller.go b/controllers/controllers/networking/cfdomain_controller.go
@@ -59,10 +59,11 @@ func (r *CFDomainReconciler) SetupWithManager(mgr ctrl.Manager) *builder.Builder
 //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfdomains/finalizers,verbs=update
 
 func (r *CFDomainReconciler) ReconcileResource(ctx context.Context, cfDomain *korifiv1alpha1.CFDomain) (ctrl.Result, error) {
-	log := r.log.WithValues("namespace", cfDomain.Namespace, "name", cfDomain.Name)
+	log := shared.ObjectLogger(r.log, cfDomain)
+	ctx = logr.NewContext(ctx, log)
 
 	if !cfDomain.GetDeletionTimestamp().IsZero() {
-		return r.finalizeCFDomain(ctx, log, cfDomain)
+		return r.finalizeCFDomain(ctx, cfDomain)
 	}
 
 	cfDomain.Status.ObservedGeneration = cfDomain.Generation
@@ -79,8 +80,8 @@ func (r *CFDomainReconciler) ReconcileResource(ctx context.Context, cfDomain *ko
 	return ctrl.Result{}, nil
 }
 
-func (r *CFDomainReconciler) finalizeCFDomain(ctx context.Context, log logr.Logger, cfDomain *korifiv1alpha1.CFDomain) (ctrl.Result, error) {
-	log = log.WithName("finalizeCFDomain")
+func (r *CFDomainReconciler) finalizeCFDomain(ctx context.Context, cfDomain *korifiv1alpha1.CFDomain) (ctrl.Result, error) {
+	log := logr.FromContextOrDiscard(ctx).WithName("finalizeCFDomain")
 
 	if !controllerutil.ContainsFinalizer(cfDomain, korifiv1alpha1.CFDomainFinalizerName) {
 		return ctrl.Result{}, nil

diff --git a/controllers/controllers/networking/cfdomain_controller_test.go b/controllers/controllers/networking/cfdomain_controller_test.go
@@ -9,6 +9,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/gbytes"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -96,7 +97,7 @@ var _ = Describe("CFDomainReconciler Integration Tests", func() {
 		})
 	})
 
-	Describe("Delete CFDomain", func() {
+	When("a domain is deleted", func() {
 		JustBeforeEach(func() {
 			Expect(k8sClient.Delete(ctx, cfDomain)).To(Succeed())
 		})
@@ -112,6 +113,10 @@ var _ = Describe("CFDomainReconciler Integration Tests", func() {
 				g.Expect(routes.Items).To(BeEmpty())
 			}).Should(Succeed())
 		})
+
+		It("writes a log message", func() {
+			Eventually(logOutput).Should(gbytes.Say("finalizer removed"))
+		})
 	})
 })
 

diff --git a/controllers/controllers/networking/cfroute_controller.go b/controllers/controllers/networking/cfroute_controller.go
@@ -23,6 +23,7 @@ import (
 
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
 	"code.cloudfoundry.org/korifi/controllers/config"
+	"code.cloudfoundry.org/korifi/controllers/controllers/shared"
 	"code.cloudfoundry.org/korifi/tools/k8s"
 
 	"github.com/go-logr/logr"
@@ -74,12 +75,13 @@ func (r *CFRouteReconciler) SetupWithManager(mgr ctrl.Manager) *builder.Builder
 //+kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch;delete
 
 func (r *CFRouteReconciler) ReconcileResource(ctx context.Context, cfRoute *korifiv1alpha1.CFRoute) (ctrl.Result, error) {
-	log := r.log.WithValues("namespace", cfRoute.Namespace, "name", cfRoute.Name)
+	log := shared.ObjectLogger(r.log, cfRoute)
+	ctx = logr.NewContext(ctx, log)
 
 	var err error
 
 	if !cfRoute.GetDeletionTimestamp().IsZero() {
-		err = r.finalizeCFRoute(ctx, log, cfRoute)
+		err = r.finalizeCFRoute(ctx, cfRoute)
 		if err != nil {
 			log.Info("failed to finalize cf route", "reason", err)
 		}
@@ -97,25 +99,25 @@ func (r *CFRouteReconciler) ReconcileResource(ctx context.Context, cfRoute *kori
 		return ctrl.Result{}, err
 	}
 
-	err = r.createOrPatchServices(ctx, log, cfRoute)
+	err = r.createOrPatchServices(ctx, cfRoute)
 	if err != nil {
 		cfRoute.Status = createInvalidRouteStatus(log, cfRoute, "Error creating/patching services", "CreatePatchServices", err.Error())
 		return ctrl.Result{}, err
 	}
 
-	err = r.createOrPatchRouteProxy(ctx, log, cfRoute)
+	err = r.createOrPatchRouteProxy(ctx, cfRoute)
 	if err != nil {
 		cfRoute.Status = createInvalidRouteStatus(log, cfRoute, "Error creating/patching Route Proxy", "CreatePatchRouteProxy", err.Error())
 		return ctrl.Result{}, err
 	}
 
-	err = r.createOrPatchFQDNProxy(ctx, log, cfRoute, cfDomain)
+	err = r.createOrPatchFQDNProxy(ctx, cfRoute, cfDomain)
 	if err != nil {
 		cfRoute.Status = createInvalidRouteStatus(log, cfRoute, "Error creating/patching FQDN Proxy", "CreatePatchFQDNProxy", err.Error())
 		return ctrl.Result{}, err
 	}
 
-	err = r.deleteOrphanedServices(ctx, log, cfRoute)
+	err = r.deleteOrphanedServices(ctx, cfRoute)
 	if err != nil {
 		// technically, failing to delete the orphaned services does not make the CFRoute invalid so we don't mess with the cfRoute status here
 		return ctrl.Result{}, err
@@ -169,23 +171,23 @@ func createInvalidRouteStatus(log logr.Logger, cfRoute *korifiv1alpha1.CFRoute,
 	return cfRouteStatus
 }
 
-func (r *CFRouteReconciler) finalizeCFRoute(ctx context.Context, log logr.Logger, cfRoute *korifiv1alpha1.CFRoute) error {
-	log = log.WithName("finalizeCRRoute")
+func (r *CFRouteReconciler) finalizeCFRoute(ctx context.Context, cfRoute *korifiv1alpha1.CFRoute) error {
+	log := logr.FromContextOrDiscard(ctx).WithName("finalizeCRRoute")
 
 	if !controllerutil.ContainsFinalizer(cfRoute, korifiv1alpha1.CFRouteFinalizerName) {
 		return nil
 	}
 
 	if cfRoute.Status.FQDN != "" {
-		fqdnHTTPProxy, foundFQDNProxy, err := r.getFQDNProxy(ctx, log, cfRoute.Status.FQDN, cfRoute.Namespace, false)
+		fqdnHTTPProxy, foundFQDNProxy, err := r.getFQDNProxy(ctx, cfRoute.Status.FQDN, cfRoute.Namespace, false)
 		if err != nil {
 			return err
 		}
 
 		// Cleanup the FQDN HTTPProxy on delete
 		if foundFQDNProxy {
 			log.V(1).Info("found FQDN proxy", "fqdn", cfRoute.Status.FQDN)
-			err := r.finalizeFQDNProxy(ctx, log, cfRoute.Name, fqdnHTTPProxy)
+			err := r.finalizeFQDNProxy(ctx, cfRoute.Name, fqdnHTTPProxy)
 			if err != nil {
 				return err
 			}
@@ -199,7 +201,9 @@ func (r *CFRouteReconciler) finalizeCFRoute(ctx context.Context, log logr.Logger
 	return nil
 }
 
-func (r *CFRouteReconciler) finalizeFQDNProxy(ctx context.Context, log logr.Logger, cfRouteName string, fqdnProxy *contourv1.HTTPProxy) error {
+func (r *CFRouteReconciler) finalizeFQDNProxy(ctx context.Context, cfRouteName string, fqdnProxy *contourv1.HTTPProxy) error {
+	log := logr.FromContextOrDiscard(ctx).WithName("finalizeFQDNProxy")
+
 	return k8s.PatchResource(ctx, r.client, fqdnProxy, func() {
 		var retainedIncludes []contourv1.Include
 		for _, include := range fqdnProxy.Spec.Includes {
@@ -213,8 +217,8 @@ func (r *CFRouteReconciler) finalizeFQDNProxy(ctx context.Context, log logr.Logg
 	})
 }
 
-func (r *CFRouteReconciler) createOrPatchServices(ctx context.Context, log logr.Logger, cfRoute *korifiv1alpha1.CFRoute) error {
-	log = log.WithName("createOrPatchServices")
+func (r *CFRouteReconciler) createOrPatchServices(ctx context.Context, cfRoute *korifiv1alpha1.CFRoute) error {
+	log := logr.FromContextOrDiscard(ctx).WithName("createOrPatchServices")
 
 	for i, destination := range cfRoute.Spec.Destinations {
 		serviceName := generateServiceName(&cfRoute.Spec.Destinations[i])
@@ -260,8 +264,8 @@ func (r *CFRouteReconciler) createOrPatchServices(ctx context.Context, log logr.
 	return nil
 }
 
-func (r *CFRouteReconciler) createOrPatchRouteProxy(ctx context.Context, log logr.Logger, cfRoute *korifiv1alpha1.CFRoute) error {
-	log = log.WithName("createOrPatchRouteProxy").WithValues("httpProxyNamespace", cfRoute.Namespace, "httpProxyName", cfRoute.Name)
+func (r *CFRouteReconciler) createOrPatchRouteProxy(ctx context.Context, cfRoute *korifiv1alpha1.CFRoute) error {
+	log := logr.FromContextOrDiscard(ctx).WithName("createOrPatchRouteProxy").WithValues("httpProxyNamespace", cfRoute.Namespace, "httpProxyName", cfRoute.Name)
 
 	services := make([]contourv1.Service, 0, len(cfRoute.Spec.Destinations))
 
@@ -311,12 +315,12 @@ func (r *CFRouteReconciler) createOrPatchRouteProxy(ctx context.Context, log log
 	return nil
 }
 
-func (r *CFRouteReconciler) createOrPatchFQDNProxy(ctx context.Context, log logr.Logger, cfRoute *korifiv1alpha1.CFRoute, cfDomain *korifiv1alpha1.CFDomain) error {
+func (r *CFRouteReconciler) createOrPatchFQDNProxy(ctx context.Context, cfRoute *korifiv1alpha1.CFRoute, cfDomain *korifiv1alpha1.CFDomain) error {
 	fqdn := buildFQDN(cfRoute, cfDomain)
 
-	log = log.WithName("createOrPatchFQDNProxy").WithValues("fqdn", fqdn)
+	log := logr.FromContextOrDiscard(ctx).WithName("createOrPatchFQDNProxy").WithValues("fqdn", fqdn)
 
-	fqdnHTTPProxy, foundFQDNProxy, err := r.getFQDNProxy(ctx, log, fqdn, cfRoute.Namespace, true)
+	fqdnHTTPProxy, foundFQDNProxy, err := r.getFQDNProxy(ctx, fqdn, cfRoute.Namespace, true)
 	if err != nil {
 		return err
 	}
@@ -371,8 +375,8 @@ func (r *CFRouteReconciler) createOrPatchFQDNProxy(ctx context.Context, log logr
 	return nil
 }
 
-func (r *CFRouteReconciler) getFQDNProxy(ctx context.Context, log logr.Logger, fqdn, namespace string, checkAllNamespaces bool) (*contourv1.HTTPProxy, bool, error) {
-	log = log.WithName("getFQDNProxy")
+func (r *CFRouteReconciler) getFQDNProxy(ctx context.Context, fqdn, namespace string, checkAllNamespaces bool) (*contourv1.HTTPProxy, bool, error) {
+	log := logr.FromContextOrDiscard(ctx).WithName("getFQDNProxy")
 
 	var fqdnHTTPProxy contourv1.HTTPProxy
 
@@ -409,14 +413,14 @@ func (r *CFRouteReconciler) getFQDNProxy(ctx context.Context, log logr.Logger, f
 	return &fqdnHTTPProxy, found, nil
 }
 
-func (r *CFRouteReconciler) deleteOrphanedServices(ctx context.Context, log logr.Logger, cfRoute *korifiv1alpha1.CFRoute) error {
-	log = log.WithName("deleteOrphanedServices")
+func (r *CFRouteReconciler) deleteOrphanedServices(ctx context.Context, cfRoute *korifiv1alpha1.CFRoute) error {
+	log := logr.FromContextOrDiscard(ctx).WithName("deleteOrphanedServices")
 
 	matchingLabelSet := map[string]string{
 		korifiv1alpha1.CFRouteGUIDLabelKey: cfRoute.Name,
 	}
 
-	serviceList, err := r.fetchServicesByMatchingLabels(ctx, log, matchingLabelSet, cfRoute.Namespace)
+	serviceList, err := r.fetchServicesByMatchingLabels(ctx, matchingLabelSet, cfRoute.Namespace)
 	if err != nil {
 		log.Info("failed to fetch services using label", "label", korifiv1alpha1.CFRouteGUIDLabelKey, "value", cfRoute.Name, "reason", err)
 		return err
@@ -432,6 +436,7 @@ func (r *CFRouteReconciler) deleteOrphanedServices(ctx context.Context, log logr
 				break
 			}
 		}
+
 		if isOrphan {
 			err = r.client.Delete(ctx, &serviceList.Items[i])
 			if err != nil {
@@ -444,7 +449,9 @@ func (r *CFRouteReconciler) deleteOrphanedServices(ctx context.Context, log logr
 	return nil
 }
 
-func (r *CFRouteReconciler) fetchServicesByMatchingLabels(ctx context.Context, log logr.Logger, labelSet map[string]string, namespace string) (*corev1.ServiceList, error) {
+func (r *CFRouteReconciler) fetchServicesByMatchingLabels(ctx context.Context, labelSet map[string]string, namespace string) (*corev1.ServiceList, error) {
+	log := logr.FromContextOrDiscard(ctx).WithName("fetchServicesByMatchingLabels")
+
 	selector, err := labels.ValidatedSelectorFromSet(labelSet)
 	if err != nil {
 		log.Info("error initializing label selector", "reason", err)

diff --git a/controllers/controllers/networking/cfroute_controller_test.go b/controllers/controllers/networking/cfroute_controller_test.go
@@ -12,6 +12,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/gbytes"
 	contourv1 "github.com/projectcontour/contour/apis/projectcontour/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -341,6 +342,10 @@ var _ = Describe("CFRouteReconciler Integration Tests", func() {
 					}))
 				}).Should(Succeed())
 			})
+
+			It("writes a log message", func() {
+				Eventually(logOutput).Should(gbytes.Say("finalizer removed"))
+			})
 		})
 	})
 

diff --git a/controllers/controllers/networking/suite_test.go b/controllers/controllers/networking/suite_test.go
@@ -20,6 +20,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/gbytes"
 	contourv1 "github.com/projectcontour/contour/apis/projectcontour/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -38,6 +39,7 @@ var (
 	cancel    context.CancelFunc
 	testEnv   *envtest.Environment
 	k8sClient client.Client
+	logOutput *gbytes.Buffer
 )
 
 func TestNetworkingControllers(t *testing.T) {
@@ -49,6 +51,8 @@ func TestNetworkingControllers(t *testing.T) {
 }
 
 var _ = BeforeSuite(func() {
+	logOutput = gbytes.NewBuffer()
+	GinkgoWriter.TeeTo(logOutput)
 	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
 
 	var ctx context.Context