Skip to content
New issue

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

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

Already on GitHub? # to your account

Retain cataloged SBOM relationships #1509

Conversation

houdini91
Copy link
Contributor

Hi,
I noticed using the sbom cataloger the relations that are read from the sbom are not aggregated in to the Syft model sbom.

I have a small fix suggestion, not sure if there was a reason behind it or simply a typo.

Are there any tests you would like me to add for this PR?

Signed-off-by: houdini91 mdstrauss91@gmail.com

@kzantow
Copy link
Contributor

kzantow commented Jan 31, 2023

@houdini91 this looks good but are you able to fix the tests?

@houdini91
Copy link
Contributor Author

@kzantow No problem.
Trying to figure out what the best why to do this.
I added a commit that should change the diff between the relationships from a full diff compare to a subset diff compare between the expected and actual relationships.

I can also go and push all the relationships statically in to the test but there are like 150 so it will be a large list.

Any suggestion are welcome.

Comment on lines 183 to 185
if err := SubsetRelationship(p.expectedRelationships, relationships, p.compareOptions...); err != nil {
diff := cmp.Diff(p.expectedRelationships, relationships, p.compareOptions...)
t.Errorf("unexpected %s relationships from parsing (-expected +actual)\n%s", err, diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure this is ideal -- if there is a diff, the diff will be very noisy and not represent what the check is actually doing. Could we just update the individual tests to have the right expected relationships?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Np, do you want me to as simply 150 relationship to the static go code? Should we remove some from the sbom?

Copy link
Contributor Author

@houdini91 houdini91 Feb 2, 2023

Choose a reason for hiding this comment

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

Fyi the diff line in the snippet your referring is simply to output for failed subset check above it was simpler for me to use it's full diff output for test outpu
But I understand you concern about the subset comparison

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be good with updating the source SBOM to make a manageable set of relationships to add explicitly to the tests.

@kzantow
Copy link
Contributor

kzantow commented Feb 2, 2023

@houdini91 I think you may also just need to run make lint-fix for the static analysis failure

@houdini91 houdini91 force-pushed the suggestion/sbom_cataloger_relationships_fix branch from 8b2d0ce to f7dae77 Compare May 14, 2023 10:20
Signed-off-by: Eitan Goldenstein <eitan@scribesecurity.com>
@houdini91 houdini91 force-pushed the suggestion/sbom_cataloger_relationships_fix branch from 9222893 to f44839e Compare May 15, 2023 07:26
@houdini91
Copy link
Contributor Author

@kzantow Hi, sorry for the delay, i admittedly forgot about this PR.
Fixed your suggestions, rebased and squashed (i forgot to sign some commit).
Have a look seem like the tests pass.

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 👍

@kzantow kzantow merged commit b4ed599 into anchore:main May 15, 2023
@kzantow
Copy link
Contributor

kzantow commented May 15, 2023

Thanks @houdini91 !

@kzantow kzantow changed the title add sbom artifacts to model Retain cataloged SBOM relationships May 15, 2023
spiffcs added a commit that referenced this pull request May 15, 2023
* main:
  fix: cyclonedx depends-on relationship inverted (#1816)
  fix: retain sbom cataloger relationships (#1509)
spiffcs added a commit that referenced this pull request May 18, 2023
* main: (32 commits)
  chore(deps): bump github.com/google/go-containerregistry (#1823)
  chore(deps): bump github.com/sirupsen/logrus from 1.9.0 to 1.9.1 (#1822)
  chore(deps): bump github.com/docker/docker (#1824)
  fix: update field plurality of 8.0.0 schema before release (#1820)
  fix: update cataloger to check for expressions before split (#1819)
  feat: update syft license concept to complex struct (#1743)
  fix: cyclonedx depends-on relationship inverted (#1816)
  fix: retain sbom cataloger relationships (#1509)
  feat: warn if parsing newer SBOM (#1810)
  feat: Add R cataloger (#1790)
  update cosign to v2 release (different go module) (#1805)
  fix: Reduce log spam on unknown relationship type (#1797)
  chore(deps): update bootstrap tools to latest versions (#1807)
  chore(deps): bump golang.org/x/net from 0.9.0 to 0.10.0 (#1802)
  chore(deps): bump github.com/docker/docker (#1795)
  chore(deps): bump github.com/google/go-containerregistry (#1796)
  chore(deps): update bootstrap tools to latest versions (#1792)
  Print package list when extra packages found (#1791)
  chore(deps): update bootstrap tools to latest versions (#1786)
  chore(deps): bump golang.org/x/term from 0.7.0 to 0.8.0 (#1787)
  ...

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@wagoodman wagoodman added the bug Something isn't working label May 22, 2023
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
Signed-off-by: Eitan Goldenstein <eitan@scribesecurity.com>
Co-authored-by: Eitan Goldenstein <eitan@scribesecurity.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants