"empty" patch (still) results in error

[ralgozino]

What happened?

Even though #5487 was closed by #5771, kustomize (v5.7.0 and 5.7.1) still fails with an error if a patch file contains only comments or is empty:

$ cat comments-only-patch.yaml
# This is a comment

$ cat empty-patch.yaml


$ cat kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
  - path: comments-only-patch.yaml
  - path: empty-patch.yaml

$ kustomize build
Error: must specify a target for JSON patch [path: "comments-only-patch.yaml"]

$ kustomize version
v5.7.1

Commenting out the patch with the comments results in the same error:

$ cat empty-patch.yaml


$ cat kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
  # - path: comments-only-patch.yaml
  - path: empty-patch.yaml

$ kustomize build
Error: must specify a target for JSON patch [path: "empty-patch.yaml"]

NOTE: adding a resource does not make a difference.

NOTE2: the error has indeed changed from previous versions, before the merge of #5771 the error was something like this:

Error: trouble configuring builtin PatchTransformer with config: `
path: comments-only-patch.yaml
`: illegally qualifies as both an SM and JSON patch: [path: "comments-only-patch.yaml"]

cc @jchanam that worked on the PR just for visibility :) If I'm "holding it wrong" please tell me

What did you expect to happen?

According to #5771 I was expecting that the empty patches would be ignored and not produce an error.

How can we reproduce it (as minimally and precisely as possible)?

# kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- resource.yaml
patches:
- empty.yaml

# empty.yaml
# This is a comment on an "empty" file

# resource.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: dummy
data:
  dummy: value

Expected output

apiVersion: v1
kind: ConfigMap
metadata:
  name: dummy
data:
  dummy: value

Actual output

Error: must specify a target for JSON patch [path: "empty.yaml"]

Kustomize version

5.7.1

Operating system

MacOS

[jchanam]

@ralgozino this is very interesting! What I saw (and fixed) was when there were a target for the patch but the patch file was empty.

I think this is different and the code managing this error seems different. Have you checked where the code for this is? I agree that it should not fail.

[ralgozino]

Hola Julio!

I'm not familiar with kustmoize's code base, but doing some debugging I believe the fix could be the following:

diff --git i/api/internal/builtins/PatchTransformer.go w/api/internal/builtins/PatchTransformer.go
index 6161ada86..4184d4ef3 100644
--- i/api/internal/builtins/PatchTransformer.go
+++ w/api/internal/builtins/PatchTransformer.go
@@ -127,6 +127,9 @@ func (p *PatchTransformerPlugin) transformStrategicMerge(m resmap.ResMap) error
 
 // transformJson6902 applies json6902 Patch to all the resources in the ResMap that match Target.
 func (p *PatchTransformerPlugin) transformJson6902(m resmap.ResMap) error {
+	if p.jsonPatches == nil {
+		return nil
+	}
 	if p.Target == nil {
 		return fmt.Errorf("must specify a target for JSON patch %s", p.patchSource)
 	}

I think that an early return if there are no patches to apply in the transformJson6902 function is the right way.

It actually works as expected with this patch (using the same example as above):

$ ~/src/github.com/kubernetes-sigs/kustomize/kustomize/kustomize build # kustomize binary built with this patch
apiVersion: v1
data:
  dummy: value
kind: ConfigMap
metadata:
  name: dummy

Adding a patch without target errors out as expected:

$ cat kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - resource.yaml
patches:
  - path: comments-only-patch.yaml
  - patch: |-
      - op: add
        path: /data/test
        value: "foo"
  #   target:
  #     version: v1
  #     kind: ConfigMap

$ ~/src/github.com/kubernetes-sigs/kustomize/kustomize/kustomize build
Error: must specify a target for JSON patch [patch: "- op: add\n  path: /data/test\n  value: \"foo\""]

Adding the target, the patch gets applied:

$ cat kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - resource.yaml
patches:
  - path: comments-only-patch.yaml
  - patch: |-
      - op: add
        path: /data/test
        value: "foo"
    target:
      version: v1
      kind: ConfigMap

$ ~/src/github.com/kubernetes-sigs/kustomize/kustomize/kustomize build
apiVersion: v1
data:
  dummy: value
  test: foo
kind: ConfigMap
metadata:
  name: dummy

[sarab97]

@ralgozino good find. Agreed this should be fixed. Seeing as you are exploring if you would like to raise the PR you can do so.

/triage accepted

[k8s-ci-robot]

@sarab97: The label triage/accepted cannot be applied. Only GitHub organization members can add the label.

In response to this:

@ralgozino good find. Agreed this should be fixed. Seeing as you are exploring if you would like to raise the PR you can do so.

/triage accepted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[ralgozino]

Sure, I'll check the contributing docs ans open the PR 👍

[sarab97]

/triage accepted

[ralgozino]

Hello @sarab97

I tried to tackle this issue and open a PR with the fix but, even though I have the fix working, I'm stuck on the first step: writing a regression test.

When I was going to write the test I found that @jchanam already has written two tests that should cover the behaviour that I'm seeing (i.e. test for no errors when the patch is empty or has only comments):

kustomize/plugin/builtin/patchtransformer/PatchTransformer_test.go

Lines 1112 to 1161 in cd30471

	func TestPatchTransformerPatchEmpty(t *testing.T) {
	th := kusttest_test.MakeEnhancedHarness(t).
	PrepBuiltin("PatchTransformer")
	defer th.Reset()
	th.WriteF("patch.yaml", `
	`)
	th.RunTransformerAndCheckError(`
	apiVersion: builtin
	kind: PatchTransformer
	metadata:
	name: notImportantHere
	path: patch.yaml
	target:
	name: myDeploy
	`, someDeploymentResources, func(t *testing.T, err error) {
	t.Helper()
	if err != nil {
	t.Fatalf("unexpected error")
	}
	})
	}
	func TestPatchTransformerPatchEmptyOnlyComments(t *testing.T) {
	th := kusttest_test.MakeEnhancedHarness(t).
	PrepBuiltin("PatchTransformer")
	defer th.Reset()
	th.WriteF("patch.yaml", `
	# File with only comments
	# Is a virtually empty yaml
	`)
	th.RunTransformerAndCheckError(`
	apiVersion: builtin
	kind: PatchTransformer
	metadata:
	name: notImportantHere
	path: patch.yaml
	target:
	name: myDeploy
	`, someDeploymentResources, func(t *testing.T, err error) {
	t.Helper()
	if err != nil {
	t.Fatalf("unexpected error")
	}
	})
	}

I can't figure out why these tests are not failing though, based on what I described in my first message they should fail.

Any hints on what could be going on? What am I missing?