Skip to content

Commit a72317d

Browse files
weyfonkweyfonk
authored
Migrate events generation to new interface (#4678) (#4804)
* Migrate events generation to new interface With `GetEentRecorderFor` being deprecated, controllers now use the new `GetEventRecorder` method, returning an `events.EventRecorder` instead of a `record.EventRecorder` interface. * Fix expectations on source component When using the new event recorder interface, the recorder does not populate the source component anymore; instead, the emitter can be checked through `ReportingController`. Besides, job creation and deletion actually happen _twice_ when re-creating a job. * Use formatting placeholders for errors in events This should prevent events from containing malformed messages, if error messages ever contain `%` symbols.
1 parent ee17a7c commit a72317d

17 files changed

Lines changed: 231 additions & 107 deletions

File tree

‎integrationtests/gitjob/controller/controller_test.go‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -438,15 +438,15 @@ var _ = Describe("GitJob controller", func() {
438438
g.Expect(events.Items[0].Reason).To(Equal("GotNewCommit"))
439439
g.Expect(events.Items[0].Message).To(Equal("9ca3a0ad308ed8bffa6602572e2a1343af9c3d2e"))
440440
g.Expect(events.Items[0].Type).To(Equal("Normal"))
441-
g.Expect(events.Items[0].Source.Component).To(Equal("gitjob-controller"))
441+
g.Expect(events.Items[0].ReportingController).To(Equal("gitjob-controller"))
442442
g.Expect(events.Items[1].Reason).To(Equal("Created"))
443443
g.Expect(events.Items[1].Message).To(Equal("GitJob was created"))
444+
g.Expect(events.Items[1].ReportingController).To(Equal("gitjob-controller"))
444445
g.Expect(events.Items[1].Type).To(Equal("Normal"))
445-
g.Expect(events.Items[1].Source.Component).To(Equal("gitjob-controller"))
446446
g.Expect(events.Items[2].Reason).To(Equal("JobDeleted"))
447447
g.Expect(events.Items[2].Message).To(Equal("job deletion triggered because job succeeded"))
448448
g.Expect(events.Items[2].Type).To(Equal("Normal"))
449-
g.Expect(events.Items[2].Source.Component).To(Equal("gitjob-controller"))
449+
g.Expect(events.Items[2].ReportingController).To(Equal("gitjob-controller"))
450450
}).Should(Succeed())
451451

452452
// job should not be present
@@ -706,30 +706,40 @@ var _ = Describe("GitJob controller", func() {
706706

707707
return string(job.UID) != string(newJob.UID)
708708
}).Should(BeTrue())
709-
// it should log 3 events
709+
// it should log 5 events:
710710
// first one is to log the new commit from the poller
711711
// second one is to inform that the job was created
712712
// third one reports on the job being deleted because of ForceUpdateGeneration
713+
// the fourth and fifth ones represent job re-creation and deletion after successful completion,
714+
// respectively
713715
Eventually(func(g Gomega) {
714716
events, _ := k8sClientSet.CoreV1().Events(gitRepo.Namespace).List(context.TODO(),
715717
metav1.ListOptions{
716718
FieldSelector: "involvedObject.name=force-deletion",
717719
TypeMeta: metav1.TypeMeta{Kind: "GitRepo"},
718720
})
719721
g.Expect(events).ToNot(BeNil())
720-
g.Expect(events.Items).To(HaveLen(3))
722+
g.Expect(events.Items).To(HaveLen(5))
723+
724+
for _, e := range events.Items {
725+
g.Expect(e.ReportingController).To(Equal("gitjob-controller"))
726+
g.Expect(e.Type).To(Equal("Normal"))
727+
}
728+
721729
g.Expect(events.Items[0].Reason).To(Equal("GotNewCommit"))
722730
g.Expect(events.Items[0].Message).To(Equal("9ca3a0ad308ed8bffa6602572e2a1343af9c3d2e"))
723-
g.Expect(events.Items[0].Type).To(Equal("Normal"))
724-
g.Expect(events.Items[0].Source.Component).To(Equal("gitjob-controller"))
731+
725732
g.Expect(events.Items[1].Reason).To(Equal("Created"))
726733
g.Expect(events.Items[1].Message).To(Equal("GitJob was created"))
727-
g.Expect(events.Items[1].Type).To(Equal("Normal"))
728-
g.Expect(events.Items[1].Source.Component).To(Equal("gitjob-controller"))
734+
729735
g.Expect(events.Items[2].Reason).To(Equal("JobDeleted"))
730736
g.Expect(events.Items[2].Message).To(Equal("job deletion triggered because job succeeded"))
731-
g.Expect(events.Items[2].Type).To(Equal("Normal"))
732-
g.Expect(events.Items[2].Source.Component).To(Equal("gitjob-controller"))
737+
738+
g.Expect(events.Items[3].Reason).To(Equal("Created"))
739+
g.Expect(events.Items[3].Message).To(Equal("GitJob was created"))
740+
741+
g.Expect(events.Items[4].Reason).To(Equal("JobDeleted"))
742+
g.Expect(events.Items[4].Message).To(Equal("job deletion triggered because job succeeded"))
733743
}).Should(Succeed())
734744
})
735745

