diff --git a/.github/workflows/build-pr.yml b/.github/workflows/build-pr.yml index 18ab101d4..0af3612f8 100644 --- a/.github/workflows/build-pr.yml +++ b/.github/workflows/build-pr.yml @@ -37,7 +37,7 @@ jobs: strategy: fail-fast: false matrix: - KUBERNETES_VERSION: ["1.28.7", "1.29.2"] + KUBERNETES_VERSION: ["1.28.12", "1.29.2"] GATEKEEPER_VERSION: ["3.14.0", "3.15.0", "3.16.0"] uses: ./.github/workflows/e2e-k8s.yml with: @@ -53,7 +53,7 @@ jobs: strategy: fail-fast: false matrix: - KUBERNETES_VERSION: ["1.27.9", "1.29.2"] + KUBERNETES_VERSION: ["1.28.12", "1.29.2"] GATEKEEPER_VERSION: ["3.14.0", "3.15.0", "3.16.0"] uses: ./.github/workflows/e2e-aks.yml with: diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 47aeb4672..82ddd78ea 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -38,7 +38,7 @@ jobs: with: go-version: "1.22" - name: Initialize CodeQL - uses: github/codeql-action/init@2c779ab0d087cd7fe7b826087247c2c81f27bfa6 # tag=v3.26.5 + uses: github/codeql-action/init@4dd16135b69a43b6c8efb853346f8437d92d3c93 # tag=v3.26.6 with: languages: go - name: Run tidy @@ -46,4 +46,4 @@ jobs: - name: Build CLI run: make build - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@2c779ab0d087cd7fe7b826087247c2c81f27bfa6 # tag=v3.26.5 + uses: github/codeql-action/analyze@4dd16135b69a43b6c8efb853346f8437d92d3c93 # tag=v3.26.6 diff --git a/.github/workflows/e2e-aks.yml b/.github/workflows/e2e-aks.yml index 30dced1e8..a202673ea 100644 --- a/.github/workflows/e2e-aks.yml +++ b/.github/workflows/e2e-aks.yml @@ -64,7 +64,7 @@ jobs: make e2e-aks KUBERNETES_VERSION=${{ inputs.k8s_version }} GATEKEEPER_VERSION=${{ inputs.gatekeeper_version }} TENANT_ID=${{ secrets.AZURE_TENANT_ID }} AZURE_SP_OBJECT_ID=${{ secrets.AZURE_SP_OBJECT_ID }} - name: Upload artifacts - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # v4.3.6 + uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 if: ${{ always() }} with: name: e2e-logs-aks-${{ inputs.k8s_version }}-${{ inputs.gatekeeper_version }} diff --git a/.github/workflows/e2e-k8s.yml b/.github/workflows/e2e-k8s.yml index 06d97d5ef..57a04b69a 100644 --- a/.github/workflows/e2e-k8s.yml +++ b/.github/workflows/e2e-k8s.yml @@ -65,7 +65,7 @@ jobs: kubectl logs -n gatekeeper-system -l app=ratify --tail=-1 > logs-ratify-preinstall-${{ matrix.KUBERNETES_VERSION }}-${{ matrix.GATEKEEPER_VERSION }}-rego-policy.json kubectl logs -n gatekeeper-system -l app.kubernetes.io/name=ratify --tail=-1 > logs-ratify-${{ matrix.KUBERNETES_VERSION }}-${{ matrix.GATEKEEPER_VERSION }}-rego-policy.json - name: Upload artifacts - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # v4.3.6 + uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 if: ${{ always() }} with: name: e2e-logs-${{ inputs.k8s_version }}-${{ inputs.gatekeeper_version }} diff --git a/.github/workflows/high-availability.yml b/.github/workflows/high-availability.yml index 09d3fe236..e711f94f8 100644 --- a/.github/workflows/high-availability.yml +++ b/.github/workflows/high-availability.yml @@ -60,7 +60,7 @@ jobs: kubectl logs -n gatekeeper-system -l app=ratify --tail=-1 > logs-ratify-preinstall-${{ matrix.DAPR_VERSION }}.json kubectl logs -n gatekeeper-system -l app.kubernetes.io/name=ratify --tail=-1 > logs-ratify-${{ matrix.DAPR_VERSION }}.json - name: Upload artifacts - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # v4.3.6 + uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 if: ${{ always() }} with: name: e2e-logs-${{ matrix.DAPR_VERSION }} diff --git a/.github/workflows/quick-start.yml b/.github/workflows/quick-start.yml index ca981e2b5..6e26af5cb 100644 --- a/.github/workflows/quick-start.yml +++ b/.github/workflows/quick-start.yml @@ -59,7 +59,7 @@ jobs: kubectl logs -n gatekeeper-system -l app=ratify --tail=-1 > logs-ratify-preinstall-${{ matrix.KUBERNETES_VERSION }}-config-policy.json kubectl logs -n gatekeeper-system -l app.kubernetes.io/name=ratify --tail=-1 > logs-ratify-${{ matrix.KUBERNETES_VERSION }}-config-policy.json - name: Upload artifacts - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # v4.3.6 + uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 if: ${{ always() }} with: name: e2e-logs-${{ matrix.KUBERNETES_VERSION }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 80e317a9d..13f91853e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -49,7 +49,7 @@ jobs: $RUNNER_TEMP/sbom-tool generate -b . -bc . -pn ratify -pv $GITHUB_REF_NAME -ps Microsoft -nsb https://microsoft.com -V Verbose - name: Upload a Build Artifact - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # tag=v4.3.6 + uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # tag=v4.4.0 with: name: SBOM SPDX files path: _manifest/spdx_2.2/** diff --git a/.github/workflows/run-full-validation.yml b/.github/workflows/run-full-validation.yml index a3ad7f595..71db17f86 100644 --- a/.github/workflows/run-full-validation.yml +++ b/.github/workflows/run-full-validation.yml @@ -26,7 +26,7 @@ jobs: strategy: fail-fast: false matrix: - KUBERNETES_VERSION: ["1.28.7", "1.29.2"] + KUBERNETES_VERSION: ["1.28.12", "1.29.2"] GATEKEEPER_VERSION: ["3.14.0", "3.15.0", "3.16.0"] uses: ./.github/workflows/e2e-k8s.yml with: @@ -41,7 +41,7 @@ jobs: strategy: fail-fast: false matrix: - KUBERNETES_VERSION: ["1.27.9", "1.29.2"] + KUBERNETES_VERSION: ["1.28.12", "1.29.2"] GATEKEEPER_VERSION: ["3.14.0", "3.15.0", "3.16.0"] uses: ./.github/workflows/e2e-aks.yml with: diff --git a/.github/workflows/scorecards.yml b/.github/workflows/scorecards.yml index 1c282693e..970cd1182 100644 --- a/.github/workflows/scorecards.yml +++ b/.github/workflows/scorecards.yml @@ -48,13 +48,13 @@ jobs: publish_results: true - name: "Upload artifact" - uses: actions/upload-artifact@834a144ee995460fba8ed112a2fc961b36a5ec5a # tag=v4.3.6 + uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # tag=v4.4.0 with: name: SARIF file path: results.sarif retention-days: 5 - name: "Upload to code-scanning" - uses: github/codeql-action/upload-sarif@2c779ab0d087cd7fe7b826087247c2c81f27bfa6 # tag=v3.26.5 + uses: github/codeql-action/upload-sarif@4dd16135b69a43b6c8efb853346f8437d92d3c93 # tag=v3.26.6 with: sarif_file: results.sarif diff --git a/charts/ratify/templates/NOTES.txt b/charts/ratify/templates/NOTES.txt new file mode 100644 index 000000000..d3ea52406 --- /dev/null +++ b/charts/ratify/templates/NOTES.txt @@ -0,0 +1,6 @@ +{{- if not (or .Values.notation.enabled .Values.cosign.enabled .Values.sbom.enabled .Values.vulnerabilityreport.enabled) }} +*********************************************************** +WARNING: All verifiers are disabled. +It's recommended that at least one is enabled for proper functionality. +*********************************************************** +{{- end }} \ No newline at end of file diff --git a/charts/ratify/templates/verifier.yaml b/charts/ratify/templates/verifier.yaml index 2c7556ab1..8ac23e5d8 100644 --- a/charts/ratify/templates/verifier.yaml +++ b/charts/ratify/templates/verifier.yaml @@ -1,4 +1,5 @@ {{- $fullname := include "ratify.fullname" . -}} +{{- if .Values.notation.enabled }} apiVersion: config.ratify.deislabs.io/v1beta1 kind: Verifier metadata: @@ -37,6 +38,7 @@ spec: - ca:certs trustedIdentities: - "*" +{{- end }} --- {{- if .Values.cosign.enabled }} apiVersion: config.ratify.deislabs.io/v1beta1 diff --git a/charts/ratify/values.yaml b/charts/ratify/values.yaml index 7e501bc74..f1206dc10 100644 --- a/charts/ratify/values.yaml +++ b/charts/ratify/values.yaml @@ -12,6 +12,9 @@ tolerations: [] notationCerts: [] cosignKeys: [] +notation: + enabled: true + cosign: enabled: true scopes: ["*"] # corresponds to a single trust policy 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/go.mod b/go.mod index 3b59e5ba2..e9f40c2d0 100644 --- a/go.mod +++ b/go.mod @@ -14,8 +14,8 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.6.0 github.com/AzureAD/microsoft-authentication-library-for-go v1.2.2 github.com/aws/aws-sdk-go-v2 v1.30.4 - github.com/aws/aws-sdk-go-v2/config v1.27.30 - github.com/aws/aws-sdk-go-v2/credentials v1.17.29 + github.com/aws/aws-sdk-go-v2/config v1.27.31 + github.com/aws/aws-sdk-go-v2/credentials v1.17.30 github.com/aws/aws-sdk-go-v2/service/ecr v1.28.6 github.com/cespare/xxhash/v2 v2.3.0 github.com/dapr/go-sdk v1.8.0 diff --git a/go.sum b/go.sum index 619fba4db..35927879f 100644 --- a/go.sum +++ b/go.sum @@ -127,10 +127,10 @@ github.com/aws/aws-sdk-go v1.51.6 h1:Ld36dn9r7P9IjU8WZSaswQ8Y/XUCRpewim5980DwYiU github.com/aws/aws-sdk-go v1.51.6/go.mod h1:LF8svs817+Nz+DmiMQKTO3ubZ/6IaTpq3TjupRn3Eqk= github.com/aws/aws-sdk-go-v2 v1.30.4 h1:frhcagrVNrzmT95RJImMHgabt99vkXGslubDaDagTk8= github.com/aws/aws-sdk-go-v2 v1.30.4/go.mod h1:CT+ZPWXbYrci8chcARI3OmI/qgd+f6WtuLOoaIA8PR0= -github.com/aws/aws-sdk-go-v2/config v1.27.30 h1:AQF3/+rOgeJBQP3iI4vojlPib5X6eeOYoa/af7OxAYg= -github.com/aws/aws-sdk-go-v2/config v1.27.30/go.mod h1:yxqvuubha9Vw8stEgNiStO+yZpP68Wm9hLmcm+R/Qk4= -github.com/aws/aws-sdk-go-v2/credentials v1.17.29 h1:CwGsupsXIlAFYuDVHv1nnK0wnxO0wZ/g1L8DSK/xiIw= -github.com/aws/aws-sdk-go-v2/credentials v1.17.29/go.mod h1:BPJ/yXV92ZVq6G8uYvbU0gSl8q94UB63nMT5ctNO38g= +github.com/aws/aws-sdk-go-v2/config v1.27.31 h1:kxBoRsjhT3pq0cKthgj6RU6bXTm/2SgdoUMyrVw0rAI= +github.com/aws/aws-sdk-go-v2/config v1.27.31/go.mod h1:z04nZdSWFPaDwK3DdJOG2r+scLQzMYuJeW0CujEm9FM= +github.com/aws/aws-sdk-go-v2/credentials v1.17.30 h1:aau/oYFtibVovr2rDt8FHlU17BTicFEMAi29V1U+L5Q= +github.com/aws/aws-sdk-go-v2/credentials v1.17.30/go.mod h1:BPJ/yXV92ZVq6G8uYvbU0gSl8q94UB63nMT5ctNO38g= github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.12 h1:yjwoSyDZF8Jth+mUk5lSPJCkMC0lMy6FaCD51jm6ayE= github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.12/go.mod h1:fuR57fAgMk7ot3WcNQfb6rSEn+SUffl7ri+aa8uKysI= github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.16 h1:TNyt/+X43KJ9IJJMjKfa3bNTiZbUP7DeCxfbTROESwY= diff --git a/internal/constants/constants.go b/internal/constants/constants.go index adda96a6d..0b47690ac 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 = 100 diff --git a/pkg/controllers/clusterresource/policy_controller.go b/pkg/controllers/clusterresource/policy_controller.go index 68a62c5a9..f511a50f1 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 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) } 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..65342b953 100644 --- a/pkg/controllers/clusterresource/store_controller.go +++ b/pkg/controllers/clusterresource/store_controller.go @@ -17,7 +17,6 @@ package clusterresource import ( "context" - "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -25,6 +24,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 +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 { diff --git a/pkg/controllers/clusterresource/store_controller_test.go b/pkg/controllers/clusterresource/store_controller_test.go index 69dabca45..fbd09341d 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) }) } } @@ -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 { diff --git a/pkg/controllers/clusterresource/verifier_controller.go b/pkg/controllers/clusterresource/verifier_controller.go index 0eaaa607d..2b7245926 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 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 { diff --git a/pkg/controllers/clusterresource/verifier_controller_test.go b/pkg/controllers/clusterresource/verifier_controller_test.go index 827f3620b..850a1fb69 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,13 @@ 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 + expectedBriefErrString string + reconciler client.StatusClient }{ { name: "success status", @@ -203,11 +206,13 @@ 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 100 characters, a long error string that exceeds the max length of 100 characters", + expectedErrString: "UNKNOWN: a long error string that exceeds the max length of 100 characters, a long error string that exceeds the max length of 100 characters", + expectedBriefErrString: "UNKNOWN: a long error string that exceeds the max length of 100 characters, a long error string t...", + reconciler: &mockStatusClient{}, }, { name: "status update failed", @@ -221,14 +226,19 @@ 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) + } + + if tc.verifier.Status.BriefError != tc.expectedBriefErrString { + t.Fatalf("Expected BriefError to be %+v , actual %+v", tc.expectedBriefErrString, tc.verifier.Status.BriefError) } }) } @@ -278,6 +288,21 @@ func TestVerifierReconcile(t *testing.T) { expectedErr: true, expectedVerifierCount: 0, }, + { + name: "unsupported verifier", + verifier: &configv1beta1.Verifier{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "", + Name: verifierName, + }, + Spec: configv1beta1.VerifierSpec{ + Name: "unsupported", + Address: dirPath, + }, + }, + expectedErr: true, + expectedVerifierCount: 0, + }, { name: "valid spec", verifier: &configv1beta1.Verifier{ 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..04417fbbf 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 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.NamespacedPolicySpec, namespace string) 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(namespace, constants.RatifyPolicy, policyEnforcer) 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..9c6b0f1b3 100644 --- a/pkg/controllers/namespaceresource/store_controller.go +++ b/pkg/controllers/namespaceresource/store_controller.go @@ -17,7 +17,6 @@ package namespaceresource import ( "context" - "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -25,6 +24,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 +65,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 @@ -87,23 +88,21 @@ func (r *StoreReconciler) SetupWithManager(mgr ctrl.Manager) error { func storeAddOrReplace(spec configv1beta1.NamespacedStoreSpec, fullname, namespace 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, 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..08ceac2fb 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) @@ -208,6 +251,21 @@ func TestStoreReconcile(t *testing.T) { expectedErr: true, expectedStoreCount: 0, }, + { + name: "unsupported store", + store: &configv1beta1.NamespacedStore{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: storeName, + }, + Spec: configv1beta1.NamespacedStoreSpec{ + Name: "unsupported", + Address: dirPath, + }, + }, + expectedErr: true, + expectedStoreCount: 0, + }, } for _, tt := range tests { diff --git a/pkg/controllers/namespaceresource/verifier_controller.go b/pkg/controllers/namespaceresource/verifier_controller.go index dbbd8ebc7..e78bd8d48 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 CR") + 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,8 @@ 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) + logrus.Error(err) + return err } return cutils.UpsertVerifier(spec.Version, spec.Address, namespace, objectName, verifierConfig) @@ -99,17 +99,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..ac433a53c 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,13 @@ 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 + expectedBriefErrString string + reconciler client.StatusClient }{ { name: "success status", @@ -205,11 +208,13 @@ 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 100 characters, a long error string that exceeds the max length of 100 characters", + expectedErrString: "UNKNOWN: a long error string that exceeds the max length of 100 characters, a long error string that exceeds the max length of 100 characters", + expectedBriefErrString: "UNKNOWN: a long error string that exceeds the max length of 100 characters, a long error string t...", + reconciler: &mockStatusClient{}, }, { name: "status update failed", @@ -223,14 +228,19 @@ 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) + } + + if tc.verifier.Status.BriefError != tc.expectedBriefErrString { + t.Fatalf("Expected BriefError to be %+v , actual %+v", tc.expectedBriefErrString, tc.verifier.Status.BriefError) } }) } @@ -280,6 +290,21 @@ func TestVerifierReconcile(t *testing.T) { expectedErr: true, expectedVerifierCount: 0, }, + { + name: "unsupported verifier", + verifier: &configv1beta1.NamespacedVerifier{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: verifierName, + }, + Spec: configv1beta1.NamespacedVerifierSpec{ + Name: "unsupported", + Address: dirPath, + }, + }, + expectedErr: true, + expectedVerifierCount: 0, + }, { name: "valid spec", verifier: &configv1beta1.NamespacedVerifier{ diff --git a/pkg/controllers/utils/kmp.go b/pkg/controllers/utils/kmp.go index 14f5d72d2..a2085cfb5 100644 --- a/pkg/controllers/utils/kmp.go +++ b/pkg/controllers/utils/kmp.go @@ -18,6 +18,7 @@ import ( "fmt" c "github.com/ratify-project/ratify/config" + re "github.com/ratify-project/ratify/errors" kmp "github.com/ratify-project/ratify/pkg/keymanagementprovider" "github.com/ratify-project/ratify/pkg/keymanagementprovider/config" "github.com/ratify-project/ratify/pkg/keymanagementprovider/factory" @@ -28,13 +29,13 @@ import ( func SpecToKeyManagementProvider(raw []byte, keyManagamentSystemName string) (kmp.KeyManagementProvider, error) { kmProviderConfig, err := rawToKeyManagementProviderConfig(raw, keyManagamentSystemName) if err != nil { - return nil, fmt.Errorf("failed to parse key management provider config: %w", err) + return nil, err } // TODO: add Version and Address to KeyManagementProviderSpec keyManagementProviderProvider, err := factory.CreateKeyManagementProviderFromConfig(kmProviderConfig, "0.1.0", c.GetDefaultPluginPath()) if err != nil { - return nil, fmt.Errorf("failed to create key management provider provider: %w", err) + return nil, err } return keyManagementProviderProvider, nil @@ -48,7 +49,7 @@ func rawToKeyManagementProviderConfig(raw []byte, keyManagamentSystemName string return config.KeyManagementProviderConfig{}, fmt.Errorf("no key management provider parameters provided") } if err := json.Unmarshal(raw, &pluginConfig); err != nil { - return config.KeyManagementProviderConfig{}, fmt.Errorf("unable to decode key management provider parameters.Raw: %s, err: %w", raw, err) + return config.KeyManagementProviderConfig{}, re.ErrorCodeConfigInvalid.WithDetail(fmt.Sprintf("Unable to decode key management provider parameters.Raw: %s", string(raw))).WithError(err) } pluginConfig[types.Type] = keyManagamentSystemName diff --git a/pkg/keymanagementprovider/refresh/kubeRefresh.go b/pkg/keymanagementprovider/refresh/kubeRefresh.go index aa06118af..acd967e0d 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" @@ -87,36 +88,41 @@ func (kr *KubeRefresher) Refresh(ctx context.Context) error { provider, err := cutils.SpecToKeyManagementProvider(keyManagementProvider.Spec.Parameters.Raw, keyManagementProvider.Spec.Type) if err != nil { - kmp.SetCertificateError(resource, err) - kmp.SetKeyError(resource, err) - writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodePluginInitFailure.WithError(err).WithDetail("Failed to create key management provider from CR") + + kmp.SetCertificateError(resource, kmpErr) + kmp.SetKeyError(resource, kmpErr) + 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 { + kmpErr := re.ErrorCodeKeyManagementProviderFailure.WithError(err).WithDetail(fmt.Sprintf("Unable to fetch certificates from key management provider [%s] of type [%s]", resource, keyManagementProvider.Spec.Type)) + kmp.SetCertificateError(resource, err) - writeKMProviderStatus(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + 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 { - kmp.SetKeyError(resource, err) - 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)) + + kmp.SetKeyError(resource, kmpErr) + 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.SaveSecrets(resource, keyManagementProvider.Spec.Type, keys, certificates) // 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) @@ -169,11 +175,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") @@ -181,15 +187,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 93432502e..8bd4ade20 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" @@ -86,36 +87,41 @@ func (kr *KubeRefresherNamespaced) Refresh(ctx context.Context) error { provider, err := cutils.SpecToKeyManagementProvider(keyManagementProvider.Spec.Parameters.Raw, keyManagementProvider.Spec.Type) if err != nil { - kmp.SetCertificateError(resource, err) - kmp.SetKeyError(resource, err) - writeKMProviderStatusNamespaced(ctx, kr, &keyManagementProvider, logger, isFetchSuccessful, err.Error(), lastFetchedTime, nil) + kmpErr := re.ErrorCodePluginInitFailure.WithError(err).WithDetail("Failed to create key management provider from CR") + + kmp.SetCertificateError(resource, kmpErr) + kmp.SetKeyError(resource, kmpErr) + 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 { - kmp.SetCertificateError(resource, err) - 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)) + + kmp.SetCertificateError(resource, kmpErr) + 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 { - kmp.SetKeyError(resource, err) - 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)) + + kmp.SetKeyError(resource, kmpErr) + 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.SaveSecrets(resource, keyManagementProvider.Spec.Type, keys, certificates) // 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) @@ -167,11 +173,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") @@ -179,15 +185,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) } }) } diff --git a/pkg/referrerstore/oras/mocks/oras_storage.go b/pkg/referrerstore/oras/mocks/oras_storage.go index b2bffa4d4..5567a318f 100644 --- a/pkg/referrerstore/oras/mocks/oras_storage.go +++ b/pkg/referrerstore/oras/mocks/oras_storage.go @@ -30,6 +30,7 @@ type TestStorage struct { ExistsMap map[digest.Digest]io.Reader ExistsErr error FetchErr error + PushErr error } func (s TestStorage) Exists(_ context.Context, target oci.Descriptor) (bool, error) { @@ -43,6 +44,9 @@ func (s TestStorage) Exists(_ context.Context, target oci.Descriptor) (bool, err } func (s TestStorage) Push(_ context.Context, expected oci.Descriptor, content io.Reader) error { + if s.PushErr != nil { + return s.PushErr + } s.ExistsMap[expected.Digest] = content return nil } diff --git a/pkg/referrerstore/oras/oras.go b/pkg/referrerstore/oras/oras.go index ef1116464..0a8f146e5 100644 --- a/pkg/referrerstore/oras/oras.go +++ b/pkg/referrerstore/oras/oras.go @@ -256,6 +256,8 @@ func (store *orasStore) ListReferrers(ctx context.Context, subjectReference comm func (store *orasStore) GetBlobContent(ctx context.Context, subjectReference common.Reference, digest digest.Digest) ([]byte, error) { var err error + var blobContent []byte + repository, err := store.createRepository(ctx, store, subjectReference) if err != nil { return nil, err @@ -270,10 +272,18 @@ func (store *orasStore) GetBlobContent(ctx context.Context, subjectReference com // check if blob exists in local ORAS cache isCached, err := store.localCache.Exists(ctx, blobDescriptor) if err != nil { - return nil, err + logger.GetLogger(ctx, logOpt).Warnf("failed to check if blob [%s] exists in cache: %v", blobDescriptor.Digest.String(), err) } metrics.ReportBlobCacheCount(ctx, isCached) + if isCached { + blobContent, err = store.getRawContentFromCache(ctx, blobDescriptor) + if err != nil { + isCached = false + logger.GetLogger(ctx, logOpt).Warnf("failed to get blob [%s] from cache: %v", blobDescriptor.Digest.String(), err) + } + } + if !isCached { // generate the reference path with digest ref := fmt.Sprintf("%s@%s", subjectReference.Path, digest) @@ -284,16 +294,20 @@ func (store *orasStore) GetBlobContent(ctx context.Context, subjectReference com evictOnError(ctx, err, subjectReference.Original) return nil, err } + if blobContent, err = io.ReadAll(rc); err != nil { + return nil, re.ErrorCodeGetBlobContentFailure.WithError(err) + } // push fetched content to local ORAS cache + // If multiple goroutines try to push the same blob to the cache, oras-go + // may return `ErrAlreadyExists` error. This is expected and can be ignored. orasExistsExpectedError := fmt.Errorf("%s: %s: %w", blobDesc.Digest, blobDesc.MediaType, errdef.ErrAlreadyExists) - err = store.localCache.Push(ctx, blobDesc, rc) - if err != nil && err.Error() != orasExistsExpectedError.Error() { - return nil, err + if err = store.localCache.Push(ctx, blobDesc, bytes.NewReader(blobContent)); err != nil && err.Error() != orasExistsExpectedError.Error() { + logger.GetLogger(ctx, logOpt).Warnf("failed to save blob [%s] in cache: %v", blobDesc.Digest, err) } } - return store.getRawContentFromCache(ctx, blobDescriptor) + return blobContent, nil } func (store *orasStore) GetReferenceManifest(ctx context.Context, subjectReference common.Reference, referenceDesc ocispecs.ReferenceDescriptor) (ocispecs.ReferenceManifest, error) { @@ -331,6 +345,8 @@ func (store *orasStore) GetReferenceManifest(ctx context.Context, subjectReferen } // push fetched manifest to local ORAS cache + // If multiple goroutines try to push the same manifest to the cache, oras-go + // may return `ErrAlreadyExists` error. This is expected and can be ignored. orasExistsExpectedError := fmt.Errorf("%s: %s: %w", referenceDesc.Descriptor.Digest, referenceDesc.Descriptor.MediaType, errdef.ErrAlreadyExists) err = store.localCache.Push(ctx, referenceDesc.Descriptor, bytes.NewReader(manifestBytes)) if err != nil && err.Error() != orasExistsExpectedError.Error() { diff --git a/pkg/referrerstore/oras/oras_test.go b/pkg/referrerstore/oras/oras_test.go index 818d9c76c..33eface56 100644 --- a/pkg/referrerstore/oras/oras_test.go +++ b/pkg/referrerstore/oras/oras_test.go @@ -36,6 +36,7 @@ import ( "github.com/ratify-project/ratify/pkg/ocispecs" "github.com/ratify-project/ratify/pkg/referrerstore/config" "github.com/ratify-project/ratify/pkg/referrerstore/oras/mocks" + "oras.land/oras-go/v2/content" "oras.land/oras-go/v2/registry" "oras.land/oras-go/v2/registry/remote/errcode" ) @@ -46,6 +47,50 @@ const ( validReferenceMediatype = "application/vnd.oci.image.manifest.right.v1+json" ) +var ( + artifactDigestNotCached = digest.FromString("testArtifactDigestNotCached") + artifactDigest = digest.FromString("testArtifactDigest") + invalidManifestBytes = []byte("invalid manifest") + blobDigest = digest.FromString("testBlobDigest") + firstDigest = digest.FromString("testDigest") + manifestNotCachedBytes []byte + manifestCachedBytesWithWrongType []byte + manifestCachedBytes []byte +) + +func init() { + manifestNotCached := oci.Manifest{ + MediaType: validReferenceMediatype, + Config: oci.Descriptor{}, + Layers: []oci.Descriptor{}, + } + manifestNotCachedBytes, _ = json.Marshal(manifestNotCached) + + manifestCachedWithWrongType := oci.Manifest{ + MediaType: wrongReferenceMediatype, + Config: oci.Descriptor{}, + Layers: []oci.Descriptor{}, + } + manifestCachedBytesWithWrongType, _ = json.Marshal(manifestCachedWithWrongType) + + manifestCached := oci.Manifest{ + MediaType: validReferenceMediatype, + Config: oci.Descriptor{}, + Layers: []oci.Descriptor{}, + } + manifestCachedBytes, _ = json.Marshal(manifestCached) +} + +type errorReader struct{} + +func (r *errorReader) Read(_ []byte) (int, error) { + return 0, errors.New("error reading") +} + +func (r *errorReader) Close() error { + return nil +} + // TestORASName tests the Name method of the oras store. func TestORASName(t *testing.T) { conf := config.StorePluginConfig{ @@ -193,366 +238,522 @@ func TestORASListReferrers_NoSubjectDesc(t *testing.T) { // TODO: add cosign test for List Referrers -// TestORASGetReferenceManifest_CachedDesc tests that the reference manifest is returned from the cache if it exists -func TestORASGetReferenceManifest_CachedDesc(t *testing.T) { - conf := config.StorePluginConfig{ - "name": "oras", - } - ctx := context.Background() - firstDigest := digest.FromString("testDigest") - artifactDigest := digest.FromString("testArtifactDigest") - store, err := createBaseStore("1.0.0", conf) - if err != nil { - t.Fatalf("failed to create oras store: %v", err) - } - manifestCached := oci.Manifest{ - MediaType: validReferenceMediatype, - Config: oci.Descriptor{}, - Layers: []oci.Descriptor{}, - } - manifestCachedBytes, err := json.Marshal(manifestCached) - if err != nil { - t.Fatalf("failed to marshal cached manifest: %v", err) - } - manifestNotCached := oci.Manifest{ - MediaType: wrongReferenceMediatype, - Config: oci.Descriptor{}, - Layers: []oci.Descriptor{}, - } - manifestNotCachedBytes, err := json.Marshal(manifestNotCached) - if err != nil { - t.Fatalf("failed to marshal not cached manifest: %v", err) - } - testRepo := mocks.TestRepository{ - FetchMap: map[digest.Digest]io.ReadCloser{ - artifactDigest: io.NopCloser(bytes.NewReader(manifestNotCachedBytes)), +func TestORASGetReferenceManifest(t *testing.T) { + tests := []struct { + name string + inputRef common.Reference + referenceDesc ocispecs.ReferenceDescriptor + repo registry.Repository + repoCreateErr error + localCache content.Storage + expectedErr bool + expectedMediaType string + }{ + { + name: "cache exists failure", + inputRef: common.Reference{ + Original: "inputOriginalPath", + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{}, + }, + localCache: mocks.TestStorage{ + ExistsErr: errors.New("cache exists error"), + }, + expectedErr: true, }, - } - store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { - return testRepo, nil - } - store.localCache = mocks.TestStorage{ - ExistsMap: map[digest.Digest]io.Reader{ - artifactDigest: bytes.NewReader(manifestCachedBytes), + { + name: "cache fetch manifest failure", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: io.NopCloser(bytes.NewReader(manifestNotCachedBytes)), + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigest: bytes.NewReader(manifestCachedBytes), + }, + FetchErr: errors.New("cache fetch error"), + }, + expectedErr: false, + expectedMediaType: validReferenceMediatype, }, - } - inputRef := common.Reference{ - Original: inputOriginalPath, - Digest: firstDigest, - } - manifest, err := store.GetReferenceManifest(ctx, inputRef, ocispecs.ReferenceDescriptor{ - Descriptor: oci.Descriptor{ - MediaType: ocispecs.MediaTypeArtifactManifest, - Digest: artifactDigest, + { + name: "not cached desc and fetch failed", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{}, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigestNotCached: bytes.NewReader(manifestCachedBytesWithWrongType), + }, + }, + expectedErr: true, }, - }) - if err != nil { - t.Fatalf("failed to get reference manifest: %v", err) - } - if manifest.MediaType != validReferenceMediatype { - t.Fatalf("expected media type %s, got %s", validReferenceMediatype, manifest.MediaType) - } -} - -// TestORASGetReferenceManifest_NotCachedDesc tests that the reference manifest is fetched from the registry if it is not cached -func TestORASGetReferenceManifest_NotCachedDesc(t *testing.T) { - conf := config.StorePluginConfig{ - "name": "oras", - } - ctx := context.Background() - firstDigest := digest.FromString("testDigest") - artifactDigest := digest.FromString("testArtifactDigest") - artifactDigestNotCached := digest.FromString("testArtifactDigestNotCached") - store, err := createBaseStore("1.0.0", conf) - if err != nil { - t.Fatalf("failed to create oras store: %v", err) - } - manifestCached := oci.Manifest{ - MediaType: wrongReferenceMediatype, - Config: oci.Descriptor{}, - Layers: []oci.Descriptor{}, - } - manifestCachedBytes, err := json.Marshal(manifestCached) - if err != nil { - t.Fatalf("failed to marshal cached manifest: %v", err) - } - manifestNotCached := oci.Manifest{ - MediaType: validReferenceMediatype, - Config: oci.Descriptor{}, - Layers: []oci.Descriptor{}, - } - manifestNotCachedBytes, err := json.Marshal(manifestNotCached) - if err != nil { - t.Fatalf("failed to marshal not cached manifest: %v", err) - } - testRepo := mocks.TestRepository{ - FetchMap: map[digest.Digest]io.ReadCloser{ - artifactDigest: io.NopCloser(bytes.NewReader(manifestNotCachedBytes)), + { + name: "create repository failed", + inputRef: common.Reference{}, + referenceDesc: ocispecs.ReferenceDescriptor{}, + repo: mocks.TestRepository{}, + repoCreateErr: errors.New("create repository error"), + expectedErr: true, }, - } - store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { - return testRepo, nil - } - store.localCache = mocks.TestStorage{ - ExistsMap: map[digest.Digest]io.Reader{ - artifactDigestNotCached: bytes.NewReader(manifestCachedBytes), + { + name: "reference manifest is returned from the cache if it exists", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: io.NopCloser(bytes.NewReader(manifestNotCachedBytes)), + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigest: bytes.NewReader(manifestCachedBytes), + }, + }, + expectedErr: false, + expectedMediaType: validReferenceMediatype, }, - } - inputRef := common.Reference{ - Original: inputOriginalPath, - Digest: firstDigest, - } - manifest, err := store.GetReferenceManifest(ctx, inputRef, ocispecs.ReferenceDescriptor{ - Descriptor: oci.Descriptor{ - MediaType: ocispecs.MediaTypeArtifactManifest, - Digest: artifactDigest, + { + name: "reference manifest is fetched from the registry if it is not cached", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: io.NopCloser(bytes.NewReader(manifestNotCachedBytes)), + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigestNotCached: bytes.NewReader(manifestCachedBytesWithWrongType), + }, + }, + expectedErr: false, + expectedMediaType: validReferenceMediatype, }, - }) - if err != nil { - t.Fatalf("failed to get reference manifest: %v", err) - } - if manifest.MediaType != validReferenceMediatype { - t.Fatalf("expected media type %s, got %s", validReferenceMediatype, manifest.MediaType) - } -} - -func TestORASGetReferenceManifest_CacheFetchManifestFailure(t *testing.T) { - conf := config.StorePluginConfig{ - "name": "oras", - } - ctx := context.Background() - firstDigest := digest.FromString("testDigest") - artifactDigest := digest.FromString("testArtifactDigest") - store, err := createBaseStore("1.0.0", conf) - if err != nil { - t.Fatalf("failed to create oras store: %v", err) - } - manifestCached := oci.Manifest{ - MediaType: wrongReferenceMediatype, - Config: oci.Descriptor{}, - Layers: []oci.Descriptor{}, - } - manifestCachedBytes, err := json.Marshal(manifestCached) - if err != nil { - t.Fatalf("failed to marshal cached manifest: %v", err) - } - manifestNotCached := oci.Manifest{ - MediaType: validReferenceMediatype, - Config: oci.Descriptor{}, - Layers: []oci.Descriptor{}, - } - manifestNotCachedBytes, err := json.Marshal(manifestNotCached) - if err != nil { - t.Fatalf("failed to marshal not cached manifest: %v", err) - } - testRepo := mocks.TestRepository{ - FetchMap: map[digest.Digest]io.ReadCloser{ - artifactDigest: io.NopCloser(bytes.NewReader(manifestNotCachedBytes)), + { + name: "descriptor not cached and fail during io.ReadAll from manifest", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: &errorReader{}, + }, + }, + localCache: mocks.TestStorage{ + FetchErr: errors.New("cache fetch error"), + }, + expectedErr: true, }, - } - store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { - return testRepo, nil - } - store.localCache = mocks.TestStorage{ - ExistsMap: map[digest.Digest]io.Reader{ - artifactDigest: bytes.NewReader(manifestCachedBytes), + { + name: "failed to unmarshal to oci manifest", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: oci.MediaTypeImageManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: io.NopCloser(bytes.NewReader(invalidManifestBytes)), + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigestNotCached: bytes.NewReader(manifestCachedBytesWithWrongType), + }, + }, + expectedErr: true, }, - FetchErr: errors.New("cache fetch error"), - } - inputRef := common.Reference{ - Original: inputOriginalPath, - Digest: firstDigest, - } - manifest, err := store.GetReferenceManifest(ctx, inputRef, ocispecs.ReferenceDescriptor{ - Descriptor: oci.Descriptor{ - MediaType: ocispecs.MediaTypeArtifactManifest, - Digest: artifactDigest, + { + name: "failed to unmarshal to artifact manifest", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: io.NopCloser(bytes.NewReader(invalidManifestBytes)), + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigestNotCached: bytes.NewReader(manifestCachedBytesWithWrongType), + }, + }, + expectedErr: true, }, - }) - if err != nil { - t.Fatalf("failed to get reference manifest: %v", err) - } - if manifest.MediaType != validReferenceMediatype { - t.Fatalf("expected media type %s, got %s", validReferenceMediatype, manifest.MediaType) - } -} - -func TestORASGetReferenceManifest_CacheExistsFailure(t *testing.T) { - conf := config.StorePluginConfig{ - "name": "oras", - } - ctx := context.Background() - firstDigest := digest.FromString("testDigest") - artifactDigest := digest.FromString("testArtifactDigest") - store, err := createBaseStore("1.0.0", conf) - if err != nil { - t.Fatalf("failed to create oras store: %v", err) - } - if err != nil { - t.Fatalf("failed to marshal cached manifest: %v", err) - } - - testRepo := mocks.TestRepository{ - FetchMap: map[digest.Digest]io.ReadCloser{}, - } - store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { - return testRepo, nil - } - store.localCache = mocks.TestStorage{ - ExistsErr: errors.New("cache exists error"), - } - inputRef := common.Reference{ - Original: inputOriginalPath, - Digest: firstDigest, - } - _, err = store.GetReferenceManifest(ctx, inputRef, ocispecs.ReferenceDescriptor{ - Descriptor: oci.Descriptor{ - MediaType: ocispecs.MediaTypeArtifactManifest, - Digest: artifactDigest, + { + name: "unsupported manifest media type", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: "unsupported media type", + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: io.NopCloser(bytes.NewReader(invalidManifestBytes)), + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigestNotCached: bytes.NewReader(manifestCachedBytesWithWrongType), + }, + }, + expectedErr: true, + }, + { + name: "failed to push manifest to cache", + inputRef: common.Reference{ + Original: inputOriginalPath, + Digest: firstDigest, + }, + referenceDesc: ocispecs.ReferenceDescriptor{ + Descriptor: oci.Descriptor{ + MediaType: ocispecs.MediaTypeArtifactManifest, + Digest: artifactDigest, + }, + }, + repo: mocks.TestRepository{ + FetchMap: map[digest.Digest]io.ReadCloser{ + artifactDigest: io.NopCloser(bytes.NewReader(manifestNotCachedBytes)), + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + artifactDigestNotCached: bytes.NewReader(manifestCachedBytesWithWrongType), + }, + PushErr: errors.New("push content error"), + }, + expectedErr: false, + expectedMediaType: validReferenceMediatype, }, - }) - if err == nil { - t.Fatalf("expected error fetching reference manifest") } -} -func TestORASGetReferenceManifest_NotCachedDescAndFetchFailed(t *testing.T) { - conf := config.StorePluginConfig{ - "name": "oras", - } - ctx := context.Background() - firstDigest := digest.FromString("testDigest") - artifactDigest := digest.FromString("testArtifactDigest") - artifactDigestNotCached := digest.FromString("testArtifactDigestNotCached") - store, err := createBaseStore("1.0.0", conf) - if err != nil { - t.Fatalf("failed to create oras store: %v", err) - } - manifestCached := oci.Manifest{ - MediaType: wrongReferenceMediatype, - Config: oci.Descriptor{}, - Layers: []oci.Descriptor{}, - } - manifestCachedBytes, err := json.Marshal(manifestCached) - if err != nil { - t.Fatalf("failed to marshal cached manifest: %v", err) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + conf := config.StorePluginConfig{ + "name": "oras", + } + store, err := createBaseStore("1.0.0", conf) + if err != nil { + t.Fatalf("failed to create oras store: %v", err) + } + store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { + return tc.repo, tc.repoCreateErr + } + store.localCache = tc.localCache + + manifest, err := store.GetReferenceManifest(context.Background(), tc.inputRef, tc.referenceDesc) + if tc.expectedErr { + if err == nil { + t.Fatalf("expected error fetching reference manifest") + } + } else { + if err != nil { + t.Fatalf("failed to get reference manifest: %v", err) + } + if manifest.MediaType != tc.expectedMediaType { + t.Fatalf("expected media type %s, got %s", tc.expectedMediaType, manifest.MediaType) + } + } + }) } +} - testRepo := mocks.TestRepository{ - FetchMap: map[digest.Digest]io.ReadCloser{}, - } - store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { - return testRepo, nil - } - store.localCache = mocks.TestStorage{ - ExistsMap: map[digest.Digest]io.Reader{ - artifactDigestNotCached: bytes.NewReader(manifestCachedBytes), +func TestORASGetBlobContent(t *testing.T) { + tests := []struct { + name string + repo registry.Repository + localCache content.Storage + repoCreateErr error + subjectReference common.Reference + digest digest.Digest + expectedContent []byte + expectedErr bool + }{ + { + name: "fail to create repository", + repo: mocks.TestRepository{}, + repoCreateErr: errors.New("create repository error"), + expectedErr: true, }, - } - inputRef := common.Reference{ - Original: inputOriginalPath, - Digest: firstDigest, - } - _, err = store.GetReferenceManifest(ctx, inputRef, ocispecs.ReferenceDescriptor{ - Descriptor: oci.Descriptor{ - MediaType: ocispecs.MediaTypeArtifactManifest, - Digest: artifactDigest, + { + name: "fail to check blob existence", + repo: mocks.TestRepository{ + BlobStoreTest: mocks.TestBlobStore{ + BlobMap: map[string]mocks.BlobPair{ + fmt.Sprintf("%s@%s", inputOriginalPath, blobDigest.String()): { + Descriptor: oci.Descriptor{ + Digest: blobDigest, + }, + Reader: io.NopCloser(bytes.NewReader([]byte("test content"))), + }, + }, + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{}, + ExistsErr: errors.New("check blob existence error"), + }, + subjectReference: common.Reference{ + Original: inputOriginalPath, + Path: inputOriginalPath, + Digest: firstDigest, + }, + digest: blobDigest, + expectedContent: []byte("test content"), + expectedErr: false, }, - }) - if err == nil { - t.Fatalf("expected error fetching reference manifest") - } -} - -// TestORASGetBlobContent_CachedDesc tests that the blob content is fetched from the cache if it is cached -func TestORASGetBlobContent_CachedDesc(t *testing.T) { - conf := config.StorePluginConfig{ - "name": "oras", - } - ctx := context.Background() - firstDigest := digest.FromString("testDigest") - blobDigest := digest.FromString("testBlobDigest") - expectedContent := []byte("test content") - inputRef := common.Reference{ - Original: inputOriginalPath, - Path: inputOriginalPath, - Digest: firstDigest, - } - store, err := createBaseStore("1.0.0", conf) - if err != nil { - t.Fatalf("failed to create oras store: %v", err) - } - testRepo := mocks.TestRepository{ - BlobStoreTest: mocks.TestBlobStore{ - BlobMap: map[string]mocks.BlobPair{ - fmt.Sprintf("%s@%s", inputRef.Path, blobDigest.String()): { - Descriptor: oci.Descriptor{ - Digest: blobDigest, + { + name: "fail to get raw content from cache", + repo: mocks.TestRepository{ + BlobStoreTest: mocks.TestBlobStore{ + BlobMap: map[string]mocks.BlobPair{ + fmt.Sprintf("%s@%s", inputOriginalPath, blobDigest.String()): { + Descriptor: oci.Descriptor{ + Digest: blobDigest, + }, + Reader: io.NopCloser(bytes.NewReader([]byte("test content"))), + }, }, - Reader: io.NopCloser(bytes.NewReader(expectedContent)), }, }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + blobDigest: bytes.NewReader([]byte("test content")), + }, + FetchErr: errors.New("fetch blob error"), + }, + subjectReference: common.Reference{ + Original: inputOriginalPath, + Path: inputOriginalPath, + Digest: firstDigest, + }, + digest: blobDigest, + expectedContent: []byte("test content"), + expectedErr: false, }, - } - store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { - return testRepo, nil - } - store.localCache = mocks.TestStorage{ - ExistsMap: map[digest.Digest]io.Reader{ - blobDigest: bytes.NewReader(expectedContent), + { + name: "fail to fetch blob from repository", + repo: mocks.TestRepository{ + BlobStoreTest: mocks.TestBlobStore{ + BlobMap: map[string]mocks.BlobPair{}, + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{}, + }, + subjectReference: common.Reference{ + Original: inputOriginalPath, + Path: inputOriginalPath, + Digest: firstDigest, + }, + digest: blobDigest, + expectedErr: true, }, - } - content, err := store.GetBlobContent(ctx, inputRef, blobDigest) - if err != nil { - t.Fatalf("failed to get blob content: %v", err) - } - if !bytes.Equal(content, expectedContent) { - t.Fatalf("expected content %s, got %s", expectedContent, content) - } -} - -// TestORASGetBlobContent_NotCachedDesc tests that the blob content is fetched from the registry if it is not cached -func TestORASGetBlobContent_NotCachedDesc(t *testing.T) { - conf := config.StorePluginConfig{ - "name": "oras", - } - ctx := context.Background() - firstDigest := digest.FromString("testDigest") - blobDigest := digest.FromString("testBlobDigest") - expectedContent := []byte("test content") - inputRef := common.Reference{ - Original: inputOriginalPath, - Path: inputOriginalPath, - Digest: firstDigest, - } - store, err := createBaseStore("1.0.0", conf) - if err != nil { - t.Fatalf("failed to create oras store: %v", err) - } - testRepo := mocks.TestRepository{ - BlobStoreTest: mocks.TestBlobStore{ - BlobMap: map[string]mocks.BlobPair{ - fmt.Sprintf("%s@%s", inputRef.Path, blobDigest.String()): { - Descriptor: oci.Descriptor{ - Digest: blobDigest, + { + name: "fail to read fetched blob", + repo: mocks.TestRepository{ + BlobStoreTest: mocks.TestBlobStore{ + BlobMap: map[string]mocks.BlobPair{ + fmt.Sprintf("%s@%s", inputOriginalPath, blobDigest.String()): { + Descriptor: oci.Descriptor{ + Digest: blobDigest, + }, + Reader: &errorReader{}, + }, }, - Reader: io.NopCloser(bytes.NewReader(expectedContent)), }, }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{}, + }, + subjectReference: common.Reference{ + Original: inputOriginalPath, + Path: inputOriginalPath, + Digest: firstDigest, + }, + digest: blobDigest, + expectedErr: true, + }, + { + name: "fail to push content to local cache", + repo: mocks.TestRepository{ + BlobStoreTest: mocks.TestBlobStore{ + BlobMap: map[string]mocks.BlobPair{ + fmt.Sprintf("%s@%s", inputOriginalPath, blobDigest.String()): { + Descriptor: oci.Descriptor{ + Digest: blobDigest, + }, + Reader: io.NopCloser(bytes.NewReader([]byte("test content"))), + }, + }, + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{}, + PushErr: errors.New("push content error"), + }, + subjectReference: common.Reference{ + Original: inputOriginalPath, + Path: inputOriginalPath, + Digest: firstDigest, + }, + digest: blobDigest, + expectedContent: []byte("test content"), + expectedErr: false, + }, + { + name: "blob content is fetched from the cache if it is cached", + repo: mocks.TestRepository{ + BlobStoreTest: mocks.TestBlobStore{ + BlobMap: map[string]mocks.BlobPair{ + fmt.Sprintf("%s@%s", inputOriginalPath, blobDigest.String()): { + Descriptor: oci.Descriptor{ + Digest: blobDigest, + }, + Reader: io.NopCloser(bytes.NewReader([]byte("test content"))), + }, + }, + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{ + blobDigest: bytes.NewReader([]byte("test content")), + }, + }, + subjectReference: common.Reference{ + Original: inputOriginalPath, + Path: inputOriginalPath, + Digest: firstDigest, + }, + digest: blobDigest, + expectedContent: []byte("test content"), + expectedErr: false, + }, + { + name: "blob content is fetched from the registry if it is not cached", + repo: mocks.TestRepository{ + BlobStoreTest: mocks.TestBlobStore{ + BlobMap: map[string]mocks.BlobPair{ + fmt.Sprintf("%s@%s", inputOriginalPath, blobDigest.String()): { + Descriptor: oci.Descriptor{ + Digest: blobDigest, + }, + Reader: io.NopCloser(bytes.NewReader([]byte("test content"))), + }, + }, + }, + }, + localCache: mocks.TestStorage{ + ExistsMap: map[digest.Digest]io.Reader{}, + }, + subjectReference: common.Reference{ + Original: inputOriginalPath, + Path: inputOriginalPath, + Digest: firstDigest, + }, + digest: blobDigest, + expectedContent: []byte("test content"), + expectedErr: false, }, } - store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { - return testRepo, nil - } - store.localCache = mocks.TestStorage{ - ExistsMap: map[digest.Digest]io.Reader{}, - } - content, err := store.GetBlobContent(ctx, inputRef, blobDigest) - if err != nil { - t.Fatalf("failed to get blob content: %v", err) - } - if !bytes.Equal(content, expectedContent) { - t.Fatalf("expected content %s, got %s", expectedContent, content) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + conf := config.StorePluginConfig{ + "name": "oras", + } + store, err := createBaseStore("1.0.0", conf) + if err != nil { + t.Fatalf("failed to create oras store: %v", err) + } + store.createRepository = func(_ context.Context, _ *orasStore, _ common.Reference) (registry.Repository, error) { + return tt.repo, tt.repoCreateErr + } + store.localCache = tt.localCache + content, err := store.GetBlobContent(context.Background(), tt.subjectReference, tt.digest) + if tt.expectedErr { + if err == nil { + t.Fatalf("expected error, got nil") + } + } else { + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if !bytes.Equal(content, tt.expectedContent) { + t.Fatalf("expected content %s, got %s", tt.expectedContent, content) + } + } + }) } }