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

refactor: use google/wire for cache #7024

Merged
merged 6 commits into from
Jun 27, 2024

Conversation

knqyf263
Copy link
Collaborator

Description

This PR simplifies the cache initialization process in the artifact scanning. Previously, we initialized the cache.Cache instance within pkg/commands/artifact/run.go to support --clear-cache and --reset operations. However, these operations have been moved to the trivy clean command.

With this change, we can now generate the cache.Cache instance through google/wire, which is consistent with other instances in the project.

Changes

  • Remove the initCache function from pkg/commands/artifact/run.go
  • Update the wire injection to include cache.Cache generation
  • Adjust the artifact scanning initialization process to use the wire-generated cache instance

Benefits

  • Improved consistency in scanner initialization across the project
  • Simplified code structure in the artifact scanning module
  • Better alignment with the project's dependency injection pattern using wire

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@knqyf263 knqyf263 self-assigned this Jun 26, 2024
@knqyf263 knqyf263 marked this pull request as ready for review June 26, 2024 13:31
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

LGTM
left a couple comments.

pkg/flag/cache_flags.go Outdated Show resolved Hide resolved
pkg/cache/client.go Outdated Show resolved Hide resolved
@@ -144,6 +145,8 @@ func (f *GlobalFlagGroup) ToOptions() (GlobalOptions, error) {
// Keep TRIVY_NON_SSL for backward compatibility
insecure := f.Insecure.Value() || os.Getenv("TRIVY_NON_SSL") != ""

log.Debug("Cache dir", log.String("dir", f.CacheDir.Value()))
Copy link
Contributor

Choose a reason for hiding this comment

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

i am not sure that we need to add this here.
We show cache dir for server mode:

log.Debug("Cache", log.String("dir", opts.CacheDir))

(perhaps it make sense to move this to
func NewRemoteCache(opts RemoteOptions) *RemoteCache {
ctx := client.WithCustomHeaders(context.Background(), opts.CustomHeaders)
httpClient := &http.Client{
Transport: &http.Transport{
Proxy: http.ProxyFromEnvironment,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: opts.Insecure,
},
},
}
c := rpcCache.NewCacheProtobufClient(opts.ServerAddr, httpClient)
return &RemoteCache{
ctx: ctx,
client: c,
}
}
)

Also we show this log for redis cache (cache directory information when using redis can be confusing for users).

But we need to add this log to

func NewFSCache(cacheDir string) (FSCache, error) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also we show this log for redis cache (cache directory information when using redis can be confusing for users).

--cache-dir is effective more than the scan cache. It also affects the vulnerability DB and the Java DB.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We show cache dir for server mode:

Deleted 1ab584e

Copy link
Contributor

Choose a reason for hiding this comment

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

--cache-dir is effective more than the scan cache. It also affects the vulnerability DB and the Java DB.

hm... you are right.
Let's update redis log then (e.g. Redis scan cache):

log.Info("Redis cache", log.String("url", opts.BackendMasked()))

Copy link
Collaborator Author

@knqyf263 knqyf263 Jun 27, 2024

Choose a reason for hiding this comment

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

BTW, I'm not sure scan cache is a good name. I tentatively named it "scan cache" because we needed a different name to distinguish between a general cache, including DB, and a specific cache for storing analysis results (fanal cache), but there are other candidates, such as "layer cache" or "analysis cache". What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like layer cache. User may think that we store layers (I mean files from layers), but we only store layer IDs.

analysis cache looks good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like layer cache. User may think that we store layers (I mean files from layers), but we only store layer IDs.

Right. That's why I didn't choose it.

While "analysis cache" describes the internal logic well, the "analyzer" concept is not exported to end users. I'm not sure people understand "analysis cache". Anyway, I'll change it to Redis scan cache for now. I'll open another issue to discuss the naming.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 2868b08

Copy link
Contributor

@DmitriyLewen DmitriyLewen Jun 27, 2024

Choose a reason for hiding this comment

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

the "analyzer" concept is not exported to end users. I'm not sure people understand "analysis cache"

We, as maintainers, constantly work with analyzers and are accustomed to this term, so perhaps scan cache does not seem to be best choice for us, but perhaps it will be clearer for users

I'll open another issue to discuss the naming.

Anyway, let's listen to the users

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DmitriyLewen Please feel free to leave any comments.
#7034

@knqyf263 knqyf263 added this pull request to the merge queue Jun 27, 2024
Merged via the queue into aquasecurity:main with commit 4be02ba Jun 27, 2024
12 checks passed
@knqyf263 knqyf263 deleted the refactor/wire_cache branch June 27, 2024 07:22
skahn007gl pushed a commit to skahn007gl/trivy that referenced this pull request Jul 23, 2024
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.

2 participants