From 4ee0f0b34610ccebcc825914af2c1e6eb8caac6c Mon Sep 17 00:00:00 2001 From: Mason Malone <651224+MasonM@users.noreply.github.com> Date: Tue, 19 Nov 2024 13:08:48 -0800 Subject: [PATCH 1/2] fix: consistent variable substitution for configMapKeyRef. Fixes #13890 Support for subsituting variables in `configMapKeyRef` was added in https://github.com/argoproj/argo-workflows/pull/7542 by manually parsing templates out of the string instead of using the standard `template.Replace()` functionality. Since the templates have the same syntax as simple template tags (e.g. `{{workflow.parameters.cm-name}}`), users reasonably expect it to work the same, but it doesn't, which leads to confusion. This replaces the manual parsing code with the standard `template.Replace()` function that everything else uses. Signed-off-by: Mason Malone <651224+MasonM@users.noreply.github.com> --- workflow/common/util.go | 33 ++++---- workflow/common/util_test.go | 141 +++++++++++++++-------------------- 2 files changed, 76 insertions(+), 98 deletions(-) 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) + } + }) } } From 6bc3015415d9eb1f9f9dc5d04c70121da0eb2712 Mon Sep 17 00:00:00 2001 From: Mason Malone <651224+MasonM@users.noreply.github.com> Date: Tue, 19 Nov 2024 13:54:15 -0800 Subject: [PATCH 2/2] test: increase timeout Signed-off-by: Mason Malone <651224+MasonM@users.noreply.github.com> --- test/e2e/hooks_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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)