Merge branch 'main' into fix/workflow-row-styles

test/e2e/hooks_test.go

@@ -5,6 +5,7 @@ package e2e
 import (
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/suite"
@@ -805,7 +806,7 @@ spec:
         args: ["sleep", "5"]
 `).When().
 		SubmitWorkflow().
-		WaitForWorkflow(fixtures.ToBeCompleted).
+		WaitForWorkflow(fixtures.ToBeCompleted, 2*time.Minute).
 		WaitForWorkflow(fixtures.Condition(func(wf *v1alpha1.Workflow) (bool, string) {
 			onExitNodeName = common.GenerateOnExitNodeName(wf.ObjectMeta.Name)
 			onExitNode := wf.Status.Nodes.FindByDisplayName(onExitNodeName)

test/e2e/signals_test.go

@@ -150,7 +150,7 @@ func (s *SignalsSuite) TestSignaledContainerSet() {
 		Workflow("@testdata/signaled-container-set-workflow.yaml").
 		When().
 		SubmitWorkflow().
-		WaitForWorkflow().
+		WaitForWorkflow(killDuration).
 		Then().
 		ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
 			assert.Equal(t, wfv1.WorkflowFailed, status.Phase)

util/errors/errors.go

@@ -26,7 +26,7 @@ func IgnoreContainerNotFoundErr(err error) error {
 // IsTransientErr reports whether the error is transient and logs it.
 func IsTransientErr(err error) bool {
 	isTransient := IsTransientErrQuiet(err)
-	if !isTransient {
+	if err != nil && !isTransient {
 		log.Warnf("Non-transient error: %v", err)
 	}
 	return isTransient

util/errors/errors_test.go

@@ -8,6 +8,8 @@ import (
 	"os/exec"
 	"testing"
 
+	log "github.com/sirupsen/logrus"
+	logtest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	apierr "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -50,12 +52,19 @@ var (
 const transientEnvVarKey = "TRANSIENT_ERROR_PATTERN"
 
 func TestIsTransientErr(t *testing.T) {
+	hook := &logtest.Hook{}
+	log.AddHook(hook)
+	defer log.StandardLogger().ReplaceHooks(nil)
+
 	t.Run("Nil", func(t *testing.T) {
 		assert.False(t, IsTransientErr(nil))
+		assert.Nil(t, hook.LastEntry())
 	})
 	t.Run("ResourceQuotaConflictErr", func(t *testing.T) {
 		assert.False(t, IsTransientErr(apierr.NewConflict(schema.GroupResource{}, "", nil)))
+		assert.Contains(t, hook.LastEntry().Message, "Non-transient error:")
 		assert.True(t, IsTransientErr(apierr.NewConflict(schema.GroupResource{Group: "v1", Resource: "resourcequotas"}, "", nil)))
+		assert.Contains(t, hook.LastEntry().Message, "Transient error:")
 	})
 	t.Run("ResourceQuotaTimeoutErr", func(t *testing.T) {
 		assert.False(t, IsTransientErr(apierr.NewInternalError(errors.New(""))))

workflow/common/util.go

@@ -133,14 +133,19 @@ func overwriteWithArguments(argParam, inParam *wfv1.Parameter) {
 func substituteAndGetConfigMapValue(inParam *wfv1.Parameter, globalParams Parameters, namespace string, configMapStore ConfigMapStore) error {
 	if inParam.ValueFrom != nil && inParam.ValueFrom.ConfigMapKeyRef != nil {
 		if configMapStore != nil {
+			replaceMap := make(map[string]interface{})
+			for k, v := range globalParams {
+				replaceMap[k] = v
+			}
+
 			// SubstituteParams is called only at the end of this method. To support parametrization of the configmap
 			// we need to perform a substitution here over the name and the key of the ConfigMapKeyRef.
-			cmName, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Name, globalParams)
+			cmName, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Name, replaceMap)
 			if err != nil {
 				log.WithError(err).Error("unable to substitute name for ConfigMapKeyRef")
 				return err
 			}
-			cmKey, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Key, globalParams)
+			cmKey, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Key, replaceMap)
 			if err != nil {
 				log.WithError(err).Error("unable to substitute key for ConfigMapKeyRef")
 				return err
@@ -219,21 +224,17 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.ArgumentsProvider, globalParams,
 	return SubstituteParams(newTmpl, globalParams, localParams)
 }
 
-// substituteConfigMapKeyRefParam check if ConfigMapKeyRef's key is a param and perform the substitution.
-func substituteConfigMapKeyRefParam(in string, globalParams Parameters) (string, error) {
-	if strings.HasPrefix(in, "{{") && strings.HasSuffix(in, "}}") {
-		k := strings.TrimSuffix(strings.TrimPrefix(in, "{{"), "}}")
-		k = strings.Trim(k, " ")
-
-		v, ok := globalParams[k]
-		if !ok {
-			err := errors.InternalError(fmt.Sprintf("parameter %s not found", k))
-			log.WithError(err).Error()
-			return "", err
-		}
-		return v, nil
+// substituteConfigMapKeyRefParam performs template substitution for ConfigMapKeyRef
+func substituteConfigMapKeyRefParam(in string, replaceMap map[string]interface{}) (string, error) {
+	tmpl, err := template.NewTemplate(in)
+	if err != nil {
+		return "", err
+	}
+	replacedString, err := tmpl.Replace(replaceMap, false)
+	if err != nil {
+		return "", fmt.Errorf("failed to substitute configMapKeyRef: %w", err)
 	}
-	return in, nil
+	return replacedString, nil
 }
 
 // SubstituteParams returns a new copy of the template with global, pod, and input parameters substituted

workflow/common/util_test.go

@@ -48,54 +48,6 @@ spec:
       command: [cowsay]
       args: ["hello world"]
 
-`
-	validConfigMapRefWf = `apiVersion: argoproj.io/v1alpha1
-kind: Workflow
-metadata:
-  name: test-template-configmapkeyselector-substitution
-spec:
-  entrypoint: whalesay
-  arguments:
-    parameters:
-    - name: name
-      value: simple-parameters
-    - name: key
-      value: msg
-  templates:
-  - name: whalesay
-    inputs:
-      parameters:
-      - name: message
-        valueFrom:
-          configMapKeyRef:
-            name: "{{ workflow.parameters.name }}"
-            key: "{{ workflow.parameters.key }}"
-    container:
-      image: argoproj/argosay:v2
-      args:
-        - echo
-        - "{{inputs.parameters.message}}"
-`
-	invalidConfigMapRefWf = `apiVersion: argoproj.io/v1alpha1
-kind: Workflow
-metadata:
-  name: test-template-configmapkeyselector-substitution
-spec:
-  entrypoint: whalesay
-  templates:
-  - name: whalesay
-    inputs:
-      parameters:
-      - name: message
-        valueFrom:
-          configMapKeyRef:
-            name: "{{ workflow.parameters.name }}"
-            key: "{{ workflow.parameters.key }}"
-    container:
-      image: argoproj/argosay:v2
-      args:
-        - echo
-        - "{{inputs.parameters.message}}"
 `
 )
 
