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

feat: add support for registry mirrors #8244

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

DmitriyLewen
Copy link
Contributor

@DmitriyLewen DmitriyLewen commented Jan 14, 2025

Description

  • added registry.mirrors flag (only for config file)
    e.g. :
    registry:
      mirrors:
        index.docker.io:
          - harbor.example.com/docker.io
          - mirror.gcr.io
    
  • add logic to check mirrors. If can't get image from mirrors - check host registry.

Related issues

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).

@DmitriyLewen DmitriyLewen self-assigned this Jan 14, 2025
@DmitriyLewen DmitriyLewen marked this pull request as ready for review January 15, 2025 11:17
Copy link
Collaborator

@knqyf263 knqyf263 left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
## 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).
Copy link
Collaborator

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.

Comment on lines +464 to +467
mirrors:
index.docker.io:
- harbor.example.com/docker.io
- mirror.gcr.io
Copy link
Collaborator

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?

Comment on lines +164 to +165
case map[string][]string:
return cast.ToStringMapStringSlice(val)
Copy link
Collaborator

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]{
Copy link
Collaborator

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
Copy link
Collaborator

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.

Comment on lines +147 to +148
ctx := hostRef.Context()
reg := ctx.RegistryStr()
Copy link
Collaborator

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.

Suggested change
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))
Copy link
Collaborator

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) {
Copy link
Collaborator

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?

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.

support registry mirrors for image scanning
2 participants