fix: Allow valueFrom in dag arguments parameters. Fixes #11900 (#11902)

Signed-off-by: nice-pink <r@nice.pink>
argoproj · Feb 5, 2024 · fbd70aa · fbd70aa
1 parent 4670b21
commit fbd70aa
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 11 deletions.
diff --git a/server/workflow/workflow_server_test.go b/server/workflow/workflow_server_test.go
@@ -929,7 +929,7 @@ func TestSubmitWorkflowFromResource(t *testing.T) {
 			ResourceKind: "workflowtemplate",
 			ResourceName: "workflow-template-whalesay-template",
 		})
-		assert.EqualError(t, err, "rpc error: code = InvalidArgument desc = spec.arguments.message.value is required")
+		assert.EqualError(t, err, "rpc error: code = InvalidArgument desc = spec.arguments.message.value or spec.arguments.message.valueFrom is required")
 	})
 	t.Run("SubmitFromWorkflowTemplate", func(t *testing.T) {
 		opts := v1alpha1.SubmitOpts{

diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go
@@ -4453,7 +4453,7 @@ func TestUnsuppliedArgValue(t *testing.T) {
 	woc := newWorkflowOperationCtx(wf, controller)
 	woc.operate(ctx)
 	assert.Equal(t, woc.wf.Status.Conditions[0].Status, metav1.ConditionStatus("True"))
-	assert.Equal(t, woc.wf.Status.Message, "invalid spec: spec.arguments.missing.value is required")
+	assert.Equal(t, woc.wf.Status.Message, "invalid spec: spec.arguments.missing.value or spec.arguments.missing.valueFrom is required")
 }
 
 var suppliedArgValue = `

diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
@@ -856,11 +856,24 @@ func validateArgumentsFieldNames(prefix string, arguments wfv1.Arguments) error
 // validateArgumentsValues ensures that all arguments have parameter values or artifact locations
 func validateArgumentsValues(prefix string, arguments wfv1.Arguments, allowEmptyValues bool) error {
 	for _, param := range arguments.Parameters {
+		// check if any value is defined
 		if param.ValueFrom == nil && param.Value == nil {
 			if !allowEmptyValues {
-				return errors.Errorf(errors.CodeBadRequest, "%s%s.value is required", prefix, param.Name)
+				return errors.Errorf(errors.CodeBadRequest, "%s%s.value or %s%s.valueFrom is required", prefix, param.Name, prefix, param.Name)
+			}
+		}
+		if param.ValueFrom != nil {
+			// check for valid valueFrom sub-parameters
+			// INFO: default needs to be accompanied by ConfigMapKeyRef.
+			if param.ValueFrom.ConfigMapKeyRef == nil && param.ValueFrom.Event == "" && param.ValueFrom.Supplied == nil {
+				return errors.Errorf(errors.CodeBadRequest, "%s%s.valueFrom only allows: default, configMapKeyRef and supplied", prefix, param.Name)
+			}
+			// check for invalid valueFrom sub-parameters
+			if param.ValueFrom.Path != "" || param.ValueFrom.JSONPath != "" || param.ValueFrom.Parameter != "" || param.ValueFrom.Expression != "" {
+				return errors.Errorf(errors.CodeBadRequest, "%s%s.valueFrom only allows: default, configMapKeyRef and supplied", prefix, param.Name)
 			}
 		}
+		// validate enum
 		if param.Enum != nil {
 			if len(param.Enum) == 0 {
 				return errors.Errorf(errors.CodeBadRequest, "%s%s.enum should contain at least one value", prefix, param.Name)
@@ -1423,10 +1436,7 @@ func validateDAGTaskArgumentDependency(arguments wfv1.Arguments, ancestry []stri
 	}
 
 	for _, param := range arguments.Parameters {
-		if param.Value == nil {
-			return errors.Errorf(errors.CodeBadRequest, "missing value for parameter '%s'", param.Name)
-		}
-		if strings.HasPrefix(param.Value.String(), "{{tasks.") {
+		if param.Value != nil && strings.HasPrefix(param.Value.String(), "{{tasks.") {
 			// All parameter values should have been validated, so
 			// index 1 should exist.
 			refTaskName := strings.Split(param.Value.String(), ".")[1]

diff --git a/workflow/validate/validate_dag_test.go b/workflow/validate/validate_dag_test.go
@@ -1075,6 +1075,71 @@ spec:
 func TestDAGMissingParamValueInTask(t *testing.T) {
 	err := validate(dagMissingParamValueInTask)
 	if assert.NotNil(t, err) {
-		assert.Contains(t, err.Error(), "templates.root.tasks.task missing value for parameter 'data'")
+		assert.Contains(t, err.Error(), ".valueFrom only allows: default, configMapKeyRef and supplied")
+	}
+}
+
+var dagArgParamValueFromConfigMapInTask = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+spec:
+  entrypoint: root
+  templates:
+    - name: template
+      inputs:
+        parameters:
+          - name: data
+      container:
+        name: main
+        image: alpine
+    - name: root
+      dag:
+        tasks:
+          - name: task
+            template: template
+            arguments:
+              parameters:
+                - name: data
+                  valueFrom:
+                    configMapKeyRef:
+                      name: my-config
+                      key: my-data
+                    default: my-default
+`
+
+func TestDAGArgParamValueFromConfigMapInTask(t *testing.T) {
+	err := validate(dagArgParamValueFromConfigMapInTask)
+	assert.NoError(t, err)
+}
+
+var failDagArgParamValueFromPathInTask = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+spec:
+  entrypoint: root
+  templates:
+    - name: template
+      inputs:
+        parameters:
+          - name: data
+      container:
+        name: main
+        image: alpine
+    - name: root
+      dag:
+        tasks:
+          - name: task
+            template: template
+            arguments:
+              parameters:
+                - name: data
+                  valueFrom:
+                    path: /tmp/my-path
+`
+
+func TestFailDAGArgParamValueFromPathInTask(t *testing.T) {
+	err := validate(failDagArgParamValueFromPathInTask)
+	if assert.NotNil(t, err) {
+		assert.Contains(t, err.Error(), "valueFrom only allows: default, configMapKeyRef and supplied")
 	}
 }
diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go
@@ -641,7 +641,7 @@ func TestGlobalParam(t *testing.T) {
 	assert.NoError(t, err)
 
 	err = validate(unsuppliedArgValue)
-	assert.EqualError(t, err, "spec.arguments.missing.value is required")
+	assert.EqualError(t, err, "spec.arguments.missing.value or spec.arguments.missing.valueFrom is required")
 }
 
 var invalidTemplateNames = `
@@ -1293,7 +1293,8 @@ spec:
 func TestInvalidArgumentNoValue(t *testing.T) {
 	err := validate(invalidArgumentNoValue)
 	if assert.NotNil(t, err) {
-		assert.Contains(t, err.Error(), ".value is required")
+		assert.Contains(t, err.Error(), ".value or ")
+		assert.Contains(t, err.Error(), ".valueFrom is required")
 	}
 }
 
@@ -2714,7 +2715,7 @@ func TestWorkflowTemplateWithEnumValueWithoutValue(t *testing.T) {
 	err = validateWorkflowTemplate(workflowTeamplateWithEnumValuesWithoutValue, ValidateOpts{Lint: true})
 	assert.Nil(t, err)
 	err = validateWorkflowTemplate(workflowTeamplateWithEnumValuesWithoutValue, ValidateOpts{Submit: true})
-	assert.EqualError(t, err, "spec.arguments.message.value is required")
+	assert.EqualError(t, err, "spec.arguments.message.value or spec.arguments.message.valueFrom is required")
 }
 
 var resourceManifestWithExpressions = `