Refactor component event handling

clm/cmd/util.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/sap/go-generics/slices"
+
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -20,7 +21,8 @@ import (
 	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 
 	"github.com/sap/component-operator-runtime/clm/internal/release"
-	"github.com/sap/component-operator-runtime/internal/cluster"
+	"github.com/sap/component-operator-runtime/internal/clientfactory"
+	"github.com/sap/component-operator-runtime/pkg/cluster"
 	"github.com/sap/component-operator-runtime/pkg/reconciler"
 )
 
@@ -56,7 +58,7 @@ func getClient(kubeConfigPath string) (cluster.Client, error) {
 	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
 	utilruntime.Must(apiextensionsv1.AddToScheme(scheme))
 	utilruntime.Must(apiregistrationv1.AddToScheme(scheme))
-	return cluster.NewClientFor(config, scheme, fullName)
+	return clientfactory.NewClientFor(config, scheme, fullName)
 }
 
 func isEphmeralError(err error) bool {

internal/cluster/client.go → internal/clientfactory/client.go

@@ -3,34 +3,21 @@ SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-op
 SPDX-License-Identifier: Apache-2.0
 */
 
-package cluster
+package clientfactory
 
 import (
-	"time"
-
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/kubernetes"
 	typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-)
 
-func NewClient(clnt client.Client, discoveryClient discovery.DiscoveryInterface, eventRecorder record.EventRecorder) Client {
-	return &clientImpl{
-		Client:          clnt,
-		discoveryClient: discoveryClient,
-		eventRecorder:   eventRecorder,
-	}
-}
-
-func NewClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (Client, error) {
-	return newClientFor(config, scheme, name)
-}
+	"github.com/sap/component-operator-runtime/pkg/cluster"
+)
 
