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

@@ -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 {

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:/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,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

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 "", ""
+}