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

remove mod and cargo from image cataloger #539

Merged
merged 5 commits into from
Oct 7, 2021

Conversation

spiffcs
Copy link
Contributor

@spiffcs spiffcs commented Oct 7, 2021

Fixes #464

  • Update test error messaging to be more clear on failure
  • add check for "" language to not pollute language map in test

Old:

Screen Shot 2021-10-07 at 2 22 41 PM

New:

Screen Shot 2021-10-07 at 2 22 31 PM

Signed-off-by: Christopher Angelo Phillips [email protected]

Signed-off-by: Christopher Angelo Phillips <[email protected]>
@github-actions
Copy link

github-actions bot commented Oct 7, 2021

Benchmark Test Results

Benchmark results from the latest changes vs base branch
name                                                   old time/op    new time/op    delta
ImagePackageCatalogers/ruby-gemspec-cataloger-2          1.07ms ± 4%    1.12ms ± 3%  +4.71%  (p=0.016 n=5+5)
ImagePackageCatalogers/python-package-cataloger-2        1.85ms ± 7%    1.88ms ± 3%    ~     (p=0.310 n=5+5)
ImagePackageCatalogers/javascript-package-cataloger-2     521µs ± 3%     534µs ± 3%    ~     (p=0.222 n=5+5)
ImagePackageCatalogers/dpkgdb-cataloger-2                 520µs ± 2%     545µs ± 1%  +4.98%  (p=0.008 n=5+5)
ImagePackageCatalogers/rpmdb-cataloger-2                  537µs ± 7%     565µs ± 2%    ~     (p=0.095 n=5+5)
ImagePackageCatalogers/java-cataloger-2                  11.3ms ± 4%    11.4ms ± 2%    ~     (p=0.690 n=5+5)
ImagePackageCatalogers/apkdb-cataloger-2                  862µs ± 3%     874µs ± 2%    ~     (p=0.690 n=5+5)
ImagePackageCatalogers/go-module-binary-cataloger-2       759ns ± 1%     756ns ± 4%    ~     (p=0.690 n=5+5)

name                                                   old alloc/op   new alloc/op   delta
ImagePackageCatalogers/ruby-gemspec-cataloger-2           146kB ± 0%     146kB ± 0%  +0.20%  (p=0.032 n=5+5)
ImagePackageCatalogers/python-package-cataloger-2         754kB ± 0%     755kB ± 0%  +0.07%  (p=0.008 n=5+5)
ImagePackageCatalogers/javascript-package-cataloger-2     118kB ± 0%     119kB ± 0%  +0.23%  (p=0.008 n=5+5)
ImagePackageCatalogers/dpkgdb-cataloger-2                 132kB ± 0%     133kB ± 0%  +0.25%  (p=0.008 n=5+5)
ImagePackageCatalogers/rpmdb-cataloger-2                  140kB ± 0%     140kB ± 0%  +0.01%  (p=0.016 n=5+4)
ImagePackageCatalogers/java-cataloger-2                  2.74MB ± 0%    2.74MB ± 0%    ~     (p=0.056 n=5+5)
ImagePackageCatalogers/apkdb-cataloger-2                 1.18MB ± 0%    1.18MB ± 0%  +0.00%  (p=0.032 n=5+5)
ImagePackageCatalogers/go-module-binary-cataloger-2        336B ± 0%      336B ± 0%    ~     (all equal)

