From a828b9da09b1b3543067ef4513b850cd85958e57 Mon Sep 17 00:00:00 2001 From: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> Date: Sat, 27 Jul 2024 12:59:26 -0400 Subject: [PATCH] fix(resource): don't use `-f` when patch file is provided (#13317) Signed-off-by: Anton Gilgur Signed-off-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> (cherry picked from commit 5d8ee22b249e1702894709ae3aaeb1995d6028a4) --- docs/fields.md | 54 ++++++---- ...k8s-patch.yaml => k8s-patch-json-pod.yaml} | 3 +- ...flow.yaml => k8s-patch-json-workflow.yaml} | 3 +- examples/k8s-patch-merge-pod.yaml | 25 +++++ ...8s-patch-basic.yaml => k8s-patch-pod.yaml} | 5 +- workflow/executor/resource.go | 28 +++-- workflow/executor/resource_test.go | 100 +++++++++--------- 7 files changed, 136 insertions(+), 82 deletions(-) rename examples/{k8s-patch.yaml => k8s-patch-json-pod.yaml} (86%) rename examples/{k8s-json-patch-workflow.yaml => k8s-patch-json-workflow.yaml} (85%) create mode 100644 examples/k8s-patch-merge-pod.yaml rename examples/{k8s-patch-basic.yaml => k8s-patch-pod.yaml} (83%) diff --git a/docs/fields.md b/docs/fields.md index 8233b4b76a22..e7111fd643bd 100644 --- a/docs/fields.md +++ b/docs/fields.md @@ -187,15 +187,17 @@ Workflow is the definition of a workflow resource - [`k8s-jobs.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-jobs.yaml) -- [`k8s-json-patch-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-json-patch-workflow.yaml) - - [`k8s-orchestration.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-orchestration.yaml) - [`k8s-owner-reference.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-owner-reference.yaml) -- [`k8s-patch-basic.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-basic.yaml) +- [`k8s-patch-json-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-pod.yaml) + +- [`k8s-patch-json-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-workflow.yaml) + +- [`k8s-patch-merge-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-merge-pod.yaml) -- [`k8s-patch.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml) +- [`k8s-patch-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-pod.yaml) - [`k8s-resource-log-selector.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-resource-log-selector.yaml) @@ -621,11 +623,13 @@ WorkflowSpec is the specification of a Workflow. - [`intermediate-parameters.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/intermediate-parameters.yaml) -- [`k8s-json-patch-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-json-patch-workflow.yaml) - - [`k8s-owner-reference.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-owner-reference.yaml) -- [`k8s-patch.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml) +- [`k8s-patch-json-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-pod.yaml) + +- [`k8s-patch-json-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-workflow.yaml) + +- [`k8s-patch-merge-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-merge-pod.yaml) - [`k8s-wait-wf.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-wait-wf.yaml) @@ -1058,11 +1062,13 @@ CronWorkflowSpec is the specification of a CronWorkflow - [`intermediate-parameters.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/intermediate-parameters.yaml) -- [`k8s-json-patch-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-json-patch-workflow.yaml) - - [`k8s-owner-reference.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-owner-reference.yaml) -- [`k8s-patch.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml) +- [`k8s-patch-json-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-pod.yaml) + +- [`k8s-patch-json-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-workflow.yaml) + +- [`k8s-patch-merge-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-merge-pod.yaml) - [`k8s-wait-wf.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-wait-wf.yaml) @@ -2802,11 +2808,13 @@ ResourceTemplate is a template subtype to manipulate kubernetes resources - [`cron-backfill.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/cron-backfill.yaml) -- [`k8s-json-patch-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-json-patch-workflow.yaml) - - [`k8s-owner-reference.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-owner-reference.yaml) -- [`k8s-patch.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml) +- [`k8s-patch-json-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-pod.yaml) + +- [`k8s-patch-json-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-workflow.yaml) + +- [`k8s-patch-merge-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-merge-pod.yaml) - [`k8s-wait-wf.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-wait-wf.yaml) @@ -3652,7 +3660,9 @@ MetricLabel is a single label for a prometheus metric - [`k8s-owner-reference.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-owner-reference.yaml) -- [`k8s-patch.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml) +- [`k8s-patch-json-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-pod.yaml) + +- [`k8s-patch-merge-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-merge-pod.yaml) - [`pod-gc-strategy-with-label-selector.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/pod-gc-strategy-with-label-selector.yaml) @@ -4695,11 +4705,13 @@ ObjectMeta is metadata that all persisted resources must have, which includes al - [`intermediate-parameters.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/intermediate-parameters.yaml) -- [`k8s-json-patch-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-json-patch-workflow.yaml) - - [`k8s-owner-reference.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-owner-reference.yaml) -- [`k8s-patch.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml) +- [`k8s-patch-json-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-pod.yaml) + +- [`k8s-patch-json-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-workflow.yaml) + +- [`k8s-patch-merge-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-merge-pod.yaml) - [`k8s-wait-wf.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-wait-wf.yaml) @@ -6022,11 +6034,13 @@ PersistentVolumeClaimSpec describes the common attributes of storage devices and - [`intermediate-parameters.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/intermediate-parameters.yaml) -- [`k8s-json-patch-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-json-patch-workflow.yaml) - - [`k8s-owner-reference.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-owner-reference.yaml) -- [`k8s-patch.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml) +- [`k8s-patch-json-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-pod.yaml) + +- [`k8s-patch-json-workflow.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-json-workflow.yaml) + +- [`k8s-patch-merge-pod.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch-merge-pod.yaml) - [`k8s-wait-wf.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-wait-wf.yaml) diff --git a/examples/k8s-patch.yaml b/examples/k8s-patch-json-pod.yaml similarity index 86% rename from examples/k8s-patch.yaml rename to examples/k8s-patch-json-pod.yaml index a44f6fc2421c..1bf9ac569a6e 100644 --- a/examples/k8s-patch.yaml +++ b/examples/k8s-patch-json-pod.yaml @@ -1,7 +1,7 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: - generateName: k8s-patch- + generateName: k8s-patch-json-pod- labels: workflows.argoproj.io/test: "true" annotations: @@ -14,6 +14,7 @@ spec: resource: action: patch mergeStrategy: json + # patch an annotation to own Pod flags: - pod - "{{pod.name}}" diff --git a/examples/k8s-json-patch-workflow.yaml b/examples/k8s-patch-json-workflow.yaml similarity index 85% rename from examples/k8s-json-patch-workflow.yaml rename to examples/k8s-patch-json-workflow.yaml index 3a43a64447a0..6e417ca1de14 100644 --- a/examples/k8s-json-patch-workflow.yaml +++ b/examples/k8s-patch-json-workflow.yaml @@ -1,7 +1,7 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: - generateName: k8s-patch-workflow- + generateName: k8s-patch-json-workflow- annotations: workflows.argoproj.io/description: | This example shows how to patch a workflow with json mergeStrategy @@ -12,6 +12,7 @@ spec: resource: action: patch mergeStrategy: json + # patch an annotation to self flags: - workflow - "{{workflow.name}}" diff --git a/examples/k8s-patch-merge-pod.yaml b/examples/k8s-patch-merge-pod.yaml new file mode 100644 index 000000000000..be7028f170ea --- /dev/null +++ b/examples/k8s-patch-merge-pod.yaml @@ -0,0 +1,25 @@ +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: k8s-patch-merge-pod- + labels: + workflows.argoproj.io/test: "true" + annotations: + workflows.argoproj.io/description: | + This example shows a more advanced JSON merge patch +spec: + entrypoint: main + templates: + - name: main + resource: + action: patch + mergeStrategy: merge + # patch an annotation to own Pod + flags: + - pod + - "{{pod.name}}" + manifest: | + metadata: + annotations: + foo: bar + diff --git a/examples/k8s-patch-basic.yaml b/examples/k8s-patch-pod.yaml similarity index 83% rename from examples/k8s-patch-basic.yaml rename to examples/k8s-patch-pod.yaml index 805525eb698e..ccbff1849c28 100644 --- a/examples/k8s-patch-basic.yaml +++ b/examples/k8s-patch-pod.yaml @@ -1,18 +1,19 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: - generateName: k8s-patch-basic- + generateName: k8s-patch-pod- labels: workflows.argoproj.io/test: "true" annotations: workflows.argoproj.io/description: | - This example shows a standard patch with the default mergeStrategy + This example shows a standard patch with the default mergeStrategy (strategic) spec: entrypoint: main templates: - name: main resource: action: patch + # patch an annotation to own Pod manifest: | apiVersion: v1 kind: Pod diff --git a/workflow/executor/resource.go b/workflow/executor/resource.go index 11053272e1b9..d714cef55b90 100644 --- a/workflow/executor/resource.go +++ b/workflow/executor/resource.go @@ -25,6 +25,7 @@ import ( gengotypes "k8s.io/gengo/types" kubectlcmd "k8s.io/kubectl/pkg/cmd" kubectlutil "k8s.io/kubectl/pkg/cmd/util" + "sigs.k8s.io/yaml" "github.com/argoproj/argo-workflows/v3/errors" wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" @@ -131,19 +132,28 @@ func (we *WorkflowExecutor) getKubectlArguments(action string, manifestPath stri mergeStrategy := "strategic" if we.Template.Resource.MergeStrategy != "" { mergeStrategy = we.Template.Resource.MergeStrategy - if mergeStrategy == "json" { - // Action "patch" require flag "-p" with resource arguments. - // But kubectl disallow specify both "-f" flag and resource arguments. - // Flag "-f" should be excluded for action "patch" here if it's a json patch. - appendFileFlag = false - } } - args = append(args, "--type") args = append(args, mergeStrategy) - args = append(args, "-p") - args = append(args, string(buff)) + args = append(args, "--patch-file") + args = append(args, manifestPath) + + // if there are flags and the manifest has no `kind`, assume: `kubectl patch --patch-file ` + // json patches also use patch files by definition and so require resource arguments + // the other form in our case is `kubectl patch -f --patch-file ` + if mergeStrategy == "json" { + appendFileFlag = false + } else { + var obj map[string]interface{} + err = yaml.Unmarshal(buff, &obj) + if err != nil { + return []string{}, errors.New(errors.CodeBadRequest, err.Error()) + } + if len(flags) != 0 && obj["kind"] == nil { + appendFileFlag = false + } + } } if len(flags) != 0 { diff --git a/workflow/executor/resource_test.go b/workflow/executor/resource_test.go index 4fbda3025eac..534cd90430ac 100644 --- a/workflow/executor/resource_test.go +++ b/workflow/executor/resource_test.go @@ -60,63 +60,65 @@ func TestResourceFlags(t *testing.T) { // TestResourcePatchFlags tests whether Resource Flags // are properly passed to `kubectl patch` command func TestResourcePatchFlags(t *testing.T) { + fakeFlags := []string{"pod", "mypod"} fakeClientset := fake.NewSimpleClientset() - manifestPath := "../../examples/hello-world.yaml" - buff, err := os.ReadFile(manifestPath) - assert.NoError(t, err) - fakeFlags := []string{"kubectl", "patch", "--type", "strategic", "-p", string(buff), "-f", manifestPath, "-o", "json"} - mockRuntimeExecutor := mocks.ContainerRuntimeExecutor{} - template := wfv1.Template{ - Resource: &wfv1.ResourceTemplate{ - Action: "patch", - Flags: fakeFlags, + tests := []struct { + name string + patchType string + appendFileFlag bool + manifestPath string + }{ + { + name: "strategic -f --patch-file", + patchType: "strategic", + appendFileFlag: true, + manifestPath: "../../examples/hello-world.yaml", // any YAML with a `kind` }, - } - - we := WorkflowExecutor{ - PodName: fakePodName, - Template: template, - ClientSet: fakeClientset, - Namespace: fakeNamespace, - RuntimeExecutor: &mockRuntimeExecutor, - } - args, err := we.getKubectlArguments("patch", manifestPath, nil) - - assert.NoError(t, err) - assert.Equal(t, args, fakeFlags) -} - -// TestResourcePatchFlagsJson tests whether Resource Flags -// are properly passed to `kubectl patch` command in json patches -func TestResourcePatchFlagsJson(t *testing.T) { - fakeClientset := fake.NewSimpleClientset() - manifestPath := "../../examples/hello-world.yaml" - buff, err := os.ReadFile(manifestPath) - assert.NoError(t, err) - fakeFlags := []string{"kubectl", "patch", "--type", "json", "-p", string(buff), "-o", "json"} - - mockRuntimeExecutor := mocks.ContainerRuntimeExecutor{} - - template := wfv1.Template{ - Resource: &wfv1.ResourceTemplate{ - Action: "patch", - Flags: fakeFlags, - MergeStrategy: "json", + { + name: "json --patch-file", + patchType: "json", + appendFileFlag: false, + manifestPath: "../../.golangci.yml", // any YAML without a `kind` + }, + { + name: "merge --patch-file", + patchType: "merge", + appendFileFlag: false, + manifestPath: "../../.golangci.yml", // any YAML without a `kind` }, } - we := WorkflowExecutor{ - PodName: fakePodName, - Template: template, - ClientSet: fakeClientset, - Namespace: fakeNamespace, - RuntimeExecutor: &mockRuntimeExecutor, + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + expectedArgs := []string{"kubectl", "patch", "--type", tt.patchType, "--patch-file", tt.manifestPath} + expectedArgs = append(expectedArgs, fakeFlags...) + if tt.appendFileFlag { + expectedArgs = append(expectedArgs, "-f", tt.manifestPath) + } + expectedArgs = append(expectedArgs, "-o", "json") + + template := wfv1.Template{ + Resource: &wfv1.ResourceTemplate{ + Action: "patch", + Flags: fakeFlags, + MergeStrategy: tt.patchType, + }, + } + we := WorkflowExecutor{ + PodName: fakePodName, + Template: template, + ClientSet: fakeClientset, + Namespace: fakeNamespace, + RuntimeExecutor: &mockRuntimeExecutor, + } + args, err := we.getKubectlArguments("patch", tt.manifestPath, fakeFlags) + + assert.NoError(t, err) + assert.Equal(t, expectedArgs, args) + }) } - args, err := we.getKubectlArguments("patch", manifestPath, nil) - assert.NoError(t, err) - assert.Equal(t, args, fakeFlags) } // TestResourceConditionsMatching tests whether the JSON response match