Update manifest provider to use config unmarshal and remove getWatchNamespace

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

Author: Per G. da Silva

internal/operator-controller/applier/provider.go

@@ -3,20 +3,17 @@ 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"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
 )
@@ -35,9 +32,6 @@ 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()
@@ -74,44 +68,18 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens
 		render.WithCertificateProvider(r.CertificateProvider),
 	}
 
-	watchNamespace, err := r.getWatchNamespace(ext)
-	if err != nil {
-		return nil, fmt.Errorf("invalid bundle configuration: %w", err)
-	}
-
-	if watchNamespace != "" {
-		opts = append(opts, render.WithTargetNamespaces(watchNamespace))
-	}
-
-	return r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...)
-}
-
-// getWatchNamespace determines the watch namespace the ClusterExtension should use based on the
-// configuration in .spec.config.Inline. Only active if SingleOwnNamespace support is enabled.
-func (r *RegistryV1ManifestProvider) getWatchNamespace(ext *ocv1.ClusterExtension) (string, error) {
-	if !r.IsSingleOwnNamespaceEnabled {
-		return "", nil
-	}
-
-	var watchNamespace string
-	if ext.Spec.Config != nil && ext.Spec.Config.Inline != nil {
-		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))
+	if r.IsSingleOwnNamespaceEnabled && ext.Spec.Config != nil && ext.Spec.Config.ConfigType == ocv1.ClusterExtensionConfigTypeInline {
+		bundleConfig, err := bundle.UnmarshallConfig(ext.Spec.Config.Inline.Raw, &rv1, ext.Spec.Namespace)
+		if err != nil {
+			return nil, fmt.Errorf("invalid bundle configuration: %w", err)
 		}
-		watchNamespace = cfg.WatchNamespace
-	} else {
-		return "", nil
-	}
 
-	if errs := validation.IsDNS1123Subdomain(watchNamespace); len(errs) > 0 {
-		return "", fmt.Errorf("invalid watch namespace '%s': namespace must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", watchNamespace)
+		if bundleConfig != nil && bundleConfig.WatchNamespace != "" {
+			opts = append(opts, render.WithTargetNamespaces(bundleConfig.WatchNamespace))
+		}
 	}
 
-	return watchNamespace, nil
+	return r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...)
 }
 
 // RegistryV1HelmChartProvider creates a Helm-Chart from a registry+v1 bundle and its associated ClusterExtension
@@ -160,27 +128,3 @@ 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

@@ -64,6 +64,42 @@ func Test_RegistryV1ManifestProvider_Integration(t *testing.T) {
 		require.Contains(t, err.Error(), "some error")
 	})
 
+	t.Run("surfaces bundle config unmarshall errors", 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
+					},
+				},
+			},
+			// must be true for now as we only unmarshal configuration when this feature is on
+			// once we go GA and remove IsSingleOwnNamespaceEnabled it's ok to just delete this
+			IsSingleOwnNamespaceEnabled: true,
+		}
+
+		// The contents of the bundle are not important for this tesy, only that it be a valid bundle
+		// to avoid errors in the deserialization process
+		bundleFS := bundlefs.Builder().WithPackageName("test").
+			WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()
+
+		ext := &ocv1.ClusterExtension{
+			Spec: ocv1.ClusterExtensionSpec{
+				Namespace: "install-namespace",
+				Config: &ocv1.ClusterExtensionConfig{
+					ConfigType: ocv1.ClusterExtensionConfigTypeInline,
+					Inline: &apiextensionsv1.JSON{
+						Raw: []byte(`{"watchNamespace": "install-namespace"}`),
+					},
+				},
+			},
+		}
+
+		_, err := provider.Get(bundleFS, ext)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "invalid bundle configuration")
+	})
+
 	t.Run("returns rendered manifests", func(t *testing.T) {
 		provider := applier.RegistryV1ManifestProvider{
 			BundleRenderer: registryv1.Renderer,
@@ -188,77 +224,6 @@ 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{
@@ -276,6 +241,54 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
 		require.Equal(t, "unsupported bundle: bundle does not support AllNamespaces install mode", err.Error())
 	})
 
+	t.Run("rejects bundles without AllNamespaces install mode and with SingleNamespace support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) {
+		expectedWatchNamespace := "some-namespace"
+		provider := applier.RegistryV1ManifestProvider{
+			BundleRenderer: render.BundleRenderer{
+				ResourceGenerators: []render.ResourceGenerator{
+					func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
+						t.Log("ensure watch namespace is appropriately configured")
+						require.Equal(t, []string{expectedWatchNamespace}, opts.TargetNamespaces)
+						return nil, nil
+					},
+				},
+			},
+			IsSingleOwnNamespaceEnabled: false,
+		}
+
+		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: []byte(`{"watchNamespace": "` + expectedWatchNamespace + `"}`),
+					},
+				},
+			},
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "unsupported bundle")
+	})
+
+	t.Run("rejects bundles without AllNamespaces install mode and with OwnNamespace support when Single/OwnNamespace install mode support is disabled", func(t *testing.T) {
+		provider := applier.RegistryV1ManifestProvider{
+			IsSingleOwnNamespaceEnabled: false,
+		}
+		bundleFS := bundlefs.Builder().WithPackageName("test").
+			WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace).Build()).Build()
+		_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
+			Spec: ocv1.ClusterExtensionSpec{
+				Namespace: "install-namespace",
+			},
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "unsupported bundle")
+	})
+
 	t.Run("accepts bundles without AllNamespaces install mode and with SingleNamespace support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) {
 		expectedWatchNamespace := "some-namespace"
 		provider := applier.RegistryV1ManifestProvider{