Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only store attestation data that is needed #2207

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

joejstuart
Copy link
Member

@joejstuart joejstuart commented Dec 8, 2024

The whole attestation is being stored for each component that is evaluated, but in most cases only a subset of the attestation is printed. This change captures only the needed attestation data based on the output selected at runtime.

https://issues.redhat.com/browse/EC-1026

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

Attention: Patch coverage is 70.37037% with 8 lines in your changes missing coverage. Please review.

Project coverage is 71.19%. Comparing base (a6f67d9) to head (3dceef7).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
cmd/validate/image.go 54.54% 5 Missing ⚠️
internal/attestation/slsa_provenance_02.go 0.00% 2 Missing ⚠️
cmd/validate/input.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2207      +/-   ##
==========================================
- Coverage   71.21%   71.19%   -0.03%     
==========================================
  Files          88       88              
  Lines        7501     7520      +19     
==========================================
+ Hits         5342     5354      +12     
- Misses       2159     2166       +7     
Flag Coverage Δ
generative 71.19% <70.37%> (-0.03%) ⬇️
integration 71.19% <70.37%> (-0.03%) ⬇️
unit 71.19% <70.37%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
internal/applicationsnapshot/attestation.go 100.00% <100.00%> (ø)
internal/applicationsnapshot/report.go 76.00% <ø> (ø)
cmd/validate/input.go 42.24% <0.00%> (ø)
internal/attestation/slsa_provenance_02.go 88.00% <0.00%> (-3.67%) ⬇️
cmd/validate/image.go 90.59% <54.54%> (-0.97%) ⬇️

@joejstuart joejstuart force-pushed the EC-1026-2 branch 6 times, most recently from 6952ce0 to 8780fe0 Compare December 9, 2024 16:08
The whole attestation is being stored for each
component that is evaluated, but in most cases
only a subset of the attestation printed.
This change captures only the needed attestation
data based on the output selected at runtime.

https://issues.redhat.com/browse/EC-1026
Copy link
Member

@simonbaird simonbaird left a comment

Choose a reason for hiding this comment

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

It seems reasonable. Can you see a significant reduction in memory use for a large snapshot?

@lcarva
Copy link
Member

lcarva commented Dec 9, 2024

I'm concerned that this doesn't go far enough. It's still accumulating large amounts of data in memory which we know it's a problem. What about writing the data to files and just track the file paths in memory?

Anyways, I think measuring the impact in memory utilization before and after this change should be done to help decide which path to take.

UPDATE: The biggest saver here is the check to only store the statement if the "attestation" output target is requested. The data that actually always gets tracked seems significantly smaller.

@joejstuart
Copy link
Member Author

Metric After change Before change
User time 999.78u (999.78 seconds) 916.93u (916.93 seconds)
System time 31.84s (31.84 seconds) 30.46s (30.46 seconds)
Real time 1232.18r (1232.18 seconds) 1342.26r (1342.26 seconds)
Memory 777280kB (~759 MB) 1147664kB (~1120 MB)

@simonbaird
Copy link
Member

The memory reduction looks good. Do you have a bash snippet that you used to produce those?

@joejstuart
Copy link
Member Author

The memory reduction looks good. Do you have a bash snippet that you used to produce those?

xtime ec validate image --images ../ec-cli/local_testing/large-snapshot.yaml --policy github.com/joejstuart/ec-config//slsa3?ref=volatile-test -k ../ec-cli/local_testing/cosign.pub --ignore-rekor --timeout 30m
$ cat ~/bin/xtime 
#!/bin/sh
/opt/homebrew/bin/gtime -f '%Uu %Ss %er %MkB %C' "$@"

@joejstuart joejstuart merged commit a0b32fc into enterprise-contract:main Dec 10, 2024
10 of 11 checks passed
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.

3 participants