diff --git a/pkg/lib/update/common.go b/pkg/lib/update/common.go index 0de48039e2..83087005fe 100644 --- a/pkg/lib/update/common.go +++ b/pkg/lib/update/common.go @@ -37,6 +37,10 @@ func PkgHasUpdatedUpstream(local, origin string) (bool, error) { return false, errors.E(op, types.UniquePath(local), err) } + if originKf.Upstream == nil { + return false, nil + } + // If the upstream information in local has changed from origin, it // means the user had updated the package independently and we don't // want to override it. diff --git a/pkg/lib/update/merge3/merge3_test.go b/pkg/lib/update/merge3/merge3_test.go index e71cde2259..e0fef7e41e 100644 --- a/pkg/lib/update/merge3/merge3_test.go +++ b/pkg/lib/update/merge3/merge3_test.go @@ -158,3 +158,17 @@ func (t *Merge3TestSuite) TestInferAssocList() { t.commonTest(name, tc) } } + +func (t *Merge3TestSuite) TestNonAssocList() { + testCases := map[string]testCase{ + "non-assoc-crd": { + dir: "non-assoc-crd", + crds: []string{}, + checkFn: nonAssocListMergeCheck, + }, + } + + for name, tc := range testCases { + t.commonTest(name, tc) + } +} diff --git a/pkg/lib/update/merge3/merge3_util_test.go b/pkg/lib/update/merge3/merge3_util_test.go index c8901d2487..765767adf8 100644 --- a/pkg/lib/update/merge3/merge3_util_test.go +++ b/pkg/lib/update/merge3/merge3_util_test.go @@ -164,6 +164,14 @@ func assocListMergeCheck(t *Merge3TestSuite, kos fn.KubeObjects) { })(t, kos) } +func nonAssocListMergeCheck(t *Merge3TestSuite, kos fn.KubeObjects) { + makeFruitCheckFunc(10, map[string]int{ + "apple": 20, + "grape": 5, + "pear": 30, + })(t, kos) +} + //nolint:unparam func makeFruitCheckFunc(expectedTemp int, expectedFruits map[string]int) func(*Merge3TestSuite, fn.KubeObjects) { return func(t *Merge3TestSuite, kos fn.KubeObjects) { diff --git a/pkg/lib/update/merge3/testdata/non-assoc-crd/destination.yaml b/pkg/lib/update/merge3/testdata/non-assoc-crd/destination.yaml new file mode 100644 index 0000000000..c7ed900e51 --- /dev/null +++ b/pkg/lib/update/merge3/testdata/non-assoc-crd/destination.yaml @@ -0,0 +1,32 @@ +apiVersion: infer.kpt.dev/v1alpha1 +kind: MergeTestResources +metadata: + name: "test-resources" +spec: + packageName: "test-package" + workspaceName: "v2" + revision: 2 + repositoryName: "test-repo" + resources: + Kptfile: | + apiVersion: kpt.dev/v1 + kind: Kptfile + metadata: + name: test-package + info: + description: test package + fruitstore.yaml: | + apiVersion: test.group/v1 + kind: FruitStore + metadata: + name: test-fruit-store + spec: + airConditioned: false + preferredTemperature: 20 + fruits: + - name: grape + amount: 5 + - name: apple + amount: 25 + - name: banana + amount: 3 diff --git a/pkg/lib/update/merge3/testdata/non-assoc-crd/original.yaml b/pkg/lib/update/merge3/testdata/non-assoc-crd/original.yaml new file mode 100644 index 0000000000..7dc5440c2f --- /dev/null +++ b/pkg/lib/update/merge3/testdata/non-assoc-crd/original.yaml @@ -0,0 +1,30 @@ +apiVersion: infer.kpt.dev/v1alpha1 +kind: MergeTestResources +metadata: + name: "test-resources" +spec: + packageName: "test-package" + workspaceName: "v1" + revision: 1 + repositoryName: "test-repo" + resources: + Kptfile: | + apiVersion: kpt.dev/v1 + kind: Kptfile + metadata: + name: test-package + info: + description: test package + fruitstore.yaml: | + apiVersion: test.group/v1 + kind: FruitStore + metadata: + name: test-fruit-store + spec: + airConditioned: false + preferredTemperature: 15 + fruits: + - name: apple + amount: 10 + - name: grape + amount: 5 diff --git a/pkg/lib/update/merge3/testdata/non-assoc-crd/updated.yaml b/pkg/lib/update/merge3/testdata/non-assoc-crd/updated.yaml new file mode 100644 index 0000000000..c3592af0c0 --- /dev/null +++ b/pkg/lib/update/merge3/testdata/non-assoc-crd/updated.yaml @@ -0,0 +1,32 @@ +apiVersion: infer.kpt.dev/v1alpha1 +kind: MergeTestResources +metadata: + name: "test-resources" +spec: + packageName: "test-package" + workspaceName: "v3" + revision: 3 + repositoryName: "test-repo" + resources: + Kptfile: | + apiVersion: kpt.dev/v1 + kind: Kptfile + metadata: + name: test-package + info: + description: test package + fruitstore.yaml: | + apiVersion: test.group/v1 + kind: FruitStore + metadata: + name: test-fruit-store + spec: + airConditioned: false + preferredTemperature: 10 + fruits: + - name: pear + amount: 30 + - name: apple + amount: 20 + - name: grape + amount: 5 diff --git a/pkg/lib/update/merge3/tuple.go b/pkg/lib/update/merge3/tuple.go index de8e39565c..5b7af366fb 100644 --- a/pkg/lib/update/merge3/tuple.go +++ b/pkg/lib/update/merge3/tuple.go @@ -20,7 +20,6 @@ import ( "sigs.k8s.io/kustomize/kyaml/kio/filters" "sigs.k8s.io/kustomize/kyaml/kio/kioutil" "sigs.k8s.io/kustomize/kyaml/yaml" - "sigs.k8s.io/kustomize/kyaml/yaml/merge3" "sigs.k8s.io/kustomize/kyaml/yaml/walk" ) @@ -34,8 +33,10 @@ type tuple struct { // merge performs a 3-way merge on the tuple func (t *tuple) merge() (*yaml.RNode, error) { return walk.Walker{ + // modified Visitor + Visitor: &Visitor{}, + // same as in merge3.Merge() - Visitor: merge3.Visitor{}, VisitKeysAsScalars: true, Sources: []*yaml.RNode{t.dest, t.original, t.updated}, diff --git a/pkg/lib/update/merge3/visitor.go b/pkg/lib/update/merge3/visitor.go new file mode 100644 index 0000000000..2ffc80fb1b --- /dev/null +++ b/pkg/lib/update/merge3/visitor.go @@ -0,0 +1,145 @@ +// Copyright 2019 The Kubernetes Authors. +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// copied from sigs.k8s.io/kustomize/kyaml@v0.21.1/yaml/merge3/visitor.go with modifications + +package merge3 + +import ( + "sigs.k8s.io/kustomize/kyaml/openapi" + "sigs.k8s.io/kustomize/kyaml/yaml" + "sigs.k8s.io/kustomize/kyaml/yaml/walk" +) + +type Visitor struct{} + +func (m *Visitor) VisitMap(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { // MODIFIED + // explicitly cleared from either dest or update + return walk.ClearNode, nil + } + if nodes.Dest() == nil && nodes.Updated() == nil { + // implicitly cleared missing from both dest and update + return walk.ClearNode, nil + } + + if nodes.Dest() == nil { + // not cleared, but missing from the dest + // initialize a new value that can be recursively merged + return yaml.NewRNode(&yaml.Node{Kind: yaml.MappingNode}), nil + } + + // recursively merge the dest with the original and updated + return nodes.Dest(), nil +} + +func (m *Visitor) visitAList(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { + if yaml.IsMissingOrNull(nodes.Updated()) && !yaml.IsMissingOrNull(nodes.Origin()) { + // implicitly cleared from update -- element was deleted + return walk.ClearNode, nil + } + if yaml.IsMissingOrNull(nodes.Dest()) { + // not cleared, but missing from the dest + // initialize a new value that can be recursively merged + return yaml.NewRNode(&yaml.Node{Kind: yaml.SequenceNode}), nil + } + + // recursively merge the dest with the original and updated + return nodes.Dest(), nil +} + +func (m *Visitor) VisitScalar(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { // MODIFIED + // explicitly cleared from either dest or update + return nil, nil + } + if yaml.IsMissingOrNull(nodes.Updated()) != yaml.IsMissingOrNull(nodes.Origin()) { + // value added or removed in update + return nodes.Updated(), nil + } + if yaml.IsMissingOrNull(nodes.Updated()) && yaml.IsMissingOrNull(nodes.Origin()) { + // value absent in both origin and update + return nodes.Dest(), nil + } + + if nodes.Updated().YNode().Value != nodes.Origin().YNode().Value { + // value changed in update + return nodes.Updated(), nil + } + + // unchanged between origin and update, keep the dest + return nodes.Dest(), nil +} + +func (m *Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { // MODIFIED + // explicitly cleared from either dest or update + return walk.ClearNode, nil + } + + if yaml.IsMissingOrNull(nodes.Updated()) != yaml.IsMissingOrNull(nodes.Origin()) { + // value added or removed in update + return nodes.Updated(), nil + } + if yaml.IsMissingOrNull(nodes.Updated()) && yaml.IsMissingOrNull(nodes.Origin()) { + // value not present in source or dest + return nodes.Dest(), nil + } + + if !m.isNodeContentEqual(nodes.Origin().YNode(), nodes.Updated().YNode()) { + // value changed in update + return nodes.Updated(), nil + } + + // unchanged between origin and update, keep the dest + return nodes.Dest(), nil +} + +// NEW +// isCleared returns if the node has not tagged null in `left` but explicitly removed in `right` +func (*Visitor) isCleared(left, right *yaml.RNode) bool { + return !left.IsTaggedNull() && right.IsTaggedNull() +} + +func (m *Visitor) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, kind walk.ListKind) (*yaml.RNode, error) { + if kind == walk.AssociativeList { + return m.visitAList(nodes, s) + } + // non-associative list + return m.visitNAList(nodes) +} + +// SIMPLIFIED +// isNodeContentEqual compares the nodes structurally (kind, tag, value and children), +// avoiding YAML serialization. Presentation style and comments are ignored. +func (m *Visitor) isNodeContentEqual(a, b *yaml.Node) bool { + if a == nil || b == nil { + return a == b + } + if a.Kind != b.Kind || a.Tag != b.Tag || a.Value != b.Value { + return false + } + if len(a.Content) != len(b.Content) { + return false + } + for i := range a.Content { + if !m.isNodeContentEqual(a.Content[i], b.Content[i]) { + return false + } + } + return true +} + +var _ walk.Visitor = &Visitor{} diff --git a/pkg/lib/update/resource-merge_test.go b/pkg/lib/update/resource-merge_test.go index b68072d607..05e4eb07f3 100644 --- a/pkg/lib/update/resource-merge_test.go +++ b/pkg/lib/update/resource-merge_test.go @@ -16,14 +16,17 @@ package update_test import ( + "os" "path/filepath" "testing" + "github.com/google/go-cmp/cmp" "github.com/kptdev/kpt/internal/testutil" "github.com/kptdev/kpt/internal/testutil/pkgbuilder" "github.com/kptdev/kpt/pkg/lib/update" "github.com/kptdev/kpt/pkg/lib/update/updatetypes" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestUpdate_ResourceMerge(t *testing.T) { @@ -301,3 +304,43 @@ func TestUpdate_ResourceMerge(t *testing.T) { }) } } + +func TestKeepTaggedNull(t *testing.T) { + updater := update.ResourceMergeUpdater{} + + testdata, err := filepath.Abs(filepath.Join(".", "testdata", "keep-tagged-null")) + require.NoError(t, err) + + tmpDir := t.TempDir() + newDest := filepath.Join(tmpDir, "dest") + err = os.MkdirAll(newDest, 0755) + require.NoError(t, err) + + oldDestFiles, err := os.ReadDir(filepath.Join(testdata, "dest")) + require.NoError(t, err) + for _, file := range oldDestFiles { + if file.Type().IsRegular() { + content, err := os.ReadFile(filepath.Join(testdata, "dest", file.Name())) + require.NoError(t, err) + err = os.WriteFile(filepath.Join(newDest, file.Name()), content, 0644) + require.NoError(t, err) + } + } + + options := updatetypes.Options{ + OriginPath: filepath.Join(testdata, "origin"), + UpdatedPath: filepath.Join(testdata, "updated"), + LocalPath: newDest, + } + err = updater.Update(options) + require.NoError(t, err) + + destBytes, err := os.ReadFile(filepath.Join(newDest, "configmap.yaml")) + require.NoError(t, err) + expBytes, err := os.ReadFile(filepath.Join(testdata, "expected", "configmap.yaml")) + require.NoError(t, err) + + if diff := cmp.Diff(expBytes, destBytes); diff != "" { + t.Errorf("unexpected result (-want, +got): %s", diff) + } +} diff --git a/pkg/lib/update/testdata/keep-tagged-null/dest/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/dest/Kptfile new file mode 100644 index 0000000000..068e1941c4 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/dest/Kptfile @@ -0,0 +1,21 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +upstream: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 +upstreamLock: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 + commit: b3e1d439516a5e8d49adc0c82d3e95578570dbfa +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml new file mode 100644 index 0000000000..73b6cea9ac --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml @@ -0,0 +1,20 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: m7 + namespace: default +data: + deletedInOrigin: + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' + deletedInDest: + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' + addedInUpdated: + unchange: origin diff --git a/pkg/lib/update/testdata/keep-tagged-null/expected/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/expected/Kptfile new file mode 100644 index 0000000000..068e1941c4 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/expected/Kptfile @@ -0,0 +1,21 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +upstream: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 +upstreamLock: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 + commit: b3e1d439516a5e8d49adc0c82d3e95578570dbfa +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml new file mode 100644 index 0000000000..b0f4b84153 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml @@ -0,0 +1,19 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: m7 + namespace: default +data: + deletedInOrigin: + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: "something" + emptyStringKey2: 'something' + deletedInDest: + emptyStringKey1: "something" + emptyStringKey2: 'something' + missKey: something + addedInUpdated: + unchange: origin + addedKey: something diff --git a/pkg/lib/update/testdata/keep-tagged-null/origin/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/origin/Kptfile new file mode 100644 index 0000000000..7857f395c5 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/origin/Kptfile @@ -0,0 +1,8 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml new file mode 100644 index 0000000000..96b5244204 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml @@ -0,0 +1,21 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: m7 + namespace: default +data: + deletedInOrigin: + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' + deletedInDest: + nullKey: origin + unsetKey: origin + tildeKey: origin + emptyStringKey1: origin + emptyStringKey2: origin + missKey: origin + addedInUpdated: + unchange: origin \ No newline at end of file diff --git a/pkg/lib/update/testdata/keep-tagged-null/updated/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/updated/Kptfile new file mode 100644 index 0000000000..7857f395c5 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/updated/Kptfile @@ -0,0 +1,8 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml new file mode 100644 index 0000000000..ccfdcd36ed --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml @@ -0,0 +1,22 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: m7 + namespace: default +data: + deletedInOrigin: + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: something + emptyStringKey2: something + deletedInDest: + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: something + emptyStringKey2: something + missKey: something + addedInUpdated: + unchange: origin + addedKey: something \ No newline at end of file