Skip to content

Commit ddcee93

Browse files
squakezsquakez
committed
fix(ctrl): don't pass nil pointer to event recorder
Closes #6587
1 parent 3b19de6 commit ddcee93

6 files changed

Lines changed: 222 additions & 53 deletions

File tree

‎pkg/controller/build/build_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ func (r *reconcileBuild) Reconcile(ctx context.Context, request reconcile.Reques
196196

197197
newTarget, err := a.Handle(ctx, target)
198198
if err != nil {
199-
camelevent.NotifyError(r.recorder, &instance, newTarget, instance.Name, instance.Kind, err)
199+
camelevent.NotifyError(r.recorder, &instance, target, instance.Name, instance.Kind, err)
200200

201201
return reconcile.Result{}, err
202202
}

‎pkg/controller/build/build_controller_test.go‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
/*
2+
Licensed to the Apache Software Foundation (ASF) under one or more
3+
contributor license agreements. See the NOTICE file distributed with
4+
this work for additional information regarding copyright ownership.
5+
The ASF licenses this file to You under the Apache License, Version 2.0
6+
(the "License"); you may not use this file except in compliance with
7+
the License. You may obtain a copy of the License at
8+
9+
http://www.apache.org/licenses/LICENSE-2.0
10+
11+
Unless required by applicable law or agreed to in writing, software
12+
distributed under the License is distributed on an "AS IS" BASIS,
13+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
See the License for the specific language governing permissions and
15+
limitations under the License.
16+
*/
17+
18+
package build
19+
20+
import (
21+
"context"
22+
"testing"
23+
24+
v1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1"
25+
"github.com/apache/camel-k/v2/pkg/internal"
26+
"github.com/stretchr/testify/assert"
27+
"github.com/stretchr/testify/require"
28+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
29+
"k8s.io/apimachinery/pkg/types"
30+
"sigs.k8s.io/controller-runtime/pkg/reconcile"
31+
)
32+
33+
func TestReconcileBuild(t *testing.T) {
34+
ctx := context.TODO()
35+
36+
build := &v1.Build{
37+
ObjectMeta: metav1.ObjectMeta{
38+
Name: "test-build",
39+
Namespace: "default",
40+
},
41+
Spec: v1.BuildSpec{
42+
Tasks: []v1.Task{
43+
{
44+
Builder: &v1.BuilderTask{
45+
BaseTask: v1.BaseTask{
46+
Name: "builder",
47+
Configuration: v1.BuildConfiguration{
48+
Strategy: v1.BuildStrategyRoutine,
49+
},
50+
},
51+
},
52+
},
53+
},
54+
},
55+
}
56+
57+
c, err := internal.NewFakeClient(build)
58+
require.NoError(t, err)
59+
60+
r := &reconcileBuild{
61+
client: c,
62+
reader: c,
63+
recorder: &internal.FakeRecorder{},
64+
}
65+
66+
req := reconcile.Request{
67+
NamespacedName: types.NamespacedName{
68+
Name: "test-build",
69+
Namespace: "default",
70+
},
71+
}
72+
73+
result, err := r.Reconcile(ctx, req)
74+
75+
require.NoError(t, err)
76+
assert.Equal(t, reconcile.Result{RequeueAfter: requeueAfterDuration}, result)
77+
78+
var updated v1.Build
79+
err = c.Get(ctx, req.NamespacedName, &updated)
80+
81+
require.NoError(t, err)
82+
assert.NotNil(t, updated.Status, "status should not be nil")
83+
assert.NotEmpty(t, updated.Status.Phase, "phase should be set")
84+
}
85+
86+
func TestReconcileBuildNotifyError(t *testing.T) {
87+
ctx := context.TODO()
88+
89+
build := &v1.Build{
90+
TypeMeta: metav1.TypeMeta{
91+
Kind: string("Build"),
92+
},
93+
ObjectMeta: metav1.ObjectMeta{
94+
Name: "test-build",
95+
Namespace: "default",
96+
},
97+
Spec: v1.BuildSpec{
98+
Tasks: []v1.Task{
99+
{
100+
Builder: &v1.BuilderTask{
101+
BaseTask: v1.BaseTask{
102+
Name: "builder",
103+
Configuration: v1.BuildConfiguration{
104+
Strategy: v1.BuildStrategyPod,
105+
},
106+
},
107+
},
108+
},
109+
},
110+
},
111+
Status: v1.BuildStatus{
112+
Phase: v1.BuildPhasePending,
113+
},
114+
}
115+
116+
c, err := internal.NewFakeClient(build)
117+
require.NoError(t, err)
118+
recorder := &internal.FakeRecorder{}
119+
r := &reconcileBuild{
120+
client: c,
121+
reader: c,
122+
recorder: recorder,
123+
}
124+
125+
req := reconcile.Request{
126+
NamespacedName: types.NamespacedName{
127+
Name: "test-build",
128+
Namespace: "default",
129+
},
130+
}
131+
132+
result, err := r.Reconcile(ctx, req)
133+
134+
require.Error(t, err)
135+
assert.Equal(t, reconcile.Result{}, result)
136+
137+
var updated v1.Build
138+
err = c.Get(ctx, req.NamespacedName, &updated)
139+
140+
require.NoError(t, err)
141+
assert.NotNil(t, updated.Status, "status should not be nil")
142+
assert.NotEmpty(t, updated.Status.Phase, "phase should be set")
143+
assert.True(t, recorder.Called)
144+
}

‎pkg/controller/catalog/catalog_controller.go‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,6 @@ func (r *reconcileCatalog) Reconcile(ctx context.Context, request reconcile.Requ
156156
}
157157

158158
var targetPhase v1.CamelCatalogPhase
159-
var err error
160159

161160
target := instance.DeepCopy()
162161
targetLog := rlog.ForCatalog(target)
@@ -172,30 +171,30 @@ func (r *reconcileCatalog) Reconcile(ctx context.Context, request reconcile.Requ
172171
targetLog.Infof("Invoking action %s", a.Name())
173172

174173
phaseFrom := target.Status.Phase
175-
target, err = a.Handle(ctx, target)
174+
newTarget, err := a.Handle(ctx, target)
176175

177176
if err != nil {
178177
camelevent.NotifyError(r.recorder, &instance, target, instance.Name, instance.Kind, err)
179178

180179
return reconcile.Result{}, err
181180
}
182181

183-
if target != nil {
184-
target.Status.ObservedGeneration = instance.GetGeneration()
182+
if newTarget != nil {
183+
newTarget.Status.ObservedGeneration = instance.GetGeneration()
185184

186-
if err := r.client.Status().Patch(ctx, target, ctrl.MergeFrom(&instance)); err != nil {
187-
camelevent.NotifyError(r.recorder, &instance, target, target.Name, target.Kind, err)
185+
if err := r.client.Status().Patch(ctx, newTarget, ctrl.MergeFrom(&instance)); err != nil {
186+
camelevent.NotifyError(r.recorder, &instance, newTarget, newTarget.Name, newTarget.Kind, err)
188187

189188
return reconcile.Result{}, err
190189
}
191190

192-
targetPhase = target.Status.Phase
191+
targetPhase = newTarget.Status.Phase
193192

194193
if targetPhase != phaseFrom {
195194
targetLog.Info(
196195
"State transition",
197196
"phase-from", phaseFrom,
198-
"phase-to", target.Status.Phase,
197+
"phase-to", newTarget.Status.Phase,
199198
)
200199
}
201200
}

‎pkg/controller/integrationplatform/integrationplatform_controller.go‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,6 @@ func (r *reconcileIntegrationPlatform) Reconcile(ctx context.Context, request re
153153
}
154154

155155
var targetPhase v1.IntegrationPlatformPhase
156-
var err error
157156

158157
target := instance.DeepCopy()
159158
targetLog := rlog.ForIntegrationPlatform(target)
@@ -170,29 +169,29 @@ func (r *reconcileIntegrationPlatform) Reconcile(ctx context.Context, request re
170169

171170
phaseFrom := target.Status.Phase
172171

173-
target, err = a.Handle(ctx, target)
172+
newTarget, err := a.Handle(ctx, target)
174173
if err != nil {
175174
camelevent.NotifyError(r.recorder, &instance, target, instance.Name, instance.Kind, err)
176175

177176
return reconcile.Result{}, err
178177
}
179178

180-
if target != nil {
181-
target.Status.ObservedGeneration = instance.Generation
179+
if newTarget != nil {
180+
newTarget.Status.ObservedGeneration = instance.Generation
182181

183-
if err := r.client.Status().Patch(ctx, target, ctrl.MergeFrom(&instance)); err != nil {
184-
camelevent.NotifyError(r.recorder, &instance, target, target.Name, target.Kind, err)
182+
if err := r.client.Status().Patch(ctx, newTarget, ctrl.MergeFrom(&instance)); err != nil {
183+
camelevent.NotifyError(r.recorder, &instance, newTarget, newTarget.Name, newTarget.Kind, err)
185184

186185
return reconcile.Result{}, err
187186
}
188187

189-
targetPhase = target.Status.Phase
188+
targetPhase = newTarget.Status.Phase
190189

191190
if targetPhase != phaseFrom {
192191
targetLog.Info(
193192
"State transition",
194193
"phase-from", phaseFrom,
195-
"phase-to", target.Status.Phase,
194+
"phase-to", newTarget.Status.Phase,
196195
)
197196
}
198197
}

‎pkg/event/manager_test.go‎

Lines changed: 14 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -21,33 +21,11 @@ import (
2121
"fmt"
2222
"testing"
2323

24+
"github.com/apache/camel-k/v2/pkg/internal"
2425
corev1 "k8s.io/api/core/v1"
2526
"k8s.io/apimachinery/pkg/runtime"
2627
)
2728

28-
// fakeRecorder implements events.EventRecorder and captures calls.
29-
type fakeRecorder struct {
30-
called bool
31-
obj runtime.Object
32-
eventtype string
33-
reason string
34-
action string
35-
message string
36-
}
37-
38-
func (f *fakeRecorder) Eventf(obj runtime.Object, old runtime.Object, eventtype, reason, action, note string, args ...interface{}) {
39-
f.called = true
40-
f.obj = obj
41-
f.eventtype = eventtype
42-
f.reason = reason
43-
f.action = action
44-
f.message = fmt.Sprintf(note, args...)
45-
}
46-
47-
// Unused but required to satisfy interface
48-
func (f *fakeRecorder) Event(obj runtime.Object, old runtime.Object, eventtype, reason, action, note string) {
49-
}
50-
5129
func TestNotifyError(t *testing.T) {
5230
err := fmt.Errorf("boom")
5331

@@ -86,37 +64,37 @@ func TestNotifyError(t *testing.T) {
8664

8765
for _, tt := range tests {
8866
t.Run(tt.name, func(t *testing.T) {
89-
rec := &fakeRecorder{}
67+
rec := &internal.FakeRecorder{}
9068

9169
NotifyError(rec, tt.old, tt.new, "my-name", "MyKind", err)
9270

93-
if tt.expectCalled != rec.called {
94-
t.Fatalf("expected called=%v, got %v", tt.expectCalled, rec.called)
71+
if tt.expectCalled != rec.Called {
72+
t.Fatalf("expected called=%v, got %v", tt.expectCalled, rec.Called)
9573
}
9674

9775
if !tt.expectCalled {
9876
return
9977
}
10078

101-
if rec.obj != tt.expectedObj {
102-
t.Errorf("expected object %v, got %v", tt.expectedObj, rec.obj)
79+
if rec.Obj != tt.expectedObj {
80+
t.Errorf("expected object %v, got %v", tt.expectedObj, rec.Obj)
10381
}
10482

105-
if rec.eventtype != corev1.EventTypeWarning {
106-
t.Errorf("expected event type %s, got %s", corev1.EventTypeWarning, rec.eventtype)
83+
if rec.Eventtype != corev1.EventTypeWarning {
84+
t.Errorf("expected event type %s, got %s", corev1.EventTypeWarning, rec.Eventtype)
10785
}
10886

109-
if rec.reason != "MyKindError" {
110-
t.Errorf("unexpected reason: %s", rec.reason)
87+
if rec.Reason != "MyKindError" {
88+
t.Errorf("unexpected reason: %s", rec.Reason)
11189
}
11290

113-
if rec.action != "MyKindReconciliation" {
114-
t.Errorf("unexpected action: %s", rec.action)
91+
if rec.Action != "MyKindReconciliation" {
92+
t.Errorf("unexpected action: %s", rec.Action)
11593
}
11694

11795
expectedMsg := "Cannot reconcile MyKind my-name: boom"
118-
if rec.message != expectedMsg {
119-
t.Errorf("expected message %q, got %q", expectedMsg, rec.message)
96+
if rec.Message != expectedMsg {
97+
t.Errorf("expected message %q, got %q", expectedMsg, rec.Message)
12098
}
12199
})
122100
}

‎pkg/internal/fakeRecorder.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/*
2+
Licensed to the Apache Software Foundation (ASF) under one or more
3+
contributor license agreements. See the NOTICE file distributed with
4+
this work for additional information regarding copyright ownership.
5+
The ASF licenses this file to You under the Apache License, Version 2.0
6+
(the "License"); you may not use this file except in compliance with
7+
the License. You may obtain a copy of the License at
8+
9+
http://www.apache.org/licenses/LICENSE-2.0
10+
11+
Unless required by applicable law or agreed to in writing, software
12+
distributed under the License is distributed on an "AS IS" BASIS,
13+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
See the License for the specific language governing permissions and
15+
limitations under the License.
16+
*/
17+
18+
package internal
19+
20+
import (
21+
"fmt"
22+
23+
"k8s.io/apimachinery/pkg/runtime"
24+
"k8s.io/apimachinery/pkg/runtime/schema"
25+
)
26+
27+
// FakeRecorder implements events.EventRecorder and captures calls.
28+
type FakeRecorder struct {
29+
Called bool
30+
Obj runtime.Object
31+
Eventtype string
32+
Reason string
33+
Action string
34+
Message string
35+
Kind schema.ObjectKind
36+
}
37+
38+
func (f *FakeRecorder) Eventf(obj runtime.Object, old runtime.Object, eventtype, reason, action, note string, args ...any) {
39+
f.Called = true
40+
f.Obj = obj
41+
f.Eventtype = eventtype
42+
f.Reason = reason
43+
f.Action = action
44+
f.Message = fmt.Sprintf(note, args...)
45+
f.Kind = obj.GetObjectKind()
46+
}
47+
48+
func (f *FakeRecorder) Event(obj runtime.Object, old runtime.Object, eventtype, reason, action, note string) {
49+
}

0 commit comments

Comments
 (0)