Merge pull request openshift#39 from estroz/bugfix/bundle-validation

validation: bundle CRDs are compared by definition key instead of name (openshift#32)

Author: kevinrizza

pkg/validation/internal/bundle.go

@@ -2,11 +2,13 @@ package internal
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/operator-framework/api/pkg/manifests"
 	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/api/pkg/validation/errors"
 	interfaces "github.com/operator-framework/api/pkg/validation/interfaces"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 var BundleValidator interfaces.Validator = interfaces.ValidatorFunc(validateBundles)
@@ -28,39 +30,62 @@ func validateBundle(bundle *manifests.Bundle) (result errors.ManifestResult) {
 }
 
 func validateOwnedCRDs(bundle *manifests.Bundle, csv *operatorsv1alpha1.ClusterServiceVersion) (result errors.ManifestResult) {
-	ownedCrdNames := getOwnedCustomResourceDefintionNames(csv)
-	crdNames, err := getBundleCRDNames(bundle)
-	if err != (errors.Error{}) {
-		result.Add(err)
-		return result
+	ownedKeys := getOwnedCustomResourceDefintionKeys(csv)
+
+	// Check for duplicate keys in the bundle, which may occur if a v1 and v1beta1 CRD of the same GVK appear.
+	keySet := make(map[schema.GroupVersionKind]struct{})
+	for _, key := range getBundleCRDKeys(bundle) {
+		if _, hasKey := keySet[key]; hasKey {
+			result.Add(errors.ErrInvalidBundle(fmt.Sprintf("duplicate CRD %q in bundle %q", key, bundle.Name), key))
+		}
+		// Always add key to keySet so the below validations run correctly.
+		keySet[key] = struct{}{}
 	}
 
-	// validating names
-	for _, crdName := range ownedCrdNames {
-		if _, ok := crdNames[crdName]; !ok {
-			result.Add(errors.ErrInvalidBundle(fmt.Sprintf("owned CRD %q not found in bundle %q", crdName, bundle.Name), crdName))
+	// All owned keys must match a CRD in bundle.
+	for _, ownedKey := range ownedKeys {
+		if _, ok := keySet[ownedKey]; !ok {
+			result.Add(errors.ErrInvalidBundle(fmt.Sprintf("owned CRD %q not found in bundle %q", ownedKey, bundle.Name), ownedKey))
 		} else {
-			delete(crdNames, crdName)
+			delete(keySet, ownedKey)
 		}
 	}
-	// CRDs not defined in the CSV present in the bundle
-	for crdName := range crdNames {
-		result.Add(errors.WarnInvalidBundle(fmt.Sprintf("owned CRD %q is present in bundle %q but not defined in CSV", crdName, bundle.Name), crdName))
+	// All CRDs present in a CSV must be present in the bundle.
+	for key := range keySet {
+		result.Add(errors.WarnInvalidBundle(fmt.Sprintf("CRD %q is present in bundle %q but not defined in CSV", key, bundle.Name), key))
 	}
+
 	return result
 }
 
-func getOwnedCustomResourceDefintionNames(csv *operatorsv1alpha1.ClusterServiceVersion) (names []string) {
-	for _, ownedCrd := range csv.Spec.CustomResourceDefinitions.Owned {
-		names = append(names, ownedCrd.Name)
+// getBundleCRDKeys returns a list of definition keys for all owned CRDs in csv.
+func getOwnedCustomResourceDefintionKeys(csv *operatorsv1alpha1.ClusterServiceVersion) (keys []schema.GroupVersionKind) {
+	for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
+		group := owned.Name
+		if split := strings.SplitN(group, ".", 2); len(split) == 2 {
+			group = split[1]
+		}
+		keys = append(keys, schema.GroupVersionKind{Group: group, Version: owned.Version, Kind: owned.Kind})
 	}
-	return names
+	return keys
 }
 
-func getBundleCRDNames(bundle *manifests.Bundle) (map[string]struct{}, errors.Error) {
-	crdNames := map[string]struct{}{}
+// getBundleCRDKeys returns a set of definition keys for all CRDs in bundle.
+func getBundleCRDKeys(bundle *manifests.Bundle) (keys []schema.GroupVersionKind) {
+	// Collect all v1 and v1beta1 CRD keys, skipping group which CSVs do not support.
+	for _, crd := range bundle.V1CRDs {
+		for _, version := range crd.Spec.Versions {
+			keys = append(keys, schema.GroupVersionKind{Group: crd.Spec.Group, Version: version.Name, Kind: crd.Spec.Names.Kind})
+		}
+	}
 	for _, crd := range bundle.V1beta1CRDs {
-		crdNames[crd.GetName()] = struct{}{}
+		if len(crd.Spec.Versions) == 0 {
+			keys = append(keys, schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Spec.Names.Kind})
+		} else {
+			for _, version := range crd.Spec.Versions {
+				keys = append(keys, schema.GroupVersionKind{Group: crd.Spec.Group, Version: version.Name, Kind: crd.Spec.Names.Kind})
+			}
+		}
 	}
-	return crdNames, errors.Error{}
+	return keys
 }

pkg/validation/internal/bundle_test.go

@@ -16,21 +16,27 @@ func TestValidateBundle(t *testing.T) {
 		errString   string
 	}{
 		{
-			description: "registryv1 bundle/valid bundle",
+			description: "registryv1 valid bundle",
 			directory:   "./testdata/valid_bundle",
 			hasError:    false,
 		},
 		{
-			description: "registryv1 bundle/valid bundle",
+			description: "registryv1 invalid bundle without CRD etcdclusters v1beta2 in bundle",
 			directory:   "./testdata/invalid_bundle",
 			hasError:    true,
-			errString:   `owned CRD "etcdclusters.etcd.database.coreos.com" not found in bundle`,
+			errString:   `owned CRD "etcd.database.coreos.com/v1beta2, Kind=EtcdCluster" not found in bundle`,
 		},
 		{
-			description: "registryv1 bundle/valid bundle",
+			description: "registryv1 invalid bundle without CRD etcdclusters v1beta2 in CSV",
 			directory:   "./testdata/invalid_bundle_2",
 			hasError:    true,
-			errString:   `owned CRD "etcdclusters.etcd.database.coreos.com" is present in bundle "etcdoperator.v0.9.4" but not defined in CSV`,
+			errString:   `CRD "etcd.database.coreos.com/v1beta2, Kind=EtcdCluster" is present in bundle "etcdoperator.v0.9.4" but not defined in CSV`,
+		},
+		{
+			description: "registryv1 invalid bundle with duplicate CRD etcdclusters in bundle",
+			directory:   "./testdata/invalid_bundle_3",
+			hasError:    true,
+			errString:   `duplicate CRD "test.example.com/v1alpha1, Kind=Test" in bundle "test-operator.v0.0.1"`,
 		},
 	}
 

pkg/validation/internal/testdata/invalid_bundle_3/test-operator.clusterserviceversion.yaml

@@ -0,0 +1,37 @@
+apiVersion: operators.coreos.com/v1alpha1
+kind: ClusterServiceVersion
+metadata:
+  annotations:
+    capabilities: Basic Install
+  name: test-operator.v0.0.1
+  namespace: placeholder
+spec:
+  apiservicedefinitions: {}
+  customresourcedefinitions:
+    owned:
+    - description: Test is the Schema for the tests API
+      kind: Test
+      name: tests.test.example.com
+      version: v1alpha1
+  installModes:
+  - supported: true
+    type: OwnNamespace
+  - supported: true
+    type: SingleNamespace
+  - supported: false
+    type: MultiNamespace
+  - supported: true
+    type: AllNamespaces
+  keywords:
+  - test-operator
+  links:
+  - name: Test Operator
+    url: https://test-operator.domain
+  maintainers:
+  - email: [email protected]
+    name: Maintainer Name
+  maturity: alpha
+  provider:
+    name: Provider Name
+    url: https://your.domain
+  version: 0.0.1

pkg/validation/internal/testdata/invalid_bundle_3/test.example.com_tests_v1_crd.yaml

@@ -0,0 +1,16 @@
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: tests.test.example.com
+spec:
+  group: test.example.com
+  names:
+    kind: Test
+    listKind: TestList
+    plural: tests
+    singular: test
+  scope: Namespaced
+  versions:
+  - name: v1alpha1
+    served: true
+    storage: true

pkg/validation/internal/testdata/invalid_bundle_3/test.example.com_tests_v1beta1_crd.yaml

@@ -0,0 +1,13 @@
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: tests.test.example.com
+spec:
+  group: test.example.com
+  names:
+    kind: Test
+    listKind: TestList
+    plural: tests
+    singular: test
+  scope: Namespaced
+  version: v1alpha1