@@ -194,45 +146,70 @@ func TestIsDone(t *testing.T) {
 }
 
 func TestSubstituteConfigMapKeyRefParam(t *testing.T) {
-	res := ParseObjects([]byte(validConfigMapRefWf), false)
-	assert.Len(t, res, 1)
-
-	obj, ok := res[0].Object.(*wfv1.Workflow)
-	assert.True(t, ok)
-	assert.NotNil(t, obj)
-
-	globalParams := Parameters{
+	globalParams := map[string]interface{}{
 		"workflow.parameters.name": "simple-parameters",
 		"workflow.parameters.key":  "msg",
 	}
-
-	for _, inParam := range obj.GetTemplateByName("whalesay").Inputs.Parameters {
-		cmName, _ := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Name, globalParams)
-		assert.Equal(t, "simple-parameters", cmName, "it should be equal")
-
-		cmKey, _ := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Key, globalParams)
-		assert.Equal(t, "msg", cmKey, "it should be equal")
+	tests := []struct {
+		name                 string
+		configMapKeyRefParam string
+		expected             string
+		expectedErr          string
+	}{
+		{
+			name:                 "No string templating",
+			configMapKeyRefParam: "simple-parameters",
+			expected:             "simple-parameters",
+			expectedErr:          "",
+		},
+		{
+			name:                 "Simple template",
+			configMapKeyRefParam: "{{ workflow.parameters.name }}",
+			expected:             "simple-parameters",
+			expectedErr:          "",
+		},
+		{
+			name:                 "Simple template with prefix and suffix",
+			configMapKeyRefParam: "prefix-{{ workflow.parameters.name }}-suffix",
+			expected:             "prefix-simple-parameters-suffix",
+			expectedErr:          "",
+		},
+		{
+			name:                 "Expression template",
+			configMapKeyRefParam: "{{=upper(workflow.parameters.key)}}",
+			expected:             "MSG",
+			expectedErr:          "",
+		},
+		{
+			name:                 "Simple template referencing nonexistent param",
+			configMapKeyRefParam: "prefix-{{ workflow.parameters.bad }}",
+			expected:             "",
+			expectedErr:          "failed to substitute configMapKeyRef: failed to resolve {{ workflow.parameters.bad }}",
+		},
+		{
+			name:                 "Expression template with invalid expression",
+			configMapKeyRefParam: "{{=!}}",
+			expected:             "",
+			expectedErr:          "failed to substitute configMapKeyRef: failed to evaluate expression: unexpected token EOF (1:1)\n | !\n | ^",
+		},
+		{
+			name:                 "Malformed template",
+			configMapKeyRefParam: "{{ workflow.parameters.bad",
+			expected:             "",
+			expectedErr:          "Cannot find end tag=\"}}\" in the template=\"{{ workflow.parameters.bad\" starting from \" workflow.parameters.bad\"",
+		},
 	}
-}
-
-func TestSubstituteConfigMapKeyRefParamWithNoParamsDefined(t *testing.T) {
-	res := ParseObjects([]byte(invalidConfigMapRefWf), false)
-	assert.Len(t, res, 1)
-
-	obj, ok := res[0].Object.(*wfv1.Workflow)
-	assert.True(t, ok)
-	assert.NotNil(t, obj)
-
-	globalParams := Parameters{}
-
-	for _, inParam := range obj.GetTemplateByName("whalesay").Inputs.Parameters {
-		cmName, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Name, globalParams)
-		require.EqualError(t, err, "parameter workflow.parameters.name not found")
-		assert.Equal(t, "", cmName)
 
-		cmKey, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Key, globalParams)
-		require.EqualError(t, err, "parameter workflow.parameters.key not found")
-		assert.Equal(t, "", cmKey)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := substituteConfigMapKeyRefParam(tt.configMapKeyRefParam, globalParams)
+			assert.Equal(t, tt.expected, result)
+			if tt.expectedErr != "" {
+				require.EqualError(t, err, tt.expectedErr)
+			} else {
+				require.NoError(t, err)
+			}
+		})
 	}
 }