From 43958b733856f319c17aa16caa590241d09fd18c Mon Sep 17 00:00:00 2001
From: Horst Gutmann <horst.gutmann@grafana.com>
Date: Wed, 26 Jun 2024 12:27:45 +0200
Subject: [PATCH] fix: Do not hide diff error details (kubectl < 1.18.0)
 (#1078)

* fix: Do not hide diff error details (kubectl < 1.18.0)

Resolves https://github.com/grafana/tanka/issues/532

* Update pkg/kubernetes/client/diff.go

Co-authored-by: Iain Lane <iain.lane@grafana.com>

---------

Co-authored-by: Iain Lane <iain.lane@grafana.com>
---
 pkg/kubernetes/client/diff.go      | 13 ++++--
 pkg/kubernetes/client/diff_test.go | 70 ++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 3 deletions(-)
 create mode 100644 pkg/kubernetes/client/diff_test.go

diff --git a/pkg/kubernetes/client/diff.go b/pkg/kubernetes/client/diff.go
index 9b6e23e72..43e40a1b7 100644
--- a/pkg/kubernetes/client/diff.go
+++ b/pkg/kubernetes/client/diff.go
@@ -2,8 +2,8 @@ package client
 
 import (
 	"bytes"
+	"fmt"
 	"os"
-	"os/exec"
 	"regexp"
 	"strings"
 
@@ -12,8 +12,15 @@ import (
 	"github.com/grafana/tanka/pkg/kubernetes/manifest"
 )
 
+// Provides just the parts of `*exec.ExitError` that we need, so we can swap for a fake in the tests
+type exitError interface {
+	error
+	ExitCode() int
+}
+
 // DiffServerSide takes the desired state and computes the differences server-side, returning them in `diff(1)` format
 func (k Kubectl) DiffServerSide(data manifest.List) (*string, error) {
+	fmt.Println(data)
 	return k.diff(data, true)
 }
 
@@ -78,7 +85,7 @@ func (k Kubectl) diff(data manifest.List, serverSide bool) (*string, error) {
 // 0: no error, no differences
 // 1: error OR differences found
 func parseDiffErr(err error, stderr string, version *semver.Version) error {
-	exitErr, ok := err.(*exec.ExitError)
+	exitErr, ok := err.(exitError)
 	if !ok {
 		// this error is not kubectl related
 		return err
@@ -91,7 +98,7 @@ func parseDiffErr(err error, stderr string, version *semver.Version) error {
 
 	// before 1.18 "exit status 1" meant error as well ... so we need to check stderr
 	if version.LessThan(semver.MustParse("1.18.0")) && stderr != "" {
-		return err
+		return fmt.Errorf("diff failed: %w (%s)", err, stderr)
 	}
 
 	// differences found is not an error
diff --git a/pkg/kubernetes/client/diff_test.go b/pkg/kubernetes/client/diff_test.go
new file mode 100644
index 000000000..38ae744c3
--- /dev/null
+++ b/pkg/kubernetes/client/diff_test.go
@@ -0,0 +1,70 @@
+package client
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/Masterminds/semver"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseDiffError(t *testing.T) {
+	tests := map[string]struct {
+		err         error
+		stderr      string
+		version     *semver.Version
+		expectedErr error
+	}{
+		// If this is not an exit error, then we just pass it through:
+		"no-exiterr": {
+			err:         fmt.Errorf("something else"),
+			stderr:      "error-details",
+			version:     semver.MustParse("1.17.0"),
+			expectedErr: fmt.Errorf("something else"),
+		},
+		// If kubectl returns with an exit code other than 1 its an indicator
+		// that it is not an internal error and so we return it as is:
+		"return-internal-as-is": {
+			err: &dummyExitError{
+				exitCode: 123,
+			},
+			stderr:      "error-details",
+			version:     semver.MustParse("1.17.0"),
+			expectedErr: fmt.Errorf("ExitError"),
+		},
+		// If kubectl is is < 1.18.0, then the error should contain then stderr
+		// content:
+		"lt-1.18.0-contains-stderr": {
+			err: &dummyExitError{
+				exitCode: 1,
+			},
+			stderr:      "error-details",
+			version:     semver.MustParse("1.17.0"),
+			expectedErr: fmt.Errorf("diff failed: ExitError (error-details)"),
+		},
+	}
+
+	for testName, test := range tests {
+		t.Run(testName, func(t *testing.T) {
+			err := parseDiffErr(test.err, test.stderr, test.version)
+			if test.expectedErr == nil {
+				require.NoError(t, err)
+				return
+			}
+			require.Error(t, err)
+			require.Equal(t, test.expectedErr.Error(), err.Error())
+		})
+	}
+}
+
+type dummyExitError struct {
+	exitCode int
+}
+
+func (e *dummyExitError) Error() string {
+	return "ExitError"
+}
+
+func (e *dummyExitError) ExitCode() int {
+	return e.exitCode
+}