✨ Update .spec.config unmarshalling to accept yaml and json inputs (#2266)

* Improve .spec.config unmarshallig

Signed-off-by: Per Goncalves da Silva <[email protected]>

* Update .spec.config api doc to ellucidate validation

Signed-off-by: Per Goncalves da Silva <[email protected]>

---------

Signed-off-by: Per Goncalves da Silva <[email protected]>
Co-authored-by: Per Goncalves da Silva <[email protected]>

Author: perdasilva

api/v1/clusterextension_types.go

@@ -98,10 +98,13 @@ type ClusterExtensionSpec struct {
 	// +optional
 	Install *ClusterExtensionInstallConfig `json:"install,omitempty"`
 
-	// config contains optional configuration values applied during rendering of the
-	// ClusterExtension's manifests. Values can be specified inline.
+	// config is an optional field used to specify bundle specific configuration
+	// used to configure the bundle. Configuration is bundle specific and a bundle may provide
+	// a configuration schema. When not specified, the default configuration of the resolved bundle will be used.
 	//
-	// config is optional. When not specified, the default configuration of the resolved bundle will be used.
+	// config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide
+	// a configuration schema the final manifests will be derived on a best-effort basis. More information on how
+	// to configure the bundle should be found in its end-user documentation.
 	//
 	// <opcon:experimental>
 	// +optional
@@ -174,6 +177,8 @@ type ClusterExtensionConfig struct {
 	// ClusterExtension.
 	//
 	// inline must be set if configType is 'Inline'.
+	// inline accepts arbitrary JSON/YAML objects.
+	// inline is validation at runtime against the schema provided by the bundle if a schema is provided.
 	//
 	// +kubebuilder:validation:Type=object
 	// +optional

docs/api-reference/olmv1-api-reference.md

@@ -254,7 +254,7 @@ _Appears in:_
 | Field | Description | Default | Validation |
 | --- | --- | --- | --- |
 | `configType` _[ClusterExtensionConfigType](#clusterextensionconfigtype)_ | configType is a required reference to the type of configuration source.<br /><br />Allowed values are "Inline"<br /><br />When this field is set to "Inline", the cluster extension configuration is defined inline within the<br />ClusterExtension resource. |  | Enum: [Inline] <br />Required: \{\} <br /> |
-| `inline` _[JSON](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#json-v1-apiextensions-k8s-io)_ | inline contains JSON or YAML values specified directly in the<br />ClusterExtension.<br /><br />inline must be set if configType is 'Inline'. |  | Type: object <br /> |
+| `inline` _[JSON](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#json-v1-apiextensions-k8s-io)_ | inline contains JSON or YAML values specified directly in the<br />ClusterExtension.<br /><br />inline must be set if configType is 'Inline'.<br />inline accepts arbitrary JSON/YAML objects.<br />inline is validation at runtime against the schema provided by the bundle if a schema is provided. |  | Type: object <br /> |
 
 
 #### ClusterExtensionConfigType
@@ -343,7 +343,7 @@ _Appears in:_
 | `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions<br />with the cluster that are required to manage the extension.<br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />The ServiceAccount must exist in the namespace referenced in the spec.<br />serviceAccount is required. |  | Required: \{\} <br /> |
 | `source` _[SourceConfig](#sourceconfig)_ | source is a required field which selects the installation source of content<br />for this ClusterExtension. Selection is performed by setting the sourceType.<br /><br />Catalog is currently the only implemented sourceType, and setting the<br />sourcetype to "Catalog" requires the catalog field to also be defined.<br /><br />Below is a minimal example of a source definition (in yaml):<br /><br />source:<br />  sourceType: Catalog<br />  catalog:<br />    packageName: example-package |  | Required: \{\} <br /> |
 | `install` _[ClusterExtensionInstallConfig](#clusterextensioninstallconfig)_ | install is an optional field used to configure the installation options<br />for the ClusterExtension such as the pre-flight check configuration. |  |  |
-| `config` _[ClusterExtensionConfig](#clusterextensionconfig)_ | config contains optional configuration values applied during rendering of the<br />ClusterExtension's manifests. Values can be specified inline.<br /><br />config is optional. When not specified, the default configuration of the resolved bundle will be used.<br /><br /><opcon:experimental> |  |  |
+| `config` _[ClusterExtensionConfig](#clusterextensionconfig)_ | config is an optional field used to specify bundle specific configuration<br />used to configure the bundle. Configuration is bundle specific and a bundle may provide<br />a configuration schema. When not specified, the default configuration of the resolved bundle will be used.<br /><br />config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide<br />a configuration schema the final manifests will be derived on a best-effort basis. More information on how<br />to configure the bundle should be found in its end-user documentation.<br /><br /><opcon:experimental> |  |  |
 
 
 #### ClusterExtensionStatus

helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml

@@ -59,10 +59,13 @@ spec:
             properties:
               config:
                 description: |-
-                  config contains optional configuration values applied during rendering of the
-                  ClusterExtension's manifests. Values can be specified inline.
+                  config is an optional field used to specify bundle specific configuration
+                  used to configure the bundle. Configuration is bundle specific and a bundle may provide
+                  a configuration schema. When not specified, the default configuration of the resolved bundle will be used.
 
-                  config is optional. When not specified, the default configuration of the resolved bundle will be used.
+                  config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide
+                  a configuration schema the final manifests will be derived on a best-effort basis. More information on how
+                  to configure the bundle should be found in its end-user documentation.
                 properties:
                   configType:
                     description: |-
@@ -81,6 +84,8 @@ spec:
                       ClusterExtension.
 
                       inline must be set if configType is 'Inline'.
+                      inline accepts arbitrary JSON/YAML objects.
+                      inline is validation at runtime against the schema provided by the bundle if a schema is provided.
                     type: object
                     x-kubernetes-preserve-unknown-fields: true
                 required:

internal/operator-controller/applier/provider.go

@@ -3,13 +3,16 @@ package applier
 import (
 	"crypto/sha256"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/fs"
+	"strings"
 
 	"helm.sh/helm/v3/pkg/chart"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/yaml"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 
@@ -32,6 +35,9 @@ type RegistryV1ManifestProvider struct {
 	IsWebhookSupportEnabled     bool
 	IsSingleOwnNamespaceEnabled bool
 }
+type registryV1Config struct {
+	WatchNamespace string `json:"watchNamespace"`
+}
 
 func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error) {
 	rv1, err := source.FromFS(bundleFS).GetBundle()
@@ -70,7 +76,7 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens
 
 	watchNamespace, err := r.getWatchNamespace(ext)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("invalid bundle configuration: %w", err)
 	}
 
 	if watchNamespace != "" {
@@ -89,11 +95,12 @@ func (r *RegistryV1ManifestProvider) getWatchNamespace(ext *ocv1.ClusterExtensio
 
 	var watchNamespace string
 	if ext.Spec.Config != nil && ext.Spec.Config.Inline != nil {
-		cfg := struct {
-			WatchNamespace string `json:"watchNamespace"`
-		}{}
-		if err := json.Unmarshal(ext.Spec.Config.Inline.Raw, &cfg); err != nil {
-			return "", fmt.Errorf("invalid bundle configuration: %w", err)
+		cfg := &registryV1Config{}
+		// Using k8s.io/yaml package as that is able to handle both json and yaml
+		// In most cases, at this point we should have a valid JSON/YAML object in the byte slice and failures will
+		// be related to object structure (e.g. additional fields).
+		if err := yaml.UnmarshalStrict(ext.Spec.Config.Inline.Raw, cfg); err != nil {
+			return "", fmt.Errorf("error unmarshalling registry+v1 configuration: %w", formatUnmarshallError(err))
 		}
 		watchNamespace = cfg.WatchNamespace
 	} else {
@@ -153,3 +160,27 @@ func (r *RegistryV1HelmChartProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExten
 
 	return chrt, nil
 }
+
+func formatUnmarshallError(err error) error {
+	var unmarshalErr *json.UnmarshalTypeError
+	if errors.As(err, &unmarshalErr) {
+		if unmarshalErr.Field == "" {
+			return errors.New("input is not a valid JSON object")
+		} else {
+			return fmt.Errorf("invalid value type for field %q: expected %q but got %q", unmarshalErr.Field, unmarshalErr.Type.String(), unmarshalErr.Value)
+		}
+	}
+
+	// unwrap error until the core and process it
+	for {
+		unwrapped := errors.Unwrap(err)
+		if unwrapped == nil {
+			// usually the errors present in the form json: <message> or yaml: <message>
+			// we want to extract <message> if we can
+			errMessageComponents := strings.Split(err.Error(), ":")
+			coreErrMessage := strings.TrimSpace(errMessageComponents[len(errMessageComponents)-1])
+			return errors.New(coreErrMessage)
+		}
+		err = unwrapped
+	}
+}

internal/operator-controller/applier/provider_test.go

@@ -188,6 +188,77 @@ func Test_RegistryV1ManifestProvider_WebhookSupport(t *testing.T) {
 	})
 }
 
+func Test_RegistryV1ManifestProvider_ConfigUnmarshalling(t *testing.T) {
+	for _, tc := range []struct {
+		name               string
+		configBytes        []byte
+		expectedErrMessage string
+	}{
+		{
+			name:        "accepts json config",
+			configBytes: []byte(`{"watchNamespace": "some-namespace"}`),
+		},
+		{
+			name:        "accepts yaml config",
+			configBytes: []byte(`watchNamespace: some-namespace`),
+		},
+		{
+			name:               "rejects invalid json",
+			configBytes:        []byte(`{"hello`),
+			expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: found unexpected end of stream`,
+		},
+		{
+			name:               "rejects valid json that isn't of object type",
+			configBytes:        []byte(`true`),
+			expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: input is not a valid JSON object`,
+		},
+		{
+			name:               "rejects additional fields",
+			configBytes:        []byte(`somekey: somevalue`),
+			expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: unknown field "somekey"`,
+		},
+		{
+			name:               "rejects valid json but invalid registry+v1",
+			configBytes:        []byte(`{"watchNamespace": {"hello": "there"}}`),
+			expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: invalid value type for field "watchNamespace": expected "string" but got "object"`,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			provider := applier.RegistryV1ManifestProvider{
+				BundleRenderer: render.BundleRenderer{
+					ResourceGenerators: []render.ResourceGenerator{
+						func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
+							return nil, nil
+						},
+					},
+				},
+				IsSingleOwnNamespaceEnabled: true,
+			}
+
+			bundleFS := bundlefs.Builder().WithPackageName("test").
+				WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()
+
+			_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
+				Spec: ocv1.ClusterExtensionSpec{
+					Namespace: "install-namespace",
+					Config: &ocv1.ClusterExtensionConfig{
+						ConfigType: ocv1.ClusterExtensionConfigTypeInline,
+						Inline: &apiextensionsv1.JSON{
+							Raw: tc.configBytes,
+						},
+					},
+				},
+			})
+			if tc.expectedErrMessage != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expectedErrMessage)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
 func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
 	t.Run("rejects bundles without AllNamespaces install mode when Single/OwnNamespace install mode support is disabled", func(t *testing.T) {
 		provider := applier.RegistryV1ManifestProvider{

manifests/experimental-e2e.yaml

@@ -864,10 +864,13 @@ spec:
             properties:
               config:
                 description: |-
-                  config contains optional configuration values applied during rendering of the
-                  ClusterExtension's manifests. Values can be specified inline.
+                  config is an optional field used to specify bundle specific configuration
+                  used to configure the bundle. Configuration is bundle specific and a bundle may provide
+                  a configuration schema. When not specified, the default configuration of the resolved bundle will be used.
 
-                  config is optional. When not specified, the default configuration of the resolved bundle will be used.
+                  config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide
+                  a configuration schema the final manifests will be derived on a best-effort basis. More information on how
+                  to configure the bundle should be found in its end-user documentation.
                 properties:
                   configType:
                     description: |-
@@ -886,6 +889,8 @@ spec:
                       ClusterExtension.
 
                       inline must be set if configType is 'Inline'.
+                      inline accepts arbitrary JSON/YAML objects.
+                      inline is validation at runtime against the schema provided by the bundle if a schema is provided.
                     type: object
                     x-kubernetes-preserve-unknown-fields: true
                 required:

manifests/experimental.yaml

@@ -829,10 +829,13 @@ spec:
             properties:
               config:
                 description: |-
-                  config contains optional configuration values applied during rendering of the
-                  ClusterExtension's manifests. Values can be specified inline.
+                  config is an optional field used to specify bundle specific configuration
+                  used to configure the bundle. Configuration is bundle specific and a bundle may provide
+                  a configuration schema. When not specified, the default configuration of the resolved bundle will be used.
 
-                  config is optional. When not specified, the default configuration of the resolved bundle will be used.
+                  config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide
+                  a configuration schema the final manifests will be derived on a best-effort basis. More information on how
+                  to configure the bundle should be found in its end-user documentation.
                 properties:
                   configType:
                     description: |-
@@ -851,6 +854,8 @@ spec:
                       ClusterExtension.
 
                       inline must be set if configType is 'Inline'.
+                      inline accepts arbitrary JSON/YAML objects.
+                      inline is validation at runtime against the schema provided by the bundle if a schema is provided.
                     type: object
                     x-kubernetes-preserve-unknown-fields: true
                 required: