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.

Sign up for GitHub

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

Jump to bottom

ignore minor parsing error when reading dpkg status files #786

Merged
merged 12 commits into from
Feb 17, 2022
Merged

ignore minor parsing error when reading dpkg status files #786

merged 12 commits into from
Feb 17, 2022

Conversation

jonasagx
Copy link
Contributor

@jonasagx jonasagx commented Feb 1, 2022 •
edited
Loading

Fixes #733

Question: should we add a smarter parser to guess approximate installed-size
value?
A: yes, this PR uses go-humanize to parse storage metadata better

This PR also drops dpkg cataloger from returning parsing errors, they should not stop Syft. Warning with context were added.

Signed-off-by: Jonas Galvão Xavier [email protected]

@jonasagx jonasagx marked this pull request as draft February 1, 2022 21:42
@jonasagx jonasagx marked this pull request as ready for review February 3, 2022 05:42
@jonasagx jonasagx requested a review from a team February 4, 2022 17:51
@jonasagx jonasagx force-pushed the 733-fix-dpkg-parser branch from 28b3ab8 to 75e5b0f Compare February 7, 2022 21:40
@jonasagx jonasagx requested a review from wagoodman February 7, 2022 22:04
@jonasagx jonasagx self-assigned this Feb 8, 2022
@jonasagx jonasagx requested a review from a team February 8, 2022 17:06
kzantow
kzantow reviewed Feb 8, 2022
View reviewed changes
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

A couple suggestions I think might help maintainability and unclear about a couple error handling path changes. The main bit of the go-humanize addition looks great, though!

@jonasagx jonasagx force-pushed the 733-fix-dpkg-parser branch from 75e5b0f to 63f75cc Compare February 9, 2022 08:19
wagoodman
wagoodman reviewed Feb 9, 2022
View reviewed changes
Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

This comment: #786 (comment) was resolved but it doesn't look like there's a corresponding change or additional conversation here.

@jonasagx jonasagx force-pushed the 733-fix-dpkg-parser branch from 63f75cc to 0b97c2f Compare February 10, 2022 00:01
@jonasagx
ignore minor parsing error when reading dpkg status files
b82c696 
helps with anchore#733

Question: should we add a smarter parser to guess approximate installed-size
value?

Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
add datasize lib to help dpkg parsing
64b91eb 
added unit tests to expand coverage of dpkg parsing

Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
drop parse error
ca8e66b 
added unit tests to handleNewKeyValue

Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
don't return parsing errors from dpkg
4ca813f 
Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
go mod tidy
a49d56e 
Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
test higher level functions
344def7 
Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
return parsing err to let cataloger handle it
2b1b8ae 
Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
feedback changes
a16009a 
Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
ignore key parsing error
5eb7e31 
log warning with relevant context

Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
go mod tidy
87a0b31 
Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
add context info to log lines
606fbcb 
simpler error assertion

Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx
use error.As to assert error in chain
75b0fde 
Signed-off-by: Jonas Galvão Xavier <[email protected]>
@jonasagx jonasagx force-pushed the 733-fix-dpkg-parser branch from 3c83f16 to 75b0fde Compare February 16, 2022 20:49
@jonasagx jonasagx requested a review from a team February 16, 2022 21:25
kzantow
kzantow approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM; just a couple suggestions that I wouldn't hold this up for

syft/pkg/cataloger/deb/parse_dpkg_status_test.go
}{
{
name: "Test Single Package",
name: "Test Single Package",
fixturePath: filepath.Join("test-fixtures", "status", "single"),
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW all OSes can handle test-fixtures/status/single, including Windows

spiffcs
spiffcs reviewed Feb 17, 2022
View reviewed changes
Copy link
Contributor

@spiffcs spiffcs left a comment

Choose a reason for hiding this comment

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

small comment about how we are parsing the size variables

@jonasagx jonasagx merged commit 4b16737 into anchore:main Feb 17, 2022
spiffcs pushed a commit that referenced this pull request Feb 18, 2022
@jonasagx @spiffcs
ignore minor parsing error when reading dpkg status files (#786)
1680cde 
* ignore minor parsing error when reading dpkg status files

helps with #733

Question: should we add a smarter parser to guess approximate installed-size
value?

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* add datasize lib to help dpkg parsing

added unit tests to expand coverage of dpkg parsing

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* drop parse error

added unit tests to handleNewKeyValue

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* don't return parsing errors from dpkg

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* go mod tidy

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* test higher level functions

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* return parsing err to let cataloger handle it

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* feedback changes

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* ignore key parsing error

log warning with relevant context

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* go mod tidy

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* add context info to log lines

simpler error assertion

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* use error.As to assert error in chain

Signed-off-by: Jonas Galvão Xavier <[email protected]>
spiffcs added a commit that referenced this pull request Feb 18, 2022
@spiffcs
Merge branch '510-attach-sbomb-attestation' of https://github.com/anc…
b9bfa48 
…hore/syft into 510-attach-sbomb-attestation

* '510-attach-sbomb-attestation' of https://github.com/anchore/syft: (63 commits)
  adjust attest options to be 12-factor-like
  update go.mod to latest cosign version
  update go mod
  update exported function for tests
  tests are passing
  configure failing tests
  ignore minor parsing error when reading dpkg status files (#786)
  update nit comments
  test harness for password verification
  update password select mechanism
  comment out test for future work
  Base64 encoder closing (#822)
  dog food attestation on syft image
  update function usage
  update correct predicate type formats for JSON
  update ci workflow to boostrap tools on cli tests
  update to bootstrap go
  update to find cosign temp
  access local temp directory
  check if cache issue
  ...
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
@jonasagx
ignore minor parsing error when reading dpkg status files (anchore#786)
4be72fd 
* ignore minor parsing error when reading dpkg status files

helps with anchore#733

Question: should we add a smarter parser to guess approximate installed-size
value?

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* add datasize lib to help dpkg parsing

added unit tests to expand coverage of dpkg parsing

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* drop parse error

added unit tests to handleNewKeyValue

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* don't return parsing errors from dpkg

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* go mod tidy

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* test higher level functions

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* return parsing err to let cataloger handle it

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* feedback changes

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* ignore key parsing error

log warning with relevant context

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* go mod tidy

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* add context info to log lines

simpler error assertion

Signed-off-by: Jonas Galvão Xavier <[email protected]>

* use error.As to assert error in chain

Signed-off-by: Jonas Galvão Xavier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wagoodman wagoodman wagoodman left review comments

@kzantow kzantow kzantow approved these changes

@spiffcs spiffcs spiffcs approved these changes

Assignees

@jonasagx jonasagx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unable to catalog dpkg package=/var/lib/dpkg/status
4 participants
@jonasagx @wagoodman @kzantow @spiffcs