Skip to content

Commit f60c1a7

Browse files
yxxheroyxxhero
authored
fix: use --dry-run=server by default for Helm v4 to fix .Capabilities.APIVersions (#928)
* fix: use --dry-run=server by default for Helm v4 to fix .Capabilities.APIVersions For Helm v4, --dry-run=server should be used by default to get the correct .Capabilities.APIVersions.Has behavior. This is only applied when the user hasn't explicitly set --dry-run=client. Fixes #894 Signed-off-by: yxxhero <aiopsclub@163.com> * add debug flag Signed-off-by: yxxhero <aiopsclub@163.com> * test: add tests for Helm v4 --dry-run=server default behavior Add unit tests for PR #928 to verify the dry-run flag behavior for Helm v4, which uses --dry-run=server by default to get correct .Capabilities.APIVersions. Signed-off-by: yxxhero <aiopsclub@163.com> * fix: handle dryRunMode true/false correctly and address review comments - Fix conditions to properly handle dryRunMode="true" (behaves like client) - Fix conditions to properly handle dryRunMode="false" (behaves like none) - Add test cases for boolean dry-run modes - Remove leftover HELM_DEBUG from CI workflow Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: yxxhero <aiopsclub@163.com>
1 parent a2c34ad commit f60c1a7

4 files changed

Lines changed: 284 additions & 43 deletions

File tree

‎cmd/helm.go‎

Lines changed: 37 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"os"
99
"os/exec"
1010
"regexp"
11+
"slices"
1112
"strconv"
1213
"strings"
1314

@@ -331,8 +332,14 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
331332
if !d.disableValidation && d.clusterAccessAllowed() {
332333
isHelmV4, err := isHelmVersionGreaterThanEqual(helmV4Version)
333334
if err == nil && isHelmV4 {
334-
// Flag --validate has been deprecated, use '--dry-run=server' instead in Helm v4+
335-
flags = append(flags, "--dry-run=server")
335+
// For Helm v4, we use --dry-run=server by default to get correct .Capabilities.APIVersions.
336+
// This is only applied if the user hasn't explicitly set --dry-run=client, --dry-run=true, or --dry-run=false.
337+
// Note: dryRunMode="true" behaves like "client" (no cluster access).
338+
// Note: dryRunMode="false" behaves like "none" (no dry-run flag at all).
339+
// See https://github.com/databus23/helm-diff/issues/894
340+
if !slices.Contains([]string{"client", "true", "false"}, d.dryRunMode) {
341+
flags = append(flags, "--dry-run=server")
342+
}
336343
} else {
337344
flags = append(flags, "--validate")
338345
}
@@ -353,42 +360,34 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
353360
// To keep the full compatibility with older helm-diff versions,
354361
// we pass --dry-run to `helm template` only if Helm is greater than v3.13.0.
355362
if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService {
356-
// However, which dry-run mode to use is still not clear.
357-
//
358-
// For compatibility with the old and new helm-diff options,
359-
// old and new helm, we assume that the user wants to use the older `helm template --dry-run=client` mode
360-
// if helm-diff has been invoked with any of the following flags:
361-
//
362-
// * no dry-run flags (to be consistent with helm-template)
363-
// * --dry-run
364-
// * --dry-run=""
365-
// * --dry-run=client
366-
//
367-
// and the newer `helm template --dry-run=server` mode when invoked with:
368-
//
369-
// * --dry-run=server
370-
//
371-
// Any other values should result in errors.
372-
//
373-
// See the fllowing link for more details:
374-
// - https://github.com/databus23/helm-diff/pull/458
375-
// - https://github.com/helm/helm/pull/9426#issuecomment-1501005666
376-
if d.dryRunMode == "server" {
377-
// This is for security reasons!
378-
//
379-
// We give helm-template the additional cluster access for the helm `lookup` function
380-
// only if the user has explicitly requested it by --dry-run=server,
381-
//
382-
// In other words, although helm-diff-upgrade implies limited cluster access by default,
383-
// helm-diff-upgrade without a --dry-run flag does NOT imply
384-
// full cluster-access via helm-template --dry-run=server!
385-
flags = append(flags, "--dry-run=server")
386-
} else {
387-
// Since helm-diff 3.9.0 and helm 3.13.0, we pass --dry-run=client to `helm template` by default.
388-
// This doesn't make any difference for helm-diff itself,
389-
// because helm-template w/o flags is equivalent to helm-template --dry-run=client.
390-
// See https://github.com/helm/helm/pull/9426#discussion_r1181397259
391-
flags = append(flags, "--dry-run=client")
363+
isHelmV4, _ := isHelmVersionGreaterThanEqual(helmV4Version)
364+
365+
// For Helm v4, --dry-run=server may already have been added above when
366+
// clusterAccessAllowed() is true and d.dryRunMode is not "client", "true", or "false".
367+
// In that case (Helm v4 and d.dryRunMode not "client"/"true"/"false"), we skip adding any
368+
// additional dry-run flag here. In all other cases (Helm v3 or d.dryRunMode is "client"/"true"),
369+
// we add the appropriate dry-run mode below.
370+
// Note: dryRunMode="false" means no dry-run flag at all.
371+
if d.dryRunMode == "false" {
372+
// "false" means no dry-run, skip adding any dry-run flag
373+
} else if !(isHelmV4 && !slices.Contains([]string{"client", "true"}, d.dryRunMode)) {
374+
if d.dryRunMode == "server" {
375+
// This is for security reasons!
376+
//
377+
// We give helm-template the additional cluster access for the helm `lookup` function
378+
// only if the user has explicitly requested it by --dry-run=server,
379+
//
380+
// In other words, although helm-diff-upgrade implies limited cluster access by default,
381+
// helm-diff-upgrade without a --dry-run flag does NOT imply
382+
// full cluster-access via helm-template --dry-run=server!
383+
flags = append(flags, "--dry-run=server")
384+
} else {
385+
// Since helm-diff 3.9.0 and helm 3.13.0, we pass --dry-run=client to `helm template` by default.
386+
// This doesn't make any difference for helm-diff itself,
387+
// because helm-template w/o flags is equivalent to helm-template --dry-run=client.
388+
// See https://github.com/helm/helm/pull/9426#discussion_r1181397259
389+
flags = append(flags, "--dry-run=client")
390+
}
392391
}
393392
}
394393

‎cmd/helm_test.go‎

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,250 @@
11
package cmd
22

33
import (
4+
"reflect"
5+
"slices"
46
"testing"
57

68
"github.com/google/go-cmp/cmp"
79
)
810

11+
type dryRunFlagsConfig struct {
12+
isHelmV4 bool
13+
supportsDryRunLookup bool
14+
clusterAccessAllowed bool
15+
disableValidation bool
16+
dryRunMode string
17+
}
18+
19+
func getTemplateDryRunFlags(cfg dryRunFlagsConfig) []string {
20+
var flags []string
21+
22+
if !cfg.disableValidation && cfg.clusterAccessAllowed {
23+
if cfg.isHelmV4 {
24+
if !slices.Contains([]string{"client", "true", "false"}, cfg.dryRunMode) {
25+
flags = append(flags, "--dry-run=server")
26+
}
27+
} else {
28+
flags = append(flags, "--validate")
29+
}
30+
}
31+
32+
if cfg.supportsDryRunLookup {
33+
if cfg.dryRunMode == "false" {
34+
// "false" means no dry-run, skip adding any dry-run flag
35+
} else if !(cfg.isHelmV4 && !slices.Contains([]string{"client", "true"}, cfg.dryRunMode)) {
36+
if cfg.dryRunMode == "server" {
37+
flags = append(flags, "--dry-run=server")
38+
} else {
39+
flags = append(flags, "--dry-run=client")
40+
}
41+
}
42+
}
43+
44+
return flags
45+
}
46+
47+
func TestGetTemplateDryRunFlags(t *testing.T) {
48+
cases := []struct {
49+
name string
50+
config dryRunFlagsConfig
51+
expected []string
52+
}{
53+
{
54+
name: "Helm v4 with no explicit dry-run flag uses server mode",
55+
config: dryRunFlagsConfig{
56+
isHelmV4: true,
57+
supportsDryRunLookup: true,
58+
clusterAccessAllowed: true,
59+
disableValidation: false,
60+
dryRunMode: "none",
61+
},
62+
expected: []string{"--dry-run=server"},
63+
},
64+
{
65+
name: "Helm v4 with dry-run=client uses client mode",
66+
config: dryRunFlagsConfig{
67+
isHelmV4: true,
68+
supportsDryRunLookup: true,
69+
clusterAccessAllowed: false,
70+
disableValidation: false,
71+
dryRunMode: "client",
72+
},
73+
expected: []string{"--dry-run=client"},
74+
},
75+
{
76+
name: "Helm v4 with dry-run=server uses server mode",
77+
config: dryRunFlagsConfig{
78+
isHelmV4: true,
79+
supportsDryRunLookup: true,
80+
clusterAccessAllowed: true,
81+
disableValidation: false,
82+
dryRunMode: "server",
83+
},
84+
expected: []string{"--dry-run=server"},
85+
},
86+
{
87+
name: "Helm v4 with validation disabled and dry-run=none skips dry-run flags",
88+
config: dryRunFlagsConfig{
89+
isHelmV4: true,
90+
supportsDryRunLookup: true,
91+
clusterAccessAllowed: true,
92+
disableValidation: true,
93+
dryRunMode: "none",
94+
},
95+
expected: nil,
96+
},
97+
{
98+
name: "Helm v4 with validation disabled and dry-run=client uses client mode",
99+
config: dryRunFlagsConfig{
100+
isHelmV4: true,
101+
supportsDryRunLookup: true,
102+
clusterAccessAllowed: true,
103+
disableValidation: true,
104+
dryRunMode: "client",
105+
},
106+
expected: []string{"--dry-run=client"},
107+
},
108+
{
109+
name: "Helm v3 with no explicit dry-run flag uses validate and client",
110+
config: dryRunFlagsConfig{
111+
isHelmV4: false,
112+
supportsDryRunLookup: true,
113+
clusterAccessAllowed: true,
114+
disableValidation: false,
115+
dryRunMode: "none",
116+
},
117+
expected: []string{"--validate", "--dry-run=client"},
118+
},
119+
{
120+
name: "Helm v3 with dry-run=server uses server mode",
121+
config: dryRunFlagsConfig{
122+
isHelmV4: false,
123+
supportsDryRunLookup: true,
124+
clusterAccessAllowed: true,
125+
disableValidation: false,
126+
dryRunMode: "server",
127+
},
128+
expected: []string{"--validate", "--dry-run=server"},
129+
},
130+
{
131+
name: "Helm v3 with dry-run=client uses client mode",
132+
config: dryRunFlagsConfig{
133+
isHelmV4: false,
134+
supportsDryRunLookup: true,
135+
clusterAccessAllowed: false,
136+
disableValidation: false,
137+
dryRunMode: "client",
138+
},
139+
expected: []string{"--dry-run=client"},
140+
},
141+
{
142+
name: "Helm v3 without dry-run lookup support uses only validate",
143+
config: dryRunFlagsConfig{
144+
isHelmV4: false,
145+
supportsDryRunLookup: false,
146+
clusterAccessAllowed: true,
147+
disableValidation: false,
148+
dryRunMode: "none",
149+
},
150+
expected: []string{"--validate"},
151+
},
152+
{
153+
name: "Helm v4 without dry-run lookup support uses server mode",
154+
config: dryRunFlagsConfig{
155+
isHelmV4: true,
156+
supportsDryRunLookup: false,
157+
clusterAccessAllowed: true,
158+
disableValidation: false,
159+
dryRunMode: "none",
160+
},
161+
expected: []string{"--dry-run=server"},
162+
},
163+
{
164+
name: "Helm v4 with empty dry-run mode uses server mode",
165+
config: dryRunFlagsConfig{
166+
isHelmV4: true,
167+
supportsDryRunLookup: true,
168+
clusterAccessAllowed: true,
169+
disableValidation: false,
170+
dryRunMode: "",
171+
},
172+
expected: []string{"--dry-run=server"},
173+
},
174+
}
175+
176+
for _, tc := range cases {
177+
t.Run(tc.name, func(t *testing.T) {
178+
actual := getTemplateDryRunFlags(tc.config)
179+
if !reflect.DeepEqual(actual, tc.expected) {
180+
t.Errorf("Expected %v, got %v", tc.expected, actual)
181+
}
182+
})
183+
}
184+
}
185+
186+
func TestGetTemplateDryRunFlagsBoolModes(t *testing.T) {
187+
cases := []struct {
188+
name string
189+
config dryRunFlagsConfig
190+
expected []string
191+
}{
192+
{
193+
name: "Helm v3 dryRunMode=true behaves like client",
194+
config: dryRunFlagsConfig{
195+
isHelmV4: false,
196+
supportsDryRunLookup: true,
197+
clusterAccessAllowed: true,
198+
disableValidation: false,
199+
dryRunMode: "true",
200+
},
201+
expected: []string{"--validate", "--dry-run=client"},
202+
},
203+
{
204+
name: "Helm v3 dryRunMode=false behaves like none",
205+
config: dryRunFlagsConfig{
206+
isHelmV4: false,
207+
supportsDryRunLookup: true,
208+
clusterAccessAllowed: true,
209+
disableValidation: false,
210+
dryRunMode: "false",
211+
},
212+
expected: []string{"--validate"},
213+
},
214+
{
215+
name: "Helm v4 dryRunMode=true behaves like client",
216+
config: dryRunFlagsConfig{
217+
isHelmV4: true,
218+
supportsDryRunLookup: true,
219+
clusterAccessAllowed: false,
220+
disableValidation: false,
221+
dryRunMode: "true",
222+
},
223+
expected: []string{"--dry-run=client"},
224+
},
225+
{
226+
name: "Helm v4 dryRunMode=false behaves like none",
227+
config: dryRunFlagsConfig{
228+
isHelmV4: true,
229+
supportsDryRunLookup: true,
230+
clusterAccessAllowed: true,
231+
disableValidation: false,
232+
dryRunMode: "false",
233+
},
234+
expected: nil,
235+
},
236+
}
237+
238+
for _, tc := range cases {
239+
t.Run(tc.name, func(t *testing.T) {
240+
actual := getTemplateDryRunFlags(tc.config)
241+
if !reflect.DeepEqual(actual, tc.expected) {
242+
t.Errorf("Expected %v, got %v", tc.expected, actual)
243+
}
244+
})
245+
}
246+
}
247+
9248
func TestExtractManifestFromHelmUpgradeDryRunOutput(t *testing.T) {
10249
type testdata struct {
11250
description string

‎cmd/upgrade.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ var (
2727

2828
const (
2929
dryRunNoOptDefVal = "client"
30+
envTrue = "true"
3031
)
3132

3233
type diffCmd struct {
@@ -116,7 +117,7 @@ func newChartCommand() *cobra.Command {
116117
diff := diffCmd{
117118
namespace: os.Getenv("HELM_NAMESPACE"),
118119
}
119-
unknownFlags := os.Getenv("HELM_DIFF_IGNORE_UNKNOWN_FLAGS") == "true"
120+
unknownFlags := os.Getenv("HELM_DIFF_IGNORE_UNKNOWN_FLAGS") == envTrue
120121

121122
cmd := &cobra.Command{
122123
Use: "upgrade [flags] [RELEASE] [CHART]",
@@ -166,10 +167,10 @@ func newChartCommand() *cobra.Command {
166167
cmd.SilenceUsage = true
167168

168169
// See https://github.com/databus23/helm-diff/issues/253
169-
diff.useUpgradeDryRun = os.Getenv("HELM_DIFF_USE_UPGRADE_DRY_RUN") == "true"
170+
diff.useUpgradeDryRun = os.Getenv("HELM_DIFF_USE_UPGRADE_DRY_RUN") == envTrue
170171

171172
if !diff.threeWayMerge && !cmd.Flags().Changed("three-way-merge") {
172-
enabled := os.Getenv("HELM_DIFF_THREE_WAY_MERGE") == "true"
173+
enabled := os.Getenv("HELM_DIFF_THREE_WAY_MERGE") == envTrue
173174
diff.threeWayMerge = enabled
174175

175176
if enabled {
@@ -178,7 +179,7 @@ func newChartCommand() *cobra.Command {
178179
}
179180

180181
if !diff.normalizeManifests && !cmd.Flags().Changed("normalize-manifests") {
181-
enabled := os.Getenv("HELM_DIFF_NORMALIZE_MANIFESTS") == "true"
182+
enabled := os.Getenv("HELM_DIFF_NORMALIZE_MANIFESTS") == envTrue
182183
diff.normalizeManifests = enabled
183184

184185
if enabled {

‎cmd/upgrade_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package cmd
22

3-
import "testing"
3+
import (
4+
"testing"
5+
)
46

57
func TestIsRemoteAccessAllowed(t *testing.T) {
68
cases := []struct {

0 commit comments

Comments
 (0)