Repository: argoproj/argo
Pull request #3480 (2020-07-14 22:28:00)

fix: Merge WorkflowTemplateRef with defaults workflow spec
Checklist:

* [ ] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
* [ ] The title of the PR is (a) [conventional](https://www.conventionalcommits.org/en/v1.0.0/), (b) states what changed, and (c) suffixes the related issues number. E.g. `"fix(controller): Updates such and such. Fixes #1234"`.  #3355 #3349 
* [ ] I've signed the CLA.
* [ ] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [ ] My builds are green. Try syncing with master if they are not. 
* [ ] My organization is added to [USERS.md](https://github.com/argoproj/argo/blob/master/USERS.md).



Patch:
diff --git a/workflow/controller/controller.go b/workflow/controller/controller.go
index 99a2aaa54..1a24d9084 100644
--- a/workflow/controller/controller.go
+++ b/workflow/controller/controller.go
@@ -2,7 +2,6 @@ package controller
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"os"
 	"strconv"
@@ -20,7 +19,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/kubernetes"
@@ -765,19 +763,7 @@ func (wfc *WorkflowController) newPodInformer() cache.SharedIndexInformer {
 // The defaults for the workflow controller are set in the workflow-controller config map
 func (wfc *WorkflowController) setWorkflowDefaults(wf *wfv1.Workflow) error {
 	if wfc.Config.WorkflowDefaults != nil {
-		defaultsSpec, err := json.Marshal(*wfc.Config.WorkflowDefaults)
-		if err != nil {
-			return err
-		}
-		workflowBytes, err := json.Marshal(wf)
-		if err != nil {
-			return err
-		}
-		mergedWf, err := strategicpatch.StrategicMergePatch(defaultsSpec, workflowBytes, wfv1.Workflow{})
-		if err != nil {
-			return err
-		}
-		err = json.Unmarshal(mergedWf, &wf)
+		err := util.MergeTo(wfc.Config.WorkflowDefaults, wf)
 		if err != nil {
 			return err
 		}
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
index 5888c0760..49f75507e 100644
--- a/workflow/controller/operator.go
+++ b/workflow/controller/operator.go
@@ -2829,17 +2829,16 @@ func (woc *wfOperationCtx) loadExecutionSpec() (wfv1.TemplateReferenceHolder, wf
 		}
 	}
 
-	woc.wfSpec = woc.wf.Status.StoredWorkflowSpec
-
-	entrypoint := woc.wf.Spec.Entrypoint
-	if entrypoint == "" {
-		entrypoint = woc.wfSpec.Entrypoint
+	// Merge the workflow spec and storedWorkflowspec.
+	targetWf := wfv1.Workflow{Spec: woc.wf.Spec}
+	err := wfutil.MergeTo(&wfv1.Workflow{Spec: *woc.wf.Status.StoredWorkflowSpec}, &targetWf)
+	if err != nil {
+		return nil, executionParameters, err
 	}
+	woc.wfSpec = &targetWf.Spec
 
 	woc.volumes = woc.wfSpec.DeepCopy().Volumes
-
-	tmplRef := &wfv1.WorkflowStep{TemplateRef: woc.wf.Spec.WorkflowTemplateRef.ToTemplateRef(entrypoint)}
-
+	tmplRef := &wfv1.WorkflowStep{TemplateRef: woc.wf.Spec.WorkflowTemplateRef.ToTemplateRef(woc.wfSpec.Entrypoint)}
 	if len(woc.wfSpec.Arguments.Parameters) > 0 {
 		executionParameters.Parameters = util.MergeParameters(executionParameters.Parameters, woc.wfSpec.Arguments.Parameters)
 	}
diff --git a/workflow/util/merge.go b/workflow/util/merge.go
new file mode 100644
index 000000000..5371132db
--- /dev/null
+++ b/workflow/util/merge.go
@@ -0,0 +1,39 @@
+package util
+
+import (
+	"encoding/json"
+
+	"k8s.io/apimachinery/pkg/util/strategicpatch"
+
+	wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
+)
+
+// MergeTo will merge one workflow (the "patch" workflow) into another (the "target" workflow.
+// If the target workflow defines a field, this take precedence over the patch.
+func MergeTo(patch, target *wfv1.Workflow) error {
+	if target == nil || patch == nil {
+		return nil
+	}
+
+	patchWfBytes, err := json.Marshal(patch)
+	if err != nil {
+		return err
+	}
+
+	targetWfByte, err := json.Marshal(target)
+	if err != nil {
+		return err
+	}
+	var mergedWfByte []byte
+
+	mergedWfByte, err = strategicpatch.StrategicMergePatch(patchWfBytes, targetWfByte, wfv1.Workflow{})
+
+	if err != nil {
+		return err
+	}
+	err = json.Unmarshal(mergedWfByte, target)
+	if err != nil {
+		return err
+	}
+	return nil
+}
diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go
index a8c5f8412..eeb4bf2be 100644
--- a/workflow/validate/validate.go
+++ b/workflow/validate/validate.go
@@ -98,6 +98,12 @@ var wfTmplRefAllowedWfSpecValidFields = map[string]bool{
 	"Arguments":             true,
 	"WorkflowTemplateRef":   true,
 	"TTLStrategy":           true,
+	"Parallelism":           true,
+	"Volumes":               true,
+	"VolumeClaimTemplates":  true,
+	"NodeSelector":          true,
+	"PodGC":                 true,
+	"ServiceAccountName":    true,
 }
 
 type FakeArguments struct{}


Test patch:
diff --git a/workflow/controller/controller_test.go b/workflow/controller/controller_test.go
index 22c5dd460..03cb025cc 100644
--- a/workflow/controller/controller_test.go
+++ b/workflow/controller/controller_test.go
@@ -395,7 +395,7 @@ func TestCheckAndInitWorkflowTmplRef(t *testing.T) {
 		wf: wf}
 	_, _, err := woc.loadExecutionSpec()
 	assert.NoError(t, err)
-	assert.Equal(t, &wftmpl.Spec.WorkflowSpec, woc.wfSpec)
+	assert.Equal(t, wftmpl.Spec.WorkflowSpec.Templates, woc.wfSpec.Templates)
 }
 
 func TestIsArchivable(t *testing.T) {
diff --git a/workflow/controller/operator_workflow_template_ref_test.go b/workflow/controller/operator_workflow_template_ref_test.go
index c88a23a60..5976a17a9 100644
--- a/workflow/controller/operator_workflow_template_ref_test.go
+++ b/workflow/controller/operator_workflow_template_ref_test.go
@@ -15,7 +15,8 @@ func TestWorkflowTemplateRef(t *testing.T) {
 	defer cancel()
 	woc := newWorkflowOperationCtx(unmarshalWF(wfWithTmplRef), controller)
 	woc.operate()
-	assert.Equal(t, &unmarshalWFTmpl(wfTmpl).Spec.WorkflowSpec, woc.wfSpec)
+	assert.Equal(t, unmarshalWFTmpl(wfTmpl).Spec.WorkflowSpec.Templates, woc.wfSpec.Templates)
+	assert.Equal(t, woc.wf.Spec.Entrypoint, woc.wfSpec.Entrypoint)
 	// verify we copy these values
 	assert.Len(t, woc.volumes, 1, "volumes from workflow template")
 	// and these
diff --git a/workflow/util/merge_test.go b/workflow/util/merge_test.go
new file mode 100644
index 000000000..f6f4b20a4
--- /dev/null
+++ b/workflow/util/merge_test.go
@@ -0,0 +1,50 @@
+package util
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var origWF = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: workflow-template-hello-world-
+spec:
+  arguments:
+    parameters:
+    - name: message
+      value: original
+  entrypoint: start
+  onExit: end
+  serviceAccountName: argo
+  workflowTemplateRef:
+    name: workflow-template-submittable
+`
+var patchWF = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: workflow-template-hello-world-
+spec:
+  arguments:
+    parameters:
+    - name: message
+      value: patch
+  serviceAccountName: argo1
+  podGC:
+    strategy: OnPodSuccess
+`
+
+func TestMergeWorkflows(t *testing.T) {
+	patchWf := unmarshalWF(origWF)
+	targetWf := unmarshalWF(patchWF)
+
+	err := MergeTo(patchWf, targetWf)
+	assert.NoError(t, err)
+	assert.Equal(t, "start", targetWf.Spec.Entrypoint)
+	assert.Equal(t, "argo1", targetWf.Spec.ServiceAccountName)
+	assert.Equal(t, "message", targetWf.Spec.Arguments.Parameters[0].Name)
+	assert.Equal(t, "patch", targetWf.Spec.Arguments.Parameters[0].Value.StrVal)
+}
diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go
index d74c043e8..e70b71dd2 100644
--- a/workflow/validate/validate_test.go
+++ b/workflow/validate/validate_test.go
@@ -2413,6 +2413,15 @@ metadata:
   generateName: hello-world-
 spec:
   entrypoint: A
+  serviceAccountName: argo
+  parallelism: 1
+  volumes:
+  - name: workdir
+    emptyDir: {}
+  podGC:
+    strategy: OnPodSuccess
+  nodeSelector:
+    beta.kubernetes.io/arch: "{{inputs.parameters.arch}}"
   arguments:
     parameters:
     - name: lines-count