Skip to content

Commit 841db34

Browse files
tmshorttmshort
authored
✨ Use error type rather than strings (#878)
* Update error handling in reconciler Separate out resolution errors from generic errors. Signed-off-by: Todd Short <[email protected]> * fixup! Update error handling in reconciler Signed-off-by: Todd Short <[email protected]> --------- Signed-off-by: Todd Short <[email protected]>
1 parent 9f0c6a9 commit 841db34

File tree

3 files changed

+42
-106
lines changed

3 files changed

+42
-106
lines changed

‎internal/controllers/clusterextension_controller.go

+23-101
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,12 @@ import (
3838
"helm.sh/helm/v3/pkg/storage/driver"
3939
corev1 "k8s.io/api/core/v1"
4040
"k8s.io/apimachinery/pkg/api/equality"
41-
apierrors "k8s.io/apimachinery/pkg/api/errors"
4241
apimeta "k8s.io/apimachinery/pkg/api/meta"
4342
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4443
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
4544
"k8s.io/apimachinery/pkg/runtime"
4645
"k8s.io/apimachinery/pkg/runtime/schema"
4746
"k8s.io/apimachinery/pkg/types"
48-
utilerrors "k8s.io/apimachinery/pkg/util/errors"
4947
"k8s.io/apimachinery/pkg/util/sets"
5048
apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
5149
ctrl "sigs.k8s.io/controller-runtime"
@@ -120,40 +118,35 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
120118

121119
var existingExt = &ocv1alpha1.ClusterExtension{}
122120
if err := r.Client.Get(ctx, req.NamespacedName, existingExt); err != nil {
123-
return ctrl.Result{}, utilerrors.NewAggregate([]error{client.IgnoreNotFound(err), nil})
121+
return ctrl.Result{}, client.IgnoreNotFound(err)
124122
}
125123

126-
reconciledExt := existingExt.DeepCopy()
127-
res, reconcileErr := r.reconcile(ctx, reconciledExt)
124+
var updateError error
128125

129-
var updateErrors []error
126+
reconciledExt := existingExt.DeepCopy()
127+
res, err := r.reconcile(ctx, reconciledExt)
128+
updateError = errors.Join(updateError, err)
130129

131130
// Do checks before any Update()s, as Update() may modify the resource structure!
132131
updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status)
133132
updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers)
134133
unexpectedFieldsChanged := checkForUnexpectedFieldChange(*existingExt, *reconciledExt)
135134

136135
if updateStatus {
137-
if updateErr := r.Client.Status().Update(ctx, reconciledExt); updateErr != nil {
138-
updateErrors = append(updateErrors, updateErr)
139-
}
136+
err = r.Client.Status().Update(ctx, reconciledExt)
137+
updateError = errors.Join(updateError, err)
140138
}
141139

142140
if unexpectedFieldsChanged {
143141
panic("spec or metadata changed by reconciler")
144142
}
145143

146144
if updateFinalizers {
147-
if updateErr := r.Client.Update(ctx, reconciledExt); updateErr != nil {
148-
updateErrors = append(updateErrors, updateErr)
149-
}
150-
}
151-
152-
if reconcileErr != nil {
153-
updateErrors = append(updateErrors, reconcileErr)
145+
err = r.Client.Update(ctx, reconciledExt)
146+
updateError = errors.Join(updateError, err)
154147
}
155148

156-
return res, utilerrors.NewAggregate(updateErrors)
149+
return res, updateError
157150
}
158151

159152
// ensureAllConditionsWithReason checks that all defined condition types exist in the given ClusterExtension,
@@ -183,35 +176,6 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool {
183176
return !equality.Semantic.DeepEqual(a, b)
184177
}
185178

