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

Less verbose logging in Golang Cataloger #904

Merged
merged 3 commits into from
Mar 22, 2022
Merged

Less verbose logging in Golang Cataloger #904

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
482a8cd
Less verbose logging in Golang Cataloger
jonasagx Mar 21, 2022
df3feff
debug for known gray errors
jonasagx Mar 21, 2022
22bd9fc
only show warnings when a binary is not a go executable
wagoodman Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion syft/pkg/cataloger/golang/scan_bin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func scanFile(reader unionReader, filename string) ([]*debug.BuildInfo, []string
for _, r := range readers {
bi, err := buildinfo.Read(r)
if err != nil {
log.Warnf("golang cataloger: scanning file %s: %v", filename, err)
log.Debugf("golang cataloger: scanning file %s: %v", filename, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

So my opinion is that this should be a WARN log entry. If Syft knows for sure that it missed an opportunity to scan and surface a piece of the software, that's noteworthy (the same idea that's culminated in #518). For example, it might be that the scanned Go binaries has no known vulnerabilities... except in the section that Syft missed. It's important to have a way to signal to the user the difference between "Everything is fine ✅ " and "Everything that we scanned is fine, but we weren't able to scan everything. ⚠️ "

Copy link
Contributor

Choose a reason for hiding this comment

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

If Syft knows for sure that it missed an opportunity to scan and surface a piece of the software, that's noteworthy (the same idea that's culminated in #518). For example, it might be that the scanned Go binaries has no known vulnerabilities... except in the section that Syft missed. It's important to have a way to signal to the user the difference between "Everything is fine ✅ " and "Everything that we scanned is fine, but we weren't able to scan everything. ⚠️ "

100% agree.

However, in this case this is a little bit grey. The golang cataloger selects candidate files based on MIME type (get all executables). There is no distinction of the binary being analyzed being a go-compiled binary (we don't know if it should be). Defaulting to warning here seems wrong since it seems to imply that we were expecting that all inputs are go-compiled binaries (which is not true).

That being said, I also agree that we don't want to not report up skipping a file that was intended to be analyzed by the golang cataloger but was not. We should show warnings for these cases.

Given the above context, we should probably default to having this being a debug (though it does really seem like it should be warning at a first glance).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could check the error? If it is a "not a go executable" error, ignore it (no log) but any other error we show as a warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

However, in this case this is a little bit grey. The golang cataloger selects candidate files based on MIME type (get all executables). There is no distinction of the binary being analyzed being a go-compiled binary (we don't know if it should be). Defaulting to warning here seems wrong since it seems to imply that we were expecting that all inputs are go-compiled binaries (which is not true).

That being said, I also agree that we don't want to not report up skipping a file that was intended to be analyzed by the golang cataloger but was not. We should show warnings for these cases.

This part resonates! I hesitate to default to debug regardless — but I do like the idea of distinguishing between two scenarios here. Your last suggestion makes a lot of sense to me! 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of debug not a go executable and warn on any other error. My design north here is: how would things look like once we add more binary catalogers? In this future scenario log messages of the type not a X executable are more noise than information, but we are not there yet.

return nil, nil
}
builds = append(builds, bi)
Expand Down