From 38eeea53d3936249ce60e289a6c7ed43d17586ee Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Mon, 27 May 2024 11:58:54 +0200 Subject: [PATCH 1/3] fix(craft): Wrap upload and craft errors to raise errors Signed-off-by: Javier Rodriguez --- internal/attestation/crafter/crafter.go | 29 +++++++------- internal/attestation/crafter/crafter_test.go | 39 ++++++++++++++----- .../crafter/materials/materials.go | 17 +++++--- 3 files changed, 53 insertions(+), 32 deletions(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index edb3062bb..bf6505178 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -517,26 +517,23 @@ func (c *Crafter) AddMaterialFromContract(ctx context.Context, attestationID, ke // AddMaterialContactFreeAutomatic adds a material to the crafting state checking the incoming material matches any of the // supported types in validation order. If the material is not found it will return an error. func (c *Crafter) AddMaterialContactFreeAutomatic(ctx context.Context, attestationID, value string, casBackend *casclient.CASBackend, runtimeAnnotations map[string]string) (schemaapi.CraftingSchema_Material_MaterialType, error) { - var kind schemaapi.CraftingSchema_Material_MaterialType - var found bool - - // We want to run the material validation in a specific order - for _, kind = range schemaapi.CraftingMaterialInValidationOrder { - if err := c.AddMaterialContractFree(ctx, attestationID, kind.String(), value, casBackend, runtimeAnnotations); err != nil { - c.logger.Debug().Err(err).Str("kind", kind.String()).Msg("failed to add material") - continue + for _, kind := range schemaapi.CraftingMaterialInValidationOrder { + err := c.AddMaterialContractFree(ctx, attestationID, kind.String(), value, casBackend, runtimeAnnotations) + if err == nil { + // Successfully added material, return the kind + return kind, nil } - // If we found a match we break the loop and stop looking - found = true - break - } - // Return an error if the material could not be added - if !found { - return kind, fmt.Errorf("failed to add material with attestationID: %s", attestationID) + c.logger.Debug().Err(err).Str("kind", kind.String()).Msg("failed to add material") + + // Handle base error for upload and craft errors except the opening file error + if errors.Is(err, materials.ErrBaseUploadAndCraft) { + return kind, err + } } - return kind, nil + // Return an error if no material could be added + return schemaapi.CraftingSchema_Material_MATERIAL_TYPE_UNSPECIFIED, fmt.Errorf("failed to add material with attestationID: %s", attestationID) } // addMaterials adds the incoming material m to the crafting state diff --git a/internal/attestation/crafter/crafter_test.go b/internal/attestation/crafter/crafter_test.go index d8d5efde6..0cf064503 100644 --- a/internal/attestation/crafter/crafter_test.go +++ b/internal/attestation/crafter/crafter_test.go @@ -26,6 +26,7 @@ import ( schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" "github.com/chainloop-dev/chainloop/internal/attestation/crafter" v1 "github.com/chainloop-dev/chainloop/internal/attestation/crafter/api/attestation/v1" + "github.com/chainloop-dev/chainloop/internal/attestation/crafter/materials" "github.com/chainloop-dev/chainloop/internal/attestation/crafter/runners" "github.com/chainloop-dev/chainloop/internal/attestation/crafter/statemanager/filesystem" "github.com/chainloop-dev/chainloop/internal/casclient" @@ -423,10 +424,11 @@ func TestSuite(t *testing.T) { func (s *crafterSuite) TestAddMaterialsAutomatic() { testCases := []struct { - name string - materialPath string - expectedType schemaapi.CraftingSchema_Material_MaterialType - wantErr bool + name string + materialPath string + expectedType schemaapi.CraftingSchema_Material_MaterialType + uploadArtifact bool + exceedSize bool }{ { name: "sarif", @@ -464,10 +466,17 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { expectedType: schemaapi.CraftingSchema_Material_ARTIFACT, }, { - name: "random string", - materialPath: "random-string", - expectedType: schemaapi.CraftingSchema_Material_STRING, - wantErr: true, + name: "random string", + materialPath: "random-string", + expectedType: schemaapi.CraftingSchema_Material_STRING, + uploadArtifact: true, + }, + { + name: "file too large", + materialPath: "./materials/testdata/sbom.cyclonedx.json", + expectedType: schemaapi.CraftingSchema_Material_SBOM_CYCLONEDX_JSON, + exceedSize: true, + uploadArtifact: true, }, } @@ -477,7 +486,7 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { contract := "testdata/contracts/empty_generic.yaml" uploader := mUploader.NewUploader(s.T()) - if !tc.wantErr { + if !tc.uploadArtifact { uploader.On("UploadFile", context.Background(), tc.materialPath). Return(&casclient.UpDownStatus{ Digest: "deadbeef", @@ -487,11 +496,21 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { backend := &casclient.CASBackend{Uploader: uploader} + if tc.exceedSize { + backend.MaxSize = 1 + } + c, err := newInitializedCrafter(s.T(), contract, &v1.WorkflowMetadata{}, false, "", runner) require.NoError(s.T(), err) kind, err := c.AddMaterialContactFreeAutomatic(context.Background(), "random-id", tc.materialPath, backend, nil) - require.NoError(s.T(), err) + fmt.Println(err) + fmt.Println(kind) + if err != nil { + assert.ErrorIs(s.T(), err, materials.ErrBaseUploadAndCraft) + } else { + require.NoError(s.T(), err) + } assert.Equal(s.T(), tc.expectedType.String(), kind.String()) }) } diff --git a/internal/attestation/crafter/materials/materials.go b/internal/attestation/crafter/materials/materials.go index 809be8a29..426d0bef2 100644 --- a/internal/attestation/crafter/materials/materials.go +++ b/internal/attestation/crafter/materials/materials.go @@ -17,6 +17,7 @@ package materials import ( "context" + "errors" "fmt" "io" "os" @@ -33,9 +34,13 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" ) -// ErrInvalidMaterialType is returned when the provided material type -// is not from the kind we are expecting -var ErrInvalidMaterialType = fmt.Errorf("unexpected material type") +var ( + // ErrInvalidMaterialType is returned when the provided material type + // is not from the kind we are expecting + ErrInvalidMaterialType = fmt.Errorf("unexpected material type") + // ErrBaseUploadAndCraft is returned as a base error when the upload and craft of a material fails + ErrBaseUploadAndCraft = errors.New("upload and craft error") +) type crafterCommon struct { logger *zerolog.Logger @@ -59,7 +64,7 @@ func uploadAndCraft(ctx context.Context, input *schemaapi.CraftingSchema_Materia // If there is a max size set and the file is bigger than that, return an error if backend.MaxSize > 0 && result.size > backend.MaxSize { - return nil, fmt.Errorf("this file is too big for the %s CAS backend, please contact your administrator: fileSize=%s, maxSize=%s", backend.Name, bytefmt.ByteSize(uint64(result.size)), bytefmt.ByteSize(uint64(backend.MaxSize))) + return nil, fmt.Errorf("%w: %w", ErrBaseUploadAndCraft, fmt.Errorf("this file is too big for the %s CAS backend, please contact your administrator: fileSize=%s, maxSize=%s", backend.Name, bytefmt.ByteSize(uint64(result.size)), bytefmt.ByteSize(uint64(backend.MaxSize)))) } material := &api.Attestation_Material{ @@ -80,7 +85,7 @@ func uploadAndCraft(ctx context.Context, input *schemaapi.CraftingSchema_Materia _, err = backend.Uploader.UploadFile(ctx, artifactPath) if err != nil { - return nil, fmt.Errorf("uploading material: %w", err) + return nil, fmt.Errorf("%w: %w", ErrBaseUploadAndCraft, fmt.Errorf("uploading material: %w", err)) } material.UploadedToCas = true @@ -89,7 +94,7 @@ func uploadAndCraft(ctx context.Context, input *schemaapi.CraftingSchema_Materia // or store it inline if no uploader is provided content, err := io.ReadAll(result.r) if err != nil { - return nil, fmt.Errorf("reading file: %w", err) + return nil, fmt.Errorf("%w: %w", ErrBaseUploadAndCraft, fmt.Errorf("reading file: %w", err)) } material.InlineCas = true From 67ba4606ae8271788e5d0599785190e8ea6718ff Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Mon, 27 May 2024 17:52:13 +0200 Subject: [PATCH 2/3] Remove prints Signed-off-by: Javier Rodriguez --- internal/attestation/crafter/crafter_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/attestation/crafter/crafter_test.go b/internal/attestation/crafter/crafter_test.go index 0cf064503..3bda2a2a8 100644 --- a/internal/attestation/crafter/crafter_test.go +++ b/internal/attestation/crafter/crafter_test.go @@ -504,8 +504,6 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { require.NoError(s.T(), err) kind, err := c.AddMaterialContactFreeAutomatic(context.Background(), "random-id", tc.materialPath, backend, nil) - fmt.Println(err) - fmt.Println(kind) if err != nil { assert.ErrorIs(s.T(), err, materials.ErrBaseUploadAndCraft) } else { From 8f80861fe265aa3a8a6d56362630d4a05ab8092c Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Mon, 27 May 2024 19:14:49 +0200 Subject: [PATCH 3/3] tackle feedback Signed-off-by: Javier Rodriguez --- internal/attestation/crafter/crafter.go | 2 +- internal/attestation/crafter/crafter_test.go | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index bf6505178..a2a7fd17c 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -533,7 +533,7 @@ func (c *Crafter) AddMaterialContactFreeAutomatic(ctx context.Context, attestati } // Return an error if no material could be added - return schemaapi.CraftingSchema_Material_MATERIAL_TYPE_UNSPECIFIED, fmt.Errorf("failed to add material with attestationID: %s", attestationID) + return schemaapi.CraftingSchema_Material_MATERIAL_TYPE_UNSPECIFIED, fmt.Errorf("failed to auto-discover material kind for value: %v", value) } // addMaterials adds the incoming material m to the crafting state diff --git a/internal/attestation/crafter/crafter_test.go b/internal/attestation/crafter/crafter_test.go index 3bda2a2a8..48b37bb35 100644 --- a/internal/attestation/crafter/crafter_test.go +++ b/internal/attestation/crafter/crafter_test.go @@ -428,7 +428,7 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { materialPath string expectedType schemaapi.CraftingSchema_Material_MaterialType uploadArtifact bool - exceedSize bool + wantErr bool }{ { name: "sarif", @@ -475,7 +475,7 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { name: "file too large", materialPath: "./materials/testdata/sbom.cyclonedx.json", expectedType: schemaapi.CraftingSchema_Material_SBOM_CYCLONEDX_JSON, - exceedSize: true, + wantErr: true, uploadArtifact: true, }, } @@ -496,7 +496,9 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { backend := &casclient.CASBackend{Uploader: uploader} - if tc.exceedSize { + // Establishing a maximum size for the artifact to be uploaded to the CAS causes an error + // if the value is exceeded + if tc.wantErr { backend.MaxSize = 1 } @@ -504,7 +506,7 @@ func (s *crafterSuite) TestAddMaterialsAutomatic() { require.NoError(s.T(), err) kind, err := c.AddMaterialContactFreeAutomatic(context.Background(), "random-id", tc.materialPath, backend, nil) - if err != nil { + if tc.wantErr { assert.ErrorIs(s.T(), err, materials.ErrBaseUploadAndCraft) } else { require.NoError(s.T(), err)