search

kubernetes-sigs / kubebuilder-declarative-pattern

A toolkit for building declarative operators with kubebuilder
Apache License 2.0
256 stars 85 forks source link

Preflight failure leads to panic in Reconciled #232

Closed fsommar closed 1 year ago

fsommar commented 2 years ago

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.

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

k8s-ci-robot commented 1 year ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/issues/232#issuecomment-1364049493): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages issues according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed > >You can: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.