diff --git a/test/e2e/hooks_test.go b/test/e2e/hooks_test.go index d10e06cb5491..23cb235ce161 100644 --- a/test/e2e/hooks_test.go +++ b/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) diff --git a/test/e2e/signals_test.go b/test/e2e/signals_test.go index 132e99a60c49..13a10020b91c 100644 --- a/test/e2e/signals_test.go +++ b/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) diff --git a/util/errors/errors.go b/util/errors/errors.go index 19addc48312c..6d41bf612213 100644 --- a/util/errors/errors.go +++ b/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 diff --git a/util/errors/errors_test.go b/util/errors/errors_test.go index 8090072c1ae1..702c3d594251 100644 --- a/util/errors/errors_test.go +++ b/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("")))) diff --git a/workflow/common/util.go b/workflow/common/util.go index 4b5aa0278225..b7ffbe291494 100644 --- a/workflow/common/util.go +++ b/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 diff --git a/workflow/common/util_test.go b/workflow/common/util_test.go index a5b02c882d8d..529da00690ce 100644 --- a/workflow/common/util_test.go +++ b/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) + } + }) } }