-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat: support multiple DB repositories for vulnerability and Java DB #7605
Conversation
pkg/db/db.go
Outdated
} | ||
continue | ||
} | ||
return nil |
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.
Should we also have a debug log to say where the DB was downloaded from?
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.
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 meant to say log the success of the DB. Something along the lines of "Successfully download from ... " as a debug print. But it's just a nit.
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.
Got it now, I added d75a584
ca4c363
to
95df470
Compare
pkg/db/db.go
Outdated
if c.artifact != nil { | ||
return c.artifact.Download(ctx, dst, downloadOpt) | ||
} |
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 need this?
We do same in initOCIArtifact:
Lines 202 to 204 in 95df470
if c.artifact != nil { | |
return c.artifact, nil | |
} |
+
Lines 237 to 239 in 95df470
if err := a.Download(ctx, dst, downloadOpt); err != nil { | |
log.Error("Failed to download DB", log.String("repo", repo.String()), log.Err(err)) | |
if i < len(c.dbRepositories)-1 { |
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 we already have one artifact initialized with a repository via WithOCIArtifact
, there is no point in looping through repositories.
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.
Maybe I didn't make it clear.
You have duplicate code.
Can we keep c.artifact != nil
check and a.Download
function in just one place?
Lines 226 to 228 in 95df470
if c.artifact != nil { | |
return c.artifact.Download(ctx, dst, downloadOpt) | |
} |
Lines 202 to 204 in 95df470
if c.artifact != nil { | |
return c.artifact, nil | |
} |
and
Line 227 in 95df470
return c.artifact.Download(ctx, dst, downloadOpt) |
Line 237 in 95df470
if err := a.Download(ctx, dst, downloadOpt); err != nil { |
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.
@DmitriyLewen If the artifact already exists (manually created), then we don't need to create it here and we can just download it instead of trying to load it from possible repositories.
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.
@DmitriyLewen Done b08004a
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
c9aff65
to
50ad0d6
Compare
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.
lgtm, just left one nit comment.
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
I'll review it tomorrow. |
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
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.
LGTM
pkg/db/db.go
Outdated
|
||
for i, art := range arts { | ||
log.Info("Downloading vulnerability DB...", log.String("repo", art.Repository())) | ||
if err := art.Download(ctx, dst, oci.DownloadOption{MediaType: dbMediaType}); err != nil { |
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.
We should try the next repository only when the error is 429 or 5xx, like this. Can we extract status code by using transport.Error with errors.As? Temporary may help.
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 think we can move this error handling when downloading an artifact.
pkg/db/db.go
Outdated
for _, repo := range c.dbRepositories { | ||
a, err := c.initOCIArtifact(repo, opt) | ||
if err != nil { | ||
return nil, err | ||
} | ||
artifacts = append(artifacts, a) | ||
} | ||
return artifacts, nil |
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.
All OCI artifacts are always initialized now, even if the primary registry works correctly. It means we waste HTTP calls. What if delaying initialization when needed? Please let me know if I'm missing something.
diff --git a/pkg/db/db.go b/pkg/db/db.go
index 46c2a0584..716dd5d71 100644
--- a/pkg/db/db.go
+++ b/pkg/db/db.go
@@ -42,7 +42,7 @@ var (
)
type options struct {
- artifact *oci.Artifact
+ artifact *oci.Artifact // For testing purpose only
dbRepositories []name.Reference
}
@@ -199,6 +199,10 @@ func (c *Client) updateDownloadedAt(ctx context.Context, dbDir string) error {
}
func (c *Client) initOCIArtifact(repository name.Reference, opt types.RegistryOptions) (*oci.Artifact, error) {
+ if c.artifact != nil {
+ return c.artifact, nil // For unit tests
+ }
+
art, err := oci.NewArtifact(repository.String(), c.quiet, opt)
// TODO: NewArtifact never returns an error
if err != nil {
@@ -218,30 +222,12 @@ func (c *Client) initOCIArtifact(repository name.Reference, opt types.RegistryOp
return art, nil
}
-func (c *Client) initArtifacts(opt types.RegistryOptions) ([]*oci.Artifact, error) {
- if c.artifact != nil {
- return []*oci.Artifact{c.artifact}, nil
- }
-
- artifacts := make([]*oci.Artifact, 0, len(c.dbRepositories))
-
- for _, repo := range c.dbRepositories {
- a, err := c.initOCIArtifact(repo, opt)
+func (c *Client) downloadDB(ctx context.Context, opt types.RegistryOptions, dst string) error {
+ for i, repo := range c.dbRepositories {
+ art, err := c.initOCIArtifact(repo, opt)
if err != nil {
- return nil, err
+ return xerrors.Errorf("failed to initialize OCI artifact: %w", err)
}
- artifacts = append(artifacts, a)
- }
- return artifacts, nil
-}
-
-func (c *Client) downloadDB(ctx context.Context, opt types.RegistryOptions, dst string) error {
- arts, err := c.initArtifacts(opt)
- if err != nil {
- return err
- }
-
- for i, art := range arts {
log.Info("Downloading vulnerability DB...", log.String("repo", art.Repository()))
if err := art.Download(ctx, dst, oci.DownloadOption{MediaType: dbMediaType}); err != nil {
log.Error("Failed to download DB", log.String("repo", art.Repository()), log.Err(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.
Client.artifact
is for testing purposes only. We don't need to expand it to a slice. It means it's okay to return the same instance, but we can change it if we really need it.
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.
@knqyf263 Artifact initialization does not cause http requests and never returns an error. https://github.com/aquasecurity/trivy/blob/main/pkg/oci/artifact.go#L60-L70 If necessary, I can do some refactoring in 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.
Hmm. We forgot to fix the error handling. I'll open a PR now.
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.
artifact can be passed through the WithOCIArtifact option, which is public. Can anyone use it?
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.
Opened #7615
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.
Merged. #7615
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.
artifact can be passed through the WithOCIArtifact option, which is public. Can anyone use it?
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.
@nikpivkin Yes, anybody can use it, and it's actually used in several places.
Line 175 in fc6b3a7
client := db.NewClient(dbDir, true, db.WithOCIArtifact(art)) |
trivy/pkg/rpc/server/listen_test.go
Line 97 in 4c6e8ca
client := db.NewClient(dbDir, true, db.WithOCIArtifact(art)) |
pkg/flag/db_flags.go
Outdated
Name: "db-repository", | ||
ConfigName: "db.repository", | ||
Default: db.DefaultRepository, | ||
Usage: "OCI repository to retrieve trivy-db from", | ||
Default: []string{db.DefaultGHCRRepository, db.DefaultECRRepository}, |
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.
We're unsure how the free tier in ECR Public works, so we should probably avoid adding ECR for now. Instead, we'll document how to use ECR Public in another PR.
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
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 want to improve logging a bit. I'll open another PR soon.
fields: fields{ | ||
SkipDBUpdate: true, | ||
DownloadDBOnly: false, | ||
DBRepository: []string{"ghcr.io/aquasecurity/trivy-db:2", "gallery.ecr.aws/aquasecurity/trivy-db:2"}, |
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 2nd default repository is wrong, it should be "public.ecr.aws/aquasecurity/trivy-db:2"
If I add a typo to the first URL, it fails to try the second. Is that expected?
|
Hello @stewartcampbell |
Thanks @DmitriyLewen |
Description
Usage:
Example:
Downloading an artifact from another repository when receiving error code 429:
Related issues
Related PRs
Checklist