-func newClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (*clientImpl, error) {
+func NewClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (*Client, error) {
 	httpClient, err := rest.HTTPClientFor(config)
 	if err != nil {
 		return nil, err
@@ -46,27 +33,15 @@ func newClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (*cl
 	eventBroadcaster := record.NewBroadcaster()
 	eventBroadcaster.StartRecordingToSink(&typedcorev1.EventSinkImpl{Interface: clientset.CoreV1().Events("")})
 	eventRecorder := eventBroadcaster.NewRecorder(scheme, corev1.EventSource{Component: name})
-	clnt := &clientImpl{
-		Client:           ctrlClient,
-		discoveryClient:  clientset,
+	clnt := &Client{
+		Client: cluster.NewClient(
+			ctrlClient,
+			clientset,
+			eventRecorder,
+			config,
+			httpClient,
+		),
 		eventBroadcaster: eventBroadcaster,
-		eventRecorder:    eventRecorder,
 	}
 	return clnt, nil
 }
-
-type clientImpl struct {
-	client.Client
-	discoveryClient  discovery.DiscoveryInterface
-	eventBroadcaster record.EventBroadcaster
-	eventRecorder    record.EventRecorder
-	validUntil       time.Time
-}
-
-func (c *clientImpl) DiscoveryClient() discovery.DiscoveryInterface {
-	return c.discoveryClient
-}
-
-func (c *clientImpl) EventRecorder() record.EventRecorder {
-	return c.eventRecorder
-}

internal/cluster/factory.go → internal/clientfactory/factory.go

@@ -3,7 +3,7 @@ SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-op
 SPDX-License-Identifier: Apache-2.0
 */
 
-package cluster
+package clientfactory
 
 import (
 	"crypto/sha256"
@@ -31,7 +31,7 @@ type ClientFactory struct {
 	controllerName string
 	config         *rest.Config
 	scheme         *runtime.Scheme
-	clients        map[string]*clientImpl
+	clients        map[string]*Client
 }
 
 const validity = 15 * time.Minute
@@ -58,7 +58,7 @@ func NewClientFactory(name string, controllerName string, config *rest.Config, s
 		controllerName: controllerName,
 		config:         config,
 		scheme:         scheme,
-		clients:        make(map[string]*clientImpl),
+		clients:        make(map[string]*Client),
 	}
 
 	go func() {
@@ -82,7 +82,7 @@ func NewClientFactory(name string, controllerName string, config *rest.Config, s
 	return factory, nil
 }
 
-func (f *ClientFactory) Get(kubeConfig []byte, impersonationUser string, impersonationGroups []string) (Client, error) {
+func (f *ClientFactory) Get(kubeConfig []byte, impersonationUser string, impersonationGroups []string) (*Client, error) {
 	f.mutex.Lock()
 	defer f.mutex.Unlock()
 
@@ -127,7 +127,7 @@ func (f *ClientFactory) Get(kubeConfig []byte, impersonationUser string, imperso
 			return rt.RoundTrip(r)
 		})
 	})
-	clnt, err := newClientFor(config, f.scheme, f.name)
+	clnt, err := NewClientFor(config, f.scheme, f.name)
 	if err != nil {
 		return nil, err
 	}

internal/clientfactory/types.go

@@ -0,0 +1,20 @@
+/*
+SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-operator-runtime contributors
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package clientfactory
+
+import (
+	"time"
+
+	"k8s.io/client-go/tools/record"
+
+	"github.com/sap/component-operator-runtime/pkg/cluster"
+)
+
+type Client struct {
+	cluster.Client
+	eventBroadcaster record.EventBroadcaster
+	validUntil       time.Time
+}

pkg/cluster/client.go

@@ -0,0 +1,49 @@
+/*
+SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-operator-runtime contributors
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package cluster
+
+import (
+	"net/http"
+
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func NewClient(clnt client.Client, discoveryClient discovery.DiscoveryInterface, eventRecorder record.EventRecorder, config *rest.Config, httpClient *http.Client) Client {
+	return &clientImpl{
+		Client:          clnt,
+		discoveryClient: discoveryClient,
+		eventRecorder:   eventRecorder,
+		config:          config,
+		httpClient:      httpClient,
+	}
+}
+
+type clientImpl struct {
+	client.Client
+	discoveryClient discovery.DiscoveryInterface
+	eventRecorder   record.EventRecorder
+	config          *rest.Config
+	httpClient      *http.Client
+}
+
+func (c *clientImpl) DiscoveryClient() discovery.DiscoveryInterface {
+	return c.discoveryClient
+}
+
+func (c *clientImpl) EventRecorder() record.EventRecorder {
+	return c.eventRecorder
+}
+
+func (c *clientImpl) Config() *rest.Config {
+	return c.config
+}
+
+func (c *clientImpl) HttpClient() *http.Client {
+	return c.httpClient
+}

pkg/cluster/types.go

@@ -5,6 +5,24 @@ SPDX-License-Identifier: Apache-2.0
 
 package cluster
 
-import "github.com/sap/component-operator-runtime/internal/cluster"
+import (
+	"net/http"
 
-type Client cluster.Client
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// The Client interface extends the controller-runtime client by discovery and event recording capabilities.
+type Client interface {
+	client.Client
+	// Return a discovery client.
+	DiscoveryClient() discovery.DiscoveryInterface
+	// Return an event recorder.
+	EventRecorder() record.EventRecorder
+	// Return a rest config for this client.
+	Config() *rest.Config
+	// Return a http client for this client.
+	HttpClient() *http.Client
+}

pkg/component/reconciler.go

@@ -40,8 +40,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	"github.com/sap/component-operator-runtime/internal/backoff"
-	"github.com/sap/component-operator-runtime/internal/cluster"
+	"github.com/sap/component-operator-runtime/internal/clientfactory"
 	"github.com/sap/component-operator-runtime/internal/metrics"
+	"github.com/sap/component-operator-runtime/pkg/cluster"
 	"github.com/sap/component-operator-runtime/pkg/manifests"
 	"github.com/sap/component-operator-runtime/pkg/reconciler"
 	"github.com/sap/component-operator-runtime/pkg/status"
@@ -85,6 +86,10 @@ const (
 // has been successful.
 type HookFunc[T Component] func(ctx context.Context, clnt client.Client, component T) error
 
+// NewClientFunc is the function signature that can be used to modify or replace the default
+// client used by the reconciler.
+type NewClientFunc func(clnt cluster.Client) (cluster.Client, error)
+
 // ReconcilerOptions are creation options for a Reconciler.
 type ReconcilerOptions struct {
 	// Which field manager to use in API calls.
@@ -114,6 +119,10 @@ type ReconcilerOptions struct {
 	// SchemeBuilder allows to define additional schemes to be made available in the
 	// target client.
 	SchemeBuilder types.SchemeBuilder
+	// NewClientFunc allows to modify or replace the default client used by the reconciler.
+	// The returned client is used by the reconciler to manage the component instances, and passed to hooks.
+	// Its scheme therefore must recognize the component type.
+	NewClient NewClientFunc
 }
 
 // Reconciler provides the implementation of controller-runtime's Reconciler interface, for a given Component type T.
@@ -126,7 +135,7 @@ type Reconciler[T Component] struct {
 	resourceGenerator  manifests.Generator
 	statusAnalyzer     status.StatusAnalyzer
 	options            ReconcilerOptions
-	clients            *cluster.ClientFactory
+	clients            *clientfactory.ClientFactory
 	backoff            *backoff.Backoff
 	postReadHooks      []HookFunc[T]
 	preReconcileHooks  []HookFunc[T]
@@ -300,10 +309,15 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 		// TODO: should we move this behind the DeepEqual check below to avoid noise?
 		// also note: it seems that no events will be written if the component's namespace is in deletion
 		state, reason, message := status.GetState()
+		var eventAnnotations map[string]string
+		// TODO: formalize this into a real published interface
+		if eventAnnotationProvider, ok := Component(component).(interface{ GetEventAnnotations() map[string]string }); ok {
+			eventAnnotations = eventAnnotationProvider.GetEventAnnotations()
+		}
 		if state == StateError {
-			r.client.EventRecorder().Event(component, corev1.EventTypeWarning, reason, message)
+			r.client.EventRecorder().AnnotatedEventf(component, eventAnnotations, corev1.EventTypeWarning, reason, message)
 		} else {
-			r.client.EventRecorder().Event(component, corev1.EventTypeNormal, reason, message)
+			r.client.EventRecorder().AnnotatedEventf(component, eventAnnotations, corev1.EventTypeNormal, reason, message)
 		}
 
 		if skipStatusUpdate {
@@ -431,15 +445,13 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 			log.V(1).Info("deletion not allowed")
 			// TODO: have an additional StateDeletionBlocked?
 			// TODO: eliminate this msg logic
-			r.client.EventRecorder().Event(component, corev1.EventTypeNormal, readyConditionReasonDeletionBlocked, "Deletion blocked: "+msg)
 			status.SetState(StateDeleting, readyConditionReasonDeletionBlocked, "Deletion blocked: "+msg)
 			return ctrl.Result{RequeueAfter: 1*time.Second + r.backoff.Next(req, readyConditionReasonDeletionBlocked)}, nil
 		}
 		if len(slices.Remove(component.GetFinalizers(), *r.options.Finalizer)) > 0 {
 			// deletion is blocked because of foreign finalizers
 			log.V(1).Info("deleted blocked due to existence of foreign finalizers")
 			// TODO: have an additional StateDeletionBlocked?
-			r.client.EventRecorder().Event(component, corev1.EventTypeNormal, readyConditionReasonDeletionBlocked, "Deletion blocked due to existing foreign finalizers")
 			status.SetState(StateDeleting, readyConditionReasonDeletionBlocked, "Deletion blocked due to existing foreign finalizers")
 			return ctrl.Result{RequeueAfter: 1*time.Second + r.backoff.Next(req, readyConditionReasonDeletionBlocked)}, nil
 		}
@@ -578,7 +590,14 @@ func (r *Reconciler[T]) SetupWithManagerAndBuilder(mgr ctrl.Manager, blder *ctrl
 	if err != nil {
 		return errors.Wrap(err, "error creating discovery client")
 	}
-	r.client = cluster.NewClient(mgr.GetClient(), discoveryClient, mgr.GetEventRecorderFor(r.name))
+	r.client = cluster.NewClient(mgr.GetClient(), discoveryClient, mgr.GetEventRecorderFor(r.name), mgr.GetConfig(), mgr.GetHTTPClient())
+	if r.options.NewClient != nil {
+		clnt, err := r.options.NewClient(r.client)
+		if err != nil {
+			return errors.Wrap(err, "error calling custom client constructor")
+		}
+		r.client = clnt
+	}
 
 	component := newComponent[T]()
 	r.groupVersionKind, err = apiutil.GVKForObject(component, r.client.Scheme())
@@ -596,7 +615,7 @@ func (r *Reconciler[T]) SetupWithManagerAndBuilder(mgr ctrl.Manager, blder *ctrl
 	if r.options.SchemeBuilder != nil {
 		schemeBuilders = append(schemeBuilders, r.options.SchemeBuilder)
 	}
-	r.clients, err = cluster.NewClientFactory(r.name, r.controllerName, mgr.GetConfig(), schemeBuilders)
+	r.clients, err = clientfactory.NewClientFactory(r.name, r.controllerName, mgr.GetConfig(), schemeBuilders)
 	if err != nil {
 		return errors.Wrap(err, "error creating client factory")
 	}

pkg/component/target.go

@@ -10,7 +10,7 @@ import (
 
 	"github.com/pkg/errors"
 
-	"github.com/sap/component-operator-runtime/internal/cluster"
+	"github.com/sap/component-operator-runtime/pkg/cluster"
 	"github.com/sap/component-operator-runtime/pkg/manifests"
 	"github.com/sap/component-operator-runtime/pkg/reconciler"
 )