fix a patch files accept multiple patches (#5194)

* fix a patch files accept multiple patches

* fix comments and variable name

* add error handling when using not allowed multiple strategic-merge patches

* fix error message of Multiple Strategic-Merge Patch file

* refactor transformStrategicMerge()

* add TODO comment and test for Multiple JSON patch Yaml documents are not allowed

* refactoring PatchTransformer

* add multiple patch test for PatchTransformer package

* improve error message to PatchTransformer

* refactor const and error message check

* fix some error messages
This commit is contained in:
yugo kobayashi
2023-09-16 08:20:13 +09:00
committed by GitHub
parent 56d37acc7d
commit 59696d1ace
5 changed files with 386 additions and 136 deletions

View File

@@ -12,99 +12,118 @@ import (
"sigs.k8s.io/kustomize/api/resmap"
"sigs.k8s.io/kustomize/api/resource"
"sigs.k8s.io/kustomize/api/types"
"sigs.k8s.io/kustomize/kyaml/errors"
"sigs.k8s.io/kustomize/kyaml/kio/kioutil"
"sigs.k8s.io/yaml"
)
type PatchTransformerPlugin struct {
loadedPatch *resource.Resource
decodedPatch jsonpatch.Patch
Path string `json:"path,omitempty" yaml:"path,omitempty"`
Patch string `json:"patch,omitempty" yaml:"patch,omitempty"`
Target *types.Selector `json:"target,omitempty" yaml:"target,omitempty"`
Options map[string]bool `json:"options,omitempty" yaml:"options,omitempty"`
smPatches []*resource.Resource // strategic-merge patches
jsonPatches jsonpatch.Patch // json6902 patch
// patchText is pure patch text created by Path or Patch
patchText string
// patchSource is patch source message
patchSource string
Path string `json:"path,omitempty" yaml:"path,omitempty"`
Patch string `json:"patch,omitempty" yaml:"patch,omitempty"`
Target *types.Selector `json:"target,omitempty" yaml:"target,omitempty"`
Options map[string]bool `json:"options,omitempty" yaml:"options,omitempty"`
}
func (p *PatchTransformerPlugin) Config(
h *resmap.PluginHelpers, c []byte) error {
err := yaml.Unmarshal(c, p)
if err != nil {
func (p *PatchTransformerPlugin) Config(h *resmap.PluginHelpers, c []byte) error {
if err := yaml.Unmarshal(c, p); err != nil {
return err
}
p.Patch = strings.TrimSpace(p.Patch)
if p.Patch == "" && p.Path == "" {
return fmt.Errorf(
"must specify one of patch and path in\n%s", string(c))
}
if p.Patch != "" && p.Path != "" {
return fmt.Errorf(
"patch and path can't be set at the same time\n%s", string(c))
}
if p.Path != "" {
loaded, loadErr := h.Loader().Load(p.Path)
if loadErr != nil {
return loadErr
switch {
case p.Patch == "" && p.Path == "":
return fmt.Errorf("must specify one of patch and path in\n%s", string(c))
case p.Patch != "" && p.Path != "":
return fmt.Errorf("patch and path can't be set at the same time\n%s", string(c))
case p.Patch != "":
p.patchText = p.Patch
p.patchSource = fmt.Sprintf("[patch: %q]", p.patchText)
case p.Path != "":
loaded, err := h.Loader().Load(p.Path)
if err != nil {
return fmt.Errorf("failed to get the patch file from path(%s): %w", p.Path, err)
}
p.Patch = string(loaded)
p.patchText = string(loaded)
p.patchSource = fmt.Sprintf("[path: %q]", p.Path)
}
patchSM, errSM := h.ResmapFactory().RF().FromBytes([]byte(p.Patch))
patchJson, errJson := jsonPatchFromBytes([]byte(p.Patch))
patchesSM, errSM := h.ResmapFactory().RF().SliceFromBytes([]byte(p.patchText))
patchesJson, errJson := jsonPatchFromBytes([]byte(p.patchText))
if (errSM == nil && errJson == nil) ||
(patchSM != nil && patchJson != nil) {
(patchesSM != nil && patchesJson != nil) {
return fmt.Errorf(
"illegally qualifies as both an SM and JSON patch: [%v]",
p.Patch)
"illegally qualifies as both an SM and JSON patch: %s",
p.patchSource)
}
if errSM != nil && errJson != nil {
return fmt.Errorf(
"unable to parse SM or JSON patch from [%v]", p.Patch)
"unable to parse SM or JSON patch from %s", p.patchSource)
}
if errSM == nil {
p.loadedPatch = patchSM
if p.Options["allowNameChange"] {
p.loadedPatch.AllowNameChange()
}
if p.Options["allowKindChange"] {
p.loadedPatch.AllowKindChange()
p.smPatches = patchesSM
for _, loadedPatch := range p.smPatches {
if p.Options["allowNameChange"] {
loadedPatch.AllowNameChange()
}
if p.Options["allowKindChange"] {
loadedPatch.AllowKindChange()
}
}
} else {
p.decodedPatch = patchJson
p.jsonPatches = patchesJson
}
return nil
}
func (p *PatchTransformerPlugin) Transform(m resmap.ResMap) error {
if p.loadedPatch == nil {
return p.transformJson6902(m, p.decodedPatch)
if p.smPatches != nil {
return p.transformStrategicMerge(m)
}
// The patch was a strategic merge patch
return p.transformStrategicMerge(m, p.loadedPatch)
return p.transformJson6902(m)
}
// transformStrategicMerge applies the provided strategic merge patch
// to all the resources in the ResMap that match either the Target or
// the identifier of the patch.
func (p *PatchTransformerPlugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resource) error {
if p.Target == nil {
// transformStrategicMerge applies each loaded strategic merge patch
// to the resource in the ResMap that matches the identifier of the patch.
// If only one patch is specified, the Target can be used instead.
func (p *PatchTransformerPlugin) transformStrategicMerge(m resmap.ResMap) error {
if p.Target != nil {
if len(p.smPatches) > 1 {
// detail: https://github.com/kubernetes-sigs/kustomize/issues/5049#issuecomment-1440604403
return fmt.Errorf("Multiple Strategic-Merge Patches in one `patches` entry is not allowed to set `patches.target` field: %s", p.patchSource)
}
// single patch
patch := p.smPatches[0]
selected, err := m.Select(*p.Target)
if err != nil {
return fmt.Errorf("unable to find patch target %q in `resources`: %w", p.Target, err)
}
return errors.Wrap(m.ApplySmPatch(resource.MakeIdSet(selected), patch))
}
for _, patch := range p.smPatches {
target, err := m.GetById(patch.OrgId())
if err != nil {
return err
return fmt.Errorf("no resource matches strategic merge patch %q: %w", patch.OrgId(), err)
}
if err := target.ApplySmPatch(patch); err != nil {
return errors.Wrap(err)
}
return target.ApplySmPatch(patch)
}
selected, err := m.Select(*p.Target)
if err != nil {
return err
}
return m.ApplySmPatch(resource.MakeIdSet(selected), patch)
return nil
}
// transformJson6902 applies the provided json6902 patch
// to all the resources in the ResMap that match the Target.
func (p *PatchTransformerPlugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) 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.Target == nil {
return fmt.Errorf("must specify a target for patch %s", p.Patch)
return fmt.Errorf("must specify a target for JSON patch %s", p.patchSource)
}
resources, err := m.Select(*p.Target)
if err != nil {
@@ -114,7 +133,7 @@ func (p *PatchTransformerPlugin) transformJson6902(m resmap.ResMap, patch jsonpa
res.StorePreviousId()
internalAnnotations := kioutil.GetInternalAnnotations(&res.RNode)
err = res.ApplyFilter(patchjson6902.Filter{
Patch: p.Patch,
Patch: p.patchText,
})
if err != nil {
return err
@@ -129,16 +148,17 @@ func (p *PatchTransformerPlugin) transformJson6902(m resmap.ResMap, patch jsonpa
return nil
}
// jsonPatchFromBytes loads a Json 6902 patch from
// a bytes input
func jsonPatchFromBytes(
in []byte) (jsonpatch.Patch, error) {
// jsonPatchFromBytes loads a Json 6902 patch from a bytes input
func jsonPatchFromBytes(in []byte) (jsonpatch.Patch, error) {
ops := string(in)
if ops == "" {
return nil, fmt.Errorf("empty json patch operations")
}
if ops[0] != '[' {
// TODO(5049):
// In the case of multiple yaml documents, return error instead of ignoring all but first.
// Details: https://github.com/kubernetes-sigs/kustomize/pull/5194#discussion_r1256686728
jsonOps, err := yaml.YAMLToJSON(in)
if err != nil {
return nil, err

View File

@@ -6,8 +6,6 @@ package krusty_test
import (
"testing"
"github.com/stretchr/testify/assert"
kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
)
@@ -303,20 +301,15 @@ patchesStrategicMerge:
m = th.Run("overlay", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, expected)
// Technique 4: "patches:" field, one patch file. Fails.
// Technique 4: "patches:" field, one patch file.
th.WriteK("overlay", `
resources:
- ../base
patches:
- path: twoPatchesInOneFile.yaml
`)
err := th.RunWithErr("overlay", th.MakeDefaultOptions())
assert.Error(t, err)
// This should fail, because the semantics of the `patches` field.
// That field allows specific patch targeting to a list of targets,
// while the `patchesStrategicMerge` field accepts patches that
// implicitly identify their targets via GVKN.
assert.Contains(t, err.Error(), "unable to parse SM or JSON patch from ")
m = th.Run("overlay", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, expected)
}
func TestRemoveEmptyDirWithNullFieldInSmp(t *testing.T) {
@@ -1670,7 +1663,7 @@ spec:
template:
spec:
containers:
- image: fluentd:latest
- image: fluentd:latest
name: fluentd
serviceAccountName: fluentd-sa
---

View File

@@ -696,8 +696,7 @@ func (m *resWrangler) DeAnchor() (err error) {
}
// ApplySmPatch applies the patch, and errors on Id collisions.
func (m *resWrangler) ApplySmPatch(
selectedSet *resource.IdSet, patch *resource.Resource) error {
func (m *resWrangler) ApplySmPatch(selectedSet *resource.IdSet, patch *resource.Resource) error {
var list []*resource.Resource
for _, res := range m.rList {
if selectedSet.Contains(res.CurId()) {

View File

@@ -13,101 +13,120 @@ import (
"sigs.k8s.io/kustomize/api/resmap"
"sigs.k8s.io/kustomize/api/resource"
"sigs.k8s.io/kustomize/api/types"
"sigs.k8s.io/kustomize/kyaml/errors"
"sigs.k8s.io/kustomize/kyaml/kio/kioutil"
"sigs.k8s.io/yaml"
)
type plugin struct {
loadedPatch *resource.Resource
decodedPatch jsonpatch.Patch
Path string `json:"path,omitempty" yaml:"path,omitempty"`
Patch string `json:"patch,omitempty" yaml:"patch,omitempty"`
Target *types.Selector `json:"target,omitempty" yaml:"target,omitempty"`
Options map[string]bool `json:"options,omitempty" yaml:"options,omitempty"`
smPatches []*resource.Resource // strategic-merge patches
jsonPatches jsonpatch.Patch // json6902 patch
// patchText is pure patch text created by Path or Patch
patchText string
// patchSource is patch source message
patchSource string
Path string `json:"path,omitempty" yaml:"path,omitempty"`
Patch string `json:"patch,omitempty" yaml:"patch,omitempty"`
Target *types.Selector `json:"target,omitempty" yaml:"target,omitempty"`
Options map[string]bool `json:"options,omitempty" yaml:"options,omitempty"`
}
var KustomizePlugin plugin //nolint:gochecknoglobals
func (p *plugin) Config(
h *resmap.PluginHelpers, c []byte) error {
err := yaml.Unmarshal(c, p)
if err != nil {
func (p *plugin) Config(h *resmap.PluginHelpers, c []byte) error {
if err := yaml.Unmarshal(c, p); err != nil {
return err
}
p.Patch = strings.TrimSpace(p.Patch)
if p.Patch == "" && p.Path == "" {
return fmt.Errorf(
"must specify one of patch and path in\n%s", string(c))
}
if p.Patch != "" && p.Path != "" {
return fmt.Errorf(
"patch and path can't be set at the same time\n%s", string(c))
}
if p.Path != "" {
loaded, loadErr := h.Loader().Load(p.Path)
if loadErr != nil {
return loadErr
switch {
case p.Patch == "" && p.Path == "":
return fmt.Errorf("must specify one of patch and path in\n%s", string(c))
case p.Patch != "" && p.Path != "":
return fmt.Errorf("patch and path can't be set at the same time\n%s", string(c))
case p.Patch != "":
p.patchText = p.Patch
p.patchSource = fmt.Sprintf("[patch: %q]", p.patchText)
case p.Path != "":
loaded, err := h.Loader().Load(p.Path)
if err != nil {
return fmt.Errorf("failed to get the patch file from path(%s): %w", p.Path, err)
}
p.Patch = string(loaded)
p.patchText = string(loaded)
p.patchSource = fmt.Sprintf("[path: %q]", p.Path)
}
patchSM, errSM := h.ResmapFactory().RF().FromBytes([]byte(p.Patch))
patchJson, errJson := jsonPatchFromBytes([]byte(p.Patch))
patchesSM, errSM := h.ResmapFactory().RF().SliceFromBytes([]byte(p.patchText))
patchesJson, errJson := jsonPatchFromBytes([]byte(p.patchText))
if (errSM == nil && errJson == nil) ||
(patchSM != nil && patchJson != nil) {
(patchesSM != nil && patchesJson != nil) {
return fmt.Errorf(
"illegally qualifies as both an SM and JSON patch: [%v]",
p.Patch)
"illegally qualifies as both an SM and JSON patch: %s",
p.patchSource)
}
if errSM != nil && errJson != nil {
return fmt.Errorf(
"unable to parse SM or JSON patch from [%v]", p.Patch)
"unable to parse SM or JSON patch from %s", p.patchSource)
}
if errSM == nil {
p.loadedPatch = patchSM
if p.Options["allowNameChange"] {
p.loadedPatch.AllowNameChange()
}
if p.Options["allowKindChange"] {
p.loadedPatch.AllowKindChange()
p.smPatches = patchesSM
for _, loadedPatch := range p.smPatches {
if p.Options["allowNameChange"] {
loadedPatch.AllowNameChange()
}
if p.Options["allowKindChange"] {
loadedPatch.AllowKindChange()
}
}
} else {
p.decodedPatch = patchJson
p.jsonPatches = patchesJson
}
return nil
}
func (p *plugin) Transform(m resmap.ResMap) error {
if p.loadedPatch == nil {
return p.transformJson6902(m, p.decodedPatch)
if p.smPatches != nil {
return p.transformStrategicMerge(m)
}
// The patch was a strategic merge patch
return p.transformStrategicMerge(m, p.loadedPatch)
return p.transformJson6902(m)
}
// transformStrategicMerge applies the provided strategic merge patch
// to all the resources in the ResMap that match either the Target or
// the identifier of the patch.
func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resource) error {
if p.Target == nil {
// transformStrategicMerge applies each loaded strategic merge patch
// to the resource in the ResMap that matches the identifier of the patch.
// If only one patch is specified, the Target can be used instead.
func (p *plugin) transformStrategicMerge(m resmap.ResMap) error {
if p.Target != nil {
if len(p.smPatches) > 1 {
// detail: https://github.com/kubernetes-sigs/kustomize/issues/5049#issuecomment-1440604403
return fmt.Errorf("Multiple Strategic-Merge Patches in one `patches` entry is not allowed to set `patches.target` field: %s", p.patchSource)
}
// single patch
patch := p.smPatches[0]
selected, err := m.Select(*p.Target)
if err != nil {
return fmt.Errorf("unable to find patch target %q in `resources`: %w", p.Target, err)
}
return errors.Wrap(m.ApplySmPatch(resource.MakeIdSet(selected), patch))
}
for _, patch := range p.smPatches {
target, err := m.GetById(patch.OrgId())
if err != nil {
return err
return fmt.Errorf("no resource matches strategic merge patch %q: %w", patch.OrgId(), err)
}
if err := target.ApplySmPatch(patch); err != nil {
return errors.Wrap(err)
}
return target.ApplySmPatch(patch)
}
selected, err := m.Select(*p.Target)
if err != nil {
return err
}
return m.ApplySmPatch(resource.MakeIdSet(selected), patch)
return nil
}
// transformJson6902 applies the provided json6902 patch
// to all the resources in the ResMap that match the Target.
func (p *plugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) error {
// transformJson6902 applies json6902 Patch to all the resources in the ResMap that match Target.
func (p *plugin) transformJson6902(m resmap.ResMap) error {
if p.Target == nil {
return fmt.Errorf("must specify a target for patch %s", p.Patch)
return fmt.Errorf("must specify a target for JSON patch %s", p.patchSource)
}
resources, err := m.Select(*p.Target)
if err != nil {
@@ -117,7 +136,7 @@ func (p *plugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) error
res.StorePreviousId()
internalAnnotations := kioutil.GetInternalAnnotations(&res.RNode)
err = res.ApplyFilter(patchjson6902.Filter{
Patch: p.Patch,
Patch: p.patchText,
})
if err != nil {
return err
@@ -132,16 +151,17 @@ func (p *plugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) error
return nil
}
// jsonPatchFromBytes loads a Json 6902 patch from
// a bytes input
func jsonPatchFromBytes(
in []byte) (jsonpatch.Patch, error) {
// jsonPatchFromBytes loads a Json 6902 patch from a bytes input
func jsonPatchFromBytes(in []byte) (jsonpatch.Patch, error) {
ops := string(in)
if ops == "" {
return nil, fmt.Errorf("empty json patch operations")
}
if ops[0] != '[' {
// TODO(5049):
// In the case of multiple yaml documents, return error instead of ignoring all but first.
// Details: https://github.com/kubernetes-sigs/kustomize/pull/5194#discussion_r1256686728
jsonOps, err := yaml.YAMLToJSON(in)
if err != nil {
return nil, err

View File

@@ -7,6 +7,7 @@ import (
"strings"
"testing"
"github.com/stretchr/testify/require"
kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
)
@@ -124,7 +125,7 @@ patch: '[{"op": "add", "path": "/spec/template/spec/dnsPolicy", "value": "Cluste
t.Fatalf("expected error")
}
if !strings.Contains(err.Error(),
"must specify a target for patch") {
"must specify a target for JSON patch") {
t.Fatalf("unexpected err: %v", err)
}
})
@@ -198,6 +199,223 @@ Patch: "something"
})
}
const (
multipleSMPatchesFile = `
apiVersion: apps/v1
kind: Deployment
metadata:
name: myDeploy
spec:
template:
spec:
containers:
- image: public.ecr.aws/nginx/nginx:mainline
name: nginx
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: yourDeploy
spec:
template:
metadata:
labels:
new-label: new-value-with-multipleSMPatchesFile
`
multipleSMPatchesSuccesfulResult = `
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
old-label: old-value
name: myDeploy
spec:
replica: 2
template:
metadata:
labels:
old-label: old-value
spec:
containers:
- image: public.ecr.aws/nginx/nginx:mainline
name: nginx
---
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
new-label: new-value
name: yourDeploy
spec:
replica: 1
template:
metadata:
labels:
new-label: new-value-with-multipleSMPatchesFile
spec:
containers:
- image: nginx:1.7.9
name: nginx
---
apiVersion: apps/v1
kind: MyKind
metadata:
label:
old-label: old-value
name: myDeploy
spec:
template:
metadata:
labels:
old-label: old-value
spec:
containers:
- image: nginx
name: nginx
`
)
func TestMultipleSMPatchesWithFilePath(t *testing.T) {
th := kusttest_test.MakeEnhancedHarness(t).
PrepBuiltin("PatchTransformer")
defer th.Reset()
th.WriteF(`multiplepatches.yaml`, multipleSMPatchesFile)
th.RunTransformerAndCheckResult(`
apiVersion: builtin
kind: PatchTransformer
metadata:
name: notImportantHere
Path: multiplepatches.yaml
`, someDeploymentResources, multipleSMPatchesSuccesfulResult)
}
func TestMultipleSMPatchesWithPatch(t *testing.T) {
th := kusttest_test.MakeEnhancedHarness(t).
PrepBuiltin("PatchTransformer")
defer th.Reset()
th.RunTransformerAndCheckResult(`
apiVersion: builtin
kind: PatchTransformer
metadata:
name: notImportantHere
patch: |-
apiVersion: apps/v1
kind: Deployment
metadata:
name: myDeploy
spec:
template:
spec:
containers:
- image: public.ecr.aws/nginx/nginx:mainline
name: nginx
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: yourDeploy
spec:
template:
metadata:
labels:
new-label: new-value-with-multipleSMPatchesFile
`, someDeploymentResources, multipleSMPatchesSuccesfulResult)
}
func TestMultipleSMPatchesAndTarget(t *testing.T) {
th := kusttest_test.MakeEnhancedHarness(t).
PrepBuiltin("PatchTransformer")
defer th.Reset()
th.WriteF(`multiplepatches.yaml`, multipleSMPatchesFile)
th.RunTransformerAndCheckError(`
apiVersion: builtin
kind: PatchTransformer
metadata:
name: notImportantHere
Path: multiplepatches.yaml
target:
name: .*Deploy
kind: Deployment
`, someDeploymentResources, func(t *testing.T, err error) {
t.Helper()
require.ErrorContains(t, err, "Multiple Strategic-Merge Patches in one `patches` entry is not allowed to set `patches.target` field")
})
}
const (
oneDeployment = `
apiVersion: apps/v1
metadata:
name: oneDeploy
kind: Deployment
spec:
replica: 1
template:
spec:
containers:
- name: nginx
image: nginx:1.7.9
- name: sidecar
image: busybox:1.36.1
`
multiplePatchTransformerConfig = `
apiVersion: builtin
kind: PatchTransformer
metadata:
name: notImportantHere
patch: |-
- op: replace
path: /spec/template/spec/containers/0/image
value: nginx:latest
---
- op: replace
path: /spec/template/spec/containers/1/image
value: busybox:latest
target:
name: .*Deploy
kind: Deployment
`
)
func TestPatchTransformerNotAllowedMultipleJsonPatches(t *testing.T) {
th := kusttest_test.MakeEnhancedHarness(t).
PrepBuiltin("PatchTransformer")
defer th.Reset()
// TODO(5049): Multiple JSON patch Yaml documents are not allowed and need to return error.
th.RunTransformerAndCheckResult(multiplePatchTransformerConfig, oneDeployment, `
apiVersion: apps/v1
kind: Deployment
metadata:
name: oneDeploy
spec:
replica: 1
template:
spec:
containers:
- image: nginx:latest
name: nginx
- image: busybox:1.36.1
name: sidecar
`)
// th.RunTransformerAndCheckError(multiplePatchTransformerConfig, oneDeployment, func(t *testing.T, err error) {
// t.Helper()
// if err == nil {
// t.Fatalf("expected error")
// }
// if !strings.Contains(err.Error(),
// "Multiple Json6902 Patch in 'patches' is not allowed.") {
// t.Fatalf("unexpected err: %v", err)
// }
// })
}
func TestPatchTransformerFromFiles(t *testing.T) {
th := kusttest_test.MakeEnhancedHarness(t).
PrepBuiltin("PatchTransformer")