fixup! fixup! fixup! fix: Make dry-run=server optional

Author: mumoshu

cmd/helm3.go

@@ -14,8 +14,9 @@ import (
 )
 
 var (
-	helmVersionRE                         = regexp.MustCompile(`Version:\s*"([^"]+)"`)
-	minHelmVersion                        = semver.MustParse("v3.1.0-rc.1")
+	helmVersionRE  = regexp.MustCompile(`Version:\s*"([^"]+)"`)
+	minHelmVersion = semver.MustParse("v3.1.0-rc.1")
+	// See https://github.com/helm/helm/pull/9426
 	minHelmVersionWithDryRunLookupSupport = semver.MustParse("v3.13.0")
 )
 
@@ -131,7 +132,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 	// Let's simulate that in helm-diff.
 	// See https://medium.com/@kcatstack/understand-helm-upgrade-flags-reset-values-reuse-values-6e58ac8f127e
 	shouldDefaultReusingValues := isUpgrade && len(d.values) == 0 && len(d.stringValues) == 0 && len(d.valueFiles) == 0 && len(d.fileValues) == 0
-	if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && d.isRemoteAccessAllowed() {
+	if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && d.clusterAccessAllowed() {
 		tmpfile, err := os.CreateTemp("", "existing-values")
 		if err != nil {
 			return nil, err
@@ -192,36 +193,28 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 		filter func([]byte) []byte
 	)
 
+	// `--dry-run=client` or `--dry-run=server`?
+	//
+	// Or what's the relationoship between helm-diff's --dry-run flag,
+	// HELM_DIFF_UPGRADE_DRY_RUN env var and the helm upgrade --dry-run flag?
+	//
+	// Read on to find out.
 	if d.useUpgradeDryRun {
-		//
-		// # `--dry-run=client` or `--dry-run=server`?
-		//
-		// Or what's the relationoship between helm-diff's --dry-run flag,
-		// HELM_DIFF_UPGRADE_DRY_RUN env var and the helm upgrade --dry-run flag?
-		//
-		// If the program reaches here,
-		// we are sure that the user wants to user the `helm upgrade --dry-run` command
-		// for generating the manifests to be diffed.
-		//
-		// However, which dry-run mode to use is still not clear.
-		//
-		// For compatibility with the old and new helm-diff options,
-		// old and new helm, we assume that the user wants to use the `--dry-run=client` mode
-		// if helm-diff has been invoked with any of the following flags:
-		//
-		// * --dry-run
-		// * --dry-run=""
-		// * --dry-run=client
-		//
-		// Otherwise, we assume that the user wants to use the `--dry-run=server` mode.
-		//
-
 		if d.isAllowUnreleased() {
 			// Otherwise you get the following error when this is a diff for a new install
 			//   Error: UPGRADE FAILED: "$RELEASE_NAME" has no deployed releases
 			flags = append(flags, "--install")
 		}
 
+		// If the program reaches here,
+		// we are sure that the user wants to user the `helm upgrade --dry-run` command
+		// for generating the manifests to be diffed.
+		//
+		// So the question is only whether to use `--dry-run=client` or `--dry-run=server`.
+		//
+		// As HELM_DIFF_UPGRADE_DRY_RUN is there for producing more complete and correct diff results,
+		// we use --dry-run=server if the version of helm supports it.
+		// Otherwise, we use --dry-run=client, as that's the best we can do.
 		if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService {
 			flags = append(flags, "--dry-run=server")
 		} else {
@@ -232,7 +225,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 			return extractManifestFromHelmUpgradeDryRunOutput(s, d.noHooks)
 		}
 	} else {
-		if !d.disableValidation && d.isRemoteAccessAllowed() {
+		if !d.disableValidation && d.clusterAccessAllowed() {
 			flags = append(flags, "--validate")
 		}
 
@@ -251,12 +244,28 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 		// To keep the full compatibility with older helm-diff versions,
 		// we pass --dry-run to `helm template` only if Helm is greater than v3.13.0.
 		if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService {
+			// However, which dry-run mode to use is still not clear.
+			//
+			// For compatibility with the old and new helm-diff options,
+			// old and new helm, we assume that the user wants to use the older `helm template --dry-run=client` mode
+			// if helm-diff has been invoked with any of the following flags:
+			//
+			// * no dry-run flags (to be consistent with helm-template)
+			// * --dry-run
+			// * --dry-run=""
+			// * --dry-run=client
+			//
+			// and the newer `helm template --dry-run=server` mode when invoked with:
+			//
+			// * --dry-run=server
+			//
+			// Any other values should result in errors.
+			//
 			// See the fllowing link for more details:
 			// - https://github.com/databus23/helm-diff/pull/458
 			// - https://github.com/helm/helm/pull/9426#issuecomment-1501005666
 			if d.dryRunMode == "server" {
-				//
-				// # This is for security reasons!
+				// This is for security reasons!
 				//
 				// We give helm-template the additional cluster access for the helm `lookup` function
 				// only if the user has explicitly requested it by --dry-run=server,
@@ -266,6 +275,10 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 				// full cluster-access via helm-template --dry-run=server!
 				flags = append(flags, "--dry-run=server")
 			} else {
+				// Since helm-diff 3.9.0 and helm 3.13.0, we pass --dry-run=client to `helm template` by default.
+				// This doesn't make any difference for helm-diff itself,
+				// because helm-template w/o flags is equivalent to helm-template --dry-run=client.
+				// See https://github.com/helm/helm/pull/9426#discussion_r1181397259
 				flags = append(flags, "--dry-run=client")
 			}
 		}

cmd/upgrade.go

@@ -69,7 +69,24 @@ func (d *diffCmd) isAllowUnreleased() bool {
 	return d.allowUnreleased || d.install
 }
 
-func (d *diffCmd) isRemoteAccessAllowed() bool {
+// clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree.
+//
+// helm-diff basically have 3 modes of operation:
+// 1. no cluster access at all when --dry-run, --dry-run=client, or --dry-run=true is specified.
+// 2. basic cluster access when --dry-run is not specified.
+// 3. extra cluster access when --dry-run=server is specified.
+//
+// clusterAccessAllowed returns true when the mode is either 2 or 3.
+//
+// If false, helm-diff should not access the cluster at all.
+// More concretely:
+// - It shouldn't pass --validate to helm-template because it requires cluster access.
+// - It shouldn't get the current release manifest using helm-get-manifest because it requires cluster access.
+// - It shouldn't get the current release hooks using helm-get-hooks because it requires cluster access.
+// - It shouldn't get the current release values using helm-get-values because it requires cluster access.
+//
+// See also https://github.com/helm/helm/pull/9426#discussion_r1181397259
+func (d *diffCmd) clusterAccessAllowed() bool {
 	clientOnly := (d.dryRunModeSpecified && d.dryRunMode == "") || d.dryRunMode == "true" || d.dryRunMode == "client"
 	return !clientOnly
 }
@@ -279,7 +296,7 @@ func (d *diffCmd) runHelm3() error {
 
 	var err error
 
-	if d.isRemoteAccessAllowed() {
+	if d.clusterAccessAllowed() {
 		releaseManifest, err = getRelease(d.release, d.namespace)
 	}
 
@@ -326,7 +343,7 @@ func (d *diffCmd) runHelm3() error {
 	}
 
 	currentSpecs := make(map[string]*manifest.MappingResult)
-	if !newInstall && d.isRemoteAccessAllowed() {
+	if !newInstall && d.clusterAccessAllowed() {
 		if !d.noHooks && !d.threeWayMerge {
 			hooks, err := getHooks(d.release, d.namespace)
 			if err != nil {

cmd/upgrade_test.go

@@ -49,7 +49,7 @@ func TestIsRemoteAccessAllowed(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			actual := tc.cmd.isRemoteAccessAllowed()
+			actual := tc.cmd.clusterAccessAllowed()
 			if actual != tc.expected {
 				t.Errorf("Expected %v, got %v", tc.expected, actual)
 			}

go.mod

@@ -135,6 +135,7 @@ require (
 	go.opentelemetry.io/otel v1.14.0 // indirect
 	go.opentelemetry.io/otel/trace v1.14.0 // indirect
 	go.starlark.net v0.0.0-20200306205701-8dd3e2ee1dd5 // indirect
+	golang.org/dl v0.0.0-20231010164639-b8ab22880620 // indirect
 	golang.org/x/net v0.17.0 // indirect
 	golang.org/x/oauth2 v0.4.0 // indirect
 	golang.org/x/sync v0.1.0 // indirect

go.sum

@@ -715,6 +715,8 @@ go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/
 go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
 go.uber.org/zap v1.10.0/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
 go.uber.org/zap v1.17.0/go.mod h1:MXVU+bhUf/A7Xi2HNOnopQOrmycQ5Ih87HtOu4q5SSo=
+golang.org/dl v0.0.0-20231010164639-b8ab22880620 h1:Ouctod3TwJPqrchQF9XERIziHPu5m6FLvxSNNOc70vQ=
+golang.org/dl v0.0.0-20231010164639-b8ab22880620/go.mod h1:IUMfjQLJQd4UTqG1Z90tenwKoCX93Gn3MAQJMOSBsDQ=
 golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
 golang.org/x/crypto v0.0.0-20181029021203-45a5f77698d3/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=