‎integrationtests/gitjob/controller/suite_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ var _ = BeforeSuite(func() {
166166
Scheduler: sched,
167167
GitFetcher: fetcherMock,
168168
Clock: reconciler.RealClock{},
169-
Recorder: mgr.GetEventRecorderFor("gitjob-controller"),
169+
Recorder: mgr.GetEventRecorder("gitjob-controller"),
170170
Workers: 50,
171171
SystemNamespace: "default",
172172
KnownHosts: ssh.KnownHosts{},

‎integrationtests/helmops/controller/suite_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ var _ = BeforeSuite(func() {
8888
err = (&reconciler.HelmOpReconciler{
8989
Client: mgr.GetClient(),
9090
Scheme: mgr.GetScheme(),
91-
Recorder: mgr.GetEventRecorderFor("helmops-controller"),
91+
Recorder: mgr.GetEventRecorder("helmops-controller"),
9292
Scheduler: sched,
9393
Workers: 50,
9494
}).SetupWithManager(mgr)

‎internal/cmd/controller/gitops/operator.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ func (g *GitOperator) Run(cmd *cobra.Command, args []string) error {
186186
JobNodeSelector: g.ShardNodeSelector,
187187
GitFetcher: &git.Fetch{KnownHosts: kh},
188188
Clock: reconciler.RealClock{},
189-
Recorder: mgr.GetEventRecorderFor(fmt.Sprintf("fleet-gitops%s", shardIDSuffix)),
189+
Recorder: mgr.GetEventRecorder(fmt.Sprintf("fleet-gitops%s", shardIDSuffix)),
190190
SystemNamespace: namespace,
191191
KnownHosts: kh,
192192
WithImagescan: imagescanEnabled,

‎internal/cmd/controller/gitops/reconciler/gitjob.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
ssh "github.com/rancher/fleet/internal/ssh"
2020
v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
2121
"github.com/rancher/fleet/pkg/cert"
22-
fleetevent "github.com/rancher/fleet/pkg/event"
2322
"github.com/rancher/fleet/pkg/sharding"
2423

2524
appsv1 "k8s.io/api/apps/v1"
@@ -70,7 +69,14 @@ func (r *GitJobReconciler) createJobAndResources(ctx context.Context, gitrepo *v
7069
return fmt.Errorf("error creating git job: %w", err)
7170
}
7271

73-
r.Recorder.Event(gitrepo, fleetevent.Normal, "Created", "GitJob was created")
72+
r.Recorder.Eventf(
73+
gitrepo,
74+
nil,
75+
corev1.EventTypeNormal,
76+
"Created",
77+
"CreateGitJob",
78+
"GitJob was created",
79+
)
7480
return nil
7581
}
7682

‎internal/cmd/controller/gitops/reconciler/gitjob_controller.go‎

Lines changed: 64 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
"github.com/rancher/fleet/internal/metrics"
2525
v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
2626
"github.com/rancher/fleet/pkg/durations"
27-
fleetevent "github.com/rancher/fleet/pkg/event"
2827
"github.com/rancher/fleet/pkg/sharding"
2928

3029
"github.com/rancher/wrangler/v3/pkg/condition"
@@ -41,7 +40,7 @@ import (
4140
"k8s.io/apimachinery/pkg/runtime"
4241
"k8s.io/apimachinery/pkg/types"
4342
errutil "k8s.io/apimachinery/pkg/util/errors"
44-
"k8s.io/client-go/tools/record"
43+
"k8s.io/client-go/tools/events"
4544
"k8s.io/client-go/util/retry"
4645
"sigs.k8s.io/cli-utils/pkg/kstatus/status"
4746
ctrl "sigs.k8s.io/controller-runtime"
@@ -141,7 +140,7 @@ type GitJobReconciler struct {
141140
JobNodeSelector string
142141
GitFetcher GitFetcher
143142
Clock TimeGetter
144-
Recorder record.EventRecorder
143+
Recorder events.EventRecorder
145144
SystemNamespace string
146145
KnownHosts KnownHostsGetter
147146
WithImagescan bool
@@ -202,7 +201,16 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
202201
// Restrictions / Overrides, gitrepo reconciler is responsible for setting error in status
203202
oldStatus := gitrepo.Status.DeepCopy()
204203
if err := AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo); err != nil {
205-
r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedToApplyRestrictions", err.Error())
204+
r.Recorder.Eventf(
205+
gitrepo,
206+
nil,
207+
corev1.EventTypeWarning,
208+
"FailedToApplyRestrictions",
209+
"ApplyGitRepoRestrictions",
210+
"%v",
211+
err,
212+
)
213+
206214
return ctrl.Result{}, updateErrorStatus(ctx, r.Client, req.NamespacedName, *oldStatus, err)
207215
}
208216

@@ -306,15 +314,31 @@ func (r *GitJobReconciler) manageGitJob(ctx context.Context, logger logr.Logger,
306314
}, &job)
307315
if err != nil && !apierrors.IsNotFound(err) {
308316
err = fmt.Errorf("error retrieving git job: %w", err)
309-
r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedToGetGitJob", err.Error())
317+
r.Recorder.Eventf(
318+
gitrepo,
319+
nil,
320+
corev1.EventTypeWarning,
321+
"FailedToGetGitJob",
322+
"GetGitJob",
323+
"%v",
324+
err,
325+
)
310326

311327
return ctrl.Result{}, err
312328
}
313329

314330
if apierrors.IsNotFound(err) {
315331
clientSecretChanged, helmSecretChanged, err := r.hasReferencedSecretChanged(ctx, gitrepo)
316332
if err != nil {
317-
r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedValidatingSecret", err.Error())
333+
r.Recorder.Eventf(
334+
gitrepo,
335+
nil,
336+
corev1.EventTypeWarning,
337+
"FailedValidatingSecret",
338+
"ValidateSecret",
339+
"%v",
340+
err,
341+
)
318342
return ctrl.Result{}, fmt.Errorf("error validating external secrets: %w", err)
319343
}
320344

@@ -333,16 +357,39 @@ func (r *GitJobReconciler) manageGitJob(ctx context.Context, logger logr.Logger,
333357
gitrepo.Status.Commit = commit
334358
}
335359
if err != nil {
336-
r.Recorder.Event(gitrepo, fleetevent.Warning, "Failed", err.Error())
360+
r.Recorder.Eventf(
361+
gitrepo,
362+
nil,
363+
corev1.EventTypeWarning,
364+
"Failed",
365+
"MonitorLatestCommit",
366+
"%v",
367+
err,
368+
)
337369
} else if oldCommit != gitrepo.Status.Commit {
338-
r.Recorder.Event(gitrepo, fleetevent.Normal, "GotNewCommit", gitrepo.Status.Commit)
370+
r.Recorder.Eventf(
371+
gitrepo,
372+
nil,
373+
corev1.EventTypeNormal,
374+
"GotNewCommit",
375+
"GetNewCommit",
376+
gitrepo.Status.Commit,
377+
)
339378
}
340379
}
341380

342381
if r.shouldCreateJob(gitrepo, oldCommit, helmSecretChanged) {
343382
r.updateGenerationValuesIfNeeded(gitrepo)
344383
if err := r.validateExternalSecretExist(ctx, gitrepo); err != nil {
345-
r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedValidatingSecret", err.Error())
384+
r.Recorder.Eventf(
385+
gitrepo,
386+
nil,
387+
corev1.EventTypeWarning,
388+
"FailedValidatingSecret",
389+
"ValidateSecret",
390+
"%v",
391+
err,
392+
)
346393
return ctrl.Result{}, fmt.Errorf("error validating external secrets: %w", err)
347394
}
348395
if err := r.createJobAndResources(ctx, gitrepo, logger); err != nil {
@@ -581,7 +628,14 @@ func (r *GitJobReconciler) deleteJobIfNeeded(ctx context.Context, gitRepo *v1alp
581628
if err := r.Delete(ctx, job, client.PropagationPolicy(metav1.DeletePropagationBackground)); err != nil && !apierrors.IsNotFound(err) {
582629
return err, false
583630
}
584-
r.Recorder.Event(gitRepo, fleetevent.Normal, "JobDeleted", jobDeletedMessage)
631+
r.Recorder.Eventf(
632+
gitRepo,
633+
nil,
634+
corev1.EventTypeNormal,
635+
"JobDeleted",
636+
"DeleteJob",
637+
jobDeletedMessage,
638+
)
585639
}
586640

587641
// finally if there's a job and any of the secrets related to the gitrepo changed,

‎internal/cmd/controller/gitops/reconciler/gitjob_test.go‎

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
//go:generate mockgen --build_flags=--mod=mod -destination=../../../../mocks/client_mock.go -package=mocks -mock_names=Client=MockK8sClient,SubResourceWriter=MockStatusWriter sigs.k8s.io/controller-runtime/pkg/client Client,SubResourceWriter
2+
//go:generate mockgen --build_flags=--mod=mod -destination=../../../../mocks/eventrecorder_mock.go -package=mocks k8s.io/client-go/tools/events EventRecorder
23

34
package reconciler
45

@@ -22,7 +23,6 @@ import (
2223
"github.com/rancher/wrangler/v3/pkg/genericcondition"
2324
"go.uber.org/mock/gomock"
2425

25-
fleetevent "github.com/rancher/fleet/pkg/event"
2626
appsv1 "k8s.io/api/apps/v1"
2727
batchv1 "k8s.io/api/batch/v1"
2828
corev1 "k8s.io/api/core/v1"
@@ -85,6 +85,24 @@ func (m gitRepoPointerMatcher) String() string {
8585
return ""
8686
}
8787

88+
// errorMatcher implements a gomock matcher on error message strings.
89+
type errorMatcher struct {
90+
errMsg string
91+
}
92+
93+
func (m errorMatcher) Matches(x interface{}) bool {
94+
err, ok := x.(error)
95+
if !ok {
96+
return false
97+
}
98+
99+
return err.Error() == m.errMsg
100+
}
101+
102+
func (m errorMatcher) String() string {
103+
return fmt.Sprintf("matches error %q", m.errMsg)
104+
}
105+
88106
func TestReconcile_Error_WhenGitrepoRestrictionsAreNotMet(t *testing.T) {
89107
mockCtrl := gomock.NewController(t)
90108
defer mockCtrl.Finish()
@@ -129,11 +147,14 @@ func TestReconcile_Error_WhenGitrepoRestrictionsAreNotMet(t *testing.T) {
129147
)
130148

131149
recorderMock := mocks.NewMockEventRecorder(mockCtrl)
132-
recorderMock.EXPECT().Event(
150+
recorderMock.EXPECT().Eventf(
133151
&gitRepoMatcher{gitRepo},
134-
fleetevent.Warning,
152+
nil,
153+
corev1.EventTypeWarning,
135154
"FailedToApplyRestrictions",
136-
"empty targetNamespace denied, because allowedTargetNamespaces restriction is present",
155+
"ApplyGitRepoRestrictions",
156+
"%v",
157+
errorMatcher{"empty targetNamespace denied, because allowedTargetNamespaces restriction is present"},
137158
)
138159

139160
r := GitJobReconciler{
@@ -204,11 +225,14 @@ func TestReconcile_Error_WhenGetGitJobErrors(t *testing.T) {
204225

205226
recorderMock := mocks.NewMockEventRecorder(mockCtrl)
206227

207-
recorderMock.EXPECT().Event(
228+
recorderMock.EXPECT().Eventf(
208229
&gitRepoMatcher{gitRepo},
209-
fleetevent.Warning,
230+
nil,
231+
corev1.EventTypeWarning,
210232
"FailedToGetGitJob",
211-
"error retrieving git job: GITJOB ERROR",
233+
"GetGitJob",
234+
"%v",
235+
errorMatcher{"error retrieving git job: GITJOB ERROR"},
212236
)
213237

214238
r := GitJobReconciler{
@@ -275,11 +299,14 @@ func TestReconcile_Error_WhenSecretDoesNotExist(t *testing.T) {
275299

276300
recorderMock := mocks.NewMockEventRecorder(mockCtrl)
277301

278-
recorderMock.EXPECT().Event(
302+
recorderMock.EXPECT().Eventf(
279303
&gitRepoMatcher{gitRepo},
280-
fleetevent.Warning,
304+
nil,
305+
corev1.EventTypeWarning,
281306
"FailedValidatingSecret",
282-
"failed to look up HelmSecretNameForPaths, error: SECRET ERROR",
307+
"ValidateSecret",
308+
"%v",
309+
errorMatcher{"failed to look up HelmSecretNameForPaths, error: SECRET ERROR"},
283310
)
284311

285312
statusClient := mocks.NewMockStatusWriter(mockCtrl)

0 commit comments

Comments
 (0)