Do not store loggers in controller stucts (#462)

Leverages the context to grab the correct logger for each method that uses a logger in ResourceGraphDefinition controller.

Author: n3wscott

Diff for: cmd/controller/main.go

@@ -24,12 +24,9 @@ import (
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
-	ctrlrtcontroller "sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
-	"sigs.k8s.io/controller-runtime/pkg/predicate"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	xv1alpha1 "github.com/kro-run/kro/api/v1alpha1"
 	kroclient "github.com/kro-run/kro/pkg/client"
@@ -161,6 +158,7 @@ func main() {
 		// if you are doing or is intended to do any operation such as perform cleanups
 		// after the manager stops then its usage might be unsafe.
 		// LeaderElectionReleaseOnCancel: true,
+		Logger: rootLogger,
 	})
 	if err != nil {
 		setupLog.Error(err, "unable to start manager")
@@ -187,26 +185,14 @@ func main() {
 		os.Exit(1)
 	}
 
-	reconciler := resourcegraphdefinitionctrl.NewResourceGraphDefinitionReconciler(
-		rootLogger,
-		mgr.GetClient(),
+	rgd := resourcegraphdefinitionctrl.NewResourceGraphDefinitionReconciler(
 		set,
 		allowCRDDeletion,
 		dc,
 		resourceGraphDefinitionGraphBuilder,
+		resourceGraphDefinitionConcurrentReconciles,
 	)
-	err = ctrl.NewControllerManagedBy(
-		mgr,
-	).For(
-		&xv1alpha1.ResourceGraphDefinition{},
-	).WithEventFilter(
-		predicate.GenerationChangedPredicate{},
-	).WithOptions(
-		ctrlrtcontroller.Options{
-			MaxConcurrentReconciles: resourceGraphDefinitionConcurrentReconciles,
-		},
-	).Complete(reconcile.AsReconciler[*xv1alpha1.ResourceGraphDefinition](mgr.GetClient(), reconciler))
-	if err != nil {
+	if err := rgd.SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ResourceGraphDefinition")
 		os.Exit(1)
 	}

Diff for: pkg/client/crd.go

@@ -18,13 +18,13 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/go-logr/logr"
 	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
+	logr "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 const (
@@ -51,15 +51,13 @@ type CRDClient interface {
 // CRDWrapper provides a simplified interface for CRD operations
 type CRDWrapper struct {
 	client       apiextensionsv1.CustomResourceDefinitionInterface
-	log          logr.Logger
 	pollInterval time.Duration
 	timeout      time.Duration
 }
 
 // CRDWrapperConfig contains configuration for the CRD wrapper
 type CRDWrapperConfig struct {
 	Client       *apiextensionsv1.ApiextensionsV1Client
-	Log          logr.Logger
 	PollInterval time.Duration
 	Timeout      time.Duration
 }
@@ -83,7 +81,6 @@ func newCRDWrapper(cfg CRDWrapperConfig) *CRDWrapper {
 
 	return &CRDWrapper{
 		client:       cfg.Client.CustomResourceDefinitions(),
-		log:          cfg.Log.WithName("crd-wrapper"),
 		pollInterval: cfg.PollInterval,
 		timeout:      cfg.Timeout,
 	}
@@ -95,18 +92,19 @@ func newCRDWrapper(cfg CRDWrapperConfig) *CRDWrapper {
 // The caller is responsible for ensuring the CRD, isn't introducing
 // breaking changes.
 func (w *CRDWrapper) Ensure(ctx context.Context, crd v1.CustomResourceDefinition) error {
+	log := logr.FromContext(ctx)
 	_, err := w.Get(ctx, crd.Name)
 	if err != nil {
 		if !apierrors.IsNotFound(err) {
 			return fmt.Errorf("failed to check for existing CRD: %w", err)
 		}
 
-		w.log.Info("Creating CRD", "name", crd.Name)
+		log.Info("Creating CRD", "name", crd.Name)
 		if err := w.create(ctx, crd); err != nil {
 			return fmt.Errorf("failed to create CRD: %w", err)
 		}
 	} else {
-		w.log.Info("Updating existing CRD", "name", crd.Name)
+		log.Info("Updating existing CRD", "name", crd.Name)
 		if err := w.patch(ctx, crd); err != nil {
 			return fmt.Errorf("failed to patch CRD: %w", err)
 		}
@@ -143,7 +141,8 @@ func (w *CRDWrapper) patch(ctx context.Context, newCRD v1.CustomResourceDefiniti
 
 // Delete removes a CRD if it exists
 func (w *CRDWrapper) Delete(ctx context.Context, name string) error {
-	w.log.Info("Deleting CRD", "name", name)
+	log := logr.FromContext(ctx)
+	log.Info("Deleting CRD", "name", name)
 
 	err := w.client.Delete(ctx, name, metav1.DeleteOptions{})
 	if err != nil && !apierrors.IsNotFound(err) {
@@ -154,7 +153,8 @@ func (w *CRDWrapper) Delete(ctx context.Context, name string) error {
 
 // waitForReady waits for a CRD to become ready
 func (w *CRDWrapper) waitForReady(ctx context.Context, name string) error {
-	w.log.Info("Waiting for CRD to become ready", "name", name)
+	log := logr.FromContext(ctx)
+	log.Info("Waiting for CRD to become ready", "name", name)
 
 	return wait.PollUntilContextTimeout(ctx, w.pollInterval, w.timeout, true,
 		func(ctx context.Context) (bool, error) {

Diff for: pkg/client/set.go

@@ -117,9 +117,9 @@ func (c *Set) RESTConfig() *rest.Config {
 }
 
 // CRD returns a new CRDWrapper instance
-func (s *Set) CRD(cfg CRDWrapperConfig) *CRDWrapper {
+func (c *Set) CRD(cfg CRDWrapperConfig) *CRDWrapper {
 	if cfg.Client == nil {
-		cfg.Client = s.apiExtensionsV1
+		cfg.Client = c.apiExtensionsV1
 	}
 
 	return newCRDWrapper(cfg)

Diff for: pkg/controller/resourcegraphdefinition/controller.go

@@ -17,10 +17,9 @@ import (
 	"context"
 
 	"github.com/go-logr/logr"
-	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/log"
+	ctrlrtcontroller "sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -37,79 +36,89 @@ import (
 
 // ResourceGraphDefinitionReconciler reconciles a ResourceGraphDefinition object
 type ResourceGraphDefinitionReconciler struct {
-	log        logr.Logger
-	rootLogger logr.Logger
-
 	allowCRDDeletion bool
 
+	// Client and instanceLogger are set with SetupWithManager
+
 	client.Client
+	instanceLogger logr.Logger
+
 	clientSet  *kroclient.Set
 	crdManager kroclient.CRDClient
 
-	metadataLabeler   metadata.Labeler
-	rgBuilder         *graph.Builder
-	dynamicController *dynamiccontroller.DynamicController
+	metadataLabeler         metadata.Labeler
+	rgBuilder               *graph.Builder
+	dynamicController       *dynamiccontroller.DynamicController
+	maxConcurrentReconciles int
 }
 
 func NewResourceGraphDefinitionReconciler(
-	log logr.Logger,
-	mgrClient client.Client,
 	clientSet *kroclient.Set,
 	allowCRDDeletion bool,
 	dynamicController *dynamiccontroller.DynamicController,
 	builder *graph.Builder,
+	maxConcurrentReconciles int,
 ) *ResourceGraphDefinitionReconciler {
-	crdWrapper := clientSet.CRD(kroclient.CRDWrapperConfig{
-		Log: log,
-	})
-	rgLogger := log.WithName("controller.resourceGraphDefinition")
+	crdWrapper := clientSet.CRD(kroclient.CRDWrapperConfig{})
 
 	return &ResourceGraphDefinitionReconciler{
-		rootLogger:        log,
-		log:               rgLogger,
-		clientSet:         clientSet,
-		Client:            mgrClient,
-		allowCRDDeletion:  allowCRDDeletion,
-		crdManager:        crdWrapper,
-		dynamicController: dynamicController,
-		metadataLabeler:   metadata.NewKROMetaLabeler(),
-		rgBuilder:         builder,
+		clientSet:               clientSet,
+		allowCRDDeletion:        allowCRDDeletion,
+		crdManager:              crdWrapper,
+		dynamicController:       dynamicController,
+		metadataLabeler:         metadata.NewKROMetaLabeler(),
+		rgBuilder:               builder,
+		maxConcurrentReconciles: maxConcurrentReconciles,
 	}
 }
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *ResourceGraphDefinitionReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	r.Client = mgr.GetClient()
+	r.instanceLogger = mgr.GetLogger()
+
+	logConstructor := func(req *reconcile.Request) logr.Logger {
+		log := mgr.GetLogger().WithName("rgd-controller").WithValues(
+			"controller", "ResourceGraphDefinition",
+			"controllerGroup", v1alpha1.GroupVersion.Group,
+			"controllerKind", "ResourceGraphDefinition",
+		)
+		if req != nil {
+			log = log.WithValues("name", req.Name)
+		}
+		return log
+	}
+
 	return ctrl.NewControllerManagedBy(mgr).
+		Named("ResourceGraphDefinition").
 		For(&v1alpha1.ResourceGraphDefinition{}).
 		WithEventFilter(predicate.GenerationChangedPredicate{}).
+		WithOptions(
+			ctrlrtcontroller.Options{
+				LogConstructor:          logConstructor,
+				MaxConcurrentReconciles: r.maxConcurrentReconciles,
+			},
+		).
 		Complete(reconcile.AsReconciler[*v1alpha1.ResourceGraphDefinition](mgr.GetClient(), r))
 }
 
-func (r *ResourceGraphDefinitionReconciler) Reconcile(ctx context.Context, resourcegraphdefinition *v1alpha1.ResourceGraphDefinition) (ctrl.Result, error) {
-	rlog := r.log.WithValues("resourcegraphdefinition", types.NamespacedName{Namespace: resourcegraphdefinition.Namespace, Name: resourcegraphdefinition.Name})
-	ctx = log.IntoContext(ctx, rlog)
-
-	if !resourcegraphdefinition.DeletionTimestamp.IsZero() {
-		if err := r.cleanupResourceGraphDefinition(ctx, resourcegraphdefinition); err != nil {
+func (r *ResourceGraphDefinitionReconciler) Reconcile(ctx context.Context, o *v1alpha1.ResourceGraphDefinition) (ctrl.Result, error) {
+	if !o.DeletionTimestamp.IsZero() {
+		if err := r.cleanupResourceGraphDefinition(ctx, o); err != nil {
 			return ctrl.Result{}, err
 		}
-
-		if err := r.setUnmanaged(ctx, resourcegraphdefinition); err != nil {
+		if err := r.setUnmanaged(ctx, o); err != nil {
 			return ctrl.Result{}, err
 		}
-
 		return ctrl.Result{}, nil
 	}
 
-	if err := r.setManaged(ctx, resourcegraphdefinition); err != nil {
+	if err := r.setManaged(ctx, o); err != nil {
 		return ctrl.Result{}, err
 	}
 
-	topologicalOrder, resourcesInformation, reconcileErr := r.reconcileResourceGraphDefinition(ctx, resourcegraphdefinition)
-
-	if err := r.setResourceGraphDefinitionStatus(ctx, resourcegraphdefinition, topologicalOrder, resourcesInformation, reconcileErr); err != nil {
-		return ctrl.Result{}, err
-	}
+	topologicalOrder, resourcesInformation, reconcileErr := r.reconcileResourceGraphDefinition(ctx, o)
 
-	return ctrl.Result{}, nil
+	return ctrl.Result{},
+		r.setResourceGraphDefinitionStatus(ctx, o, topologicalOrder, resourcesInformation, reconcileErr)
 }

Diff for: pkg/controller/resourcegraphdefinition/controller_cleanup.go

@@ -18,9 +18,9 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/go-logr/logr"
 	"github.com/gobuffalo/flect"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	ctrl "sigs.k8s.io/controller-runtime"
 
 	"github.com/kro-run/kro/api/v1alpha1"
 	"github.com/kro-run/kro/pkg/metadata"
@@ -31,8 +31,7 @@ import (
 // 1. Shuts down the microcontroller
 // 2. Deletes the associated CRD (if CRD deletion is enabled)
 func (r *ResourceGraphDefinitionReconciler) cleanupResourceGraphDefinition(ctx context.Context, rgd *v1alpha1.ResourceGraphDefinition) error {
-	log, _ := logr.FromContext(ctx)
-	log.V(1).Info("cleaning up resource graph definition", "name", rgd.Name)
+	ctrl.LoggerFrom(ctx).V(1).Info("cleaning up resource graph definition", "name", rgd.Name)
 
 	// shutdown microcontroller
 	gvr := metadata.GetResourceGraphDefinitionInstanceGVR(rgd.Spec.Schema.Group, rgd.Spec.Schema.APIVersion, rgd.Spec.Schema.Kind)
@@ -66,8 +65,7 @@ func (r *ResourceGraphDefinitionReconciler) shutdownResourceGraphDefinitionMicro
 // If CRD deletion is disabled, it logs the skip and returns nil.
 func (r *ResourceGraphDefinitionReconciler) cleanupResourceGraphDefinitionCRD(ctx context.Context, crdName string) error {
 	if !r.allowCRDDeletion {
-		log, _ := logr.FromContext(ctx)
-		log.Info("skipping CRD deletion (disabled)", "crd", crdName)
+		ctrl.LoggerFrom(ctx).Info("skipping CRD deletion (disabled)", "crd", crdName)
 		return nil
 	}
 

Diff for: pkg/controller/resourcegraphdefinition/controller_reconcile.go

@@ -18,9 +18,9 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/go-logr/logr"
 	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	ctrl "sigs.k8s.io/controller-runtime"
 
 	"github.com/kro-run/kro/api/v1alpha1"
 	instancectrl "github.com/kro-run/kro/pkg/controller/instance"
@@ -34,7 +34,7 @@ import (
 // 2. Ensuring CRDs are present
 // 3. Setting up and starting the microcontroller
 func (r *ResourceGraphDefinitionReconciler) reconcileResourceGraphDefinition(ctx context.Context, rgd *v1alpha1.ResourceGraphDefinition) ([]string, []v1alpha1.ResourceInformation, error) {
-	log, _ := logr.FromContext(ctx)
+	log := ctrl.LoggerFrom(ctx)
 
 	// Process resource graph definition graph first to validate structure
 	log.V(1).Info("reconciling resource graph definition graph")
@@ -63,6 +63,9 @@ func (r *ResourceGraphDefinitionReconciler) reconcileResourceGraphDefinition(ctx
 	controller := r.setupMicroController(gvr, processedRGD, rgd.Spec.DefaultServiceAccounts, graphExecLabeler)
 
 	log.V(1).Info("reconciling resource graph definition micro controller")
+	// TODO: the context that is passed here is tied to the reconciliation of the rgd, we might need to make
+	// a new context with our own cancel function here to allow us to cleanly term the dynamic controller
+	// rather than have it ignore this context and use the background context.
 	if err := r.reconcileResourceGraphDefinitionMicroController(ctx, &gvr, controller.Reconcile); err != nil {
 		return processedRGD.TopologicalOrder, resourcesInfo, err
 	}
@@ -83,8 +86,11 @@ func (r *ResourceGraphDefinitionReconciler) setupMicroController(
 	defaultSVCs map[string]string,
 	labeler metadata.Labeler,
 ) *instancectrl.Controller {
-
-	instanceLogger := r.rootLogger.WithName("controller." + gvr.Resource)
+	instanceLogger := r.instanceLogger.WithName(fmt.Sprintf("%s-controller", gvr.Resource)).WithValues(
+		"controller", gvr.Resource,
+		"controllerGroup", processedRGD.Instance.GetCRD().Spec.Group,
+		"controllerKind", processedRGD.Instance.GetCRD().Spec.Names.Kind,
+	)
 
 	return instancectrl.NewController(
 		instanceLogger,

Diff for: pkg/controller/resourcegraphdefinition/controller_status.go

@@ -20,10 +20,10 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/util/retry"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/go-logr/logr"
-
 	"github.com/kro-run/kro/api/v1alpha1"
 	"github.com/kro-run/kro/pkg/metadata"
 )
@@ -143,8 +143,7 @@ func (r *ResourceGraphDefinitionReconciler) setResourceGraphDefinitionStatus(
 // setManaged sets the resourcegraphdefinition as managed, by adding the
 // default finalizer if it doesn't exist.
 func (r *ResourceGraphDefinitionReconciler) setManaged(ctx context.Context, rgd *v1alpha1.ResourceGraphDefinition) error {
-	log, _ := logr.FromContext(ctx)
-	log.V(1).Info("setting resourcegraphdefinition as managed")
+	ctrl.LoggerFrom(ctx).V(1).Info("setting resourcegraphdefinition as managed")
 
 	// Skip if finalizer already exists
 	if metadata.HasResourceGraphDefinitionFinalizer(rgd) {
@@ -159,8 +158,7 @@ func (r *ResourceGraphDefinitionReconciler) setManaged(ctx context.Context, rgd
 // setUnmanaged sets the resourcegraphdefinition as unmanaged, by removing the
 // default finalizer if it exists.
 func (r *ResourceGraphDefinitionReconciler) setUnmanaged(ctx context.Context, rgd *v1alpha1.ResourceGraphDefinition) error {
-	log, _ := logr.FromContext(ctx)
-	log.V(1).Info("setting resourcegraphdefinition as unmanaged")
+	ctrl.LoggerFrom(ctx).V(1).Info("setting resourcegraphdefinition as unmanaged")
 
 	// Skip if finalizer already removed
 	if !metadata.HasResourceGraphDefinitionFinalizer(rgd) {