From 5c95364ed332ca4321f43e9a13f72543edaa0fbe Mon Sep 17 00:00:00 2001 From: Sven Haardiek Date: Mon, 13 Sep 2021 15:38:56 +0200 Subject: [PATCH] Apply Suggestions from Code Review * Use a single configuration key to describe the registry as well as the mirror. * More meaningful error inspection in tests. * Use github.com/google/go-containerregistry to parse image reference. --- .../vulnerability-scanners/trivy.md | 3 +- pkg/plugin/trivy/plugin.go | 72 +++++------------ pkg/plugin/trivy/plugin_test.go | 77 ++++++------------- 3 files changed, 42 insertions(+), 110 deletions(-) diff --git a/docs/integrations/vulnerability-scanners/trivy.md b/docs/integrations/vulnerability-scanners/trivy.md index 3c6d9786b..e4ed41dd5 100644 --- a/docs/integrations/vulnerability-scanners/trivy.md +++ b/docs/integrations/vulnerability-scanners/trivy.md @@ -92,8 +92,7 @@ EOF | `trivy.serverURL` | N/A | The endpoint URL of the Trivy server. Required in `ClientServer` mode. | | `trivy.serverTokenHeader` | `Trivy-Token` | The name of the HTTP header to send the authentication token to Trivy server. Only application in `ClientServer` mode when `trivy.serverToken` is specified. | | `trivy.insecureRegistry.` | N/A | The registry to which insecure connections are allowed. There can be multiple registries with different registry ``. | -| `trivy.mirrors.registry.` | N/A | A registry for which a mirror should be used to get an image. For the mirror url `trivy.mirrors.mirror.` with the matching ``. For each entry there must be a corresponding `trivy.mirrors.mirror.` entry. There can be multiple registries with different registry ``. | -| `trivy.mirrors.mirror.` | N/A | The mirror for the registry with ``. | +| `trivy.registry.mirror.`| N/A | Mirror for the registry ``, e.g. `trivy.registry.mirror.index.docker.io: mirror.io` would use `mirror.io` to get images originated from `index.docker.io` | | `trivy.httpProxy` | N/A | The HTTP proxy used by Trivy to download the vulnerabilities database from GitHub. | | `trivy.httpsProxy` | N/A | The HTTPS proxy used by Trivy to download the vulnerabilities database from GitHub. | | `trivy.noProxy` | N/A | A comma separated list of IPs and domain names that are not subject to proxy settings. | diff --git a/pkg/plugin/trivy/plugin.go b/pkg/plugin/trivy/plugin.go index 3c340b1e7..c58c74d64 100644 --- a/pkg/plugin/trivy/plugin.go +++ b/pkg/plugin/trivy/plugin.go @@ -31,8 +31,7 @@ const ( keyTrivyIgnoreUnfixed = "trivy.ignoreUnfixed" keyTrivyIgnoreFile = "trivy.ignoreFile" keyTrivyInsecureRegistryPrefix = "trivy.insecureRegistry." - keyTrivyMirrorSrcPrefix = "trivy.mirrors.registry." - keyTrivyMirrorDstPrefix = "trivy.mirrors.mirror." + keyTrivyMirrorPrefix = "trivy.registry.mirror." keyTrivyHTTPProxy = "trivy.httpProxy" keyTrivyHTTPSProxy = "trivy.httpsProxy" keyTrivyNoProxy = "trivy.noProxy" @@ -107,26 +106,15 @@ func (c Config) GetInsecureRegistries() map[string]bool { return insecureRegistries } -func (c Config) GetMirrors() (map[string]string, error) { +func (c Config) GetMirrors() map[string]string { res := make(map[string]string) - for registyKey, registry := range c.Data { - if !strings.HasPrefix(registyKey, keyTrivyMirrorSrcPrefix) { + for registryKey, mirror := range c.Data { + if !strings.HasPrefix(registryKey, keyTrivyMirrorPrefix) { continue } - mirrorKey := fmt.Sprintf( - "%v%v", - keyTrivyMirrorDstPrefix, - strings.TrimPrefix(registyKey, keyTrivyMirrorSrcPrefix), - ) - - mirror, ok := c.Data[mirrorKey] - if !ok { - return res, fmt.Errorf("mirror %v missing for %v", mirrorKey, registyKey) - } - - res[registry] = mirror + res[strings.TrimPrefix(registryKey, keyTrivyMirrorPrefix)] = mirror } - return res, nil + return res } // GetResourceRequirements creates ResourceRequirements from the Config. @@ -530,7 +518,7 @@ func (p *plugin) getPodSpecForStandaloneMode(config Config, spec corev1.PodSpec, return corev1.PodSpec{}, nil, err } - mirrors, err := config.GetMirrors() + optionalMirroredImage, err := GetMirroredImage(c.Image, config.GetMirrors()) if err != nil { return corev1.PodSpec{}, nil, err } @@ -551,7 +539,7 @@ func (p *plugin) getPodSpecForStandaloneMode(config Config, spec corev1.PodSpec, "--quiet", "--format", "json", - GetMirroredImage(c.Image, mirrors), + optionalMirroredImage, }, Resources: resourceRequirements, VolumeMounts: volumeMounts, @@ -805,7 +793,7 @@ func (p *plugin) getPodSpecForClientServerMode(config Config, spec corev1.PodSpe return corev1.PodSpec{}, nil, err } - mirrors, err := config.GetMirrors() + optionalMirroredImage, err := GetMirroredImage(container.Image, config.GetMirrors()) if err != nil { return corev1.PodSpec{}, nil, err } @@ -826,7 +814,7 @@ func (p *plugin) getPodSpecForClientServerMode(config Config, spec corev1.PodSpe "json", "--remote", trivyServerURL, - GetMirroredImage(container.Image, mirrors), + optionalMirroredImage, }, VolumeMounts: volumeMounts, Resources: requirements, @@ -981,42 +969,18 @@ func GetScoreFromCVSS(CVSSs map[string]*CVSS) *float64 { return nvdScore } -func fullImagePath(image string) string { - const defaultRegistry = "index.docker.io" - registryAndImage := strings.Split(image, "/") - // Empty string - if len(registryAndImage) == 0 { - return image - } - - // Add default Registry, if none is present - if len(registryAndImage) == 1 || - !(strings.Contains(registryAndImage[0], ".") || - strings.Contains(registryAndImage[0], ":") || - registryAndImage[0] == "localhost") { - registryAndImage = append([]string{defaultRegistry}, registryAndImage...) - } - - // Add "library" if only image name is given and registry is DockerHub - if len(registryAndImage) == 2 && - (strings.HasSuffix(registryAndImage[0], "docker.io") || - strings.HasSuffix(registryAndImage[0], "docker.com")) { - registryAndImage = append(registryAndImage[:2], registryAndImage[1:]...) - registryAndImage[1] = "library" +func GetMirroredImage(image string, mirrors map[string]string) (string, error) { + ref, err := name.ParseReference(image) + if err != nil { + return "", err } - - return strings.Join(registryAndImage, "/") -} - -func GetMirroredImage(image string, mirrors map[string]string) string { - mirroredImage := fullImagePath(image) - + mirroredImage := ref.Name() for k, v := range mirrors { if strings.HasPrefix(mirroredImage, k) { mirroredImage = strings.Replace(mirroredImage, k, v, 1) - return mirroredImage + return mirroredImage, nil } } - // If nothing is mirrord, we can simply use the input image without the fullpath. - return image + // If nothing is mirrord, we can simply use the input image. + return image, nil } diff --git a/pkg/plugin/trivy/plugin_test.go b/pkg/plugin/trivy/plugin_test.go index 9446e8c9e..b809b5527 100644 --- a/pkg/plugin/trivy/plugin_test.go +++ b/pkg/plugin/trivy/plugin_test.go @@ -266,7 +266,6 @@ func TestConfig_GetMirrors(t *testing.T) { name string configData trivy.Config expectedOutput map[string]string - expectError bool }{ { name: "Should return empty map when there is no key with trivy.mirrors.registry. prefix", @@ -281,32 +280,16 @@ func TestConfig_GetMirrors(t *testing.T) { name: "Should return mirrors in a map", configData: trivy.Config{PluginConfig: starboard.PluginConfig{ Data: map[string]string{ - "trivy.mirrors.registry.docker": "docker.io", - "trivy.mirrors.mirror.docker": "mirror.io", + "trivy.registry.mirror.docker.io": "mirror.io", }, }}, expectedOutput: map[string]string{"docker.io": "mirror.io"}, }, - { - name: "Should return error, if mirror is missing for registry", - configData: trivy.Config{PluginConfig: starboard.PluginConfig{ - Data: map[string]string{ - "trivy.mirrors.registry.docker": "docker.io", - }, - }}, - expectError: true, - }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - mirrors, err := tc.configData.GetMirrors() - if tc.expectError { - assert.Error(t, err) - } else { - assert.Equal(t, tc.expectedOutput, mirrors) - } - + assert.Equal(t, tc.expectedOutput, tc.configData.GetMirrors()) }) } } @@ -1213,8 +1196,7 @@ CVE-2019-1543`, "trivy.resources.limits.cpu": "500m", "trivy.resources.limits.memory": "500M", - "trivy.mirrors.registry.dockerio": "index.docker.io", - "trivy.mirrors.mirror.dockerio": "mirror.io", + "trivy.registry.mirror.index.docker.io": "mirror.io", }, workloadSpec: corev1.PodSpec{ Containers: []corev1.Container{ @@ -2280,53 +2262,40 @@ func TestGetScoreFromCVSS(t *testing.T) { func TestGetMirroredImage(t *testing.T) { testCases := []struct { - name string - image string - mirrors map[string]string - expected string + name string + image string + mirrors map[string]string + expected string + expectedError string }{ { - name: "no matching mirror, same image", - image: "index.docker.io/library/alpine", - mirrors: map[string]string{"gcr.io": "mirror.io"}, - expected: "index.docker.io/library/alpine", - }, - { - name: "matching mirror, changed image", - image: "index.docker.io/library/alpine", - mirrors: map[string]string{"index.docker.io": "mirror.io"}, - expected: "mirror.io/library/alpine", - }, - { - name: "no matching mirror, default registry", - image: "library/alpine", + name: "Mirror not match", + image: "alpine", mirrors: map[string]string{"gcr.io": "mirror.io"}, - expected: "library/alpine", + expected: "alpine", }, { - name: "matching mirror, default registry", - image: "library/alpine", - mirrors: map[string]string{"index.docker.io": "mirror.io"}, - expected: "mirror.io/library/alpine", - }, - { - name: "matching mirror, default registry, expanded image name", + name: "Mirror match", image: "alpine", mirrors: map[string]string{"index.docker.io": "mirror.io"}, - expected: "mirror.io/library/alpine", + expected: "mirror.io/library/alpine:latest", }, { - name: "matching mirror, no expanded image name", - image: "quay.io/alpine", - mirrors: map[string]string{"quay.io": "mirror.io"}, - expected: "mirror.io/alpine", + name: "Broken image", + image: "alpine@sha256:broken", + expectedError: "could not parse reference: alpine@sha256:broken", }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - expected := trivy.GetMirroredImage(tc.image, tc.mirrors) - assert.Equal(t, tc.expected, expected) + expected, err := trivy.GetMirroredImage(tc.image, tc.mirrors) + if tc.expectedError != "" { + require.EqualError(t, err, tc.expectedError) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expected, expected) + } }) } }