generate: fix "invalid CSV name" bug (operator-framework#4276)

This commit fixes a bug caused by an invalid CSV name being generated. 
`operator-sdk generate kustomize manifests` will auto-migrate a base
CSV's name to `<package name>.v<version>` if it ends in `.vX.Y.Z`.
`operator-sdk generate <bundle|packagemanifests` will always persist
base CSV metadata unless `--version` or `--from-version` are set.

internal/generate/clusterserviceversion: use an empty base if no
CSVs are in the collector

internal/generate/clusterserviceversion/bases: auto-migrate base name

docs/olm-integration: document updated behavior

Co-authored-by: anmol372 <[email protected]>

changelog/fragments/csv-base-name-bugfix.yaml

@@ -0,0 +1,6 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      `generate kustomize manifests` will (re)generate a base `ClusterServiceVersion` manifest with a valid name.
+    kind: bugfix

internal/cmd/operator-sdk/generate/bundle/bundle.go

@@ -189,11 +189,13 @@ func (c bundleCmd) runManifests() (err error) {
 	// If no CSV is passed via stdin, an on-disk base is expected at basePath.
 	if len(col.ClusterServiceVersions) == 0 {
 		basePath := filepath.Join(c.kustomizeDir, "bases", c.packageName+".clusterserviceversion.yaml")
-		base, err := bases.ClusterServiceVersion{BasePath: basePath}.GetBase()
-		if err != nil {
-			return fmt.Errorf("error reading CSV base: %v", err)
+		if genutil.IsExist(basePath) {
+			base, err := bases.ClusterServiceVersion{BasePath: basePath}.GetBase()
+			if err != nil {
+				return fmt.Errorf("error reading CSV base: %v", err)
+			}
+			col.ClusterServiceVersions = append(col.ClusterServiceVersions, *base)
 		}
-		col.ClusterServiceVersions = append(col.ClusterServiceVersions, *base)
 	}
 
 	var opts []gencsv.Option
@@ -257,11 +259,6 @@ func writeScorecardConfig(dir string, cfg v1alpha3.Configuration) error {
 	return ioutil.WriteFile(scorecardConfigPath, b, 0666)
 }
 
-// validateMetadata validates c for bundle metadata generation.
-func (c bundleCmd) validateMetadata() (err error) {
-	return nil
-}
-
 // runMetadata generates a bundle.Dockerfile and bundle metadata.
 func (c bundleCmd) runMetadata() error {
 
@@ -342,7 +339,7 @@ func updateMetadata(layout, bundleRoot string) error {
 // writeDockerfileCOPYScorecardConfig checks if bundle.Dockerfile and scorecard config exists in
 // the operator project. If it does, it injects the scorecard configuration into bundle image.
 func writeDockerfileCOPYScorecardConfig(dockerfileName, localConfigDir string) error {
-	if isExist(bundle.DockerFile) && isExist(localConfigDir) {
+	if genutil.IsExist(bundle.DockerFile) && genutil.IsExist(localConfigDir) {
 		scorecardFileContent := fmt.Sprintf("COPY %s %s\n", localConfigDir, "/"+scorecard.DefaultConfigDir)
 		return projutil.RewriteFileContents(dockerfileName, "COPY", scorecardFileContent)
 	}
@@ -402,9 +399,3 @@ func rewriteAnnotations(bundleRoot string, kvs map[string]string) error {
 	}
 	return ioutil.WriteFile(annotationsPath, b, mode)
 }
-
-// isExist returns true if path exists.
-func isExist(path string) bool {
-	_, err := os.Stat(path)
-	return err == nil || os.IsExist(err)
-}

internal/cmd/operator-sdk/generate/bundle/cmd.go

@@ -80,11 +80,6 @@ func NewCmd() *cobra.Command {
 					return fmt.Errorf("invalid command options: %v", err)
 				}
 			}
-			if c.metadata {
-				if err := c.validateMetadata(); err != nil {
-					return fmt.Errorf("invalid command options: %v", err)
-				}
-			}
 
 			// Run command logic.
 			if c.manifests {

internal/cmd/operator-sdk/generate/kustomize/manifests.go

@@ -187,16 +187,15 @@ func (c manifestsCmd) run(cfg *config.Config) error {
 		}
 	}
 
+	relBasePath := filepath.Join("bases", c.packageName+".clusterserviceversion.yaml")
+	basePath := filepath.Join(c.inputDir, relBasePath)
 	base := bases.ClusterServiceVersion{
 		OperatorName: c.packageName,
 		OperatorType: projutil.PluginKeyToOperatorType(cfg.Layout),
 		APIsDir:      c.apisDir,
-		Interactive:  isInteractive(c.interactiveLevel),
+		Interactive:  requiresInteraction(basePath, c.interactiveLevel),
 		GVKs:         getGVKs(cfg),
 	}
-	relBasePath := filepath.Join("bases", c.packageName+".clusterserviceversion.yaml")
-	basePath := filepath.Join(c.inputDir, relBasePath)
-
 	// Set BasePath only if it exists. If it doesn't, a new base will be generated
 	// if BasePath is empty.
 	if genutil.IsExist(basePath) {
@@ -231,8 +230,10 @@ func (c manifestsCmd) run(cfg *config.Config) error {
 	return nil
 }
 
-func isInteractive(ilvl projutil.InteractiveLevel) bool {
-	return (ilvl == projutil.InteractiveSoftOff || ilvl == projutil.InteractiveOnAll)
+// requiresInteraction checks if the combination of ilvl and basePath existence
+// requires the generator prompt a user interactively.
+func requiresInteraction(basePath string, ilvl projutil.InteractiveLevel) bool {
+	return (ilvl == projutil.InteractiveSoftOff && genutil.IsNotExist(basePath)) || ilvl == projutil.InteractiveOnAll
 }
 
 func getGVKs(cfg *config.Config) []schema.GroupVersionKind {

internal/cmd/operator-sdk/generate/packagemanifests/packagemanifests.go

@@ -178,11 +178,13 @@ func (c packagemanifestsCmd) run() error {
 	// If no CSV is passed via stdin, an on-disk base is expected at basePath.
 	if len(col.ClusterServiceVersions) == 0 {
 		basePath := filepath.Join(c.kustomizeDir, "bases", c.packageName+".clusterserviceversion.yaml")
-		base, err := bases.ClusterServiceVersion{BasePath: basePath}.GetBase()
-		if err != nil {
-			return fmt.Errorf("error reading CSV base: %v", err)
+		if genutil.IsExist(basePath) {
+			base, err := bases.ClusterServiceVersion{BasePath: basePath}.GetBase()
+			if err != nil {
+				return fmt.Errorf("error reading CSV base: %v", err)
+			}
+			col.ClusterServiceVersions = append(col.ClusterServiceVersions, *base)
 		}
-		col.ClusterServiceVersions = append(col.ClusterServiceVersions, *base)
 	}
 
 	var opts []gencsv.Option

internal/generate/clusterserviceversion/bases/clusterserviceversion.go

@@ -66,9 +66,14 @@ func (b ClusterServiceVersion) GetBase() (base *v1alpha1.ClusterServiceVersion,
 		if base, err = readClusterServiceVersionBase(b.BasePath); err != nil {
 			return nil, fmt.Errorf("error reading existing ClusterServiceVersion base %s: %v", b.BasePath, err)
 		}
+		// Auto-migrate bases with names matching "<operator name>.vX.Y.Z", which
+		// may cause problems with the CSV validator.
+		if base.GetName() == b.OperatorName+".vX.Y.Z" {
+			base.SetName(fmt.Sprintf("%s.v%s", b.OperatorName, base.Spec.Version.Version.String()))
+		}
 	} else {
 		b.setDefaults()
-		base = b.makeNewBase()
+		base = b.newBase()
 	}
 
 	// Interactively fill in UI metadata.
@@ -92,6 +97,13 @@ func (b ClusterServiceVersion) GetBase() (base *v1alpha1.ClusterServiceVersion,
 	return base, nil
 }
 
+// New returns a base with default data with name opertorName.
+func New(operatorName string) *v1alpha1.ClusterServiceVersion {
+	b := ClusterServiceVersion{OperatorName: operatorName}
+	b.setDefaults()
+	return b.newBase()
+}
+
 // setDefaults sets default values in b using b's existing values.
 func (b *ClusterServiceVersion) setDefaults() {
 	if b.DisplayName == "" {
@@ -136,15 +148,15 @@ func (b *ClusterServiceVersion) setDefaults() {
 	}
 }
 
-// makeNewBase returns a base v1alpha1.ClusterServiceVersion to modify.
-func (b ClusterServiceVersion) makeNewBase() *v1alpha1.ClusterServiceVersion {
+// newBase returns a base v1alpha1.ClusterServiceVersion to modify.
+func (b ClusterServiceVersion) newBase() *v1alpha1.ClusterServiceVersion {
 	return &v1alpha1.ClusterServiceVersion{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
 			Kind:       v1alpha1.ClusterServiceVersionKind,
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      b.OperatorName + ".vX.Y.Z",
+			Name:      b.OperatorName + ".v0.0.0",
 			Namespace: "placeholder",
 			Annotations: map[string]string{
 				"capabilities": b.Capabilities,

internal/generate/clusterserviceversion/bases/metadata_test.go

@@ -55,7 +55,7 @@ var _ = Describe("Metadata", func() {
 	It("populates a CSV with existing values", func() {
 		b := ClusterServiceVersion{OperatorName: "test-operator"}
 		b.setDefaults()
-		csv := b.makeNewBase()
+		csv := b.newBase()
 
 		meta.apply(csv)
 

internal/generate/clusterserviceversion/clusterserviceversion.go

@@ -45,7 +45,6 @@ var (
 type Generator struct {
 	// OperatorName is the operator's name, ex. app-operator.
 	OperatorName string
-	OperatorType projutil.OperatorType
 	// Version is the CSV current version.
 	Version string
 	// FromVersion is the version of a previous CSV to upgrade from.
@@ -57,10 +56,6 @@ type Generator struct {
 
 	// Func that returns the writer the generated CSV's bytes are written to.
 	getWriter func() (io.Writer, error)
-	// If the CSV is destined for a bundle this will be the path of the updated
-	// CSV. Used to bring over data from an existing CSV that is not captured
-	// in a base. Not set if a non-file or base writer is returned by getWriter.
-	bundledPath string
 }
 
 // Option is a function that modifies a Generator.
@@ -81,7 +76,6 @@ func WithWriter(w io.Writer) Option {
 func WithBundleWriter(dir string) Option {
 	return func(g *Generator) error {
 		fileName := makeCSVFileName(g.OperatorName)
-		g.bundledPath = filepath.Join(dir, bundle.ManifestsDir, fileName)
 		g.getWriter = func() (io.Writer, error) {
 			return genutil.Open(filepath.Join(dir, bundle.ManifestsDir), fileName)
 		}
@@ -94,9 +88,6 @@ func WithBundleWriter(dir string) Option {
 func WithPackageWriter(dir string) Option {
 	return func(g *Generator) error {
 		fileName := makeCSVFileName(g.OperatorName)
-		if g.FromVersion != "" {
-			g.bundledPath = filepath.Join(dir, g.FromVersion, fileName)
-		}
 		g.getWriter = func() (io.Writer, error) {
 			return genutil.Open(filepath.Join(dir, g.Version), fileName)
 		}
@@ -149,34 +140,28 @@ func (g *Generator) generate() (base *operatorsv1alpha1.ClusterServiceVersion, e
 		return nil, fmt.Errorf("cannot generate CSV without a manifests collection")
 	}
 
-	// Search for a CSV in the collector with a name matching the package name,
-	// but prefer an exact match "<package name>.vX.Y.Z" to preserve existing behavior.
-	var oldBase *operatorsv1alpha1.ClusterServiceVersion
+	// Search for a CSV in the collector with a name matching the package name.
 	csvNamePrefix := g.OperatorName + "."
-	oldBaseCSVName := genutil.MakeCSVName(g.OperatorName, "X.Y.Z")
 	for _, csv := range g.Collector.ClusterServiceVersions {
-		if csv.GetName() == oldBaseCSVName {
-			oldBase = csv.DeepCopy()
-		} else if base == nil && strings.HasPrefix(csv.GetName(), csvNamePrefix) {
+		if base == nil && strings.HasPrefix(csv.GetName(), csvNamePrefix) {
 			base = csv.DeepCopy()
 		}
 	}
 
-	if base == nil && oldBase == nil {
-		return nil, fmt.Errorf("no CSV found with name prefix %q", csvNamePrefix)
-	} else if oldBase != nil {
-		// Only update versions in the old way to preserve existing behavior.
-		base = oldBase
-		if err := g.updateVersionsWithReplaces(base); err != nil {
-			return nil, err
-		}
-	} else if g.Version != "" {
+	// Use a default base if none was supplied.
+	if base == nil {
+		base = bases.New(g.OperatorName)
+	}
+	if g.Version != "" {
 		// Use the existing version/name unless g.Version is set.
 		base.SetName(genutil.MakeCSVName(g.OperatorName, g.Version))
 		if base.Spec.Version.Version, err = semver.Parse(g.Version); err != nil {
 			return nil, err
 		}
 	}
+	if g.FromVersion != "" {
+		base.Spec.Replaces = genutil.MakeCSVName(g.OperatorName, g.Version)
+	}
 
 	if err := ApplyTo(g.Collector, base); err != nil {
 		return nil, err
@@ -195,39 +180,3 @@ func makeCSVFileName(name string) string {
 func requiresInteraction(basePath string, ilvl projutil.InteractiveLevel) bool {
 	return (ilvl == projutil.InteractiveSoftOff && genutil.IsNotExist(basePath)) || ilvl == projutil.InteractiveOnAll
 }
-
-// updateVersionsWithReplaces updates csv's version and data involving the version,
-// ex. ObjectMeta.Name, and place the old version in the `replaces` object,
-// if there is an old version to replace.
-func (g Generator) updateVersionsWithReplaces(csv *operatorsv1alpha1.ClusterServiceVersion) (err error) {
-
-	oldVer, newVer := csv.Spec.Version.String(), g.Version
-	newName := genutil.MakeCSVName(g.OperatorName, newVer)
-	oldName := csv.GetName()
-
-	// A bundled CSV may not have a base containing the previous version to use,
-	// so use the current bundled CSV for version information.
-	if genutil.IsExist(g.bundledPath) {
-		existing, err := (bases.ClusterServiceVersion{BasePath: g.bundledPath}).GetBase()
-		if err != nil {
-			return fmt.Errorf("error reading existing ClusterServiceVersion: %v", err)
-		}
-		oldVer = existing.Spec.Version.String()
-		oldName = existing.GetName()
-	}
-
-	// If the new version is empty, either because a CSV is only being updated or
-	// a base was generated, no update is needed.
-	if newVer == "0.0.0" || newVer == "" {
-		return nil
-	}
-
-	// Set replaces by default.
-	if oldVer != "0.0.0" && newVer != oldVer {
-		csv.Spec.Replaces = oldName
-	}
-
-	csv.SetName(newName)
-	csv.Spec.Version.Version, err = semver.Parse(newVer)
-	return err
-}

internal/generate/clusterserviceversion/clusterserviceversion_test.go

@@ -30,6 +30,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	"sigs.k8s.io/yaml"
 
+	"github.com/operator-framework/operator-sdk/internal/generate/clusterserviceversion/bases"
 	"github.com/operator-framework/operator-sdk/internal/generate/collector"
 	genutil "github.com/operator-framework/operator-sdk/internal/generate/internal"
 	"github.com/operator-framework/operator-sdk/internal/util/projutil"
@@ -164,7 +165,22 @@ var _ = Describe("Generating a ClusterServiceVersion", func() {
 		})
 
 		Context("to create a new ClusterServiceVersion", func() {
-			It("should return a valid object", func() {
+			It("should return a default object when no base is supplied", func() {
+				col.ClusterServiceVersions = nil
+				g = Generator{
+					OperatorName: operatorName,
+					Version:      zeroZeroOne,
+					Collector:    col,
+				}
+				csv, err := g.generate()
+				Expect(err).ToNot(HaveOccurred())
+				col.ClusterServiceVersions = []v1alpha1.ClusterServiceVersion{*(bases.New(operatorName))}
+				csvExp, err := g.generate()
+				Expect(err).ToNot(HaveOccurred())
+				Expect(csv).To(Equal(csvExp))
+			})
+
+			It("should return a default object", func() {
 				col.ClusterServiceVersions = []v1alpha1.ClusterServiceVersion{*baseCSV}
 				g = Generator{
 					OperatorName: operatorName,
@@ -253,18 +269,6 @@ var _ = Describe("Generating a ClusterServiceVersion", func() {
 			})
 		})
 	})
-
-	Context("with incorrect configuration", func() {
-		It("should return an error when a base CSV has an invalid name", func() {
-			col.ClusterServiceVersions = []v1alpha1.ClusterServiceVersion{*baseCSV}
-			g = Generator{
-				OperatorName: operatorName,
-				Collector:    col,
-			}
-			_, err := g.generate()
-			Expect(err).To(HaveOccurred())
-		})
-	})
 })
 
 var _ = Describe("Generation requires interaction", func() {

internal/generate/testdata/clusterserviceversions/bases/memcached-operator.clusterserviceversion.yaml

@@ -1,7 +1,7 @@
 apiVersion: operators.coreos.com/v1alpha1
 kind: ClusterServiceVersion
 metadata:
-  name: memcached-operator.vX.Y.Z
+  name: memcached-operator.v0.0.0
   namespace: placeholder
 spec:
   apiservicedefinitions: {}

internal/generate/testdata/clusterserviceversions/bases/with-ui-metadata.clusterserviceversion.yaml

@@ -4,7 +4,7 @@ metadata:
   annotations:
     alm-examples: '[]'
     capabilities: Basic Install
-  name: memcached-operator.vX.Y.Z
+  name: memcached-operator.v0.0.0
   namespace: placeholder
 spec:
   apiservicedefinitions: {}

testdata/ansible/memcached-operator/config/manifests/bases/memcached-operator.clusterserviceversion.yaml

@@ -4,7 +4,7 @@ metadata:
   annotations:
     alm-examples: '[]'
     capabilities: Basic Install
-  name: memcached-operator.vX.Y.Z
+  name: memcached-operator.v0.0.0
   namespace: placeholder
 spec:
   apiservicedefinitions: {}

testdata/go/memcached-operator/config/manifests/bases/memcached-operator.clusterserviceversion.yaml

@@ -4,7 +4,7 @@ metadata:
   annotations:
     alm-examples: '[]'
     capabilities: Basic Install
-  name: memcached-operator.vX.Y.Z
+  name: memcached-operator.v0.0.0
   namespace: placeholder
 spec:
   apiservicedefinitions: {}

testdata/helm/memcached-operator/config/manifests/bases/memcached-operator.clusterserviceversion.yaml

@@ -4,7 +4,7 @@ metadata:
   annotations:
     alm-examples: '[]'
     capabilities: Basic Install
-  name: memcached-operator.vX.Y.Z
+  name: memcached-operator.v0.0.0
   namespace: placeholder
 spec:
   apiservicedefinitions: {}