diff --git a/README.md b/README.md index 255de41a..c128bdec 100644 --- a/README.md +++ b/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: diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index d6a3e842..a8130eed 100644 --- a/cmd/deploy_test.go +++ b/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 diff --git a/cmd/main.go b/cmd/main.go index 466fb8d5..d1de98a2 100644 --- a/cmd/main.go +++ b/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) } diff --git a/internal/deployer/config.go b/internal/deployer/config.go index 37c2f770..fdaf3ef2 100644 --- a/internal/deployer/config.go +++ b/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 diff --git a/internal/deployer/configure_spec_envvars_test.go b/internal/deployer/configure_spec_envvars_test.go new file mode 100644 index 00000000..cbc9b45e --- /dev/null +++ b/internal/deployer/configure_spec_envvars_test.go @@ -0,0 +1,126 @@ +package deployer + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// TestConfigureSpec_EnvVarDeduplication tests that ConfigureSpec correctly merges +// feature flags with pre-existing env vars in the spec, producing no duplicates +// and preserving non-overlapping vars. Covers both Central and SecuredCluster. +func TestConfigureSpec_EnvVarDeduplication(t *testing.T) { + tests := []struct { + name string + existingEnvVars []any + featureFlags map[string]bool + expectedEnvVars map[string]string + }{ + { + name: "config envVars preserved when features add non-overlapping flags", + existingEnvVars: envVarSlice( + map[string]string{"name": "MY_CUSTOM_VAR", "value": "hello"}, + ), + featureFlags: map[string]bool{ + "ROX_NEW": true, + }, + expectedEnvVars: map[string]string{ + "MY_CUSTOM_VAR": "hello", + "ROX_NEW": "true", + }, + }, + { + name: "features override overlapping config envVar without duplicates", + existingEnvVars: envVarSlice( + map[string]string{"name": "ROX_FOO", "value": "true"}, + ), + featureFlags: map[string]bool{ + "ROX_FOO": false, + }, + expectedEnvVars: map[string]string{ + "ROX_FOO": "false", + }, + }, + { + name: "mixed envVars with partial overlap", + existingEnvVars: envVarSlice( + map[string]string{"name": "MY_CUSTOM_VAR", "value": "hello"}, + map[string]string{"name": "ROX_FOO", "value": "true"}, + ), + featureFlags: map[string]bool{ + "ROX_FOO": false, + }, + expectedEnvVars: map[string]string{ + "MY_CUSTOM_VAR": "hello", + "ROX_FOO": "false", + }, + }, + { + name: "no existing envVars, features added cleanly", + existingEnvVars: nil, + featureFlags: map[string]bool{ + "ROX_A": true, + "ROX_B": false, + }, + expectedEnvVars: map[string]string{ + "ROX_A": "true", + "ROX_B": "false", + }, + }, + } + + for _, tt := range tests { + roxie := NewRoxieConfig() + roxie.FeatureFlags = tt.featureFlags + + t.Run("central/"+tt.name, func(t *testing.T) { + cfg := NewCentralConfig() + if tt.existingEnvVars != nil { + cfg.Spec["customize"] = map[string]any{"envVars": tt.existingEnvVars} + } + + require.NoError(t, cfg.ConfigureSpec(&roxie)) + + got := extractEnvVars(t, cfg.Spec) + assert.Equal(t, tt.expectedEnvVars, got) + }) + + t.Run("securedCluster/"+tt.name, func(t *testing.T) { + cfg := SecuredClusterConfig{Namespace: "stackrox", Spec: make(map[string]any)} + if tt.existingEnvVars != nil { + cfg.Spec["customize"] = map[string]any{"envVars": tt.existingEnvVars} + } + + require.NoError(t, cfg.ConfigureSpec(&roxie, &CentralConfig{Namespace: "acs-central"})) + + got := extractEnvVars(t, cfg.Spec) + assert.Equal(t, tt.expectedEnvVars, got) + }) + } +} + +func envVarSlice(vars ...map[string]string) []any { + result := make([]any, len(vars)) + for i, v := range vars { + result[i] = map[string]any{"name": v["name"], "value": v["value"]} + } + return result +} + +func extractEnvVars(t *testing.T, spec map[string]any) map[string]string { + t.Helper() + envVars, found, err := unstructured.NestedSlice(spec, "customize", "envVars") + require.NoError(t, err) + require.True(t, found, "customize.envVars not found in spec") + + result := make(map[string]string) + for _, item := range envVars { + ev := item.(map[string]any) + name := ev["name"].(string) + require.NotContains(t, result, name, "duplicate envVar %q in slice", name) + result[name] = ev["value"].(string) + } + return result +} diff --git a/internal/deployer/feature_flags.go b/internal/deployer/feature_flags.go index 5912feb7..701dd2a3 100644 --- a/internal/deployer/feature_flags.go +++ b/internal/deployer/feature_flags.go @@ -2,13 +2,8 @@ package deployer import ( "fmt" - "slices" - "sort" "strconv" "strings" - - "github.com/stackrox/roxie/internal/helpers" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) // parseFlagWithEquals parses a feature flag in the format "ROX_FOO=true" or "ROX_FOO=false" @@ -111,48 +106,3 @@ func featureFlagsToOverrides(flags map[string]bool) map[string]interface{} { }, } } - -// mergeEnvVars smartly merges envVars arrays, with newer values overriding older ones by name. -// This allows feature flags to override specific env vars without wiping the entire array. -func mergeEnvVars(base, overlay []interface{}) []interface{} { - envVarMap := make(map[string]interface{}) - - for _, item := range slices.Concat(base, overlay) { - if envVar, ok := item.(map[string]interface{}); ok { - if name, ok := envVar["name"].(string); ok { - envVarMap[name] = envVar - } - } - } - - result := make([]interface{}, 0, len(envVarMap)) - for _, envVar := range envVarMap { - result = append(result, envVar) - } - - sort.Slice(result, func(i, j int) bool { - return result[i].(map[string]interface{})["name"].(string) < - result[j].(map[string]interface{})["name"].(string) - }) - - return result -} - -// mergeWithEnvVarSupport merges two maps, with special handling for spec.customize.envVars arrays. -// Instead of replacing the entire envVars array, it merges individual env vars by name, -// allowing overlay to override specific env vars while preserving others from base. -func mergeWithEnvVarSupport(base, overlay map[string]interface{}) (map[string]interface{}, error) { - result, err := helpers.MergeMaps(base, overlay) - if err != nil { - return nil, err - } - - baseEnvVars, baseFound, _ := unstructured.NestedSlice(base, "spec", "customize", "envVars") - overlayEnvVars, overlayFound, _ := unstructured.NestedSlice(overlay, "spec", "customize", "envVars") - - if baseFound && overlayFound { - _ = unstructured.SetNestedSlice(result, mergeEnvVars(baseEnvVars, overlayEnvVars), "spec", "customize", "envVars") - } - - return result, nil -} diff --git a/internal/deployer/feature_flags_test.go b/internal/deployer/feature_flags_test.go index bc6c72ae..6bd1e0c1 100644 --- a/internal/deployer/feature_flags_test.go +++ b/internal/deployer/feature_flags_test.go @@ -4,6 +4,7 @@ import ( "reflect" "testing" + "github.com/stackrox/roxie/internal/envvars" "github.com/stretchr/testify/require" ) @@ -142,266 +143,30 @@ func TestParseFeatureFlags(t *testing.T) { } } -func TestMergeEnvVars(t *testing.T) { - tests := []struct { - name string - base []interface{} - overlay []interface{} - expected map[string]string - checkOrdering bool - expectedOrder []string - }{ - { - name: "overlapping names - overlay wins", - base: []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "old"}, - map[string]interface{}{"name": "ROX_BAR", "value": "keep"}, - }, - overlay: []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "new"}, - }, - expected: map[string]string{ - "ROX_FOO": "new", - "ROX_BAR": "keep", - }, - }, - { - name: "no overlapping names", - base: []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "foo"}, - }, - overlay: []interface{}{ - map[string]interface{}{"name": "ROX_BAR", "value": "bar"}, - }, - expected: map[string]string{ - "ROX_FOO": "foo", - "ROX_BAR": "bar", - }, - }, - { - name: "empty base", - base: []interface{}{}, - overlay: []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "foo"}, - }, - expected: map[string]string{ - "ROX_FOO": "foo", - }, - }, - { - name: "empty overlay", - base: []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "foo"}, - }, - overlay: []interface{}{}, - expected: map[string]string{ - "ROX_FOO": "foo", - }, - }, - { - name: "malformed envVar without name field", - base: []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "foo"}, - }, - overlay: []interface{}{ - map[string]interface{}{"value": "no-name"}, // missing "name" field - map[string]interface{}{"name": "ROX_BAR", "value": "bar"}, - }, - expected: map[string]string{ - "ROX_FOO": "foo", - "ROX_BAR": "bar", - }, - }, - { - name: "result is sorted alphabetically", - base: []interface{}{ - map[string]interface{}{"name": "ROX_ZZZ", "value": "z"}, - map[string]interface{}{"name": "ROX_AAA", "value": "a"}, - }, - overlay: []interface{}{ - map[string]interface{}{"name": "ROX_MMM", "value": "m"}, - }, - expected: map[string]string{ - "ROX_AAA": "a", - "ROX_MMM": "m", - "ROX_ZZZ": "z", - }, - checkOrdering: true, - expectedOrder: []string{"ROX_AAA", "ROX_MMM", "ROX_ZZZ"}, - }, +func TestFeatureFlagMergeWithEnvVarDedup(t *testing.T) { + base := []any{ + map[string]any{"name": "MY_VAR", "value": "keep"}, + map[string]any{"name": "ROX_FOO", "value": "old"}, } + spec := featureFlagsToOverrides(map[string]bool{"ROX_FOO": false, "ROX_NEW": true}) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := mergeEnvVars(tt.base, tt.overlay) + err := envvars.MergeByNameInUnstructured(spec, base, "customize", "envVars") + require.NoError(t, err) - if len(result) != len(tt.expected) { - t.Fatalf("expected %d env vars, got %d", len(tt.expected), len(result)) - } + customize := spec["customize"].(map[string]any) + envVars := customize["envVars"].([]any) - resultMap := make(map[string]string) - for _, item := range result { - envVar := item.(map[string]interface{}) - if name, ok := envVar["name"].(string); ok { - resultMap[name] = envVar["value"].(string) - } - } - - if !reflect.DeepEqual(resultMap, tt.expected) { - t.Errorf("got %v, want %v", resultMap, tt.expected) - } - - if tt.checkOrdering { - actualOrder := make([]string, len(result)) - for i, item := range result { - envVar := item.(map[string]interface{}) - actualOrder[i] = envVar["name"].(string) - } - if !reflect.DeepEqual(actualOrder, tt.expectedOrder) { - t.Errorf("ordering: got %v, want %v", actualOrder, tt.expectedOrder) - } - } - }) + resultMap := make(map[string]string) + for _, item := range envVars { + envVar := item.(map[string]any) + resultMap[envVar["name"].(string)] = envVar["value"].(string) } -} -func TestMergeWithEnvVarSupport(t *testing.T) { - tests := []struct { - name string - base map[string]interface{} - overlay map[string]interface{} - expectedEnvVars map[string]string - checkOtherFields bool - }{ - { - name: "both have envVars with overlap", - base: map[string]interface{}{ - "spec": map[string]interface{}{ - "customize": map[string]interface{}{ - "envVars": []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "old"}, - map[string]interface{}{"name": "ROX_KEEP", "value": "me"}, - }, - }, - }, - }, - overlay: map[string]interface{}{ - "spec": map[string]interface{}{ - "customize": map[string]interface{}{ - "envVars": []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "new"}, - }, - }, - }, - }, - expectedEnvVars: map[string]string{ - "ROX_FOO": "new", - "ROX_KEEP": "me", - }, - }, - { - name: "base has envVars, overlay has none", - base: map[string]interface{}{ - "spec": map[string]interface{}{ - "customize": map[string]interface{}{ - "envVars": []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "foo"}, - }, - }, - }, - }, - overlay: map[string]interface{}{ - "spec": map[string]interface{}{ - "other": "field", - }, - }, - expectedEnvVars: map[string]string{ - "ROX_FOO": "foo", - }, - }, - { - name: "overlay has envVars, base has none", - base: map[string]interface{}{ - "spec": map[string]interface{}{ - "other": "field", - }, - }, - overlay: map[string]interface{}{ - "spec": map[string]interface{}{ - "customize": map[string]interface{}{ - "envVars": []interface{}{ - map[string]interface{}{"name": "ROX_FOO", "value": "foo"}, - }, - }, - }, - }, - expectedEnvVars: map[string]string{ - "ROX_FOO": "foo", - }, - }, - { - name: "neither has envVars", - base: map[string]interface{}{ - "spec": map[string]interface{}{ - "other": "field", - }, - }, - overlay: map[string]interface{}{ - "spec": map[string]interface{}{ - "another": "field", - }, - }, - expectedEnvVars: nil, - checkOtherFields: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := mergeWithEnvVarSupport(tt.base, tt.overlay) - require.NoError(t, err) - - spec, ok := result["spec"].(map[string]interface{}) - if !ok { - t.Fatal("spec should be a map") - } - - if tt.expectedEnvVars != nil { - customize, ok := spec["customize"].(map[string]interface{}) - if !ok { - t.Fatal("customize should be a map") - } - - envVars, ok := customize["envVars"].([]interface{}) - if !ok { - t.Fatal("envVars should be a slice") - } - - if len(envVars) != len(tt.expectedEnvVars) { - t.Fatalf("expected %d env vars, got %d", len(tt.expectedEnvVars), len(envVars)) - } - - resultMap := make(map[string]string) - for _, item := range envVars { - envVar := item.(map[string]interface{}) - resultMap[envVar["name"].(string)] = envVar["value"].(string) - } - - if !reflect.DeepEqual(resultMap, tt.expectedEnvVars) { - t.Errorf("got %v, want %v", resultMap, tt.expectedEnvVars) - } - } - - if tt.checkOtherFields { - if spec["other"] != "field" { - t.Errorf("expected base field to be preserved") - } - if spec["another"] != "field" { - t.Errorf("expected overlay field to be added") - } - } - }) - } + require.Equal(t, map[string]string{ + "MY_VAR": "keep", + "ROX_FOO": "false", + "ROX_NEW": "true", + }, resultMap) } func TestFeatureFlagsConversion(t *testing.T) { diff --git a/internal/envvars/envvars.go b/internal/envvars/envvars.go new file mode 100644 index 00000000..afb82130 --- /dev/null +++ b/internal/envvars/envvars.go @@ -0,0 +1,115 @@ +package envvars + +import ( + "fmt" + "regexp" + "strings" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +var ( + // Note, we match the dollars in a separate match group to be able to check for escaped dollar characters. + varRefPattern = regexp.MustCompile(`(\$+)\(([^)]+)\)`) +) + +// UnresolvedRef describes an env var that references a variable not +// yet defined at that point in the list. +type UnresolvedRef struct { + Name string + RefName string +} + +// FindUnresolvedRefs checks a Kubernetes-style envVars slice for $(VAR) +// references that Kubernetes cannot resolve because the referent is not +// defined at the point of reference (absent or appears later in the list). +// Also handles escaping such as "$$(NOT_GETTING_INTERPOLATED)". +func FindUnresolvedRefs(envVars []any) []UnresolvedRef { + defined := make(map[string]bool) + var refs []UnresolvedRef + + for _, item := range envVars { + ev, ok := item.(map[string]any) + if !ok { + continue + } + name, ok := ev["name"].(string) + if !ok { + continue + } + if ev["value"] == nil && ev["valueFrom"] == nil { + continue + } + defined[name] = true + val, ok := ev["value"].(string) + if !ok { + continue + } + for _, match := range varRefPattern.FindAllStringSubmatch(val, -1) { + dollars := match[1] + refName := match[2] + if len(dollars)%2 == 0 { + continue // even number of $ — escaped, e.g. $$(VAR) + } + if !defined[refName] { + refs = append(refs, UnresolvedRef{Name: name, RefName: refName}) + } + } + } + return refs +} + +// deduplicate deduplicates a Kubernetes-style envVars slice by name. +// First occurrence of a name determines its position in the output; +// last occurrence determines its value. This preserves the dependency +// ordering required by Kubernetes' $(VAR) interpolation: a variable +// that references another via $(VAR) must appear after the referent. +// +// Limitation: if a later 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. +func deduplicate(envVars []any) []any { + var result []any + indices := make(map[string]int) // Stores name-keyed indices in result slice. + + // Takes an any-item and returns its name and its complete unstructured obj. + // Returns non-empty name on success. + extractItem := func(item any) (name string, envVarItem map[string]any) { + var ok bool + envVarItem, ok = item.(map[string]any) + if !ok { + return + } + name, _ = envVarItem["name"].(string) + return + } + + for _, item := range envVars { + name, envVarItem := extractItem(item) + if name == "" { + continue + } + if i, exists := indices[name]; exists { + result[i] = envVarItem + } else { + indices[name] = len(result) + result = append(result, envVarItem) + } + } + return result +} + +// MergeByNameInUnstructured prepends baseEnvVars to the envVars already at +// path in obj, then deduplicates by name (last-one-wins, so current entries +// from an overlay take precedence over base). No-op if both sources are empty. +func MergeByNameInUnstructured(obj map[string]any, baseEnvVars []any, path ...string) error { + current, _, err := unstructured.NestedSlice(obj, path...) + if err != nil { + return fmt.Errorf("retrieving nested slice at path %s from unstructured object: %w", strings.Join(path, "."), err) + } + merged := append(baseEnvVars, current...) + if len(merged) == 0 { + return nil + } + return unstructured.SetNestedSlice(obj, deduplicate(merged), path...) +} diff --git a/internal/envvars/envvars_test.go b/internal/envvars/envvars_test.go new file mode 100644 index 00000000..d4dd8e71 --- /dev/null +++ b/internal/envvars/envvars_test.go @@ -0,0 +1,253 @@ +package envvars + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestMergeByNameInUnstructured(t *testing.T) { + type envVar struct { + Name string `json:"name"` + Value string `json:"value"` + } + + tests := []struct { + name string + base []any + spec map[string]interface{} + expectedEnvVars []envVar + }{ + { + name: "duplicates are resolved with last-one-wins", + spec: map[string]interface{}{ + "customize": map[string]interface{}{ + "envVars": []interface{}{ + map[string]interface{}{"name": "ROX_FOO", "value": "old"}, + map[string]interface{}{"name": "ROX_KEEP", "value": "me"}, + map[string]interface{}{"name": "ROX_FOO", "value": "new"}, + }, + }, + }, + expectedEnvVars: []envVar{ + {"ROX_FOO", "new"}, + {"ROX_KEEP", "me"}, + }, + }, + { + name: "no duplicates is a no-op", + spec: map[string]interface{}{ + "customize": map[string]interface{}{ + "envVars": []interface{}{ + map[string]interface{}{"name": "ROX_A", "value": "a"}, + map[string]interface{}{"name": "ROX_B", "value": "b"}, + }, + }, + }, + expectedEnvVars: []envVar{ + {"ROX_A", "a"}, + {"ROX_B", "b"}, + }, + }, + { + name: "no envVars is a no-op", + spec: map[string]interface{}{"other": "field"}, + expectedEnvVars: nil, + }, + { + name: "empty spec is a no-op", + spec: map[string]interface{}{}, + expectedEnvVars: nil, + }, + { + name: "overlay wins over base", + base: []any{map[string]any{"name": "ROX_A", "value": "from-base"}}, + spec: map[string]interface{}{ + "customize": map[string]interface{}{ + "envVars": []interface{}{ + map[string]interface{}{"name": "ROX_A", "value": "from-overlay"}, + }, + }, + }, + expectedEnvVars: []envVar{{"ROX_A", "from-overlay"}}, + }, + { + name: "base entries preserved when not in overlay", + base: []any{map[string]any{"name": "ROX_BASE", "value": "kept"}}, + spec: map[string]interface{}{ + "customize": map[string]interface{}{ + "envVars": []interface{}{ + map[string]interface{}{"name": "ROX_NEW", "value": "added"}, + }, + }, + }, + expectedEnvVars: []envVar{{"ROX_BASE", "kept"}, {"ROX_NEW", "added"}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := MergeByNameInUnstructured(tt.spec, tt.base, "customize", "envVars") + require.NoError(t, err) + + if tt.expectedEnvVars == nil { + return + } + + customize := tt.spec["customize"].(map[string]any) + envVars := customize["envVars"].([]any) + expectedEnvVarsJSON, err := json.Marshal(tt.expectedEnvVars) + require.NoError(t, err) + gotJSON, err := json.Marshal(envVars) + require.NoError(t, err) + require.JSONEq(t, string(expectedEnvVarsJSON), string(gotJSON)) + }) + } +} + +func TestMergeByNameInUnstructured_PreservesValueFrom(t *testing.T) { + valueFrom := map[string]any{ + "secretKeyRef": map[string]any{ + "name": "my-secret", + "key": "password", + }, + } + spec := map[string]any{ + "customize": map[string]any{ + "envVars": []any{ + map[string]any{"name": "SECRET_VAR", "valueFrom": valueFrom}, + map[string]any{"name": "PLAIN_VAR", "value": "hello"}, + }, + }, + } + + require.NoError(t, MergeByNameInUnstructured(spec, nil, "customize", "envVars")) + + customize := spec["customize"].(map[string]any) + envVars := customize["envVars"].([]any) + require.Len(t, envVars, 2) + + secretVar := envVars[0].(map[string]any) + require.Equal(t, "SECRET_VAR", secretVar["name"]) + require.Equal(t, valueFrom, secretVar["valueFrom"]) + _, hasValue := secretVar["value"] + require.False(t, hasValue, "valueFrom entry should not have a value field") +} + +func TestMergeByNameInUnstructured_PreservesDependencyOrder(t *testing.T) { + spec := map[string]any{ + "customize": map[string]any{ + "envVars": []any{ + map[string]any{"name": "BASE_URL", "value": "https://example.com"}, + map[string]any{"name": "API_URL", "value": "$(BASE_URL)/api"}, + map[string]any{"name": "BASE_URL", "value": "https://override.com"}, + }, + }, + } + + require.NoError(t, MergeByNameInUnstructured(spec, nil, "customize", "envVars")) + + customize := spec["customize"].(map[string]any) + envVars := customize["envVars"].([]any) + require.Len(t, envVars, 2) + + require.Equal(t, "BASE_URL", envVars[0].(map[string]any)["name"]) + require.Equal(t, "https://override.com", envVars[0].(map[string]any)["value"]) + require.Equal(t, "API_URL", envVars[1].(map[string]any)["name"]) + require.Equal(t, "$(BASE_URL)/api", envVars[1].(map[string]any)["value"]) +} + +func TestFindUnresolvedRefs(t *testing.T) { + tests := []struct { + name string + envVars []any + expected []UnresolvedRef + }{ + { + name: "no references", + envVars: []any{ + map[string]any{"name": "A", "value": "hello"}, + map[string]any{"name": "B", "value": "world"}, + }, + expected: nil, + }, + { + name: "resolved forward reference", + envVars: []any{ + map[string]any{"name": "BASE", "value": "https://example.com"}, + map[string]any{"name": "URL", "value": "$(BASE)/api"}, + }, + expected: nil, + }, + { + name: "unresolved: referent defined later", + envVars: []any{ + map[string]any{"name": "URL", "value": "$(BASE)/api"}, + map[string]any{"name": "BASE", "value": "https://example.com"}, + }, + expected: []UnresolvedRef{ + {Name: "URL", RefName: "BASE"}, + }, + }, + { + name: "unresolved: referent not defined", + envVars: []any{ + map[string]any{"name": "URL", "value": "$(MISSING)/api"}, + }, + expected: []UnresolvedRef{ + {Name: "URL", RefName: "MISSING"}, + }, + }, + { + name: "multiple references in one value", + envVars: []any{ + map[string]any{"name": "FULL", "value": "$(PROTO)://$(HOST)"}, + map[string]any{"name": "PROTO", "value": "https"}, + }, + expected: []UnresolvedRef{ + {Name: "FULL", RefName: "PROTO"}, + {Name: "FULL", RefName: "HOST"}, + }, + }, + { + name: "escaped $$(VAR) is not a reference", + envVars: []any{ + map[string]any{"name": "LITERAL", "value": "$$(NOT_A_REF)"}, + }, + expected: nil, + }, + { + name: "$$$(VAR) is literal $ followed by a real reference", + envVars: []any{ + map[string]any{"name": "BASE", "value": "x"}, + map[string]any{"name": "COMBO", "value": "$$$(BASE)"}, + }, + expected: nil, + }, + { + name: "$$$(VAR) with undefined referent is flagged", + envVars: []any{ + map[string]any{"name": "COMBO", "value": "$$$(UNDEF)"}, + }, + expected: []UnresolvedRef{ + {Name: "COMBO", RefName: "UNDEF"}, + }, + }, + { + name: "valueFrom entries are skipped", + envVars: []any{ + map[string]any{"name": "SECRET", "valueFrom": map[string]any{"secretKeyRef": map[string]any{"name": "s", "key": "k"}}}, + map[string]any{"name": "PLAIN", "value": "$(SECRET)"}, + }, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FindUnresolvedRefs(tt.envVars) + require.Equal(t, tt.expected, got) + }) + } +} diff --git a/internal/helpers/helpers.go b/internal/helpers/helpers.go index b56160bc..7a6cad65 100644 --- a/internal/helpers/helpers.go +++ b/internal/helpers/helpers.go @@ -7,7 +7,6 @@ import ( "os/exec" "strings" - "dario.cat/mergo" "gopkg.in/yaml.v3" ) @@ -62,26 +61,13 @@ func LoadYAMLFile(path string) (map[string]interface{}, error) { return result, nil } -// MergeMaps deeply merges multiple maps, with later maps taking precedence -func MergeMaps(base map[string]interface{}, overlays ...map[string]interface{}) (map[string]interface{}, error) { - result := deepCopy(base) - - for _, overlay := range overlays { - if err := DeepMerge(result, overlay); err != nil { - return nil, err - } - } - - return result, nil -} - -// deepCopy creates a deep copy of a map -func deepCopy(m map[string]interface{}) map[string]interface{} { +// DeepCopyMap creates a deep copy of an unstructured map. +func DeepCopyMap(m map[string]interface{}) map[string]interface{} { result := make(map[string]interface{}) for k, v := range m { switch val := v.(type) { case map[string]interface{}: - result[k] = deepCopy(val) + result[k] = DeepCopyMap(val) case []interface{}: result[k] = deepCopySlice(val) default: @@ -97,7 +83,7 @@ func deepCopySlice(s []interface{}) []interface{} { for i, v := range s { switch val := v.(type) { case map[string]interface{}: - result[i] = deepCopy(val) + result[i] = DeepCopyMap(val) case []interface{}: result[i] = deepCopySlice(val) default: @@ -107,12 +93,6 @@ func deepCopySlice(s []interface{}) []interface{} { return result } -// DeepMerge recursively merges overlay into base. -// Slices are appended, with values from overlay appearing after values from base. -func DeepMerge(base, overlay map[string]any) error { - return mergo.Merge(&base, &overlay, mergo.WithOverride, mergo.WithAppendSlice) -} - func MapToStruct(m map[string]interface{}, out interface{}) error { bytes, err := yaml.Marshal(m) if err != nil { diff --git a/internal/helpers/helpers_test.go b/internal/helpers/helpers_test.go index 1cbc0bcc..7566b52a 100644 --- a/internal/helpers/helpers_test.go +++ b/internal/helpers/helpers_test.go @@ -4,80 +4,8 @@ import ( "os" "path/filepath" "testing" - - "github.com/stretchr/testify/require" ) -func TestMergeMaps(t *testing.T) { - base := map[string]interface{}{ - "a": "value_a", - "b": map[string]interface{}{ - "c": "value_c", - "d": "value_d", - }, - } - - overlay := map[string]interface{}{ - "b": map[string]interface{}{ - "d": "value_d_override", - "e": "value_e_new", - }, - "f": "value_f", - } - - result, err := MergeMaps(base, overlay) - require.NoError(t, err, "MergeMaps failed") - - // Check that base values are preserved - if result["a"] != "value_a" { - t.Errorf("Expected a='value_a', got '%v'", result["a"]) - } - - // Check deep merge - bMap, ok := result["b"].(map[string]interface{}) - if !ok { - t.Fatal("Expected b to be a map") - } - - if bMap["c"] != "value_c" { - t.Errorf("Expected b.c='value_c', got '%v'", bMap["c"]) - } - - if bMap["d"] != "value_d_override" { - t.Errorf("Expected b.d to be overridden to 'value_d_override', got '%v'", bMap["d"]) - } - - if bMap["e"] != "value_e_new" { - t.Errorf("Expected b.e='value_e_new', got '%v'", bMap["e"]) - } - - // Check new top-level key - if result["f"] != "value_f" { - t.Errorf("Expected f='value_f', got '%v'", result["f"]) - } -} - -func TestMergeMapsMultipleOverlays(t *testing.T) { - base := map[string]interface{}{ - "key": "base", - } - - overlay1 := map[string]interface{}{ - "key": "overlay1", - } - - overlay2 := map[string]interface{}{ - "key": "overlay2", - } - - result, err := MergeMaps(base, overlay1, overlay2) - require.NoError(t, err, "MergeMaps failed") - - if result["key"] != "overlay2" { - t.Errorf("Expected last overlay to win, got '%v'", result["key"]) - } -} - func TestLoadYAMLFileValid(t *testing.T) { tmpDir := t.TempDir() yamlPath := filepath.Join(tmpDir, "test.yaml") @@ -147,49 +75,6 @@ func TestLoadYAMLFileInvalidYAML(t *testing.T) { } } -func TestDeepMergeAppendsSlices(t *testing.T) { - base := map[string]interface{}{ - "spec": map[string]interface{}{ - "imagePullSecrets": []interface{}{ - map[string]interface{}{"name": "secret-a"}, - }, - "keep": "this", - }, - } - - overlay := map[string]interface{}{ - "spec": map[string]interface{}{ - "imagePullSecrets": []interface{}{ - map[string]interface{}{"name": "secret-b"}, - }, - }, - } - - err := DeepMerge(base, overlay) - require.NoError(t, err) - - spec := base["spec"].(map[string]interface{}) - require.Equal(t, "this", spec["keep"]) - - secrets := spec["imagePullSecrets"].([]interface{}) - require.Len(t, secrets, 2) - require.Equal(t, map[string]interface{}{"name": "secret-a"}, secrets[0]) - require.Equal(t, map[string]interface{}{"name": "secret-b"}, secrets[1]) -} - -func TestDeepMergeOverrideOnTypeMismatch(t *testing.T) { - base := map[string]interface{}{ - "items": []interface{}{"a"}, - } - overlay := map[string]interface{}{ - "items": "not-a-slice", - } - - err := DeepMerge(base, overlay) - require.NoError(t, err) - require.Equal(t, "not-a-slice", base["items"]) -} - func TestDeepCopy(t *testing.T) { original := map[string]interface{}{ "a": "value", @@ -198,7 +83,7 @@ func TestDeepCopy(t *testing.T) { }, } - copied := deepCopy(original) + copied := DeepCopyMap(original) // Modify the copy copied["a"] = "modified"