From 2f054ab55fb605cc0021f304b3aa3bf1ec44ec85 Mon Sep 17 00:00:00 2001 From: OpenShift Cherrypick Robot Date: Wed, 13 Apr 2022 07:16:03 -0700 Subject: [PATCH] Fix related image environment variable discovery (#5656) The related image discovery feature currently only reads environment variables from the manager container. This was causing an error when the deployment labels or container name that was expected were not present. This fixes that by collecting related images from all containers across all deployments. This change also enables users to use related images in other containers since related images from everywhere will be considered. Signed-off-by: Ryan King Co-authored-by: Ryan King --- changelog/fragments/fix-make-bundle.yaml | 11 + .../operator-sdk/generate/bundle/bundle.go | 52 +---- .../generate/internal/relatedimages.go | 151 ++++++++++++++ .../generate/internal/relatedimages_test.go | 194 ++++++++++++++++++ .../clusterserviceversion.go | 15 +- 5 files changed, 359 insertions(+), 64 deletions(-) create mode 100644 changelog/fragments/fix-make-bundle.yaml create mode 100644 internal/cmd/operator-sdk/generate/internal/relatedimages.go create mode 100644 internal/cmd/operator-sdk/generate/internal/relatedimages_test.go diff --git a/changelog/fragments/fix-make-bundle.yaml b/changelog/fragments/fix-make-bundle.yaml new file mode 100644 index 00000000000..2e4bb462e4a --- /dev/null +++ b/changelog/fragments/fix-make-bundle.yaml @@ -0,0 +1,11 @@ +entries: + - description: > + The related image discovery feature currently only reads environment + variables from the manager container. This was causing an error when the + deployment labels or container name that was expected were not present. + This fixes that by collecting related images from all containers across + all deployments. + + This change also enables users to use related images in other containers + since related images from everywhere will be considered. + kind: "bugfix" diff --git a/internal/cmd/operator-sdk/generate/bundle/bundle.go b/internal/cmd/operator-sdk/generate/bundle/bundle.go index fd683b9ba3e..ad15a75afe7 100644 --- a/internal/cmd/operator-sdk/generate/bundle/bundle.go +++ b/internal/cmd/operator-sdk/generate/bundle/bundle.go @@ -20,14 +20,12 @@ import ( "io/ioutil" "os" "path/filepath" - "strings" "github.com/operator-framework/api/pkg/apis/scorecard/v1alpha3" "github.com/operator-framework/operator-manifest-tools/pkg/image" "github.com/operator-framework/operator-manifest-tools/pkg/imageresolver" "github.com/operator-framework/operator-manifest-tools/pkg/pullspec" "github.com/operator-framework/operator-registry/pkg/lib/bundle" - corev1 "k8s.io/api/core/v1" "sigs.k8s.io/yaml" metricsannotations "github.com/operator-framework/operator-sdk/internal/annotations/metrics" @@ -193,7 +191,7 @@ func (c bundleCmd) runManifests() (err error) { c.println("Building a ClusterServiceVersion without an existing base") } - relatedImages, err := c.findRelatedImages(col) + relatedImages, err := genutil.FindRelatedImages(col) if err != nil { return err } @@ -305,54 +303,6 @@ func (c bundleCmd) runMetadata() error { return bundleMetadata.GenerateMetadata() } -// findRelatedImages looks in the controller manager's environment for images used by the operator. -func (c bundleCmd) findRelatedImages(col *collector.Manifests) (map[string]string, error) { - const relatedImagePrefix = "RELATED_IMAGE_" - env, err := c.findManagerEnvironment(col) - if err != nil { - return nil, err - } - imageNames := make(map[string]string, len(env)) - for _, envVar := range env { - if strings.HasPrefix(envVar.Name, relatedImagePrefix) { - if envVar.ValueFrom != nil { - return nil, fmt.Errorf("related images with valueFrom field unsupported, found in %s`", envVar.Name) - } - - // transforms RELATED_IMAGE_This_IS_a_cool_image to this-is-a-cool-image - name := strings.ToLower(strings.Replace(strings.TrimPrefix(envVar.Name, relatedImagePrefix), "_", "-", -1)) - imageNames[name] = envVar.Value - } - } - - return imageNames, nil -} - -// findManagerEnvironment returns the environment passed to the controller manager container. -func (c bundleCmd) findManagerEnvironment(col *collector.Manifests) ([]corev1.EnvVar, error) { - const ( - managerLabel = "control-plane" - managerLabelValue = "controller-manager" - managerContainerName = "manager" - ) - - for _, deployment := range col.Deployments { - if val, ok := deployment.GetLabels()[managerLabel]; ok && val == managerLabelValue { - for _, container := range deployment.Spec.Template.Spec.Containers { - if container.Name == managerContainerName { - return container.Env, nil - } - } - - return nil, fmt.Errorf("manager deployment does not have container named %q", managerContainerName) - } - } - - return nil, fmt.Errorf( - "could not find manager deployment, should have label %s=%s", managerLabel, managerLabelValue, - ) -} - // pinImages is used to replace all image tags in the given manifests with digests func (c bundleCmd) pinImages(manifestPath string) error { manifests, err := pullspec.FromDirectory(manifestPath, nil) diff --git a/internal/cmd/operator-sdk/generate/internal/relatedimages.go b/internal/cmd/operator-sdk/generate/internal/relatedimages.go new file mode 100644 index 00000000000..81816115250 --- /dev/null +++ b/internal/cmd/operator-sdk/generate/internal/relatedimages.go @@ -0,0 +1,151 @@ +// Copyright 2020 The Operator-SDK Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package genutil + +import ( + "fmt" + "strings" + + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-sdk/internal/generate/collector" + log "github.com/sirupsen/logrus" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" +) + +// FindRelatedImages looks in the controller manager's environment for images used by the operator. +func FindRelatedImages(manifestCol *collector.Manifests) ([]operatorsv1alpha1.RelatedImage, error) { + col := relatedImageCollector{ + relatedImages: []*relatedImage{}, + relatedImagesByName: make(map[string][]*relatedImage), + relatedImagesByImageRef: make(map[string][]*relatedImage), + seenRelatedImages: sets.NewString(), + } + + for _, deployment := range manifestCol.Deployments { + containers := append(deployment.Spec.Template.Spec.Containers, deployment.Spec.Template.Spec.InitContainers...) + for _, container := range containers { + // containerRef can just be the deployment if there's only one container + // otherwise we need {{ deployment.Name }}-{{ container.Name }} + containerRef := deployment.Name + if len(containers) > 1 { + containerRef += "-" + container.Name + } + + if err := col.collectFromEnvironment(containerRef, container.Env); err != nil { + return nil, err + } + } + } + + return col.collectedRelatedImages(), nil +} + +const relatedImagePrefix = "RELATED_IMAGE_" + +type relatedImage struct { + name string + imageRef string + containerRef string // If 1 container then {{deployment}} else {{deployment}}-{{container}} +} + +type relatedImageCollector struct { + relatedImages []*relatedImage + relatedImagesByName map[string][]*relatedImage + relatedImagesByImageRef map[string][]*relatedImage + seenRelatedImages sets.String +} + +func (c *relatedImageCollector) collectFromEnvironment(containerRef string, env []corev1.EnvVar) error { + for _, envVar := range env { + if strings.HasPrefix(envVar.Name, relatedImagePrefix) { + if envVar.ValueFrom != nil { + return fmt.Errorf("related images with valueFrom field unsupported, found in %s`", envVar.Name) + } + + name := c.formatName(envVar.Name) + c.collect(name, envVar.Value, containerRef) + } + } + + return nil +} + +func (c *relatedImageCollector) collect(name, imageRef, containerRef string) { + // Don't add exact duplicates (same name and image) + key := name + "-" + imageRef + if c.seenRelatedImages.Has(key) { + return + } + c.seenRelatedImages.Insert(key) + + relatedImg := relatedImage{ + name: name, + imageRef: imageRef, + containerRef: containerRef, + } + + c.relatedImages = append(c.relatedImages, &relatedImg) + if relatedImages, ok := c.relatedImagesByName[name]; ok { + c.relatedImagesByName[name] = append(relatedImages, &relatedImg) + } else { + c.relatedImagesByName[name] = []*relatedImage{&relatedImg} + } + + if relatedImages, ok := c.relatedImagesByImageRef[imageRef]; ok { + c.relatedImagesByImageRef[imageRef] = append(relatedImages, &relatedImg) + } else { + c.relatedImagesByImageRef[imageRef] = []*relatedImage{&relatedImg} + } +} + +func (c *relatedImageCollector) collectedRelatedImages() []operatorsv1alpha1.RelatedImage { + final := make([]operatorsv1alpha1.RelatedImage, 0, len(c.relatedImages)) + + for _, relatedImage := range c.relatedImages { + name := relatedImage.name + + // Prefix the name with the containerRef on name collisions. + if len(c.relatedImagesByName[relatedImage.name]) > 1 { + name = relatedImage.containerRef + "-" + name + } + + // Only add the related image to the final list if it's the first occurrence of an image. + // Blank out the name since the image is used multiple times and warn the user. + // Multiple containers using she same related image should use the same exact name. + if relatedImages := c.relatedImagesByImageRef[relatedImage.imageRef]; len(relatedImages) > 1 { + if relatedImages[0].name != relatedImage.name { + continue + } + + name = "" + log.Warnf( + "warning: multiple related images with the same image ref, %q, and different names found."+ + "The image will only be listed once with an empty name."+ + "It is recmmended to either remove the duplicate or use the exact same name.", + relatedImage.name, + ) + } + + final = append(final, operatorsv1alpha1.RelatedImage{Name: name, Image: relatedImage.imageRef}) + } + + return final +} + +// formatName transforms RELATED_IMAGE_This_IS_a_cool_image to this-is-a-cool-image +func (c *relatedImageCollector) formatName(name string) string { + return strings.ToLower(strings.Replace(strings.TrimPrefix(name, relatedImagePrefix), "_", "-", -1)) +} diff --git a/internal/cmd/operator-sdk/generate/internal/relatedimages_test.go b/internal/cmd/operator-sdk/generate/internal/relatedimages_test.go new file mode 100644 index 00000000000..724cb206fdc --- /dev/null +++ b/internal/cmd/operator-sdk/generate/internal/relatedimages_test.go @@ -0,0 +1,194 @@ +// Copyright 2022 The Operator-SDK Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package genutil_test + +import ( + "fmt" + "io" + "os" + "strings" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" + . "github.com/onsi/gomega" + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + genutil "github.com/operator-framework/operator-sdk/internal/cmd/operator-sdk/generate/internal" + "github.com/operator-framework/operator-sdk/internal/generate/collector" + log "github.com/sirupsen/logrus" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" +) + +var _ = Describe("FindRelatedImages", func() { + var images = struct { + memcached string + memcachedLatest string + nginx string + }{"memcached:1.4.36-alpine", "memcached:alpine", "nginx:1.21.6"} + + BeforeSuite(func() { + log.SetOutput(io.Discard) + }) + + AfterSuite(func() { + log.SetOutput(os.Stdout) + }) + + DescribeTable("Valid related image definitions", + func(deployments []appsv1.Deployment, expected []operatorsv1alpha1.RelatedImage) { + col := collector.Manifests{Deployments: deployments} + relatedImages, err := genutil.FindRelatedImages(&col) + Expect(err).ToNot(HaveOccurred()) + Expect(relatedImages).To(Equal(expected)) + }, + Entry("One related image", []appsv1.Deployment{ + deployment("controller-manager", container("manager", relatedImageEnvVar("MEMCACHED", images.memcached))), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("memcached", images.memcached), + }), + Entry("Two related images", []appsv1.Deployment{ + deployment("controller-manager", container("manager", + relatedImageEnvVar("MEMCACHED", images.memcached), + relatedImageEnvVar("NGINX", images.nginx), + )), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("memcached", images.memcached), + relatedImage("nginx", images.nginx), + }), + Entry("No related images", []appsv1.Deployment{ + deployment("controller-manager", container("manager")), + }, []operatorsv1alpha1.RelatedImage{}), + Entry("Two related image across different containers", []appsv1.Deployment{ + deployment("controller-manager", + container("manager", relatedImageEnvVar("MEMCACHED", images.memcached)), + container("manager-proxy", relatedImageEnvVar("NGINX", images.nginx)), + ), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("memcached", images.memcached), + relatedImage("nginx", images.nginx), + }), + Entry("Two related image across different deployments", []appsv1.Deployment{ + deployment("controller-manager", container("manager", relatedImageEnvVar("MEMCACHED", images.memcached))), + deployment("controller-manager-proxy", container("proxy", relatedImageEnvVar("NGINX", images.nginx))), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("memcached", images.memcached), + relatedImage("nginx", images.nginx), + }), + Entry("Two related images with the same name and image", []appsv1.Deployment{ + deployment("controller-manager", + container("manager", relatedImageEnvVar("MEMCACHED", images.memcached)), + container("manager-canary", relatedImageEnvVar("MEMCACHED", images.memcached)), + ), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("memcached", images.memcached), + }), + Entry("Two related images with the same name and different images", []appsv1.Deployment{ + deployment("controller-manager", + container("manager", relatedImageEnvVar("MEMCACHED", images.memcached)), + container("manager-canary", relatedImageEnvVar("MEMCACHED", images.memcachedLatest)), + ), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("controller-manager-manager-memcached", images.memcached), + relatedImage("controller-manager-manager-canary-memcached", images.memcachedLatest), + }), + Entry("Two related images with the same name and different images in separate deployments", []appsv1.Deployment{ + deployment("controller-manager", container("manager", relatedImageEnvVar("MEMCACHED", images.memcached))), + deployment("controller-manager-canary", + container("manager", relatedImageEnvVar("MEMCACHED", images.memcachedLatest)), + ), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("controller-manager-memcached", images.memcached), + relatedImage("controller-manager-canary-memcached", images.memcachedLatest), + }), + Entry("Two related images with different names and the same image", []appsv1.Deployment{ + deployment("controller-manager", + container("manager", relatedImageEnvVar("MEMCACHED", images.memcached)), + container("manager-canary", relatedImageEnvVar("MEMCACHED_CANARY", images.memcached)), + ), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("", images.memcached), + }), + Entry("Three related images with both a name and image overlap", []appsv1.Deployment{ + deployment("controller-manager", + container("manager", relatedImageEnvVar("MEMCACHED", images.memcached)), + container("manager-replica", relatedImageEnvVar("MEMCACHED", images.memcachedLatest)), + container("manager-canary", relatedImageEnvVar("MEMCACHED_CANARY", images.memcached)), + ), + }, []operatorsv1alpha1.RelatedImage{ + relatedImage("", images.memcached), + relatedImage("controller-manager-manager-replica-memcached", images.memcachedLatest), + }), + ) + + Context("There is an invald environment variable", func() { + var ( + relatedImages []operatorsv1alpha1.RelatedImage + err error + ) + + BeforeEach(func() { + d := deployment("controller-manager", + container("manager", relatedImageEnvVar("MEMCACHED", images.memcached)), + ) + d.Spec.Template.Spec.Containers[0].Env[0].Value = "" + d.Spec.Template.Spec.Containers[0].Env[0].ValueFrom = &corev1.EnvVarSource{} + col := collector.Manifests{Deployments: []appsv1.Deployment{d}} + relatedImages, err = genutil.FindRelatedImages(&col) + }) + + It("should return an error", func() { + Expect(err).To(HaveOccurred()) + }) + + It("should not return any related images", func() { + Expect(relatedImages).To(BeNil()) + }) + + It("should tell you which environment variable was invalid", func() { + Expect(err.Error()).To(ContainSubstring("RELATED_IMAGE_MEMCACHED")) + }) + }) +}) + +func relatedImage(name, image string) operatorsv1alpha1.RelatedImage { + return operatorsv1alpha1.RelatedImage{Name: name, Image: image} +} + +func relatedImageEnvVar(name, image string) string { + return fmt.Sprintf("RELATED_IMAGE_%s=%s", name, image) +} + +func deployment(name string, containers ...corev1.Container) appsv1.Deployment { + var d appsv1.Deployment + d.Name = name + d.Spec.Template.Spec.Containers = containers + return d +} + +func container(name string, envVars ...string) corev1.Container { + var c corev1.Container + c.Name = name + c.Env = make([]corev1.EnvVar, len(envVars)) + for i, envVar := range envVars { + envVarParts := strings.Split(envVar, "=") + if len(envVarParts) != 2 { + panic("invalid environment variable: " + envVar + "\nShould be in the form 'name=value'") + } + + c.Env[i] = corev1.EnvVar{Name: envVarParts[0], Value: envVarParts[1]} + } + + return c +} diff --git a/internal/generate/clusterserviceversion/clusterserviceversion.go b/internal/generate/clusterserviceversion/clusterserviceversion.go index bfd7db9a1a8..6e66aaa0f8c 100644 --- a/internal/generate/clusterserviceversion/clusterserviceversion.go +++ b/internal/generate/clusterserviceversion/clusterserviceversion.go @@ -18,7 +18,6 @@ import ( "fmt" "io" "path/filepath" - "sort" "strings" "github.com/blang/semver/v4" @@ -58,8 +57,7 @@ type Generator struct { // {Cluster}Roles to include in a CSV via their Bindings. ExtraServiceAccounts []string // RelatedImages are additional images used by the operator. - // It is a mapping of an image name to an image URL - RelatedImages map[string]string + RelatedImages []operatorsv1alpha1.RelatedImage // Func that returns the writer the generated CSV's bytes are written to. getWriter func() (io.Writer, error) @@ -169,16 +167,7 @@ func (g *Generator) generate() (base *operatorsv1alpha1.ClusterServiceVersion, e if g.FromVersion != "" { base.Spec.Replaces = genutil.MakeCSVName(g.OperatorName, g.FromVersion) } - if len(g.RelatedImages) > 0 { - base.Spec.RelatedImages = make([]operatorsv1alpha1.RelatedImage, 0, len(g.RelatedImages)) - for name, image := range g.RelatedImages { - base.Spec.RelatedImages = append(base.Spec.RelatedImages, operatorsv1alpha1.RelatedImage{Name: name, Image: image}) - } - // ensure deterministic order - sort.SliceStable(base.Spec.RelatedImages, func(i, j int) bool { - return strings.Compare(base.Spec.RelatedImages[i].Name, base.Spec.RelatedImages[j].Name) > 0 - }) - } + base.Spec.RelatedImages = g.RelatedImages if err := ApplyTo(g.Collector, base, g.ExtraServiceAccounts); err != nil { return nil, err