From 46586f1f1e6cec33b52c8c2054d5dd2f772a1ef6 Mon Sep 17 00:00:00 2001 From: Philip Laine Date: Wed, 7 Aug 2024 16:20:59 +0200 Subject: [PATCH] refactor: FindImages to return errors immediatly Signed-off-by: Philip Laine --- src/pkg/packager/prepare.go | 222 +++++++++++++--------------- src/pkg/packager/prepare_test.go | 80 ++++++++++ src/test/e2e/13_find_images_test.go | 3 +- 3 files changed, 180 insertions(+), 125 deletions(-) diff --git a/src/pkg/packager/prepare.go b/src/pkg/packager/prepare.go index 214258634a..9d1e42c78f 100644 --- a/src/pkg/packager/prepare.go +++ b/src/pkg/packager/prepare.go @@ -34,8 +34,8 @@ import ( "github.com/zarf-dev/zarf/src/types" ) -// imageMap is a map of image/boolean pairs. -type imageMap map[string]bool +var imageSanityCheck = regexp.MustCompile(`(?mi)"image":"([^"]+)"`) +var imageFuzzyCheck = regexp.MustCompile(`(?mi)["|=]([a-z0-9\-.\/:]+:[\w.\-]*[a-z\.\-][\w.\-]*)"`) // FindImages iterates over a Zarf.yaml and attempts to parse any images. func (p *Packager) FindImages(ctx context.Context) (map[string][]string, error) { @@ -54,53 +54,42 @@ func (p *Packager) FindImages(ctx context.Context) (map[string][]string, error) } message.Note(fmt.Sprintf("Using build directory %s", p.cfg.CreateOpts.BaseDir)) - c := creator.NewPackageCreator(p.cfg.CreateOpts, cwd) - if err := helpers.CreatePathAndCopy(layout.ZarfYAML, p.layout.ZarfYAML); err != nil { return nil, err } + c := creator.NewPackageCreator(p.cfg.CreateOpts, cwd) pkg, warnings, err := c.LoadPackageDefinition(ctx, p.layout) if err != nil { return nil, err } - p.cfg.Pkg = pkg - for _, warning := range warnings { message.Warn(warning) } + p.cfg.Pkg = pkg return p.findImages(ctx) } -func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, err error) { - repoHelmChartPath := p.cfg.FindImagesOpts.RepoHelmChartPath - kubeVersionOverride := p.cfg.FindImagesOpts.KubeVersionOverride - whyImage := p.cfg.FindImagesOpts.Why - - imagesMap := make(map[string][]string) - erroredCharts := []string{} - erroredCosignLookups := []string{} - whyResources := []string{} - +// TODO: Refactor to return output string instead of printing inside of function. +func (p *Packager) findImages(ctx context.Context) (map[string][]string, error) { for _, component := range p.cfg.Pkg.Components { - if len(component.Repos) > 0 && repoHelmChartPath == "" { + if len(component.Repos) > 0 && p.cfg.FindImagesOpts.RepoHelmChartPath == "" { message.Note("This Zarf package contains git repositories, " + "if any repos contain helm charts you want to template and " + "search for images, make sure to specify the helm chart path " + "via the --repo-chart-path flag") + break } } - componentDefinition := "\ncomponents:\n" - if err := p.populatePackageVariableConfig(); err != nil { return nil, fmt.Errorf("unable to set the active variables: %w", err) } // Set default builtin values so they exist in case any helm charts rely on them registryInfo := types.RegistryInfo{Address: p.cfg.FindImagesOpts.RegistryURL} - err = registryInfo.FillInEmptyValues() + err := registryInfo.FillInEmptyValues() if err != nil { return nil, err } @@ -117,41 +106,34 @@ func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, ArtifactServer: artifactServer, } + componentDefinition := "\ncomponents:\n" + imagesMap := map[string][]string{} + whyResources := []string{} for _, component := range p.cfg.Pkg.Components { if len(component.Charts)+len(component.Manifests)+len(component.Repos) < 1 { // Skip if it doesn't have what we need continue } - if repoHelmChartPath != "" { + if p.cfg.FindImagesOpts.RepoHelmChartPath != "" { // Also process git repos that have helm charts for _, repo := range component.Repos { matches := strings.Split(repo, "@") if len(matches) < 2 { - message.Warnf("Cannot convert git repo %s to helm chart without a version tag", repo) - continue + return nil, fmt.Errorf("cannot convert the Git repository %s to a Helm chart without a version tag", repo) } - // Trim the first char to match how the packager expects it, this is messy,need to clean up better - repoHelmChartPath = strings.TrimPrefix(repoHelmChartPath, "/") - // If a repo helm chart path is specified, component.Charts = append(component.Charts, v1alpha1.ZarfChart{ Name: repo, URL: matches[0], Version: matches[1], - GitPath: repoHelmChartPath, + // Trim the first char to match how the packager expects it, this is messy,need to clean up better + GitPath: strings.TrimPrefix(p.cfg.FindImagesOpts.RepoHelmChartPath, "/"), }) } } - // matchedImages holds the collection of images, reset per-component - matchedImages := make(imageMap) - maybeImages := make(imageMap) - - // resources are a slice of generic structs that represent parsed K8s resources - var resources []*unstructured.Unstructured - componentPaths, err := p.layout.Components.Create(component) if err != nil { return nil, err @@ -161,56 +143,59 @@ func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, return nil, err } + resources := []*unstructured.Unstructured{} + matchedImages := map[string]bool{} + maybeImages := map[string]bool{} for _, chart := range component.Charts { + valuesFilePaths, err := helpers.RecursiveFileList(componentPaths.Values, nil, false) + if err != nil { + return nil, err + } + for _, valueFilePath := range valuesFilePaths { + err := p.variableConfig.ReplaceTextTemplate(valueFilePath) + if err != nil { + return nil, err + } + } + + // Generate helm templates for this chart helmCfg := helm.New( chart, componentPaths.Charts, componentPaths.Values, - helm.WithKubeVersion(kubeVersionOverride), + helm.WithKubeVersion(p.cfg.FindImagesOpts.KubeVersionOverride), helm.WithVariableConfig(p.variableConfig), ) - err = helmCfg.PackageChart(ctx, component.DeprecatedCosignKeyPath) if err != nil { return nil, fmt.Errorf("unable to package the chart %s: %w", chart.Name, err) } - - valuesFilePaths, _ := helpers.RecursiveFileList(componentPaths.Values, nil, false) - for _, valueFilePath := range valuesFilePaths { - if err := p.variableConfig.ReplaceTextTemplate(valueFilePath); err != nil { - return nil, err - } - } - - // Generate helm templates for this chart chartTemplate, chartValues, err := helmCfg.TemplateChart(ctx) if err != nil { - message.WarnErrf(err, "Problem rendering the helm template for %s: %s", chart.Name, err.Error()) - erroredCharts = append(erroredCharts, chart.Name) - continue + return nil, fmt.Errorf("could not render the Helm template for chart %s: %w", chart.Name, err) } // Break the template into separate resources - yamls, _ := utils.SplitYAML([]byte(chartTemplate)) + yamls, err := utils.SplitYAML([]byte(chartTemplate)) + if err != nil { + return nil, err + } resources = append(resources, yamls...) chartTarball := helm.StandardName(componentPaths.Charts, chart) + ".tgz" - annotatedImages, err := helm.FindAnnotatedImagesForChart(chartTarball, chartValues) if err != nil { - message.WarnErrf(err, "Problem looking for image annotations for %s: %s", chart.URL, err.Error()) - erroredCharts = append(erroredCharts, chart.URL) - continue + return nil, fmt.Errorf("could not look up image annotations for chart URL %s: %w", chart.URL, err) } for _, image := range annotatedImages { matchedImages[image] = true } // Check if the --why flag is set - if whyImage != "" { - whyResourcesChart, err := findWhyResources(yamls, whyImage, component.Name, chart.Name, true) + if p.cfg.FindImagesOpts.Why != "" { + whyResourcesChart, err := findWhyResources(yamls, p.cfg.FindImagesOpts.Why, component.Name, chart.Name, true) if err != nil { - message.WarnErrf(err, "Error finding why resources for chart %s: %s", chart.Name, err.Error()) + return nil, fmt.Errorf("could not determine why resource for the chart %s: %w", chart.Name, err) } whyResources = append(whyResources, whyResourcesChart...) } @@ -250,20 +235,21 @@ func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, // Read the contents of each file contents, err := os.ReadFile(f) if err != nil { - message.WarnErrf(err, "Unable to read the file %s", f) - continue + return nil, fmt.Errorf("could not read the file %s: %w", f, err) } // Break the manifest into separate resources - // TODO: Do not dogsled error - yamls, _ := utils.SplitYAML(contents) + yamls, err := utils.SplitYAML(contents) + if err != nil { + return nil, err + } resources = append(resources, yamls...) // Check if the --why flag is set and if it is process the manifests - if whyImage != "" { - whyResourcesManifest, err := findWhyResources(yamls, whyImage, component.Name, manifest.Name, false) + if p.cfg.FindImagesOpts.Why != "" { + whyResourcesManifest, err := findWhyResources(yamls, p.cfg.FindImagesOpts.Why, component.Name, manifest.Name, false) if err != nil { - message.WarnErrf(err, "Error finding why resources for manifest %s: %s", manifest.Name, err.Error()) + return nil, fmt.Errorf("could not find why resources for manifest %s: %w", manifest.Name, err) } whyResources = append(whyResources, whyResourcesManifest...) } @@ -274,15 +260,17 @@ func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, defer spinner.Stop() for _, resource := range resources { - if matchedImages, maybeImages, err = p.processUnstructuredImages(resource, matchedImages, maybeImages); err != nil { - message.WarnErrf(err, "Problem processing K8s resource %s", resource.GetName()) + if matchedImages, maybeImages, err = processUnstructuredImages(resource, matchedImages, maybeImages); err != nil { + return nil, fmt.Errorf("could not process the Kubernetes resource %s: %w", resource.GetName(), err) } } - if sortedImages := sortImages(matchedImages, nil); len(sortedImages) > 0 { + sortedMatchedImages, sortedExpectedImages := getSortedImages(matchedImages, maybeImages) + + if len(sortedMatchedImages) > 0 { // Log the header comment componentDefinition += fmt.Sprintf("\n - name: %s\n images:\n", component.Name) - for _, image := range sortedImages { + for _, image := range sortedMatchedImages { // Use print because we want this dumped to stdout imagesMap[component.Name] = append(imagesMap[component.Name], image) componentDefinition += fmt.Sprintf(" - %s\n", image) @@ -290,9 +278,9 @@ func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, } // Handle the "maybes" - if sortedImages := sortImages(maybeImages, matchedImages); len(sortedImages) > 0 { + if len(sortedExpectedImages) > 0 { var validImages []string - for _, image := range sortedImages { + for _, image := range sortedExpectedImages { if descriptor, err := crane.Head(image, images.WithGlobalInsecureFlag()...); err != nil { // Test if this is a real image, if not just quiet log to debug, this is normal message.Debugf("Suspected image does not appear to be valid: %#v", err) @@ -325,8 +313,7 @@ func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, spinner.Updatef("Looking up cosign artifacts for discovered images (%d/%d)", idx+1, len(imagesMap[component.Name])) cosignArtifacts, err := utils.GetCosignArtifacts(image) if err != nil { - message.WarnErrf(err, "Problem looking up cosign artifacts for %s: %s", image, err.Error()) - erroredCosignLookups = append(erroredCosignLookups, image) + return nil, fmt.Errorf("could not lookup the cosing artifacts for image %s: %w", image, err) } cosignArtifactList = append(cosignArtifactList, cosignArtifacts...) } @@ -344,80 +331,64 @@ func (p *Packager) findImages(ctx context.Context) (imgMap map[string][]string, } } - if whyImage != "" { + if p.cfg.FindImagesOpts.Why != "" { if len(whyResources) == 0 { - message.Warnf("image %q not found in any charts or manifests", whyImage) + return nil, fmt.Errorf("image %s not found in any charts or manifests", p.cfg.FindImagesOpts.Why) } return nil, nil } fmt.Println(componentDefinition) - if len(erroredCharts) > 0 || len(erroredCosignLookups) > 0 { - errMsg := "" - if len(erroredCharts) > 0 { - errMsg = fmt.Sprintf("the following charts had errors: %s", erroredCharts) - } - if len(erroredCosignLookups) > 0 { - if errMsg != "" { - errMsg += "\n" - } - errMsg += fmt.Sprintf("the following images errored on cosign lookups: %s", erroredCosignLookups) - } - return imagesMap, fmt.Errorf(errMsg) - } - return imagesMap, nil } -func (p *Packager) processUnstructuredImages(resource *unstructured.Unstructured, matchedImages, maybeImages imageMap) (imageMap, imageMap, error) { - var imageSanityCheck = regexp.MustCompile(`(?mi)"image":"([^"]+)"`) - var imageFuzzyCheck = regexp.MustCompile(`(?mi)["|=]([a-z0-9\-.\/:]+:[\w.\-]*[a-z\.\-][\w.\-]*)"`) - var json string - +func processUnstructuredImages(resource *unstructured.Unstructured, matchedImages, maybeImages map[string]bool) (map[string]bool, map[string]bool, error) { contents := resource.UnstructuredContent() - bytes, _ := resource.MarshalJSON() - json = string(bytes) + b, err := resource.MarshalJSON() + if err != nil { + return nil, nil, err + } switch resource.GetKind() { case "Deployment": var deployment v1.Deployment if err := runtime.DefaultUnstructuredConverter.FromUnstructured(contents, &deployment); err != nil { - return matchedImages, maybeImages, fmt.Errorf("could not parse deployment: %w", err) + return nil, nil, fmt.Errorf("could not parse deployment: %w", err) } - matchedImages = buildImageMap(matchedImages, deployment.Spec.Template.Spec) + matchedImages = appendToImageMap(matchedImages, deployment.Spec.Template.Spec) case "DaemonSet": var daemonSet v1.DaemonSet if err := runtime.DefaultUnstructuredConverter.FromUnstructured(contents, &daemonSet); err != nil { - return matchedImages, maybeImages, fmt.Errorf("could not parse daemonset: %w", err) + return nil, nil, fmt.Errorf("could not parse daemonset: %w", err) } - matchedImages = buildImageMap(matchedImages, daemonSet.Spec.Template.Spec) + matchedImages = appendToImageMap(matchedImages, daemonSet.Spec.Template.Spec) case "StatefulSet": var statefulSet v1.StatefulSet if err := runtime.DefaultUnstructuredConverter.FromUnstructured(contents, &statefulSet); err != nil { - return matchedImages, maybeImages, fmt.Errorf("could not parse statefulset: %w", err) + return nil, nil, fmt.Errorf("could not parse statefulset: %w", err) } - matchedImages = buildImageMap(matchedImages, statefulSet.Spec.Template.Spec) + matchedImages = appendToImageMap(matchedImages, statefulSet.Spec.Template.Spec) case "ReplicaSet": var replicaSet v1.ReplicaSet if err := runtime.DefaultUnstructuredConverter.FromUnstructured(contents, &replicaSet); err != nil { - return matchedImages, maybeImages, fmt.Errorf("could not parse replicaset: %w", err) + return nil, nil, fmt.Errorf("could not parse replicaset: %w", err) } - matchedImages = buildImageMap(matchedImages, replicaSet.Spec.Template.Spec) + matchedImages = appendToImageMap(matchedImages, replicaSet.Spec.Template.Spec) case "Job": var job batchv1.Job if err := runtime.DefaultUnstructuredConverter.FromUnstructured(contents, &job); err != nil { - return matchedImages, maybeImages, fmt.Errorf("could not parse job: %w", err) + return nil, nil, fmt.Errorf("could not parse job: %w", err) } - matchedImages = buildImageMap(matchedImages, job.Spec.Template.Spec) + matchedImages = appendToImageMap(matchedImages, job.Spec.Template.Spec) default: // Capture any custom images - matches := imageSanityCheck.FindAllStringSubmatch(json, -1) + matches := imageSanityCheck.FindAllStringSubmatch(string(b), -1) for _, group := range matches { message.Debugf("Found unknown match, Kind: %s, Value: %s", resource.GetKind(), group[1]) matchedImages[group[1]] = true @@ -425,7 +396,7 @@ func (p *Packager) processUnstructuredImages(resource *unstructured.Unstructured } // Capture "maybe images" too for all kinds because they might be in unexpected places.... 👀 - matches := imageFuzzyCheck.FindAllStringSubmatch(json, -1) + matches := imageFuzzyCheck.FindAllStringSubmatch(string(b), -1) for _, group := range matches { message.Debugf("Found possible fuzzy match, Kind: %s, Value: %s", resource.GetKind(), group[1]) maybeImages[group[1]] = true @@ -437,11 +408,11 @@ func (p *Packager) processUnstructuredImages(resource *unstructured.Unstructured func findWhyResources(resources []*unstructured.Unstructured, whyImage, componentName, resourceName string, isChart bool) ([]string, error) { foundWhyResources := []string{} for _, resource := range resources { - bytes, err := yaml.Marshal(resource.Object) + b, err := yaml.Marshal(resource.Object) if err != nil { return nil, err } - yaml := string(bytes) + yaml := string(b) resourceTypeKey := "manifest" if isChart { resourceTypeKey = "chart" @@ -455,29 +426,34 @@ func findWhyResources(resources []*unstructured.Unstructured, whyImage, componen return foundWhyResources, nil } -// BuildImageMap looks for init container, ephemeral and regular container images. -func buildImageMap(images imageMap, pod corev1.PodSpec) imageMap { +func appendToImageMap(imgMap map[string]bool, pod corev1.PodSpec) map[string]bool { for _, container := range pod.InitContainers { - images[container.Image] = true + imgMap[container.Image] = true } for _, container := range pod.Containers { - images[container.Image] = true + imgMap[container.Image] = true } for _, container := range pod.EphemeralContainers { - images[container.Image] = true + imgMap[container.Image] = true } - return images + return imgMap } -// SortImages returns a sorted list of images. -func sortImages(images, compareWith imageMap) []string { - sortedImages := sort.StringSlice{} - for image := range images { - if !compareWith[image] || compareWith == nil { - // Check compareWith, if it exists only add if not in that list. - sortedImages = append(sortedImages, image) +func getSortedImages(matchedImages map[string]bool, maybeImages map[string]bool) ([]string, []string) { + sortedMatchedImages := sort.StringSlice{} + for image := range matchedImages { + sortedMatchedImages = append(sortedMatchedImages, image) + } + sort.Sort(sortedMatchedImages) + + sortedMaybeImages := sort.StringSlice{} + for image := range maybeImages { + if matchedImages[image] { + continue } + sortedMaybeImages = append(sortedMaybeImages, image) } - sort.Sort(sortedImages) - return sortedImages + sort.Sort(sortedMaybeImages) + + return sortedMatchedImages, sortedMaybeImages } diff --git a/src/pkg/packager/prepare_test.go b/src/pkg/packager/prepare_test.go index 8c43e194ac..afeb9aef07 100644 --- a/src/pkg/packager/prepare_test.go +++ b/src/pkg/packager/prepare_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" "github.com/zarf-dev/zarf/src/pkg/lint" "github.com/zarf-dev/zarf/src/test/testutil" @@ -39,4 +40,83 @@ func TestFindImages(t *testing.T) { for k, v := range expectedImages { require.ElementsMatch(t, v, images[k]) } + + cfg = &types.PackagerConfig{ + CreateOpts: types.ZarfCreateOptions{ + BaseDir: "../../../examples/dos-games/", + }, + FindImagesOpts: types.ZarfFindImagesOptions{ + Why: "foobar", + }, + } + p, err = New(cfg) + require.NoError(t, err) + _, err = p.FindImages(ctx) + require.EqualError(t, err, "image foobar not found in any charts or manifests") +} + +func TestBuildImageMap(t *testing.T) { + t.Parallel() + + podSpec := corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Image: "init-image", + }, + { + Image: "duplicate-image", + }, + }, + Containers: []corev1.Container{ + + { + Image: "container-image", + }, + { + Image: "alpine:latest", + }, + }, + EphemeralContainers: []corev1.EphemeralContainer{ + { + EphemeralContainerCommon: corev1.EphemeralContainerCommon{ + Image: "ephemeral-image", + }, + }, + { + EphemeralContainerCommon: corev1.EphemeralContainerCommon{ + Image: "duplicate-image", + }, + }, + }, + } + imgMap := appendToImageMap(map[string]bool{}, podSpec) + expectedImgMap := map[string]bool{ + "init-image": true, + "duplicate-image": true, + "container-image": true, + "alpine:latest": true, + "ephemeral-image": true, + } + require.Equal(t, expectedImgMap, imgMap) +} + +func TestGetSortedImages(t *testing.T) { + t.Parallel() + + matchedImages := map[string]bool{ + "C": true, + "A": true, + "E": true, + "D": true, + } + maybeImages := map[string]bool{ + "Z": true, + "A": true, + "B": true, + } + sortedMatchedImages, sortedMaybeImages := getSortedImages(matchedImages, maybeImages) + expectedSortedMatchedImages := []string{"A", "C", "D", "E"} + require.Equal(t, expectedSortedMatchedImages, sortedMatchedImages) + expectedSortedMaybeImages := []string{"B", "Z"} + require.Equal(t, expectedSortedMaybeImages, sortedMaybeImages) } diff --git a/src/test/e2e/13_find_images_test.go b/src/test/e2e/13_find_images_test.go index 39f6691b16..6d01bdd569 100644 --- a/src/test/e2e/13_find_images_test.go +++ b/src/test/e2e/13_find_images_test.go @@ -42,8 +42,7 @@ func TestFindImages(t *testing.T) { // Test `zarf prepare find-images` with `--kube-version` specified and less than than the declared minimum (v1.21.0) stdOut, stdErr, err = e2e.Zarf(t, "prepare", "find-images", "--kube-version=v1.20.0", "src/test/packages/00-kube-version-override") require.Error(t, err, stdOut, stdErr) - require.Contains(t, stdErr, "Problem rendering the helm template for cert-manager", "The kubeVersion declaration should prevent this from templating") - require.Contains(t, stdErr, "following charts had errors: [cert-manager]", "Zarf should print an ending error message") + require.Contains(t, stdErr, "could not render the Helm template for chart cert-manager", "The kubeVersion declaration should prevent this from templating") }) t.Run("zarf dev find-images with helm or manifest vars", func(t *testing.T) {