Skip to content

Conversation

@javirln
Copy link
Member

@javirln javirln commented May 27, 2024

This patch creates a base error for the uploadAndCraft method that can be used to determine errors during the upload of and artifact.

This is needed for cases where the incoming material is detected correctly but the error happens when trying to upload it to a CAS.

A part from being added to the debug sequence is returned to the user as follows:

$ chainloop --insecure attestation add --value ./internal/attestation/crafter/materials/testdata/sbom.cyclonedx.json --debug | pbcopy
WRN API contacted in insecure mode
DBG Telemetry enabled, to disable it use DO_NOT_TRACK=1
DBG loading state state=file:///var/folders/ct/h8h0pjcd6q180cnxqhykjv3c0000gn/T/chainloop-attestation.tmp.json
DBG loaded state state=file:///var/folders/ct/h8h0pjcd6q180cnxqhykjv3c0000gn/T/chainloop-attestation.tmp.json
DBG decoding OpenVex file path=./internal/attestation/crafter/materials/testdata/sbom.cyclonedx.json
DBG failed to add material error="crafting material: invalid OpenVEX file: unexpected material type" kind=OPENVEX
DBG crafting file backend=INLINE digest=sha256:16159bb881eb4ab7eb5d8afc5350b0feeed1e31c0a268e355e74f9ccbe885e0c filename=sbom.cyclonedx.json max_size=500K path=./internal/attestation/crafter/materials/testdata/sbom.cyclonedx.json size=3.8M
DBG failed to add material error="crafting material: upload and craft error: this file is too big for the INLINE CAS backend, please contact your administrator: fileSize=3.8M, maxSize=500K" kind=SBOM_CYCLONEDX_JSON
ERR adding material: crafting material: upload and craft error: this file is too big for the INLINE CAS backend, please contact your administrator: fileSize=3.8M, maxSize=500K

Without --debug:

$ chainloop --insecure attestation add --value ./internal/attestation/crafter/materials/testdata/sbom.cyclonedx.json
WRN API contacted in insecure mode
ERR adding material: crafting material: upload and craft error: this file is too big for the INLINE CAS backend, please contact your administrator: fileSize=3.8M, maxSize=500K

Refs: #828

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln requested review from jiparis and migmartri May 27, 2024 10:02
@javirln javirln self-assigned this May 27, 2024
// 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))))
Copy link
Member Author

@javirln javirln May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would personally use errors.Join(ErrBaseUploadAndCraft, ...) here

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests need some work. Approved though, thanks!

@javirln
Copy link
Member Author

javirln commented May 27, 2024

Let met rework on the test cases and the default error :)

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine for now but whenever we add a string validation this test will not work I think. Either way, that's smth for the future :)

@javirln javirln merged commit c5a831e into chainloop-dev:main May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants