From 62d14b574ff127d12b664695999b1cf2e3449584 Mon Sep 17 00:00:00 2001 From: jregan Date: Thu, 3 Dec 2020 07:45:17 -0800 Subject: [PATCH] Extract conflict detection to it's own interface. This PR - defines a patch conflict detector interface, - extracts implementations of the interface from the merginator code, making the merginator code independent of --enable_kyaml. - injects those implementations into kustomize as a function of --enable_kyaml. So, instead of using different merginators to combine resmaps, this pr allows the use of a single patch merge code path that uses different conflict detectors. So instead of debating how to merge, we're now only considering whether to warn on conflict detection in one transformer. This PR is in service of #3304, eliminating seven instances where --enable_kyaml was consulted. These were cases where conflict detection wasn't an issue (but merging patches was). --- .../PatchStrategicMergeTransformer.go | 5 + api/go.sum | 3 + api/internal/conflict/factory.go | 23 ++ .../conflict/smpatchmergeonlydetector.go | 28 ++ .../k8sdeps/conflict/conflictdetectorjson.go | 43 ++++ .../k8sdeps/conflict/conflictdetectorsm.go | 65 +++++ api/internal/k8sdeps/conflict/factory.go | 45 ++++ api/internal/k8sdeps/merge/merginator.go | 239 ------------------ api/internal/merge/merginator.go | 31 --- api/internal/merge/merginator_test.go | 4 - .../plugins/execplugin/execplugin_test.go | 3 +- api/internal/plugins/loader/loader_test.go | 3 +- api/internal/target/maker_test.go | 3 +- api/krusty/kustomizer.go | 2 +- api/provider/depprovider.go | 52 ++-- api/resmap/factory.go | 20 +- api/resmap/merginator.go | 118 +++++++++ api/resource/conflictdetector.go | 20 ++ api/testutils/kusttest/harnessenhanced.go | 3 +- .../krmfunction/funcwrappersrc/main.go | 3 +- .../PatchStrategicMergeTransformer.go | 6 + .../PatchStrategicMergeTransformer_test.go | 143 +---------- 22 files changed, 418 insertions(+), 444 deletions(-) create mode 100644 api/internal/conflict/factory.go create mode 100644 api/internal/conflict/smpatchmergeonlydetector.go create mode 100644 api/internal/k8sdeps/conflict/conflictdetectorjson.go create mode 100644 api/internal/k8sdeps/conflict/conflictdetectorsm.go create mode 100644 api/internal/k8sdeps/conflict/factory.go delete mode 100644 api/internal/k8sdeps/merge/merginator.go delete mode 100644 api/internal/merge/merginator.go delete mode 100644 api/internal/merge/merginator_test.go create mode 100644 api/resmap/merginator.go create mode 100644 api/resource/conflictdetector.go diff --git a/api/builtins/PatchStrategicMergeTransformer.go b/api/builtins/PatchStrategicMergeTransformer.go index 7b9bbb17164..e2f61238f9d 100644 --- a/api/builtins/PatchStrategicMergeTransformer.go +++ b/api/builtins/PatchStrategicMergeTransformer.go @@ -31,6 +31,11 @@ func (p *PatchStrategicMergeTransformerPlugin) Config( } if len(p.Paths) != 0 { for _, onePath := range p.Paths { + // The following oddly attempts to interpret a path string as an + // actual patch (instead of as a path to a file containing a patch). + // All tests pass if this code is commented out. This code should + // be deleted; the user should use the Patches field which + // exists for this purpose (inline patch declaration). res, err := p.h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) if err == nil { p.loadedPatches = append(p.loadedPatches, res...) diff --git a/api/go.sum b/api/go.sum index 9f0a9748f98..c361fd12ece 100644 --- a/api/go.sum +++ b/api/go.sum @@ -167,6 +167,7 @@ github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7a github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zVXpSg4= github.com/gogo/protobuf v1.2.2-0.20190723190241-65acae22fc9d h1:3PaI8p3seN09VjbTYC/QWlUZdZ1qS1zGjy7LH2Wt07I= github.com/gogo/protobuf v1.2.2-0.20190723190241-65acae22fc9d/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o= +github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b h1:VKtxabqXZkF25pY9ekfRL6a582T4P37/31XEstQ5p58= github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q= github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= @@ -226,6 +227,7 @@ github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+ github.com/googleapis/gnostic v0.0.0-20170729233727-0c5108395e2d h1:7XGaL1e6bYS1yIonGp9761ExpPPV1ui0SAC59Yube9k= github.com/googleapis/gnostic v0.0.0-20170729233727-0c5108395e2d/go.mod h1:sJBsCZ4ayReDTBIg8b9dl28c5xFWyhBTVRp3pOg5EKY= github.com/gophercloud/gophercloud v0.1.0/go.mod h1:vxM41WHh5uqHVBMZHzuwNOHh8XEoIEcSTewFxm1c5g8= +github.com/gorilla/websocket v1.4.0 h1:WDFjx/TMzVgy9VdMMQi2K2Emtwi2QcUQsztZ/zLaH/Q= github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoAMk2YaljkQ= github.com/gostaticanalysis/analysisutil v0.0.0-20190318220348-4088753ea4d3 h1:JVnpOZS+qxli+rgVl98ILOXVNbW+kb5wcxeGx8ShUIw= github.com/gostaticanalysis/analysisutil v0.0.0-20190318220348-4088753ea4d3/go.mod h1:eEOZF4jCKGi+aprrirO9e7WKB3beBRtWgqGunKl6pKE= @@ -367,6 +369,7 @@ github.com/shurcooL/go v0.0.0-20180423040247-9e1955d9fb6e h1:MZM7FHLqUHYI0Y/mQAt github.com/shurcooL/go v0.0.0-20180423040247-9e1955d9fb6e/go.mod h1:TDJrrUr11Vxrven61rcy3hJMUqaf/CLWYhHNPmT14Lk= github.com/shurcooL/go-goon v0.0.0-20170922171312-37c2f522c041 h1:llrF3Fs4018ePo4+G/HV/uQUqEI1HMDjCeOf2V6puPc= github.com/shurcooL/go-goon v0.0.0-20170922171312-37c2f522c041/go.mod h1:N5mDOmsrJOB+vfqUK+7DmDyjhSLIIBnXo9lvZJj3MWQ= +github.com/shurcooL/sanitized_anchor_name v1.0.0 h1:PdmoCO6wvbs+7yrJyMORt4/BmY5IYyJwS/kOiWx8mHo= github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc= github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo= github.com/sirupsen/logrus v1.4.2 h1:SPIRibHv4MatM3XXNO2BJeFLZwZ2LvZgfQ5+UNI2im4= diff --git a/api/internal/conflict/factory.go b/api/internal/conflict/factory.go new file mode 100644 index 00000000000..3cfed193a1b --- /dev/null +++ b/api/internal/conflict/factory.go @@ -0,0 +1,23 @@ +// Copyright 2020 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package conflict + +import ( + "sigs.k8s.io/kustomize/api/resid" + "sigs.k8s.io/kustomize/api/resource" +) + +type cdFactory struct{} + +var _ resource.ConflictDetectorFactory = &cdFactory{} + +// NewFactory returns a new conflict detector factory. +func NewFactory() resource.ConflictDetectorFactory { + return &cdFactory{} +} + +// New returns an instance of smPatchMergeOnlyDetector. +func (c cdFactory) New(_ resid.Gvk) (resource.ConflictDetector, error) { + return &smPatchMergeOnlyDetector{}, nil +} diff --git a/api/internal/conflict/smpatchmergeonlydetector.go b/api/internal/conflict/smpatchmergeonlydetector.go new file mode 100644 index 00000000000..4dda0a4cfc2 --- /dev/null +++ b/api/internal/conflict/smpatchmergeonlydetector.go @@ -0,0 +1,28 @@ +// Copyright 2020 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package conflict + +import ( + "sigs.k8s.io/kustomize/api/resource" +) + +// smPatchMergeOnlyDetector ignores conflicts, +// but does real strategic merge patching. +// This is part of an effort to eliminate dependence on +// apimachinery package to allow kustomize integration +// into kubectl (#2506 and #1500) +type smPatchMergeOnlyDetector struct{} + +var _ resource.ConflictDetector = &smPatchMergeOnlyDetector{} + +func (c *smPatchMergeOnlyDetector) HasConflict( + _, _ *resource.Resource) (bool, error) { + return false, nil +} + +func (c *smPatchMergeOnlyDetector) MergePatches( + r, patch *resource.Resource) (*resource.Resource, error) { + err := r.ApplySmPatch(patch) + return r, err +} diff --git a/api/internal/k8sdeps/conflict/conflictdetectorjson.go b/api/internal/k8sdeps/conflict/conflictdetectorjson.go new file mode 100644 index 00000000000..8f088517720 --- /dev/null +++ b/api/internal/k8sdeps/conflict/conflictdetectorjson.go @@ -0,0 +1,43 @@ +// Copyright 2019 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package conflict + +import ( + "encoding/json" + + jsonpatch "github.com/evanphx/json-patch" + "k8s.io/apimachinery/pkg/util/mergepatch" + "sigs.k8s.io/kustomize/api/resource" +) + +// conflictDetectorJson detects conflicts in a list of JSON patches. +type conflictDetectorJson struct { + resourceFactory *resource.Factory +} + +var _ resource.ConflictDetector = &conflictDetectorJson{} + +func (cd *conflictDetectorJson) HasConflict( + p1, p2 *resource.Resource) (bool, error) { + return mergepatch.HasConflicts(p1.Map(), p2.Map()) +} + +func (cd *conflictDetectorJson) MergePatches( + patch1, patch2 *resource.Resource) (*resource.Resource, error) { + baseBytes, err := json.Marshal(patch1.Map()) + if err != nil { + return nil, err + } + patchBytes, err := json.Marshal(patch2.Map()) + if err != nil { + return nil, err + } + mergedBytes, err := jsonpatch.MergeMergePatches(baseBytes, patchBytes) + if err != nil { + return nil, err + } + mergedMap := make(map[string]interface{}) + err = json.Unmarshal(mergedBytes, &mergedMap) + return cd.resourceFactory.FromMap(mergedMap), err +} diff --git a/api/internal/k8sdeps/conflict/conflictdetectorsm.go b/api/internal/k8sdeps/conflict/conflictdetectorsm.go new file mode 100644 index 00000000000..a50aa3af0ba --- /dev/null +++ b/api/internal/k8sdeps/conflict/conflictdetectorsm.go @@ -0,0 +1,65 @@ +// Copyright 2019 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package conflict + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/util/strategicpatch" + "sigs.k8s.io/kustomize/api/resource" +) + +// conflictDetectorSm detects conflicts in a list of strategic merge patches. +type conflictDetectorSm struct { + lookupPatchMeta strategicpatch.LookupPatchMeta + resourceFactory *resource.Factory +} + +var _ resource.ConflictDetector = &conflictDetectorSm{} + +func (cd *conflictDetectorSm) HasConflict( + p1, p2 *resource.Resource) (bool, error) { + return strategicpatch.MergingMapsHaveConflicts( + p1.Map(), p2.Map(), cd.lookupPatchMeta) +} + +func (cd *conflictDetectorSm) MergePatches( + patch1, patch2 *resource.Resource) (*resource.Resource, error) { + if cd.hasDeleteDirectiveMarker(patch2.Map()) { + if cd.hasDeleteDirectiveMarker(patch1.Map()) { + return nil, fmt.Errorf( + "cannot merge patches both containing '$patch: delete' directives") + } + patch1, patch2 = patch2, patch1 + } + mergedMap, err := strategicpatch.MergeStrategicMergeMapPatchUsingLookupPatchMeta( + cd.lookupPatchMeta, patch1.Map(), patch2.Map()) + return cd.resourceFactory.FromMap(mergedMap), err +} + +func (cd *conflictDetectorSm) hasDeleteDirectiveMarker( + patch map[string]interface{}) bool { + if v, ok := patch["$patch"]; ok && v == "delete" { + return true + } + for _, v := range patch { + switch typedV := v.(type) { + case map[string]interface{}: + if cd.hasDeleteDirectiveMarker(typedV) { + return true + } + case []interface{}: + for _, sv := range typedV { + typedE, ok := sv.(map[string]interface{}) + if !ok { + break + } + if cd.hasDeleteDirectiveMarker(typedE) { + return true + } + } + } + } + return false +} diff --git a/api/internal/k8sdeps/conflict/factory.go b/api/internal/k8sdeps/conflict/factory.go new file mode 100644 index 00000000000..1d4ac3f22c7 --- /dev/null +++ b/api/internal/k8sdeps/conflict/factory.go @@ -0,0 +1,45 @@ +// Copyright 2020 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package conflict + +import ( + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + sp "k8s.io/apimachinery/pkg/util/strategicpatch" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/kustomize/api/resid" + "sigs.k8s.io/kustomize/api/resource" +) + +type cdFactory struct { + rf *resource.Factory +} + +var _ resource.ConflictDetectorFactory = &cdFactory{} + +// NewFactory returns a conflict detector factory. +// The detector uses a resource factory to convert resources to/from +// json/yaml/maps representations. +func NewFactory(rf *resource.Factory) resource.ConflictDetectorFactory { + return &cdFactory{rf: rf} +} + +// New returns a conflict detector that's aware of the GVK type. +func (f *cdFactory) New(gvk resid.Gvk) (resource.ConflictDetector, error) { + // Convert to apimachinery representation of object + obj, err := scheme.Scheme.New(schema.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }) + if err == nil { + meta, err := sp.NewPatchMetaFromStruct(obj) + return &conflictDetectorSm{ + lookupPatchMeta: meta, resourceFactory: f.rf}, err + } + if runtime.IsNotRegisteredError(err) { + return &conflictDetectorJson{resourceFactory: f.rf}, nil + } + return nil, err +} diff --git a/api/internal/k8sdeps/merge/merginator.go b/api/internal/k8sdeps/merge/merginator.go deleted file mode 100644 index 627908f23bc..00000000000 --- a/api/internal/k8sdeps/merge/merginator.go +++ /dev/null @@ -1,239 +0,0 @@ -// Copyright 2019 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package merge - -import ( - "encoding/json" - "fmt" - - jsonpatch "github.com/evanphx/json-patch" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/mergepatch" - "k8s.io/apimachinery/pkg/util/strategicpatch" - "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/kustomize/api/resid" - "sigs.k8s.io/kustomize/api/resmap" - "sigs.k8s.io/kustomize/api/resource" -) - -type conflictDetector interface { - hasConflict(patch1, patch2 *resource.Resource) (bool, error) - findConflict( - conflictingPatchIdx int, - patches []*resource.Resource) (*resource.Resource, error) - mergePatches(patch1, patch2 *resource.Resource) (*resource.Resource, error) -} - -type jsonMergePatch struct { - resourceFactory *resource.Factory -} - -var _ conflictDetector = &jsonMergePatch{} - -func newJMPConflictDetector(rf *resource.Factory) conflictDetector { - return &jsonMergePatch{resourceFactory: rf} -} - -func (jmp *jsonMergePatch) hasConflict( - patch1, patch2 *resource.Resource) (bool, error) { - return mergepatch.HasConflicts(patch1.Map(), patch2.Map()) -} - -func (jmp *jsonMergePatch) findConflict( - conflictingPatchIdx int, - patches []*resource.Resource) (*resource.Resource, error) { - for i, patch := range patches { - if i == conflictingPatchIdx { - continue - } - if !patches[conflictingPatchIdx].OrgId().Equals(patch.OrgId()) { - continue - } - conflict, err := mergepatch.HasConflicts( - patch.Map(), - patches[conflictingPatchIdx].Map()) - if err != nil { - return nil, err - } - if conflict { - return patch, nil - } - } - return nil, nil -} - -func (jmp *jsonMergePatch) mergePatches( - patch1, patch2 *resource.Resource) (*resource.Resource, error) { - baseBytes, err := json.Marshal(patch1.Map()) - if err != nil { - return nil, err - } - patchBytes, err := json.Marshal(patch2.Map()) - if err != nil { - return nil, err - } - mergedBytes, err := jsonpatch.MergeMergePatches(baseBytes, patchBytes) - if err != nil { - return nil, err - } - mergedMap := make(map[string]interface{}) - err = json.Unmarshal(mergedBytes, &mergedMap) - return jmp.resourceFactory.FromMap(mergedMap), err -} - -type strategicMergePatch struct { - lookupPatchMeta strategicpatch.LookupPatchMeta - rf *resource.Factory -} - -var _ conflictDetector = &strategicMergePatch{} - -func newSMPConflictDetector( - versionedObj runtime.Object, - rf *resource.Factory) (conflictDetector, error) { - lookupPatchMeta, err := strategicpatch.NewPatchMetaFromStruct(versionedObj) - return &strategicMergePatch{lookupPatchMeta: lookupPatchMeta, rf: rf}, err -} - -func (smp *strategicMergePatch) hasConflict( - p1, p2 *resource.Resource) (bool, error) { - return strategicpatch.MergingMapsHaveConflicts( - p1.Map(), p2.Map(), smp.lookupPatchMeta) -} - -func (smp *strategicMergePatch) findConflict( - conflictingPatchIdx int, - patches []*resource.Resource) (*resource.Resource, error) { - for i, patch := range patches { - if i == conflictingPatchIdx { - continue - } - if !patches[conflictingPatchIdx].OrgId().Equals(patch.OrgId()) { - continue - } - conflict, err := strategicpatch.MergingMapsHaveConflicts( - patch.Map(), - patches[conflictingPatchIdx].Map(), - smp.lookupPatchMeta) - if err != nil { - return nil, err - } - if conflict { - return patch, nil - } - } - return nil, nil -} - -func (smp *strategicMergePatch) mergePatches( - patch1, patch2 *resource.Resource) (*resource.Resource, error) { - if hasDeleteDirectiveMarker(patch2.Map()) { - if hasDeleteDirectiveMarker(patch1.Map()) { - return nil, fmt.Errorf( - "cannot merge patches both containing '$patch: delete' directives") - } - patch1, patch2 = patch2, patch1 - } - mergeJSONMap, err := strategicpatch.MergeStrategicMergeMapPatchUsingLookupPatchMeta( - smp.lookupPatchMeta, patch1.Map(), patch2.Map()) - return smp.rf.FromMap(mergeJSONMap), err -} - -type merginatorImpl struct { - rf *resource.Factory -} - -// NewMerginator returns a new implementation of resmap.Merginator. -func NewMerginator(rf *resource.Factory) resmap.Merginator { - return &merginatorImpl{rf: rf} -} - -var _ resmap.Merginator = (*merginatorImpl)(nil) - -// Merge merges the incoming resources into a new resmap.ResMap. -// Returns an error on conflict. -func (m *merginatorImpl) Merge( - patches []*resource.Resource) (resmap.ResMap, error) { - rc := resmap.New() - for ix, patch := range patches { - id := patch.OrgId() - existing := rc.GetMatchingResourcesByOriginalId(id.Equals) - if len(existing) == 0 { - rc.Append(patch) - continue - } - if len(existing) > 1 { - return nil, fmt.Errorf("self conflict in patches") - } - - versionedObj, err := scheme.Scheme.New(toSchemaGvk(id.Gvk)) - if err != nil && !runtime.IsNotRegisteredError(err) { - return nil, err - } - var cd conflictDetector - if err != nil { - cd = newJMPConflictDetector(m.rf) - } else { - cd, err = newSMPConflictDetector(versionedObj, m.rf) - if err != nil { - return nil, err - } - } - - conflict, err := cd.hasConflict(existing[0], patch) - if err != nil { - return nil, err - } - if conflict { - conflictingPatch, err := cd.findConflict(ix, patches) - if err != nil { - return nil, err - } - return nil, fmt.Errorf( - "conflict between %#v and %#v", - conflictingPatch.Map(), patch.Map()) - } - merged, err := cd.mergePatches(existing[0], patch) - if err != nil { - return nil, err - } - rc.Replace(merged) - } - return rc, nil -} - -// toSchemaGvk converts to a schema.GroupVersionKind. -func toSchemaGvk(x resid.Gvk) schema.GroupVersionKind { - return schema.GroupVersionKind{ - Group: x.Group, - Version: x.Version, - Kind: x.Kind, - } -} - -func hasDeleteDirectiveMarker(patch map[string]interface{}) bool { - if v, ok := patch["$patch"]; ok && v == "delete" { - return true - } - for _, v := range patch { - switch typedV := v.(type) { - case map[string]interface{}: - if hasDeleteDirectiveMarker(typedV) { - return true - } - case []interface{}: - for _, sv := range typedV { - typedE, ok := sv.(map[string]interface{}) - if !ok { - break - } - if hasDeleteDirectiveMarker(typedE) { - return true - } - } - } - } - return false -} diff --git a/api/internal/merge/merginator.go b/api/internal/merge/merginator.go deleted file mode 100644 index af257e5b45d..00000000000 --- a/api/internal/merge/merginator.go +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright 2020 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package merge - -import ( - "sigs.k8s.io/kustomize/api/resmap" - "sigs.k8s.io/kustomize/api/resource" -) - -// Merginator implements resmap.Merginator using kyaml libs. -type Merginator struct { -} - -var _ resmap.Merginator = (*Merginator)(nil) - -func NewMerginator(_ *resource.Factory) *Merginator { - return &Merginator{} -} - -// Merge implements resmap.Merginator -// TODO: Detect conflicts, and return an error. -// https://github.com/kubernetes-sigs/kustomize/issues/3303 -func (m Merginator) Merge( - resources []*resource.Resource) (resmap.ResMap, error) { - rm := resmap.New() - for i := range resources { - rm.Append(resources[i]) - } - return rm, nil -} diff --git a/api/internal/merge/merginator_test.go b/api/internal/merge/merginator_test.go deleted file mode 100644 index 3a15de64b84..00000000000 --- a/api/internal/merge/merginator_test.go +++ /dev/null @@ -1,4 +0,0 @@ -// Copyright 2020 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package merge_test diff --git a/api/internal/plugins/execplugin/execplugin_test.go b/api/internal/plugins/execplugin/execplugin_test.go index 94e3787ce67..dd71c6edc66 100644 --- a/api/internal/plugins/execplugin/execplugin_test.go +++ b/api/internal/plugins/execplugin/execplugin_test.go @@ -32,7 +32,8 @@ s/$BAR/bar baz/g t.Fatal(err) } pvd := provider.NewDefaultDepProvider() - rf := resmap.NewFactory(pvd.GetResourceFactory(), pvd.GetMerginator()) + rf := resmap.NewFactory( + pvd.GetResourceFactory(), pvd.GetConflictDetectorFactory()) pluginConfig := rf.RF().FromMap( map[string]interface{}{ "apiVersion": "someteam.example.com/v1", diff --git a/api/internal/plugins/loader/loader_test.go b/api/internal/plugins/loader/loader_test.go index 20cca8be534..da6c16debec 100644 --- a/api/internal/plugins/loader/loader_test.go +++ b/api/internal/plugins/loader/loader_test.go @@ -51,7 +51,8 @@ func TestLoader(t *testing.T) { BuildGoPlugin("someteam.example.com", "v1", "SomeServiceGenerator") defer th.Reset() p := provider.NewDefaultDepProvider() - rmF := resmap.NewFactory(p.GetResourceFactory(), p.GetMerginator()) + rmF := resmap.NewFactory( + p.GetResourceFactory(), p.GetConflictDetectorFactory()) fLdr, err := loader.NewLoader( loader.RestrictionRootOnly, filesys.Separator, filesys.MakeFsInMemory()) diff --git a/api/internal/target/maker_test.go b/api/internal/target/maker_test.go index b60cde5ffcd..1a4f3c80a2f 100644 --- a/api/internal/target/maker_test.go +++ b/api/internal/target/maker_test.go @@ -36,7 +36,8 @@ func makeKustTargetWithRf( if err != nil { t.Fatal(err) } - rf := resmap.NewFactory(pvd.GetResourceFactory(), pvd.GetMerginator()) + rf := resmap.NewFactory( + pvd.GetResourceFactory(), pvd.GetConflictDetectorFactory()) pc := konfig.DisabledPluginConfig() return target.NewKustTarget( ldr, diff --git a/api/krusty/kustomizer.go b/api/krusty/kustomizer.go index f2d20bb84a0..b3b7ec32287 100644 --- a/api/krusty/kustomizer.go +++ b/api/krusty/kustomizer.go @@ -53,7 +53,7 @@ func MakeKustomizer(fSys filesys.FileSystem, o *Options) *Kustomizer { func (b *Kustomizer) Run(path string) (resmap.ResMap, error) { resmapFactory := resmap.NewFactory( b.depProvider.GetResourceFactory(), - b.depProvider.GetMerginator()) + b.depProvider.GetConflictDetectorFactory()) lr := fLdr.RestrictionNone if b.options.LoadRestrictions == types.LoadRestrictionsRootOnly { lr = fLdr.RestrictionRootOnly diff --git a/api/provider/depprovider.go b/api/provider/depprovider.go index 6a144bc1282..6f831c14192 100644 --- a/api/provider/depprovider.go +++ b/api/provider/depprovider.go @@ -5,14 +5,13 @@ package provider import ( "sigs.k8s.io/kustomize/api/ifc" - "sigs.k8s.io/kustomize/api/internal/k8sdeps/merge" - kmerge "sigs.k8s.io/kustomize/api/internal/merge" + "sigs.k8s.io/kustomize/api/internal/conflict" + k8sconflict "sigs.k8s.io/kustomize/api/internal/k8sdeps/conflict" "sigs.k8s.io/kustomize/api/internal/validate" "sigs.k8s.io/kustomize/api/internal/wrappy" "sigs.k8s.io/kustomize/api/k8sdeps/kunstruct" "sigs.k8s.io/kustomize/api/k8sdeps/validator" "sigs.k8s.io/kustomize/api/konfig" - "sigs.k8s.io/kustomize/api/resmap" "sigs.k8s.io/kustomize/api/resource" ) @@ -96,17 +95,27 @@ import ( // would really reduce the work // (e.g. drop Vars, drop ReplacementTranformer). // -// - resmap.Merginator +// - resource.ConflictDetector // -// 1) api/internal/k8sdeps/merge.Merginator +// 1) api/internal/k8sdeps/conflict.conflictDetectorJson +// api/internal/k8sdeps/conflict.conflictDetectorSm // // Uses k8s.io/apimachinery/pkg/util/strategicpatch, // apimachinery/pkg/util/mergepatch, etc. to merge // resource.Resource instances. // -// 2) api/internal/merge.Merginator +// 2) api/internal/conflict.smPatchMergeOnlyDetector // -// At time of writing, this is unimplemented. +// At time of writing, this doesn't report conflicts, +// but it does know how to merge patches. Conflict +// reporting isn't vital to kustomize function. It's +// rare that a person would configure one transformer +// with many patches, much less so many that it became +// hard to spot conflicts. In the case of an undetected +// conflict, the last patch applied wins, likely what +// the user wants anyway. Regardless, the effect of this +// is plainly visible and usable in the output, even if +// a conflict happened but wasn't reported as an error. // // - ifc.Validator // @@ -117,6 +126,7 @@ import ( // // 2) api/internal/validate.FieldValidator // +// See TODO inside the validator for status. // At time of writing, this is a do-nothing // validator as it's not critical to kustomize function. // @@ -139,20 +149,20 @@ import ( // If you're reading this, plan not done. // type DepProvider struct { - kFactory ifc.KunstructuredFactory - resourceFactory *resource.Factory - merginator resmap.Merginator - fieldValidator ifc.Validator + kFactory ifc.KunstructuredFactory + resourceFactory *resource.Factory + conflictDectectorFactory resource.ConflictDetectorFactory + fieldValidator ifc.Validator } func makeK8sdepBasedInstances() *DepProvider { kf := kunstruct.NewKunstructuredFactoryImpl() rf := resource.NewFactory(kf) return &DepProvider{ - kFactory: kf, - resourceFactory: rf, - merginator: merge.NewMerginator(rf), - fieldValidator: validator.NewKustValidator(), + kFactory: kf, + resourceFactory: rf, + conflictDectectorFactory: k8sconflict.NewFactory(rf), + fieldValidator: validator.NewKustValidator(), } } @@ -160,10 +170,10 @@ func makeKyamlBasedInstances() *DepProvider { kf := &wrappy.WNodeFactory{} rf := resource.NewFactory(kf) return &DepProvider{ - kFactory: kf, - resourceFactory: rf, - merginator: kmerge.NewMerginator(rf), - fieldValidator: validate.NewFieldValidator(), + kFactory: kf, + resourceFactory: rf, + conflictDectectorFactory: conflict.NewFactory(), + fieldValidator: validate.NewFieldValidator(), } } @@ -186,8 +196,8 @@ func (dp *DepProvider) GetResourceFactory() *resource.Factory { return dp.resourceFactory } -func (dp *DepProvider) GetMerginator() resmap.Merginator { - return dp.merginator +func (dp *DepProvider) GetConflictDetectorFactory() resource.ConflictDetectorFactory { + return dp.conflictDectectorFactory } func (dp *DepProvider) GetFieldValidator() ifc.Validator { diff --git a/api/resmap/factory.go b/api/resmap/factory.go index cdbeb2086e9..e4e4e43f2ce 100644 --- a/api/resmap/factory.go +++ b/api/resmap/factory.go @@ -12,24 +12,18 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) -// Merginator merges resources. -type Merginator interface { - // Merge creates a new ResMap by merging incoming resources. - // Error if conflict found. - Merge([]*resource.Resource) (ResMap, error) -} - // Factory makes instances of ResMap. type Factory struct { // Makes resources. resF *resource.Factory - // Makes ResMaps via merging. - pm Merginator + // Makes ConflictDetectors. + cdf resource.ConflictDetectorFactory } // NewFactory returns a new resmap.Factory. -func NewFactory(rf *resource.Factory, pm Merginator) *Factory { - return &Factory{resF: rf, pm: pm} +func NewFactory( + rf *resource.Factory, cdf resource.ConflictDetectorFactory) *Factory { + return &Factory{resF: rf, cdf: cdf} } // RF returns a resource.Factory. @@ -134,8 +128,8 @@ func (rmF *Factory) FromSecretArgs( // Merge creates a new ResMap by merging incoming resources. // Error if conflict found. -func (rmF *Factory) Merge(patches []*resource.Resource) (ResMap, error) { - return rmF.pm.Merge(patches) +func (rmF *Factory) Merge(incoming []*resource.Resource) (ResMap, error) { + return (&merginator{cdf: rmF.cdf}).Merge(incoming) } func newResMapFromResourceSlice( diff --git a/api/resmap/merginator.go b/api/resmap/merginator.go new file mode 100644 index 00000000000..9af6d11c334 --- /dev/null +++ b/api/resmap/merginator.go @@ -0,0 +1,118 @@ +// Copyright 2020 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package resmap + +import ( + "fmt" + + "sigs.k8s.io/kustomize/api/resource" +) + +// merginator coordinates merging the resources in incoming to the result. +type merginator struct { + incoming []*resource.Resource + cdf resource.ConflictDetectorFactory + result ResMap +} + +func (m *merginator) Merge(in []*resource.Resource) (ResMap, error) { + m.result = New() + m.incoming = in + for index := range m.incoming { + alreadyInResult, err := m.appendIfNoMatch(index) + if err != nil { + return nil, err + } + if alreadyInResult != nil { + // The resource at index has the same resId as a previously + // considered resource. + // + // If they conflict with each other (e.g. they both want to change + // the image name in a Deployment, but to different values), + // return an error. + // + // If they don't conflict, then merge them into a single resource, + // since they both target the same item, and we want cumulative + // behavior. E.g. say both patches modify a map. Without a merge, + // the last patch wins, replacing the entire map. + err = m.mergeWithExisting(index, alreadyInResult) + if err != nil { + return nil, err + } + } + } + return m.result, nil +} + +func (m *merginator) appendIfNoMatch(index int) (*resource.Resource, error) { + candidate := m.incoming[index] + matchedResources := m.result.GetMatchingResourcesByOriginalId( + candidate.OrgId().Equals) + if len(matchedResources) == 0 { + m.result.Append(candidate) + return nil, nil + } + if len(matchedResources) > 1 { + return nil, fmt.Errorf("multiple resources targeted by patch") + } + return matchedResources[0], nil +} + +func (m *merginator) mergeWithExisting( + index int, alreadyInResult *resource.Resource) error { + candidate := m.incoming[index] + cd, err := m.cdf.New(candidate.OrgId().Gvk) + if err != nil { + return err + } + hasConflict, err := cd.HasConflict(candidate, alreadyInResult) + if err != nil { + return err + } + if hasConflict { + return m.makeError(cd, index) + } + merged, err := cd.MergePatches(alreadyInResult, candidate) + if err != nil { + return err + } + _, err = m.result.Replace(merged) + return err +} + +// Make an error message describing the conflict. +func (m *merginator) makeError(cd resource.ConflictDetector, index int) error { + conflict, err := m.findConflict(cd, index) + if err != nil { + return err + } + if conflict == nil { + return fmt.Errorf("expected conflict for %s", m.incoming[index].OrgId()) + } + return fmt.Errorf( + "conflict between %#v at index %d and %#v", + m.incoming[index].Map(), index, conflict.Map()) +} + +// findConflict looks for a conflict in a resource slice. +// It returns the first conflict between the resource at index +// and some other resource. Two resources can only conflict if +// they have the same original ResId. +func (m *merginator) findConflict( + cd resource.ConflictDetector, index int) (*resource.Resource, error) { + targetId := m.incoming[index].OrgId() + for i, p := range m.incoming { + if i == index || !targetId.Equals(p.OrgId()) { + continue + } + conflict, err := cd.HasConflict(p, m.incoming[index]) + if err != nil { + return nil, err + } + if conflict { + return p, nil + } + } + return nil, nil +} diff --git a/api/resource/conflictdetector.go b/api/resource/conflictdetector.go new file mode 100644 index 00000000000..f079cdd5abb --- /dev/null +++ b/api/resource/conflictdetector.go @@ -0,0 +1,20 @@ +// Copyright 2019 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package resource + +import "sigs.k8s.io/kustomize/api/resid" + +// ConflictDetector detects conflicts between resources. +type ConflictDetector interface { + // HasConflict returns true if the given resources have a conflict. + HasConflict(patch1, patch2 *Resource) (bool, error) + // Merge two resources into one. + MergePatches(patch1, patch2 *Resource) (*Resource, error) +} + +// ConflictDetectorFactory makes instances of ConflictDetector that know +// how to handle the given Group, Version, Kind tuple. +type ConflictDetectorFactory interface { + New(gvk resid.Gvk) (ConflictDetector, error) +} diff --git a/api/testutils/kusttest/harnessenhanced.go b/api/testutils/kusttest/harnessenhanced.go index fb8cc1bb60f..baa22edb6e0 100644 --- a/api/testutils/kusttest/harnessenhanced.go +++ b/api/testutils/kusttest/harnessenhanced.go @@ -46,7 +46,8 @@ func MakeEnhancedHarness(t *testing.T) *HarnessEnhanced { } p := provider.NewDefaultDepProvider() resourceFactory := p.GetResourceFactory() - resmapFactory := resmap.NewFactory(resourceFactory, p.GetMerginator()) + resmapFactory := resmap.NewFactory( + resourceFactory, p.GetConflictDetectorFactory()) result := &HarnessEnhanced{ Harness: MakeHarness(t), diff --git a/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go b/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go index 822c4fe9c7c..3f4cb7f7322 100644 --- a/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go +++ b/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go @@ -16,7 +16,8 @@ import ( func main() { var plugin resmap.Configurable p := provider.NewDefaultDepProvider() - resmapFactory := resmap.NewFactory(p.GetResourceFactory(), p.GetMerginator()) + resmapFactory := resmap.NewFactory( + p.GetResourceFactory(), p.GetConflictDetectorFactory()) pluginHelpers := resmap.NewPluginHelpers( nil, p.GetFieldValidator(), resmapFactory) diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go index 5d7b4f73857..afb5dc0fb36 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go +++ b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go @@ -6,6 +6,7 @@ package main import ( "fmt" + "sigs.k8s.io/kustomize/api/resmap" "sigs.k8s.io/kustomize/api/resource" "sigs.k8s.io/kustomize/api/types" @@ -34,6 +35,11 @@ func (p *plugin) Config( } if len(p.Paths) != 0 { for _, onePath := range p.Paths { + // The following oddly attempts to interpret a path string as an + // actual patch (instead of as a path to a file containing a patch). + // All tests pass if this code is commented out. This code should + // be deleted; the user should use the Patches field which + // exists for this purpose (inline patch declaration). res, err := p.h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) if err == nil { p.loadedPatches = append(p.loadedPatches, res...) diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go index 189d8e05742..fab9670f9c6 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go +++ b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go @@ -13,7 +13,6 @@ import ( kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" ) - // TODO(#3304): eliminate branching on konfig.FlagEnableKyamlDefaultValue // Details: https://github.com/kubernetes-sigs/kustomize/issues/3304 // All tests should pass for either true or false values @@ -240,8 +239,7 @@ patches: |- - name: nginx image: nginx:latest `, - target, - ifApiMachineryElseKyaml(` + target, ` apiVersion: apps/v1 kind: Deployment metadata: @@ -256,22 +254,7 @@ spec: containers: - image: nginx:latest name: nginx -`, ` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: myDeploy -spec: - replica: 3 - template: - metadata: - labels: - old-label: old-value - spec: - containers: - - image: nginx - name: nginx -`)) +`) } func TestPatchStrategicMergeTransformerMultiplePatches(t *testing.T) { @@ -321,8 +304,7 @@ paths: - patch1.yaml - patch2.yaml `, - target, - ifApiMachineryElseKyaml(` + target, ` apiVersion: apps/v1 kind: Deployment metadata: @@ -344,25 +326,7 @@ spec: name: nginx - image: busybox name: busybox -`, ` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: myDeploy -spec: - replica: 2 - template: - metadata: - labels: - old-label: old-value - spec: - containers: - - env: - - name: SOMEENV - value: BAR - image: nginx:latest - name: nginx -`)) +`) } func TestStrategicMergeTransformerMultiplePatchesWithConflicts(t *testing.T) { @@ -665,7 +629,11 @@ metadata: spec: bar: A: X + B: "Y" C: Z + D: W + baz: + hello: world `)) } @@ -1041,26 +1009,7 @@ func TestMultiplePatches(t *testing.T) { changeImagePatch(Deployment, "nginx:latest"), addContainerAndEnvPatch(Deployment), }, - expected: ifApiMachineryElseKyaml( - expectedResultMultiPatch(Deployment, false), ` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: deploy1 -spec: - template: - metadata: - labels: - old-label: old-value - some-label: some-value - spec: - containers: - - env: - - name: SOMEENV - value: SOMEVALUE - image: nginx - name: nginx -`), + expected: expectedResultMultiPatch(Deployment, false), }, "withschema-image-container-label": { base: baseResource(Deployment), @@ -1069,22 +1018,7 @@ spec: addContainerAndEnvPatch(Deployment), addLabelAndEnvPatch(Deployment), }, - expected: ifApiMachineryElseKyaml( - expectedResultMultiPatch(Deployment, true), ` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: deploy1 -spec: - template: - metadata: - labels: - old-label: old-value - spec: - containers: - - image: nginx:latest - name: nginx -`), + expected: expectedResultMultiPatch(Deployment, true), }, "withschema-container-label-image": { base: baseResource(Deployment), @@ -1093,27 +1027,7 @@ spec: addLabelAndEnvPatch(Deployment), changeImagePatch(Deployment, "nginx:latest"), }, - expected: ifApiMachineryElseKyaml( - expectedResultMultiPatch(Deployment, true), ` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: deploy1 -spec: - template: - metadata: - labels: - old-label: old-value - spec: - containers: - - env: - - name: ANOTHERENV - value: ANOTHERVALUE - image: nginx - name: nginx - - image: anotherimage - name: anothercontainer -`), + expected: expectedResultMultiPatch(Deployment, true), }, "noschema-label-image-container": { base: baseResource(MyCRD), @@ -1245,22 +1159,7 @@ func TestMultiplePatchesWithConflict(t *testing.T) { // There is no conflict detected. It should // be but the JMPConflictDector ignores it. // See https://github.com/kubernetes-sigs/kustomize/issues/1370 - expected: ifApiMachineryElseKyaml( - expectedResultJMP("nginx:1.7.9"), ` -apiVersion: apps/v1 -kind: MyCRD -metadata: - name: deploy1 -spec: - template: - metadata: - labels: - old-label: old-value - spec: - containers: - - image: nginx:latest - name: nginx -`), + expected: expectedResultJMP("nginx:1.7.9"), }, "noschema-1.7.9-label-latest": { base: baseResource(MyCRD), @@ -1273,23 +1172,7 @@ spec: // There is no conflict detected. It should // be but the JMPConflictDector ignores it. // See https://github.com/kubernetes-sigs/kustomize/issues/1370 - - expected: ifApiMachineryElseKyaml( - expectedResultJMP("nginx:latest"), ` -apiVersion: apps/v1 -kind: MyCRD -metadata: - name: deploy1 -spec: - template: - metadata: - labels: - old-label: old-value - spec: - containers: - - image: nginx:1.7.9 - name: nginx -`), + expected: expectedResultJMP("nginx:latest"), }, "noschema-1.7.9-latest-label": { base: baseResource(MyCRD),