Repository: argoproj/argo (go)

Issue:
Panic on addOutputsToLocalScope
Checklist:

 * [x] I've included the version.
 * [ ] I've included reproduction steps.
 * [ ] I've included the workflow YAML.
 * [x] I've included the logs.
 
**What happened**: 
I'm not sure if stable is also non-viable or is it just me, but I'm getting
```
runtime error: invalid memory address or nil pointer dereference" stack="goroutine 343 [running]:
runtime/debug.Stack(0xc05fda1f51, 0x173d180, 0x28720d0)
  /usr/local/go/src/runtime/debug/stack.go:24 +0x9d
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).operate.func2(0xc0023d40b0)
  /go/src/github.com/argoproj/argo/workflow/controller/operator.go:164 +0xc4
panic(0x173d180, 0x28720d0)
  /usr/local/go/src/runtime/panic.go:679 +0x1b2
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).addOutputsToLocalScope(0xc0023d40b0, 0xc002a460a0, 0x15, 0xc0024fa540, 0xc002071a90)
  /go/src/github.com/argoproj/argo/workflow/controller/operator.go:2072 +0x493
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).buildLocalScope(0xc0023d40b0, 0xc002071a90, 0xc002a460a0, 0x15, 0xc0023ddb80)
 /go/src/github.com/argoproj/argo/workflow/controller/operator.go:2050 +0xaa
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).buildLocalScopeFromTask(0xc0023d40b0, 0xc0018ddea0, 0xc001001ce0, 0x0, 0x0, 0x0)
  /go/src/github.com/argoproj/argo/workflow/controller/dag.go:475 +0x24c
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).resolveDependencyReferences(0xc0023d40b0, 0xc0018ddea0, 0xc001001ce0, 0xf, 0x0, 0x0)
  /go/src/github.com/argoproj/argo/workflow/controller/dag.go:485 +0x5d
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).executeDAGTask(0xc0023d40b0, 0xc0018ddea0, 0xc003465870, 0xf)
  /go/src/github.com/argoproj/argo/workflow/controller/dag.go:372 +0x7e6
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).executeDAG(0xc0023d40b0, 0xc002520160, 0x1f, 0xc0024faa00, 0xc0005167b0, 0x25, 0xc0048778c0, 0x1bba5a0, 0xc0023d4160, 0xc00243def0, ...)
  /go/src/github.com/argoproj/argo/workflow/controller/dag.go:231 +0x2d6
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).executeTemplate(0xc0023d40b0, 0xc002520160, 0x1f, 0x1bba5a0, 0xc0023d4160, 0xc0024fa980, 0x0, 0x0, 0x0, 0x0, ...)
  /go/src/github.com/argoproj/argo/workflow/controller/operator.go:1517 +0x1de4
github.com/argoproj/argo/workflow/controller.(*wfOperationCtx).operate(0xc0023d40b0)
  /go/src/github.com/argoproj/argo/workflow/controller/operator.go:290 +0x125d
github.com/argoproj/argo/workflow/controller.(*WorkflowController).processNextItem(0xc000097c00, 0x0)
 /go/src/github.com/argoproj/argo/workflow/controller/controller.go:433 +0x8c2
github.com/argoproj/argo/workflow/controller.(*WorkflowController).runWorker(0xc000097c00)
 /go/src/github.com/argoproj/argo/workflow/controller/controller.go:359 +0x2b
k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1(0xc002e4f020)
  /go/pkg/mod/k8s.io/apimachinery@v0.16.7-beta.0/pkg/util/wait/wait.go:152 +0x5e
k8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc002e4f020, 0x3b9aca00, 0x0, 0x1, 0xc0000abc80)
  /go/pkg/mod/k8s.io/apimachinery@v0.16.7-beta.0/pkg/util/wait/wait.go:153 +0xf8
k8s.io/apimachinery/pkg/util/wait.Until(0xc002e4f020, 0x3b9aca00, 0xc0000abc80)
  /go/pkg/mod/k8s.io/apimachinery@v0.16.7-beta.0/pkg/util/wait/wait.go:88 +0x4d
created by github.com/argoproj/argo/workflow/controller.(*WorkflowController).Run
 /go/src/github.com/argoproj/argo/workflow/controller/controller.go:172 +0x975
```
with the stable build.