name                                                   old allocs/op  new allocs/op  delta
ImagePackageCatalogers/ruby-gemspec-cataloger-2           2.41k ± 0%     2.41k ± 0%    ~     (all equal)
ImagePackageCatalogers/python-package-cataloger-2         9.58k ± 0%     9.58k ± 0%    ~     (p=0.683 n=5+5)
ImagePackageCatalogers/javascript-package-cataloger-2     1.99k ± 0%     1.99k ± 0%    ~     (all equal)
ImagePackageCatalogers/dpkgdb-cataloger-2                 2.54k ± 0%     2.54k ± 0%    ~     (all equal)
ImagePackageCatalogers/rpmdb-cataloger-2                  3.25k ± 0%     3.25k ± 0%    ~     (all equal)
ImagePackageCatalogers/java-cataloger-2                   37.5k ± 0%     37.5k ± 0%    ~     (p=0.286 n=5+5)
ImagePackageCatalogers/apkdb-cataloger-2                  2.49k ± 0%     2.49k ± 0%    ~     (p=0.556 n=4+5)
ImagePackageCatalogers/go-module-binary-cataloger-2        9.00 ± 0%      9.00 ± 0%    ~     (all equal)

@spiffcs spiffcs requested a review from a team October 7, 2021 14:25
@kzantow
Copy link
Contributor

kzantow commented Oct 7, 2021

I noticed this mentioned earlier today... I wonder, is there a case we have a build image that we want to scan? Maybe a company has a library of build images that have a bunch of tools ready to go to speed things up for CI? ... or a multi-stage build (which would be trickier to scan in the middle)

@spiffcs
Copy link
Contributor Author

spiffcs commented Oct 7, 2021

I noticed this mentioned earlier today... I wonder, is there a case we have a build image that we want to scan? Maybe a company has a library of build images that have a bunch of tools ready to go to speed things up for CI? ... or a multi-stage build (which would be trickier to scan in the middle)

Interesting. Are you saying that in this case we don't want to remove these catalogers since these intermediary or build images would have things like go.mod or cargo.toml which we would want to capture?

@kzantow
Copy link
Contributor

kzantow commented Oct 7, 2021

@spiffcs yeah it seems like there might be some valid use cases for cataloging these things inside containers to me. unless they're causing a distinct problem, i'd probably leave them in -- oh, especially if we're scanning all layers of an image!

Signed-off-by: Christopher Angelo Phillips <[email protected]>
@spiffcs
Copy link
Contributor Author

spiffcs commented Oct 7, 2021

@wagoodman @luhring You guys brokered this issue.

I'm leaning toward agreeing with @kzantow on this one, but you two might have another context that I'm missing.

Signed-off-by: Christopher Angelo Phillips <[email protected]>
@luhring
Copy link
Contributor

luhring commented Oct 7, 2021

My vote is to proceed with removing them... for now. The intention behind #464 is to make a small change that makes Syft consistent with its own current philosophy as to how image and directory catalogers are determined: image scans show what's positively installed; directory scans show what's described, or what would be installed.

Might someone want to scan a go.mod in an image for some reason? Absolutely. We want to make all of this configurable. That's captured in a separate, larger-scoped ticket: #465.

@spiffcs
Copy link
Contributor Author

spiffcs commented Oct 7, 2021

My vote is to proceed with removing them... for now. The intention behind #464 is to make a small change that makes Syft consistent with its own current philosophy as to how image and directory catalogers are determined: image scans show what's positively installed; directory scans show what's described, or what would be installed.

Might someone want to scan a go.mod in an image for some reason? Absolutely. We want to make all of this configurable. That's captured in a separate, larger-scoped ticket: #465.

#465

Cool, I'll follow up with #465 on the configuration aspect right after. @kzantow does that work with you?

@kzantow
Copy link
Contributor

kzantow commented Oct 7, 2021

No qualms, was just pointing out that there were valid use cases. As long as we're aware 👍

Signed-off-by: Christopher Angelo Phillips <[email protected]>
@spiffcs spiffcs merged commit b25f5b6 into main Oct 7, 2021
@spiffcs spiffcs deleted the 464-remove-file-from-image-cataloger branch October 7, 2021 19:18
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
* remove mod and cargo from image cataloger

Signed-off-by: Christopher Angelo Phillips <[email protected]>

* update test error messages for clear failures

Signed-off-by: Christopher Angelo Phillips <[email protected]>
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.

Remove go and rust catalogers from image cataloger set
4 participants