Skip to content

Commit 64bb8e4

Browse files
cbarbian-sapcbarbian-sap
authored
Refactor component event handling (#239)
* Refactor component event handling * add comments * rename NewClientFunc -> NewClient
1 parent f3caee5 commit 64bb8e4

File tree

9 files changed

+138
-78
lines changed

9 files changed

+138
-78
lines changed

‎clm/cmd/util.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"time"
1212

1313
"github.com/sap/go-generics/slices"
14+
1415
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
1516
apierrors "k8s.io/apimachinery/pkg/api/errors"
1617
"k8s.io/apimachinery/pkg/runtime"
@@ -20,7 +21,8 @@ import (
2021
apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
2122

2223
"github.com/sap/component-operator-runtime/clm/internal/release"
23-
"github.com/sap/component-operator-runtime/internal/cluster"
24+
"github.com/sap/component-operator-runtime/internal/clientfactory"
25+
"github.com/sap/component-operator-runtime/pkg/cluster"
2426
"github.com/sap/component-operator-runtime/pkg/reconciler"
2527
)
2628

@@ -56,7 +58,7 @@ func getClient(kubeConfigPath string) (cluster.Client, error) {
5658
utilruntime.Must(clientgoscheme.AddToScheme(scheme))
5759
utilruntime.Must(apiextensionsv1.AddToScheme(scheme))
5860
utilruntime.Must(apiregistrationv1.AddToScheme(scheme))
59-
return cluster.NewClientFor(config, scheme, fullName)
61+
return clientfactory.NewClientFor(config, scheme, fullName)
6062
}
6163

6264
func isEphmeralError(err error) bool {

‎internal/cluster/client.go‎ renamed to ‎internal/clientfactory/client.go‎

Lines changed: 12 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,34 +3,21 @@ SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-op
33
SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
package cluster
6+
package clientfactory
77

88
import (
9-
"time"
10-
119
corev1 "k8s.io/api/core/v1"
1210
"k8s.io/apimachinery/pkg/runtime"
13-
"k8s.io/client-go/discovery"
1411
"k8s.io/client-go/kubernetes"
1512
typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
1613
"k8s.io/client-go/rest"
1714
"k8s.io/client-go/tools/record"
1815
"sigs.k8s.io/controller-runtime/pkg/client"
19-
)
2016

21-
func NewClient(clnt client.Client, discoveryClient discovery.DiscoveryInterface, eventRecorder record.EventRecorder) Client {
22-
return &clientImpl{
23-
Client: clnt,
24-
discoveryClient: discoveryClient,
25-
eventRecorder: eventRecorder,
26-
}
27-
}
28-
29-
func NewClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (Client, error) {
30-
return newClientFor(config, scheme, name)
31-
}
17+
"github.com/sap/component-operator-runtime/pkg/cluster"
18+
)
3219

33-
func newClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (*clientImpl, error) {
20+
func NewClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (*Client, error) {
3421
httpClient, err := rest.HTTPClientFor(config)
3522
if err != nil {
3623
return nil, err
@@ -46,27 +33,15 @@ func newClientFor(config *rest.Config, scheme *runtime.Scheme, name string) (*cl
4633
eventBroadcaster := record.NewBroadcaster()
4734
eventBroadcaster.StartRecordingToSink(&typedcorev1.EventSinkImpl{Interface: clientset.CoreV1().Events("")})
4835
eventRecorder := eventBroadcaster.NewRecorder(scheme, corev1.EventSource{Component: name})
49-
clnt := &clientImpl{
50-
Client: ctrlClient,
51-
discoveryClient: clientset,
36+
clnt := &Client{
37+
Client: cluster.NewClient(
38+
ctrlClient,
39+
clientset,
40+
eventRecorder,
41+
config,
42+
httpClient,
43+
),
5244
eventBroadcaster: eventBroadcaster,
53-
eventRecorder: eventRecorder,
5445
}
5546
return clnt, nil
5647
}
57-
58-
type clientImpl struct {
59-
client.Client
60-
discoveryClient discovery.DiscoveryInterface
61-
eventBroadcaster record.EventBroadcaster
62-
eventRecorder record.EventRecorder
63-
validUntil time.Time
64-
}
65-
66-
func (c *clientImpl) DiscoveryClient() discovery.DiscoveryInterface {
67-
return c.discoveryClient
68-
}
69-
70-
func (c *clientImpl) EventRecorder() record.EventRecorder {
71-
return c.eventRecorder
72-
}

‎internal/cluster/factory.go‎ renamed to ‎internal/clientfactory/factory.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-op
33
SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
package cluster
6+
package clientfactory
77

88
import (
99
"crypto/sha256"
@@ -31,7 +31,7 @@ type ClientFactory struct {
3131
controllerName string
3232
config *rest.Config
3333
scheme *runtime.Scheme
34-
clients map[string]*clientImpl
34+
clients map[string]*Client
3535
}
3636

3737
const validity = 15 * time.Minute
@@ -58,7 +58,7 @@ func NewClientFactory(name string, controllerName string, config *rest.Config, s
5858
controllerName: controllerName,
5959
config: config,
6060
scheme: scheme,
61-
clients: make(map[string]*clientImpl),
61+
clients: make(map[string]*Client),
6262
}
6363

6464
go func() {
@@ -82,7 +82,7 @@ func NewClientFactory(name string, controllerName string, config *rest.Config, s
8282
return factory, nil
8383
}
8484

85-
func (f *ClientFactory) Get(kubeConfig []byte, impersonationUser string, impersonationGroups []string) (Client, error) {
85+
func (f *ClientFactory) Get(kubeConfig []byte, impersonationUser string, impersonationGroups []string) (*Client, error) {
8686
f.mutex.Lock()
8787
defer f.mutex.Unlock()
8888

@@ -127,7 +127,7 @@ func (f *ClientFactory) Get(kubeConfig []byte, impersonationUser string, imperso
127127
return rt.RoundTrip(r)
128128
})
129129
})
130-
clnt, err := newClientFor(config, f.scheme, f.name)
130+
clnt, err := NewClientFor(config, f.scheme, f.name)
131131
if err != nil {
132132
return nil, err
133133
}

‎internal/clientfactory/types.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/*
2+
SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-operator-runtime contributors
3+
SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package clientfactory
7+
8+
import (
9+
"time"
10+
11+
"k8s.io/client-go/tools/record"
12+
13+
"github.com/sap/component-operator-runtime/pkg/cluster"
14+
)
15+
16+
type Client struct {
17+
cluster.Client
18+
eventBroadcaster record.EventBroadcaster
19+
validUntil time.Time
20+
}

‎internal/cluster/types.go‎

Lines changed: 0 additions & 23 deletions
This file was deleted.

‎pkg/cluster/client.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/*
2+
SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and component-operator-runtime contributors
3+
SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package cluster
7+
8+
import (
9+
"net/http"
10+
11+
"k8s.io/client-go/discovery"
12+
"k8s.io/client-go/rest"
13+
"k8s.io/client-go/tools/record"
14+
"sigs.k8s.io/controller-runtime/pkg/client"
15+
)
16+
17+
func NewClient(clnt client.Client, discoveryClient discovery.DiscoveryInterface, eventRecorder record.EventRecorder, config *rest.Config, httpClient *http.Client) Client {
18+
return &clientImpl{
19+
Client: clnt,
20+
discoveryClient: discoveryClient,
21+
eventRecorder: eventRecorder,
22+
config: config,
23+
httpClient: httpClient,
24+
}
25+
}
26+
27+
type clientImpl struct {
28+
client.Client
29+
discoveryClient discovery.DiscoveryInterface
30+
eventRecorder record.EventRecorder
31+
config *rest.Config
32+
httpClient *http.Client
33+
}
34+
35+
func (c *clientImpl) DiscoveryClient() discovery.DiscoveryInterface {
36+
return c.discoveryClient
37+
}
38+
39+
func (c *clientImpl) EventRecorder() record.EventRecorder {
40+
return c.eventRecorder
41+
}
42+
43+
func (c *clientImpl) Config() *rest.Config {
44+
return c.config
45+
}
46+
47+
func (c *clientImpl) HttpClient() *http.Client {
48+
return c.httpClient
49+
}

‎pkg/cluster/types.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,24 @@ SPDX-License-Identifier: Apache-2.0
55

66
package cluster
77

8-
import "github.com/sap/component-operator-runtime/internal/cluster"
8+
import (
9+
"net/http"
910

10-
type Client cluster.Client
11+
"k8s.io/client-go/discovery"
12+
"k8s.io/client-go/rest"
13+
"k8s.io/client-go/tools/record"
14+
"sigs.k8s.io/controller-runtime/pkg/client"
15+
)
16+
17+
// The Client interface extends the controller-runtime client by discovery and event recording capabilities.
18+
type Client interface {
19+
client.Client
20+
// Return a discovery client.
21+
DiscoveryClient() discovery.DiscoveryInterface
22+
// Return an event recorder.
23+
EventRecorder() record.EventRecorder
24+
// Return a rest config for this client.
25+
Config() *rest.Config
26+
// Return a http client for this client.
27+
HttpClient() *http.Client
28+
}

‎pkg/component/reconciler.go‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,9 @@ import (
4040
"sigs.k8s.io/controller-runtime/pkg/source"
4141

4242
"github.com/sap/component-operator-runtime/internal/backoff"
43-
"github.com/sap/component-operator-runtime/internal/cluster"
43+
"github.com/sap/component-operator-runtime/internal/clientfactory"
4444
"github.com/sap/component-operator-runtime/internal/metrics"
45+
"github.com/sap/component-operator-runtime/pkg/cluster"
4546
"github.com/sap/component-operator-runtime/pkg/manifests"
4647
"github.com/sap/component-operator-runtime/pkg/reconciler"
4748
"github.com/sap/component-operator-runtime/pkg/status"
@@ -85,6 +86,10 @@ const (
8586
// has been successful.
8687
type HookFunc[T Component] func(ctx context.Context, clnt client.Client, component T) error
8788

89+
// NewClientFunc is the function signature that can be used to modify or replace the default
90+
// client used by the reconciler.
91+
type NewClientFunc func(clnt cluster.Client) (cluster.Client, error)
92+
8893
// ReconcilerOptions are creation options for a Reconciler.
8994
type ReconcilerOptions struct {
9095
// Which field manager to use in API calls.
@@ -114,6 +119,10 @@ type ReconcilerOptions struct {
114119
// SchemeBuilder allows to define additional schemes to be made available in the
115120
// target client.
116121
SchemeBuilder types.SchemeBuilder
122+
// NewClientFunc allows to modify or replace the default client used by the reconciler.
123+
// The returned client is used by the reconciler to manage the component instances, and passed to hooks.
124+
// Its scheme therefore must recognize the component type.
125+
NewClient NewClientFunc
117126
}
118127

119128
// 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 {
126135
resourceGenerator manifests.Generator
127136
statusAnalyzer status.StatusAnalyzer
128137
options ReconcilerOptions
129-
clients *cluster.ClientFactory
138+
clients *clientfactory.ClientFactory
130139
backoff *backoff.Backoff
131140
postReadHooks []HookFunc[T]
132141
preReconcileHooks []HookFunc[T]
@@ -300,10 +309,15 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
300309
// TODO: should we move this behind the DeepEqual check below to avoid noise?
301310
// also note: it seems that no events will be written if the component's namespace is in deletion
302311
state, reason, message := status.GetState()
312+
var eventAnnotations map[string]string
313+
// TODO: formalize this into a real published interface
314+
if eventAnnotationProvider, ok := Component(component).(interface{ GetEventAnnotations() map[string]string }); ok {
315+
eventAnnotations = eventAnnotationProvider.GetEventAnnotations()
316+
}
303317
if state == StateError {
304-
r.client.EventRecorder().Event(component, corev1.EventTypeWarning, reason, message)
318+
r.client.EventRecorder().AnnotatedEventf(component, eventAnnotations, corev1.EventTypeWarning, reason, message)
305319
} else {
306-
r.client.EventRecorder().Event(component, corev1.EventTypeNormal, reason, message)
320+
r.client.EventRecorder().AnnotatedEventf(component, eventAnnotations, corev1.EventTypeNormal, reason, message)
307321
}
308322

309323
if skipStatusUpdate {
@@ -431,15 +445,13 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
431445
log.V(1).Info("deletion not allowed")
432446
// TODO: have an additional StateDeletionBlocked?
433447
// TODO: eliminate this msg logic
434-
r.client.EventRecorder().Event(component, corev1.EventTypeNormal, readyConditionReasonDeletionBlocked, "Deletion blocked: "+msg)
435448
status.SetState(StateDeleting, readyConditionReasonDeletionBlocked, "Deletion blocked: "+msg)
436449
return ctrl.Result{RequeueAfter: 1*time.Second + r.backoff.Next(req, readyConditionReasonDeletionBlocked)}, nil
437450
}
438451
if len(slices.Remove(component.GetFinalizers(), *r.options.Finalizer)) > 0 {
439452
// deletion is blocked because of foreign finalizers
440453
log.V(1).Info("deleted blocked due to existence of foreign finalizers")
441454
// TODO: have an additional StateDeletionBlocked?
442-
r.client.EventRecorder().Event(component, corev1.EventTypeNormal, readyConditionReasonDeletionBlocked, "Deletion blocked due to existing foreign finalizers")
443455
status.SetState(StateDeleting, readyConditionReasonDeletionBlocked, "Deletion blocked due to existing foreign finalizers")
444456
return ctrl.Result{RequeueAfter: 1*time.Second + r.backoff.Next(req, readyConditionReasonDeletionBlocked)}, nil
445457
}
@@ -578,7 +590,14 @@ func (r *Reconciler[T]) SetupWithManagerAndBuilder(mgr ctrl.Manager, blder *ctrl
578590
if err != nil {
579591
return errors.Wrap(err, "error creating discovery client")
580592
}
581-
r.client = cluster.NewClient(mgr.GetClient(), discoveryClient, mgr.GetEventRecorderFor(r.name))
593+
r.client = cluster.NewClient(mgr.GetClient(), discoveryClient, mgr.GetEventRecorderFor(r.name), mgr.GetConfig(), mgr.GetHTTPClient())
594+
if r.options.NewClient != nil {
595+
clnt, err := r.options.NewClient(r.client)
596+
if err != nil {
597+
return errors.Wrap(err, "error calling custom client constructor")
598+
}
599+
r.client = clnt
600+
}
582601

583602
component := newComponent[T]()
584603
r.groupVersionKind, err = apiutil.GVKForObject(component, r.client.Scheme())
@@ -596,7 +615,7 @@ func (r *Reconciler[T]) SetupWithManagerAndBuilder(mgr ctrl.Manager, blder *ctrl
596615
if r.options.SchemeBuilder != nil {
597616
schemeBuilders = append(schemeBuilders, r.options.SchemeBuilder)
598617
}
599-
r.clients, err = cluster.NewClientFactory(r.name, r.controllerName, mgr.GetConfig(), schemeBuilders)
618+
r.clients, err = clientfactory.NewClientFactory(r.name, r.controllerName, mgr.GetConfig(), schemeBuilders)
600619
if err != nil {
601620
return errors.Wrap(err, "error creating client factory")
602621
}

‎pkg/component/target.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010

1111
"github.com/pkg/errors"
1212

13-
"github.com/sap/component-operator-runtime/internal/cluster"
13+
"github.com/sap/component-operator-runtime/pkg/cluster"
1414
"github.com/sap/component-operator-runtime/pkg/manifests"
1515
"github.com/sap/component-operator-runtime/pkg/reconciler"
1616
)

0 commit comments

Comments
 (0)