Rework env var handling with proper deduplication in place

README.md

@@ -198,6 +198,37 @@ Add-ons are installed as Helm releases prefixed with `roxie-addon-` in the Centr
 During teardown, roxie discovers which add-ons have been installed as part of the most
 recent deployment and uninstalls those add-ons as part of a Central teardown.
 
+### Environment variables in CR specs
+
+roxie supports Kubernetes-style `$(VAR)` interpolation in `customize.envVars`.
+When env vars come from multiple sources (config files, `--set` flags, feature
+flags), roxie merges them by name: the first source to introduce a variable
+determines its position in the list, while later sources can override its value.
+
+This preserves **dependent env var ordering** required by Kubernetes — a variable
+that references another via `$(VAR)` must appear after the referent:
+
+```yaml
+central:
+  spec:
+    customize:
+      envVars:
+        - name: BASE_URL
+          value: "https://example.com"
+        - name: API_URL
+          value: "$(BASE_URL)/api"   # resolved by Kubernetes at runtime
+```
+
+roxie emits a warning when it detects a `$(VAR)` reference that cannot be
+resolved because the referent is not defined at that point in the list.
+
+**Limitation:** if a later config source introduces a new variable that an
+earlier source already references via `$(VAR)`, the new variable will appear
+after the reference and Kubernetes will not resolve it. To avoid this, define
+variables before their dependents within the same config source. This is a
+deliberate trade-off to keep the env var merging logic simple and predictable
+while still supporting dependent env vars in the common case.
+
 ## Development
 
 Enter the dev shell:

cmd/deploy_test.go

@@ -340,6 +340,130 @@ roxie:
 	}
 }
 
