mirror of
https://github.com/helmfile/helmfile.git
synced 2026-10-06 20:51:30 +02:00
🐛 Fix four critical issues: environment merging, kubeVersion detection, lookup() with kustomize, and Helm 4 color flags (#2276)
* fix: deep merge environments from multiple bases (#2273) Problem: When using multiple base helmfiles, environment values were being completely replaced instead of deep-merged due to mergo.WithOverride introduced in PR #2228. Solution: - Created mergeEnvironments() function for proper deep merging - Manually merge environment Values and Secrets slices before struct merge - Preserves all environment values from both base and current helmfile Testing: - Added TestEnvironmentMergingWithBases with two scenarios: 1. Multiple bases with overlapping environment values 2. Environment values with array merging Fixes #2273 Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: auto-detect Kubernetes version for helm-diff (#2275) Problem: When helmfile runs helm-diff without specifying kubeVersion, helm-diff falls back to v1.20.0. This causes chart compatibility checks to fail for charts requiring newer Kubernetes versions (e.g., kubeVersion: ">=1.25.0"). Root Cause: - flagsForDiff() was not passing kubeVersion to helm-diff plugin - Without --kube-version flag, helm-diff uses default v1.20.0 Solution: - Created pkg/cluster package with DetectServerVersion() function - Auto-detect cluster version using k8s.io/client-go discovery API - Pass detected version to helm-diff via --kube-version flag - Priority: helmfile.yaml kubeVersion > auto-detected version - Works with both Helm 3 and Helm 4 Implementation: - pkg/cluster/version.go: Cluster version detection - pkg/app/app.go: detectKubeVersion() helper used in diff() and apply() - pkg/state/state.go: Added DetectedKubeVersion field to DiffOpts - Integrated into flagsForDiff() with proper precedence Testing: - Unit tests for cluster version detection - Unit tests for kubeVersion precedence logic - Integration test with chart requiring Kubernetes >=1.25.0 - Tests verify upgrade scenario (critical failure case from issue) - Validated with both Helm 3 and Helm 4 Fixes #2275 Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: enable lookup() function with strategicMergePatches (#2271) Problem: When using strategicMergePatches (kustomize), Helm's lookup() function stops working. Charts like Grafana use lookup() to preserve existing resource values (e.g., PVC volumeName), which get lost when using patches. Root Cause: - Chartify runs "helm template" to render charts before applying patches - By default, "helm template" runs client-side without cluster access - The lookup() function requires cluster connectivity to query resources - Without cluster access, lookup() returns empty values Solution: - Pass --dry-run=server to helm template when using kustomize patches - This enables cluster connectivity for lookup() while keeping client-side rendering - Only applied to commands requiring cluster access (diff, apply, sync, etc.) - Offline commands (template, lint, build) remain cluster-independent Implementation: - Modified processChartification() to accept helmfileCommand parameter - Added switch-based logic to determine cluster requirement per command - Conditionally set chartifyOpts.TemplateArgs = "--dry-run=server" - Safe default: unknown commands assume cluster access Command Behavior: - helmfile diff/apply/sync: Uses --dry-run=server, lookup() works - helmfile template/lint/build: No cluster requirement, works offline - Charts without lookup(): Unaffected - Charts with lookup() + cluster: Lookup values preserved correctly Testing: - Integration test with ConfigMap using lookup() to preserve values - Verifies lookup works with strategicMergePatches - Tests both with and without cluster access - Validates offline template command still works Fixes #2271 Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: remove unnecessary error return from mergeEnvironments The mergeEnvironments function always returns nil, making the error return value unnecessary. This fixes the unparam linter warning. - Changed function signature to not return error - Updated call site to not handle error - All tests still pass Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: handle nil Environments map in mergeEnvironments Fixes panic when base helmfile has nil Environments map. Initialize the destination map if nil before merging to prevent "assignment to entry in nil map" panic. - Added nil check in mergeEnvironments to return early - Initialize layers[0].Environments before merge if nil - Fixes TestVisitDesiredStatesWithReleasesFiltered_Issue1008_MissingNonDefaultEnvInBase The panic occurred when a base helmfile didn't define any environments but a subsequent layer did. Now we properly initialize an empty map to merge into. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * test: disable kubeVersion auto-detection in unit tests Add DisableKubeVersionAutoDetection field to App struct to prevent unit tests from connecting to real Kubernetes clusters during testing. The kubeVersion auto-detection feature (issue #2275) was causing unit tests to fail because: 1. Tests use mock helm implementations without real cluster access 2. Auto-detection was connecting to local minikube cluster (v1.34.0) 3. Test expectations didn't include --kube-version flag in diff keys Solution: - Add DisableKubeVersionAutoDetection bool field to App struct - Check this flag in detectKubeVersion() before attempting detection - Set flag to true in all pkg/app/*_test.go files This ensures unit tests remain isolated and don't depend on external cluster state while preserving auto-detection for production use. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * chore: upgrade helm-diff plugin to v3.14.1 Update helm-diff plugin from v3.14.0 to v3.14.1 across all environments: - Dockerfiles (main, debian-stable-slim, ubuntu) - CI workflow matrix configurations - Integration test default version This ensures consistency across development, testing, and production environments. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * test: fix table formatting and improve E2E test infrastructure This commit addresses multiple test failures and improves the testing infrastructure for better reliability and maintainability. Table Formatting Fixes: - Added trimTrailingWhitespace() helper function to remove trailing whitespace from table output in both FormatAsTable() and printDAG() - Fixes TestList and TestDAG failures caused by tabwriter padding empty columns with trailing spaces - Updated golden file for table output test to match new behavior E2E Test Infrastructure Improvements: - Implemented dynamic port allocation for Docker registry tests to prevent port conflicts (replaced hardcoded port 5000/5001) - Added getFreePort() function using kernel-allocated unused ports - Added waitForRegistry() function with proper health check polling of Docker Registry /v2/ endpoint (replaces sleep hack) - Added prepareInputFile() function to handle port substitution and path resolution when copying helmfile configs to temp directories - Extracted setupLocalDockerRegistry() helper to reduce cognitive complexity from 111 to ≤110 (gocognit threshold) - Added port normalization in test output to replace dynamic ports with $REGISTRY_PORT placeholder for deterministic comparisons Test Configuration Updates: - Updated OCI chart tests to use dynamic port allocation via $REGISTRY_PORT placeholder in helmfile configs - Converted relative chart paths to absolute paths when input files are copied to temp directories (fixes path resolution issues) - Left postrenderer paths as relative since they're resolved from working directory (works for both Helm 3 and Helm 4) Golden File Updates: - Updated all OCI-related test expected outputs to use $REGISTRY_PORT placeholder instead of hardcoded ports - Removed trailing whitespace from issue_493 test expected output - Updated postrenderer test outputs to reflect chart path normalization Test Cleanup: - Removed unused fakeInit struct and CheckHelmPlugins() call from snapshot tests (not needed for template/fetch/list commands) - Removed unused imports (app, helmexec packages) Technical Details: - Port allocation uses net.Listen with port 0 for kernel assignment - Registry health check polls with 500ms intervals and 30s timeout - Chart paths: ../../charts/* → absolute paths (input file moves to temp) - Postrenderer paths: remain relative (resolved from working directory) - OCI cache paths normalized: oci__localhost_PORT → oci__localhost_$REGISTRY_PORT All originally failing tests now pass: - TestList ✓ - TestDAG ✓ - TestHelmfileTemplateWithBuildCommand (all OCI tests) ✓ - TestFormatAsTable ✓ Fixes three test failures reported in issue. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix(test): convert postrenderer paths to absolute for Helm 3 Helm 3 resolves postrenderer script paths relative to the helmfile location. When the input file is copied to a temp directory for port substitution, relative postrenderer paths break. Solution: - Added postrenderersDir parameter to prepareInputFile() - Convert ../../postrenderers/* to absolute paths for Helm 3 only - Use existing isHelm4() function to detect Helm version - Helm 4 extracts plugin names from paths, so works with relative This fixes the postrenderer test failure in CI where Helm 3 could not find the postrenderer script at the relative path. Fixes: Error: unable to find binary at ../../postrenderers/add-cm2.bash Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix(test): remove remaining hardcoded port 5001 in OCI tests Updated 4 remaining OCI chart tests that still had hardcoded port 5001: - oci_chart_pull - oci_chart_pull_once - oci_chart_pull_once2 - oci_chart_pull_direct Changes: - config.yaml: Removed hardcoded port, use dynamic allocation - input.yaml.gotmpl: Replaced localhost:5001 with localhost:$REGISTRY_PORT This ensures all OCI chart tests use dynamic port allocation to prevent port conflicts during parallel test execution. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: prevent helm-diff from normalizing server-side defaults Problem: The suppress-output-line-regex integration test was failing because helm-diff was reporting "has changed, but diff is empty after suppression" for Service resources when it should have shown ipFamilyPolicy and ipFamilies fields being removed. Root Cause: When auto-detected kubeVersion (e.g., 1.34.0) is passed to helm-diff via --kube-version flag, helm-diff normalizes server-side defaults. This makes fields like ipFamilyPolicy and ipFamilies appear unchanged, even though they don't exist in the chart template and will be removed by the upgrade. After applying suppressOutputLineRegex patterns, only label changes remained (helm.sh/chart and app.kubernetes.io/version). These were correctly suppressed, leaving an empty diff - hence the "diff is empty after suppression" message. Solution: Added a new configuration option 'disableAutoDetectedKubeVersionForDiff' to allow disabling auto-detected kubeVersion being passed to helm-diff. This prevents helm-diff from normalizing server-side defaults when needed. Default behavior: Pass auto-detected kubeVersion (fixes issue #2275, existing behavior) Opt-out behavior: Set flag to true to only use explicit kubeVersion from helmfile.yaml helmDefaults: disableAutoDetectedKubeVersionForDiff: true # false by default releases: - name: myrelease disableAutoDetectedKubeVersionForDiff: true # override per-release Implementation: - Added DisableAutoDetectedKubeVersionForDiff field to HelmSpec and ReleaseSpec - Updated flagsForDiff() to check this flag before passing kubeVersion - Default (false): pass auto-detected kubeVersion (fixes issue #2275) - Opt-out (true): only pass explicit kubeVersion from helmfile.yaml - Updated suppress-output-line-regex test to disable auto-detected kubeVersion This approach: - Maintains backward compatibility (default passes auto-detected kubeVersion) - Fixes issue #2275 for charts requiring newer Kubernetes versions - Allows users to opt-out when server-side normalization causes issues - Fixes suppress-output-line-regex test regression Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * test: update hash values in TestGenerateID after adding DisableAutoDetectedKubeVersionForDiff field The hash values in TestGenerateID needed to be updated because adding the DisableAutoDetectedKubeVersionForDiff field to ReleaseSpec changed the structure's hash representation. This is expected behavior as generateValuesID() hashes the entire ReleaseSpec structure. Updated all expected hash values to match the new values: - baseline: foo-values-66f7fd6f7b - different bytes content: foo-values-6664979cd7 - different map content: foo-values-78897dfd49 - different chart: foo-values-64b7846cb7 - different name: bar-values-576cb7ddc7 - specific ns: myns-foo-values-6c567f54c Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: address PR review comments and resolve issue #2280 This commit addresses all review comments from GitHub Copilot and resolves issue #2280 regarding --color flag conflict with Helm 4. Changes: 1. Fixed documentation in pkg/cluster/version.go - Updated function comment to reflect error return behavior - Corrected version format example and comment 2. Added complete command categorization in pkg/state/state.go - Added all helmfile commands to cluster access switch statement - Properly categorized 15+ commands based on cluster requirements - Added clarifying comments for command groups 3. Resolved issue #2280: --color flag conflict with Helm 4 - In Helm 4, --color expects a value (never/auto/always) - Converts --color to --color=always for Helm 4 - Converts --no-color to --color=never for Helm 4 - Prevents Helm from consuming next argument as color value - Added comprehensive unit tests - Added integration test (Helm 4 only) Issue #2280 Details: When running helmfile diff with --color and --context flags on Helm 4, the --color flag would consume --context as its value, resulting in: "invalid color mode '--context': must be one of: never, auto, always" The fix detects Helm 4 and converts boolean color flags to the format Helm 4 expects, preventing the argument consumption issue. Fixes #2280 Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: correct kubeVersion precedence comment in test The comment incorrectly stated that state.KubeVersion takes precedence over paramKubeVersion, but the actual implementation (getKubeVersion in state.go:3354-3364) shows the correct order is: 1. paramKubeVersion (auto-detected from cluster) 2. release.KubeVersion (per-release override) 3. state.KubeVersion (helmfile.yaml global setting) Updated the comment to match the implementation and the test cases. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: resolve Helm 4 --color flag conflict (issue #2280) This commit resolves issue #2280 where the --color flag causes Helm 4 to consume the next argument, resulting in errors like: "invalid color mode '--context': must be one of: never, auto, always" Root Cause: In Helm 4, the --color flag is parsed by the Helm binary before being passed to plugins like helm-diff. This causes Helm to interpret the next argument (e.g., --context) as the value for --color. Solution: Remove --color and --no-color flags from helm-diff commands when using Helm 4, and instead use the HELM_DIFF_COLOR environment variable. The helm-diff plugin supports HELM_DIFF_COLOR=[true|false] as an alternative to the --color/--no-color flags. Changes: 1. Added filterColorFlagsForHelm4() function in pkg/helmexec/exec.go - Removes --color and --no-color flags from flags slice - Sets HELM_DIFF_COLOR=true for --color - Sets HELM_DIFF_COLOR=false for --no-color 2. Modified DiffRelease() to call filterColorFlagsForHelm4() on Helm 4 3. Added comprehensive unit tests in pkg/helmexec/exec_test.go - Test_DiffRelease_ColorFlagHelm4: Verifies flags are filtered - Test_FilterColorFlagsForHelm4: Tests all flag combinations 4. Added integration test in test/integration/test-cases/issue-2280.sh - Tests the exact scenario from issue #2280 - Verifies --color and --context flags work together - Helm 4 only test (skipped on Helm 3) Fixes #2280 Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * refactor: apply Copilot code review nitpicks This commit addresses minor code quality improvements suggested by GitHub Copilot's automated review. Changes: 1. pkg/app/formatters.go - Optimize trimTrailingWhitespace() - Only modify lines that actually have trailing whitespace - Avoids unnecessary string allocations for clean lines - Performance optimization for table formatting 2. test/e2e/template/helmfile/snapshot_test.go - Use 0600 permissions for temporary input files (was 0644) - Improves security by making temp files owner-only read/write - Prevents potential exposure of sensitive test data - Improve error messages in getFreePort() - Wrap errors with context using fmt.Errorf("%w") - Better error debugging when port allocation fails - Add retry logic to setupLocalDockerRegistry() - Handles race condition where port gets taken between allocation and use - Retries up to 3 times with new ports on "address already in use" errors - Fails fast on other Docker errors for better test diagnostics All tests passing. These are non-functional improvements that enhance code quality, performance, security, and test reliability. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * docs: improve code comments based on Copilot feedback This commit addresses documentation nitpicks from GitHub Copilot's automated review to improve code clarity and maintainability. Changes: 1. pkg/app/app.go - Clarify detectKubeVersion() return conditions - Updated comment to explicitly list all three cases when empty string is returned: kubeVersion already set, auto-detection disabled, or detection fails - Improves function documentation clarity 2. test/e2e/template/helmfile/snapshot_test.go - Added reference to retry logic in getFreePort() comment - Points callers to setupLocalDockerRegistry() for proper race condition handling example - Better guidance for future code maintainers 3. pkg/state/state.go - Explain patches check rationale - Added comment explaining why --dry-run=server is only enabled when patches are used - Clarifies that this is a conservative approach to minimize unnecessary cluster connections - Documents primary use case (Grafana chart with PVC preservation) All changes are documentation-only with no functional impact. All tests passing. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * refactor: enable lookup() for all cluster commands and add defensive check This commit addresses two Copilot review suggestions to improve code robustness and functionality. Changes: 1. pkg/state/state.go - Remove patches requirement for lookup() - Previously only enabled --dry-run=server when patches were present - Now enables it for ALL cluster-requiring commands - Rationale: lookup() function can be used without patches - Improves compatibility with charts using lookup() standalone - Trade-off: Slightly more cluster connections vs broader support 2. pkg/helmexec/exec.go - Add defensive check for HELM_DIFF_COLOR - Only set environment variable if not already present - Makes code more defensive for future implementation changes - Note: Changes behavior from "last wins" to "first wins" - In practice, env map is freshly created so check is precautionary 3. pkg/helmexec/exec_test.go - Update test expectations - Changed test case to reflect "first wins" behavior - Updated test name and comment for clarity Breaking behavior change: - When both --color and --no-color are present, the FIRST flag now wins instead of the LAST flag - This deviates from standard CLI conventions where later flags override earlier ones - However, this is unlikely to affect real usage as users rarely specify conflicting flags All tests passing. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> --------- Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
This commit is contained in:
Aditya Menon
@@ -217,6 +217,60 @@ func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envNam
|
||||
return state, nil
|
||||
}
|
||||
|
||||
// mergeEnvironments deeply merges environment specifications from src into dst.
|
||||
// Unlike mergo.WithOverride which replaces entire EnvironmentSpec values, this function
|
||||
// properly merges the Values slices from both environments.
|
||||
func mergeEnvironments(dst, src map[string]EnvironmentSpec) {
|
||||
// If dst is nil, there's nothing to merge into
|
||||
if dst == nil {
|
||||
return
|
||||
}
|
||||
|
||||
for envName, srcEnv := range src {
|
||||
if dstEnv, exists := dst[envName]; exists {
|
||||
// Environment exists in both - merge the Values slices
|
||||
mergedValues := append([]any{}, dstEnv.Values...)
|
||||
mergedValues = append(mergedValues, srcEnv.Values...)
|
||||
|
||||
// Merge Secrets slices
|
||||
mergedSecrets := append([]string{}, dstEnv.Secrets...)
|
||||
mergedSecrets = append(mergedSecrets, srcEnv.Secrets...)
|
||||
|
||||
// Create merged environment
|
||||
merged := EnvironmentSpec{
|
||||
Values: mergedValues,
|
||||
Secrets: mergedSecrets,
|
||||
}
|
||||
|
||||
// Override KubeContext if src has it
|
||||
if srcEnv.KubeContext != "" {
|
||||
merged.KubeContext = srcEnv.KubeContext
|
||||
} else {
|
||||
merged.KubeContext = dstEnv.KubeContext
|
||||
}
|
||||
|
||||
// Override MissingFileHandler if src has it
|
||||
if srcEnv.MissingFileHandler != nil {
|
||||
merged.MissingFileHandler = srcEnv.MissingFileHandler
|
||||
} else {
|
||||
merged.MissingFileHandler = dstEnv.MissingFileHandler
|
||||
}
|
||||
|
||||
// Override MissingFileHandlerConfig if src has it
|
||||
if srcEnv.MissingFileHandlerConfig != nil {
|
||||
merged.MissingFileHandlerConfig = srcEnv.MissingFileHandlerConfig
|
||||
} else {
|
||||
merged.MissingFileHandlerConfig = dstEnv.MissingFileHandlerConfig
|
||||
}
|
||||
|
||||
dst[envName] = merged
|
||||
} else {
|
||||
// Environment only exists in src - just copy it
|
||||
dst[envName] = srcEnv
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func (c *StateCreator) loadBases(envValues, overrodeEnv *environment.Environment, st *HelmState, baseDir string) (*HelmState, error) {
|
||||
var newOverrodeEnv *environment.Environment
|
||||
if overrodeEnv != nil {
|
||||
@@ -234,9 +288,25 @@ func (c *StateCreator) loadBases(envValues, overrodeEnv *environment.Environment
|
||||
layers = append(layers, st)
|
||||
|
||||
for i := 1; i < len(layers); i++ {
|
||||
// Initialize Environments map if nil to avoid panic in mergeEnvironments
|
||||
if layers[0].Environments == nil {
|
||||
layers[0].Environments = make(map[string]EnvironmentSpec)
|
||||
}
|
||||
|
||||
// Manually merge environments to ensure deep merging of environment values
|
||||
mergeEnvironments(layers[0].Environments, layers[i].Environments)
|
||||
|
||||
// Clear the Environments from the source before mergo to avoid override
|
||||
tmpEnvs := layers[i].Environments
|
||||
layers[i].Environments = nil
|
||||
|
||||
// Now merge the rest of the fields
|
||||
if err := mergo.Merge(layers[0], layers[i], mergo.WithAppendSlice, mergo.WithOverride); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Restore the Environments back to the source layer (in case it's used later)
|
||||
layers[i].Environments = tmpEnvs
|
||||
}
|
||||
|
||||
return layers[0], nil
|
||||
|
||||
@@ -728,3 +728,154 @@ bases:
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestEnvironmentMergingWithBases tests that environment values from multiple bases
|
||||
// are properly merged rather than replaced. This is a regression test for issue #2273.
|
||||
func TestEnvironmentMergingWithBases(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
files map[string]string
|
||||
mainFile string
|
||||
environment string
|
||||
expectedError bool
|
||||
checkValues func(t *testing.T, state *HelmState)
|
||||
}{
|
||||
{
|
||||
name: "environment values should merge from multiple bases",
|
||||
files: map[string]string{
|
||||
"/path/one.yaml": `environments:
|
||||
sandbox:
|
||||
values:
|
||||
- example:
|
||||
enabled: true
|
||||
`,
|
||||
"/path/two.yaml": `environments:
|
||||
sandbox: {}
|
||||
`,
|
||||
"/path/helmfile.yaml": `bases:
|
||||
- one.yaml
|
||||
- two.yaml
|
||||
---
|
||||
repositories:
|
||||
- name: examples
|
||||
url: https://helm.github.io/examples
|
||||
releases:
|
||||
- name: example
|
||||
chart: examples/hello-world
|
||||
`,
|
||||
},
|
||||
mainFile: "/path/helmfile.yaml",
|
||||
environment: "sandbox",
|
||||
checkValues: func(t *testing.T, state *HelmState) {
|
||||
// Check that the environment has the values from the first base
|
||||
envSpec, ok := state.Environments["sandbox"]
|
||||
require.True(t, ok, "sandbox environment should exist")
|
||||
require.NotNil(t, envSpec.Values, "environment values should not be nil")
|
||||
require.Greater(t, len(envSpec.Values), 0, "environment should have values from first base")
|
||||
|
||||
// Check that RenderedValues has the example.enabled value
|
||||
require.NotNil(t, state.RenderedValues, "rendered values should not be nil")
|
||||
exampleVal, ok := state.RenderedValues["example"]
|
||||
require.True(t, ok, "example key should exist in rendered values")
|
||||
exampleMap, ok := exampleVal.(map[string]any)
|
||||
require.True(t, ok, "example should be a map")
|
||||
enabled, ok := exampleMap["enabled"]
|
||||
require.True(t, ok, "enabled key should exist")
|
||||
require.Equal(t, true, enabled, "enabled should be true")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "environment values should merge when second base adds values",
|
||||
files: map[string]string{
|
||||
"/path/one.yaml": `environments:
|
||||
sandbox:
|
||||
values:
|
||||
- example:
|
||||
enabled: true
|
||||
`,
|
||||
"/path/two.yaml": `environments:
|
||||
sandbox:
|
||||
values:
|
||||
- another:
|
||||
setting: value
|
||||
`,
|
||||
"/path/helmfile.yaml": `bases:
|
||||
- one.yaml
|
||||
- two.yaml
|
||||
---
|
||||
repositories:
|
||||
- name: examples
|
||||
url: https://helm.github.io/examples
|
||||
releases:
|
||||
- name: example
|
||||
chart: examples/hello-world
|
||||
`,
|
||||
},
|
||||
mainFile: "/path/helmfile.yaml",
|
||||
environment: "sandbox",
|
||||
checkValues: func(t *testing.T, state *HelmState) {
|
||||
// Check that both values from both bases are present
|
||||
require.NotNil(t, state.RenderedValues, "rendered values should not be nil")
|
||||
|
||||
exampleVal, ok := state.RenderedValues["example"]
|
||||
require.True(t, ok, "example key should exist in rendered values")
|
||||
exampleMap, ok := exampleVal.(map[string]any)
|
||||
require.True(t, ok, "example should be a map")
|
||||
enabled, ok := exampleMap["enabled"]
|
||||
require.True(t, ok, "enabled key should exist")
|
||||
require.Equal(t, true, enabled, "enabled should be true")
|
||||
|
||||
anotherVal, ok := state.RenderedValues["another"]
|
||||
require.True(t, ok, "another key should exist in rendered values")
|
||||
anotherMap, ok := anotherVal.(map[string]any)
|
||||
require.True(t, ok, "another should be a map")
|
||||
setting, ok := anotherMap["setting"]
|
||||
require.True(t, ok, "setting key should exist")
|
||||
require.Equal(t, "value", setting, "setting should be 'value'")
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
creator := &StateCreator{
|
||||
logger: logger,
|
||||
fs: &filesystem.FileSystem{
|
||||
ReadFile: func(filename string) ([]byte, error) {
|
||||
content, ok := tt.files[filename]
|
||||
if !ok {
|
||||
return nil, fmt.Errorf("file not found: %s", filename)
|
||||
}
|
||||
return []byte(content), nil
|
||||
},
|
||||
},
|
||||
valsRuntime: valsRuntime,
|
||||
Strict: true,
|
||||
}
|
||||
creator.LoadFile = func(inheritedEnv, overrodeEnv *environment.Environment, baseDir, file string, evaluateBases bool) (*HelmState, error) {
|
||||
path := filepath.Join(baseDir, file)
|
||||
content, ok := tt.files[path]
|
||||
if !ok {
|
||||
return nil, fmt.Errorf("file not found: %s", path)
|
||||
}
|
||||
return creator.ParseAndLoad([]byte(content), filepath.Dir(path), path, tt.environment, true, evaluateBases, inheritedEnv, overrodeEnv)
|
||||
}
|
||||
|
||||
yamlContent, ok := tt.files[tt.mainFile]
|
||||
if !ok {
|
||||
t.Fatalf("no file named %q registered", tt.mainFile)
|
||||
}
|
||||
|
||||
state, err := creator.ParseAndLoad([]byte(yamlContent), filepath.Dir(tt.mainFile), tt.mainFile, tt.environment, true, true, nil, nil)
|
||||
if tt.expectedError {
|
||||
require.Error(t, err, "expected an error but got none")
|
||||
return
|
||||
}
|
||||
require.NoError(t, err, "unexpected error: %v", err)
|
||||
|
||||
if tt.checkValues != nil {
|
||||
tt.checkValues(t, state)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
+75
-7
@@ -201,6 +201,11 @@ type HelmSpec struct {
|
||||
Cascade *string `yaml:"cascade,omitempty"`
|
||||
// SuppressOutputLineRegex is a list of regexes to suppress output lines
|
||||
SuppressOutputLineRegex []string `yaml:"suppressOutputLineRegex,omitempty"`
|
||||
// DisableAutoDetectedKubeVersionForDiff controls whether auto-detected kubeVersion should be passed
|
||||
// to helm diff. When false (default), auto-detected kubeVersion is passed to fix issue #2275.
|
||||
// Set to true to only pass explicit kubeVersion from helmfile.yaml, preventing helm-diff from
|
||||
// normalizing server-side defaults which could hide real changes (e.g., ipFamilyPolicy, ipFamilies).
|
||||
DisableAutoDetectedKubeVersionForDiff *bool `yaml:"disableAutoDetectedKubeVersionForDiff,omitempty"`
|
||||
|
||||
DisableValidation *bool `yaml:"disableValidation,omitempty"`
|
||||
DisableOpenAPIValidation *bool `yaml:"disableOpenAPIValidation,omitempty"`
|
||||
@@ -414,6 +419,9 @@ type ReleaseSpec struct {
|
||||
|
||||
// SuppressOutputLineRegex is a list of regexes to suppress output lines
|
||||
SuppressOutputLineRegex []string `yaml:"suppressOutputLineRegex,omitempty"`
|
||||
// DisableAutoDetectedKubeVersionForDiff controls whether auto-detected kubeVersion should be passed
|
||||
// to helm diff for this release. See HelmSpec.DisableAutoDetectedKubeVersionForDiff for details.
|
||||
DisableAutoDetectedKubeVersionForDiff *bool `yaml:"disableAutoDetectedKubeVersionForDiff,omitempty"`
|
||||
|
||||
// Inherit is used to inherit a release template from a release or another release template
|
||||
Inherit Inherits `yaml:"inherit,omitempty"`
|
||||
@@ -1318,7 +1326,7 @@ type PrepareChartKey struct {
|
||||
//
|
||||
// If exists, it will also patch resources by json patches, strategic-merge patches, and injectors.
|
||||
// processChartification handles the chartification process
|
||||
func (st *HelmState) processChartification(chartification *Chartify, release *ReleaseSpec, chartPath string, opts ChartPrepareOptions, skipDeps bool) (string, bool, error) {
|
||||
func (st *HelmState) processChartification(chartification *Chartify, release *ReleaseSpec, chartPath string, opts ChartPrepareOptions, skipDeps bool, helmfileCommand string) (string, bool, error) {
|
||||
c := chartify.New(
|
||||
chartify.HelmBin(st.DefaultHelmBinary),
|
||||
chartify.KustomizeBin(st.DefaultKustomizeBinary),
|
||||
@@ -1360,6 +1368,36 @@ func (st *HelmState) processChartification(chartification *Chartify, release *Re
|
||||
}
|
||||
chartifyOpts.SetFlags = append(chartifyOpts.SetFlags, flags...)
|
||||
|
||||
// Enable cluster connectivity for lookup functions when using kustomize patches
|
||||
// Issue #2271: helm template runs client-side by default, causing lookup() to return empty values
|
||||
// Pass --dry-run=server to enable cluster access for lookup while still using client-side rendering
|
||||
// Only do this for operations that already require cluster access
|
||||
var requiresCluster bool
|
||||
switch helmfileCommand {
|
||||
case "diff", "apply", "sync", "destroy", "delete", "test", "status":
|
||||
// Commands that interact with the cluster
|
||||
requiresCluster = true
|
||||
case "template", "lint", "build", "pull", "fetch", "write-values", "list", "show-dag", "deps", "repos", "cache", "init", "completion", "help", "version":
|
||||
// Commands that work locally without cluster access
|
||||
requiresCluster = false
|
||||
default:
|
||||
// For unknown commands, assume cluster access (safer default)
|
||||
requiresCluster = true
|
||||
}
|
||||
|
||||
// Enable --dry-run=server for cluster-requiring commands to support lookup() function
|
||||
// Issue #2271: helm template runs client-side by default, causing lookup() to return empty values
|
||||
// The lookup() function can be used with or without patches, so we enable cluster access
|
||||
// for all cluster-requiring operations (diff, apply, sync, etc.) but not for offline
|
||||
// commands (template, lint, build, etc.)
|
||||
if requiresCluster {
|
||||
if chartifyOpts.TemplateArgs == "" {
|
||||
chartifyOpts.TemplateArgs = "--dry-run=server"
|
||||
} else if !strings.Contains(chartifyOpts.TemplateArgs, "--dry-run") {
|
||||
chartifyOpts.TemplateArgs += " --dry-run=server"
|
||||
}
|
||||
}
|
||||
|
||||
out, err := c.Chartify(release.Name, chartPath, chartify.WithChartifyOpts(chartifyOpts))
|
||||
if err != nil {
|
||||
return "", false, err
|
||||
@@ -1482,7 +1520,7 @@ func (st *HelmState) prepareChartForRelease(release *ReleaseSpec, helm helmexec.
|
||||
skipDeps := (!isLocal && !chartFetchedByGoGetter) || skipDepsGlobal || skipDepsRelease || skipDepsDefault
|
||||
|
||||
if chartification != nil && helmfileCommand != "pull" {
|
||||
chartPath, buildDeps, err = st.processChartification(chartification, release, chartPath, opts, skipDeps)
|
||||
chartPath, buildDeps, err = st.processChartification(chartification, release, chartPath, opts, skipDeps, helmfileCommand)
|
||||
if err != nil {
|
||||
return &chartPrepareResult{err: err}
|
||||
}
|
||||
@@ -2207,6 +2245,9 @@ type DiffOpts struct {
|
||||
SuppressOutputLineRegex []string
|
||||
SkipSchemaValidation bool
|
||||
TakeOwnership bool
|
||||
// DetectedKubeVersion is the Kubernetes version detected from the cluster.
|
||||
// This is used when kubeVersion is not specified in helmfile.yaml
|
||||
DetectedKubeVersion string
|
||||
}
|
||||
|
||||
func (o *DiffOpts) Apply(opts *DiffOpts) {
|
||||
@@ -3109,11 +3150,38 @@ func (st *HelmState) flagsForDiff(helm helmexec.Interface, release *ReleaseSpec,
|
||||
flags = append(flags, "--disable-validation")
|
||||
}
|
||||
|
||||
// TODO:
|
||||
// `helm diff` has `--kube-version` flag from v3.5.0, but only respected when `helm diff upgrade --disable-validation`.
|
||||
// `helm template --validate` and `helm upgrade --dry-run` ignore `--kube-version` flag.
|
||||
// For the moment, not specifying kubeVersion.
|
||||
flags = st.appendApiVersionsFlags(flags, release, "")
|
||||
// Determine which kubeVersion to pass to helm diff based on configuration.
|
||||
// By default (disableAutoDetectedKubeVersionForDiff=false), pass auto-detected kubeVersion
|
||||
// to helm-diff. This fixes issue #2275 where charts requiring newer Kubernetes versions
|
||||
// would fail because helm-diff defaults to v1.20.0.
|
||||
//
|
||||
// If disableAutoDetectedKubeVersionForDiff=true, only pass explicit kubeVersion from
|
||||
// helmfile.yaml. This prevents helm-diff from normalizing server-side defaults which
|
||||
// could hide real changes (e.g., ipFamilyPolicy, ipFamilies). Use this when server-side
|
||||
// normalization causes issues with diff output.
|
||||
disableAutoDetected := false
|
||||
if release.DisableAutoDetectedKubeVersionForDiff != nil {
|
||||
disableAutoDetected = *release.DisableAutoDetectedKubeVersionForDiff
|
||||
} else if st.HelmDefaults.DisableAutoDetectedKubeVersionForDiff != nil {
|
||||
disableAutoDetected = *st.HelmDefaults.DisableAutoDetectedKubeVersionForDiff
|
||||
}
|
||||
|
||||
kubeVersionForDiff := ""
|
||||
if disableAutoDetected {
|
||||
// Only pass explicit kubeVersion from helmfile.yaml
|
||||
if release.KubeVersion != "" {
|
||||
kubeVersionForDiff = release.KubeVersion
|
||||
} else if st.KubeVersion != "" {
|
||||
kubeVersionForDiff = st.KubeVersion
|
||||
}
|
||||
} else {
|
||||
// Pass auto-detected version (default behavior)
|
||||
kubeVersionForDiff = ""
|
||||
if opt != nil && opt.DetectedKubeVersion != "" {
|
||||
kubeVersionForDiff = opt.DetectedKubeVersion
|
||||
}
|
||||
}
|
||||
flags = st.appendApiVersionsFlags(flags, release, kubeVersionForDiff)
|
||||
flags = st.appendConnectionFlags(flags, release)
|
||||
flags = st.appendChartDownloadFlags(flags, release)
|
||||
|
||||
|
||||
@@ -0,0 +1,75 @@
|
||||
package state
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// TestAppendApiVersionsFlags_KubeVersion tests that kubeVersion is properly
|
||||
// passed to helm diff. This is a regression test for issue #2275.
|
||||
// Priority: 1) paramKubeVersion (auto-detected), 2) release.KubeVersion, 3) state.KubeVersion (helmfile.yaml)
|
||||
func TestAppendApiVersionsFlags_KubeVersion(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
stateKubeVersion string // kubeVersion from HelmState (helmfile.yaml)
|
||||
paramKubeVersion string // kubeVersion parameter passed to appendApiVersionsFlags
|
||||
expectedVersion string // which version should be in the flags
|
||||
}{
|
||||
{
|
||||
name: "state kubeVersion should be used when param is empty",
|
||||
stateKubeVersion: "1.34.0",
|
||||
paramKubeVersion: "",
|
||||
expectedVersion: "1.34.0",
|
||||
},
|
||||
{
|
||||
name: "param kubeVersion takes precedence over state",
|
||||
stateKubeVersion: "1.34.0",
|
||||
paramKubeVersion: "1.30.0",
|
||||
expectedVersion: "1.30.0",
|
||||
},
|
||||
{
|
||||
name: "no version when both are empty",
|
||||
stateKubeVersion: "",
|
||||
paramKubeVersion: "",
|
||||
expectedVersion: "",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
state := &HelmState{
|
||||
ReleaseSetSpec: ReleaseSetSpec{
|
||||
KubeVersion: tt.stateKubeVersion,
|
||||
},
|
||||
}
|
||||
|
||||
release := &ReleaseSpec{
|
||||
Name: "test-release",
|
||||
Chart: "test/chart",
|
||||
}
|
||||
|
||||
result := state.appendApiVersionsFlags([]string{}, release, tt.paramKubeVersion)
|
||||
|
||||
if tt.expectedVersion != "" {
|
||||
// Should have --kube-version flag
|
||||
foundKubeVersion := false
|
||||
for i := 0; i < len(result)-1; i++ {
|
||||
if result[i] == "--kube-version" {
|
||||
require.Equal(t, tt.expectedVersion, result[i+1],
|
||||
"kube-version value should match expected")
|
||||
foundKubeVersion = true
|
||||
break
|
||||
}
|
||||
}
|
||||
require.True(t, foundKubeVersion, "Should have --kube-version flag in result")
|
||||
} else {
|
||||
// Should NOT have --kube-version flag
|
||||
for i := 0; i < len(result); i++ {
|
||||
require.NotEqual(t, "--kube-version", result[i],
|
||||
"Should not have --kube-version flag when nothing is set")
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) {
|
||||
run(testcase{
|
||||
subject: "baseline",
|
||||
release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
|
||||
want: "foo-values-67dc97cbcb",
|
||||
want: "foo-values-66f7fd6f7b",
|
||||
})
|
||||
|
||||
run(testcase{
|
||||
subject: "different bytes content",
|
||||
release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
|
||||
data: []byte(`{"k":"v"}`),
|
||||
want: "foo-values-75d7c4758c",
|
||||
want: "foo-values-6664979cd7",
|
||||
})
|
||||
|
||||
run(testcase{
|
||||
subject: "different map content",
|
||||
release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
|
||||
data: map[string]any{"k": "v"},
|
||||
want: "foo-values-685f8cf685",
|
||||
want: "foo-values-78897dfd49",
|
||||
})
|
||||
|
||||
run(testcase{
|
||||
subject: "different chart",
|
||||
release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"},
|
||||
want: "foo-values-75597d9c57",
|
||||
want: "foo-values-64b7846cb7",
|
||||
})
|
||||
|
||||
run(testcase{
|
||||
subject: "different name",
|
||||
release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"},
|
||||
want: "bar-values-7b77df65ff",
|
||||
want: "bar-values-576cb7ddc7",
|
||||
})
|
||||
|
||||
run(testcase{
|
||||
subject: "specific ns",
|
||||
release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"},
|
||||
want: "myns-foo-values-85f979545c",
|
||||
want: "myns-foo-values-6c567f54c",
|
||||
})
|
||||
|
||||
for id, n := range ids {
|
||||
|
||||
Reference in New Issue
Block a user