Skip to content

Commit

Permalink
fix: valueFrom in template parameter should be overridable. Fixes 101…
Browse files Browse the repository at this point in the history 
…82 (#10281)

Signed-off-by: LilTwo <w0005151@gmail.com>
  • Loading branch information
@LilTwo @terrytangyuan
LilTwo authored and terrytangyuan committed Aug 11, 2023
1 parent 9e1d1e5 commit 5797668
Show file tree
Hide file tree
Showing 7 changed files with 384 additions and 49 deletions.
203 changes: 203 additions & 0 deletions test/e2e/workflow_inputs_orverridable_test.go
View file
@@ -0,0 +1,203 @@
//go:build functional
// +build functional

package e2e

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/suite"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
"github.com/argoproj/argo-workflows/v3/test/e2e/fixtures"
)

type WorkflowInputsOverridableSuite struct {
fixtures.E2ESuite
}

func (s *WorkflowInputsOverridableSuite) TestArgsValueParamsOverrideInputParamsValueFrom() {
s.Given().
Workflow(`apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: workflow-inputs-overridable-wf-
label:
workflows.argoproj.io/test: "true"
spec:
serviceAccountName: argo
entrypoint: whalesay
arguments:
parameters:
- name: message
value: arg-value
templates:
- name: whalesay
container:
image: argoproj/argosay:v2
args:
- echo
- "{{inputs.parameters.message}}"
inputs:
parameters:
- name: message
valueFrom:
configMapKeyRef:
name: cmref-parameters
key: cmref-key
`).
When().
CreateConfigMap(
"cmref-parameters",
map[string]string{"cmref-key": "input-value"},
map[string]string{"workflows.argoproj.io/configmap-type": "Parameter"}).
SubmitWorkflow().
WaitForWorkflow(fixtures.ToBeSucceeded).
DeleteConfigMap("cmref-parameters").
Then().
ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
assert.Equal(t, "arg-value", status.Nodes[metadata.Name].Inputs.Parameters[0].Value.String())
assert.Equal(t, wfv1.WorkflowSucceeded, status.Phase)
})
}

func (s *WorkflowInputsOverridableSuite) TestArgsValueFromParamsOverrideInputParamsValueFrom() {
s.Given().
Workflow(`apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: workflow-inputs-overridable-wf-
label:
workflows.argoproj.io/test: "true"
spec:
serviceAccountName: argo
entrypoint: whalesay
arguments:
parameters:
- name: message
valueFrom:
configMapKeyRef:
name: new-cmref-parameters
key: cmref-key
templates:
- name: whalesay
container:
image: argoproj/argosay:v2
args:
- echo
- "{{inputs.parameters.message}}"
inputs:
parameters:
- name: message
valueFrom:
configMapKeyRef:
name: cmref-parameters
key: cmref-key
`).
When().
CreateConfigMap(
"cmref-parameters",
map[string]string{"cmref-key": "input-value"},
map[string]string{"workflows.argoproj.io/configmap-type": "Parameter"}).
CreateConfigMap(
"new-cmref-parameters",
map[string]string{"cmref-key": "arg-value"},
map[string]string{"workflows.argoproj.io/configmap-type": "Parameter"}).
SubmitWorkflow().
WaitForWorkflow(fixtures.ToBeSucceeded).
DeleteConfigMap("cmref-parameters").
DeleteConfigMap("new-cmref-parameters").
Then().
ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
assert.Equal(t, "arg-value", status.Nodes[metadata.Name].Inputs.Parameters[0].Value.String())
assert.Equal(t, wfv1.WorkflowSucceeded, status.Phase)
})
}