+// TestNewDeployCmd_ConfigMergePreservesEnvVars verifies that spec.customize.envVars
+// from multiple --config files and --set expressions are merged by name rather
+// than silently replaced. Overlapping names should use the last-specified value;
+// non-overlapping names from earlier sources must be preserved.
+func TestNewDeployCmd_ConfigMergePreservesEnvVars(t *testing.T) {
+	firstConfigPath := filepath.Join(t.TempDir(), "first.yaml")
+	secondConfigPath := filepath.Join(t.TempDir(), "second.yaml")
+
+	tests := []struct {
+		name            string
+		firstConfig     string
+		secondConfig    string
+		args            []string
+		expectedEnvVars map[string]string
+	}{
+		{
+			name: "second config preserves first config's non-overlapping envVars",
+			firstConfig: `
+central:
+  spec:
+    customize:
+      envVars:
+        - name: FIRST_ONLY
+          value: "from-first"
+`,
+			secondConfig: `
+central:
+  spec:
+    customize:
+      envVars:
+        - name: SECOND_ONLY
+          value: from-second
+`,
+			args: []string{"--config", firstConfigPath, "--config", secondConfigPath},
+			expectedEnvVars: map[string]string{
+				"FIRST_ONLY":  "from-first",
+				"SECOND_ONLY": "from-second",
+			},
+		},
+		{
+			name: "second config overrides overlapping envVar without losing others",
+			firstConfig: `
+central:
+  spec:
+    customize:
+      envVars:
+        - name: SHARED
+          value: from-first
+        - name: FIRST_ONLY
+          value: keep
+`,
+			secondConfig: `
+central:
+  spec:
+    customize:
+      envVars:
+        - name: SHARED
+          value: from-second
+`,
+			args: []string{"--config", firstConfigPath, "--config", secondConfigPath},
+			expectedEnvVars: map[string]string{
+				"SHARED":     "from-second",
+				"FIRST_ONLY": "keep",
+			},
+		},
+		{
+			name: "set expression preserves config file envVars",
+			firstConfig: `
+central:
+  spec:
+    customize:
+      envVars:
+        - name: FROM_CONFIG
+          value: keep
+`,
+			// The [0] index is strvals syntax for "first element of the array
+			// I'm constructing", not "replace element 0 of the existing array".
+			// strvals builds a standalone [{FROM_SET: also-keep}] which then
+			// gets merged at the struct level — the index is irrelevant to the
+			// merge. Both envVars should survive, merged by name.
+			args: []string{
+				"--config", firstConfigPath,
+				"--set", "central.spec.customize.envVars[0].name=FROM_SET_1,central.spec.customize.envVars[0].value=one",
+				"--set", "central.spec.customize.envVars[0].name=FROM_SET_2,central.spec.customize.envVars[0].value=two",
+			},
+			expectedEnvVars: map[string]string{
+				"FROM_CONFIG": "keep",
+				"FROM_SET_1":  "one",
+				"FROM_SET_2":  "two",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.NoError(t, os.WriteFile(firstConfigPath, []byte(tt.firstConfig), 0o644))
+			if tt.secondConfig != "" {
+				require.NoError(t, os.WriteFile(secondConfigPath, []byte(tt.secondConfig), 0o644))
+			}
+
+			// Global variable that we keep reusing for the flags parsing.
+			deploySettingsFromArgs = deployer.NewConfig()
+			deployCmd, _, err := rootCmd.Find([]string{"deploy"})
+			require.NoError(t, err)
+			require.NoError(t, deployCmd.ParseFlags(tt.args))
+
+			spec := deploySettingsFromArgs.Central.Spec
+			customize, ok := spec["customize"].(map[string]any)
+			require.True(t, ok, "customize should be present in spec")
+			envVars, ok := customize["envVars"].([]any)
+			require.True(t, ok, "envVars should be present in customize")
+
+			got := make(map[string]string)
+			for _, item := range envVars {
+				ev := item.(map[string]any)
+				got[ev["name"].(string)] = ev["value"].(string)
+			}
+			assert.Equal(t, tt.expectedEnvVars, got)
+			assert.Equal(t, len(tt.expectedEnvVars), len(envVars),
+				"envVars slice should have no duplicates")
+		})
+	}
+}
+
 func TestNewDeployCmd_SetRejectsSpec(t *testing.T) {
 	tests := []struct {
 		name string

cmd/main.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"reflect"
 
-	"dario.cat/mergo"
 	"github.com/fatih/color"
 	"github.com/mattn/go-isatty"
 	"github.com/spf13/cobra"
@@ -60,7 +59,7 @@ func tryApplyUserDefaults(config *deployer.Config) error {
 	if err := yaml.Unmarshal(data, &userDefaults); err != nil {
 		return fmt.Errorf("parsing user config %q: %w", path, err)
 	}
-	if err := mergo.Merge(config, &userDefaults, mergo.WithOverride, mergo.WithoutDereference); err != nil {
+	if err := deployer.MergeConfig(config, &userDefaults); err != nil {
 		return fmt.Errorf("merging user config %q: %w", path, err)
 	}
 	log.Dimf("Applied user config from %s", path)
@@ -111,7 +110,7 @@ func init() {
 			if err := yaml.Unmarshal(data, &configFromFile); err != nil {
 				return fmt.Errorf("failed to unmarshal config from %q: %w", filename, err)
 			}
-			if err := mergo.Merge(config, &configFromFile, mergo.WithOverride, mergo.WithoutDereference); err != nil {
+			if err := deployer.MergeConfig(config, &configFromFile); err != nil {
 				return fmt.Errorf("merging config from %q into deployer Config: %w", filename, err)
 			}
 			return nil
@@ -135,7 +134,7 @@ func init() {
 				return fmt.Errorf("set expression %q had no effect -- typo?", expr)
 			}
 
-			if err := mergo.Merge(config, &patch, mergo.WithOverride, mergo.WithoutDereference); err != nil {
+			if err := deployer.MergeConfig(config, &patch); err != nil {
 				return fmt.Errorf("merging set-expression %q into deployer Config: %w", expr, err)
 			}
 

internal/deployer/config.go

@@ -4,11 +4,15 @@ import (
 	"fmt"
 	"time"
 
+	"dario.cat/mergo"
 	"github.com/stackrox/roxie/internal/constants"
+	"github.com/stackrox/roxie/internal/envvars"
 	"github.com/stackrox/roxie/internal/helpers"
 	"github.com/stackrox/roxie/internal/imagetag"
+	log "github.com/stackrox/roxie/internal/logger"
 	"github.com/stackrox/roxie/internal/types"
 	"gopkg.in/yaml.v3"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 // Config is the top-level deployment configuration, combining settings for
@@ -38,6 +42,42 @@ func NewConfig() Config {
 	}
 }
 
+// MergeConfig merges overlay into base with envVar-aware handling.
+// Generic slices use replacement semantics (overlay wins), but
+// customize.envVars entries are merged by name across sources
+// (--config, --set, user defaults) with last-one-wins deduplication.
+func MergeConfig(base, overlay *Config) error {
+	envVarsPath := []string{"customize", "envVars"}
+	baseCentralEnvVars, _, _ := unstructured.NestedSlice(base.Central.Spec, envVarsPath...)
+	baseSCEnvVars, _, _ := unstructured.NestedSlice(base.SecuredCluster.Spec, envVarsPath...)
+
+	if err := mergo.Merge(base, overlay, mergo.WithOverride, mergo.WithoutDereference); err != nil {
+		return err
+	}
+
+	if err := envvars.MergeByNameInUnstructured(base.Central.Spec, baseCentralEnvVars, envVarsPath...); err != nil {
+		return fmt.Errorf("merging Central env vars: %w", err)
+	}
+	if err := envvars.MergeByNameInUnstructured(base.SecuredCluster.Spec, baseSCEnvVars, envVarsPath...); err != nil {
+		return fmt.Errorf("merging SecuredCluster env vars: %w", err)
+	}
+
+	warnUnresolvedEnvVarRefs("Central", base.Central.Spec, envVarsPath)
+	warnUnresolvedEnvVarRefs("SecuredCluster", base.SecuredCluster.Spec, envVarsPath)
+	return nil
+}
+
+func warnUnresolvedEnvVarRefs(component string, spec map[string]any, path []string) {
+	envVars, found, _ := unstructured.NestedSlice(spec, path...)
+	if !found {
+		return
+	}
+	for _, ref := range envvars.FindUnresolvedRefs(envVars) {
+		log.Warningf("%s env var %s references $(%s), which is not defined at that point — Kubernetes will not resolve it",
+			component, ref.Name, ref.RefName)
+	}
+}
+
 // DeepCopy creates a deep-copy of the provided config using a YAML marshaling/unmarshaling roundtrip.
 // Due the `omitempty`, this causes empty values (e.g. empty maps) from being discarded (replace with nil
 // in the resulting copy).
@@ -323,15 +363,20 @@ func (c *CentralConfig) GetExposure() types.Exposure {
 
 // ConfigureSpec applies feature flags and exposure settings to the Central spec.
 func (c *CentralConfig) ConfigureSpec(roxieConfig *RoxieConfig) error {
-	err := helpers.DeepMerge(c.Spec, featureFlagsToOverrides(roxieConfig.FeatureFlags))
-	if err != nil {
+	envVarsPath := []string{"customize", "envVars"}
+	baseEnvVars, _, _ := unstructured.NestedSlice(c.Spec, envVarsPath...)
+	if err := mergo.Merge(&c.Spec, featureFlagsToOverrides(roxieConfig.FeatureFlags), mergo.WithOverride); err != nil {
 		return err
 	}
-	if err = helpers.DeepMerge(c.Spec, map[string]interface{}{
-		"central": map[string]interface{}{
+	if err := envvars.MergeByNameInUnstructured(c.Spec, baseEnvVars, envVarsPath...); err != nil {
+		return fmt.Errorf("merging Central env vars by name: %w", err)
+	}
+	warnUnresolvedEnvVarRefs("Central", c.Spec, []string{"customize", "envVars"})
+	if err := mergo.Merge(&c.Spec, map[string]any{
+		"central": map[string]any{
 			"exposure": c.Exposure.ToUnstructuredConfig(),
 		},
-	}); err != nil {
+	}, mergo.WithOverride); err != nil {
 		return err
 	}
 	return nil
@@ -361,18 +406,15 @@ func (c *CentralConfig) CustomResource() (map[string]interface{}, error) {
 		return nil, fmt.Errorf("resource profile 'auto' should have been resolved before building the CR")
 	}
 	if c.ResourceProfile != types.ResourceProfileAcsDefaults {
-		profileCopy, err := helpers.MergeMaps(getCentralResourcesOperator(c.ResourceProfile))
-		if err != nil {
-			return nil, fmt.Errorf("copying resource profile: %w", err)
-		}
-		if err := helpers.DeepMerge(cr, profileCopy); err != nil {
+		profileCopy := helpers.DeepCopyMap(getCentralResourcesOperator(c.ResourceProfile))
+		if err := mergo.Merge(&cr, profileCopy, mergo.WithOverride, mergo.WithAppendSlice); err != nil {
 			return nil, fmt.Errorf("merging resource profile into Central CR: %w", err)
 		}
 	}
-	if err := helpers.DeepMerge(cr, map[string]interface{}{
+	if err := mergo.Merge(&cr, map[string]interface{}{
 		"metadata": c.Metadata.toMap(),
 		"spec":     c.Spec,
-	}); err != nil {
+	}, mergo.WithOverride, mergo.WithAppendSlice); err != nil {
 		return nil, fmt.Errorf("merging metadata and spec into Central CR: %w", err)
 	}
 	return cr, nil
@@ -438,10 +480,15 @@ func (s *SecuredClusterConfig) GetWaitConfig() WaitConfig {
 
 // ConfigureSpec applies feature flags and the central endpoint to the SecuredCluster spec.
 func (s *SecuredClusterConfig) ConfigureSpec(roxieConfig *RoxieConfig, centralConfig *CentralConfig) error {
-	if err := helpers.DeepMerge(s.Spec, featureFlagsToOverrides(roxieConfig.FeatureFlags)); err != nil {
+	envVarsPath := []string{"customize", "envVars"}
+	baseEnvVars, _, _ := unstructured.NestedSlice(s.Spec, envVarsPath...)
+	if err := mergo.Merge(&s.Spec, featureFlagsToOverrides(roxieConfig.FeatureFlags), mergo.WithOverride); err != nil {
 		return err
 	}
-
+	if err := envvars.MergeByNameInUnstructured(s.Spec, baseEnvVars, envVarsPath...); err != nil {
+		return fmt.Errorf("merging SecuredCluster env vars by name: %w", err)
+	}
+	warnUnresolvedEnvVarRefs("SecuredCluster", s.Spec, []string{"customize", "envVars"})
 	if _, exists := s.Spec["centralEndpoint"]; !exists {
 		s.Spec["centralEndpoint"] = internalCentralEndpoint(centralConfig.Namespace)
 	}
@@ -466,19 +513,16 @@ func (s *SecuredClusterConfig) CustomResource() (map[string]interface{}, error)
 		return nil, fmt.Errorf("resource profile 'auto' must have been resolved before building the CR")
 	}
 	if s.ResourceProfile != types.ResourceProfileAcsDefaults {
-		profileCopy, err := helpers.MergeMaps(getSecuredClusterResourcesOperator(s.ResourceProfile))
-		if err != nil {
-			return nil, fmt.Errorf("copying resource profile: %w", err)
-		}
-		if err := helpers.DeepMerge(cr, profileCopy); err != nil {
+		profileCopy := helpers.DeepCopyMap(getSecuredClusterResourcesOperator(s.ResourceProfile))
+		if err := mergo.Merge(&cr, profileCopy, mergo.WithOverride, mergo.WithAppendSlice); err != nil {
 			return nil, fmt.Errorf("merging resource profile into SecuredCluster CR: %w", err)
 		}
 	}
 
-	if err := helpers.DeepMerge(cr, map[string]interface{}{
+	if err := mergo.Merge(&cr, map[string]interface{}{
 		"metadata": s.Metadata.toMap(),
 		"spec":     s.Spec,
-	}); err != nil {
+	}, mergo.WithOverride, mergo.WithAppendSlice); err != nil {
 		return nil, fmt.Errorf("merging metadata and spec into SecuredCluster CR: %w", err)
 	}
 	return cr, nil