feat: enhance CR status with clearer brief error message (ratify-proj…

…ect#1734)

Signed-off-by: akashsinghal <[email protected]>

errors/pluginerrors.go

@@ -168,4 +168,11 @@ var (
 		Message:     "Key vault operation failed",
 		Description: "Key vault operation failed. Please validate correct key vault configuration is provided or check the error details for further investigation.",
 	})
+
+	// ErrorCodeKeyManagementProviderFailure is returned when a key management provider operation fails.
+	ErrorCodeKeyManagementProviderFailure = Register("errcode", ErrorDescriptor{
+		Value:       "KEY_MANAGEMENT_PROVIDER_FAILURE",
+		Message:     "Key management provider failure",
+		Description: "Generic failure in key management provider. Please validate correct key management provider configuration is provided or check the error details for further investigation.",
+	})
 )

errors/types.go

@@ -253,6 +253,18 @@ func (e Error) GetRemediation() string {
 	return err.remediation
 }
 
+// GetConciseError returns a formatted error message consisting of the error code and reason.
+// If the generated error message exceeds the specified maxLength, it truncates the message and appends an ellipsis ("...").
+// The function ensures that the returned error message is concise and within the length limit.
+func (e Error) GetConciseError(maxLength int) string {
+	err, _ := e.getRootError()
+	errMsg := fmt.Sprintf("%s: %s", err.ErrorCode().Descriptor().Value, e.GetErrorReason())
+	if len(errMsg) > maxLength {
+		return fmt.Sprintf("%s...", errMsg[:maxLength-3])
+	}
+	return errMsg
+}
+
 func (e Error) getRootError() (err Error, details []string) {
 	err = e
 	for !err.isRootError {

errors/types_test.go

@@ -139,6 +139,18 @@ func TestWithDescription(t *testing.T) {
 	}
 }
 
+func TestGetConciseError(t *testing.T) {
+	err := testEC.WithDetail("long message, long message, long message")
+	if err.GetConciseError(30) != "TEST_ERROR_CODE_1: long mes..." {
+		t.Fatalf("expected: TEST_ERROR_CODE_1: long mes..., got: %s", err.GetConciseError(30))
+	}
+
+	err = testEC.WithDetail("short message")
+	if err.GetConciseError(100) != "TEST_ERROR_CODE_1: short message" {
+		t.Fatalf("expected: TEST_ERROR_CODE_1: short message, got: %s", err.GetConciseError(100))
+	}
+}
+
 func TestIs(t *testing.T) {
 	err := testEC.WithDetail(testDetail1)
 	result := err.Is(err)

internal/constants/constants.go

@@ -19,4 +19,4 @@ package constants
 const RatifyPolicy = "ratify-policy"
 const EmptyNamespace = ""
 const NamespaceSeperator = "/"
-const MaxBriefErrLength = 30
+const MaxBriefErrLength = 100

pkg/controllers/clusterresource/policy_controller.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
+	re "github.com/ratify-project/ratify/errors"
 	"github.com/ratify-project/ratify/internal/constants"
 	"github.com/ratify-project/ratify/pkg/controllers"
 	"github.com/ratify-project/ratify/pkg/controllers/utils"
@@ -63,19 +64,20 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	}
 
 	if resource != constants.RatifyPolicy {
-		errStr := fmt.Sprintf("metadata.name must be ratify-policy, got %s", resource)
-		policyLogger.Error(errStr)
-		writePolicyStatus(ctx, r, &policy, policyLogger, false, errStr)
+		err := re.ErrorCodeConfigInvalid.WithDetail(fmt.Sprintf("metadata.name must be ratify-policy, got %s", resource))
+		policyLogger.Error(err)
+		writePolicyStatus(ctx, r, &policy, policyLogger, false, &err)
 		return ctrl.Result{}, nil
 	}
 
 	if err := policyAddOrReplace(policy.Spec); err != nil {
-		policyLogger.Error("unable to create policy from policy crd: ", err)
-		writePolicyStatus(ctx, r, &policy, policyLogger, false, err.Error())
-		return ctrl.Result{}, err
+		policyErr := re.ErrorCodePluginInitFailure.WithError(err).WithDetail("Unable to create policy from policy CR")
+		policyLogger.Error(policyErr)
+		writePolicyStatus(ctx, r, &policy, policyLogger, false, &policyErr)
+		return ctrl.Result{}, policyErr
 	}
 
-	writePolicyStatus(ctx, r, &policy, policyLogger, true, "")
+	writePolicyStatus(ctx, r, &policy, policyLogger, true, nil)
 	return ctrl.Result{}, nil
 }
 
