internal/pkg/scaffold/olm-catalog: preserve certain fields, better CR/CRD updates (#1689)

* internal/pkg/scaffold/olm-catalog: preserve spec.customresourcedefinitions
fields and use unique ID's for each owned CRD

* internal/util/k8sutil: more robust GetTypeMetaFromBytes instead of
GetKindFromYAML

Author: Eric Stroczynski

internal/pkg/scaffold/olm-catalog/csv.go

@@ -168,7 +168,7 @@ func getCSVFromFSIfExists(fs afero.Fs, path string) (*olmapiv1alpha1.ClusterServ
 
 	csv := &olmapiv1alpha1.ClusterServiceVersion{}
 	if err := yaml.Unmarshal(csvBytes, csv); err != nil {
-		return nil, false, errors.Wrap(err, path)
+		return nil, false, errors.Wrapf(err, "error unmarshalling CSV %s", path)
 	}
 
 	return csv, true, nil
@@ -336,28 +336,29 @@ func (s *CSV) updateCSVFromManifestFiles(cfg *CSVConfig, csv *olmapiv1alpha1.Clu
 		scanner := yamlutil.NewYAMLScanner(yamlData)
 		for scanner.Scan() {
 			yamlSpec := scanner.Bytes()
-			kind, err := k8sutil.GetKindfromYAML(yamlSpec)
+			typemeta, err := k8sutil.GetTypeMetaFromBytes(yamlSpec)
 			if err != nil {
-				return errors.Wrap(err, f)
+				return errors.Wrapf(err, "error getting type metadata from manifest %s", f)
 			}
-			found, err := store.AddToUpdater(yamlSpec, kind)
+			found, err := store.AddToUpdater(yamlSpec, typemeta.Kind)
 			if err != nil {
-				return errors.Wrap(err, f)
+				return errors.Wrapf(err, "error adding manifest %s to CSV updaters", f)
 			}
 			if !found {
-				if _, ok := otherSpecs[kind]; !ok {
-					otherSpecs[kind] = make([][]byte, 0)
+				id := gvkID(typemeta.GroupVersionKind())
+				if _, ok := otherSpecs[id]; !ok {
+					otherSpecs[id] = make([][]byte, 0)
 				}
-				otherSpecs[kind] = append(otherSpecs[kind], yamlSpec)
+				otherSpecs[id] = append(otherSpecs[id], yamlSpec)
 			}
 		}
 		if err = scanner.Err(); err != nil {
 			return err
 		}
 	}
 
-	for k := range store.crds.crKinds {
-		if crSpecs, ok := otherSpecs[k]; ok {
+	for id := range store.crds.crIDs {
+		if crSpecs, ok := otherSpecs[id]; ok {
 			for _, spec := range crSpecs {
 				if err := store.AddCR(spec); err != nil {
 					return err

internal/pkg/scaffold/olm-catalog/csv_updaters.go

@@ -17,19 +17,20 @@ package catalog
 import (
 	"bytes"
 	"encoding/json"
-	"fmt"
 	"strings"
 
 	"github.com/operator-framework/operator-sdk/pkg/k8sutil"
 
 	"github.com/ghodss/yaml"
 	olmapiv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	olminstall "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
+	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 // CSVUpdater is an interface for any data that can be in a CSV, which will be
@@ -207,7 +208,7 @@ func (u *InstallStrategyUpdate) Apply(csv *olmapiv1alpha1.ClusterServiceVersion)
 		u.updateClusterPermissions(s)
 		u.updateDeploymentSpecs(s)
 	default:
-		return fmt.Errorf("install strategy (%v) of unknown type", strat)
+		return errors.Errorf("install strategy (%v) of unknown type", strat)
 	}
 
 	// Re-serialize permissions into csv strategy.
@@ -240,40 +241,82 @@ func (u *InstallStrategyUpdate) updateDeploymentSpecs(strat *olminstall.Strategy
 
 type CustomResourceDefinitionsUpdate struct {
 	*olmapiv1alpha1.CustomResourceDefinitions
-	crKinds map[string]struct{}
+	crIDs map[string]struct{}
 }
 
 func (store *updaterStore) AddOwnedCRD(yamlDoc []byte) error {
 	crd := &apiextv1beta1.CustomResourceDefinition{}
 	if err := yaml.Unmarshal(yamlDoc, crd); err != nil {
 		return err
 	}
-	store.crds.Owned = append(store.crds.Owned, olmapiv1alpha1.CRDDescription{
-		Name:    crd.ObjectMeta.Name,
-		Version: crd.Spec.Version,
-		Kind:    crd.Spec.Names.Kind,
-	})
-	store.crds.crKinds[crd.Spec.Names.Kind] = struct{}{}
+	versions, err := getCRDVersions(crd)
+	if err != nil {
+		return errors.Wrapf(err, "failed to get owned CRD %s versions", crd.GetName())
+	}
+	for _, ver := range versions {
+		kind := crd.Spec.Names.Kind
+		crdDesc := olmapiv1alpha1.CRDDescription{
+			Name:    crd.ObjectMeta.Name,
+			Version: ver,
+			Kind:    kind,
+		}
+		store.crds.crIDs[crdDescID(crdDesc)] = struct{}{}
+		store.crds.Owned = append(store.crds.Owned, crdDesc)
+	}
 	return nil
 }
 
+func getCRDVersions(crd *apiextv1beta1.CustomResourceDefinition) (versions []string, err error) {
+	if len(crd.Spec.Versions) != 0 {
+		for _, ver := range crd.Spec.Versions {
+			// Only versions served by the API server are relevant to a CSV.
+			if ver.Served {
+				versions = append(versions, ver.Name)
+			}
+		}
+	} else if crd.Spec.Version != "" {
+		versions = append(versions, crd.Spec.Version)
+	}
+	if len(versions) == 0 {
+		return nil, errors.Errorf("no versions in CRD %s", crd.GetName())
+	}
+	return versions, nil
+}
+
+// crdDescID produces an opaque, unique string identifying a CRDDescription.
+func crdDescID(desc olmapiv1alpha1.CRDDescription) string {
+	// Name should always be <lower kind>.<group>, so this is effectively a GVK.
+	splitName := strings.Split(desc.Name, ".")
+	return getGVKID(strings.Join(splitName[1:], "."), desc.Version, desc.Kind)
+}
+
+// gvkID produces an opaque, unique string identifying a GVK.
+func gvkID(gvk schema.GroupVersionKind) string {
+	return getGVKID(gvk.Group, gvk.Version, gvk.Kind)
+}
+
+func getGVKID(g, v, k string) string {
+	return g + v + k
+}
+
 // Apply updates csv's "owned" CRDDescriptions. "required" CRDDescriptions are
 // left as-is, since they are user-defined values.
+// Apply will only make a new spec.customresourcedefinitions.owned element if
+// the CRD key is not in spec.customresourcedefinitions.owned already.
 func (u *CustomResourceDefinitionsUpdate) Apply(csv *olmapiv1alpha1.ClusterServiceVersion) error {
 	set := make(map[string]olmapiv1alpha1.CRDDescription)
-	for _, csvDesc := range csv.Spec.CustomResourceDefinitions.Owned {
-		set[csvDesc.Name] = csvDesc
+	for _, uDesc := range u.Owned {
+		set[crdDescID(uDesc)] = uDesc
 	}
-	du := u.DeepCopy()
-	for i, uDesc := range u.Owned {
-		if csvDesc, ok := set[uDesc.Name]; ok {
-			csvDesc.Name = uDesc.Name
-			csvDesc.Version = uDesc.Version
-			csvDesc.Kind = uDesc.Kind
-			du.Owned[i] = csvDesc
+	newDescs := []olmapiv1alpha1.CRDDescription{}
+	for _, csvDesc := range csv.Spec.CustomResourceDefinitions.Owned {
+		if uDesc, ok := set[crdDescID(csvDesc)]; !ok {
+			newDescs = append(newDescs, uDesc)
+		} else {
+			newDescs = append(newDescs, csvDesc)
 		}
 	}
-	csv.Spec.CustomResourceDefinitions.Owned = du.Owned
+	csv.Spec.CustomResourceDefinitions.Owned = newDescs
 	return nil
 }
 

internal/util/k8sutil/k8sutil.go

@@ -15,11 +15,16 @@
 package k8sutil
 
 import (
+	"bytes"
 	"fmt"
+	"io"
 	"strings"
 	"unicode"
 
-	"github.com/ghodss/yaml"
+	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/util/yaml"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
@@ -55,16 +60,6 @@ func GetKubeconfigAndNamespace(configPath string) (*rest.Config, string, error)
 	return kubeconfig, namespace, nil
 }
 
-func GetKindfromYAML(yamlData []byte) (string, error) {
-	var temp struct {
-		Kind string
-	}
-	if err := yaml.Unmarshal(yamlData, &temp); err != nil {
-		return "", err
-	}
-	return temp.Kind, nil
-}
-
 // GetDisplayName turns a project dir name in any of {snake, chain, camel}
 // cases, hierarchical dot structure, or space-delimited into a
 // space-delimited, title'd display name.
@@ -99,3 +94,22 @@ func GetDisplayName(name string) string {
 	}
 	return strings.TrimSpace(strings.Title(strings.Join(splitName, " ")))
 }
+
+// GetTypeMetaFromBytes gets the type and object metadata from b. b is assumed
+// to be a single Kubernetes resource manifest.
+func GetTypeMetaFromBytes(b []byte) (t metav1.TypeMeta, err error) {
+	u := unstructured.Unstructured{}
+	r := bytes.NewReader(b)
+	dec := yaml.NewYAMLOrJSONDecoder(r, 8)
+	// There is only one YAML doc if there are no more bytes to be read or EOF
+	// is hit.
+	if err := dec.Decode(&u); err == nil && r.Len() != 0 {
+		return t, errors.New("error getting TypeMeta from bytes: more than one manifest in b")
+	} else if err != nil && err != io.EOF {
+		return t, errors.Wrap(err, "error getting TypeMeta from bytes")
+	}
+	return metav1.TypeMeta{
+		APIVersion: u.GetAPIVersion(),
+		Kind:       u.GetKind(),
+	}, nil
+}