fix(controller): Fix podSpecPatch (#5360)

Signed-off-by: Alex Collins <[email protected]>
argoproj · Mar 12, 2021 · d828717 · d828717
1 parent 2d331f3
commit d828717
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 36 deletions.
diff --git a/test/e2e/cli_test.go b/test/e2e/cli_test.go
@@ -768,7 +768,7 @@ func (s *CLISuite) TestWorkflowRetry() {
 		WaitForWorkflow(fixtures.Condition(func(wf *wfv1.Workflow) (bool, string) {
 			retryTime = wf.Status.FinishedAt
 			return wf.Status.Phase == wfv1.WorkflowFailed, "is terminated"
-		}), 20*time.Second).
+		})).
 		Wait(3*time.Second).
 		RunCli([]string{"retry", "retry-test", "--restart-successful", "--node-field-selector", "templateName==steps-inner"}, func(t *testing.T, output string, err error) {
 			if assert.NoError(t, err, output) {

diff --git a/test/e2e/manifests/mixins/workflow-controller-configmap.yaml b/test/e2e/manifests/mixins/workflow-controller-configmap.yaml
@@ -8,6 +8,8 @@ data:
       activeDeadlineSeconds: 300
       ttlStrategy:
         secondsAfterCompletion: 600
+      podSpecPatch: |
+        terminationGracePeriodSeconds: 3
   executor: |
     imagePullPolicy: IfNotPresent
     resources:

diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go
@@ -382,10 +382,8 @@ func (woc *wfOperationCtx) createWorkflowPod(ctx context.Context, nodeName strin
 			return nil, errors.Wrap(err, "", "Fail to substitute the PodSpecPatch variables")
 		}
 
-		var spec apiv1.PodSpec
-
-		if !util.ValidateJsonStr(tmpl.PodSpecPatch, spec) {
-			return nil, errors.New("", "Invalid PodSpecPatch String")
+		if err := json.Unmarshal([]byte(tmpl.PodSpecPatch), &apiv1.PodSpec{}); err != nil {
+			return nil, fmt.Errorf("invalid podSpecPatch %q: %w", tmpl.PodSpecPatch, err)
 		}
 
 		modJson, err := strategicpatch.StrategicMergePatch(jsonstr, []byte(tmpl.PodSpecPatch), apiv1.PodSpec{})

diff --git a/workflow/util/util.go b/workflow/util/util.go
@@ -1004,39 +1004,16 @@ func ConvertYAMLToJSON(str string) (string, error) {
 
 // PodSpecPatchMerge will do strategic merge the workflow level PodSpecPatch and template level PodSpecPatch
 func PodSpecPatchMerge(wf *wfv1.Workflow, tmpl *wfv1.Template) (string, error) {
-	var wfPatch, tmplPatch, mergedPatch string
-	var err error
-
-	if wf.Spec.HasPodSpecPatch() {
-		wfPatch, err = ConvertYAMLToJSON(wf.Spec.PodSpecPatch)
-		if err != nil {
-			return "", err
-		}
+	wfPatch, err := ConvertYAMLToJSON(wf.Spec.PodSpecPatch)
+	if err != nil {
+		return "", err
 	}
-	if tmpl.HasPodSpecPatch() {
-		tmplPatch, err = ConvertYAMLToJSON(tmpl.PodSpecPatch)
-		if err != nil {
-			return "", err
-		}
-
-		if wfPatch != "" {
-			mergedByte, err := strategicpatch.StrategicMergePatch([]byte(wfPatch), []byte(tmplPatch), apiv1.PodSpec{})
-			if err != nil {
-				return "", err
-			}
-			mergedPatch = string(mergedByte)
-		} else {
-			mergedPatch = tmplPatch
-		}
-	} else {
-		mergedPatch = wfPatch
+	tmplPatch, err := ConvertYAMLToJSON(tmpl.PodSpecPatch)
+	if err != nil {
+		return "", err
 	}
-	return mergedPatch, nil
-}
-
-func ValidateJsonStr(jsonStr string, schema interface{}) bool {
-	err := json.Unmarshal([]byte(jsonStr), &schema)
-	return err == nil
+	data, err := strategicpatch.StrategicMergePatch([]byte(wfPatch), []byte(tmplPatch), apiv1.PodSpec{})
+	return string(data), err
 }
 
 func GetNodeType(tmpl *wfv1.Template) wfv1.NodeType {