diff --git a/commands/alpha/rpkg/pull/command.go b/commands/alpha/rpkg/pull/command.go index 7515cf6ec9..e022f80ee7 100644 --- a/commands/alpha/rpkg/pull/command.go +++ b/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) diff --git a/commands/alpha/rpkg/pull/command_test.go b/commands/alpha/rpkg/pull/command_test.go index eca7212b4c..2adc9b0b1d 100644 --- a/commands/alpha/rpkg/pull/command_test.go +++ b/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' diff --git a/commands/alpha/rpkg/push/command.go b/commands/alpha/rpkg/push/command.go index 3e83cf4529..e96f713292 100644 --- a/commands/alpha/rpkg/push/command.go +++ b/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) } diff --git a/commands/alpha/rpkg/util/common.go b/commands/alpha/rpkg/util/common.go index e4cd3a61c8..7dafe9795d 100644 --- a/commands/alpha/rpkg/util/common.go +++ b/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 +} diff --git a/e2e/porch_test.go b/e2e/porch_test.go index 3d18b77d6f..2f9b277e33 100644 --- a/e2e/porch_test.go +++ b/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,6 +121,13 @@ 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) != "" { @@ -109,9 +135,43 @@ func runTestCase(t *testing.T, repoURL string, tc porch.TestCaseConfig) { } } +// 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 } diff --git a/e2e/testdata/porch/rpkg-push/config.yaml b/e2e/testdata/porch/rpkg-push/config.yaml index e9ba0c4429..378ab6e724 100644 --- a/e2e/testdata/porch/rpkg-push/config.yaml +++ b/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 diff --git a/e2e/testdata/porch/rpkg-update/config.yaml b/e2e/testdata/porch/rpkg-update/config.yaml index 1a7139a09f..e813f1b760 100644 --- a/e2e/testdata/porch/rpkg-update/config.yaml +++ b/e2e/testdata/porch/rpkg-update/config.yaml @@ -111,51 +111,32 @@ commands: git-3f036055f7ba68706372cbe0c4b14d553794f7c4 No update available git-7fcdd499f0790ac3bd8f805e3e5e00825641eb60 No update available git-7ab0219ace10c1081a8b40a6b97d5da58bdb62e0 git No update available + - args: + - alpha + - rpkg + - pull + - --namespace=rpkg-update + - git-7ab0219ace10c1081a8b40a6b97d5da58bdb62e0 + - /tmp/porch-e2e/pkg-update-git-7ab0219ace10c1081a8b40a6b97d5da58bdb62e0 + - args: + - fn + - eval + - --image + - gcr.io/kpt-fn/search-replace:v0.2.0 + - --match-kind + - Kptfile + - /tmp/porch-e2e/pkg-update-git-7ab0219ace10c1081a8b40a6b97d5da58bdb62e0 + - -- + - by-path=upstreamLock.git.ref + - put-value=invalid + stderr: "[RUNNING] \"gcr.io/kpt-fn/search-replace:v0.2.0\" on 1 resource(s)\n Results:\n [info] upstreamLock.git.ref: Mutated field value to \"invalid\"\n" - args: - alpha - rpkg - push - --namespace=rpkg-update - git-7ab0219ace10c1081a8b40a6b97d5da58bdb62e0 - stdin: | - apiVersion: config.kubernetes.io/v1 - kind: ResourceList - items: - - apiVersion: kpt.dev/v1 - kind: Kptfile - metadata: - name: basens-edit-clone - annotations: - 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' - upstream: - type: git - git: - repo: http://git-server.test-git-namespace.svc.cluster.local:8080/rpkg-update - directory: base-ns - ref: basens/v1 - upstreamLock: - type: git - git: - repo: http://git-server.test-git-namespace.svc.cluster.local:8080/rpkg-update - directory: base-ns - ref: invalid - commit: 8f3ee9e0d5724fbd0fbdcf81bef0d47d6d29cc64 - - apiVersion: v1 - data: - name: example - kind: ConfigMap - metadata: - annotations: - config.kubernetes.io/index: "0" - config.kubernetes.io/local-config: "true" - config.kubernetes.io/path: package-context.yaml - internal.config.kubernetes.io/index: "0" - internal.config.kubernetes.io/path: package-context.yaml - name: kptfile.kpt.dev + - /tmp/porch-e2e/pkg-update-git-7ab0219ace10c1081a8b40a6b97d5da58bdb62e0 - args: - alpha - rpkg @@ -163,4 +144,4 @@ commands: - --namespace=rpkg-update - --discover=upstream stderr: "Error: could not parse upstreamLock in Kptfile of package \"git-7ab0219ace10c1081a8b40a6b97d5da58bdb62e0\": malformed upstreamLock.Git.Ref \"invalid\" \n" - exitCode: 1 \ No newline at end of file + exitCode: 1 diff --git a/go.mod b/go.mod index 81565b4367..8b66c6bc26 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/GoogleContainerTools/kpt go 1.20 require ( + github.com/GoogleContainerTools/kpt-functions-sdk/go/fn v0.0.0-20220506190241-f85503febd54 github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20230121152246-dc44dbd18a33 github.com/GoogleContainerTools/kpt/rollouts v0.0.0-20230209223911-c6c49d0a0636 github.com/bytecodealliance/wasmtime-go v0.39.0 diff --git a/go.sum b/go.sum index d623e09822..e3698c6d9d 100644 --- a/go.sum +++ b/go.sum @@ -23,6 +23,8 @@ github.com/Azure/go-autorest/logger v0.2.1/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZ github.com/Azure/go-autorest/tracing v0.6.0 h1:TYi4+3m5t6K48TGI9AUdb+IzbnSxvnvUMfuitfgcfuo= github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/GoogleContainerTools/kpt-functions-sdk/go/fn v0.0.0-20220506190241-f85503febd54 h1:z5iYiugZJiTzQ6ggU0Cae/T+LkrDwcqZyebh8SbmQ0E= +github.com/GoogleContainerTools/kpt-functions-sdk/go/fn v0.0.0-20220506190241-f85503febd54/go.mod h1:vl3iiwgrqdDgvGi5ckt3O9IoyaHUgFkfxE4RjQIqgwk= github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20230121152246-dc44dbd18a33 h1:9M1bvq7hU/JTY4VVcqhCQT0eAa5HznrFaLAm2ldfe70= github.com/GoogleContainerTools/kpt/porch/api v0.0.0-20230121152246-dc44dbd18a33/go.mod h1:ASrhnLAL4ahTuiUJyepqcpVRXIoRMJyDs8/eSxwhgZM= github.com/GoogleContainerTools/kpt/rollouts v0.0.0-20230209223911-c6c49d0a0636 h1:9oUTINyozQvQ78QvKM3jTLI54oKMrq9V1VYpb4m8Asc= diff --git a/porch/pkg/engine/engine.go b/porch/pkg/engine/engine.go index ec3699dc18..b1a8944df6 100644 --- a/porch/pkg/engine/engine.go +++ b/porch/pkg/engine/engine.go @@ -53,6 +53,10 @@ import ( var tracer = otel.Tracer("engine") +const ( + OptimisticLockErrorMsg = "the object has been modified; please apply your changes to the latest version and try again" +) + type CaDEngine interface { // ObjectCache() is a cache of all our objects. ObjectCache() WatcherManager @@ -626,6 +630,15 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, repositoryObj * ctx, span := tracer.Start(ctx, "cadEngine::UpdatePackageRevision", trace.WithAttributes()) defer span.End() + newRV := newObj.GetResourceVersion() + if len(newRV) == 0 { + return nil, fmt.Errorf("resourceVersion must be specified for an update") + } + + if newRV != oldObj.GetResourceVersion() { + return nil, apierrors.NewConflict(api.Resource("packagerevisions"), oldObj.GetName(), fmt.Errorf(OptimisticLockErrorMsg)) + } + repo, err := cad.cache.OpenRepository(ctx, repositoryObj) if err != nil { return nil, err @@ -1049,6 +1062,15 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj return nil, nil, err } + newRV := new.GetResourceVersion() + if len(newRV) == 0 { + return nil, nil, fmt.Errorf("resourceVersion must be specified for an update") + } + + if newRV != old.GetResourceVersion() { + return nil, nil, apierrors.NewConflict(api.Resource("packagerevisionresources"), old.GetName(), fmt.Errorf(OptimisticLockErrorMsg)) + } + // Validate package lifecycle. Can only update a draft. switch lifecycle := rev.Spec.Lifecycle; lifecycle { default: diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go index 0c0fec48a8..d3c55220d5 100644 --- a/porch/test/e2e/e2e_test.go +++ b/porch/test/e2e/e2e_test.go @@ -1835,6 +1835,13 @@ func (t *PorchSuite) TestNewPackageRevisionLabels(ctx context.Context) { // Propose the package. pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed t.UpdateF(ctx, &pr) + + // retrieve the updated object + t.GetF(ctx, client.ObjectKey{ + Namespace: pr.Namespace, + Name: pr.Name, + }, &pr) + t.validateLabelsAndAnnos(ctx, pr.Name, map[string]string{ labelKey1: labelVal1, @@ -1859,6 +1866,12 @@ func (t *PorchSuite) TestNewPackageRevisionLabels(ctx context.Context) { }, ) + // retrieve the updated object + t.GetF(ctx, client.ObjectKey{ + Namespace: pr.Namespace, + Name: pr.Name, + }, &pr) + // Update the labels and annotations on the approved package. delete(pr.ObjectMeta.Labels, labelKey1) pr.ObjectMeta.Labels[labelKey2] = labelVal2 @@ -2508,7 +2521,7 @@ func (t *PorchSuite) waitUntilRepositoryDeleted(ctx context.Context, name, names } func (t *PorchSuite) waitUntilAllPackagesDeleted(ctx context.Context, repoName string) { - err := wait.PollImmediateWithContext(ctx, time.Second, 20*time.Second, func(ctx context.Context) (done bool, err error) { + err := wait.PollImmediateWithContext(ctx, time.Second, 60*time.Second, func(ctx context.Context) (done bool, err error) { var pkgRevList porchapi.PackageRevisionList if err := t.client.List(ctx, &pkgRevList); err != nil { t.Logf("error listing packages: %v", err)