Redo config merging a bit and test thereof

Signed-off-by: Brett Tofel <btofel@redhat.com>

Author: bentito

api/v1/clusterextensionconfig_deepcopy.go

@@ -0,0 +1,35 @@
+package v1
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+// DeepCopyInto copies the receiver into out. in must be non-nil.
+func (in *ClusterExtensionConfig) DeepCopyInto(out *ClusterExtensionConfig) {
+	*out = *in
+	if in.Inline != nil {
+		out.Inline = make(map[string]apiextensionsv1.JSON, len(in.Inline))
+		for k, v := range in.Inline {
+			if v.Raw != nil {
+				out.Inline[k] = apiextensionsv1.JSON{Raw: append([]byte(nil), v.Raw...)}
+			} else {
+				out.Inline[k] = apiextensionsv1.JSON{}
+			}
+		}
+	}
+	if in.SecretRef != nil {
+		out.SecretRef = new(corev1.SecretKeySelector)
+		in.SecretRef.DeepCopyInto(out.SecretRef)
+	}
+}
+
+// DeepCopy creates a new ClusterExtensionConfig by copying the receiver.
+func (in *ClusterExtensionConfig) DeepCopy() *ClusterExtensionConfig {
+	if in == nil {
+		return nil
+	}
+	out := new(ClusterExtensionConfig)
+	in.DeepCopyInto(out)
+	return out
+}

internal/operator-controller/applier/helm.go

@@ -20,7 +20,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	apitypes "k8s.io/apimachinery/pkg/types"
 	apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -72,33 +71,6 @@ type Helm struct {
 	Client                     client.Client
 }
 
-func decodeValues(data []byte) (map[string]any, error) {
-	m := map[string]any{}
-	if len(bytes.TrimSpace(data)) == 0 {
-		return m, nil
-	}
-	j, err := apimachyaml.ToJSON(data)
-	if err != nil {
-		return nil, err
-	}
-	if err := json.Unmarshal(j, &m); err != nil {
-		return nil, err
-	}
-	return m, nil
-}
-
-func mergeValueMaps(dst, src map[string]any) {
-	for k, v := range src {
-		if dstMap, ok := dst[k].(map[string]any); ok {
-			if srcMap, ok2 := v.(map[string]any); ok2 {
-				mergeValueMaps(dstMap, srcMap)
-				continue
-			}
-		}
-		dst[k] = v
-	}
-}
-
 // shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND
 // if it is set to enforcement None.
 func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.ClusterExtension, state string) bool {
@@ -153,41 +125,55 @@ func (h *Helm) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterE
 	return nil
 }
 
