diff --git a/api/builtins/PatchStrategicMergeTransformer.go b/api/builtins/PatchStrategicMergeTransformer.go index 7b9bbb171..e2f61238f 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 9f0a9748f..c361fd12e 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 000000000..3cfed193a --- /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 000000000..4dda0a4cf --- /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 000000000..8f0885177 --- /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 000000000..a50aa3af0 --- /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 000000000..1d4ac3f22 --- /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 627908f23..000000000 --- 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 af257e5b4..000000000 --- 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 3a15de64b..000000000 --- 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 94e3787ce..dd71c6edc 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 20cca8be5..da6c16deb 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 b60cde5ff..1a4f3c80a 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 f2d20bb84..b3b7ec322 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 6a144bc12..6f831c141 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 cdbeb2086..e4e4e43f2 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 000000000..9af6d11c3 --- /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 000000000..f079cdd5a --- /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 fb8cc1bb6..baa22edb6 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 822c4fe9c..3f4cb7f73 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 5d7b4f738..afb5dc0fb 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 189d8e057..380c9c27d 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 @@ -28,6 +27,9 @@ func ifApiMachineryElseKyaml(s1, s2 string) string { return s2 } +// TODO(#3304) +const skipConflictDetectionTests = konfig.FlagEnableKyamlDefaultValue + func errorContains(err error, possibilities ...string) bool { for _, x := range possibilities { if strings.Contains(err.Error(), x) { @@ -240,8 +242,7 @@ patches: |- - name: nginx image: nginx:latest `, - target, - ifApiMachineryElseKyaml(` + target, ` apiVersion: apps/v1 kind: Deployment metadata: @@ -256,22 +257,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 +307,7 @@ paths: - patch1.yaml - patch2.yaml `, - target, - ifApiMachineryElseKyaml(` + target, ` apiVersion: apps/v1 kind: Deployment metadata: @@ -344,28 +329,13 @@ 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) { + if skipConflictDetectionTests { + t.Skip("Skipping patch merge conflict tests.") + } th := kusttest_test.MakeEnhancedHarness(t). PrepBuiltin("PatchStrategicMergeTransformer") defer th.Reset() @@ -412,13 +382,8 @@ paths: - patch1.yaml - patch2.yaml `, target) - // TODO(#3304) - if konfig.FlagEnableKyamlDefaultValue { - assert.NoError(t, err) - } else { - if assert.Error(t, err) && !errorContains(err, "conflict") { - t.Fatalf("expected error to contain %q but get %v", "conflict", err) - } + if assert.Error(t, err) && !errorContains(err, "conflict") { + t.Fatalf("expected error to contain %q but get %v", "conflict", err) } } @@ -665,11 +630,18 @@ metadata: spec: bar: A: X + B: "Y" C: Z + D: W + baz: + hello: world `)) } func TestStrategicMergeTransformerNoSchemaMultiPatchesWithConflict(t *testing.T) { + if skipConflictDetectionTests { + t.Skip("Skipping patch merge conflict tests.") + } th := kusttest_test.MakeEnhancedHarness(t). PrepBuiltin("PatchStrategicMergeTransformer") defer th.Reset() @@ -702,13 +674,8 @@ paths: - patch1.yaml - patch2.yaml `, targetNoschema) - // TODO(#3304) - if konfig.FlagEnableKyamlDefaultValue { - assert.NoError(t, err) - } else { - if assert.Error(t, err) && !errorContains(err, "conflict") { - t.Fatalf("expected error to contain %q but get %v", "conflict", err) - } + if assert.Error(t, err) && !errorContains(err, "conflict") { + t.Fatalf("expected error to contain %q but get %v", "conflict", err) } } @@ -1041,26 +1008,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 +1017,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,68 +1026,34 @@ 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": { + "noschema-1.7.9-label-latest": { base: baseResource(MyCRD), patch: []string{ - addLabelAndEnvPatch(MyCRD), - changeImagePatch(MyCRD, "nginx:latest"), - addContainerAndEnvPatch(MyCRD), - }, - // This should work - errorExpected: true, - errorMsg: "conflict", - }, - "noschema-image-container-label": { - base: baseResource(MyCRD), - patch: []string{ - changeImagePatch(MyCRD, "nginx:latest"), - addContainerAndEnvPatch(MyCRD), - addLabelAndEnvPatch(MyCRD), - }, - // This should work - errorExpected: true, - errorMsg: "conflict", - }, - "noschema-container-label-image": { - base: baseResource(MyCRD), - patch: []string{ - addContainerAndEnvPatch(MyCRD), + changeImagePatch(MyCRD, "nginx:1.7.9"), addLabelAndEnvPatch(MyCRD), changeImagePatch(MyCRD, "nginx:latest"), }, - // This should work - errorExpected: true, - errorMsg: "conflict", + errorExpected: false, + // There is no conflict detected. It should + // be but the JMPConflictDector ignores it. + // See https://github.com/kubernetes-sigs/kustomize/issues/1370 + expected: expectedResultJMP("nginx:latest"), + }, + "noschema-latest-label-1.7.9": { + base: baseResource(MyCRD), + patch: []string{ + changeImagePatch(MyCRD, "nginx:latest"), + addLabelAndEnvPatch(MyCRD), + changeImagePatch(MyCRD, "nginx:1.7.9"), + }, + errorExpected: false, + // There is no conflict detected. It should + // be but the JMPConflictDector ignores it. + // See https://github.com/kubernetes-sigs/kustomize/issues/1370 + expected: expectedResultJMP("nginx:1.7.9"), }, - } - - // TODO(#3304) - if konfig.FlagEnableKyamlDefaultValue { - delete(tests, "noschema-label-image-container") - delete(tests, "noschema-image-container-label") - delete(tests, "noschema-container-label-image") } th := kusttest_test.MakeEnhancedHarness(t). @@ -1182,6 +1081,9 @@ spec: // detected regardless of the order of the patches and regardless // of the schema availibility (SMP vs JSON) func TestMultiplePatchesWithConflict(t *testing.T) { + if skipConflictDetectionTests { + t.Skip("Skipping patch merge conflict tests.") + } tests := map[string]testRecord{ "withschema-label-latest-1.7.9": { base: baseResource(Deployment), @@ -1234,63 +1136,6 @@ func TestMultiplePatchesWithConflict(t *testing.T) { errorExpected: true, errorMsg: "conflict", }, - "noschema-latest-label-1.7.9": { - base: baseResource(MyCRD), - patch: []string{ - changeImagePatch(MyCRD, "nginx:latest"), - addLabelAndEnvPatch(MyCRD), - changeImagePatch(MyCRD, "nginx:1.7.9"), - }, - errorExpected: false, - // 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 -`), - }, - "noschema-1.7.9-label-latest": { - base: baseResource(MyCRD), - patch: []string{ - changeImagePatch(MyCRD, "nginx:1.7.9"), - addLabelAndEnvPatch(MyCRD), - changeImagePatch(MyCRD, "nginx:latest"), - }, - errorExpected: false, - // 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 -`), - }, "noschema-1.7.9-latest-label": { base: baseResource(MyCRD), patch: []string{ @@ -1301,15 +1146,36 @@ spec: }, errorExpected: true, }, - } - // TODO(#3304) - if konfig.FlagEnableKyamlDefaultValue { - delete(tests, "withschema-label-latest-1.7.9") - delete(tests, "withschema-1.7.9-latest-label") - delete(tests, "withschema-1.7.9-label-latest") - delete(tests, "withschema-latest-label-1.7.9-difforder") - delete(tests, "noschema-1.7.9-latest-label") - delete(tests, "noschema-label-latest-1.7.9") + "noschema-label-image-container": { + base: baseResource(MyCRD), + patch: []string{ + addLabelAndEnvPatch(MyCRD), + changeImagePatch(MyCRD, "nginx:latest"), + addContainerAndEnvPatch(MyCRD), + }, + errorExpected: true, + errorMsg: "conflict", + }, + "noschema-image-container-label": { + base: baseResource(MyCRD), + patch: []string{ + changeImagePatch(MyCRD, "nginx:latest"), + addContainerAndEnvPatch(MyCRD), + addLabelAndEnvPatch(MyCRD), + }, + errorExpected: true, + errorMsg: "conflict", + }, + "noschema-container-label-image": { + base: baseResource(MyCRD), + patch: []string{ + addContainerAndEnvPatch(MyCRD), + addLabelAndEnvPatch(MyCRD), + changeImagePatch(MyCRD, "nginx:latest"), + }, + errorExpected: true, + errorMsg: "conflict", + }, } th := kusttest_test.MakeEnhancedHarness(t). PrepBuiltin("PatchStrategicMergeTransformer")