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

@@ -109,6 +109,17 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req
 		)
 	}
 
+	// Validate BuildRun for disallowed field combinations (could technically be also done in a validating webhook)
+	if err := validate.BuildRunFields(buildRun); err != nil {
+		return reconcile.Result{}, resources.UpdateConditionWithFalseStatus(
+			ctx,
+			r.client,
+			buildRun,
+			err.Error(),
+			resources.BuildRunInvalid,
+		)
+	}
+
 	// 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
 	if getBuildRunErr == nil && apierrors.IsNotFound(getTaskRunErr) && buildRun.Status.LatestTaskRunRef != nil {
 		getTaskRunErr = r.client.Get(ctx, types.NamespacedName{Name: *buildRun.Status.LatestTaskRunRef, Namespace: request.Namespace}, lastTaskRun)
@@ -128,9 +139,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.NewCredentials(r.client, build),
+						validate.NewStrategies(r.client, build),
+						validate.NewSourceURL(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 {

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,191 @@ 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 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.BuildRunInvalid))
+						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.BuildRunInvalid))
+						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.BuildRunInvalid))
+						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.BuildRunInvalid))
+						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.BuildRunInvalid))
+						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{

pkg/reconciler/buildrun/resources/conditions.go

@@ -41,6 +41,7 @@ const (
 	ConditionIncompleteConfigMapValueParameterValues string = "IncompleteConfigMapValueParameterValues"
 	ConditionIncompleteSecretValueParameterValues    string = "IncompleteSecretValueParameterValues"
 	BuildRunNameInvalid                              string = "BuildRunNameInvalid"
+	BuildRunInvalid                                  string = "BuildRunInvalid"
 )
 
 // UpdateBuildRunUsingTaskRunCondition updates the BuildRun Succeeded Condition

pkg/validate/validate.go

@@ -68,7 +68,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 +78,31 @@ 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) error {
+	if buildRun.Spec.BuildSpec != nil {
+		if buildRun.Spec.BuildRef != nil {
+			return fmt.Errorf("fields 'buildRef' and 'buildSpec' are mutually exclusive")
+		}
+
+		if buildRun.Spec.Output != nil {
+			return fmt.Errorf("cannot use 'output' override and 'buildSpec' simultaneously")
+		}
+
+		if len(buildRun.Spec.ParamValues) > 0 {
+			return fmt.Errorf("cannot use 'paramValues' override and 'buildSpec' simultaneously")
+		}
+
+		if len(buildRun.Spec.Env) > 0 {
+			return fmt.Errorf("cannot use 'env' override and 'buildSpec' simultaneously")
+		}
+
+		if buildRun.Spec.Timeout != nil {
+			return fmt.Errorf("cannot use 'timeout' override and 'buildSpec' simultaneously")
+		}
+	}
+
+	return nil
+}