@@ -89,18 +91,18 @@ func (r *PolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func policyAddOrReplace(spec configv1beta1.PolicySpec) error {
 	policyEnforcer, err := utils.SpecToPolicyEnforcer(spec.Parameters.Raw, spec.Type)
 	if err != nil {
-		return fmt.Errorf("failed to create policy enforcer: %w", err)
+		return err
 	}
 
 	controllers.NamespacedPolicies.AddPolicy(constants.EmptyNamespace, constants.RatifyPolicy, policyEnforcer)
 	return nil
 }
 
-func writePolicyStatus(ctx context.Context, r client.StatusClient, policy *configv1beta1.Policy, logger *logrus.Entry, isSuccess bool, errString string) {
+func writePolicyStatus(ctx context.Context, r client.StatusClient, policy *configv1beta1.Policy, logger *logrus.Entry, isSuccess bool, err *re.Error) {
 	if isSuccess {
 		updatePolicySuccessStatus(policy)
 	} else {
-		updatePolicyErrorStatus(policy, errString)
+		updatePolicyErrorStatus(policy, err)
 	}
 	if statusErr := r.Status().Update(ctx, policy); statusErr != nil {
 		logger.Error(statusErr, ", unable to update policy error status")
@@ -113,12 +115,8 @@ func updatePolicySuccessStatus(policy *configv1beta1.Policy) {
 	policy.Status.BriefError = ""
 }
 
-func updatePolicyErrorStatus(policy *configv1beta1.Policy, errString string) {
-	briefErr := errString
-	if len(errString) > constants.MaxBriefErrLength {
-		briefErr = fmt.Sprintf("%s...", errString[:constants.MaxBriefErrLength])
-	}
+func updatePolicyErrorStatus(policy *configv1beta1.Policy, err *re.Error) {
 	policy.Status.IsSuccess = false
-	policy.Status.Error = errString
-	policy.Status.BriefError = briefErr
+	policy.Status.Error = err.Error()
+	policy.Status.BriefError = err.GetConciseError(constants.MaxBriefErrLength)
 }

pkg/controllers/clusterresource/policy_controller_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
+	re "github.com/ratify-project/ratify/errors"
 	"github.com/ratify-project/ratify/internal/constants"
 	"github.com/ratify-project/ratify/pkg/controllers"
 	"github.com/ratify-project/ratify/pkg/customresources/policies"
@@ -110,7 +111,8 @@ func TestWritePolicyStatus(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(_ *testing.T) {
-			writePolicyStatus(context.Background(), tc.reconciler, tc.policy, logger, tc.isSuccess, tc.errString)
+			err := re.ErrorCodeUnknown.WithDetail(tc.errString)
+			writePolicyStatus(context.Background(), tc.reconciler, tc.policy, logger, tc.isSuccess, &err)
 		})
 	}
 }

pkg/controllers/clusterresource/store_controller.go

@@ -17,14 +17,14 @@ package clusterresource
 
 import (
 	"context"
-	"fmt"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
+	re "github.com/ratify-project/ratify/errors"
 	"github.com/ratify-project/ratify/internal/constants"
 	"github.com/ratify-project/ratify/pkg/controllers"
 	"github.com/ratify-project/ratify/pkg/controllers/utils"
@@ -65,12 +65,13 @@ func (r *StoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	}
 
 	if err := storeAddOrReplace(store.Spec, resource); err != nil {
-		storeLogger.Error(err, "unable to create store from store crd")
-		writeStoreStatus(ctx, r, &store, storeLogger, false, err.Error())
-		return ctrl.Result{}, err
+		storeErr := re.ErrorCodeReferrerStoreFailure.WithError(err).WithDetail("Unable to create store from store CR")
+		storeLogger.Error(err)
+		writeStoreStatus(ctx, r, &store, storeLogger, false, &storeErr)
+		return ctrl.Result{}, storeErr
 	}
 
-	writeStoreStatus(ctx, r, &store, storeLogger, true, "")
+	writeStoreStatus(ctx, r, &store, storeLogger, true, nil)
 
 	// returning empty result and no error to indicate we’ve successfully reconciled this object
 	return ctrl.Result{}, nil
@@ -87,23 +88,21 @@ func (r *StoreReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func storeAddOrReplace(spec configv1beta1.StoreSpec, fullname string) error {
 	storeConfig, err := utils.CreateStoreConfig(spec.Parameters.Raw, spec.Name, spec.Source)
 	if err != nil {
-		return fmt.Errorf("unable to convert store spec to store config, err: %w", err)
+		return err
 	}
 
 	return utils.UpsertStoreMap(spec.Version, spec.Address, fullname, constants.EmptyNamespace, storeConfig)
 }
 
-func writeStoreStatus(ctx context.Context, r client.StatusClient, store *configv1beta1.Store, logger *logrus.Entry, isSuccess bool, errorString string) {
+func writeStoreStatus(ctx context.Context, r client.StatusClient, store *configv1beta1.Store, logger *logrus.Entry, isSuccess bool, err *re.Error) {
 	if isSuccess {
 		store.Status.IsSuccess = true
 		store.Status.Error = ""
 		store.Status.BriefError = ""
 	} else {
 		store.Status.IsSuccess = false
-		store.Status.Error = errorString
-		if len(errorString) > constants.MaxBriefErrLength {
-			store.Status.BriefError = fmt.Sprintf("%s...", errorString[:constants.MaxBriefErrLength])
-		}
+		store.Status.Error = err.Error()
+		store.Status.BriefError = err.GetConciseError(constants.MaxBriefErrLength)
 	}
 
 	if statusErr := r.Status().Update(ctx, store); statusErr != nil {

pkg/controllers/clusterresource/store_controller_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
+	re "github.com/ratify-project/ratify/errors"
 	"github.com/ratify-project/ratify/internal/constants"
 	"github.com/ratify-project/ratify/pkg/controllers"
 	rs "github.com/ratify-project/ratify/pkg/customresources/referrerstores"
@@ -120,7 +121,8 @@ func TestWriteStoreStatus(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(_ *testing.T) {
-			writeStoreStatus(context.Background(), tc.reconciler, tc.store, logger, tc.isSuccess, tc.errString)
+			err := re.ErrorCodeUnknown.WithDetail(tc.errString)
+			writeStoreStatus(context.Background(), tc.reconciler, tc.store, logger, tc.isSuccess, &err)
 		})
 	}
 }
@@ -238,6 +240,21 @@ func TestStoreReconcile(t *testing.T) {
 			expectedErr:        true,
 			expectedStoreCount: 0,
 		},
+		{
+			name: "unsupported store",
+			store: &configv1beta1.Store{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: constants.EmptyNamespace,
+					Name:      storeName,
+				},
+				Spec: configv1beta1.StoreSpec{
+					Name:    "unsupported",
+					Address: dirPath,
+				},
+			},
+			expectedErr:        true,
+			expectedStoreCount: 0,
+		},
 	}
 
 	for _, tt := range tests {

pkg/controllers/clusterresource/verifier_controller.go

@@ -17,9 +17,9 @@ package clusterresource
 
 import (
 	"context"
-	"fmt"
 
 	configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
+	re "github.com/ratify-project/ratify/errors"
 	"github.com/ratify-project/ratify/internal/constants"
 	"github.com/ratify-project/ratify/pkg/controllers"
 
@@ -70,12 +70,13 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	}
 
 	if err := verifierAddOrReplace(verifier.Spec, resource); err != nil {
-		verifierLogger.Error(err, "unable to create verifier from verifier crd")
-		writeVerifierStatus(ctx, r, &verifier, verifierLogger, false, err.Error())
-		return ctrl.Result{}, err
+		verifierErr := re.ErrorCodePluginInitFailure.WithError(err).WithDetail("Unable to create verifier from verifier CR")
+		verifierLogger.Error(verifierErr)
+		writeVerifierStatus(ctx, r, &verifier, verifierLogger, false, &verifierErr)
+		return ctrl.Result{}, verifierErr
 	}
 
-	writeVerifierStatus(ctx, r, &verifier, verifierLogger, true, "")
+	writeVerifierStatus(ctx, r, &verifier, verifierLogger, true, nil)
 
 	// returning empty result and no error to indicate we’ve successfully reconciled this object
 	return ctrl.Result{}, nil
@@ -85,8 +86,8 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 func verifierAddOrReplace(spec configv1beta1.VerifierSpec, objectName string) error {
 	verifierConfig, err := cutils.SpecToVerifierConfig(spec.Parameters.Raw, objectName, spec.Name, spec.ArtifactTypes, spec.Source)
 	if err != nil {
-		logrus.Error(err, "unable to convert crd specification to verifier config")
-		return fmt.Errorf("unable to convert crd specification to verifier config, err: %w", err)
+		logrus.Error(err)
+		return err
 	}
 
 	return cutils.UpsertVerifier(spec.Version, spec.Address, constants.EmptyNamespace, objectName, verifierConfig)
@@ -99,17 +100,15 @@ func (r *VerifierReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-func writeVerifierStatus(ctx context.Context, r client.StatusClient, verifier *configv1beta1.Verifier, logger *logrus.Entry, isSuccess bool, errorString string) {
+func writeVerifierStatus(ctx context.Context, r client.StatusClient, verifier *configv1beta1.Verifier, logger *logrus.Entry, isSuccess bool, err *re.Error) {
 	if isSuccess {
 		verifier.Status.IsSuccess = true
 		verifier.Status.Error = ""
 		verifier.Status.BriefError = ""
 	} else {
 		verifier.Status.IsSuccess = false
-		verifier.Status.Error = errorString
-		if len(errorString) > constants.MaxBriefErrLength {
-			verifier.Status.BriefError = fmt.Sprintf("%s...", errorString[:constants.MaxBriefErrLength])
-		}
+		verifier.Status.Error = err.Error()
+		verifier.Status.BriefError = err.GetConciseError(constants.MaxBriefErrLength)
 	}
 
 	if statusErr := r.Status().Update(ctx, verifier); statusErr != nil {