Not sure if there's any change I need to make in my workflow definition.

**What you expected to happen**: no panic

**How to reproduce it (as minimally and precisely as possible)**: 

**Anything else we need to know?**: I upgraded from 2.8.0 to. stable and all workflows that involve output passing failed with this panic.

**Environment**:

- Argo version: 
```
$ argo version
argo: v2.8.0
  BuildDate: 2020-05-11T22:55:43Z
  GitCommit: 8f696174746ed01b9bf1941ad03da62d312df641
  GitTreeState: clean
  GitTag: v2.8.0
  GoVersion: go1.13.4
  Compiler: gc
  Platform: darwin/amd64
```
- Kubernetes version :
```
$ kubectl version -o yaml
clientVersion:
  buildDate: "2020-02-13T18:08:14Z"
  compiler: gc
  gitCommit: 06ad960bfd03b39c8310aaf92d1e7c12ce618213
  gitTreeState: clean
  gitVersion: v1.17.3
  goVersion: go1.13.8
  major: "1"
  minor: "17"
  platform: darwin/amd64
serverVersion:
  buildDate: "2020-05-29T15:41:17Z"
  compiler: gc
  gitCommit: b20d34dbc46f336ed52ea4c01356da9952f0b8fc
  gitTreeState: clean
  gitVersion: v1.15.11-gke.17
  goVersion: go1.12.17b4
  major: "1"
  minor: 15+
  platform: linux/amd64
```

---
<!-- Issue Author: Don't delete this message to encourage other users to support your issue! -->
**Message from the maintainers**:

If you are impacted by this bug please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.


Pull request description:
fix(controller): Do not panic on nil output value. Fixes #3505
Checklist:

