Improve the declcfg and model validation

1. Missing `defaultChannel` doesn’t get caught and error out as an
empty channel issue which is incorrect.
2. Missing `package` property will result out of bound error
3. Bundles without channel is allowed which will lead to orphaned bundles
4. Bundle without a bundle image is valid when it shouldn’t be. Bundle
must either have bundle image or bundle data (objects).

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

Author: dinhxuanvu

cmd/opm/root/cmd.go

@@ -25,7 +25,7 @@ func NewCmd() *cobra.Command {
 		},
 	}
 
-	cmd.AddCommand(registry.NewOpmRegistryCmd(), alpha.NewCmd(), serve.NewCmd(), newAddCmd(), validate.NewConfigValidateCmd())
+	cmd.AddCommand(registry.NewOpmRegistryCmd(), alpha.NewCmd(), serve.NewCmd(), newAddCmd(), validate.NewCmd())
 	index.AddCommand(cmd)
 	version.AddCommand(cmd)
 

cmd/opm/validate/validate.go

@@ -10,15 +10,13 @@ import (
 	"github.com/operator-framework/operator-registry/pkg/lib/config"
 )
 
-func NewConfigValidateCmd() *cobra.Command {
+func NewCmd() *cobra.Command {
 	validate := &cobra.Command{
 		Use:   "validate <directory>",
 		Short: "Validate the declarative index config",
 		Long:  "Validate the declarative config JSON file(s) in a given directory",
-		Args: func(cmd *cobra.Command, args []string) error {
-			return cobra.ExactArgs(1)(cmd, args)
-		},
-		RunE: configValidate,
+		Args:  cobra.ExactArgs(1),
+		RunE:  configValidate,
 	}
 
 	validate.Flags().BoolP("debug", "d", false, "enable debug log output")
@@ -31,16 +29,17 @@ func configValidate(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	directory := args[0]
+	if _, err := os.Stat(directory); os.IsNotExist(err) {
+		return err
+	}
+
 	logger := logrus.WithField("cmd", "validate")
 	if debug {
 		logger.Logger.SetLevel(logrus.DebugLevel)
 	}
 
-	if _, err := os.Stat(args[0]); os.IsNotExist(err) {
-		logger.Error(err.Error())
-	}
-
-	err = config.ValidateConfig(args[0])
+	err = config.ValidateConfig(directory)
 	if err != nil {
 		logger.Error(err.Error())
 		return fmt.Errorf("failed to validate config: %s", err)

internal/declcfg/declcfg_to_model.go

@@ -41,10 +41,18 @@ func ConvertToModel(cfg DeclarativeConfig) (model.Model, error) {
 			return nil, fmt.Errorf("parse properties for bundle %q: %v", b.Name, err)
 		}
 
+		if len(props.Packages) == 0 {
+			return nil, fmt.Errorf("missing package property for bundle %q", b.Name)
+		}
+
 		if b.Package != props.Packages[0].PackageName {
 			return nil, fmt.Errorf("package %q does not match %q property %q", b.Package, property.TypePackage, props.Packages[0].PackageName)
 		}
 
+		if len(props.Channels) == 0 {
+			return nil, fmt.Errorf("bundle %q is missing channel information", b.Name)
+		}
+
 		for _, bundleChannel := range props.Channels {
 			pkgChannel, ok := mpkg.Channels[bundleChannel.Name]
 			if !ok {
@@ -75,7 +83,7 @@ func ConvertToModel(cfg DeclarativeConfig) (model.Model, error) {
 
 	for _, mpkg := range mpkgs {
 		defaultChannelName := defaultChannels[mpkg.Name]
-		if mpkg.DefaultChannel == nil {
+		if defaultChannelName != "" && mpkg.DefaultChannel == nil {
 			dch := &model.Channel{
 				Package: mpkg,
 				Name:    defaultChannelName,

internal/declcfg/declcfg_to_model_test.go

@@ -28,11 +28,11 @@ func TestConvertToModel(t *testing.T) {
 			assertion: require.Error,
 			cfg: DeclarativeConfig{
 				Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
-				Bundles:  []Bundle{newTestBundle("bar", "0.1.0")},
+				Bundles:  []Bundle{newTestBundle("bar", "0.1.0", withChannel("alpha", ""))},
 			},
 		},
 		{
-			name:      "Error/FailedModelValidation",
+			name:      "Error/BundleMissingChannel",
 			assertion: require.Error,
 			cfg: DeclarativeConfig{
 				Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
@@ -47,6 +47,38 @@ func TestConvertToModel(t *testing.T) {
 				Bundles:  []Bundle{newTestBundle("foo", "0.1.0", withChannel("alpha", "1"), withChannel("alpha", "2"))},
 			},
 		},
+		{
+			name:      "Error/BundleMissingDefaultChannel",
+			assertion: require.Error,
+			cfg: DeclarativeConfig{
+				Packages: []Package{newTestPackage("foo", "", svgSmallCircle)},
+				Bundles:  []Bundle{newTestBundle("foo", "0.1.0", withChannel("alpha", ""))},
+			},
+		},
+		{
+			name:      "Error/BundleMissingImageAndData",
+			assertion: require.Error,
+			cfg: DeclarativeConfig{
+				Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
+				Bundles:  []Bundle{newTestBundle("foo", "0.1.0", withChannel("alpha", ""), withNoBundleImage(), withNoBundleData())},
+			},
+		},
+		{
+			name:      "NoError/BundleMissingProperties",
+			assertion: require.Error,
+			cfg: DeclarativeConfig{
+				Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
+				Bundles:  []Bundle{newTestBundle("foo", "0.1.0", withChannel("alpha", ""), withNoProperties())},
+			},
+		},
+		{
+			name:      "NoError/BundleWithDataButMissingImage",
+			assertion: require.NoError,
+			cfg: DeclarativeConfig{
+				Packages: []Package{newTestPackage("foo", "alpha", svgSmallCircle)},
+				Bundles:  []Bundle{newTestBundle("foo", "0.1.0", withChannel("alpha", ""), withNoBundleImage())},
+			},
+		},
 		{
 			name:      "Success/ValidModel",
 			assertion: require.NoError,

internal/declcfg/helpers_test.go

@@ -79,6 +79,25 @@ func withSkips(name string) func(*Bundle) {
 	}
 }
 
+func withNoProperties() func(*Bundle) {
+	return func(b *Bundle) {
+		b.Properties = []property.Property{}
+	}
+}
+
+func withNoBundleImage() func(*Bundle) {
+	return func(b *Bundle) {
+		b.Image = ""
+	}
+}
+
+func withNoBundleData() func(*Bundle) {
+	return func(b *Bundle) {
+		b.Objects = []string{}
+		b.CsvJSON = ""
+	}
+}
+
 func newTestBundle(packageName, version string, opts ...bundleOpt) Bundle {
 	csvJson := fmt.Sprintf(`{"kind": "ClusterServiceVersion", "apiVersion": "operators.coreos.com/v1alpha1", "metadata":{"name":%q}}`, testBundleName(packageName, version))
 	b := Bundle{

internal/model/model.go

@@ -54,14 +54,14 @@ func (m *Package) Validate() error {
 		result = multierror.Append(result, fmt.Errorf("invalid icon: %v", err))
 	}
 
-	if len(m.Channels) == 0 {
-		result = multierror.Append(result, fmt.Errorf("package must contain at least one channel"))
-	}
-
 	if m.DefaultChannel == nil {
 		result = multierror.Append(result, fmt.Errorf("default channel must be set"))
 	}
 
+	if len(m.Channels) == 0 {
+		result = multierror.Append(result, fmt.Errorf("package must contain at least one channel"))
+	}
+
 	foundDefault := false
 	for name, ch := range m.Channels {
 		if name != ch.Name {
@@ -260,6 +260,10 @@ func (b *Bundle) Validate() error {
 		result = multierror.Append(result, fmt.Errorf("must be exactly one property with type %q", property.TypePackage))
 	}
 
+	if b.Image == "" && len(b.Objects) == 0 {
+		result = multierror.Append(result, errors.New("bundle image must be set"))
+	}
+
 	return result.ErrorOrNil()
 }
 

internal/model/model_test.go

@@ -362,7 +362,7 @@ func TestValidators(t *testing.T) {
 				Package:  pkg,
 				Channel:  ch,
 				Name:     "anakin.v0.1.0",
-				Image:    "",
+				Image:    "registry.io/image",
 				Replaces: "anakin.v0.0.1",
 				Skips:    []string{"anakin.v0.0.2"},
 				Properties: []property.Property{
@@ -375,6 +375,39 @@ func TestValidators(t *testing.T) {
 			},
 			assertion: require.NoError,
 		},
+		{
+			name: "Bundle/Success/NoBundleImage/HaveBundleData",
+			v: &Bundle{
+				Package: pkg,
+				Channel: ch,
+				Name:    "anakin.v0.1.0",
+				Image:   "",
+				Properties: []property.Property{
+					property.MustBuildPackage("anakin", "0.1.0"),
+					property.MustBuildGVK("skywalker.me", "v1alpha1", "PodRacer"),
+					property.MustBuildChannel("light", "anakin.v0.0.1"),
+					property.MustBuildBundleObjectRef("path/to/data"),
+				},
+				Objects: []string{"testdata"},
+				CsvJSON: "CSVjson",
+			},
+			assertion: require.NoError,
+		},
+		{
+			name: "Bundle/Error/NoBundleImage/NoBundleData",
+			v: &Bundle{
+				Package: pkg,
+				Channel: ch,
+				Name:    "anakin.v0.1.0",
+				Image:   "",
+				Properties: []property.Property{
+					property.MustBuildPackage("anakin", "0.1.0"),
+					property.MustBuildGVK("skywalker.me", "v1alpha1", "PodRacer"),
+					property.MustBuildChannel("light", "anakin.v0.0.1"),
+				},
+			},
+			assertion: require.Error,
+		},
 		{
 			name:      "Bundle/Error/NoName",
 			v:         &Bundle{},

pkg/lib/config/validate.go

@@ -12,12 +12,15 @@ import (
 // Outputs:
 // error: a wrapped error that contains a list of error strings
 func ValidateConfig(directory string) error {
-	// Load config files
+	// Load config files and convert them to declcfg objects
 	cfg, err := declcfg.LoadDir(directory)
 	if err != nil {
 		return err
 	}
-	// Validate the config using model validation
+	// Validate the config using model validation:
+	// This will convert declcfg objects to intermediate model objects that are
+	// also used for serve and add commands. The conversion process will run
+	// validation for the model objects and ensure they are valid.
 	_, err = declcfg.ConvertToModel(*cfg)
 	if err != nil {
 		return err