Address feedbacks from comments/reviews

1. Improve errors handling
2. Flattering some nested conditional loops
3. Refactoring supported types
4. Handle plain type validation

Signed-off-by: Vu Dinh <[email protected]>

Author: dinhxuanvu

.gitignore

@@ -449,6 +449,7 @@ bin
 
 # Ignore sqlite
 *.db
+*.db-journal
 
 # Ignore vscode
 .vscode

cmd/opm/alpha/bundle/validate.go

@@ -62,7 +62,7 @@ func validateFunc(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	err = imageValidator.ValidateBundleFormat(filepath.Join(dir, bundle.ManifestsDir))
+	err = imageValidator.ValidateBundleContent(filepath.Join(dir, bundle.ManifestsDir))
 	if err != nil {
 		return err
 	}

docs/design/operator-bundle.md

@@ -224,7 +224,7 @@ $ ./opm alpha bundle build --tag quay.io/coreos/test-operator.v0.1.0:latest --im
 
 The `validate` command will first extract the contents of the bundle image into a temporary directory after it pulls the image from its image registry. Then, it will validate the format of bundle image to ensure manifests and metadata are located in their appropriate directories (`/manifests/` for bundle manifests files such as CSV and `/metadata/` for metadata files such as `annotations.yaml`). Also, it will validate the information in `annotations.yaml` to confirm that metadata is matching the provided data. For example, the provided media type in annotations.yaml just matches the actual media type is provided in the bundle image.
 
-After the bundle image format is confirmed, the command will validate the bundle contents such as manifests and metadata files only if the bundle format is `RegistryV1` type meaning it contains `ClusterResourceVersion` and its associated Kubernetes objects that makes up an Operator. The content validation process will ensure the individual file in the bundle image is valid and can be applied to an OLM-enabled cluster provided all necessary permissions and configurations are met.
+After the bundle image format is confirmed, the command will validate the bundle contents such as manifests and metadata files if the bundle format is `RegistryV1` or "Plain" type. "RegistryV1" format means it contains `ClusterResourceVersion` and its associated Kubernetes objects while `PlainType` means it contains all Kubernetes objects. The content validation process will ensure the individual file in the bundle image is valid and can be applied to an OLM-enabled cluster provided all necessary permissions and configurations are met.
 
 *Notes:*
 * The bundle content validation is best effort which means it will not guarantee 100% accuracy due to nature of Kubernetes objects may need certain permissions and configurations, which users may not have, in order to be applied successfully in a cluster.

go.mod

@@ -14,7 +14,7 @@ require (
 	github.com/maxbrunsfeld/counterfeiter/v6 v6.2.2
 	github.com/onsi/ginkgo v1.10.1
 	github.com/onsi/gomega v1.7.0
-	github.com/operator-framework/api v0.0.0-20191127212340-9066a6e95573
+	github.com/operator-framework/api v0.0.0-20200120235816-80fd2f1a09c9
 	github.com/operator-framework/operator-lifecycle-manager v0.0.0-20191115003340-16619cd27fa5
 	github.com/otiai10/copy v1.0.1
 	github.com/pkg/errors v0.8.1

go.sum

@@ -516,6 +516,8 @@ github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFSt
 github.com/openzipkin/zipkin-go v0.1.6/go.mod h1:QgAqvLzwWbR/WpD4A3cGpPtJrZXNIiJc5AZX7/PBEpw=
 github.com/operator-framework/api v0.0.0-20191127212340-9066a6e95573 h1:bMO43IWWPM3HCGIiuM/GyXjtSJWsrhOlzUpZMesVUw0=
 github.com/operator-framework/api v0.0.0-20191127212340-9066a6e95573/go.mod h1:S5IdlJvmKkF84K2tBvsrqJbI2FVy03P88R75snpRxJo=
+github.com/operator-framework/api v0.0.0-20200120235816-80fd2f1a09c9 h1:HfxMEPJ0djo/RNfrmli3kI2oKS6IeuIZWu1Q5Rewt/o=
+github.com/operator-framework/api v0.0.0-20200120235816-80fd2f1a09c9/go.mod h1:S5IdlJvmKkF84K2tBvsrqJbI2FVy03P88R75snpRxJo=
 github.com/operator-framework/operator-lifecycle-manager v0.0.0-20191115003340-16619cd27fa5 h1:rjaihxY50c5C+kbQIK4s36R8zxByATYrgRbua4eiG6o=
 github.com/operator-framework/operator-lifecycle-manager v0.0.0-20191115003340-16619cd27fa5/go.mod h1:zL34MNy92LPutBH5gQK+gGhtgTUlZZX03I2G12vWHF4=
 github.com/operator-framework/operator-registry v1.5.1/go.mod h1:agrQlkWOo1q8U1SAaLSS2WQ+Z9vswNT2M2HFib9iuLY=

pkg/lib/bundle/supported_resources.go

@@ -0,0 +1,41 @@
+package bundle
+
+const (
+	CSVKind                = "ClusterServiceVersion"
+	CRDKind                = "CustomResourceDefinition"
+	SecretKind             = "Secret"
+	ClusterRoleKind        = "ClusterRole"
+	ClusterRoleBindingKind = "ClusterRoleBinding"
+	ServiceAccountKind     = "ServiceAccount"
+	ServiceKind            = "Service"
+	RoleKind               = "Role"
+	RoleBindingKind        = "RoleBinding"
+	PrometheusRuleKind     = "PrometheusRule"
+	ServiceMonitorKind     = "ServiceMonitor"
+)
+
+var supportedResources map[string]bool
+
+// Add a list of supported resources to the map
+// Key: Kind name
+// Value: If namaspaced kind, true. Otherwise, false
+func init() {
+	supportedResources = make(map[string]bool, 11)
+
+	supportedResources[CSVKind] = true
+	supportedResources[CRDKind] = false
+	supportedResources[ClusterRoleKind] = false
+	supportedResources[ClusterRoleBindingKind] = false
+	supportedResources[ServiceKind] = true
+	supportedResources[ServiceAccountKind] = true
+	supportedResources[RoleKind] = true
+	supportedResources[RoleBindingKind] = true
+	supportedResources[PrometheusRuleKind] = true
+	supportedResources[ServiceMonitorKind] = true
+}
+
+// IsSupported checks if the object kind is OLM-supported and if it is namespaced
+func IsSupported(kind string) (bool, bool) {
+	namespaced, ok := supportedResources[kind]
+	return ok, namespaced
+}

pkg/lib/bundle/testdata/validate/invalid_manifests_bundle/invalid_crd/etcdrestores.etcd.database.coreos.com.crd.yaml

@@ -16,4 +16,4 @@ spec:
     storage: true
   - name: v1beta2
     served: true
-    storage: true
+    storage: false

pkg/lib/bundle/validate.go

@@ -25,18 +25,6 @@ import (
 	"gopkg.in/yaml.v2"
 )
 
-const (
-	csvKind                = "ClusterServiceVersion"
-	crdKind                = "CustomResourceDefinition"
-	secretKind             = "Secret"
-	clusterRoleKind        = "ClusterRole"
-	clusterRoleBindingKind = "ClusterRoleBinding"
-	serviceAccountKind     = "ServiceAccount"
-	serviceKind            = "Service"
-	roleKind               = "Role"
-	roleBindingKind        = "RoleBinding"
-)
-
 type Meta struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata"`
@@ -73,7 +61,11 @@ func (i imageValidator) ValidateBundleFormat(directory string) error {
 	var annotationsDir, manifestsDir string
 	var validationErrors []error
 
-	items, _ := ioutil.ReadDir(directory)
+	items, err := ioutil.ReadDir(directory)
+	if err != nil {
+		validationErrors = append(validationErrors, err)
+	}
+
 	for _, item := range items {
 		if item.IsDir() {
 			switch s := item.Name(); s {
@@ -200,29 +192,21 @@ func (i imageValidator) ValidateBundleContent(manifestDir string) error {
 	}
 
 	switch mediaType {
-	case HelmType, PlainType:
+	case HelmType:
 		return nil
 	}
 
-	supportedTypes := map[string]string{
-		csvKind:                "",
-		crdKind:                "",
-		secretKind:             "",
-		clusterRoleKind:        "",
-		clusterRoleBindingKind: "",
-		serviceKind:            "",
-		serviceAccountKind:     "",
-		roleKind:               "",
-		roleBindingKind:        "",
-	}
-
 	var csvName string
 	unstObjs := []*unstructured.Unstructured{}
 	csvValidator := v.ClusterServiceVersionValidator
 	crdValidator := v.CustomResourceDefinitionValidator
 
 	// Read all files in manifests directory
-	items, _ := ioutil.ReadDir(manifestDir)
+	items, err := ioutil.ReadDir(manifestDir)
+	if err != nil {
+		validationErrors = append(validationErrors, err)
+	}
+
 	for _, item := range items {
 		fileWithPath := filepath.Join(manifestDir, item.Name())
 		data, err := ioutil.ReadFile(fileWithPath)
@@ -233,50 +217,56 @@ func (i imageValidator) ValidateBundleContent(manifestDir string) error {
 
 		dec := k8syaml.NewYAMLOrJSONDecoder(strings.NewReader(string(data)), 30)
 		k8sFile := &unstructured.Unstructured{}
-		if err := dec.Decode(k8sFile); err == nil {
-			unstObjs = append(unstObjs, k8sFile)
-			kind := k8sFile.GetObjectKind().GroupVersionKind().Kind
-			i.logger.Debugf(`Validating file "%s" with type "%s"`, item.Name(), kind)
-			// Verify if the object kind is supported for registryV1 format
-			if _, ok := supportedTypes[kind]; !ok {
-				validationErrors = append(validationErrors, fmt.Errorf("%s is not supported type for registryV1 bundle: %s", kind, fileWithPath))
-			} else {
-				if kind == csvKind {
-					csv := &v1.ClusterServiceVersion{}
-					err := runtime.DefaultUnstructuredConverter.FromUnstructured(k8sFile.Object, csv)
-					if err != nil {
-						validationErrors = append(validationErrors, err)
-					}
-
-					csvName = csv.GetName()
-					results := csvValidator.Validate(csv)
-					if len(results) > 0 {
-						for _, err := range results[0].Errors {
-							validationErrors = append(validationErrors, err)
-						}
-					}
-				} else if kind == crdKind {
-					crd := &v1beta1.CustomResourceDefinition{}
-					err := runtime.DefaultUnstructuredConverter.FromUnstructured(k8sFile.Object, crd)
-					if err != nil {
-						validationErrors = append(validationErrors, err)
-					}
-
-					results := crdValidator.Validate(crd)
-					if len(results) > 0 {
-						for _, err := range results[0].Errors {
-							validationErrors = append(validationErrors, err)
-						}
-					}
-				} else {
-					err := validateKubectlable(data)
-					if err != nil {
-						validationErrors = append(validationErrors, err)
-					}
+		err = dec.Decode(k8sFile)
+		if err != nil {
+			validationErrors = append(validationErrors, err)
+			continue
+		}
+
+		unstObjs = append(unstObjs, k8sFile)
+		gvk := k8sFile.GetObjectKind().GroupVersionKind()
+		i.logger.Debugf(`Validating "%s" from file "%s"`, gvk.String(), item.Name())
+		// Verify if the object kind is supported for RegistryV1 format
+		ok, _ := IsSupported(gvk.Kind)
+		if mediaType == RegistryV1Type && !ok {
+			validationErrors = append(validationErrors, fmt.Errorf("%s is not supported type for registryV1 bundle: %s", gvk.Kind, fileWithPath))
+			continue
+		}
+
+		if gvk.Kind == CSVKind {
+			csv := &v1.ClusterServiceVersion{}
+			err := runtime.DefaultUnstructuredConverter.FromUnstructured(k8sFile.Object, csv)
+			if err != nil {
+				validationErrors = append(validationErrors, err)
+				continue
+			}
+
+			csvName = csv.GetName()
+			results := csvValidator.Validate(csv)
+			if len(results) > 0 {
+				for _, err := range results[0].Errors {
+					validationErrors = append(validationErrors, err)
+				}
+			}
+		} else if gvk.Kind == CRDKind {
+			crd := &v1beta1.CustomResourceDefinition{}
+			err := runtime.DefaultUnstructuredConverter.FromUnstructured(k8sFile.Object, crd)
+			if err != nil {
+				validationErrors = append(validationErrors, err)
+				continue
+			}
+
+			results := crdValidator.Validate(crd)
+			if len(results) > 0 {
+				for _, err := range results[0].Errors {
+					validationErrors = append(validationErrors, err)
 				}
 			}
 		} else {
-			validationErrors = append(validationErrors, err)
+			err := validateKubectlable(data)
+			if err != nil {
+				validationErrors = append(validationErrors, err)
+			}
 		}
 	}
 
@@ -301,13 +291,13 @@ func (i imageValidator) ValidateBundleContent(manifestDir string) error {
 
 // Validate if the file is kubecle-able
 func validateKubectlable(fileBytes []byte) error {
-	exampleFileBytesJson, err := y.YAMLToJSON(fileBytes)
+	exampleFileBytesJSON, err := y.YAMLToJSON(fileBytes)
 	if err != nil {
 		return err
 	}
 
 	parsedMeta := &Meta{}
-	err = json.Unmarshal(exampleFileBytesJson, parsedMeta)
+	err = json.Unmarshal(exampleFileBytesJSON, parsedMeta)
 	if err != nil {
 		return err
 	}
@@ -322,7 +312,7 @@ func validateKubectlable(fileBytes []byte) error {
 	)
 
 	if len(errs) > 0 {
-		return fmt.Errorf("error validating object metadata: %s. %v", errs, parsedMeta)
+		return fmt.Errorf("error validating object metadata: %s. %v", errs.ToAggregate(), parsedMeta)
 	}
 
 	return nil

pkg/lib/bundle/validate_test.go

@@ -107,7 +107,7 @@ func TestValidateBundleContent(t *testing.T) {
 			mediaType:   RegistryV1Type,
 			directory:   "./testdata/validate/invalid_manifests_bundle/invalid_crd/",
 			numErrors:   1,
-			errString:   "cannot restore slice from string",
+			errString:   "must contain unique version name",
 		},
 		{
 			description: "registryv1 bundle/invalid sa",
@@ -135,8 +135,10 @@ func TestValidateBundleContent(t *testing.T) {
 		fmt.Println(tt.directory)
 		err := validator.ValidateBundleContent(tt.directory)
 		var validationError ValidationError
-		isValidationErr := errors.As(err, &validationError)
-		require.True(t, isValidationErr)
+		if err != nil {
+			isValidationErr := errors.As(err, &validationError)
+			require.True(t, isValidationErr)
+		}
 		require.Len(t, validationError.Errors, tt.numErrors, table[i].description)
 		if len(validationError.Errors) > 0 {
 			e := validationError.Errors[0]