-func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) {
-	chrt, err := h.buildHelmChart(contentFS, ext)
-	if err != nil {
-		return nil, "", err
+func (h *Helm) configValues(ctx context.Context, ext *ocv1.ClusterExtension) (map[string]interface{}, error) {
+	cfg := map[string]interface{}{}
+	if ext.Spec.Config == nil {
+		return cfg, nil
 	}
-	// gather configuration values from the ClusterExtension specification
-	// and any referenced Secrets, merging them into a single map
-	valuesMap := map[string]any{}
-	if ext.Spec.Config != nil {
-		if ext.Spec.Config.Inline != nil && len(ext.Spec.Config.Inline.Raw) > 0 {
-			inlineMap, err := decodeValues(ext.Spec.Config.Inline.Raw)
-			if err != nil {
-				return nil, "", fmt.Errorf("invalid spec.config.inline: %w", err)
-			}
-			mergeValueMaps(valuesMap, inlineMap)
-		}
-		if ext.Spec.Config.SecretRef != nil {
-			if h.Client == nil {
-				return nil, "", errors.New("client is nil")
-			}
-			secret := &corev1.Secret{}
-			nn := apitypes.NamespacedName{Name: ext.Spec.Config.SecretRef.Name, Namespace: ext.Spec.Namespace}
-			if err := h.Client.Get(ctx, nn, secret); err != nil {
-				return nil, "", fmt.Errorf("failed to get config secret: %w", err)
-			}
-			data, ok := secret.Data[ext.Spec.Config.SecretRef.Key]
-			if !ok {
-				return nil, "", fmt.Errorf("secret %q missing key %q", nn.Name, ext.Spec.Config.SecretRef.Key)
+	if ext.Spec.Config.Inline != nil {
+		for k, v := range ext.Spec.Config.Inline {
+			if len(v.Raw) == 0 {
+				cfg[k] = nil
+				continue
 			}
-			secretMap, err := decodeValues(data)
-			if err != nil {
-				return nil, "", fmt.Errorf("invalid config secret %q key %q: %w", nn.Name, ext.Spec.Config.SecretRef.Key, err)
+			var val interface{}
+			if err := json.Unmarshal(v.Raw, &val); err != nil {
+				return nil, fmt.Errorf("invalid JSON in spec.config.inline[%s]: %w", k, err)
 			}
-			mergeValueMaps(valuesMap, secretMap)
+			cfg[k] = val
+		}
+	}
+	if ext.Spec.Config.SecretRef != nil {
+		if h.Client == nil {
+			return nil, fmt.Errorf("secretRef specified but Helm client is nil")
+		}
+		secret := &corev1.Secret{}
+		if err := h.Client.Get(ctx, client.ObjectKey{Name: ext.Spec.Config.SecretRef.Name, Namespace: ext.Spec.Namespace}, secret); err != nil {
+			return nil, fmt.Errorf("failed to get config secret: %w", err)
 		}
+		data, ok := secret.Data[ext.Spec.Config.SecretRef.Key]
+		if !ok {
+			return nil, fmt.Errorf("missing key %s in secret %s", ext.Spec.Config.SecretRef.Key, ext.Spec.Config.SecretRef.Name)
+		}
+		var secretCfg map[string]interface{}
+		if err := json.Unmarshal(data, &secretCfg); err != nil {
+			return nil, fmt.Errorf("invalid JSON in secret %s/%s: %w", ext.Spec.Namespace, ext.Spec.Config.SecretRef.Name, err)
+		}
+		for k, v := range secretCfg {
+			cfg[k] = v
+		}
+	}
+	return cfg, nil
+}
+
+func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) {
+	chrt, err := h.buildHelmChart(ctx, contentFS, ext)
+	if err != nil {
+		return nil, "", err
+	}
+	valuesMap, err := h.configValues(ctx, ext)
+	if err != nil {
+		return nil, "", err
 	}
 	values := chartutil.Values{"Values": valuesMap}
 
@@ -266,7 +252,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 	return relObjects, state, nil
 }
 
-func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
+func (h *Helm) buildHelmChart(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension) (*chart.Chart, error) {
 	if h.BundleToHelmChartConverter == nil {
 		return nil, errors.New("BundleToHelmChartConverter is nil")
 	}
@@ -285,14 +271,9 @@ func (h *Helm) buildHelmChart(bundleFS fs.FS, ext *ocv1.ClusterExtension) (*char
 		}
 	}
 
-	// Unmarshal any provided configuration on the ClusterExtension and pass it
-	// to the registry+v1 renderer so that static manifests (e.g. ConfigMaps)
-	// can be overridden with values from spec.config.
-	cfg := map[string]interface{}{}
-	if ext.Spec.Config != nil && ext.Spec.Config.Raw != nil {
-		if err := json.Unmarshal(ext.Spec.Config.Raw, &cfg); err != nil {
-			return nil, fmt.Errorf("invalid JSON in spec.config: %w", err)
-		}
+	cfg, err := h.configValues(ctx, ext)
+	if err != nil {
+		return nil, err
 	}
 
 	return h.BundleToHelmChartConverter.ToHelmChartWithConfig(source.FromFS(bundleFS), ext.Spec.Namespace, watchNamespace, cfg)

internal/operator-controller/applier/helm_test.go

@@ -20,7 +20,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
+	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 
@@ -228,6 +228,49 @@ func TestApply_Base(t *testing.T) {
 	})
 }
 
+func TestApply_ConfigMerging(t *testing.T) {
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "ns"},
+		Data: map[string][]byte{
+			"config": []byte(`{"secretKey":"secretValue","sharedKey":"secretVal"}`),
+		},
+	}
+	cl := fakeclient.NewClientBuilder().WithObjects(secret).Build()
+
+	ext := &ocv1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"},
+		Spec: ocv1.ClusterExtensionSpec{
+			Namespace:      "ns",
+			ServiceAccount: ocv1.ServiceAccountReference{Name: "sa"},
+			Source:         ocv1.SourceConfig{SourceType: ocv1.SourceTypeCatalog, Catalog: &ocv1.CatalogFilter{PackageName: "pkg"}},
+			Config: &ocv1.ClusterExtensionConfig{
+				Inline: map[string]apiextensionsv1.JSON{
+					"inlineKey": {Raw: []byte(`"inlineValue"`)},
+					"sharedKey": {Raw: []byte(`"inlineVal"`)},
+				},
+				SecretRef: &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "cfg"}, Key: "config"},
+			},
+		},
+	}
+
+	mag := &mockActionGetter{desiredRel: &release.Release{Manifest: validManifest}}
+
+	helmApplier := applier.Helm{
+		ActionClientGetter: mag,
+		BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
+			return &chart.Chart{}, nil
+		}},
+		Client: cl,
+	}
+
+	_, _, err := helmApplier.Apply(context.TODO(), validFS, ext, testObjectLabels, testStorageLabels)
+	require.NoError(t, err)
+	require.NotNil(t, mag.vals)
+	assert.Equal(t, "inlineValue", mag.vals["inlineKey"])
+	assert.Equal(t, "secretValue", mag.vals["secretKey"])
+	assert.Equal(t, "secretVal", mag.vals["sharedKey"])
+}
+
 func TestApply_Installation(t *testing.T) {
 	t.Run("fails during dry-run installation", func(t *testing.T) {
 		mockAcg := &mockActionGetter{
@@ -626,45 +669,6 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) {
 	})
 }
 
-func TestApply_ConfigMerging(t *testing.T) {
-	secret := &corev1.Secret{
-		ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "ns"},
-		Data:       map[string][]byte{"values": []byte("foo:\n  baz: 2\n")},
-	}
-	fakeClient := clientfake.NewClientBuilder().WithObjects(secret).Build()
-
-	mag := &mockActionGetter{
-		getClientErr: driver.ErrReleaseNotFound,
-		desiredRel:   &release.Release{Manifest: "---\n"},
-	}
-	helmApplier := applier.Helm{
-		ActionClientGetter: mag,
-		BundleToHelmChartConverter: &fakeBundleToHelmChartConverter{fn: func(bundle source.BundleSource, installNamespace string, watchNamespace string) (*chart.Chart, error) {
-			return &chart.Chart{}, nil
-		}},
-		Client: fakeClient,
-	}
-
-	ext := &ocv1.ClusterExtension{
-		ObjectMeta: metav1.ObjectMeta{Name: "test"},
-		Spec: ocv1.ClusterExtensionSpec{
-			Namespace:      "ns",
-			ServiceAccount: ocv1.ServiceAccountReference{Name: "sa"},
-			Source:         ocv1.SourceConfig{SourceType: ocv1.SourceTypeCatalog, Catalog: &ocv1.CatalogFilter{PackageName: "pkg"}},
-			Config: &ocv1.ClusterExtensionConfig{
-				Inline:    &apiextensionsv1.JSON{Raw: []byte("foo:\n  bar: 1\n")},
-				SecretRef: &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "cfg"}, Key: "values"},
-			},
-		},
-	}
-
-	_, _, err := helmApplier.Apply(context.TODO(), validFS, ext, testObjectLabels, testStorageLabels)
-	require.NoError(t, err)
-
-	expected := map[string]any{"foo": map[string]any{"bar": float64(1), "baz": float64(2)}}
-	assert.Equal(t, expected, mag.vals["Values"])
-}
-
 type fakeBundleToHelmChartConverter struct {
 	fn func(source.BundleSource, string, string) (*chart.Chart, error)
 }