From 98073a99b8610c1f7ff9a5ae2df7014e9f66d3a8 Mon Sep 17 00:00:00 2001 From: Christoph Barbian Date: Sat, 22 Feb 2025 18:23:42 +0100 Subject: [PATCH 1/3] Refactor component event handling --- clm/cmd/util.go | 6 ++- internal/{cluster => clientfactory}/client.go | 49 +++++-------------- .../{cluster => clientfactory}/factory.go | 10 ++-- internal/clientfactory/types.go | 20 ++++++++ internal/cluster/types.go | 23 --------- pkg/cluster/client.go | 49 +++++++++++++++++++ pkg/cluster/types.go | 22 ++++++++- pkg/component/reconciler.go | 33 ++++++++++--- pkg/component/target.go | 2 +- 9 files changed, 136 insertions(+), 78 deletions(-) rename internal/{cluster => clientfactory}/client.go (53%) rename internal/{cluster => clientfactory}/factory.go (95%) create mode 100644 internal/clientfactory/types.go delete mode 100644 internal/cluster/types.go create mode 100644 pkg/cluster/client.go diff --git a/clm/cmd/util.go b/clm/cmd/util.go index 635865aa..8a02659a 100644 --- a/clm/cmd/util.go +++ b/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 { diff --git a/internal/cluster/client.go b/internal/clientfactory/client.go similarity index 53% rename from internal/cluster/client.go rename to internal/clientfactory/client.go index 8357739d..3fc48b9c 100644 --- a/internal/cluster/client.go +++ b/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 -} diff --git a/internal/cluster/factory.go b/internal/clientfactory/factory.go similarity index 95% rename from internal/cluster/factory.go rename to internal/clientfactory/factory.go index 367d006f..08d4f1db 100644 --- a/internal/cluster/factory.go +++ b/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 } diff --git a/internal/clientfactory/types.go b/internal/clientfactory/types.go new file mode 100644 index 00000000..7eaa9463 --- /dev/null +++ b/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 +} diff --git a/internal/cluster/types.go b/internal/cluster/types.go deleted file mode 100644 index 5fec3717..00000000 --- a/internal/cluster/types.go +++ /dev/null @@ -1,23 +0,0 @@ -/* -SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-operator-runtime contributors -SPDX-License-Identifier: Apache-2.0 -*/ - -package cluster - -import ( - "k8s.io/client-go/discovery" - "k8s.io/client-go/tools/record" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// TODO: should we add a Config() method, i.e. expose the rest.Config used by the 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 -} diff --git a/pkg/cluster/client.go b/pkg/cluster/client.go new file mode 100644 index 00000000..cf7deba5 --- /dev/null +++ b/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 +} diff --git a/pkg/cluster/types.go b/pkg/cluster/types.go index 5f215a38..70e657a6 100644 --- a/pkg/cluster/types.go +++ b/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 +} diff --git a/pkg/component/reconciler.go b/pkg/component/reconciler.go index 7a06e988..fff9f1d6 100644 --- a/pkg/component/reconciler.go +++ b/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,8 @@ type ReconcilerOptions struct { // SchemeBuilder allows to define additional schemes to be made available in the // target client. SchemeBuilder types.SchemeBuilder + // NewClientFunc allows to mofify or replace the default client used by the reconciler. + NewClientFunc NewClientFunc } // Reconciler provides the implementation of controller-runtime's Reconciler interface, for a given Component type T. @@ -126,7 +133,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 +307,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,7 +443,6 @@ 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 } @@ -439,7 +450,6 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result // 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 +588,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.NewClientFunc != nil { + clnt, err := r.options.NewClientFunc(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 +613,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") } diff --git a/pkg/component/target.go b/pkg/component/target.go index 306d6778..eaa810a8 100644 --- a/pkg/component/target.go +++ b/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" ) From d07c2cf5c50d7c5b4d2cc4c15fbfd24803685ead Mon Sep 17 00:00:00 2001 From: Christoph Barbian Date: Mon, 24 Feb 2025 11:53:04 +0100 Subject: [PATCH 2/3] add comments --- pkg/component/reconciler.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/component/reconciler.go b/pkg/component/reconciler.go index fff9f1d6..4d7313ca 100644 --- a/pkg/component/reconciler.go +++ b/pkg/component/reconciler.go @@ -119,7 +119,9 @@ type ReconcilerOptions struct { // SchemeBuilder allows to define additional schemes to be made available in the // target client. SchemeBuilder types.SchemeBuilder - // NewClientFunc allows to mofify or replace the default client used by the reconciler. + // 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. NewClientFunc NewClientFunc } From 32d81bc9f55303dad8dc920138c6e837f3041180 Mon Sep 17 00:00:00 2001 From: Christoph Barbian Date: Mon, 24 Feb 2025 11:54:39 +0100 Subject: [PATCH 3/3] rename NewClientFunc -> NewClient --- pkg/component/reconciler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/component/reconciler.go b/pkg/component/reconciler.go index 4d7313ca..cf2529a7 100644 --- a/pkg/component/reconciler.go +++ b/pkg/component/reconciler.go @@ -122,7 +122,7 @@ type ReconcilerOptions struct { // 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. - NewClientFunc NewClientFunc + NewClient NewClientFunc } // Reconciler provides the implementation of controller-runtime's Reconciler interface, for a given Component type T. @@ -591,8 +591,8 @@ func (r *Reconciler[T]) SetupWithManagerAndBuilder(mgr ctrl.Manager, blder *ctrl return errors.Wrap(err, "error creating discovery client") } r.client = cluster.NewClient(mgr.GetClient(), discoveryClient, mgr.GetEventRecorderFor(r.name), mgr.GetConfig(), mgr.GetHTTPClient()) - if r.options.NewClientFunc != nil { - clnt, err := r.options.NewClientFunc(r.client) + if r.options.NewClient != nil { + clnt, err := r.options.NewClient(r.client) if err != nil { return errors.Wrap(err, "error calling custom client constructor") }