-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: add support for registry mirrors #8244
base: main
Are you sure you want to change the base?
feat: add support for registry mirrors #8244
Conversation
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 looks great 👍 Left small comments.
@@ -117,3 +117,23 @@ The following example will fail when a critical vulnerability is found or the OS | |||
``` | |||
$ trivy image --exit-code 1 --exit-on-eol 1 --severity CRITICAL alpine:3.16.3 | |||
``` | |||
|
|||
## Mirrors support |
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.
nit
## Mirrors support | |
## Mirror Registries |
|
||
Trivy supports mirrors for [remote container images](../target/container_image.md#container-registry) and [databases](./db.md). | ||
|
||
To configure them, add a list of mirrors along with the host to the [trivy config file](../references/configuration/config-file.md#registry-options). |
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 should explain how it works with some examples. How about orders? When does Trivy fall back into the next mirror? Is the original registry used? And so on.
mirrors: | ||
index.docker.io: | ||
- harbor.example.com/docker.io | ||
- mirror.gcr.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.
Currently, this page shows the default values. I'm wondering if it confuses users. Since the document describes an example, isn't it enough?
case map[string][]string: | ||
return cast.ToStringMapStringSlice(val) |
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 impressed it works!
@@ -31,26 +31,33 @@ var ( | |||
ConfigName: "registry.token", | |||
Usage: "registry token", | |||
} | |||
RegistryMirrorsFlag = Flag[map[string][]string]{ |
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.
Smart 👏
return nil, xerrors.Errorf("platform error: %w", err) | ||
// Try each mirrors/host until it succeeds | ||
for _, r := range append(registryMirrors(ref, option), ref) { | ||
// Try each authentication method until it succeeds |
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.
Instead of nesting it, I prefer adding another method, like tryRef
, tryImage
or something like that.
ctx := hostRef.Context() | ||
reg := ctx.RegistryStr() |
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.
nit: I thought ctx
would be used later in the function, but actually just used to get a registry string. IMO, it's better not to define a new variable here to reduce confusion. But it's no big deal. You can decide it.
ctx := hostRef.Context() | |
reg := ctx.RegistryStr() | |
reg := hostRef.Context().RegistryStr() |
mirrorImageName := strings.Replace(hostRef.Name(), reg, m, 1) | ||
ref, err := name.ParseReference(mirrorImageName, nameOpts...) | ||
if err != nil { | ||
log.WithPrefix("remote").Warn("Unable to parse mirror of image", log.String("mirror", mirrorImageName)) |
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.
This means users specified the wrong registry. I think we should return an error. What do you think?
if err != nil { | ||
return nil, xerrors.Errorf("platform error: %w", err) | ||
// Try each mirrors/host until it succeeds | ||
for _, r := range append(registryMirrors(ref, option), ref) { |
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 make sure that container runtimes work in the same way. Even if it fails due to an authentication error, do they try the next mirror?
Description
registry.mirrors
flag (only for config file)e.g. :
Related issues
Checklist