Preflight failure leads to panic in Reconciled

[fsommar]

What happened:

When my Preflight check fails, I get a panic in the Reconciled implementation that I'm using (status.NewAggregator). It panics on the objs.Items access. On a brief inspection, the same seems to hold true for all the other Reconciled implementations too.

What you expected to happen:

No panic, but at least log the error and try to recover.

How to reproduce it (as minimally and precisely as possible):

Create an implementation of declarative.Preflight that returns an error:

type preflight struct {}

func (_ *preflight) Preflight(_ context.Context, _ declarative.DeclarativeObject) error {
    return errors.New("dummy error")
}

In the reconciler setup, use declarative.StatusBuilder along with any of the built-in Reconciled implementations. Let's assume status.NewAggregator:

return watchLabels, r.Reconciler.Init(mgr, &v1beta1.MyResource{}, ..., declarative.WithStatus(&declarative.StatusBuilder{
    PreflightImpl: &preflight{},
    ReconciledImpl: status.NewAggregator(mgr.GetClient()),
})

More details

The current implementation expects manifest.Objects to always be non-nil. What happens when the Preflight check fails is that the deferred function in the reconciler calls Reconciled with nil objects, as that variable is only set after a successful Preflight check.

I don't think there is any sense in omitting the Reconciled call altogether, so another option would be to have all declarative.Reconciled implementations robust in the face of nil manifest.Objects. One could argue that the contract interface already dictates that manifest.Objects can be nil. Or rather, any time the error parameter is passed, the Reconciled implementation should deal with it, either by returning early or executing a second code path.

Workaround

We already have our own Reconciled implementation that delegates to status.NewAggregator, so what we can do for now is return early from there and not call the delegate on error.