From f66e5bb923509e4d8445472f6aa88ce326faf576 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 | 288 +++++------------- 22 files changed, 482 insertions(+), 525 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 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")