fix: workflowMetadata needs to be loaded into globalParams in both Ar…

…goServer and Controller (#8907) * fix: workflowMetadata needs to be loaded into globalParams in both ArgoServer and Controller Signed-off-by: Julie Vogelman <[email protected]> * fix: use the right condition for determining if template is set Signed-off-by: Julie Vogelman <[email protected]> * fix: need to make maps if nil Signed-off-by: Julie Vogelman <[email protected]> * docs: result of 'make codegen' Signed-off-by: Julie Vogelman <[email protected]> * Revert "docs: result of 'make codegen'" This reverts commit 68682d8. Signed-off-by: Julie Vogelman <[email protected]> * docs: result of 'make codegen' Signed-off-by: Julie Vogelman <[email protected]> * feat: unit test for operator.go for issue 8837 Signed-off-by: Julie Vogelman <[email protected]> * feat: unit test for validation of Labels and Annotations when Workflows reference WorkflowTemplates Signed-off-by: Julie Vogelman <[email protected]> * feat: fixing lint issues Signed-off-by: Julie Vogelman <[email protected]> * fix: empty commit Signed-off-by: Julie Vogelman <[email protected]> * fix: add an e2e test which verifies that a submitted WorkflowTemplate referencing a workflowMetadata label works Signed-off-by: Julie Vogelman <[email protected]> * fix: assert.Equal() terms were backward Signed-off-by: Julie Vogelman <[email protected]> * feat: add a unit test for LabelsFrom Signed-off-by: Julie Vogelman <[email protected]> * feat: unit testing for LabelsFrom Signed-off-by: Julie Vogelman <[email protected]>
argoproj · Jun 9, 2022 · ede1a39 · ede1a39
1 parent df37649
commit ede1a39
Show file tree

Hide file tree

Showing 30 changed files with 522 additions and 23 deletions.
diff --git a/api/jsonschema/schema.json b/api/jsonschema/schema.json
@@ -8188,7 +8188,7 @@
         },
         "workflowMetadata": {
           "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.WorkflowMetadata",
-          "description": "WorkflowMetadata contains some metadata of the workflow to be refer"
+          "description": "WorkflowMetadata contains some metadata of the workflow to refer to"
         },
         "workflowTemplateRef": {
           "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.WorkflowTemplateRef",

diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
@@ -12611,7 +12611,7 @@
           "x-kubernetes-patch-strategy": "merge"
         },
         "workflowMetadata": {
-          "description": "WorkflowMetadata contains some metadata of the workflow to be refer",
+          "description": "WorkflowMetadata contains some metadata of the workflow to refer to",
           "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.WorkflowMetadata"
         },
         "workflowTemplateRef": {

diff --git a/docs/fields.md b/docs/fields.md
@@ -813,7 +813,7 @@ WorkflowSpec is the specification of a Workflow.
 |`volumeClaimGC`|[`VolumeClaimGC`](#volumeclaimgc)|VolumeClaimGC describes the strategy to use when deleting volumes from completed workflows|
 |`volumeClaimTemplates`|`Array<`[`PersistentVolumeClaim`](#persistentvolumeclaim)`>`|VolumeClaimTemplates is a list of claims that containers are allowed to reference. The Workflow controller will create the claims at the beginning of the workflow and delete the claims upon completion of the workflow|
 |`volumes`|`Array<`[`Volume`](#volume)`>`|Volumes is a list of volumes that can be mounted by containers in a io.argoproj.workflow.v1alpha1.|
-|`workflowMetadata`|[`WorkflowMetadata`](#workflowmetadata)|WorkflowMetadata contains some metadata of the workflow to be refer|
+|`workflowMetadata`|[`WorkflowMetadata`](#workflowmetadata)|WorkflowMetadata contains some metadata of the workflow to refer to|
 |`workflowTemplateRef`|[`WorkflowTemplateRef`](#workflowtemplateref)|WorkflowTemplateRef holds a reference to a WorkflowTemplate for execution|
 
 ## WorkflowStatus

diff --git a/go.mod b/go.mod
@@ -48,6 +48,7 @@ require (
 	github.com/valyala/fasttemplate v1.2.1
 	github.com/xeipuuv/gojsonschema v1.2.0
 	golang.org/x/crypto v0.0.0-20220411220226-7b82a4e95df4
+	golang.org/x/exp v0.0.0-20220602145555-4a0574d9293f
 	golang.org/x/net v0.0.0-20220526153639-5463443f8c37
 	golang.org/x/oauth2 v0.0.0-20220524215830-622c5d57e401
 	golang.org/x/sync v0.0.0-20220513210516-0976fa681c29

diff --git a/go.sum b/go.sum
@@ -1538,6 +1538,8 @@ golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EH
 golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
 golang.org/x/exp v0.0.0-20200331195152-e8c3332aa8e5/go.mod h1:4M0jN8W1tt0AVLNr8HDosyJCDCDuyL9N9+3m7wDWgKw=
 golang.org/x/exp v0.0.0-20200908183739-ae8ad444f925/go.mod h1:1phAWC201xIgDyaFpmDeZkgf70Q4Pd/CNqfRtVPtxNw=
+golang.org/x/exp v0.0.0-20220602145555-4a0574d9293f h1:KK6mxegmt5hGJRcAnEDjSNLxIRhZxDcgwMbcO/lMCRM=
+golang.org/x/exp v0.0.0-20220602145555-4a0574d9293f/go.mod h1:yh0Ynu2b5ZUe3MQfp2nM0ecK7wsgouWTDN0FNeJuIys=
 golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
 golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=

diff --git a/pkg/apis/workflow/v1alpha1/generated.proto b/pkg/apis/workflow/v1alpha1/generated.proto
diff --git a/pkg/apis/workflow/v1alpha1/openapi_generated.go b/pkg/apis/workflow/v1alpha1/openapi_generated.go
diff --git a/pkg/apis/workflow/v1alpha1/workflow_types.go b/pkg/apis/workflow/v1alpha1/workflow_types.go
@@ -414,7 +414,7 @@ type WorkflowSpec struct {
 	// step, irrespective of the success, failure, or error status of the primary step
 	Hooks LifecycleHooks `json:"hooks,omitempty" protobuf:"bytes,41,opt,name=hooks"`
 
-	// WorkflowMetadata contains some metadata of the workflow to be refer
+	// WorkflowMetadata contains some metadata of the workflow to refer to
 	WorkflowMetadata *WorkflowMetadata `json:"workflowMetadata,omitempty" protobuf:"bytes,42,opt,name=workflowMetadata"`
 
 	// ArtifactGC describes the strategy to use when deleting artifacts from completed or deleted workflows (applies to all output Artifacts

diff --git a/test/e2e/testdata/workflow-template-sub-test.yaml b/test/e2e/testdata/workflow-template-sub-test.yaml
@@ -0,0 +1,18 @@
+apiVersion: argoproj.io/v1alpha1
+kind: WorkflowTemplate
+metadata:
+  name: workflow-template-submittable
+spec:
+  entrypoint: whalesay-template
+  templates:
+    - name: whalesay-template
+      container:
+        image: 'argoproj/argosay:v2'
+        command:
+          - /argosay
+        args:
+          - echo
+          - '{{workflow.labels.arg-name}}'
+  workflowMetadata:
+    labels:
+      arg-name: myLabelArg
diff --git a/test/e2e/workflow_template_test.go b/test/e2e/workflow_template_test.go
@@ -64,6 +64,19 @@ func (s *WorkflowTemplateSuite) TestSubmitWorkflowTemplateWithEnum() {
 		})
 }
 
+func (s *WorkflowTemplateSuite) TestSubmitWorkflowTemplateWorkflowMetadataSubstitution() {
+	s.Given().
+		WorkflowTemplate("@testdata/workflow-template-sub-test.yaml").
+		When().
+		CreateWorkflowTemplates().
+		SubmitWorkflowsFromWorkflowTemplates().
+		WaitForWorkflow().
+		Then().
+		ExpectWorkflow(func(t *testing.T, metadata *v1.ObjectMeta, status *v1alpha1.WorkflowStatus) {
+			assert.Equal(t, status.Phase, v1alpha1.WorkflowSucceeded)
+		})
+}
+
 func TestWorkflowTemplateSuite(t *testing.T) {
 	suite.Run(t, new(WorkflowTemplateSuite))
 }
diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go
@@ -272,11 +272,6 @@ func (woc *wfOperationCtx) operate(ctx context.Context) {
 			return
 		}
 
-		if err := woc.updateWorkflowMetadata(); err != nil {
-			woc.markWorkflowError(ctx, err)
-			return
-		}
-
 		woc.workflowDeadline = woc.getWorkflowDeadline()
 
 		// Workflow will not be requeued if workflow steps are in pending state.
@@ -486,11 +481,19 @@ func (woc *wfOperationCtx) operate(ctx context.Context) {
 
 func (woc *wfOperationCtx) updateWorkflowMetadata() error {
 	if md := woc.execWf.Spec.WorkflowMetadata; md != nil {
+		if woc.wf.ObjectMeta.Labels == nil {
+			woc.wf.ObjectMeta.Labels = make(map[string]string)
+		}
 		for n, v := range md.Labels {
 			woc.wf.Labels[n] = v
+			woc.globalParams["workflow.labels."+n] = v
+		}
+		if woc.wf.ObjectMeta.Annotations == nil {
+			woc.wf.ObjectMeta.Annotations = make(map[string]string)
 		}
 		for n, v := range md.Annotations {
 			woc.wf.Annotations[n] = v
+			woc.globalParams["workflow.annotations."+n] = v
 		}
 		env := env.GetFuncMap(template.EnvMap(woc.globalParams))
 		for n, f := range md.LabelsFrom {
@@ -503,6 +506,7 @@ func (woc *wfOperationCtx) updateWorkflowMetadata() error {
 				return fmt.Errorf("failed to evaluate label %q expression %q evaluted to %T but must be a string", n, f.Expression, r)
 			}
 			woc.wf.Labels[n] = v
+			woc.globalParams["workflow.labels."+n] = v
 		}
 		woc.updated = true
 	}
@@ -3505,6 +3509,12 @@ func (woc *wfOperationCtx) setExecWorkflow(ctx context.Context) error {
 		woc.markWorkflowFailed(ctx, fmt.Sprintf("failed to set global parameters: %s", err.Error()))
 		return err
 	}
+	if woc.wf.Status.Phase == wfv1.WorkflowUnknown {
+		if err := woc.updateWorkflowMetadata(); err != nil {
+			woc.markWorkflowError(ctx, err)
+			return err
+		}
+	}
 	err = woc.substituteGlobalVariables()
 	if err != nil {
 		return err

diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go
@@ -6976,6 +6976,84 @@ func TestSubstituteGlobalVariables(t *testing.T) {
 	assert.Contains(t, string(tempStr), "{{workflow.parameters.message}}")
 }
 
+// test that Labels and Annotations are correctly substituted in the case of
+// a Workflow referencing a WorkflowTemplate, where Labels and Annotations can come from:
+// - Workflow metadata
+// - Workflow spec.workflowMetadata
+// - WorkflowTemplate spec.workflowMetadata
+func TestSubstituteGlobalVariablesLabelsAnnotations(t *testing.T) {
+
+	tests := []struct {
+		name                  string
+		workflow              string
+		workflowTemplate      string
+		expectedMutexName     string
+		expectedSchedulerName string
+	}{
+		{
+			// entire template referenced; value not contained in WorkflowTemplate or Workflow
+			workflow:              "@testdata/workflow-sub-test-1.yaml",
+			workflowTemplate:      "@testdata/workflow-template-sub-test-1.yaml",
+			expectedMutexName:     "{{workflow.labels.mutex-name}}",
+			expectedSchedulerName: "{{workflow.annotations.scheduler-name}}",
+		},
+		{
+			// entire template referenced; value is in Workflow.Labels
+			workflow:              "@testdata/workflow-sub-test-2.yaml",
+			workflowTemplate:      "@testdata/workflow-template-sub-test-1.yaml",
+			expectedMutexName:     "myMutex",
+			expectedSchedulerName: "myScheduler",
+		},
+		{
+			// entire template referenced; value is in WorkflowTemplate.workflowMetadata
+			workflow:              "@testdata/workflow-sub-test-2.yaml",
+			workflowTemplate:      "@testdata/workflow-template-sub-test-2.yaml",
+			expectedMutexName:     "wfMetadataTemplateMutex",
+			expectedSchedulerName: "wfMetadataTemplateScheduler",
+		},
+		{
+			// entire template referenced; value is in Workflow.workflowMetadata
+			workflow:              "@testdata/workflow-sub-test-3.yaml",
+			workflowTemplate:      "@testdata/workflow-template-sub-test-2.yaml",
+			expectedMutexName:     "wfMetadataMutex",
+			expectedSchedulerName: "wfMetadataScheduler",
+		},
+		// test using LabelsFrom
+		{
+			workflow:              "@testdata/workflow-sub-test-4.yaml",
+			workflowTemplate:      "@testdata/workflow-template-sub-test-3.yaml",
+			expectedMutexName:     "wfMetadataTemplateMutex",
+			expectedSchedulerName: "wfMetadataScheduler",
+		},
+		{
+			// just a single template from the WorkflowTemplate is referenced:
+			// shouldn't have access to the global scope of the WorkflowTemplate
+			workflow:              "@testdata/workflow-sub-test-5.yaml",
+			workflowTemplate:      "@testdata/workflow-template-sub-test-4.yaml",
+			expectedMutexName:     "myMutex",
+			expectedSchedulerName: "myScheduler",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			wf := wfv1.MustUnmarshalWorkflow(tt.workflow)
+			wftmpl := wfv1.MustUnmarshalWorkflowTemplate(tt.workflowTemplate)
+			cancel, controller := newController(wf, wftmpl)
+			defer cancel()
+
+			woc := newWorkflowOperationCtx(wf, controller)
+			err := woc.setExecWorkflow(context.Background())
+
+			assert.Nil(t, err)
+			assert.NotNil(t, woc.execWf)
+			assert.Equal(t, tt.expectedMutexName, woc.execWf.Spec.Synchronization.Mutex.Name)
+			assert.Equal(t, tt.expectedSchedulerName, woc.execWf.Spec.SchedulerName)
+		})
+	}
+}
+
 var wfPending = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow

diff --git a/workflow/controller/testdata/workflow-sub-test-1.yaml b/workflow/controller/testdata/workflow-sub-test-1.yaml
@@ -0,0 +1,12 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: workflow-template-hello-world-
+  namespace: test
+spec:
+  workflowTemplateRef:
+    name: workflow-template-submittable
+  synchronization:
+    mutex:
+      name:  "{{workflow.labels.mutex-name}}"
+  schedulerName:  "{{workflow.annotations.scheduler-name}}"
diff --git a/workflow/controller/testdata/workflow-sub-test-2.yaml b/workflow/controller/testdata/workflow-sub-test-2.yaml
@@ -0,0 +1,16 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: workflow-template-hello-world-
+  namespace: test
+  labels:
+    mutex-name: myMutex
+  annotations:
+    scheduler-name: myScheduler
+spec:
+  workflowTemplateRef:
+    name: workflow-template-submittable
+  synchronization:
+    mutex:
+      name:  "{{workflow.labels.mutex-name}}"
+  schedulerName:  "{{workflow.annotations.scheduler-name}}"
diff --git a/workflow/controller/testdata/workflow-sub-test-3.yaml b/workflow/controller/testdata/workflow-sub-test-3.yaml
@@ -0,0 +1,21 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: workflow-template-hello-world-
+  namespace: test
+  labels:
+    mutex-name: myMutex
+  annotations:
+    scheduler-name: myScheduler
+spec:
+  workflowTemplateRef:
+    name: workflow-template-submittable
+  synchronization:
+    mutex:
+      name:  "{{workflow.labels.mutex-name}}"
+  schedulerName:  "{{workflow.annotations.scheduler-name}}"
+  workflowMetadata:
+    labels:
+      mutex-name: wfMetadataMutex
+    annotations:
+      scheduler-name: wfMetadataScheduler
diff --git a/workflow/controller/testdata/workflow-sub-test-4.yaml b/workflow/controller/testdata/workflow-sub-test-4.yaml
@@ -0,0 +1,19 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: workflow-template-hello-world-
+  namespace: test
+  labels:
+    mutex-name: myMutex
+  annotations:
+    scheduler-name: myScheduler
+spec:
+  workflowTemplateRef:
+    name: workflow-template-submittable
+  synchronization:
+    mutex:
+      name:  "{{workflow.labels.mutex-name}}"
+  schedulerName:  "{{workflow.annotations.scheduler-name}}"
+  workflowMetadata:
+    annotations:
+      scheduler-name: wfMetadataScheduler
diff --git a/workflow/controller/testdata/workflow-sub-test-5.yaml b/workflow/controller/testdata/workflow-sub-test-5.yaml
@@ -0,0 +1,22 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Workflow
+metadata:
+  generateName: workflow-template-hello-world-
+  namespace: test
+  labels:
+    mutex-name: myMutex
+  annotations:
+    scheduler-name: myScheduler
+spec:
+  entrypoint: myTemplate
+  templates:
+    - name: myTemplate
+      steps:
+        - - name: whalesay
+            templateRef:
+              name: workflow-template-submittable
+              template: whalesay-template
+  synchronization:
+    mutex:
+      name:  "{{workflow.labels.mutex-name}}"
+  schedulerName:  "{{workflow.annotations.scheduler-name}}"
diff --git a/workflow/controller/testdata/workflow-template-sub-test-1.yaml b/workflow/controller/testdata/workflow-template-sub-test-1.yaml
@@ -0,0 +1,17 @@
+apiVersion: argoproj.io/v1alpha1
+kind: WorkflowTemplate
+metadata:
+  name: workflow-template-submittable
+  namespace: test
+  labels:
+    mutex-name: myMutex
+  annotations:
+    scheduler-name: myScheduler
+spec:
+  entrypoint: whalesay-template
+  templates:
+    - name: whalesay-template
+      container:
+        image: docker/whalesay
+        command: [cowsay]
+        args: ['hello']
diff --git a/workflow/controller/testdata/workflow-template-sub-test-2.yaml b/workflow/controller/testdata/workflow-template-sub-test-2.yaml
@@ -0,0 +1,22 @@
+apiVersion: argoproj.io/v1alpha1
+kind: WorkflowTemplate
+metadata:
+  name: workflow-template-submittable
+  namespace: test
+  labels:
+    mutex-name: myMutex
+  annotations:
+    scheduler-name: myScheduler
+spec:
+  entrypoint: whalesay-template
+  templates:
+    - name: whalesay-template
+      container:
+        image: docker/whalesay
+        command: [cowsay]
+        args: ['hello']
+  workflowMetadata:
+    labels:
+      mutex-name: wfMetadataTemplateMutex
+    annotations:
+      scheduler-name: wfMetadataTemplateScheduler