From 6663ff0afe6ffc994905e0a0cbe9bc6e35a5c80d Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Sun, 20 Mar 2022 23:16:21 +0100 Subject: [PATCH] Add embedded Build validation and test cases Add E2E test cases for embedded builds in buildruns. Introduce validation code for field permutations. Add test cases for field validation. --- pkg/reconciler/buildrun/buildrun.go | 77 +++++-- pkg/reconciler/buildrun/buildrun_test.go | 201 ++++++++++++++++++ .../buildrun/resources/conditions.go | 3 + pkg/validate/validate.go | 42 +++- test/e2e/e2e_one_off_builds_test.go | 110 ++++++++++ 5 files changed, 420 insertions(+), 13 deletions(-) create mode 100644 test/e2e/e2e_one_off_builds_test.go diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go index 00211d52ab..f7090b603b 100644 --- a/pkg/reconciler/buildrun/buildrun.go +++ b/pkg/reconciler/buildrun/buildrun.go @@ -97,16 +97,30 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req return reconcile.Result{}, nil } - // Validating buildrun name is a valid label value - if errs := validation.IsValidLabelValue(buildRun.Name); len(errs) > 0 { - // stop reconciling and mark the BuildRun as Failed - return reconcile.Result{}, resources.UpdateConditionWithFalseStatus( - ctx, - r.client, - buildRun, - strings.Join(errs, ", "), - resources.BuildRunNameInvalid, - ) + // Skip validation in case buildrun could not be found, otherwise validate it + if getBuildRunErr == nil { + // Validating buildrun name is a valid label value + if errs := validation.IsValidLabelValue(buildRun.Name); len(errs) > 0 { + // stop reconciling and mark the BuildRun as Failed + return reconcile.Result{}, resources.UpdateConditionWithFalseStatus( + ctx, + r.client, + buildRun, + strings.Join(errs, ", "), + resources.BuildRunNameInvalid, + ) + } + + // Validate BuildRun for disallowed field combinations (could technically be also done in a validating webhook) + if reason, message := validate.BuildRunFields(buildRun); reason != "" { + return reconcile.Result{}, resources.UpdateConditionWithFalseStatus( + ctx, + r.client, + buildRun, + message, + reason, + ) + } } // if this is a build run event after we've set the task run ref, get the task run using the task run name stored in the build run @@ -128,9 +142,48 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req // Validate if the Build was successfully registered if build.Status.Registered == nil || *build.Status.Registered == "" { - err := fmt.Errorf("the Build is not yet validated, build: %s", build.Name) + switch { + // When the build is referenced by name, it means the build is + // an actual resource in the cluster and _should_ have been + // validated and registered by now ... // reconcile again until it gets a registration value - return reconcile.Result{}, err + case buildRun.Spec.BuildRef != nil: + return reconcile.Result{}, fmt.Errorf("the Build is not yet validated, build: %s", build.Name) + + // When the build(spec) is embedded in the buildrun, the now + // transient/volatile build resource needs to be validated first + case buildRun.Spec.BuildSpec != nil: + err := validate.All(ctx, + validate.NewSourceURL(r.client, build), + validate.NewCredentials(r.client, build), + validate.NewStrategies(r.client, build), + validate.NewSourcesRef(build), + validate.NewBuildName(build), + validate.NewEnv(build), + ) + + // an internal/technical error during validation happened + if err != nil { + return reconcile.Result{}, err + } + + // one or more of the validations failed + if build.Status.Reason != nil { + return reconcile.Result{}, + resources.UpdateConditionWithFalseStatus( + ctx, + r.client, + buildRun, + *build.Status.Message, + resources.ConditionBuildRegistrationFailed, + ) + } + + // mark transient build as "registered" and validated + build.Status.Registered = buildv1alpha1.ConditionStatusPtr(corev1.ConditionTrue) + build.Status.Reason = buildv1alpha1.BuildReasonPtr(buildv1alpha1.SucceedStatus) + build.Status.Message = pointer.String(buildv1alpha1.AllValidationsSucceeded) + } } if *build.Status.Registered != corev1.ConditionTrue { diff --git a/pkg/reconciler/buildrun/buildrun_test.go b/pkg/reconciler/buildrun/buildrun_test.go index a00e969b85..358b121a90 100644 --- a/pkg/reconciler/buildrun/buildrun_test.go +++ b/pkg/reconciler/buildrun/buildrun_test.go @@ -7,10 +7,12 @@ package buildrun_test import ( "context" "fmt" + "strings" "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + v1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -1266,6 +1268,205 @@ var _ = Describe("Reconcile BuildRun", func() { }) }) + Context("from an existing BuildRun resource with embedded BuildSpec", func() { + var clusterBuildStrategy = build.ClusterBuildStrategyKind + + BeforeEach(func() { + buildRunRequest = newReconcileRequest(buildRunName, ns) + }) + + Context("invalid BuildRun resource", func() { + simpleReconcileRunWithCustomUpdateCall := func(f func(*build.Condition)) { + client.GetCalls(ctl.StubBuildRun(buildRunSample)) + statusWriter.UpdateCalls(func(_ context.Context, o crc.Object, _ ...crc.UpdateOption) error { + Expect(o).To(BeAssignableToTypeOf(&build.BuildRun{})) + switch buildRun := o.(type) { + case *build.BuildRun: + f(buildRun.Status.GetCondition(build.Succeeded)) + } + + return nil + }) + + var taskRunCreates int + client.CreateCalls(func(_ context.Context, o crc.Object, _ ...crc.CreateOption) error { + switch o.(type) { + case *v1beta1.TaskRun: + taskRunCreates++ + } + + return nil + }) + + // Reconcile should run through without an error + _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) + Expect(err).To(BeNil()) + + // But, make sure no TaskRun is created based upon an invalid BuildRun + Expect(taskRunCreates).To(Equal(0)) + } + + It("should mark BuildRun as invalid if both BuildRef and BuildSpec are unspecified", func() { + buildRunSample = &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + }, + Spec: build.BuildRunSpec{}, + } + + simpleReconcileRunWithCustomUpdateCall(func(condition *build.Condition) { + Expect(condition.Reason).To(Equal(resources.BuildRunNoRefOrSpec)) + Expect(condition.Message).To(Equal("no build referenced or specified, either 'buildRef' or 'buildSpec' has to be set")) + }) + }) + + It("should mark BuildRun as invalid if BuildRef and BuildSpec are used", func() { + buildRunSample = &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + }, + Spec: build.BuildRunSpec{ + BuildRef: &build.BuildRef{}, + BuildSpec: &build.BuildSpec{}, + }, + } + + simpleReconcileRunWithCustomUpdateCall(func(condition *build.Condition) { + Expect(condition.Reason).To(Equal(resources.BuildRunAmbiguousBuild)) + Expect(condition.Message).To(Equal("fields 'buildRef' and 'buildSpec' are mutually exclusive")) + }) + }) + + It("should mark BuildRun as invalid if Output and BuildSpec are used", func() { + buildRunSample = &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + }, + Spec: build.BuildRunSpec{ + Output: &build.Image{Image: "foo:bar"}, + BuildSpec: &build.BuildSpec{}, + }, + } + + simpleReconcileRunWithCustomUpdateCall(func(condition *build.Condition) { + Expect(condition.Reason).To(Equal(resources.BuildRunBuildFieldOverrideForbidden)) + Expect(condition.Message).To(Equal("cannot use 'output' override and 'buildSpec' simultaneously")) + }) + }) + + It("should mark BuildRun as invalid if ParamValues and BuildSpec are used", func() { + buildRunSample = &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + }, + Spec: build.BuildRunSpec{ + ParamValues: []build.ParamValue{{ + Name: "foo", + SingleValue: &build.SingleValue{Value: pointer.String("bar")}, + }}, + BuildSpec: &build.BuildSpec{}, + }, + } + + simpleReconcileRunWithCustomUpdateCall(func(condition *build.Condition) { + Expect(condition.Reason).To(Equal(resources.BuildRunBuildFieldOverrideForbidden)) + Expect(condition.Message).To(Equal("cannot use 'paramValues' override and 'buildSpec' simultaneously")) + }) + }) + + It("should mark BuildRun as invalid if Env and BuildSpec are used", func() { + buildRunSample = &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + }, + Spec: build.BuildRunSpec{ + Env: []corev1.EnvVar{{Name: "foo", Value: "bar"}}, + BuildSpec: &build.BuildSpec{}, + }, + } + + simpleReconcileRunWithCustomUpdateCall(func(condition *build.Condition) { + Expect(condition.Reason).To(Equal(resources.BuildRunBuildFieldOverrideForbidden)) + Expect(condition.Message).To(Equal("cannot use 'env' override and 'buildSpec' simultaneously")) + }) + }) + + It("should mark BuildRun as invalid if Timeout and BuildSpec are used", func() { + buildRunSample = &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + }, + Spec: build.BuildRunSpec{ + Timeout: &metav1.Duration{Duration: time.Second}, + BuildSpec: &build.BuildSpec{}, + }, + } + + simpleReconcileRunWithCustomUpdateCall(func(condition *build.Condition) { + Expect(condition.Reason).To(Equal(resources.BuildRunBuildFieldOverrideForbidden)) + Expect(condition.Message).To(Equal("cannot use 'timeout' override and 'buildSpec' simultaneously")) + }) + }) + }) + + Context("valid BuildRun resource", func() { + It("should reconcile a BuildRun with an embedded BuildSpec", func() { + buildRunSample = &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + }, + Spec: build.BuildRunSpec{ + BuildSpec: &build.BuildSpec{ + Source: build.Source{ + URL: pointer.String("https://github.com/shipwright-io/sample-go.git"), + ContextDir: pointer.String("source-build"), + }, + Strategy: build.Strategy{ + Kind: &clusterBuildStrategy, + Name: strategyName, + }, + Output: build.Image{ + Image: "foo/bar:latest", + }, + }, + ServiceAccount: &build.ServiceAccount{ + Generate: pointer.Bool(true), + }, + }, + } + + client.GetCalls(func(_ context.Context, nn types.NamespacedName, o crc.Object) error { + switch object := o.(type) { + case *build.BuildRun: + buildRunSample.DeepCopyInto(object) + return nil + + case *build.ClusterBuildStrategy: + ctl.ClusterBuildStrategy(strategyName).DeepCopyInto(object) + return nil + } + + return k8serrors.NewNotFound(schema.GroupResource{}, nn.Name) + }) + + var taskRunCreates int + client.CreateCalls(func(_ context.Context, o crc.Object, _ ...crc.CreateOption) error { + switch o.(type) { + case *v1beta1.TaskRun: + taskRunCreates++ + } + + return nil + }) + + _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) + Expect(err).To(BeNil()) + + Expect(taskRunCreates).To(Equal(1)) + }) + }) + }) + Context("when environment variables are specified", func() { It("fails when the name is blank", func() { buildRunSample.Spec.Env = []corev1.EnvVar{ diff --git a/pkg/reconciler/buildrun/resources/conditions.go b/pkg/reconciler/buildrun/resources/conditions.go index a8ca482c70..602dd55f8e 100644 --- a/pkg/reconciler/buildrun/resources/conditions.go +++ b/pkg/reconciler/buildrun/resources/conditions.go @@ -41,6 +41,9 @@ const ( ConditionIncompleteConfigMapValueParameterValues string = "IncompleteConfigMapValueParameterValues" ConditionIncompleteSecretValueParameterValues string = "IncompleteSecretValueParameterValues" BuildRunNameInvalid string = "BuildRunNameInvalid" + BuildRunNoRefOrSpec string = "BuildRunNoRefOrSpec" + BuildRunAmbiguousBuild string = "BuildRunAmbiguousBuild" + BuildRunBuildFieldOverrideForbidden string = "BuildRunBuildFieldOverrideForbidden" ) // UpdateBuildRunUsingTaskRunCondition updates the BuildRun Succeeded Condition diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index a36748829d..566c6c0c2f 100644 --- a/pkg/validate/validate.go +++ b/pkg/validate/validate.go @@ -12,6 +12,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" + "github.com/shipwright-io/build/pkg/reconciler/buildrun/resources" ) const ( @@ -68,7 +69,8 @@ func NewValidation( } } -func Build(ctx context.Context, validations ...BuildPath) error { +// All runs all given validations and exists at the first technical error +func All(ctx context.Context, validations ...BuildPath) error { for _, validatation := range validations { if err := validatation.ValidatePath(ctx); err != nil { return err @@ -77,3 +79,41 @@ func Build(ctx context.Context, validations ...BuildPath) error { return nil } + +// BuildRunFields runs field validations against a BuildRun to detect +// disallowed field combinations +func BuildRunFields(buildRun *build.BuildRun) (string, string) { + if buildRun.Spec.BuildSpec == nil && buildRun.Spec.BuildRef == nil { + return resources.BuildRunNoRefOrSpec, + "no build referenced or specified, either 'buildRef' or 'buildSpec' has to be set" + } + + if buildRun.Spec.BuildSpec != nil { + if buildRun.Spec.BuildRef != nil { + return resources.BuildRunAmbiguousBuild, + "fields 'buildRef' and 'buildSpec' are mutually exclusive" + } + + if buildRun.Spec.Output != nil { + return resources.BuildRunBuildFieldOverrideForbidden, + "cannot use 'output' override and 'buildSpec' simultaneously" + } + + if len(buildRun.Spec.ParamValues) > 0 { + return resources.BuildRunBuildFieldOverrideForbidden, + "cannot use 'paramValues' override and 'buildSpec' simultaneously" + } + + if len(buildRun.Spec.Env) > 0 { + return resources.BuildRunBuildFieldOverrideForbidden, + "cannot use 'env' override and 'buildSpec' simultaneously" + } + + if buildRun.Spec.Timeout != nil { + return resources.BuildRunBuildFieldOverrideForbidden, + "cannot use 'timeout' override and 'buildSpec' simultaneously" + } + } + + return "", "" +} diff --git a/test/e2e/e2e_one_off_builds_test.go b/test/e2e/e2e_one_off_builds_test.go new file mode 100644 index 0000000000..aa3e33369f --- /dev/null +++ b/test/e2e/e2e_one_off_builds_test.go @@ -0,0 +1,110 @@ +// Copyright The Shipwright Contributors +// +// SPDX-License-Identifier: Apache-2.0 + +package e2e_test + +import ( + "fmt" + "os" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/google/go-containerregistry/pkg/name" + buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" +) + +var _ = Describe("Using One-Off Builds", func() { + var ( + testID string + err error + + buildRun *buildv1alpha1.BuildRun + ) + + AfterEach(func() { + if CurrentGinkgoTestDescription().Failed { + printTestFailureDebugInfo(testBuild, testBuild.Namespace, testID) + + } else if buildRun != nil { + validateServiceAccountDeletion(buildRun, testBuild.Namespace) + } + + if buildRun != nil { + testBuild.DeleteBR(buildRun.Name) + buildRun = nil + } + }) + + Context("Embed BuildSpec in BuildRun", func() { + var outputImage name.Reference + + BeforeEach(func() { + testID = generateTestID("onoff") + + outputImage, err = name.ParseReference(fmt.Sprintf("%s/%s:%s", + os.Getenv(EnvVarImageRepo), + testID, + "latest", + )) + Expect(err).ToNot(HaveOccurred()) + }) + + It("should build an image using Buildpacks and a Git source", func() { + buildRun, err = NewBuildRunPrototype(). + Namespace(testBuild.Namespace). + Name(testID). + WithBuildSpec(NewBuildPrototype(). + ClusterBuildStrategy("buildpacks-v3"). + Namespace(testBuild.Namespace). + Name(testID). + SourceGit("https://github.com/shipwright-io/sample-go.git"). + SourceContextDir("source-build"). + OutputImage(outputImage.String()). + OutputImageCredentials(os.Getenv(EnvVarImageRepoSecret)). + BuildSpec()). + Create() + Expect(err).ToNot(HaveOccurred()) + validateBuildRunToSucceed(testBuild, buildRun) + }) + + It("should build an image using Buildah and a Git source", func() { + buildRun, err = NewBuildRunPrototype(). + Namespace(testBuild.Namespace). + Name(testID). + WithBuildSpec(NewBuildPrototype(). + ClusterBuildStrategy("buildah"). + Namespace(testBuild.Namespace). + Name(testID). + SourceGit("https://github.com/shipwright-io/sample-go.git"). + SourceContextDir("docker-build"). + Dockerfile("Dockerfile"). + ArrayParamValue("registries-insecure", outputImage.Context().RegistryStr()). + OutputImage(outputImage.String()). + OutputImageCredentials(os.Getenv(EnvVarImageRepoSecret)). + BuildSpec()). + Create() + Expect(err).ToNot(HaveOccurred()) + validateBuildRunToSucceed(testBuild, buildRun) + }) + + It("should build an image using Buildpacks and a bundle source", func() { + buildRun, err = NewBuildRunPrototype(). + Namespace(testBuild.Namespace). + Name(testID). + WithBuildSpec(NewBuildPrototype(). + ClusterBuildStrategy("buildpacks-v3"). + Namespace(testBuild.Namespace). + Name(testID). + SourceBundle("ghcr.io/shipwright-io/sample-go/source-bundle:latest"). + SourceContextDir("source-build"). + OutputImage(outputImage.String()). + OutputImageCredentials(os.Getenv(EnvVarImageRepoSecret)). + BuildSpec()). + Create() + Expect(err).ToNot(HaveOccurred()) + validateBuildRunToSucceed(testBuild, buildRun) + }) + }) +})