From 4ca8d30763b3628aec48e12753f0ca596586886d Mon Sep 17 00:00:00 2001 From: Vitor Rodrigo Vezani Date: Tue, 26 Dec 2023 10:56:44 -0300 Subject: [PATCH] FWI-5471 - Fix CI image owner matching and add multiple owners support (#862) * Add image deduplication logic * bump changelog and version * fix tests * fix slice mutation --- plugins/ci/CHANGELOG.md | 3 +++ plugins/ci/pkg/ci/ci.go | 31 +++++++++++++++++++++++++- plugins/ci/pkg/ci/ci_test.go | 2 +- plugins/ci/pkg/ci/trivy.go | 37 ++++++++++++++++++++----------- plugins/ci/pkg/ci/trivy_test.go | 39 +++++++++++++++++++++++++++++++++ plugins/ci/version.txt | 2 +- 6 files changed, 98 insertions(+), 16 deletions(-) diff --git a/plugins/ci/CHANGELOG.md b/plugins/ci/CHANGELOG.md index 3807dcca5..6fe87622d 100644 --- a/plugins/ci/CHANGELOG.md +++ b/plugins/ci/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +## 5.3.4 +* Fix image owners matching logic + ## 5.3.3 * Trim spaces from masterBranch before using it diff --git a/plugins/ci/pkg/ci/ci.go b/plugins/ci/pkg/ci/ci.go index 060f0b6f9..bdbf5e66c 100644 --- a/plugins/ci/pkg/ci/ci.go +++ b/plugins/ci/pkg/ci/ci.go @@ -198,7 +198,36 @@ func (ci *CIScan) getAllResources() ([]trivymodels.Image, []models.Resource, err errors = multierror.Append(errors, fmt.Errorf("error walking directory %s: %v", ci.configFolder, err)) return nil, nil, errors } - return images, resources, errors.ErrorOrNil() + + // multiple images with the same name may belong to different owners, so we need to deduplicate them + dedupedImages := dedupImages(images) + + return dedupedImages, resources, errors.ErrorOrNil() +} + +func dedupImages(images []trivymodels.Image) []trivymodels.Image { + imageOwnersMap := map[string][]trivymodels.Resource{} + for _, img := range images { + if _, ok := imageOwnersMap[img.Name]; !ok { + imageOwnersMap[img.Name] = []trivymodels.Resource{} // initialize + } + imageOwnersMap[img.Name] = append(imageOwnersMap[img.Name], img.Owners...) + } + + imagesMap := lo.KeyBy(images, func(img trivymodels.Image) string { return img.Name }) + + dedupedImages := []trivymodels.Image{} + for k, i := range imagesMap { + dedupedImages = append(dedupedImages, trivymodels.Image{ + ID: i.ID, + PullRef: i.PullRef, + Name: i.Name, + Owners: imageOwnersMap[k], + RecommendationOnly: i.RecommendationOnly, + }) + } + + return dedupedImages } func (ci *CIScan) getDisplayFilenameAndHelmName(path string) (string, string, error) { diff --git a/plugins/ci/pkg/ci/ci_test.go b/plugins/ci/pkg/ci/ci_test.go index 622c13e92..cb034b099 100644 --- a/plugins/ci/pkg/ci/ci_test.go +++ b/plugins/ci/pkg/ci/ci_test.go @@ -88,7 +88,7 @@ func TestGetAllResources(t *testing.T) { line 5: mapping key "kind" already defined at line 4 `) - assert.Len(t, images, 3, "even though there are errors, we should still get the images") + assert.Len(t, images, 2, "even though there are errors, we should still get the images") assert.Len(t, resources, 7, "even though there are errors, we should still get the resources") } diff --git a/plugins/ci/pkg/ci/trivy.go b/plugins/ci/pkg/ci/trivy.go index d08754479..821ba5e79 100644 --- a/plugins/ci/pkg/ci/trivy.go +++ b/plugins/ci/pkg/ci/trivy.go @@ -25,12 +25,12 @@ import ( func (ci *CIScan) GetTrivyReport(dockerImages []trivymodels.DockerImage, manifestImages []trivymodels.Image) (report *models.ReportInfo, errs error) { allErrs := new(multierror.Error) - err := updatePullRef(ci.config.Images.FolderName, dockerImages, manifestImages) + dockerImages, manifestImages, err := updatePullRef(ci.config.Images.FolderName, dockerImages, manifestImages) if err != nil { return nil, err } - filenameToImageName, err := downloadMissingImages(ci.config.Images.FolderName, dockerImages, manifestImages, ci.config.Options.RegistryCredentials) + filenameToImageName, dockerImages, manifestImages, err := downloadMissingImages(ci.config.Images.FolderName, downloadImageViaSkopeo, dockerImages, manifestImages, ci.config.Options.RegistryCredentials) if err != nil { allErrs = multierror.Append(allErrs, err) } @@ -89,10 +89,11 @@ func walkImages(folderPath string, cb imageCallback) error { } // updatePullRef looks through image tarballs and mark which ones are already there, by setting image.PullRef -func updatePullRef(folderPath string, dockerImages []trivymodels.DockerImage, manifestImages []trivymodels.Image) error { +func updatePullRef(folderPath string, dockerImages []trivymodels.DockerImage, manifestImages []trivymodels.Image) ([]trivymodels.DockerImage, []trivymodels.Image, error) { logrus.Infof("Looking through images in %s", folderPath) - return walkImages(folderPath, func(filename string, sha string, repoTags []string) { - for _, image := range manifestImages { + err := walkImages(folderPath, func(filename string, sha string, repoTags []string) { + for i := range manifestImages { + image := &manifestImages[i] if slices.Contains(repoTags, image.Name) { // already downloaded logrus.Infof("image (manifest) %s already downloaded", image.Name) @@ -100,7 +101,8 @@ func updatePullRef(folderPath string, dockerImages []trivymodels.DockerImage, ma } } - for _, image := range dockerImages { + for i := range dockerImages { + image := &dockerImages[i] if slices.Contains(repoTags, image.Name) { // already downloaded logrus.Infof("image (docker) %s already downloaded", image.Name) @@ -108,13 +110,18 @@ func updatePullRef(folderPath string, dockerImages []trivymodels.DockerImage, ma } } }) + if err != nil { + return nil, nil, err + } + return dockerImages, manifestImages, nil } -func downloadMissingImages(folderPath string, dockerImages []trivymodels.DockerImage, manifestImages []trivymodels.Image, registryCredentials models.RegistryCredentials) (map[string]string, error) { +func downloadMissingImages(folderPath string, imageDownloaderFunc ImageDownloaderFunc, dockerImages []trivymodels.DockerImage, manifestImages []trivymodels.Image, registryCredentials models.RegistryCredentials) (map[string]string, []trivymodels.DockerImage, []trivymodels.Image, error) { allErrs := new(multierror.Error) refLookup := map[string]string{} // postgres:15.1-bullseye -> postgres_15_1_bullseye // Download missing images - for _, image := range manifestImages { + for i := range manifestImages { + image := &manifestImages[i] if image.PullRef != "" { continue } @@ -123,7 +130,7 @@ func downloadMissingImages(folderPath string, dockerImages []trivymodels.DockerI continue } rc := registryCredentials.FindCredentialForImage(image.Name) - output, err := downloadImageViaSkopeo(commands.ExecWithMessage, folderPath, image.Name, rc) + output, err := imageDownloaderFunc(commands.ExecWithMessage, folderPath, image.Name, rc) if err != nil { allErrs = multierror.Append(allErrs, fmt.Errorf("%v: %s", err, output)) } else { @@ -132,7 +139,8 @@ func downloadMissingImages(folderPath string, dockerImages []trivymodels.DockerI } } - for _, image := range dockerImages { + for i := range dockerImages { + image := &dockerImages[i] if image.PullRef != "" { continue } @@ -141,7 +149,7 @@ func downloadMissingImages(folderPath string, dockerImages []trivymodels.DockerI continue } rc := registryCredentials.FindCredentialForImage(image.Name) - output, err := downloadImageViaSkopeo(commands.ExecWithMessage, folderPath, image.Name, rc) + output, err := imageDownloaderFunc(commands.ExecWithMessage, folderPath, image.Name, rc) if err != nil { allErrs = multierror.Append(allErrs, fmt.Errorf("%v: %s", err, output)) } else { @@ -150,7 +158,7 @@ func downloadMissingImages(folderPath string, dockerImages []trivymodels.DockerI } } if len(allErrs.Errors) > 0 { - return ciutil.ReverseMap(refLookup), models.ScanErrorsReportResult{ + return ciutil.ReverseMap(refLookup), dockerImages, manifestImages, models.ScanErrorsReportResult{ ErrorMessage: allErrs.Error(), // keep multiple errors combined into a single error / action item ErrorContext: "downloading missing images to be scanned by trivy", Kind: "InternalOperation", @@ -159,9 +167,12 @@ func downloadMissingImages(folderPath string, dockerImages []trivymodels.DockerI Filename: filepath.Clean(folderPath), // clean() removes potential trailing slash } } - return ciutil.ReverseMap(refLookup), nil // postgres_15_1_bullseye -> postgres:15.1-bullseye + return ciutil.ReverseMap(refLookup), dockerImages, manifestImages, nil // postgres_15_1_bullseye -> postgres:15.1-bullseye } +// ImageDownloaderFunc - downloads an image and returns the output and error +type ImageDownloaderFunc = func(cmdExecutor cmdExecutor, folderPath, imageName string, rc *models.RegistryCredential) (string, error) + func downloadImageViaSkopeo(cmdExecutor cmdExecutor, folderPath, imageName string, rc *models.RegistryCredential) (string, error) { logrus.Infof("Downloading missing image %s", imageName) dockerURL := "docker://" + imageName diff --git a/plugins/ci/pkg/ci/trivy_test.go b/plugins/ci/pkg/ci/trivy_test.go index f7d9a5ab7..0f78f533f 100644 --- a/plugins/ci/pkg/ci/trivy_test.go +++ b/plugins/ci/pkg/ci/trivy_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/fairwindsops/insights-plugins/plugins/ci/pkg/models" + trivymodels "github.com/fairwindsops/insights-plugins/plugins/trivy/pkg/models" "github.com/stretchr/testify/assert" ) @@ -57,3 +58,41 @@ func TestDownloadImageViaSkopeo(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "[skopeo copy --src-creds my-username:my-password random args docker://postgres:15.1-bullseye docker-archive:./postgres_15_1_bullseye]", cmd) } + +func TestDownloadMissingImages(t *testing.T) { + mockDownloaderFn := func(cmdExecutor cmdExecutor, folderPath, imageName string, rc *models.RegistryCredential) (string, error) { + return "", nil // noop + } + rc := models.RegistryCredentials{} + + dockerImages := []trivymodels.DockerImage{{Name: "postgres:15.1-bullseye", PullRef: "postgres_15_1_bullseye"}, {Name: "nginx:1.23.2-alpine", PullRef: "nginx_1_23_2_alpine"}} + manifestImages := []trivymodels.Image{ + { + Name: "postgres:15.1-bullseye", + Owners: []trivymodels.Resource{{Name: "postgres", Kind: "Deployment", Namespace: "default", Container: "postgres"}}, + }, + { + Name: "nginx:1.23.2-alpine", + Owners: []trivymodels.Resource{{Name: "nginx", Kind: "Deployment", Namespace: "default", Container: "nginx"}}, + }, + { + Name: "alpine:1.24.2", + Owners: []trivymodels.Resource{{Name: "nginx", Kind: "Deployment", Namespace: "default", Container: "nginx"}}, + }, + } + + refToImageName, dockerImages, manifestImages, err := downloadMissingImages("_/images", mockDownloaderFn, dockerImages, manifestImages, rc) + assert.NoError(t, err) + assert.Equal(t, map[string]string{ + "postgres_15_1_bullseye": "postgres:15.1-bullseye", + "nginx_1_23_2_alpine": "nginx:1.23.2-alpine", + "alpine_1_24_2": "alpine:1.24.2", + }, refToImageName) + + assert.Len(t, dockerImages, 2) + assert.Len(t, manifestImages, 3) + + for _, mi := range manifestImages { + assert.NotEmpty(t, mi.PullRef) + } +} diff --git a/plugins/ci/version.txt b/plugins/ci/version.txt index 74664af74..e0a61e6a8 100644 --- a/plugins/ci/version.txt +++ b/plugins/ci/version.txt @@ -1 +1 @@ -5.3.3 +5.3.4