fix: don't print help for non-validation errors. Fixes #14234 (cherry-pick #14249) (#14283)

Signed-off-by: Koichi Shimada <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Co-authored-by: koichi <[email protected]>
Co-authored-by: Mason Malone <[email protected]>

Author: 3 people

cmd/argoexec/commands/root.go

@@ -55,6 +55,14 @@ func NewRootCommand() *cobra.Command {
 		},
 		PersistentPreRun: func(cmd *cobra.Command, args []string) {
 			initConfig()
+
+			// Disable printing of usage string on errors, except for argument validation errors
+			// (i.e. when the "Args" function returns an error).
+			//
+			// This is set here instead of directly in "command" because Cobra
+			// executes PersistentPreRun after performing argument validation:
+			// https://github.com/spf13/cobra/blob/3a5efaede9d389703a792e2f7bfe3a64bc82ced9/command.go#L939-L957
+			cmd.SilenceUsage = true
 		},
 	}
 

test/e2e/expectedfailures/serviceaccount-insufficient-permissions.yaml

@@ -0,0 +1,13 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: serviceaccount-insufficient-permissions
+spec:
+  # This ServiceAccount is only intended for use in webhooks, and doesn't have
+  # the permissions needed by the executor.
+  serviceAccountName: github.com
+  entrypoint: main
+  templates:
+  - name: main
+    container:
+      image: argoproj/argosay:v2

test/e2e/fixtures/then.go

@@ -3,6 +3,7 @@ package fixtures
 import (
 	"context"
 	"fmt"
+	"io"
 	"reflect"
 	"testing"
 	"time"
@@ -264,6 +265,25 @@ func (t *Then) ExpectPods(f func(t *testing.T, pods []apiv1.Pod)) *Then {
 	return t
 }
 
+func (t *Then) ExpectContainerLogs(container string, f func(t *testing.T, logs string)) *Then {
+	t.t.Helper()
+
+	stream, err := t.kubeClient.CoreV1().Pods(t.wf.Namespace).GetLogs(t.wf.Name, &apiv1.PodLogOptions{Container: container}).Stream(context.Background())
+	if err != nil {
+		t.t.Fatal(err)
+	}
+
+	defer stream.Close()
+	logBytes, err := io.ReadAll(stream)
+	if err != nil {
+		t.t.Fatal(err)
+	}
+
+	f(t.t, string(logBytes))
+
+	return t
+}
+
 func (t *Then) ExpectWorkflowTaskSet(block func(t *testing.T, wfts *wfv1.WorkflowTaskSet)) *Then {
 	t.t.Helper()
 	ctx := context.Background()

test/e2e/functional_test.go

@@ -1446,3 +1446,20 @@ func (s *FunctionalSuite) TestWorkflowParallelismDAGFailFast() {
 			assert.Equal(t, wfv1.NodeSucceeded, status.Nodes.FindByDisplayName("task2").Phase)
 		})
 }
+
+func (s *FunctionalSuite) TestWorkflowInvalidServiceAccountError() {
+	s.Given().
+		Workflow("@expectedfailures/serviceaccount-insufficient-permissions.yaml").
+		When().
+		SubmitWorkflow().
+		WaitForWorkflow(fixtures.ToBeErrored).
+		Then().
+		ExpectContainerLogs("main", func(t *testing.T, logs string) {
+			assert.Contains(t, logs, "hello argo")
+		}).
+		ExpectContainerLogs("wait", func(t *testing.T, logs string) {
+			assert.Contains(t, logs, "Error: workflowtaskresults.argoproj.io is forbidden: User \"system:serviceaccount:argo:github.com\" cannot create resource")
+			// Shouldn't have print help text
+			assert.NotContains(t, logs, "Usage:")
+		})
+}

test/e2e/retry_test.go

@@ -3,8 +3,6 @@
 package e2e
 
 import (
-	"context"
-	"io"
 	"strings"
 	"testing"
 	"time"
@@ -123,8 +121,6 @@ spec:
 }
 
 func (s *RetryTestSuite) TestWorkflowTemplateWithRetryStrategyInContainerSet() {
-	var name string
-	var ns string
 	s.Given().
 		WorkflowTemplate("@testdata/workflow-template-with-containerset.yaml").
 		Workflow(`
@@ -142,55 +138,25 @@ spec:
 		ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
 			assert.Equal(t, wfv1.WorkflowFailed, status.Phase)
 		}).
-		ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool {
-			return status.Name == "workflow-template-containerset"
-		}, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) {
-			name = pod.GetName()
-			ns = pod.GetNamespace()
+		// Success, no need retry
+		ExpectContainerLogs("c1", func(t *testing.T, logs string) {
+			count := strings.Count(logs, "capturing logs")
+			assert.Equal(t, 1, count)
+			assert.Contains(t, logs, "hi")
+		}).
+		// Command err. No retry logic is entered.
+		ExpectContainerLogs("c2", func(t *testing.T, logs string) {
+			count := strings.Count(logs, "capturing logs")
+			assert.Equal(t, 0, count)
+			assert.Contains(t, logs, "executable file not found in $PATH")
+		}).
+		// Retry when err.
+		ExpectContainerLogs("c3", func(t *testing.T, logs string) {
+			count := strings.Count(logs, "capturing logs")
+			assert.Equal(t, 2, count)
+			countFailureInfo := strings.Count(logs, "intentional failure")
+			assert.Equal(t, 2, countFailureInfo)
 		})
-	// Success, no need retry
-	s.Run("ContainerLogs", func() {
-		ctx := context.Background()
-		podLogOptions := &apiv1.PodLogOptions{Container: "c1"}
-		stream, err := s.KubeClient.CoreV1().Pods(ns).GetLogs(name, podLogOptions).Stream(ctx)
-		s.Require().NoError(err)
-		defer stream.Close()
-		logBytes, err := io.ReadAll(stream)
-		s.Require().NoError(err)
-		output := string(logBytes)
-		count := strings.Count(output, "capturing logs")
-		s.Equal(1, count)
-		s.Contains(output, "hi")
-	})
-	// Command err. No retry logic is entered.
-	s.Run("ContainerLogs", func() {
-		ctx := context.Background()
-		podLogOptions := &apiv1.PodLogOptions{Container: "c2"}
-		stream, err := s.KubeClient.CoreV1().Pods(ns).GetLogs(name, podLogOptions).Stream(ctx)
-		s.Require().NoError(err)
-		defer stream.Close()
-		logBytes, err := io.ReadAll(stream)
-		s.Require().NoError(err)
-		output := string(logBytes)
-		count := strings.Count(output, "capturing logs")
-		s.Equal(0, count)
-		s.Contains(output, "executable file not found in $PATH")
-	})
-	// Retry when err.
-	s.Run("ContainerLogs", func() {
-		ctx := context.Background()
-		podLogOptions := &apiv1.PodLogOptions{Container: "c3"}
-		stream, err := s.KubeClient.CoreV1().Pods(ns).GetLogs(name, podLogOptions).Stream(ctx)
-		s.Require().NoError(err)
-		defer stream.Close()
-		logBytes, err := io.ReadAll(stream)
-		s.Require().NoError(err)
-		output := string(logBytes)
-		count := strings.Count(output, "capturing logs")
-		s.Equal(2, count)
-		countFailureInfo := strings.Count(output, "intentional failure")
-		s.Equal(2, countFailureInfo)
-	})
 }
 
 func (s *RetryTestSuite) TestRetryNodeAntiAffinity() {