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..6535b10d149 --- /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 its 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..6dcfdccbf48 --- /dev/null +++ b/internal/cmd/operator-sdk/generate/internal/relatedimages_test.go @@ -0,0 +1,180 @@ +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