fix: Ensure target Workflow hooks not nil (#11521) (#11535)

.github/workflows/ci-build.yaml

@@ -58,7 +58,7 @@ jobs:
  e2e-tests:
  name: E2E Tests
  runs-on: ubuntu-latest
- timeout-minutes: 25
+ timeout-minutes: 30
  needs: [ argoexec-image ]
  env:
  KUBECONFIG: /home/runner/.kubeconfig

test/e2e/testdata/workflow-templates/success-hook.yaml

@@ -0,0 +1,21 @@
+apiVersion: argoproj.io/v1alpha1
+kind: WorkflowTemplate
+metadata:
+ name: hook
+spec:
+ entrypoint: main
+ templates:
+ - name: main
+ container:
+ image: argoproj/argosay:v2
+ command: ["/bin/sh", "-c"]
+ # To avoid flakiness, we sleep 1 second.
+ args: ["/bin/sleep 1; /argosay"]
+
+ hooks:
+ running:
+ expression: workflow.status == "Running"
+ template: main
+ succeed:
+ expression: workflow.status == "Succeeded"
+ template: main

test/e2e/workflow_template_test.go

@@ -4,10 +4,12 @@
 package e2e
 
 import (
+ "strings"
  "testing"
 
  "github.com/stretchr/testify/assert"
  "github.com/stretchr/testify/suite"
+ apiv1 "k8s.io/api/core/v1"
  v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
  "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
@@ -126,6 +128,36 @@ spec:
  ExpectPVCDeleted()
 }
 
+func (s *WorkflowTemplateSuite) TestWorkflowTemplateWithHook() {
+ s.Given().
+ WorkflowTemplate("@testdata/workflow-templates/success-hook.yaml").
+ Workflow(`
+metadata:
+ generateName: workflow-template-hook-
+spec:
+ workflowTemplateRef:
+ name: hook
+`).
+ When().
+ CreateWorkflowTemplates().
+ SubmitWorkflow().
+ WaitForWorkflow(fixtures.ToBeSucceeded).
+ Then().
+ ExpectWorkflow(func(t *testing.T, metadata *v1.ObjectMeta, status *v1alpha1.WorkflowStatus) {
+ assert.Equal(t, status.Phase, v1alpha1.WorkflowSucceeded)
+ }).
+ ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool {
+ return strings.Contains(status.Name, "hooks.running")
+ }, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) {
+ assert.Equal(t, v1alpha1.NodeSucceeded, status.Phase)
+ }).
+ ExpectWorkflowNode(func(status v1alpha1.NodeStatus) bool {
+ return strings.Contains(status.Name, "hooks.succeed")
+ }, func(t *testing.T, status *v1alpha1.NodeStatus, pod *apiv1.Pod) {
+ assert.Equal(t, v1alpha1.NodeSucceeded, status.Phase)
+ })
+}
+
 func (s *WorkflowTemplateSuite) TestWorkflowTemplateInvalidEntryPoint() {
  s.Given().
  WorkflowTemplate("@testdata/workflow-template-invalid-entrypoint.yaml").

workflow/util/merge.go

@@ -39,6 +39,9 @@ func MergeTo(patch, target *wfv1.Workflow) error {
  return err
  }
 
+ if len(patchHooks) != 0 && target.Spec.Hooks == nil {
+ target.Spec.Hooks = make(wfv1.LifecycleHooks)
+ }
  for name, hook := range patchHooks {
  // If the patch hook doesn't exist in target
  if _, ok := target.Spec.Hooks[name]; !ok {

workflow/util/merge_test.go

@@ -272,6 +272,14 @@ func TestJoinWorkflowMetaData(t *testing.T) {
  })
 }
 
+var baseNilHookWF = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+ generateName: workflow-template-hello-world-
+spec:
+`
+
 var baseHookWF = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow
@@ -284,6 +292,12 @@ spec:
  expression: workflow.status == "Pending"
 `
 
+var patchNilHookWF = `
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+spec:
+`
+
 var patchHookWF = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow
@@ -297,14 +311,36 @@ spec:
  expression: workflow.status == "Pending"
 `
 
-// Ensure hook bar ends up in result, but foo is unchanged
 func TestMergeHooks(t *testing.T) {
- patchHookWf := wfv1.MustUnmarshalWorkflow(patchHookWF)
- targetHookWf := wfv1.MustUnmarshalWorkflow(baseHookWF)
+ t.Run("NilBaseAndNilPatch", func(t *testing.T) {
+ patchHookWf := wfv1.MustUnmarshalWorkflow(patchNilHookWF)
+ targetHookWf := wfv1.MustUnmarshalWorkflow(baseNilHookWF)
 
- err := MergeTo(patchHookWf, targetHookWf)
- assert.NoError(t, err)
- assert.Equal(t, 2, len(targetHookWf.Spec.Hooks))
- assert.Equal(t, "a", targetHookWf.Spec.Hooks[`foo`].Template)
- assert.Equal(t, "b", targetHookWf.Spec.Hooks[`bar`].Template)
+ err := MergeTo(patchHookWf, targetHookWf)
+ assert.NoError(t, err)
+ assert.Nil(t, targetHookWf.Spec.Hooks)
+ })
+
+ t.Run("NilBaseAndNotNilPatch", func(t *testing.T) {
+ patchHookWf := wfv1.MustUnmarshalWorkflow(patchHookWF)
+ targetHookWf := wfv1.MustUnmarshalWorkflow(baseNilHookWF)
+
+ err := MergeTo(patchHookWf, targetHookWf)
+ assert.NoError(t, err)
+ assert.Equal(t, 2, len(targetHookWf.Spec.Hooks))
+ assert.Equal(t, "c", targetHookWf.Spec.Hooks[`foo`].Template)
+ assert.Equal(t, "b", targetHookWf.Spec.Hooks[`bar`].Template)
+ })
+
+ // Ensure hook bar ends up in result, but foo is unchanged
+ t.Run("NotNilBaseAndPatch", func(t *testing.T) {
+ patchHookWf := wfv1.MustUnmarshalWorkflow(patchHookWF)
+ targetHookWf := wfv1.MustUnmarshalWorkflow(baseHookWF)
+
+ err := MergeTo(patchHookWf, targetHookWf)
+ assert.NoError(t, err)
+ assert.Equal(t, 2, len(targetHookWf.Spec.Hooks))
+ assert.Equal(t, "a", targetHookWf.Spec.Hooks[`foo`].Template)
+ assert.Equal(t, "b", targetHookWf.Spec.Hooks[`bar`].Template)
+ })
 }