186-
func (r *ClusterExtensionReconciler) handleResolutionErrors(ext *ocv1alpha1.ClusterExtension, err error) (ctrl.Result, error) {
187-
var aggErrs utilerrors.Aggregate
188-
if errors.As(err, &aggErrs) {
189-
for _, err := range aggErrs.Errors() {
190-
errorMessage := err.Error()
191-
if strings.Contains(errorMessage, "no package") {
192-
// Handle no package found errors, potentially setting status conditions
193-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
194-
ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage)
195-
} else if strings.Contains(errorMessage, "invalid version range") {
196-
// Handle invalid version range errors, potentially setting status conditions
197-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
198-
ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage)
199-
} else {
200-
// General error handling
201-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
202-
ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "")
203-
}
204-
}
205-
} else {
206-
// If the error is not an aggregate, handle it as a general error
207-
errorMessage := err.Error()
208-
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
209-
ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "")
210-
}
211-
ext.Status.ResolvedBundle = nil
212-
return ctrl.Result{}, err
213-
}
214-
215179
// Helper function to do the actual reconcile
216180
//
217181
// Today we always return ctrl.Result{} and an error.
@@ -232,10 +196,18 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
232196
// run resolution
233197
bundle, err := r.resolve(ctx, *ext)
234198
if err != nil {
235-
return r.handleResolutionErrors(ext, err)
199+
// Note: We don't distinguish between resolution-specific errors and generic errors
200+
ext.Status.ResolvedBundle = nil
201+
ext.Status.InstalledBundle = nil
202+
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
203+
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonResolutionFailed, err.Error())
204+
return ctrl.Result{}, err
236205
}
237206

238207
if err := r.validateBundle(bundle); err != nil {
208+
ext.Status.ResolvedBundle = nil
209+
ext.Status.InstalledBundle = nil
210+
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
239211
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
240212
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
241213
return ctrl.Result{}, err
@@ -291,23 +263,13 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
291263

292264
bundleFS, err := r.Storage.Load(ctx, ext)
293265
if err != nil {
294-
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
295-
Type: ocv1alpha1.TypeHasValidBundle,
296-
Status: metav1.ConditionFalse,
297-
Reason: ocv1alpha1.ReasonBundleLoadFailed,
298-
Message: err.Error(),
299-
})
266+
setHasValidBundleFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
300267
return ctrl.Result{}, err
301268
}
302269

303270
chrt, values, err := r.Handler.Handle(ctx, bundleFS, ext)
304271
if err != nil {
305-
apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
306-
Type: ocv1alpha1.TypeInstalled,
307-
Status: metav1.ConditionFalse,
308-
Reason: ocv1alpha1.ReasonInstallationFailed,
309-
Message: err.Error(),
310-
})
272+
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
311273
return ctrl.Result{}, err
312274
}
313275

@@ -342,26 +304,17 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
342304
return nil
343305
}, helmclient.AppendInstallPostRenderer(post))
344306
if err != nil {
345-
if isResourceNotFoundErr(err) {
346-
err = errRequiredResourceNotFound{err}
347-
}
348307
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err), ext.Generation)
349308
return ctrl.Result{}, err
350309
}
351310
case stateNeedsUpgrade:
352311
rel, err = ac.Upgrade(ext.GetName(), r.ReleaseNamespace, chrt, values, helmclient.AppendUpgradePostRenderer(post))
353312
if err != nil {
354-
if isResourceNotFoundErr(err) {
355-
err = errRequiredResourceNotFound{err}
356-
}
357313
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonUpgradeFailed, err), ext.Generation)
358314
return ctrl.Result{}, err
359315
}
360316
case stateUnchanged:
361317
if err := ac.Reconcile(rel); err != nil {
362-
if isResourceNotFoundErr(err) {
363-
err = errRequiredResourceNotFound{err}
364-
}
365318
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonResolutionFailed, err), ext.Generation)
366319
return ctrl.Result{}, err
367320
}
@@ -414,7 +367,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
414367
func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) {
415368
allBundles, err := r.BundleProvider.Bundles(ctx)
416369
if err != nil {
417-
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("error fetching bundles: %w", err)})
370+
return nil, fmt.Errorf("error fetching bundles: %w", err)
418371
}
419372

420373
packageName := ext.Spec.PackageName
@@ -437,7 +390,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1
437390
if versionRange != "" {
438391
vr, err := mmsemver.NewConstraint(versionRange)
439392
if err != nil {
440-
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("invalid version range '%s': %w", versionRange, err)})
393+
return nil, fmt.Errorf("invalid version range %q: %w", versionRange, err)
441394
}
442395
predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr))
443396
}
@@ -753,37 +706,6 @@ func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, a
753706
return resultSet[0], nil
754707
}
755708

756-
type errRequiredResourceNotFound struct {
757-
error
758-
}
759-
760-
func (err errRequiredResourceNotFound) Error() string {
761-
return fmt.Sprintf("required resource not found: %v", err.error)
762-
}
763-
764-
func isResourceNotFoundErr(err error) bool {
765-
var agg utilerrors.Aggregate
766-
if errors.As(err, &agg) {
767-
for _, err := range agg.Errors() {
768-
return isResourceNotFoundErr(err)
769-
}
770-
}
771-
772-
nkme := &apimeta.NoKindMatchError{}
773-
if errors.As(err, &nkme) {
774-
return true
775-
}
776-
if apierrors.IsNotFound(err) {
777-
return true
778-
}
779-
780-
// TODO: improve NoKindMatchError matching
781-
// An error that is bubbled up from the k8s.io/cli-runtime library
782-
// does not wrap meta.NoKindMatchError, so we need to fallback to
783-
// the use of string comparisons for now.
784-
return strings.Contains(err.Error(), "no matches for kind")
785-
}
786-
787709
type postrenderer struct {
788710
labels map[string]string
789711
cascade postrender.PostRenderer

‎internal/controllers/clusterextension_controller_test.go

+7-4
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ func TestClusterExtensionNonExistentVersion(t *testing.T) {
120120
require.Equal(t, metav1.ConditionFalse, cond.Status)
121121
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
122122
require.Equal(t, fmt.Sprintf(`no package %q matching version "0.50.0" found`, pkgName), cond.Message)
123+
123124
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
124125
require.NotNil(t, cond)
125126
require.Equal(t, metav1.ConditionFalse, cond.Status)
@@ -286,11 +287,11 @@ func TestClusterExtensionVersionNoChannel(t *testing.T) {
286287
require.Equal(t, metav1.ConditionFalse, cond.Status)
287288
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
288289
require.Equal(t, fmt.Sprintf("no package %q matching version %q in channel %q found", pkgName, pkgVer, pkgChan), cond.Message)
289-
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
290290

291+
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
291292
require.NotNil(t, cond)
292293
require.Equal(t, metav1.ConditionFalse, cond.Status)
293-
require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason)
294+
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
294295

295296
verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
296297
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
@@ -334,10 +335,11 @@ func TestClusterExtensionNoChannel(t *testing.T) {
334335
require.Equal(t, metav1.ConditionFalse, cond.Status)
335336
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
336337
require.Equal(t, fmt.Sprintf("no package %q in channel %q found", pkgName, pkgChan), cond.Message)
338+
337339
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
338340
require.NotNil(t, cond)
339341
require.Equal(t, metav1.ConditionFalse, cond.Status)
340-
require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason)
342+
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
341343

342344
verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
343345
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
@@ -383,10 +385,11 @@ func TestClusterExtensionNoVersion(t *testing.T) {
383385
require.Equal(t, metav1.ConditionFalse, cond.Status)
384386
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
385387
require.Equal(t, fmt.Sprintf("no package %q matching version %q in channel %q found", pkgName, pkgVer, pkgChan), cond.Message)
388+
386389
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
387390
require.NotNil(t, cond)
388391
require.Equal(t, metav1.ConditionFalse, cond.Status)
389-
require.Equal(t, ocv1alpha1.ReasonInstallationStatusUnknown, cond.Reason)
392+
require.Equal(t, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
390393

391394
verifyInvariants(ctx, t, reconciler.Client, clusterExtension)
392395
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))

‎internal/controllers/common_controller.go

+12-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func setInstalledStatusConditionUnknown(conditions *[]metav1.Condition, message
5757
})
5858
}
5959

60-
// setHasValidBundleUnknown sets the installed status condition to unknown.
60+
// setHasValidBundleUnknown sets the valid bundle condition to unknown.
6161
func setHasValidBundleUnknown(conditions *[]metav1.Condition, message string, generation int64) {
6262
apimeta.SetStatusCondition(conditions, metav1.Condition{
6363
Type: ocv1alpha1.TypeHasValidBundle,
@@ -68,6 +68,17 @@ func setHasValidBundleUnknown(conditions *[]metav1.Condition, message string, ge
6868
})
6969
}
7070

71+
// setHasValidBundleFalse sets the ivalid bundle condition to false
72+
func setHasValidBundleFailed(conditions *[]metav1.Condition, message string, generation int64) {
73+
apimeta.SetStatusCondition(conditions, metav1.Condition{
74+
Type: ocv1alpha1.TypeHasValidBundle,
75+
Status: metav1.ConditionFalse,
76+
Reason: ocv1alpha1.ReasonBundleLoadFailed,
77+
Message: message,
78+
ObservedGeneration: generation,
79+
})
80+
}
81+
7182
// setResolvedStatusConditionFailed sets the resolved status condition to failed.
7283
func setResolvedStatusConditionFailed(conditions *[]metav1.Condition, message string, generation int64) {
7384
apimeta.SetStatusCondition(conditions, metav1.Condition{

0 commit comments

Comments
 (0)