func (s *WorkflowInputsOverridableSuite) TestArgsValueParamsOverrideInputParamsValue() {
s.Given().
Workflow(`apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: workflow-template-wf-
label:
workflows.argoproj.io/test: "true"
spec:
serviceAccountName: argo
entrypoint: whalesay
arguments:
parameters:
- name: message
value: arg-value
templates:
- name: whalesay
container:
image: argoproj/argosay:v2
args:
- echo
- "{{inputs.parameters.message}}"
inputs:
parameters:
- name: message
value: input-value
`).
When().
SubmitWorkflow().
WaitForWorkflow(fixtures.ToBeSucceeded).
Then().
ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
assert.Equal(t, "arg-value", status.Nodes[metadata.Name].Inputs.Parameters[0].Value.String())
assert.Equal(t, wfv1.WorkflowSucceeded, status.Phase)
})
}

func (s *WorkflowInputsOverridableSuite) TestArgsValueFromParamsOverrideInputParamsValue() {
s.Given().
Workflow(`apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: workflow-inputs-overridable-wf-
label:
workflows.argoproj.io/test: "true"
spec:
serviceAccountName: argo
entrypoint: whalesay
arguments:
parameters:
- name: message
valueFrom:
configMapKeyRef:
name: cmref-parameters
key: cmref-key
templates:
- name: whalesay
container:
image: argoproj/argosay:v2
args:
- echo
- "{{inputs.parameters.message}}"
inputs:
parameters:
- name: message
value: input-value
`).
When().
CreateConfigMap(
"cmref-parameters",
map[string]string{"cmref-key": "arg-value"},
map[string]string{"workflows.argoproj.io/configmap-type": "Parameter"}).
SubmitWorkflow().
WaitForWorkflow(fixtures.ToBeSucceeded).
DeleteConfigMap("cmref-parameters").
Then().
ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
assert.Equal(t, "arg-value", status.Nodes[metadata.Name].Inputs.Parameters[0].Value.String())
assert.Equal(t, wfv1.WorkflowSucceeded, status.Phase)
})
}

func TestWorkflowInputsOverridableSuiteSuite(t *testing.T) {
suite.Run(t, new(WorkflowInputsOverridableSuite))
}
9 changes: 6 additions & 3 deletions workflow/common/configmap.go
View file
Expand Up @@ -4,14 +4,17 @@ import (
"fmt"

apiv1 "k8s.io/api/core/v1"
"k8s.io/client-go/tools/cache"

"github.com/argoproj/argo-workflows/v3/errors"
)

type ConfigMapStore interface {
GetByKey(key string) (interface{}, bool, error)
}

