fix(resource): don't use -f when patch file is provided (#13317)

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
(cherry picked from commit 5d8ee22)

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)
 

examples/k8s-patch.yaml → 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}}"

examples/k8s-json-patch-workflow.yaml → 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}}"

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
+

examples/k8s-patch-basic.yaml → 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

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 <kind> <name> --patch-file <path>`
+		// json patches also use patch files by definition and so require resource arguments
+		// the other form in our case is `kubectl patch -f <path> --patch-file <path>`
+		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 {

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