diff --git a/pkg/patch/transformer/factory.go b/pkg/patch/transformer/factory.go index 4868a0938f..c59f5e8e8a 100644 --- a/pkg/patch/transformer/factory.go +++ b/pkg/patch/transformer/factory.go @@ -20,7 +20,6 @@ import ( "fmt" "github.com/evanphx/json-patch" - "github.com/krishicks/yaml-patch" "github.com/kubernetes-sigs/kustomize/pkg/loader" "github.com/kubernetes-sigs/kustomize/pkg/patch" "github.com/kubernetes-sigs/kustomize/pkg/resource" @@ -28,16 +27,32 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" ) -// PatchJson6902Factory makes PatchJson6902 transformers. +// PatchJson6902Factory makes PatchJson6902 transformers type PatchJson6902Factory struct { - targetId resource.ResId - operationsYAML yamlpatch.Patch - operationsJSON jsonpatch.Patch + loader loader.Loader } // NewPatchJson6902Factory returns a new PatchJson6902Factory. -func NewPatchJson6902Factory(l loader.Loader, p patch.PatchJson6902) (*PatchJson6902Factory, error) { +func NewPatchJson6902Factory(l loader.Loader) PatchJson6902Factory { + return PatchJson6902Factory{loader: l} +} + +// MakePatchJson6902Transformer returns a transformer for applying Json6902 patch +func (f PatchJson6902Factory) MakePatchJson6902Transformer(patches []patch.PatchJson6902) (transformers.Transformer, error) { + var ts []transformers.Transformer + for _, p := range patches { + t, err := f.makeOnePatchJson6902Transformer(p) + if err != nil { + return nil, err + } + if t != nil { + ts = append(ts, t) + } + } + return transformers.NewMultiTransformerWithConflictCheck(ts), nil +} +func (f PatchJson6902Factory) makeOnePatchJson6902Transformer(p patch.PatchJson6902) (transformers.Transformer, error) { if p.Target == nil { return nil, fmt.Errorf("must specify the target field in patchesJson6902") } @@ -57,26 +72,19 @@ func NewPatchJson6902Factory(l loader.Loader, p patch.PatchJson6902) (*PatchJson ) if p.JsonPatch != nil { - return &PatchJson6902Factory{targetId: targetId, operationsYAML: p.JsonPatch}, nil + return newPatchJson6902YAMLTransformer(targetId, p.JsonPatch) } if p.Path != "" { - rawOp, err := l.Load(p.Path) + rawOp, err := f.loader.Load(p.Path) if err != nil { return nil, err } - patch, err := jsonpatch.DecodePatch(rawOp) + decodedPatch, err := jsonpatch.DecodePatch(rawOp) if err != nil { return nil, err } - return &PatchJson6902Factory{targetId: targetId, operationsJSON: patch}, nil - } - return nil, nil -} + return newPatchJson6902JSONTransformer(targetId, decodedPatch) -// MakePatchJson6902Transformer returns a transformer for applying Json6902 patch -func (f *PatchJson6902Factory) MakePatchJson6902Transformer() (transformers.Transformer, error) { - if f.operationsJSON != nil { - return newPatchJson6902JSONTransformer(f.targetId, f.operationsJSON) } - return newPatchJson6902YAMLTransformer(f.targetId, f.operationsYAML) + return nil, nil } diff --git a/pkg/patch/transformer/factory_test.go b/pkg/patch/transformer/factory_test.go index 060966a966..7e94a8a587 100644 --- a/pkg/patch/transformer/factory_test.go +++ b/pkg/patch/transformer/factory_test.go @@ -17,12 +17,16 @@ limitations under the License. package transformer import ( + "reflect" "strings" "testing" "github.com/kubernetes-sigs/kustomize/pkg/internal/loadertest" "github.com/kubernetes-sigs/kustomize/pkg/patch" + "github.com/kubernetes-sigs/kustomize/pkg/resmap" + "github.com/kubernetes-sigs/kustomize/pkg/resource" "gopkg.in/yaml.v2" + "k8s.io/apimachinery/pkg/runtime/schema" ) func TestNewPatchJson6902FactoryNull(t *testing.T) { @@ -31,18 +35,19 @@ func TestNewPatchJson6902FactoryNull(t *testing.T) { Name: "some-name", }, } - f, err := NewPatchJson6902Factory(nil, p) + f := NewPatchJson6902Factory(nil) + tr, err := f.makeOnePatchJson6902Transformer(p) if err != nil { t.Fatalf("unexpected error : %v", err) } - if f != nil { + if tr != nil { t.Fatal("a nil should be returned") } } func TestNewPatchJson6902FactoryNoTarget(t *testing.T) { p := patch.PatchJson6902{} - _, err := NewPatchJson6902Factory(nil, p) + _, err := NewPatchJson6902Factory(nil).makeOnePatchJson6902Transformer(p) if err == nil { t.Fatal("expected error") } @@ -70,7 +75,8 @@ path: /some/dir/some/file if err != nil { t.Fatalf("expected error %v", err) } - _, err = NewPatchJson6902Factory(nil, p) + f := NewPatchJson6902Factory(nil) + _, err = f.makeOnePatchJson6902Transformer(p) if err == nil { t.Fatal("expected error") } @@ -103,17 +109,13 @@ path: /testpath/patch.json t.Fatal("expected error") } - f, err := NewPatchJson6902Factory(ldr, p) + f := NewPatchJson6902Factory(ldr) + tr, err := f.makeOnePatchJson6902Transformer(p) if err != nil { t.Fatalf("unexpected error : %v", err) } - if f == nil { - t.Fatalf("the returned factory shouldn't be nil ") - } - - _, err = f.MakePatchJson6902Transformer() - if err != nil { - t.Fatalf("unexpected error : %v", err) + if tr == nil { + t.Fatal("the returned transformer should not be nil") } } @@ -139,17 +141,197 @@ jsonPatch: t.Fatalf("unexpected error : %v", err) } - f, err := NewPatchJson6902Factory(nil, p) + f := NewPatchJson6902Factory(nil) + tr, err := f.makeOnePatchJson6902Transformer(p) + if err != nil { + t.Fatalf("unexpected error : %v", err) + } + if tr == nil { + t.Fatal("the returned transformer should not be nil") + } +} + +func TestNewPatchJson6902FactoryMulti(t *testing.T) { + ldr := loadertest.NewFakeLoader("/testpath") + operations := []byte(`[ + {"op": "replace", "path": "/spec/template/spec/containers/0/name", "value": "my-nginx"}, + {"op": "add", "path": "/spec/replica", "value": "3"} +]`) + err := ldr.AddFile("/testpath/patch.json", operations) + if err != nil { + t.Fatalf("Failed to setup fake ldr.") + } + + jsonPatches := []byte(` +- target: + kind: foo + name: some-name + path: /testpath/patch.json + +- target: + kind: foo + name: some-name + jsonPatch: + - op: add + path: /spec/template/spec/containers/0/command + value: ["arg1", "arg2", "arg3"] +`) + var p []patch.PatchJson6902 + err = yaml.Unmarshal(jsonPatches, &p) + if err != nil { + t.Fatalf("unexpected error : %v", err) + } + + f := NewPatchJson6902Factory(ldr) + tr, err := f.MakePatchJson6902Transformer(p) + if err != nil { + t.Fatalf("unexpected error : %v", err) + } + if tr == nil { + t.Fatal("the returned transformer should not be nil") + } + + id := resource.NewResId(schema.GroupVersionKind{Kind: "foo"}, "some-name") + base := resmap.ResMap{ + id: resource.NewResourceFromMap( + map[string]interface{}{ + "kind": "foo", + "metadata": map[string]interface{}{ + "name": "some-name", + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "old-label": "old-value", + }, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "image": "nginx", + "name": "nginx", + }, + }, + }, + }, + }, + }), + } + expected := resmap.ResMap{ + id: resource.NewResourceFromMap( + map[string]interface{}{ + "kind": "foo", + "metadata": map[string]interface{}{ + "name": "some-name", + }, + "spec": map[string]interface{}{ + "replica": "3", + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "old-label": "old-value", + }, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "image": "nginx", + "name": "my-nginx", + "command": []interface{}{ + "arg1", + "arg2", + "arg3", + }, + }, + }, + }, + }, + }, + }), + } + err = tr.Transform(base) if err != nil { t.Fatalf("unexpected error : %v", err) } - if f == nil { - t.Fatalf("the returned factory shouldn't be nil ") + if !reflect.DeepEqual(base, expected) { + err = expected.ErrorIfNotEqual(base) + t.Fatalf("actual doesn't match expected: %v", err) + } +} + +func TestNewPatchJson6902FactoryMultiConflict(t *testing.T) { + ldr := loadertest.NewFakeLoader("/testpath") + operations := []byte(`[ + {"op": "add", "path": "/spec/replica", "value": "3"} +]`) + err := ldr.AddFile("/testpath/patch.json", operations) + if err != nil { + t.Fatalf("Failed to setup fake ldr.") + } + + jsonPatches := []byte(` +- target: + kind: foo + name: some-name + path: /testpath/patch.json + +- target: + kind: foo + name: some-name + jsonPatch: + - op: add + path: /spec/replica + value: 4 +`) + var p []patch.PatchJson6902 + err = yaml.Unmarshal(jsonPatches, &p) + if err != nil { + t.Fatalf("unexpected error : %v", err) } - _, err = f.MakePatchJson6902Transformer() + f := NewPatchJson6902Factory(ldr) + tr, err := f.MakePatchJson6902Transformer(p) if err != nil { t.Fatalf("unexpected error : %v", err) } + if tr == nil { + t.Fatal("the returned transformer should not be nil") + } + + id := resource.NewResId(schema.GroupVersionKind{Kind: "foo"}, "some-name") + base := resmap.ResMap{ + id: resource.NewResourceFromMap( + map[string]interface{}{ + "kind": "foo", + "metadata": map[string]interface{}{ + "name": "somename", + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "old-label": "old-value", + }, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "image": "nginx", + "name": "nginx", + }, + }, + }, + }, + }, + }), + } + err = tr.Transform(base) + if err == nil { + t.Fatal("expected conflict") + } + if !strings.Contains(err.Error(), "found conflict between different patches") { + t.Fatalf("incorrect error happened %v", err) + } } diff --git a/pkg/patch/transformer/patchjson6902json.go b/pkg/patch/transformer/patchjson6902json.go index dacce721a0..d7e69fb608 100644 --- a/pkg/patch/transformer/patchjson6902json.go +++ b/pkg/patch/transformer/patchjson6902json.go @@ -23,7 +23,7 @@ import ( "github.com/kubernetes-sigs/kustomize/pkg/transformers" ) -// patchJson6902Transformer applies patches. +// patchJson6902JSONTransformer applies patches. type patchJson6902JSONTransformer struct { target resource.ResId patch jsonpatch.Patch diff --git a/pkg/patch/transformer/patchjson6902yaml.go b/pkg/patch/transformer/patchjson6902yaml.go index 4dfc739f20..407a212724 100644 --- a/pkg/patch/transformer/patchjson6902yaml.go +++ b/pkg/patch/transformer/patchjson6902yaml.go @@ -24,7 +24,7 @@ import ( "github.com/kubernetes-sigs/kustomize/pkg/transformers" ) -// patchJson6902Transformer applies patches. +// patchJson6902YAMLTransformer applies patches. type patchJson6902YAMLTransformer struct { target resource.ResId patch yamlpatch.Patch diff --git a/pkg/resmap/resmap.go b/pkg/resmap/resmap.go index 3f91366bf5..d0a2586fd8 100644 --- a/pkg/resmap/resmap.go +++ b/pkg/resmap/resmap.go @@ -117,6 +117,16 @@ func (m ResMap) ErrorIfNotEqual(m2 ResMap) error { return nil } +// DeepCopy clone the resmap into a new one +func (m ResMap) DeepCopy() ResMap { + mcopy := make(ResMap) + for id, obj := range m { + mcopy[id] = resource.NewResourceFromUnstruct(obj.Unstructured) + mcopy[id].SetBehavior(obj.Behavior()) + } + return mcopy +} + func (m ResMap) insert(newName string, obj *unstructured.Unstructured) error { oldName := obj.GetName() gvk := obj.GroupVersionKind() diff --git a/pkg/transformers/multitransformer.go b/pkg/transformers/multitransformer.go index cc36a099db..da5ae48753 100644 --- a/pkg/transformers/multitransformer.go +++ b/pkg/transformers/multitransformer.go @@ -16,11 +16,16 @@ limitations under the License. package transformers -import "github.com/kubernetes-sigs/kustomize/pkg/resmap" +import ( + "fmt" + + "github.com/kubernetes-sigs/kustomize/pkg/resmap" +) // multiTransformer contains a list of transformers. type multiTransformer struct { - transformers []Transformer + transformers []Transformer + checkConflictEnabled bool } var _ Transformer = &multiTransformer{} @@ -28,13 +33,29 @@ var _ Transformer = &multiTransformer{} // NewMultiTransformer constructs a multiTransformer. func NewMultiTransformer(t []Transformer) Transformer { r := &multiTransformer{ - transformers: make([]Transformer, len(t))} + transformers: make([]Transformer, len(t)), + checkConflictEnabled: false} + copy(r.transformers, t) + return r +} + +// NewMultiTransformerWithConflictCheck constructs a multiTransformer with checking of conflicts. +func NewMultiTransformerWithConflictCheck(t []Transformer) Transformer { + r := &multiTransformer{ + transformers: make([]Transformer, len(t)), + checkConflictEnabled: true} copy(r.transformers, t) return r } // Transform prepends the name prefix. func (o *multiTransformer) Transform(m resmap.ResMap) error { + if o.checkConflictEnabled { + return o.transformWithCheckConflict(m) + } + return o.transform(m) +} +func (o *multiTransformer) transform(m resmap.ResMap) error { for _, t := range o.transformers { err := t.Transform(m) if err != nil { @@ -43,3 +64,30 @@ func (o *multiTransformer) Transform(m resmap.ResMap) error { } return nil } + +// Of the len(o.transformers)! possible transformer orderings, compare to a reversed order. +// A spot check to perform when the transformations are supposed to be commutative. +// Fail if there's a difference in the result. +func (o *multiTransformer) transformWithCheckConflict(m resmap.ResMap) error { + mcopy := m.DeepCopy() + err := o.transform(m) + if err != nil { + return err + } + o.reverseTransformers() + err = o.transform(mcopy) + if err != nil { + return err + } + err = m.ErrorIfNotEqual(mcopy) + if err != nil { + return fmt.Errorf("found conflict between different patches\n%v", err) + } + return nil +} + +func (o *multiTransformer) reverseTransformers() { + for i, j := 0, len(o.transformers)-1; i < j; i, j = i+1, j-1 { + o.transformers[i], o.transformers[j] = o.transformers[j], o.transformers[i] + } +}