Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: workflowMetadata needs to be loaded into globalParams in both ArgoServer and Controller #8907

Merged
merged 15 commits into from
Jun 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/jsonschema/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion api/openapi-spec/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion docs/fields.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/workflow/v1alpha1/generated.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/apis/workflow/v1alpha1/openapi_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/apis/workflow/v1alpha1/workflow_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions test/e2e/testdata/workflow-template-sub-test.yaml
Original file line number Diff line number Diff line change
@@ -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
13 changes: 13 additions & 0 deletions test/e2e/workflow_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
20 changes: 15 additions & 5 deletions workflow/controller/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
78 changes: 78 additions & 0 deletions workflow/controller/operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions workflow/controller/testdata/workflow-sub-test-1.yaml
Original file line number Diff line number Diff line change
@@ -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}}"
16 changes: 16 additions & 0 deletions workflow/controller/testdata/workflow-sub-test-2.yaml
Original file line number Diff line number Diff line change
@@ -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}}"
21 changes: 21 additions & 0 deletions workflow/controller/testdata/workflow-sub-test-3.yaml
Original file line number Diff line number Diff line change
@@ -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
19 changes: 19 additions & 0 deletions workflow/controller/testdata/workflow-sub-test-4.yaml
Original file line number Diff line number Diff line change
@@ -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
22 changes: 22 additions & 0 deletions workflow/controller/testdata/workflow-sub-test-5.yaml
Original file line number Diff line number Diff line change
@@ -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}}"
17 changes: 17 additions & 0 deletions workflow/controller/testdata/workflow-template-sub-test-1.yaml
Original file line number Diff line number Diff line change
@@ -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']
22 changes: 22 additions & 0 deletions workflow/controller/testdata/workflow-template-sub-test-2.yaml
Original file line number Diff line number Diff line change
@@ -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
Loading