Add resource version checks (#3993)

* Add resource version checks

* Retrieve the updated object before a second update

* gofmt

* Increase timeout for cleanup wait

* Ignore resource-version in test output

* Oops

* Move annotation handling to the client

* Clean up go.mod

* Do not swallow errors

* Fix return value checking

* Update e2e tests

* Fix empty string issue with reorderYamlStdout

* Revert some unnecessary changes

commands/alpha/rpkg/pull/command.go

@@ -22,6 +22,7 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/GoogleContainerTools/kpt/commands/alpha/rpkg/util"
 	"github.com/GoogleContainerTools/kpt/internal/docs/generated/rpkgdocs"
 	"github.com/GoogleContainerTools/kpt/internal/errors"
 	"github.com/GoogleContainerTools/kpt/internal/util/cmdutil"
@@ -112,6 +113,10 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
 		return errors.E(op, err)
 	}
 
+	if err := util.AddResourceVersionAnnotation(&resources); err != nil {
+		return errors.E(op, err)
+	}
+
 	if len(args) > 1 {
 		if err := writeToDir(resources.Spec.Resources, args[1]); err != nil {
 			return errors.E(op, err)

commands/alpha/rpkg/pull/command_test.go

@@ -74,6 +74,7 @@ items:
     name: bar
     annotations:
       config.kubernetes.io/local-config: "true"
+      internal.kpt.dev/resource-version: "999"
       config.kubernetes.io/index: '0'
       internal.config.kubernetes.io/index: '0'
       internal.config.kubernetes.io/path: 'Kptfile'
@@ -126,6 +127,7 @@ items:
     name: bar
     annotations:
       config.kubernetes.io/local-config: "true"
+      internal.kpt.dev/resource-version: "999"
       config.kubernetes.io/index: '0'
       internal.config.kubernetes.io/index: '0'
       internal.config.kubernetes.io/path: 'Kptfile'

commands/alpha/rpkg/push/command.go

@@ -25,6 +25,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/GoogleContainerTools/kpt/commands/alpha/rpkg/util"
 	"github.com/GoogleContainerTools/kpt/internal/docs/generated/rpkgdocs"
 	"github.com/GoogleContainerTools/kpt/internal/errors"
 	"github.com/GoogleContainerTools/kpt/internal/fnruntime"
@@ -132,6 +133,16 @@ func (r *runner) runE(cmd *cobra.Command, args []string) error {
 			Resources: resources,
 		},
 	}
+
+	rv, err := util.GetResourceVersionAnnotation(&pkgResources)
+	if err != nil {
+		return errors.E(op, err)
+	}
+	pkgResources.ResourceVersion = rv
+	if err = util.RemoveResourceVersionAnnotation(&pkgResources); err != nil {
+		return errors.E(op, err)
+	}
+
 	if err := r.client.Update(r.ctx, &pkgResources); err != nil {
 		return errors.E(op, err)
 	}

commands/alpha/rpkg/util/common.go

@@ -1,4 +1,4 @@
-// Copyright 2022 The kpt Authors
+// Copyright 2023 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.
@@ -16,15 +16,21 @@ package util
 
 import (
 	"context"
+	"fmt"
 
-	porchapi "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
+	fnsdk "github.com/GoogleContainerTools/kpt-functions-sdk/go/fn"
+	api "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+const (
+	ResourceVersionAnnotation = "internal.kpt.dev/resource-version"
+)
+
 func PackageAlreadyExists(ctx context.Context, c client.Client, repository, packageName, namespace string) (bool, error) {
 	// only the first package revision can be created from init or clone, so
 	// we need to check that the package doesn't already exist.
-	packageRevisionList := porchapi.PackageRevisionList{}
+	packageRevisionList := api.PackageRevisionList{}
 	if err := c.List(ctx, &packageRevisionList, &client.ListOptions{
 		Namespace: namespace,
 	}); err != nil {
@@ -37,3 +43,67 @@ func PackageAlreadyExists(ctx context.Context, c client.Client, repository, pack
 	}
 	return false, nil
 }
+
+func GetResourceFileKubeObject(prr *api.PackageRevisionResources, file, kind, name string) (*fnsdk.KubeObject, error) {
+	if prr.Spec.Resources == nil {
+		return nil, fmt.Errorf("nil resources found for PackageRevisionResources '%s/%s'", prr.Namespace, prr.Name)
+	}
+
+	if _, ok := prr.Spec.Resources[file]; !ok {
+		return nil, fmt.Errorf("%q not found in PackageRevisionResources '%s/%s'", file, prr.Namespace, prr.Name)
+	}
+
+	ko, err := fnsdk.ParseKubeObject([]byte(prr.Spec.Resources[file]))
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse %q of PackageRevisionResources %s/%s: %w", file, prr.Namespace, prr.Name, err)
+	}
+	if kind != "" && ko.GetKind() != kind {
+		return nil, fmt.Errorf("%q does not contain kind %q in PackageRevisionResources '%s/%s'", file, kind, prr.Namespace, prr.Name)
+	}
+	if name != "" && ko.GetName() != name {
+		return nil, fmt.Errorf("%q does not contain resource named %q in PackageRevisionResources '%s/%s'", file, name, prr.Namespace, prr.Name)
+	}
+
+	return ko, nil
+}
+
+func GetResourceVersionAnnotation(prr *api.PackageRevisionResources) (string, error) {
+	ko, err := GetResourceFileKubeObject(prr, "Kptfile", "Kptfile", "")
+
+	if err != nil {
+		return "", err
+	}
+	annotations := ko.GetAnnotations()
+	rv, ok := annotations[ResourceVersionAnnotation]
+	if !ok {
+		rv = ""
+	}
+	return rv, nil
+}
+
+func AddResourceVersionAnnotation(prr *api.PackageRevisionResources) error {
+	ko, err := GetResourceFileKubeObject(prr, "Kptfile", "Kptfile", "")
+	if err != nil {
+		return err
+	}
+
+	ko.SetAnnotation(ResourceVersionAnnotation, prr.GetResourceVersion())
+	prr.Spec.Resources["Kptfile"] = ko.String()
+
+	return nil
+}
+
+func RemoveResourceVersionAnnotation(prr *api.PackageRevisionResources) error {
+	ko, err := GetResourceFileKubeObject(prr, "Kptfile", "Kptfile", "")
+	if err != nil {
+		return err
+	}
+
+	_, err = ko.RemoveNestedField("metadata", "annotations", ResourceVersionAnnotation)
+	if err != nil {
+		return err
+	}
+	prr.Spec.Resources["Kptfile"] = ko.String()
+
+	return nil
+}

e2e/porch_test.go

@@ -17,6 +17,7 @@
 package e2e
 
 import (
+	"bufio"
 	"bytes"
 	"errors"
 	"io/fs"
@@ -45,10 +46,26 @@ func TestPorch(t *testing.T) {
 	runTests(t, abs)
 }
 
+func runUtilityCommand(t *testing.T, command string, args ...string) error {
+	cmd := exec.Command(command, args...)
+	t.Logf("running utility command %s %s", command, strings.Join(args, " "))
+	return cmd.Run()
+}
+
 func runTests(t *testing.T, path string) {
 	gitServerURL := startGitServer(t, path)
 	testCases := scanTestCases(t, path)
 
+	// remove any tmp files from previous test runs
+	err := runUtilityCommand(t, "rm", "-rf", "/tmp/porch-e2e")
+	if err != nil {
+		t.Fatalf("Failed to clean up older run: %v", err)
+	}
+	err = runUtilityCommand(t, "mkdir", "/tmp/porch-e2e")
+	if err != nil {
+		t.Fatalf("Failed to create temp directory: %v", err)
+	}
+
 	for _, tc := range testCases {
 		t.Run(tc.TestCase, func(t *testing.T) {
 			if tc.Skip != "" {
@@ -71,7 +88,7 @@ func runTestCase(t *testing.T, repoURL string, tc porch.TestCaseConfig) {
 	}
 
 	for i := range tc.Commands {
-		time.Sleep(5 * time.Second)
+		time.Sleep(1 * time.Second)
 		command := &tc.Commands[i]
 		cmd := exec.Command("kpt", command.Args...)
 
@@ -89,6 +106,8 @@ func runTestCase(t *testing.T, repoURL string, tc porch.TestCaseConfig) {
 			reorderYamlStdout(t, &stdout)
 		}
 
+		cleanupStderr(t, &stderr)
+
 		if os.Getenv(updateGoldenFiles) != "" {
 			updateCommand(command, err, stdout.String(), stderr.String())
 		}
@@ -102,16 +121,57 @@ func runTestCase(t *testing.T, repoURL string, tc porch.TestCaseConfig) {
 		if got, want := stderr.String(), command.Stderr; got != want {
 			t.Errorf("unexpected stderr content from 'kpt %s'; (-want, +got) %s", strings.Join(command.Args, " "), cmp.Diff(want, got))
 		}
+
+		// hack here; but if the command registered a repo, give a few extra seconds for the repo to reach readiness
+		for _, arg := range command.Args {
+			if arg == "register" {
+				time.Sleep(5 * time.Second)
+			}
+		}
 	}
 
 	if os.Getenv(updateGoldenFiles) != "" {
 		porch.WriteTestCaseConfig(t, &tc)
 	}
 }
 
+// remove PASS lines from kpt fn eval, which includes a duration and will vary
+func cleanupStderr(t *testing.T, buf *bytes.Buffer) {
+	scanner := bufio.NewScanner(buf)
+	var newBuf bytes.Buffer
+	for scanner.Scan() {
+		line := scanner.Text()
+		if !strings.Contains(line, "[PASS]") {
+			newBuf.Write([]byte(line))
+			newBuf.Write([]byte("\n"))
+		}
+	}
+
+	buf.Reset()
+	if _, err := buf.Write(newBuf.Bytes()); err != nil {
+		t.Fatalf("Failed to update cleaned up stderr: %v", err)
+	}
+}
+
 func reorderYamlStdout(t *testing.T, buf *bytes.Buffer) {
+	if buf.Len() == 0 {
+		return
+	}
+
+	// strip out the internal.kpt.dev/resource-version
+	// annotation, because that will change with every run
+	scanner := bufio.NewScanner(buf)
+	var newBuf bytes.Buffer
+	for scanner.Scan() {
+		line := scanner.Text()
+		if !strings.Contains(line, "internal.kpt.dev/resource-version:") {
+			newBuf.Write([]byte(line))
+			newBuf.Write([]byte("\n"))
+		}
+	}
+
 	var data interface{}
-	if err := yaml.Unmarshal(buf.Bytes(), &data); err != nil {
+	if err := yaml.Unmarshal(newBuf.Bytes(), &data); err != nil {
 		// not yaml.
 		return
 	}

e2e/testdata/porch/rpkg-push/config.yaml

@@ -58,30 +58,19 @@ commands:
       - git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f
     stdin: |
       apiVersion: config.kubernetes.io/v1
-      kind: ResourceList
       items:
       - apiVersion: kpt.dev/v1
+        info:
+          description: sample description
         kind: Kptfile
         metadata:
-          name: test-package
           annotations:
-            config.kubernetes.io/index: '0'
+            config.kubernetes.io/index: "0"
             config.kubernetes.io/local-config: "true"
             config.kubernetes.io/path: Kptfile
-            internal.config.kubernetes.io/index: '0'
-            internal.config.kubernetes.io/path: 'Kptfile'
-        info:
-          site: http://kpt.dev/test-package
-          description: Updated Test Package Description
-      - apiVersion: v1
-        kind: ConfigMap
-        metadata:
-          name: new-config-map
-          annotations:
-            internal.config.kubernetes.io/path: 'config-map.yaml'
-            config.kubernetes.io/path: 'config-map.yaml'
-        data:
-          key: value
+            internal.config.kubernetes.io/index: "0"
+            internal.config.kubernetes.io/path: Kptfile
+          name: test-package
       - apiVersion: v1
         data:
           name: example
@@ -94,6 +83,36 @@ commands:
             internal.config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/path: package-context.yaml
           name: kptfile.kpt.dev
+      kind: ResourceList
+    yaml: true
+    exitCode: 1
+    stderr: "Error: Internal error occurred: resourceVersion must be specified for an update \n"
+  - args:
+      - alpha
+      - rpkg
+      - pull
+      - --namespace=rpkg-push
+      - git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f
+      - /tmp/porch-e2e/rpkg-push-git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f
+  - args:
+      - fn
+      - eval
+      - --image
+      - gcr.io/kpt-fn/search-replace:v0.2.0
+      - --match-kind
+      - Kptfile
+      - /tmp/porch-e2e/rpkg-push-git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f
+      - --
+      - by-path=info.description
+      - put-value=Updated Test Package Description
+    stderr: "[RUNNING] \"gcr.io/kpt-fn/search-replace:v0.2.0\" on 1 resource(s)\n  Results:\n    [info] info.description: Mutated field value to \"Updated Test Package Description\"\n"
+  - args:
+      - alpha
+      - rpkg
+      - push
+      - --namespace=rpkg-push
+      - git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f
+      - /tmp/porch-e2e/rpkg-push-git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f
   - args:
       - alpha
       - rpkg
@@ -106,7 +125,6 @@ commands:
       - apiVersion: kpt.dev/v1
         info:
           description: Updated Test Package Description
-          site: http://kpt.dev/test-package
         kind: Kptfile
         metadata:
           annotations:
@@ -116,17 +134,6 @@ commands:
             internal.config.kubernetes.io/index: "0"
             internal.config.kubernetes.io/path: Kptfile
           name: test-package
-      - apiVersion: v1
-        data:
-          key: value
-        kind: ConfigMap
-        metadata:
-          annotations:
-            config.kubernetes.io/index: "0"
-            config.kubernetes.io/path: config-map.yaml
-            internal.config.kubernetes.io/index: "0"
-            internal.config.kubernetes.io/path: config-map.yaml
-          name: new-config-map
       - apiVersion: v1
         data:
           name: example