// GetConfigMapValue retrieves a configmap value
func GetConfigMapValue(configMapInformer cache.SharedIndexInformer, namespace, name, key string) (string, error) {
obj, exists, err := configMapInformer.GetIndexer().GetByKey(namespace + "/" + name)
func GetConfigMapValue(configMapStore ConfigMapStore, namespace, name, key string) (string, error) {
obj, exists, err := configMapStore.GetByKey(namespace + "/" + name)
if err != nil {
return "", err
}
Expand Down
104 changes: 62 additions & 42 deletions workflow/common/util.go
View file
Expand Up @@ -14,7 +14,6 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/remotecommand"

"github.com/argoproj/argo-workflows/v3/errors"
Expand Down Expand Up @@ -112,61 +111,82 @@ func GetExecutorOutput(exec remotecommand.Executor) (*bytes.Buffer, *bytes.Buffe
return &stdOut, &stdErr, nil
}

func overwriteWithDefaultParams(inParam *wfv1.Parameter) {
if inParam.Value == nil && inParam.Default != nil {
inParam.Value = inParam.Default
}
}

func overwriteWithArguments(argParam, inParam *wfv1.Parameter) {
if argParam != nil {
if argParam.Value != nil {
inParam.Value = argParam.Value
inParam.ValueFrom = nil
} else {
inParam.ValueFrom = argParam.ValueFrom
inParam.Value = nil
}
}
}

func substituteAndGetConfigMapValue(inParam *wfv1.Parameter, globalParams Parameters, namespace string, configMapStore ConfigMapStore) error {
if inParam.ValueFrom != nil && inParam.ValueFrom.ConfigMapKeyRef != nil {
if configMapStore != nil {
// 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)
if err != nil {
log.WithError(err).Error("unable to substitute name for ConfigMapKeyRef")
return err
}
cmKey, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Key, globalParams)
if err != nil {
log.WithError(err).Error("unable to substitute key for ConfigMapKeyRef")
return err
}

cmValue, err := GetConfigMapValue(configMapStore, namespace, cmName, cmKey)
if err != nil {
if inParam.ValueFrom.Default != nil && errors.IsCode(errors.CodeNotFound, err) {
inParam.Value = inParam.ValueFrom.Default
} else {
return errors.Errorf(errors.CodeBadRequest, "unable to retrieve inputs.parameters.%s from ConfigMap: %s", inParam.Name, err)
}
} else {
inParam.Value = wfv1.AnyStringPtr(cmValue)
}
}
} else {
if inParam.Value == nil {
return errors.Errorf(errors.CodeBadRequest, "inputs.parameters.%s was not supplied", inParam.Name)
}
}
return nil
}

// ProcessArgs sets in the inputs, the values either passed via arguments, or the hardwired values
// It substitutes:
// * parameters in the template from the arguments
// * global parameters (e.g. {{workflow.parameters.XX}}, {{workflow.name}}, {{workflow.status}})
// * local parameters (e.g. {{pod.name}})
func ProcessArgs(tmpl *wfv1.Template, args wfv1.ArgumentsProvider, globalParams, localParams Parameters, validateOnly bool, namespace string, configMapInformer cache.SharedIndexInformer) (*wfv1.Template, error) {
func ProcessArgs(tmpl *wfv1.Template, args wfv1.ArgumentsProvider, globalParams, localParams Parameters, validateOnly bool, namespace string, configMapStore ConfigMapStore) (*wfv1.Template, error) {
// For each input parameter:
// 1) check if was supplied as argument. if so use the supplied value from arg
// 2) if not, use default value.
// 3) if no default value, it is an error
newTmpl := tmpl.DeepCopy()
for i, inParam := range newTmpl.Inputs.Parameters {
if inParam.Value == nil && inParam.Default != nil {
// first set to default value
inParam.Value = inParam.Default
}
// first set to default value
overwriteWithDefaultParams(&inParam)

// overwrite value from argument (if supplied)
argParam := args.GetParameterByName(inParam.Name)
if argParam != nil {
if argParam.Value != nil {
inParam.Value = argParam.Value
} else {
inParam.ValueFrom = argParam.ValueFrom
}
}
if inParam.ValueFrom != nil && inParam.ValueFrom.ConfigMapKeyRef != nil {
if configMapInformer != nil {
// 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)
if err != nil {
log.WithError(err).Error("unable to substitute name for ConfigMapKeyRef")
return nil, err
}
cmKey, err := substituteConfigMapKeyRefParam(inParam.ValueFrom.ConfigMapKeyRef.Key, globalParams)
if err != nil {
log.WithError(err).Error("unable to substitute key for ConfigMapKeyRef")
return nil, err
}
overwriteWithArguments(argParam, &inParam)

cmValue, err := GetConfigMapValue(configMapInformer, namespace, cmName, cmKey)
if err != nil {
if inParam.ValueFrom.Default != nil && errors.IsCode(errors.CodeNotFound, err) {
inParam.Value = inParam.ValueFrom.Default
} else {
return nil, errors.Errorf(errors.CodeBadRequest, "unable to retrieve inputs.parameters.%s from ConfigMap: %s", inParam.Name, err)
}
} else {
inParam.Value = wfv1.AnyStringPtr(cmValue)
}
}
} else {
if inParam.Value == nil {
return nil, errors.Errorf(errors.CodeBadRequest, "inputs.parameters.%s was not supplied", inParam.Name)
}
// substitute configmap string and get value from store
err := substituteAndGetConfigMapValue(&inParam, globalParams, namespace, configMapStore)
if err != nil {
return nil, err
}

newTmpl.Inputs.Parameters[i] = inParam
Expand Down

0 comments on commit 5797668

Please sign in to comment.