From 47a441871efbdb79bd92d3f02251a0e2fb673a8e Mon Sep 17 00:00:00 2001 From: Binbin Li Date: Wed, 21 Aug 2024 08:20:07 +0000 Subject: [PATCH] feat: refine the brief error displayed in CR status --- errors/pluginerrors.go | 7 +++ errors/types.go | 12 +++++ errors/types_test.go | 12 +++++ internal/constants/constants.go | 2 +- .../clusterresource/policy_controller.go | 30 ++++++------- .../clusterresource/policy_controller_test.go | 4 +- .../clusterresource/store_controller.go | 18 ++++---- .../clusterresource/store_controller_test.go | 4 +- .../clusterresource/verifier_controller.go | 24 +++++----- .../verifier_controller_test.go | 30 +++++++------ .../certificatestore_controller_test.go | 6 +-- .../namespaceresource/policy_controller.go | 30 ++++++------- .../policy_controller_test.go | 4 +- .../namespaceresource/store_controller.go | 18 ++++---- .../store_controller_test.go | 43 ++++++++++++++++++ .../namespaceresource/verifier_controller.go | 23 +++++----- .../verifier_controller_test.go | 30 +++++++------ .../refresh/kubeRefresh.go | 35 +++++++-------- .../refresh/kubeRefreshNamespaced.go | 34 +++++++------- ..._test.go => kubeRefreshNamespaced_test.go} | 42 +++++++++--------- .../refresh/kubeRefresh_test.go | 44 ++++++++++--------- 21 files changed, 267 insertions(+), 185 deletions(-) rename pkg/keymanagementprovider/refresh/{kubeRefreshNamedspaced_test.go => kubeRefreshNamespaced_test.go} (89%) diff --git a/errors/pluginerrors.go b/errors/pluginerrors.go index 0ec7f4c53..1e44d19d6 100644 --- a/errors/pluginerrors.go +++ b/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.", + }) ) diff --git a/errors/types.go b/errors/types.go index cb5e0311b..0f8f6ea24 100644 --- a/errors/types.go +++ b/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 { diff --git a/errors/types_test.go b/errors/types_test.go index a1f66e388..93a96d962 100644 --- a/errors/types_test.go +++ b/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) diff --git a/internal/constants/constants.go b/internal/constants/constants.go index adda96a6d..d9a6c329c 100644 --- a/internal/constants/constants.go +++ b/internal/constants/constants.go @@ -19,4 +19,4 @@ package constants const RatifyPolicy = "ratify-policy" const EmptyNamespace = "" const NamespaceSeperator = "/" -const MaxBriefErrLength = 30 +const MaxBriefErrLength = 150 diff --git a/pkg/controllers/clusterresource/policy_controller.go b/pkg/controllers/clusterresource/policy_controller.go index 68a62c5a9..3f05be8d2 100644 --- a/pkg/controllers/clusterresource/policy_controller.go +++ b/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 crd.") + 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 } @@ -96,11 +98,11 @@ func policyAddOrReplace(spec configv1beta1.PolicySpec) error { 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) } diff --git a/pkg/controllers/clusterresource/policy_controller_test.go b/pkg/controllers/clusterresource/policy_controller_test.go index 2df24f83c..909c702b2 100644 --- a/pkg/controllers/clusterresource/policy_controller_test.go +++ b/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) }) } } diff --git a/pkg/controllers/clusterresource/store_controller.go b/pkg/controllers/clusterresource/store_controller.go index 54cd91ece..005f121bc 100644 --- a/pkg/controllers/clusterresource/store_controller.go +++ b/pkg/controllers/clusterresource/store_controller.go @@ -25,6 +25,7 @@ import ( "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 +66,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 crd") + 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 @@ -93,17 +95,15 @@ func storeAddOrReplace(spec configv1beta1.StoreSpec, fullname string) error { 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 { diff --git a/pkg/controllers/clusterresource/store_controller_test.go b/pkg/controllers/clusterresource/store_controller_test.go index 69dabca45..7d64416c5 100644 --- a/pkg/controllers/clusterresource/store_controller_test.go +++ b/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) }) } } diff --git a/pkg/controllers/clusterresource/verifier_controller.go b/pkg/controllers/clusterresource/verifier_controller.go index 0eaaa607d..191608c25 100644 --- a/pkg/controllers/clusterresource/verifier_controller.go +++ b/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 crd") + verifierLogger.Error(err) + 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,9 @@ 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) + verifierErr := re.ErrorCodePluginInitFailure.WithError(err).WithDetail("Unable to create verifier from verifier crd.") + logrus.Error(verifierErr) + return verifierErr } return cutils.UpsertVerifier(spec.Version, spec.Address, constants.EmptyNamespace, objectName, verifierConfig) @@ -99,17 +101,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 { diff --git a/pkg/controllers/clusterresource/verifier_controller_test.go b/pkg/controllers/clusterresource/verifier_controller_test.go index 827f3620b..04bf9476b 100644 --- a/pkg/controllers/clusterresource/verifier_controller_test.go +++ b/pkg/controllers/clusterresource/verifier_controller_test.go @@ -23,6 +23,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/verifiers" @@ -189,11 +190,12 @@ func TestVerifier_UpdateAndDelete(t *testing.T) { func TestWriteVerifierStatus(t *testing.T) { logger := logrus.WithContext(context.Background()) testCases := []struct { - name string - isSuccess bool - verifier *configv1beta1.Verifier - errString string - reconciler client.StatusClient + name string + isSuccess bool + verifier *configv1beta1.Verifier + errString string + expectedErrString string + reconciler client.StatusClient }{ { name: "success status", @@ -203,11 +205,12 @@ func TestWriteVerifierStatus(t *testing.T) { reconciler: &mockStatusClient{}, }, { - name: "error status", - isSuccess: false, - verifier: &configv1beta1.Verifier{}, - errString: "a long error string that exceeds the max length of 30 characters", - reconciler: &mockStatusClient{}, + name: "error status", + isSuccess: false, + verifier: &configv1beta1.Verifier{}, + errString: "a long error string that exceeds the max length of 30 characters", + expectedErrString: "UNKNOWN: a long error string that exceeds the max length of 30 characters", + reconciler: &mockStatusClient{}, }, { name: "status update failed", @@ -221,14 +224,15 @@ func TestWriteVerifierStatus(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - writeVerifierStatus(context.Background(), tc.reconciler, tc.verifier, logger, tc.isSuccess, tc.errString) + ratifyErr := re.ErrorCodeUnknown.WithDetail(tc.errString) + writeVerifierStatus(context.Background(), tc.reconciler, tc.verifier, logger, tc.isSuccess, &ratifyErr) if tc.verifier.Status.IsSuccess != tc.isSuccess { t.Fatalf("Expected isSuccess to be %+v , actual %+v", tc.isSuccess, tc.verifier.Status.IsSuccess) } - if tc.verifier.Status.Error != tc.errString { - t.Fatalf("Expected Error to be %+v , actual %+v", tc.errString, tc.verifier.Status.Error) + if tc.verifier.Status.Error != tc.expectedErrString { + t.Fatalf("Expected Error to be %+v , actual %+v", tc.expectedErrString, tc.verifier.Status.Error) } }) } diff --git a/pkg/controllers/namespaceresource/certificatestore_controller_test.go b/pkg/controllers/namespaceresource/certificatestore_controller_test.go index a3e93fb3d..4e9da5189 100644 --- a/pkg/controllers/namespaceresource/certificatestore_controller_test.go +++ b/pkg/controllers/namespaceresource/certificatestore_controller_test.go @@ -17,7 +17,6 @@ package namespaceresource import ( "context" - "fmt" "testing" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" @@ -106,9 +105,8 @@ func TestUpdateErrorStatus(t *testing.T) { if certStore.Status.Error != expectedErr { t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedErr, certStore.Status.Error) } - expectedBriedErr := fmt.Sprintf("%s...", expectedErr[:30]) - if certStore.Status.BriefError != expectedBriedErr { - t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedBriedErr, certStore.Status.Error) + if certStore.Status.BriefError != expectedErr { + t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedErr, certStore.Status.Error) } //make sure properties of last cached cert was not overridden diff --git a/pkg/controllers/namespaceresource/policy_controller.go b/pkg/controllers/namespaceresource/policy_controller.go index d77e492ae..d43407f2c 100644 --- a/pkg/controllers/namespaceresource/policy_controller.go +++ b/pkg/controllers/namespaceresource/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, req.Namespace); 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 crd.") + 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 } @@ -96,11 +98,11 @@ func policyAddOrReplace(spec configv1beta1.NamespacedPolicySpec, namespace strin return nil } -func writePolicyStatus(ctx context.Context, r client.StatusClient, policy *configv1beta1.NamespacedPolicy, logger *logrus.Entry, isSuccess bool, errString string) { +func writePolicyStatus(ctx context.Context, r client.StatusClient, policy *configv1beta1.NamespacedPolicy, 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.NamespacedPolicy) { policy.Status.BriefError = "" } -func updatePolicyErrorStatus(policy *configv1beta1.NamespacedPolicy, errString string) { - briefErr := errString - if len(errString) > constants.MaxBriefErrLength { - briefErr = fmt.Sprintf("%s...", errString[:constants.MaxBriefErrLength]) - } +func updatePolicyErrorStatus(policy *configv1beta1.NamespacedPolicy, 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) } diff --git a/pkg/controllers/namespaceresource/policy_controller_test.go b/pkg/controllers/namespaceresource/policy_controller_test.go index 4dc980e34..abf3b8a2e 100644 --- a/pkg/controllers/namespaceresource/policy_controller_test.go +++ b/pkg/controllers/namespaceresource/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/pkg/controllers" "github.com/ratify-project/ratify/pkg/customresources/policies" _ "github.com/ratify-project/ratify/pkg/policyprovider/configpolicy" @@ -138,7 +139,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) }) } } diff --git a/pkg/controllers/namespaceresource/store_controller.go b/pkg/controllers/namespaceresource/store_controller.go index c021a82ca..af8694274 100644 --- a/pkg/controllers/namespaceresource/store_controller.go +++ b/pkg/controllers/namespaceresource/store_controller.go @@ -25,6 +25,7 @@ import ( "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 +66,13 @@ func (r *StoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl } if err := storeAddOrReplace(store.Spec, resource, req.Namespace); 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(storeErr) + 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 @@ -93,17 +95,15 @@ func storeAddOrReplace(spec configv1beta1.NamespacedStoreSpec, fullname, namespa return utils.UpsertStoreMap(spec.Version, spec.Address, fullname, namespace, storeConfig) } -func writeStoreStatus(ctx context.Context, r client.StatusClient, store *configv1beta1.NamespacedStore, logger *logrus.Entry, isSuccess bool, errorString string) { +func writeStoreStatus(ctx context.Context, r client.StatusClient, store *configv1beta1.NamespacedStore, 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 { diff --git a/pkg/controllers/namespaceresource/store_controller_test.go b/pkg/controllers/namespaceresource/store_controller_test.go index 54ddeb000..270e61d9b 100644 --- a/pkg/controllers/namespaceresource/store_controller_test.go +++ b/pkg/controllers/namespaceresource/store_controller_test.go @@ -20,12 +20,15 @@ import ( "testing" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" + re "github.com/ratify-project/ratify/errors" "github.com/ratify-project/ratify/pkg/controllers" "github.com/ratify-project/ratify/pkg/customresources/referrerstores" test "github.com/ratify-project/ratify/pkg/utils" + "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -107,6 +110,46 @@ func TestStoreAddOrReplace_PluginNotFound(t *testing.T) { } } +func TestWriteStoreStatus(t *testing.T) { + logger := logrus.WithContext(context.Background()) + testCases := []struct { + name string + isSuccess bool + store *configv1beta1.NamespacedStore + errString string + reconciler client.StatusClient + }{ + { + name: "success status", + isSuccess: true, + store: &configv1beta1.NamespacedStore{}, + reconciler: &test.MockStatusClient{}, + }, + { + name: "error status", + isSuccess: false, + store: &configv1beta1.NamespacedStore{}, + errString: "a long error string that exceeds the max length of 30 characters", + reconciler: &test.MockStatusClient{}, + }, + { + name: "status update failed", + isSuccess: true, + store: &configv1beta1.NamespacedStore{}, + reconciler: &test.MockStatusClient{ + UpdateFailed: true, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(_ *testing.T) { + err := re.ErrorCodeUnknown.WithDetail(tc.errString) + writeStoreStatus(context.Background(), tc.reconciler, tc.store, logger, tc.isSuccess, &err) + }) + } +} + func TestStore_UpdateAndDelete(t *testing.T) { resetStoreMap() dirPath, err := test.CreatePlugin(sampleName) diff --git a/pkg/controllers/namespaceresource/verifier_controller.go b/pkg/controllers/namespaceresource/verifier_controller.go index dbbd8ebc7..aac5f26c9 100644 --- a/pkg/controllers/namespaceresource/verifier_controller.go +++ b/pkg/controllers/namespaceresource/verifier_controller.go @@ -17,12 +17,12 @@ package namespaceresource import ( "context" - "fmt" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" "github.com/ratify-project/ratify/internal/constants" "github.com/ratify-project/ratify/pkg/controllers" + re "github.com/ratify-project/ratify/errors" cutils "github.com/ratify-project/ratify/pkg/controllers/utils" "github.com/sirupsen/logrus" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -70,12 +70,12 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } if err := verifierAddOrReplace(verifier.Spec, resource, req.Namespace); 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 crd.") + 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 +85,9 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c func verifierAddOrReplace(spec configv1beta1.NamespacedVerifierSpec, objectName string, namespace 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) + verifierErr := re.ErrorCodePluginInitFailure.WithError(err).WithDetail("Unable to convert crd specification to verifier config.") + logrus.Error(verifierErr) + return verifierErr } return cutils.UpsertVerifier(spec.Version, spec.Address, namespace, 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.NamespacedVerifier, logger *logrus.Entry, isSuccess bool, errorString string) { +func writeVerifierStatus(ctx context.Context, r client.StatusClient, verifier *configv1beta1.NamespacedVerifier, 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 { diff --git a/pkg/controllers/namespaceresource/verifier_controller_test.go b/pkg/controllers/namespaceresource/verifier_controller_test.go index c276a9e93..20904fb34 100644 --- a/pkg/controllers/namespaceresource/verifier_controller_test.go +++ b/pkg/controllers/namespaceresource/verifier_controller_test.go @@ -23,6 +23,7 @@ import ( "testing" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" + re "github.com/ratify-project/ratify/errors" "github.com/ratify-project/ratify/pkg/controllers" "github.com/ratify-project/ratify/pkg/customresources/verifiers" "github.com/ratify-project/ratify/pkg/utils" @@ -191,11 +192,12 @@ func TestVerifier_UpdateAndDelete(t *testing.T) { func TestWriteVerifierStatus(t *testing.T) { logger := logrus.WithContext(context.Background()) testCases := []struct { - name string - isSuccess bool - verifier *configv1beta1.NamespacedVerifier - errString string - reconciler client.StatusClient + name string + isSuccess bool + verifier *configv1beta1.NamespacedVerifier + errString string + expectedErrString string + reconciler client.StatusClient }{ { name: "success status", @@ -205,11 +207,12 @@ func TestWriteVerifierStatus(t *testing.T) { reconciler: &mockStatusClient{}, }, { - name: "error status", - isSuccess: false, - verifier: &configv1beta1.NamespacedVerifier{}, - errString: "a long error string that exceeds the max length of 30 characters", - reconciler: &mockStatusClient{}, + name: "error status", + isSuccess: false, + verifier: &configv1beta1.NamespacedVerifier{}, + errString: "a long error string that exceeds the max length of 30 characters", + expectedErrString: "UNKNOWN: a long error string that exceeds the max length of 30 characters", + reconciler: &mockStatusClient{}, }, { name: "status update failed", @@ -223,14 +226,15 @@ func TestWriteVerifierStatus(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - writeVerifierStatus(context.Background(), tc.reconciler, tc.verifier, logger, tc.isSuccess, tc.errString) + err := re.ErrorCodeUnknown.WithDetail(tc.errString) + writeVerifierStatus(context.Background(), tc.reconciler, tc.verifier, logger, tc.isSuccess, &err) if tc.verifier.Status.IsSuccess != tc.isSuccess { t.Fatalf("Expected isSuccess to be %+v , actual %+v", tc.isSuccess, tc.verifier.Status.IsSuccess) } - if tc.verifier.Status.Error != tc.errString { - t.Fatalf("Expected Error to be %+v , actual %+v", tc.errString, tc.verifier.Status.Error) + if tc.verifier.Status.Error != tc.expectedErrString { + t.Fatalf("Expected Error to be %+v , actual %+v", tc.expectedErrString, tc.verifier.Status.Error) } }) } diff --git a/pkg/keymanagementprovider/refresh/kubeRefresh.go b/pkg/keymanagementprovider/refresh/kubeRefresh.go index 21098d4a5..4ad607137 100644 --- a/pkg/keymanagementprovider/refresh/kubeRefresh.go +++ b/pkg/keymanagementprovider/refresh/kubeRefresh.go @@ -24,6 +24,7 @@ import ( "time" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" + re "github.com/ratify-project/ratify/errors" "github.com/ratify-project/ratify/internal/constants" cutils "github.com/ratify-project/ratify/pkg/controllers/utils" kmp "github.com/ratify-project/ratify/pkg/keymanagementprovider" @@ -88,33 +89,36 @@ func (kr *KubeRefresher) Refresh(ctx context.Context) error { provider, err := cutils.SpecToKeyManagementProvider(keyManagementProvider.Spec.Parameters.Raw, keyManagementProvider.Spec.Type) if err != nil { - writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodePluginInitFailure.WithError(err) + writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, &kmpErr, lastFetchedTime, nil) kr.Request = ctrl.Request{} - return err + return kmpErr } // fetch certificates and store in map certificates, certAttributes, err := provider.GetCertificates(ctx) if err != nil { - writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodeKeyManagementProviderFailure.WithError(err).WithDetail(fmt.Sprintf("Unable to fetch certificates from key management provider: %s of type: %s", resource, keyManagementProvider.Spec.Type)) + writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, &kmpErr, lastFetchedTime, nil) kr.Request = ctrl.Request{} - return fmt.Errorf("error fetching certificates in KMProvider %v with %v provider, error: %w", resource, keyManagementProvider.Spec.Type, err) + return kmpErr } // fetch keys and store in map keys, keyAttributes, err := provider.GetKeys(ctx) if err != nil { - writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodeKeyManagementProviderFailure.WithError(err).WithDetail(fmt.Sprintf("Unable to fetch keys from key management provider: %s of type: %s", resource, keyManagementProvider.Spec.Type)) + + writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, &kmpErr, lastFetchedTime, nil) kr.Request = ctrl.Request{} - return fmt.Errorf("error fetching keys in KMProvider %v with %v provider, error: %w", resource, keyManagementProvider.Spec.Type, err) + return kmpErr } kmp.SetCertificatesInMap(resource, certificates) kmp.SetKeysInMap(resource, keyManagementProvider.Spec.Type, keys) // merge certificates and keys status into one maps.Copy(keyAttributes, certAttributes) isFetchSuccessful = true - emptyErrorString := "" - writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, emptyErrorString, lastFetchedTime, keyAttributes) + writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, nil, lastFetchedTime, keyAttributes) logger.Infof("%v certificate(s) & %v key(s) fetched for key management provider %v", len(certificates), len(keys), resource) @@ -167,11 +171,11 @@ func (kr *KubeRefresher) Create(config map[string]interface{}) (Refresher, error }, nil } -func writeKMProviderStatus(ctx context.Context, r client.StatusClient, keyManagementProvider *configv1beta1.KeyManagementProvider, logger *logrus.Entry, isSuccess bool, errorString string, operationTime metav1.Time, kmProviderStatus kmp.KeyManagementProviderStatus) { +func writeKMProviderStatus(ctx context.Context, r client.StatusClient, keyManagementProvider *configv1beta1.KeyManagementProvider, logger *logrus.Entry, isSuccess bool, err *re.Error, operationTime metav1.Time, kmProviderStatus kmp.KeyManagementProviderStatus) { if isSuccess { updateKMProviderSuccessStatus(keyManagementProvider, &operationTime, kmProviderStatus) } else { - updateKMProviderErrorStatus(keyManagementProvider, errorString, &operationTime) + updateKMProviderErrorStatus(keyManagementProvider, err, &operationTime) } if statusErr := r.Status().Update(ctx, keyManagementProvider); statusErr != nil { logger.Error(statusErr, ",unable to update key management provider error status") @@ -179,15 +183,10 @@ func writeKMProviderStatus(ctx context.Context, r client.StatusClient, keyManage } // updateKMProviderErrorStatus updates the key management provider status with error, brief error and last fetched time -func updateKMProviderErrorStatus(keyManagementProvider *configv1beta1.KeyManagementProvider, errorString string, operationTime *metav1.Time) { - // truncate brief error string to maxBriefErrLength - briefErr := errorString - if len(errorString) > constants.MaxBriefErrLength { - briefErr = fmt.Sprintf("%s...", errorString[:constants.MaxBriefErrLength]) - } +func updateKMProviderErrorStatus(keyManagementProvider *configv1beta1.KeyManagementProvider, err *re.Error, operationTime *metav1.Time) { keyManagementProvider.Status.IsSuccess = false - keyManagementProvider.Status.Error = errorString - keyManagementProvider.Status.BriefError = briefErr + keyManagementProvider.Status.Error = err.Error() + keyManagementProvider.Status.BriefError = err.GetConciseError(constants.MaxBriefErrLength) keyManagementProvider.Status.LastFetchedTime = operationTime } diff --git a/pkg/keymanagementprovider/refresh/kubeRefreshNamespaced.go b/pkg/keymanagementprovider/refresh/kubeRefreshNamespaced.go index ab72e1076..b2db5d094 100644 --- a/pkg/keymanagementprovider/refresh/kubeRefreshNamespaced.go +++ b/pkg/keymanagementprovider/refresh/kubeRefreshNamespaced.go @@ -23,6 +23,7 @@ import ( "time" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" + re "github.com/ratify-project/ratify/errors" "github.com/ratify-project/ratify/internal/constants" cutils "github.com/ratify-project/ratify/pkg/controllers/utils" kmp "github.com/ratify-project/ratify/pkg/keymanagementprovider" @@ -87,33 +88,35 @@ func (kr *KubeRefresherNamespaced) Refresh(ctx context.Context) error { provider, err := cutils.SpecToKeyManagementProvider(keyManagementProvider.Spec.Parameters.Raw, keyManagementProvider.Spec.Type) if err != nil { - writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodePluginInitFailure.WithError(err) + writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, &kmpErr, lastFetchedTime, nil) kr.Result = ctrl.Result{} - return err + return kmpErr } // fetch certificates and store in map certificates, certAttributes, err := provider.GetCertificates(ctx) if err != nil { - writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodeKeyManagementProviderFailure.WithError(err).WithDetail(fmt.Sprintf("Unable to fetch certificates from key management provider: %s of type: %s", resource, keyManagementProvider.Spec.Type)) + writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, &kmpErr, lastFetchedTime, nil) kr.Result = ctrl.Result{} - return fmt.Errorf("error fetching certificates in KMProvider %v with %v provider, error: %w", resource, keyManagementProvider.Spec.Type, err) + return kmpErr } // fetch keys and store in map keys, keyAttributes, err := provider.GetKeys(ctx) if err != nil { - writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodeKeyManagementProviderFailure.WithError(err).WithDetail(fmt.Sprintf("Unable to fetch keys from key management provider: %s of type: %s", resource, keyManagementProvider.Spec.Type)) + writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, &kmpErr, lastFetchedTime, nil) kr.Result = ctrl.Result{} - return fmt.Errorf("error fetching keys in KMProvider %v with %v provider, error: %w", resource, keyManagementProvider.Spec.Type, err) + return kmpErr } kmp.SetCertificatesInMap(resource, certificates) kmp.SetKeysInMap(resource, keyManagementProvider.Spec.Type, keys) // merge certificates and keys status into one maps.Copy(keyAttributes, certAttributes) isFetchSuccessful = true - emptyErrorString := "" - writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, emptyErrorString, lastFetchedTime, keyAttributes) + writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, nil, lastFetchedTime, keyAttributes) logger.Infof("%v certificate(s) & %v key(s) fetched for key management provider %v", len(certificates), len(keys), resource) @@ -165,11 +168,11 @@ func (kr *KubeRefresherNamespaced) Create(config map[string]interface{}) (Refres } // writeKMProviderStatus updates the status of the key management provider resource -func writeKMProviderStatusNamespaced(ctx context.Context, r client.StatusClient, keyManagementProvider *configv1beta1.NamespacedKeyManagementProvider, logger *logrus.Entry, isSuccess bool, errorString string, operationTime metav1.Time, kmProviderStatus kmp.KeyManagementProviderStatus) { +func writeKMProviderStatusNamespaced(ctx context.Context, r client.StatusClient, keyManagementProvider *configv1beta1.NamespacedKeyManagementProvider, logger *logrus.Entry, isSuccess bool, err *re.Error, operationTime metav1.Time, kmProviderStatus kmp.KeyManagementProviderStatus) { if isSuccess { updateKMProviderSuccessStatusNamespaced(keyManagementProvider, &operationTime, kmProviderStatus) } else { - updateKMProviderErrorStatusNamespaced(keyManagementProvider, errorString, &operationTime) + updateKMProviderErrorStatusNamespaced(keyManagementProvider, err, &operationTime) } if statusErr := r.Status().Update(ctx, keyManagementProvider); statusErr != nil { logger.Error(statusErr, ",unable to update key management provider error status") @@ -177,15 +180,10 @@ func writeKMProviderStatusNamespaced(ctx context.Context, r client.StatusClient, } // updateKMProviderErrorStatus updates the key management provider status with error, brief error and last fetched time -func updateKMProviderErrorStatusNamespaced(keyManagementProvider *configv1beta1.NamespacedKeyManagementProvider, errorString string, operationTime *metav1.Time) { - // truncate brief error string to maxBriefErrLength - briefErr := errorString - if len(errorString) > constants.MaxBriefErrLength { - briefErr = fmt.Sprintf("%s...", errorString[:constants.MaxBriefErrLength]) - } +func updateKMProviderErrorStatusNamespaced(keyManagementProvider *configv1beta1.NamespacedKeyManagementProvider, err *re.Error, operationTime *metav1.Time) { keyManagementProvider.Status.IsSuccess = false - keyManagementProvider.Status.Error = errorString - keyManagementProvider.Status.BriefError = briefErr + keyManagementProvider.Status.Error = err.Error() + keyManagementProvider.Status.BriefError = err.GetConciseError(constants.MaxBriefErrLength) keyManagementProvider.Status.LastFetchedTime = operationTime } diff --git a/pkg/keymanagementprovider/refresh/kubeRefreshNamedspaced_test.go b/pkg/keymanagementprovider/refresh/kubeRefreshNamespaced_test.go similarity index 89% rename from pkg/keymanagementprovider/refresh/kubeRefreshNamedspaced_test.go rename to pkg/keymanagementprovider/refresh/kubeRefreshNamespaced_test.go index 884d94fe3..944f54884 100644 --- a/pkg/keymanagementprovider/refresh/kubeRefreshNamedspaced_test.go +++ b/pkg/keymanagementprovider/refresh/kubeRefreshNamespaced_test.go @@ -17,12 +17,12 @@ package refresh import ( "context" - "fmt" "reflect" "testing" "time" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" + re "github.com/ratify-project/ratify/errors" "github.com/ratify-project/ratify/pkg/keymanagementprovider" "github.com/ratify-project/ratify/pkg/keymanagementprovider/mocks" test "github.com/ratify-project/ratify/pkg/utils" @@ -244,20 +244,19 @@ func TestKMProviderUpdateErrorStatusNamespaced(t *testing.T) { keyManagementProvider := configv1beta1.NamespacedKeyManagementProvider{ Status: status, } - expectedErr := "it's a long error from unit test" + expectedErr := re.ErrorCodeUnknown.WithDetail("it's a long error from unit test") lastFetchedTime := metav1.Now() - updateKMProviderErrorStatusNamespaced(&keyManagementProvider, expectedErr, &lastFetchedTime) + updateKMProviderErrorStatusNamespaced(&keyManagementProvider, &expectedErr, &lastFetchedTime) if keyManagementProvider.Status.IsSuccess != false { t.Fatalf("Unexpected error, expected isSuccess to be false , actual %+v", keyManagementProvider.Status.IsSuccess) } - if keyManagementProvider.Status.Error != expectedErr { + if keyManagementProvider.Status.Error != expectedErr.Error() { t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedErr, keyManagementProvider.Status.Error) } - expectedBriedErr := fmt.Sprintf("%s...", expectedErr[:30]) - if keyManagementProvider.Status.BriefError != expectedBriedErr { - t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedBriedErr, keyManagementProvider.Status.Error) + if keyManagementProvider.Status.BriefError != expectedErr.GetConciseError(150) { + t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedErr.GetConciseError(150), keyManagementProvider.Status.Error) } //make sure properties of last cached cert was not overridden @@ -330,11 +329,12 @@ func TestWriteKMProviderStatusNamespaced(t *testing.T) { logger := logrus.WithContext(context.Background()) lastFetchedTime := metav1.Now() testCases := []struct { - name string - isSuccess bool - kmProvider *configv1beta1.NamespacedKeyManagementProvider - errString string - reconciler client.StatusClient + name string + isSuccess bool + kmProvider *configv1beta1.NamespacedKeyManagementProvider + errString string + expectedErrString string + reconciler client.StatusClient }{ { name: "success status", @@ -344,11 +344,12 @@ func TestWriteKMProviderStatusNamespaced(t *testing.T) { reconciler: &test.MockStatusClient{}, }, { - name: "error status", - isSuccess: false, - kmProvider: &configv1beta1.NamespacedKeyManagementProvider{}, - errString: "a long error string that exceeds the max length of 30 characters", - reconciler: &test.MockStatusClient{}, + name: "error status", + isSuccess: false, + kmProvider: &configv1beta1.NamespacedKeyManagementProvider{}, + errString: "a long error string that exceeds the max length of 30 characters", + expectedErrString: "UNKNOWN: a long error string that exceeds the max length of 30 characters", + reconciler: &test.MockStatusClient{}, }, { name: "status update failed", @@ -362,14 +363,15 @@ func TestWriteKMProviderStatusNamespaced(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - writeKMProviderStatusNamespaced(context.Background(), tc.reconciler, tc.kmProvider, logger, tc.isSuccess, tc.errString, lastFetchedTime, nil) + err := re.ErrorCodeUnknown.WithDetail(tc.errString) + writeKMProviderStatusNamespaced(context.Background(), tc.reconciler, tc.kmProvider, logger, tc.isSuccess, &err, lastFetchedTime, nil) if tc.kmProvider.Status.IsSuccess != tc.isSuccess { t.Fatalf("Expected isSuccess to be %+v , actual %+v", tc.isSuccess, tc.kmProvider.Status.IsSuccess) } - if tc.kmProvider.Status.Error != tc.errString { - t.Fatalf("Expected Error to be %+v , actual %+v", tc.errString, tc.kmProvider.Status.Error) + if tc.kmProvider.Status.Error != tc.expectedErrString { + t.Fatalf("Expected Error to be %+v , actual %+v", tc.expectedErrString, tc.kmProvider.Status.Error) } }) } diff --git a/pkg/keymanagementprovider/refresh/kubeRefresh_test.go b/pkg/keymanagementprovider/refresh/kubeRefresh_test.go index beb503753..29319abcd 100644 --- a/pkg/keymanagementprovider/refresh/kubeRefresh_test.go +++ b/pkg/keymanagementprovider/refresh/kubeRefresh_test.go @@ -18,12 +18,12 @@ package refresh import ( "context" - "fmt" "reflect" "testing" "time" configv1beta1 "github.com/ratify-project/ratify/api/v1beta1" + re "github.com/ratify-project/ratify/errors" "github.com/ratify-project/ratify/pkg/keymanagementprovider" _ "github.com/ratify-project/ratify/pkg/keymanagementprovider/inline" "github.com/ratify-project/ratify/pkg/keymanagementprovider/mocks" @@ -246,20 +246,19 @@ func TestKMProviderUpdateErrorStatus(t *testing.T) { keyManagementProvider := configv1beta1.KeyManagementProvider{ Status: status, } - expectedErr := "it's a long error from unit test" + expectedErr := re.ErrorCodeUnknown.WithDetail("it's a long error from unit test") lastFetchedTime := metav1.Now() - updateKMProviderErrorStatus(&keyManagementProvider, expectedErr, &lastFetchedTime) + updateKMProviderErrorStatus(&keyManagementProvider, &expectedErr, &lastFetchedTime) if keyManagementProvider.Status.IsSuccess != false { t.Fatalf("Unexpected error, expected isSuccess to be false , actual %+v", keyManagementProvider.Status.IsSuccess) } - if keyManagementProvider.Status.Error != expectedErr { + if keyManagementProvider.Status.Error != expectedErr.Error() { t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedErr, keyManagementProvider.Status.Error) } - expectedBriedErr := fmt.Sprintf("%s...", expectedErr[:30]) - if keyManagementProvider.Status.BriefError != expectedBriedErr { - t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedBriedErr, keyManagementProvider.Status.Error) + if keyManagementProvider.Status.BriefError != expectedErr.GetConciseError(150) { + t.Fatalf("Unexpected error string, expected %+v, got %+v", expectedErr.GetConciseError(150), keyManagementProvider.Status.Error) } //make sure properties of last cached cert was not overridden @@ -334,11 +333,12 @@ func TestWriteKMProviderStatus(t *testing.T) { logger := logrus.WithContext(context.Background()) lastFetchedTime := metav1.Now() testCases := []struct { - name string - isSuccess bool - kmProvider *configv1beta1.KeyManagementProvider - errString string - reconciler client.StatusClient + name string + isSuccess bool + kmProvider *configv1beta1.KeyManagementProvider + errString string + expectedErrString string + reconciler client.StatusClient }{ { name: "success status", @@ -348,11 +348,12 @@ func TestWriteKMProviderStatus(t *testing.T) { reconciler: &test.MockStatusClient{}, }, { - name: "error status", - isSuccess: false, - kmProvider: &configv1beta1.KeyManagementProvider{}, - errString: "a long error string that exceeds the max length of 30 characters", - reconciler: &test.MockStatusClient{}, + name: "error status", + isSuccess: false, + kmProvider: &configv1beta1.KeyManagementProvider{}, + errString: "a long error string that exceeds the max length of 150 characters", + expectedErrString: "UNKNOWN: a long error string that exceeds the max length of 150 characters", + reconciler: &test.MockStatusClient{}, }, { name: "status update failed", @@ -366,14 +367,15 @@ func TestWriteKMProviderStatus(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - writeKMProviderStatus(context.Background(), tc.reconciler, tc.kmProvider, logger, tc.isSuccess, tc.errString, lastFetchedTime, nil) + err := re.ErrorCodeUnknown.WithDetail(tc.errString) + writeKMProviderStatus(context.Background(), tc.reconciler, tc.kmProvider, logger, tc.isSuccess, &err, lastFetchedTime, nil) if tc.kmProvider.Status.IsSuccess != tc.isSuccess { - t.Fatalf("Expected isSuccess to be %+v , actual %+v", tc.isSuccess, tc.kmProvider.Status.IsSuccess) + t.Fatalf("Expected isSuccess to be: %+v, actual: %+v", tc.isSuccess, tc.kmProvider.Status.IsSuccess) } - if tc.kmProvider.Status.Error != tc.errString { - t.Fatalf("Expected Error to be %+v , actual %+v", tc.errString, tc.kmProvider.Status.Error) + if tc.kmProvider.Status.Error != tc.expectedErrString { + t.Fatalf("Expected Error to be: %+v, actual: %+v", tc.expectedErrString, tc.kmProvider.Status.Error) } }) }