-
Notifications
You must be signed in to change notification settings - Fork 593
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
Add v6 DB curator #2151
Add v6 DB curator #2151
Conversation
881d98b
to
8b398b3
Compare
e89455d
to
3a38d98
Compare
bc7ea6a
to
adc9785
Compare
b08b288
to
e834ef7
Compare
87cb469
to
2c1e5c7
Compare
c.setLastSuccessfulUpdateCheck() | ||
} | ||
|
||
mon.Set("no update available") |
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.
if update == nil && checkErr != nil
, we don't have any update time updates for last unsuccessful update check. we probably want to track this too such as not to DoS if we retry for any reason
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.
The point is valid but I don't know what a reasonable low-pass filter is for a case like this. I think this can be a future enhancement.
grype/db/v6/installation/curator.go
Outdated
} | ||
|
||
func readDatabaseDescription(fs afero.Fs, dir string) (*db.Description, error) { | ||
metadataFilePath := path.Join(dir, db.DescriptionFileName) |
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'm just going to ask: does reading any data like db build time directly from the db help anything, or make anything more challenging aside from needing to open a SQLite db instead of just JSON? I keep thinking certain aspects could be simplified if an external file isn't needed. One minor concern I have is that we never validate that we can actually read the decompressed db -- curator.Status() calls this
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've made a lot of changes based on this comment -- though the execution path is still dependent on the v6.Descirption
the way this description is gathered now has changed. I've replaced the description.json
file with a checksums file (vulnerability.db.checksum
). This is not distributed along the DB within the tar, instead this is calculated after downloading the tar and unarchiving. This means that there are two new important functions v6.CalculateDescription
and v6.ReadDescription
, where both depend on the DB being there, the data that can be fetched from the DB is, and the checksums is either read or calculated based on the current DB in place.
Overall we now have the following qualities:
- now we have a single source of truth for metadata that can be stored in the DB
- we exercise the DB on the validate path
16b947d
to
cb1084c
Compare
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
cb1084c
to
9edb689
Compare
force pushed to rebase after some of the store PRs landed with conflicts |
Signed-off-by: Alex Goodman <[email protected]>
|
||
log.Infof("downloading new vulnerability DB") | ||
mon.Set("downloading") | ||
dest, err := c.client.Download(*update, filepath.Dir(c.config.DBRootDir), mon.downloadProgress) |
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.
no change needed at the moment, but just noting an incongruity with the afero.FS
, above, that lead to some frustrations testing scenarios/etc. when I was looking at something related
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.
agreed, I also have no solution for this
Adds in a new DB curator for v6, which is responsible for:
distribution.Client
to facilitate validated updates of the DBPartially implements #2125