Skip to content

Commit e776324

Browse files
negznegz
authored
Merge pull request crossplane#6395 from negz/watchful
2 parents 5ce2644 + 48290e8 commit e776324

File tree

14 files changed

+250
-272
lines changed

14 files changed

+250
-272
lines changed

‎internal/controller/apiextensions/composite/api.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -128,13 +128,13 @@ func (a *APIFilteredSecretPublisher) UnpublishConnection(_ context.Context, _ re
128128
// for compatibility with existing Composition logic while CompositionRevisions
129129
// are in alpha.
130130
type APIRevisionFetcher struct {
131-
ca resource.ClientApplicator
131+
client client.Client
132132
}
133133

134134
// NewAPIRevisionFetcher returns a RevisionFetcher that fetches the
135135
// Revision referenced by a composite resource.
136-
func NewAPIRevisionFetcher(ca resource.ClientApplicator) *APIRevisionFetcher {
137-
return &APIRevisionFetcher{ca: ca}
136+
func NewAPIRevisionFetcher(c client.Client) *APIRevisionFetcher {
137+
return &APIRevisionFetcher{client: c}
138138
}
139139

140140
// Fetch the appropriate CompositionRevision for the supplied XR. Panics if the
@@ -148,15 +148,15 @@ func (f *APIRevisionFetcher) Fetch(ctx context.Context, cr resource.Composite) (
148148
// Just fetch and return the selected revision.
149149
if current != nil && pol != nil && *pol == xpv1.UpdateManual {
150150
rev := &v1.CompositionRevision{}
151-
err := f.ca.Get(ctx, types.NamespacedName{Name: current.Name}, rev)
151+
err := f.client.Get(ctx, types.NamespacedName{Name: current.Name}, rev)
152152
return rev, errors.Wrap(err, errGetCompositionRevision)
153153
}
154154

155155
// We either haven't yet selected a revision, or our update policy is
156156
// automatic. Either way we need to determine the latest revision.
157157

158158
comp := &v1.Composition{}
159-
if err := f.ca.Get(ctx, meta.NamespacedNameOf(cr.GetCompositionReference()), comp); err != nil {
159+
if err := f.client.Get(ctx, meta.NamespacedNameOf(cr.GetCompositionReference()), comp); err != nil {
160160
return nil, errors.Wrap(err, errGetComposition)
161161
}
162162

@@ -172,7 +172,7 @@ func (f *APIRevisionFetcher) Fetch(ctx context.Context, cr resource.Composite) (
172172

173173
if current == nil || current.Name != latest.GetName() {
174174
cr.SetCompositionRevisionReference(&corev1.LocalObjectReference{Name: latest.GetName()})
175-
if err := f.ca.Apply(ctx, cr); err != nil {
175+
if err := f.client.Update(ctx, cr); err != nil {
176176
return nil, errors.Wrap(err, errUpdate)
177177
}
178178
}
@@ -190,7 +190,7 @@ func (f *APIRevisionFetcher) getCompositionRevisionList(ctx context.Context, cr
190190
}
191191

192192
ml[v1.LabelCompositionName] = comp.GetName()
193-
if err := f.ca.List(ctx, rl, ml); err != nil {
193+
if err := f.client.List(ctx, rl, ml); err != nil {
194194
return nil, errors.Wrap(err, errListCompositionRevisions)
195195
}
196196
return rl, nil

‎internal/controller/apiextensions/composite/api_test.go‎

Lines changed: 87 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -213,15 +213,15 @@ func TestFetchRevision(t *testing.T) {
213213

214214
cases := map[string]struct {
215215
reason string
216-
client resource.ClientApplicator
216+
client client.Client
217217
args args
218218
want want
219219
}{
220220
"GetCompositionRevisionError": {
221221
reason: "We should wrap and return errors encountered getting the CompositionRevision.",
222-
client: resource.ClientApplicator{Client: &test.MockClient{
222+
client: &test.MockClient{
223223
MockGet: test.NewMockGetFn(errBoom),
224-
}},
224+
},
225225
args: args{
226226
cr: &fake.Composite{
227227
CompositionRevisionReferencer: fake.CompositionRevisionReferencer{Ref: &corev1.LocalObjectReference{}},
@@ -235,12 +235,12 @@ func TestFetchRevision(t *testing.T) {
235235
},
236236
"UpdateManual": {
237237
reason: "When we're using the manual update policy and a revision reference is set we should return that revision as a composition.",
238-
client: resource.ClientApplicator{Client: &test.MockClient{
238+
client: &test.MockClient{
239239
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
240240
*obj.(*v1.CompositionRevision) = *rev3
241241
return nil
242242
}),
243-
}},
243+
},
244244
args: args{
245245
cr: &fake.Composite{
246246
CompositionRevisionReferencer: fake.CompositionRevisionReferencer{Ref: &corev1.LocalObjectReference{}},
@@ -253,9 +253,9 @@ func TestFetchRevision(t *testing.T) {
253253
},
254254
"GetCompositionError": {
255255
reason: "We should wrap and return errors encountered getting the Composition.",
256-
client: resource.ClientApplicator{Client: &test.MockClient{
256+
client: &test.MockClient{
257257
MockGet: test.NewMockGetFn(errBoom),
258-
}},
258+
},
259259
args: args{
260260
cr: &fake.Composite{
261261
CompositionReferencer: fake.CompositionReferencer{Ref: &corev1.ObjectReference{}},
@@ -267,10 +267,10 @@ func TestFetchRevision(t *testing.T) {
267267
},
268268
"ListCompositionRevisionsError": {
269269
reason: "We should wrap and return errors encountered listing CompositionRevisions.",
270-
client: resource.ClientApplicator{Client: &test.MockClient{
270+
client: &test.MockClient{
271271
MockGet: test.NewMockGetFn(nil),
272272
MockList: test.NewMockListFn(errBoom),
273-
}},
273+
},
274274
args: args{
275275
cr: &fake.Composite{
276276
CompositionReferencer: fake.CompositionReferencer{Ref: &corev1.ObjectReference{}},
@@ -282,10 +282,10 @@ func TestFetchRevision(t *testing.T) {
282282
},
283283
"NoCompositionRevisionsError": {
284284
reason: "We should return an error if we don't find any suitable CompositionRevisions.",
285-
client: resource.ClientApplicator{Client: &test.MockClient{
285+
client: &test.MockClient{
286286
MockGet: test.NewMockGetFn(nil),
287287
MockList: test.NewMockListFn(nil),
288-
}},
288+
},
289289
args: args{
290290
cr: &fake.Composite{
291291
CompositionReferencer: fake.CompositionReferencer{Ref: &corev1.ObjectReference{}},
@@ -297,33 +297,31 @@ func TestFetchRevision(t *testing.T) {
297297
},
298298
"AlreadyAtLatestRevision": {
299299
reason: "We should return the latest revision without updating our reference if we already reference it.",
300-
client: resource.ClientApplicator{
301-
Client: &test.MockClient{
302-
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
303-
*obj.(*v1.Composition) = *comp
304-
return nil
305-
}),
306-
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
307-
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
308-
Items: []v1.CompositionRevision{
309-
// We should ignore this revision because it does not have
310-
// our composition above as its controller reference.
311-
*rev3,
312-
313-
// This revision is owned by our composition, and is the
314-
// latest revision.
315-
*rev2,
316-
317-
// This revision is owned by our composition, but is not the
318-
// latest revision.
319-
*rev1,
320-
},
321-
}
322-
return nil
323-
}),
324-
},
300+
client: &test.MockClient{
301+
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
302+
*obj.(*v1.Composition) = *comp
303+
return nil
304+
}),
305+
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
306+
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
307+
Items: []v1.CompositionRevision{
308+
// We should ignore this revision because it does not have
309+
// our composition above as its controller reference.
310+
*rev3,
311+
312+
// This revision is owned by our composition, and is the
313+
// latest revision.
314+
*rev2,
315+
316+
// This revision is owned by our composition, but is not the
317+
// latest revision.
318+
*rev1,
319+
},
320+
}
321+
return nil
322+
}),
325323
// This should not be called.
326-
Applicator: resource.ApplyFn(func(_ context.Context, _ client.Object, _ ...resource.ApplyOption) error { return errBoom }),
324+
MockUpdate: test.NewMockUpdateFn(errBoom),
327325
},
328326
args: args{
329327
cr: &fake.Composite{
@@ -342,25 +340,22 @@ func TestFetchRevision(t *testing.T) {
342340
},
343341
"NoRevisionSet": {
344342
reason: "We should return the latest revision and update our reference if none is set.",
345-
client: resource.ClientApplicator{
346-
Client: &test.MockClient{
347-
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
348-
*obj.(*v1.Composition) = *comp
349-
return nil
350-
}),
351-
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
352-
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
353-
Items: []v1.CompositionRevision{
354-
// This revision is owned by our composition, and is the
355-
// latest revision.
356-
*rev2,
357-
},
358-
}
359-
return nil
360-
}),
361-
},
362-
Applicator: resource.ApplyFn(func(_ context.Context, o client.Object, _ ...resource.ApplyOption) error {
363-
// Ensure we were updated to reference the latest CompositionRevision.
343+
client: &test.MockClient{
344+
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
345+
*obj.(*v1.Composition) = *comp
346+
return nil
347+
}),
348+
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
349+
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
350+
Items: []v1.CompositionRevision{
351+
// This revision is owned by our composition, and is the
352+
// latest revision.
353+
*rev2,
354+
},
355+
}
356+
return nil
357+
}),
358+
MockUpdate: test.NewMockUpdateFn(nil, func(obj client.Object) error {
364359
want := &fake.Composite{
365360
CompositionReferencer: fake.CompositionReferencer{
366361
Ref: &corev1.ObjectReference{Name: comp.GetName()},
@@ -372,7 +367,7 @@ func TestFetchRevision(t *testing.T) {
372367
},
373368
CompositionUpdater: fake.CompositionUpdater{Policy: &manual},
374369
}
375-
if diff := cmp.Diff(want, o); diff != "" {
370+
if diff := cmp.Diff(want, obj); diff != "" {
376371
t.Errorf("Apply(): -want, +got: %s", diff)
377372
}
378373
return nil
@@ -394,27 +389,25 @@ func TestFetchRevision(t *testing.T) {
394389
},
395390
"OutdatedRevisionSet": {
396391
reason: "We should return the latest revision and update our reference if an outdated revision is referenced.",
397-
client: resource.ClientApplicator{
398-
Client: &test.MockClient{
399-
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
400-
*obj.(*v1.Composition) = *comp
401-
return nil
402-
}),
403-
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
404-
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
405-
Items: []v1.CompositionRevision{
406-
// This revision is owned by our composition, and is the
407-
// latest revision.
408-
*rev2,
409-
410-
// This is an outdated revision.
411-
*rev1,
412-
},
413-
}
414-
return nil
415-
}),
416-
},
417-
Applicator: resource.ApplyFn(func(_ context.Context, o client.Object, _ ...resource.ApplyOption) error {
392+
client: &test.MockClient{
393+
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
394+
*obj.(*v1.Composition) = *comp
395+
return nil
396+
}),
397+
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
398+
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
399+
Items: []v1.CompositionRevision{
400+
// This revision is owned by our composition, and is the
401+
// latest revision.
402+
*rev2,
403+
404+
// This is an outdated revision.
405+
*rev1,
406+
},
407+
}
408+
return nil
409+
}),
410+
MockUpdate: test.NewMockUpdateFn(nil, func(obj client.Object) error {
418411
// Ensure we were updated to reference the latest CompositionRevision.
419412
want := &fake.Composite{
420413
CompositionReferencer: fake.CompositionReferencer{
@@ -426,7 +419,7 @@ func TestFetchRevision(t *testing.T) {
426419
},
427420
},
428421
}
429-
if diff := cmp.Diff(want, o); diff != "" {
422+
if diff := cmp.Diff(want, obj); diff != "" {
430423
t.Errorf("Apply(): -want, +got: %s", diff)
431424
}
432425
return nil
@@ -451,26 +444,22 @@ func TestFetchRevision(t *testing.T) {
451444
},
452445
"SetRevisionError": {
453446
reason: "We should return the latest revision and update our reference if none is set.",
454-
client: resource.ClientApplicator{
455-
Client: &test.MockClient{
456-
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
457-
*obj.(*v1.Composition) = *comp
458-
return nil
459-
}),
460-
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
461-
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
462-
Items: []v1.CompositionRevision{
463-
// This revision is owned by our composition, and is the
464-
// latest revision.
465-
*rev2,
466-
},
467-
}
468-
return nil
469-
}),
470-
},
471-
Applicator: resource.ApplyFn(func(_ context.Context, _ client.Object, _ ...resource.ApplyOption) error {
472-
return errBoom
447+
client: &test.MockClient{
448+
MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
449+
*obj.(*v1.Composition) = *comp
450+
return nil
451+
}),
452+
MockList: test.NewMockListFn(nil, func(obj client.ObjectList) error {
453+
*obj.(*v1.CompositionRevisionList) = v1.CompositionRevisionList{
454+
Items: []v1.CompositionRevision{
455+
// This revision is owned by our composition, and is the
456+
// latest revision.
457+
*rev2,
458+
},
459+
}
460+
return nil
473461
}),
462+
MockUpdate: test.NewMockUpdateFn(errBoom),
474463
},
475464
args: args{
476465
cr: &fake.Composite{

‎internal/controller/apiextensions/composite/reconciler.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -355,21 +355,23 @@ func (fn CompositionRevisionValidatorFn) Validate(c *v1.CompositionRevision) err
355355
// start watches when they compose new kinds of resources.
356356
type WatchStarter interface {
357357
// StartWatches starts the supplied watches, if they're not running already.
358-
StartWatches(name string, ws ...engine.Watch) error
358+
StartWatches(ctx context.Context, name string, ws ...engine.Watch) error
359359
}
360360

361361
// A NopWatchStarter does nothing.
362362
type NopWatchStarter struct{}
363363

364364
// StartWatches does nothing.
365-
func (n *NopWatchStarter) StartWatches(_ string, _ ...engine.Watch) error { return nil }
365+
func (n *NopWatchStarter) StartWatches(_ context.Context, _ string, _ ...engine.Watch) error {
366+
return nil
367+
}
366368

367369
// A WatchStarterFn is a function that can start a new watch.
368-
type WatchStarterFn func(name string, ws ...engine.Watch) error
370+
type WatchStarterFn func(ctx context.Context, name string, ws ...engine.Watch) error
369371

370372
// StartWatches starts the supplied watches, if they're not running already.
371-
func (fn WatchStarterFn) StartWatches(name string, ws ...engine.Watch) error {
372-
return fn(name, ws...)
373+
func (fn WatchStarterFn) StartWatches(ctx context.Context, name string, ws ...engine.Watch) error {
374+
return fn(ctx, name, ws...)
373375
}
374376

375377
type compositeResource struct {
@@ -519,6 +521,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
519521

520522
orig := xr.GetCompositionReference()
521523
if err := r.composite.SelectComposition(ctx, xr); err != nil {
524+
if kerrors.IsConflict(err) {
525+
return reconcile.Result{Requeue: true}, nil
526+
}
522527
err = errors.Wrap(err, errSelectComp)
523528
r.record.Event(xr, event.Warning(reasonResolve, err))
524529
xr.SetConditions(xpv1.ReconcileError(err))
@@ -532,6 +537,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
532537
origRev := xr.GetCompositionRevisionReference()
533538
rev, err := r.revision.Fetch(ctx, xr)
534539
if err != nil {
540+
if kerrors.IsConflict(err) {
541+
return reconcile.Result{Requeue: true}, nil
542+
}
535543
log.Debug(errFetchComp, "error", err)
536544
err = errors.Wrap(err, errFetchComp)
537545
r.record.Event(xr, event.Warning(reasonCompose, err))
@@ -542,8 +550,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
542550
r.record.Event(xr, event.Normal(reasonResolve, fmt.Sprintf("Selected composition revision: %s", rev.Name)))
543551
}
544552

545-
// TODO(negz): Update this to validate the revision? In practice that's what
546-
// it's doing today when revis are enabled.
547553
if err := r.revision.Validate(rev); err != nil {
548554
log.Debug(errValidate, "error", err)
549555
err = errors.Wrap(err, errValidate)
@@ -610,7 +616,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
610616
// StartWatches is a no-op unless the realtime compositions feature flag is
611617
// enabled. When the flag is enabled, the ControllerEngine that starts this
612618
// controller also starts a garbage collector for its watches.
613-
if err := r.engine.StartWatches(r.controllerName, ws...); err != nil {
619+
if err := r.engine.StartWatches(ctx, r.controllerName, ws...); err != nil {
614620
// TODO(negz): If we stop polling this will be a more serious error.
615621
log.Debug("Cannot start watches for composed resources. Relying on polling to know when they change.", "controller-name", r.controllerName, "error", err)
616622
}

0 commit comments

Comments
 (0)