-
Notifications
You must be signed in to change notification settings - Fork 576
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
Fail when grype cant check for db update #1247
Conversation
Closes anchore#310 Signed-off-by: Shane Dell <[email protected]>
60a8519
to
e38fdba
Compare
grype/db/curator.go
Outdated
log.Warnf("unable to check for vulnerability database update") | ||
log.Debugf("check for vulnerability update failed: %+v", err) | ||
return false, fmt.Errorf("unable to update vulnerability database: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to only return an error if there is no database to use after this check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But wouldn't you still want return an error if unable to update the database? For example if I had an old outdated database locally and tried to update it but it failed to update, say from a dns error like in this example. If the error is not thrown the command would return that No vulnerability database update available
which would be misleading. So this could also cause the user to believe their outdated database is up to date as the returned value of No vulnerability database update available
, at least to me, makes me think I have the latest database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kzantow Or would something like this work?
stage.Current = "no update available"
updateAvailable, metadata, updateEntry, err := c.IsUpdateAvailable()
if err != nil {
log.Warnf("unable to check for vulnerability database update")
log.Debugf("check for vulnerability update failed: %+v", err)
stage.Current = fmt.Sprintf("unable to update vulnerability database: %w", err)
}
...
if stage.Current != "no update available" {
return false, errors.New(state.Current)
} else {
return false, nil
}
?
The "Fail when grype cant check for db update" issue continues to be sporadic and have been occuring across Grype versions. Recently, I'm facing a similar issue when tried using Grype v0.61.1. On my Ubuntu 18x VM it worked smoothly but it does not work on my RHEL 8x VM. All my VMs are on the same network. Also when exploring Kubeclarity that integrates with Grype, a similar issue blocks me when I try to setup kubeclarity-grype-server. Below are my K8s logs.
|
Hi all -- what if we modified the
Would this be an acceptable solution where grype itself will still be able to function if there is a slightly old db downloaded (it already complains if the db is too old)? |
Agreed with @kzantow we should not have an exit code of 1 in this case -- grype should work offline without error. We have a separate validator to not use stale DBs incase you're offline for too long (which is configurable). The issue is more about ensuring the UI doesn't lie to the user and say "there is no DB update available" when it didn't check. Instead the UI should really be updated to be clear that it was not able to check for an update and gracefully continue processing. @shanedell did you want to update this PR to account for the UI update? if not, we can also close this PR instead. |
Wanted to add that #1463 will make it so that Maybe |
Signed-off-by: Keith Zantow <[email protected]>
cmd/grype/cli/commands/db_update.go
Outdated
@@ -26,7 +26,9 @@ func DBUpdate(app clio.Application) *cobra.Command { | |||
} | |||
|
|||
func runDBUpdate(opts options.Database) error { | |||
dbCurator, err := db.NewCurator(opts.ToCuratorConfig()) | |||
cfg := opts.ToCuratorConfig() | |||
cfg.RequireUpdateCheck = true // always consider an update check failure a db update failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to move this to PreRunE so that the config is printed with this change in debug logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out the PreRunE didn't work. I think, though, that any grype db <command>
which happens to run the db update should, by default, consider failure to check for an update to be an update failure, so moved the default configuration to the appropriate place. Note that this does not currently apply to running the normal grype scan, only the grype db
operations (and of those, only the grype db update
is using the function in question). This also allows users to disable this behavior if they really want to, using an explicitly configured GRYPE_DB_REQUIRE_UPDATE_CHECK=false
...
Signed-off-by: Keith Zantow <[email protected]>
Fail when grype cant check for db update
Closes #310
Before change when running:
It would output:
and exit with 0.
After update when running:
the output is
with exit code 1.