From 33b52b3e52caa8587ef124e817cfa1aef2997a14 Mon Sep 17 00:00:00 2001 From: Catalin Stratulat Date: Tue, 26 Nov 2024 14:01:58 +0000 Subject: [PATCH 1/5] enforcing api error if pipeline render fails instead of ignoring error and allowing it to pass --- pkg/engine/engine.go | 4 +++- pkg/registry/porch/packagerevisionresources.go | 1 + pkg/task/generictaskhandler.go | 17 ++++++++++++----- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 6893e579..2ccc3fc4 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -504,7 +504,9 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj } renderStatus, err := cad.taskHandler.DoPRResourceMutations(ctx, pr2Update, draft, oldRes, newRes) - + if err != nil { + return nil, renderStatus, err + } // No lifecycle change when updating package resources; updates are done. repoPkgRev, err := draft.Close(ctx, "") if err != nil { diff --git a/pkg/registry/porch/packagerevisionresources.go b/pkg/registry/porch/packagerevisionresources.go index 10388484..0aa15826 100644 --- a/pkg/registry/porch/packagerevisionresources.go +++ b/pkg/registry/porch/packagerevisionresources.go @@ -183,6 +183,7 @@ func (r *packageRevisionResources) Update(ctx context.Context, name string, objI rev, renderStatus, err := r.cad.UpdatePackageResources(ctx, &repositoryObj, oldRepoPkgRev, oldApiPkgRevResources, newObj) if err != nil { + err := fmt.Errorf("%w%s%s%s", err, "\n\nKpt function pipeline error occurred during render.", "\nPackage has NOT been pushed to remote.", "\nAmend local package using error message above & retry.") return nil, false, apierrors.NewInternalError(err) } diff --git a/pkg/task/generictaskhandler.go b/pkg/task/generictaskhandler.go index e5ba8e51..9bfa37fb 100644 --- a/pkg/task/generictaskhandler.go +++ b/pkg/task/generictaskhandler.go @@ -225,19 +225,26 @@ func (th *genericTaskHandler) DoPRResourceMutations(ctx context.Context, pr2Upda var renderStatus *api.RenderStatus if len(appliedResources.Contents) > 0 { + // a porch package which fails render validation will no longer be accepted. // render the package - // Render failure will not fail the overall API operation. + // Render failure WILL fail the overall API operation. // The render error and result is captured as part of renderStatus above - // and is returned in packageresourceresources API's status field. We continue with - // saving the non-rendered resources to avoid losing user's changes. - // and supress this err. - _, renderStatus, _ = applyResourceMutations(ctx, + // and is returned in PackageRevisionResources API's status field. + // We do not push the package further to remote + // the users changes are captured on their local + // and can be amended using the error returned to properly validate/mutate the package before retrying + // we no longer suppress this err. + _, renderStatus, err = applyResourceMutations(ctx, draft, appliedResources, []mutation{&renderPackageMutation{ runnerOptions: runnerOptions, runtime: th.runtime, }}) + if err != nil { + klog.Errorf("Kpt function pipeline error occurred in render. Package has NOT been pushed to remote. Modify and Fix local package and retry.") + return renderStatus, err + } } else { renderStatus = nil } From aae5d46225220ef516abab0c2ebe64b7d2f16635 Mon Sep 17 00:00:00 2001 From: Catalin Stratulat Date: Tue, 26 Nov 2024 16:05:39 +0000 Subject: [PATCH 2/5] render error message should only print when a render error occurs not in the case of any error --- pkg/registry/porch/packagerevisionresources.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/registry/porch/packagerevisionresources.go b/pkg/registry/porch/packagerevisionresources.go index 0aa15826..40cc7b13 100644 --- a/pkg/registry/porch/packagerevisionresources.go +++ b/pkg/registry/porch/packagerevisionresources.go @@ -22,6 +22,7 @@ import ( api "github.com/nephio-project/porch/api/porch/v1alpha1" "github.com/nephio-project/porch/api/porchconfig/v1alpha1" + "github.com/nephio-project/porch/internal/kpt/errors" "github.com/nephio-project/porch/pkg/repository" "github.com/nephio-project/porch/pkg/util" "go.opentelemetry.io/otel/trace" @@ -183,7 +184,11 @@ func (r *packageRevisionResources) Update(ctx context.Context, name string, objI rev, renderStatus, err := r.cad.UpdatePackageResources(ctx, &repositoryObj, oldRepoPkgRev, oldApiPkgRevResources, newObj) if err != nil { - err := fmt.Errorf("%w%s%s%s", err, "\n\nKpt function pipeline error occurred during render.", "\nPackage has NOT been pushed to remote.", "\nAmend local package using error message above & retry.") + var specificErr *errors.Error + if errors.As(err, &specificErr) { + err := fmt.Errorf("%w%s%s%s", err, "\n\nKpt function pipeline error occurred during render.", "\nPackage has NOT been pushed to remote.", "\nAmend local package using error message above & retry.") + return nil, false, apierrors.NewInternalError(err) + } return nil, false, apierrors.NewInternalError(err) } From c6eeaff36131dd074f25303e605228eb98d06738 Mon Sep 17 00:00:00 2001 From: Catalin Stratulat Date: Wed, 27 Nov 2024 15:23:31 +0000 Subject: [PATCH 3/5] moved error formattion into relevant area --- pkg/registry/porch/packagerevisionresources.go | 6 ------ pkg/task/generictaskhandler.go | 1 + 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/registry/porch/packagerevisionresources.go b/pkg/registry/porch/packagerevisionresources.go index 40cc7b13..10388484 100644 --- a/pkg/registry/porch/packagerevisionresources.go +++ b/pkg/registry/porch/packagerevisionresources.go @@ -22,7 +22,6 @@ import ( api "github.com/nephio-project/porch/api/porch/v1alpha1" "github.com/nephio-project/porch/api/porchconfig/v1alpha1" - "github.com/nephio-project/porch/internal/kpt/errors" "github.com/nephio-project/porch/pkg/repository" "github.com/nephio-project/porch/pkg/util" "go.opentelemetry.io/otel/trace" @@ -184,11 +183,6 @@ func (r *packageRevisionResources) Update(ctx context.Context, name string, objI rev, renderStatus, err := r.cad.UpdatePackageResources(ctx, &repositoryObj, oldRepoPkgRev, oldApiPkgRevResources, newObj) if err != nil { - var specificErr *errors.Error - if errors.As(err, &specificErr) { - err := fmt.Errorf("%w%s%s%s", err, "\n\nKpt function pipeline error occurred during render.", "\nPackage has NOT been pushed to remote.", "\nAmend local package using error message above & retry.") - return nil, false, apierrors.NewInternalError(err) - } return nil, false, apierrors.NewInternalError(err) } diff --git a/pkg/task/generictaskhandler.go b/pkg/task/generictaskhandler.go index 9bfa37fb..cf41f2a4 100644 --- a/pkg/task/generictaskhandler.go +++ b/pkg/task/generictaskhandler.go @@ -462,6 +462,7 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, renderStatus = taskResult.RenderStatus } if err != nil { + err = fmt.Errorf("%w%s%s%s", err, "\n\nKpt function pipeline error occured during render.", "\nPackage has NOT been pushed to remote.", "\nAmend local package using error message above & retry.") return updatedResources, renderStatus, err } From dd6b9d96e3cab2e1b5bf79a9c83b36047a21c8d7 Mon Sep 17 00:00:00 2001 From: Catalin Stratulat Date: Wed, 27 Nov 2024 16:15:28 +0000 Subject: [PATCH 4/5] cleared up render comment as per suggestion and compressed render status error for log and console output --- pkg/task/generictaskhandler.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/task/generictaskhandler.go b/pkg/task/generictaskhandler.go index cf41f2a4..1d863d5b 100644 --- a/pkg/task/generictaskhandler.go +++ b/pkg/task/generictaskhandler.go @@ -225,15 +225,14 @@ func (th *genericTaskHandler) DoPRResourceMutations(ctx context.Context, pr2Upda var renderStatus *api.RenderStatus if len(appliedResources.Contents) > 0 { - // a porch package which fails render validation will no longer be accepted. - // render the package - // Render failure WILL fail the overall API operation. - // The render error and result is captured as part of renderStatus above - // and is returned in PackageRevisionResources API's status field. - // We do not push the package further to remote - // the users changes are captured on their local - // and can be amended using the error returned to properly validate/mutate the package before retrying - // we no longer suppress this err. + // Render the package + // Render failure will fail the overall API operation. + // The render error and result are captured as part of renderStatus above + // and are returned in the PackageRevisionResources API's status field. + // We do not push the package further to remote: + // the user's changes are captured on their local package, + // and can be amended using the error returned as a reference point to ensure + // the package renders properly, before retrying the push. _, renderStatus, err = applyResourceMutations(ctx, draft, appliedResources, @@ -242,7 +241,6 @@ func (th *genericTaskHandler) DoPRResourceMutations(ctx context.Context, pr2Upda runtime: th.runtime, }}) if err != nil { - klog.Errorf("Kpt function pipeline error occurred in render. Package has NOT been pushed to remote. Modify and Fix local package and retry.") return renderStatus, err } } else { @@ -462,7 +460,8 @@ func applyResourceMutations(ctx context.Context, draft repository.PackageDraft, renderStatus = taskResult.RenderStatus } if err != nil { - err = fmt.Errorf("%w%s%s%s", err, "\n\nKpt function pipeline error occured during render.", "\nPackage has NOT been pushed to remote.", "\nAmend local package using error message above & retry.") + klog.Error(err) + err = fmt.Errorf("%w\n\n%s\n%s\n%s", err, "Error occurred rendering package in kpt function pipeline.", "Package has NOT been pushed to remote.", "Please fix package locally (modify until 'kpt fn render' succeeds) and retry.") return updatedResources, renderStatus, err } From 563a39dc0569aba106b069a4247600a2e3258dcb Mon Sep 17 00:00:00 2001 From: Catalin Stratulat Date: Mon, 2 Dec 2024 09:25:40 +0000 Subject: [PATCH 5/5] added test cases for rpkg push not latest version & render failiure --- test/e2e/cli/config.go | 2 + test/e2e/cli/suite.go | 4 ++ test/e2e/cli/testdata/rpkg-push/config.yaml | 79 +++++++++++++++++++++ 3 files changed, 85 insertions(+) diff --git a/test/e2e/cli/config.go b/test/e2e/cli/config.go index 84aea6e9..689b51a9 100644 --- a/test/e2e/cli/config.go +++ b/test/e2e/cli/config.go @@ -38,6 +38,8 @@ type Command struct { Yaml bool `yaml:"yaml,omitempty"` // IgnoreWhitespace indicates that whitespace differences should be ignored in the output IgnoreWhitespace bool `yaml:"ignoreWhitespace,omitempty"` + // StdErrTabToWhitespace replaces "\t" (tab) character with whitespace " " + StdErrTabToWhitespace bool `yaml:"stdErrTabToWhitespace,omitempty"` } type TestCaseConfig struct { diff --git a/test/e2e/cli/suite.go b/test/e2e/cli/suite.go index 0960f55b..380320ee 100644 --- a/test/e2e/cli/suite.go +++ b/test/e2e/cli/suite.go @@ -148,6 +148,10 @@ func (s *CliTestSuite) RunTestCase(t *testing.T, tc TestCaseConfig) { command.Stdout = strings.ReplaceAll(command.Stdout, search, replace) command.Stderr = strings.ReplaceAll(command.Stderr, search, replace) } + + if command.StdErrTabToWhitespace { + stderrStr = strings.ReplaceAll(stderrStr, "\t", " ") // Replace tabs with spaces + } if command.IgnoreWhitespace { command.Stdout = normalizeWhitespace(command.Stdout) diff --git a/test/e2e/cli/testdata/rpkg-push/config.yaml b/test/e2e/cli/testdata/rpkg-push/config.yaml index 16f94c84..1fc7fe99 100644 --- a/test/e2e/cli/testdata/rpkg-push/config.yaml +++ b/test/e2e/cli/testdata/rpkg-push/config.yaml @@ -188,3 +188,82 @@ commands: name: kptfile.kpt.dev kind: ResourceList yaml: true + - args: + - porchctl + - rpkg + - push + - --namespace=rpkg-push + - git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f + stdin: | + apiVersion: config.kubernetes.io/v1 + items: + - apiVersion: "" + kind: KptRevisionMetadata + metadata: + annotations: + config.kubernetes.io/index: "0" + config.kubernetes.io/path: .KptRevisionMetadata + internal.config.kubernetes.io/index: "0" + internal.config.kubernetes.io/path: .KptRevisionMetadata + name: git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f + namespace: rpkg-push + uid: 8d61685e-584c-5b89-bd30-a8d8dccb13f9 + resourceVersion: "1" + - apiVersion: kpt.dev/v1 + info: + description: Updated Test Package Description + kind: Kptfile + metadata: + 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 + name: test-package + - 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 + kind: ResourceList + stderr: | + Error: Internal error occurred: Operation cannot be fulfilled on packagerevisionresources.porch.kpt.dev "git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f": the object has been modified; please apply your changes to the latest version and try again + exitCode: 1 + yaml: true + - args: + - porchctl + - rpkg + - pull + - --namespace=rpkg-push + - git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f + - /tmp/porch-e2e/testing-invalid-render + - args: + - sh + - -c + - | + echo "pipeline:\n mutators:\n - image: gcr.io/kpt-fn/set-namespace:v0.4.0\n configMap:\n namespace: example-ns\n - image: gcr.io/kpt-fn/set-annotations:v0.1.4\n" >> /tmp/porch-e2e/testing-invalid-render/Kptfile + - args: + - porchctl + - rpkg + - push + - --namespace=rpkg-push + - git-efe3d01c68dfdcdd69114c9a7c65cce0d662a46f + - /tmp/porch-e2e/testing-invalid-render + stderr: | + Error: Internal error occurred: fn.render: pkg /: + pkg.render: + pipeline.run: func eval "gcr.io/kpt-fn/set-annotations:v0.1.4" failed: rpc error: code = Internal desc = Failed to execute function "gcr.io/kpt-fn/set-annotations:v0.1.4": exit status 1 ([error] : failed to configure function: `functionConfig` must be a `ConfigMap` or `SetAnnotations`) + + Error occurred rendering package in kpt function pipeline. + Package has NOT been pushed to remote. + Please fix package locally (modify until 'kpt fn render' succeeds) and retry. + stdErrTabToWhitespace: true + exitCode: 1