* [x] 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.
* [x] 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"`.  
* [x] I've signed the CLA.
* [x] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [x] My builds are green. Try syncing with master if they are not. 
* [x] My organization is added to [USERS.md](https://github.com/argoproj/argo/blob/master/USERS.md).

Fixes #3505


Patch:
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
index bbb122dd3..076db614b 100644
--- a/workflow/controller/operator.go
+++ b/workflow/controller/operator.go
@@ -2149,32 +2149,22 @@ func (woc *wfOperationCtx) buildLocalScope(scope *wfScope, prefix string, node *
 }
 
 func (woc *wfOperationCtx) addOutputsToLocalScope(prefix string, outputs *wfv1.Outputs, scope *wfScope) {
-	if outputs == nil {
+	if outputs == nil || scope == nil {
 		return
 	}
 	if prefix != "workflow" && outputs.Result != nil {
-		key := fmt.Sprintf("%s.outputs.result", prefix)
-		if scope != nil {
-			scope.addParamToScope(key, *outputs.Result)
-		}
+		scope.addParamToScope(fmt.Sprintf("%s.outputs.result", prefix), *outputs.Result)
 	}
 	if prefix != "workflow" && outputs.ExitCode != nil {
-		key := fmt.Sprintf("%s.exitCode", prefix)
-		if scope != nil {
-			scope.addParamToScope(key, *outputs.ExitCode)
-		}
+		scope.addParamToScope(fmt.Sprintf("%s.exitCode", prefix), *outputs.ExitCode)
 	}
 	for _, param := range outputs.Parameters {
-		key := fmt.Sprintf("%s.outputs.parameters.%s", prefix, param.Name)
-		if scope != nil {
-			scope.addParamToScope(key, param.Value.String())
+		if param.Value != nil {
+			scope.addParamToScope(fmt.Sprintf("%s.outputs.parameters.%s", prefix, param.Name), param.Value.String())
 		}
 	}
 	for _, art := range outputs.Artifacts {
-		key := fmt.Sprintf("%s.outputs.artifacts.%s", prefix, art.Name)
-		if scope != nil {
-			scope.addArtifactToScope(key, art)
-		}
+		scope.addArtifactToScope(fmt.Sprintf("%s.outputs.artifacts.%s", prefix, art.Name), art)
 	}
 }
 


Test patch:
diff --git a/test/test.go b/test/test.go
index 836c61743..03e79a3ea 100644
--- a/test/test.go
+++ b/test/test.go
@@ -5,10 +5,8 @@ import (
 	"path/filepath"
 	"runtime"
 
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"sigs.k8s.io/yaml"
-
 	wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
+	"github.com/argoproj/argo/test/util"
 )
 
 var (
@@ -43,12 +41,9 @@ func LoadTestWorkflow(path string) *wfv1.Workflow {
 
 // LoadWorkflowFromBytes returns a workflow unmarshalled from an yaml byte array
 func LoadWorkflowFromBytes(yamlBytes []byte) *wfv1.Workflow {
-	var wf wfv1.Workflow
-	err := yaml.Unmarshal(yamlBytes, &wf)
-	if err != nil {
-		panic(err)
-	}
-	return &wf
+	v := &wfv1.Workflow{}
+	util.MustUnmarshallYAML(string(yamlBytes), v)
+	return v
 }
 
 // LoadTestWorkflow returns a workflow relative to the test file
@@ -62,20 +57,13 @@ func LoadTestWorkflowTemplate(path string) *wfv1.WorkflowTemplate {
 
 // LoadWorkflowFromBytes returns a workflow unmarshalled from an yaml byte array
 func LoadWorkflowTemplateFromBytes(yamlBytes []byte) *wfv1.WorkflowTemplate {
-	var wf wfv1.WorkflowTemplate
-	err := yaml.Unmarshal(yamlBytes, &wf)
-	if err != nil {
-		panic(err)
-	}
-	return &wf
+	v := &wfv1.WorkflowTemplate{}
+	util.MustUnmarshallYAML(string(yamlBytes), v)
+	return v
 }
 
-// LoadUnstructuredFromBytes returns an Unstructured unmarshalled from an yaml byte array
-func LoadUnstructuredFromBytes(yamlBytes []byte) *unstructured.Unstructured {
-	var un unstructured.Unstructured
-	err := yaml.Unmarshal(yamlBytes, &un)
-	if err != nil {
-		panic(err)
-	}
-	return &un
+func LoadClusterWorkflowTemplateFromBytes(yamlBytes []byte) *wfv1.ClusterWorkflowTemplate {
+	v := &wfv1.ClusterWorkflowTemplate{}
+	util.MustUnmarshallYAML(string(yamlBytes), v)
+	return v
 }
diff --git a/test/util/yaml.go b/test/util/yaml.go
index ec44000b3..5ae109aa0 100644
--- a/test/util/yaml.go
+++ b/test/util/yaml.go
@@ -1,9 +1,16 @@
 package util
 
-import "sigs.k8s.io/yaml"
+import (
+	log "github.com/sirupsen/logrus"
+	"sigs.k8s.io/yaml"
+)
 
 func MustUnmarshallYAML(text string, v interface{}) {
-	err := yaml.Unmarshal([]byte(text), v)
+	err := yaml.UnmarshalStrict([]byte(text), v)
+	if err != nil {
+		log.Warn("invalid YAML: %w", err)
+		err = yaml.Unmarshal([]byte(text), v)
+	}
 	if err != nil {
 		panic(err)
 	}
diff --git a/workflow/controller/controller_test.go b/workflow/controller/controller_test.go
index 17b0020e0..4bb31d323 100644
--- a/workflow/controller/controller_test.go
+++ b/workflow/controller/controller_test.go
@@ -19,13 +19,13 @@ import (
 	k8stesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
-	"sigs.k8s.io/yaml"
 
 	"github.com/argoproj/argo/config"
 	"github.com/argoproj/argo/persist/sqldb"
 	wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
 	fakewfclientset "github.com/argoproj/argo/pkg/client/clientset/versioned/fake"
 	wfextv "github.com/argoproj/argo/pkg/client/informers/externalversions"
+	"github.com/argoproj/argo/test"
 	controllercache "github.com/argoproj/argo/workflow/controller/cache"
 	hydratorfake "github.com/argoproj/argo/workflow/hydrator/fake"
 	"github.com/argoproj/argo/workflow/metrics"
@@ -193,30 +193,15 @@ func newControllerWithComplexDefaults() (context.CancelFunc, *WorkflowController
 }
 
 func unmarshalWF(yamlStr string) *wfv1.Workflow {
-	var wf wfv1.Workflow
-	err := yaml.Unmarshal([]byte(yamlStr), &wf)
-	if err != nil {
-		panic(err)
-	}
-	return &wf
+	return test.LoadWorkflowFromBytes([]byte(yamlStr))
 }
 
 func unmarshalWFTmpl(yamlStr string) *wfv1.WorkflowTemplate {
-	var wftmpl wfv1.WorkflowTemplate
-	err := yaml.Unmarshal([]byte(yamlStr), &wftmpl)
-	if err != nil {
-		panic(err)
-	}
-	return &wftmpl
+	return test.LoadWorkflowTemplateFromBytes([]byte(yamlStr))
 }
 
 func unmarshalCWFTmpl(yamlStr string) *wfv1.ClusterWorkflowTemplate {
-	var cwftmpl wfv1.ClusterWorkflowTemplate
-	err := yaml.Unmarshal([]byte(yamlStr), &cwftmpl)
-	if err != nil {
-		panic(err)
-	}
-	return &cwftmpl
+	return test.LoadClusterWorkflowTemplateFromBytes([]byte(yamlStr))
 }
 
 // makePodsPhase acts like a pod controller and simulates the transition of pods transitioning into a specified state
diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go
index 1804ac6ca..779fda2f0 100644
--- a/workflow/controller/operator_test.go
+++ b/workflow/controller/operator_test.go
@@ -4227,6 +4227,53 @@ status:
 	}
 }
 
+func TestWorkflowOutputs(t *testing.T) {
+	wf := unmarshalWF(`
+metadata:
+  name: my-wf
+  namespace: my-ns
+spec:
+  entrypoint: main
+  templates:
+  - name: main
+    dag:
+      tasks:
+      - name: step-1
+        template: child
+  - name: child
+    container:
+      image: my-image
+    outputs:
+      parameters:
+      - name: my-param
+        valueFrom: 
+          path: /my-path
+`)
+	cancel, controller := newController(wf)
+	defer cancel()
+	woc := newWorkflowOperationCtx(wf, controller)
+
+	// reconcille
+	woc.operate()
+	assert.Equal(t, wfv1.NodeRunning, woc.wf.Status.Phase)
+
+	// make all created pods as successful
+	podInterface := controller.kubeclientset.CoreV1().Pods("my-ns")
+	list, err := podInterface.List(metav1.ListOptions{})
+	assert.NoError(t, err)
+	assert.Len(t, list.Items, 1)
+	for _, pod := range list.Items {
+		pod.Status.Phase = apiv1.PodSucceeded
+		pod.GetAnnotations()[common.AnnotationKeyOutputs] = `{"parameters": [{"name": "my-param"}]}`
+		_, err := podInterface.Update(&pod)
+		assert.NoError(t, err)
+	}
+
+	// reconcille
+	woc.operate()
+	assert.Equal(t, wfv1.NodeSucceeded, woc.wf.Status.Phase)
+}
+
 var globalVarsOnExit = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow