Skip to content

Commit

Permalink
fix: consistent variable substitution for configMapKeyRef. Fixes #13890
Browse files Browse the repository at this point in the history 
Support for subsituting variables in `configMapKeyRef` was added in
#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 <[email protected]>
  • Loading branch information
@MasonM
MasonM committed Nov 19, 2024
1 parent 2fd5488 commit 4ee0f0b
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 98 deletions.
33 changes: 17 additions & 16 deletions workflow/common/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
141 changes: 59 additions & 82 deletions workflow/common/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}}"
`
)

Expand Down Expand Up @@ -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)
}
})
}
}

Expand Down

0 comments on commit 4ee0f0b

Please sign in to comment.