Repository: GoogleContainerTools/kpt
Pull request #3360 (2022-07-12 03:20:12)

add upstream identifier in pkg update, get and clone(porch)
- This PR lets `kpt` to add `internal.kpt.dev/upstream-identifier` annotation together with `kpt-merge` comment. 
- This function is called in [kpt pkg get](https://github.com/GoogleContainerTools/kpt/blob/72ef5b29cb372ed1d28e1380782651f366e2d14f/internal/util/get/get.go#L117), [kpt pkg update](https://github.com/GoogleContainerTools/kpt/blob/6008784a2a6de63ecba903d16c1a317022287b6e/internal/util/update/update.go#L185) and porch [package clone](https://github.com/GoogleContainerTools/kpt/blob/604b6a483f434d599abb2616fd5b392ee768654b/porch/pkg/engine/clone.go#L95), porch [package update](https://github.com/GoogleContainerTools/kpt/blob/604b6a483f434d599abb2616fd5b392ee768654b/porch/pkg/engine/engine.go#L532)  
- As discussed offline, we currently continue to rely on `kpt-merge` comment to match upstream resource and establish three-way merge. 

- Fix an existing issue where `addmergecomment` detects the resource meta by kyaml `IsNilOrEmpty`. not the meta can never be NilorEmpty since helper annotations are always added.    

yuwenma: Can you review this change @justinsb @droot 

Patch:
diff --git a/internal/util/addmergecomment/addmergecomment.go b/internal/util/addmergecomment/addmergecomment.go
index 215aea51..2b4a3903 100644
--- a/internal/util/addmergecomment/addmergecomment.go
+++ b/internal/util/addmergecomment/addmergecomment.go
@@ -23,9 +23,22 @@ import (
 	"github.com/GoogleContainerTools/kpt/internal/util/merge"
 	"sigs.k8s.io/kustomize/kyaml/copyutil"
 	"sigs.k8s.io/kustomize/kyaml/kio"
+	"sigs.k8s.io/kustomize/kyaml/resid"
 	kyaml "sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
+// TODO(yuwenma): Those const vars are defined in kpt-functions-sdk/go/fn v0.0.0-20220706221933-7181f451a663+
+// we cannot import go/fn directly because the porch/set-namespace uses an older go/fn version. Bumping kpt module alone fails
+// kpt CI.
+// We should update porch/set-namespace once https://github.com/GoogleContainerTools/kpt-functions-catalog/pull/885 is released.
+// and cleanup the const vars below
+const (
+	upstreamIdentifierFmt = "%s|%s|%s|%s"
+	upstreamIdentifier    = "internal.kpt.dev/upstream-identifier"
+	unknownNamespace      = "~C"
+	defaultNamespace      = "default"
+)
+
 // AddMergeComment adds merge comments with format "kpt-merge: namespace/name"
 // to all resources in the package
 type AddMergeComment struct{}
@@ -49,6 +62,38 @@ func Process(paths ...string) error {
 	return nil
 }
 
+// addUpstreamAnnotation adds internal.kpt.dev/upstream-identifier annotation to resource.
+// In a 3 level package chain (root -> branch -> deployable), the downstream package uses the upstream package meta GKNN
+// as its upstream origin, not the upstream its own origin. For example
+// root: No `upstreamIdentifier` annotation
+// branch: `upstream-identifier=rootGKNN`
+// deployable: `upstream-identifier=branchGKNN`
+// One known caveat is that upstream meta change can cause downstream origin mismatch. This potentially causes the 3-way merge
+// to fail in pkg update step.
+func addUpstreamAnnotation(object *kyaml.RNode, mergeComment string) error {
+	annotations := object.GetAnnotations()
+	group, _ := resid.ParseGroupVersion(object.GetApiVersion())
+	var name, namespace string
+	if strings.Contains(mergeComment, merge.MergeCommentPrefix) {
+		nsAndName := merge.NsAndNameForMerge(mergeComment)
+		namespace = nsAndName[0]
+		name = nsAndName[1]
+	} else {
+		namespace = object.GetNamespace()
+		name = object.GetName()
+	}
+	// Convert namespace to follow the upstream identifier convention, where
+	// - empty string is treated as "default"
+	// - unknown custom resource or cluster scoped resource use placeholder "~C"
+	if namespace == "" {
+		namespace = defaultNamespace
+	} else if object.GetNamespace() == resid.TotallyNotANamespace {
+		namespace = unknownNamespace
+	}
+	annotations[upstreamIdentifier] = fmt.Sprintf(upstreamIdentifierFmt, group, object.GetKind(), namespace, name)
+	return object.SetAnnotations(annotations)
+}
+
 // Filter implements kyaml.Filter
 // this filter adds merge comment with format "kpt-merge: namespace/name" to
 // the input resource, if the namespace field doesn't exist on the resource,
@@ -60,15 +105,33 @@ func (amc *AddMergeComment) Filter(object *kyaml.RNode) (*kyaml.RNode, error) {
 		return object, nil
 	}
 	mf := object.Field(kyaml.MetadataField)
-	if mf.IsNilOrEmpty() {
-		// skip adding merge comment if empty metadata
+	if object.GetName() == "" && object.GetNamespace() == "" && len(object.GetLabels()) == 0 {
+		// skip adding merge comment if empty metadata. Since the intermediate annotations always exist,
+		// mf.IsNilOrEmpty cannot tell whether it's empty meta or not.
+		// e.g. Empty meta with internal annotations.
+		//kind: MyKind
+		//spec:
+		//  replicas: 3
+		//metadata:
+		//  annotations:
+		//    config.kubernetes.io/index: '0'
+		//    config.kubernetes.io/path: 'k8s-cli-982798852.yaml'
+		//    internal.config.kubernetes.io/index: '0'
+		//    internal.config.kubernetes.io/path: 'k8s-cli-982798852.yaml'
+		//    internal.config.kubernetes.io/seqindent: 'compact'
+		//    internal.config.kubernetes.io/annotations-migration-resource-id: '0'
 		return object, nil
 	}
-	if strings.Contains(mf.Key.YNode().LineComment, merge.MergeCommentPrefix) {
-		// skip adding merge comment if merge comment is already present
+
+	// Only add merge comment if merge comment does not present
+	if !strings.Contains(mf.Key.YNode().LineComment, merge.MergeCommentPrefix) {
+		mf.Key.YNode().LineComment = fmt.Sprintf("%s %s/%s", merge.MergeCommentPrefix, rm.Namespace, rm.Name)
+	}
+	// We will migrate kpt-merge comment to upstream-identifier annotation. As an intermediate stage, this filter
+	// preserves the mergeComment behavior to guarantee the backward compatibility.
+	if err := addUpstreamAnnotation(object, mf.Key.YNode().LineComment); err != nil {
 		return object, nil
 	}
-	mf.Key.YNode().LineComment = fmt.Sprintf("%s %s/%s", merge.MergeCommentPrefix, rm.Namespace, rm.Name)
 	return object, nil
 }
 
diff --git a/internal/util/merge/merge3.go b/internal/util/merge/merge3.go
index effdf27b..836d6367 100644
--- a/internal/util/merge/merge3.go
+++ b/internal/util/merge/merge3.go
@@ -247,7 +247,7 @@ func resolveGroup(meta yaml.ResourceMeta) string {
 // resolveNamespace resolves the namespace which should be used for merging resources
 // uses namespace from comment on metadata field if present, falls back to resource namespace
 func resolveNamespace(meta yaml.ResourceMeta, metadataComment string) string {
-	nsName := nsAndNameForMerge(metadataComment)
+	nsName := NsAndNameForMerge(metadataComment)
 	if nsName == nil {
 		return meta.Namespace
 	}
@@ -257,17 +257,17 @@ func resolveNamespace(meta yaml.ResourceMeta, metadataComment string) string {
 // resolveName resolves the name which should be used for merging resources
 // uses name from comment on metadata field if present, falls back to resource name
 func resolveName(meta yaml.ResourceMeta, metadataComment string) string {
-	nsName := nsAndNameForMerge(metadataComment)
+	nsName := NsAndNameForMerge(metadataComment)
 	if nsName == nil {
 		return meta.Name
 	}
 	return nsName[1]
 }
 
-// nsAndNameForMerge returns the namespace and name for merge
+// NsAndNameForMerge returns the namespace and name for merge
 // from the line comment on the metadata field
 // e.g. metadata: # kpt-merge: default/foo returns [default, foo]
-func nsAndNameForMerge(metadataComment string) []string {
+func NsAndNameForMerge(metadataComment string) []string {
 	comment := strings.TrimPrefix(metadataComment, "#")
 	comment = strings.TrimSpace(comment)
 	if !strings.HasPrefix(comment, MergeCommentPrefix) {


Test patch:
diff --git a/e2e/testdata/porch/rpkg-clone/config.yaml b/e2e/testdata/porch/rpkg-clone/config.yaml
index 43034098..5594e47b 100644
--- a/e2e/testdata/porch/rpkg-clone/config.yaml
+++ b/e2e/testdata/porch/rpkg-clone/config.yaml
@@ -60,6 +60,7 @@ commands:
             config.kubernetes.io/path: Kptfile
             internal.config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/path: Kptfile
+            internal.kpt.dev/upstream-identifier: kpt.dev|Kptfile|default|basens-clone
           name: basens-clone
         upstream:
           git:
@@ -82,6 +83,7 @@ commands:
             config.kubernetes.io/path: namespace.yaml
             internal.config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/path: namespace.yaml
+            internal.kpt.dev/upstream-identifier: '|Namespace|default|example'
           name: example
       kind: ResourceList
     yaml: true
@@ -103,6 +105,7 @@ commands:
             config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/path: Kptfile
+            internal.kpt.dev/upstream-identifier: kpt.dev|Kptfile|default|empty-clone
           name: empty-clone
         upstream:
           git:
diff --git a/e2e/testdata/porch/rpkg-copy/config.yaml b/e2e/testdata/porch/rpkg-copy/config.yaml
index 64c7eeb8..b6af4dec 100644
--- a/e2e/testdata/porch/rpkg-copy/config.yaml
+++ b/e2e/testdata/porch/rpkg-copy/config.yaml
@@ -58,6 +58,7 @@ commands:
             config.kubernetes.io/path: Kptfile
             internal.config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/path: Kptfile
+            internal.kpt.dev/upstream-identifier: kpt.dev|Kptfile|default|basens-edit
           name: basens-edit
         upstream:
           git:
@@ -80,6 +81,7 @@ commands:
             config.kubernetes.io/path: namespace.yaml
             internal.config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/path: namespace.yaml
+            internal.kpt.dev/upstream-identifier: '|Namespace|default|example'
           name: example
       kind: ResourceList
     yaml: true
diff --git a/internal/util/addmergecomment/addmergecomment_test.go b/internal/util/addmergecomment/addmergecomment_test.go
index d2c78500..d40f414e 100644
--- a/internal/util/addmergecomment/addmergecomment_test.go
+++ b/internal/util/addmergecomment/addmergecomment_test.go
@@ -46,6 +46,8 @@ kind: Deployment
 metadata: # kpt-merge: my-space/nginx-deployment
   name: nginx-deployment
   namespace: my-space
+  annotations:
+    internal.kpt.dev/upstream-identifier: apps|Deployment|my-space|nginx-deployment
 spec:
   replicas: 3
  `,
@@ -67,6 +69,8 @@ kind: Deployment
 metadata: # kpt-merge: default/nginx-deployment
   name: nginx-deployment
   namespace: default
+  annotations:
+    internal.kpt.dev/upstream-identifier: apps|Deployment|default|nginx-deployment
 spec:
   replicas: 3
  `,
@@ -86,6 +90,8 @@ apiVersion: apps/v1
 kind: Deployment
 metadata: # kpt-merge: /nginx-deployment
   name: nginx-deployment
+  annotations:
+    internal.kpt.dev/upstream-identifier: apps|Deployment|default|nginx-deployment
 spec:
   replicas: 3
  `,
@@ -105,12 +111,14 @@ apiVersion: apps/v1
 kind: Deployment
 metadata: # kpt-merge: my-space/nginx-deployment
   name: nginx-deployment-new
+  annotations:
+    internal.kpt.dev/upstream-identifier: apps|Deployment|my-space|nginx-deployment
 spec:
   replicas: 3
  `,
 		},
 		{
-			name: "Skip adding kpt merge comment if already present",
+			name: "Skip adding kpt merge comment if skip meta",
 			input: `
 apiVersion: apps/v1
 kind: MyKind
diff --git a/internal/util/diff/diff_test.go b/internal/util/diff/diff_test.go
index ae4b4269..fdd1d0ea 100644
--- a/internal/util/diff/diff_test.go
+++ b/internal/util/diff/diff_test.go
@@ -72,11 +72,11 @@ func TestCommand_Diff(t *testing.T) {
 			diffTool: "diff",
 			diffOpts: "-r -i -w",
 			expDiff: `
-39c39
+41c41
 <             - containerPort: 80
 ---
 >             - containerPort: 8081
-25,27c25,27
+27,29c27,29
 <     - name: "80"
 <       port: 80
 <       targetPort: 80
@@ -112,11 +112,11 @@ func TestCommand_Diff(t *testing.T) {
 			diffTool: "diff",
 			diffOpts: "-r -i -w",
 			expDiff: `
-39c39
+41c41
 <             - containerPort: 80
 ---
 >             - containerPort: 8081
-25,27c25,27
+27,29c27,29
 <     - name: "80"
 <       port: 80
 <       targetPort: 80
@@ -153,11 +153,11 @@ func TestCommand_Diff(t *testing.T) {
 			diffTool: "diff",
 			diffOpts: "-r -i -w",
 			expDiff: `
-39c39
+41c41
 <             - containerPort: 8081
 ---
 >             - containerPort: 80
-25,27c25,27
+27,29c27,29
 <     - name: "8081"
 <       port: 8081
 <       targetPort: 8081
@@ -199,7 +199,7 @@ func TestCommand_Diff(t *testing.T) {
 			diffTool: "diff",
 			diffOpts: "-r -i -w",
 			expDiff: `
-7c7
+9c9
 <   replicas: 5
 ---
 >   replicas: 3
@@ -250,7 +250,7 @@ locally changed: foo
 			diffTool: "diff",
 			diffOpts: "-r -i -w",
 			expDiff: `
-7c7
+9c9
 <   replicas: 5
 ---
 >   replicas: 3
@@ -305,7 +305,7 @@ locally changed: foo
 			diffTool: "diff",
 			diffOpts: "-r -i -w",
 			expDiff: `
-7c7
+9c9
 <   replicas: 3
 ---
 >   replicas: 5