Merge pull request #87437 from apelisse/kubectl-diff-exit-code

kubectl-diff: Return non-1 errors on kubectl failures

hack/testdata/invalid-pod.yaml

@@ -0,0 +1,8 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test
+spec:
+  containers:
+  - name:
+    image: nginx

staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go

@@ -68,6 +68,15 @@ var (
 // Number of times we try to diff before giving-up
 const maxRetries = 4
 
+// diffError returns the ExitError if the status code is less than 1,
+// nil otherwise.
+func diffError(err error) exec.ExitError {
+	if err, ok := err.(exec.ExitError); ok && err.ExitStatus() <= 1 {
+		return err
+	}
+	return nil
+}
+
 type DiffOptions struct {
 	FilenameOptions resource.FilenameOptions
 
@@ -109,9 +118,20 @@ func NewCmdDiff(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra.C
 		Long:                  diffLong,
 		Example:               diffExample,
 		Run: func(cmd *cobra.Command, args []string) {
-			cmdutil.CheckErr(options.Complete(f, cmd))
+			cmdutil.CheckDiffErr(options.Complete(f, cmd))
-			cmdutil.CheckErr(validateArgs(cmd, args))
+			cmdutil.CheckDiffErr(validateArgs(cmd, args))
-			cmdutil.CheckErr(options.Run())
+			// `kubectl diff` propagates the error code from
+			// diff or `KUBECTL_EXTERNAL_DIFF`. Also, we
+			// don't want to print an error if diff returns
+			// error code 1, which simply means that changes
+			// were found. We also don't want kubectl to
+			// return 1 if there was a problem.
+			if err := options.Run(); err != nil {
+				if exitErr := diffError(err); exitErr != nil {
+					os.Exit(exitErr.ExitStatus())
+				}
+				cmdutil.CheckDiffErr(err)
+			}
 		},
 	}
 
@@ -150,6 +170,11 @@ func (d *DiffProgram) getCommand(args ...string) (string, exec.Cmd) {
 func (d *DiffProgram) Run(from, to string) error {
 	diff, cmd := d.getCommand(from, to)
 	if err := cmd.Run(); err != nil {
+		// Let's not wrap diff errors, or we won't be able to
+		// differentiate them later.
+		if diffErr := diffError(err); diffErr != nil {
+			return diffErr
+		}
 		return fmt.Errorf("failed to run %q: %v", diff, err)
 	}
 	return nil

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go

@@ -115,6 +115,18 @@ func CheckErr(err error) {
 	checkErr(err, fatalErrHandler)
 }
 
+// CheckDiffErr prints a user friendly error to STDERR and exits with a
+// non-zero and non-one exit code. Unrecognized errors will be printed
+// with an "error: " prefix.
+//
+// This method is meant specifically for `kubectl diff` and may be used
+// by other commands.
+func CheckDiffErr(err error) {
+	checkErr(err, func(msg string, code int) {
+		fatalErrHandler(msg, code+1)
+	})
+}
+
 // checkErr formats a given error as a string and calls the passed handleErr
 // func with that string and an kubectl exit code.
 func checkErr(err error, handleErr func(string, int)) {

test/cmd/diff.sh

@@ -32,9 +32,18 @@ run_kubectl_diff_tests() {
 
     kubectl apply -f hack/testdata/pod.yaml
 
-    output_message=$(! kubectl diff -f hack/testdata/pod-changed.yaml)
+    # Make sure that diffing the resource right after returns nothing (0 exit code).
+    kubectl diff -f hack/testdata/pod.yaml
+
+    # Make sure that:
+    # 1. the exit code for diff is 1 because it found a difference
+    # 2. the difference contains the changed image
+    output_message=$(kubectl diff -f hack/testdata/pod-changed.yaml || test $? -eq 1)
     kube::test::if_has_string "${output_message}" 'k8s.gcr.io/pause:3.0'
 
+    # Test that we have a return code bigger than 1 if there is an error when diffing
+    kubectl diff -f hack/testdata/invalid-pod.yaml || test $? -gt 1
+
     kubectl delete -f hack/testdata/pod.yaml
 
     set +o nounset