-
Notifications
You must be signed in to change notification settings - Fork 40
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
Clean trivy cache before each cycle #2168
Conversation
71782b7
to
fccff44
Compare
📊 Allure Report - 💚 No failures were reported.
|
c08ac8f
to
ff19585
Compare
e960abf
to
da8bb81
Compare
internal/vulnerability/runner.go
Outdated
@@ -33,7 +34,13 @@ type VulnerabilityRunner struct { | |||
} | |||
|
|||
func NewVulnerabilityRunner(log *logp.Logger) (VulnerabilityRunner, error) { | |||
ctx := context.Background() // TODO: carry |
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.
Todo 😁
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.
Yeah, context.Background was used in this constructor function before these changes and thus the todo, because is out of the scope of this pr.
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.
Please open a follow-up task instead of todo 🙏
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.
ok removed the comment and I will open a task
internal/vulnerability/runner.go
Outdated
log.Debug("NewVulnerabilityRunner: New") | ||
|
||
if err := clearTrivyCache(ctx, log); err != nil { | ||
log.Warnf("error during runner cache clearing %s", err.Error()) |
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.
Why is this a warning and not an error? I mean, this can lead to out of memory, right?
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.
Not necessarily; it depends on the error. Since it's not critical to cycle workflow, I used the warning level.
Update: changed it to Errorf
.
internal/vulnerability/runner.go
Outdated
} | ||
} | ||
|
||
// it should never go here (NewRunner should always return artifact.SkipScan and nil runner), but just in case it goes, lets close the runner. |
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 skip scan is always expected, I would invert the flow to highlight as main flow.
So
if errors.Is(err, trivy.SkipScan) {
continue
}
// handle error and close r here then
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 was like that at first 6bd6349
But then I wanted to be 100% sure that if a runner is returned, we should close it.
Because there can be only one runner instance to hold the cache bbolt db open. So if for some reason, a runner instance is not closed, then no other instance can be initialized and it holds at the db open step for ever (happened to me during testing). So this is a really critical thing and we need to be 100% sure we will avoid it.
For the same reason I also wrote the Also Note
in the description but I failed to describe that the opposite is also a for-ever-blocking situation (if something holds the db open, then the runner we use will never be initialized).
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.
Also, don't we have any tests covering this?
@moukoublen would that change mean we download the entire vuln DB for every cycle? |
@eyalkraft I thought the DB is being downloaded on each cycle either way. But I will check and perhaps remove the db reset option. @romulets I couldn't think of a way to test this, to be honest, other than the already existing test (constructor does not return an error). I couldn't think of a way to safely test that "ensure that the cache is cleared after init". |
Apparently, one of the two databases being downloaded —the Java one— is cached for three days, so in that case, yes, we would lose that. The other one is downloaded on every run, no matter what. example log
So I removed the |
e4a8fb5
to
958d698
Compare
958d698
to
0840408
Compare
0840408
to
3d64363
Compare
Summary of your changes
Clean trivy cache after each cycle
Trivy artifact.NewRunner calls initCache.
Depending on the provided configuration, the latter (initCache) could either return a runner or the error
SkipScan
(and nil runner).In case one of the below flags is
true
, theartifact.NewRunner
will perform the relevant clean/reset operation and returnSkipScan
.CacheOptions.ClearCache
DBOptions.Reset
MisconfOptions.ResetPolicyBundle
This means that:
initCache
, called insideartifact.NewRunner
. We cannot execute them standalonely.SkipScan
andnil
runner, which means we cannot set the flags and get a valid runner with a single call (that's why I created a separate functionClearCache
in my code changes).initCache
could perform only one of each clean/reset operation and then returnSkipScan
, which means we cannot perform a singleartifact.NewRunner
call and init all of those flags to true and expect all those clear/reset operations to run. We need to do one at a time (that's why I created the loop insideClearCache
in my code changes).Also Note:
initCache
with reset/clean flags could run only when no other bbolt client "holds" open the db. So it must be run before we create theartifact.NewRunner
.Screenshot/Data
With this PR the below happens at the beginning of the cycle:
Related Issues
Fixes: #2142
Checklist
Introducing a new rule?