Tests for Tolerations on Build and BuildRun objects

Signed-off-by: Dylan Orzel <[email protected]>

pkg/reconciler/build/build_test.go

@@ -16,6 +16,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/utils/ptr"
 	crc "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -621,7 +622,22 @@ var _ = Describe("Reconcile Build", func() {
 				buildSample.Spec.NodeSelector = map[string]string{strings.Repeat("s", 64): "amd64"}
 				buildSample.Spec.Output.PushSecret = nil
 
-				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.NodeSelectorNotValid, "name part must be no more than 63 characters")
+				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.NodeSelectorNotValid, "name part "+validation.MaxLenError(63))
+				statusWriter.UpdateCalls(statusCall)
+
+				_, err := reconciler.Reconcile(context.TODO(), request)
+				Expect(err).To(BeNil())
+				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
+			})
+		})
+
+		Context("when Tolerations is specified", func() {
+			It("should fail to validate when the Toleration is invalid", func() {
+				// set Toleration to be invalid
+				buildSample.Spec.Tolerations = []corev1.Toleration{{Key: strings.Repeat("s", 64), Operator: "Equal", Value: "test-value"}}
+				buildSample.Spec.Output.PushSecret = nil
+
+				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.TolerationNotValid, "name part "+validation.MaxLenError(63))
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(context.TODO(), request)

pkg/reconciler/buildrun/buildrun.go

@@ -292,6 +292,24 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req
 				return reconcile.Result{}, nil
 			}
 
+			// Validate the nodeSelector
+			valid, reason, message = validate.BuildRunNodeSelector(buildRun.Spec.NodeSelector)
+			if !valid {
+				if err := resources.UpdateConditionWithFalseStatus(ctx, r.client, buildRun, message, reason); err != nil {
+					return reconcile.Result{}, err
+				}
+				return reconcile.Result{}, nil
+			}
+
+			// Validate the tolerations
+			valid, reason, message = validate.BuildRunTolerations(buildRun.Spec.Tolerations)
+			if !valid {
+				if err := resources.UpdateConditionWithFalseStatus(ctx, r.client, buildRun, message, reason); err != nil {
+					return reconcile.Result{}, err
+				}
+				return reconcile.Result{}, nil
+			}
+
 			// Create the TaskRun, this needs to be the last step in this block to be idempotent
 			generatedTaskRun, err := r.createTaskRun(ctx, svcAccount, strategy, build, buildRun)
 			if err != nil {

pkg/reconciler/buildrun/buildrun_test.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/utils/ptr"
 	knativeapi "knative.dev/pkg/apis"
@@ -1635,9 +1636,23 @@ var _ = Describe("Reconcile BuildRun", func() {
 		Context("when nodeSelector is specified", func() {
 			It("fails when the nodeSelector is invalid", func() {
 				// set nodeSelector to be invalid
-				buildSample.Spec.NodeSelector = map[string]string{strings.Repeat("s", 64): "amd64"}
+				buildRunSample.Spec.NodeSelector = map[string]string{strings.Repeat("s", 64): "amd64"}
 
-				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.NodeSelectorNotValid, "must be no more than 63 characters")
+				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.NodeSelectorNotValid, "name part "+validation.MaxLenError(63))
+				statusWriter.UpdateCalls(statusCall)
+
+				_, err := reconciler.Reconcile(context.TODO(), buildRunRequest)
+				Expect(err).To(BeNil())
+				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
+			})
+		})
+
+		Context("when Tolerations is specified", func() {
+			It("should fail to validate when the Toleration is invalid", func() {
+				// set Toleration to be invalid
+				buildRunSample.Spec.Tolerations = []corev1.Toleration{{Key: strings.Repeat("s", 64), Operator: "Equal", Value: "test-value"}}
+
+				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.TolerationNotValid, validation.MaxLenError(63))
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(context.TODO(), buildRunRequest)

pkg/reconciler/buildrun/resources/build.go

@@ -40,10 +40,6 @@ func GetBuildObject(ctx context.Context, client client.Client, buildRun *buildv1
 		build.Namespace = buildRun.Namespace
 		build.Status = buildv1beta1.BuildStatus{}
 		buildRun.Spec.Build.Spec.DeepCopyInto(&build.Spec)
-		// In this case, fields set only on the BuildRun object do not get validated as they are not copied to the transient Build resource.
-		// explicitly setting them here is required for validation to happen.
-		build.Spec.NodeSelector = buildRun.Spec.NodeSelector
-		build.Spec.Tolerations = buildRun.Spec.Tolerations
 		return nil
 	}
 

pkg/reconciler/buildrun/resources/taskrun_test.go

@@ -635,7 +635,7 @@ var _ = Describe("GenerateTaskrun", func() {
 
 		Context("when the build and buildrun both specify a nodeSelector", func() {
 			BeforeEach(func() {
-				build, err = ctl.LoadBuildYAML([]byte(test.MinimalBuildRunWithNodeSelector))
+				build, err = ctl.LoadBuildYAML([]byte(test.MinimalBuildWithNodeSelector))
 				Expect(err).To(BeNil())
 
 				buildRun, err = ctl.LoadBuildRunFromBytes([]byte(test.MinimalBuildRunWithNodeSelector))
@@ -654,5 +654,29 @@ var _ = Describe("GenerateTaskrun", func() {
 				Expect(got.Spec.PodTemplate.NodeSelector).To(Equal(buildRun.Spec.NodeSelector))
 			})
 		})
+
+		Context("when the build and buildrun both specify a Toleration", func() {
+			BeforeEach(func() {
+				build, err = ctl.LoadBuildYAML([]byte(test.MinimalBuildWithToleration))
+				Expect(err).To(BeNil())
+
+				buildRun, err = ctl.LoadBuildRunFromBytes([]byte(test.MinimalBuildRunWithToleration))
+				Expect(err).To(BeNil())
+
+				buildStrategy, err = ctl.LoadBuildStrategyFromBytes([]byte(test.ClusterBuildStrategyNoOp))
+				Expect(err).To(BeNil())
+			})
+
+			JustBeforeEach(func() {
+				got, err = resources.GenerateTaskRun(config.NewDefaultConfig(), build, buildRun, serviceAccountName, buildStrategy)
+				Expect(err).To(BeNil())
+			})
+
+			It("should give precedence to the Toleration values specified in the buildRun", func() {
+				Expect(got.Spec.PodTemplate.Tolerations[0].Key).To(Equal(buildRun.Spec.Tolerations[0].Key))
+				Expect(got.Spec.PodTemplate.Tolerations[0].Operator).To(Equal(buildRun.Spec.Tolerations[0].Operator))
+				Expect(got.Spec.PodTemplate.Tolerations[0].Value).To(Equal(buildRun.Spec.Tolerations[0].Value))
+			})
+		})
 	})
 })

pkg/validate/nodeselector.go

@@ -6,6 +6,7 @@ package validate
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -30,13 +31,26 @@ func (b *NodeSelectorRef) ValidatePath(_ context.Context) error {
 	for key, value := range b.Build.Spec.NodeSelector {
 		if errs := validation.IsQualifiedName(key); len(errs) > 0 {
 			b.Build.Status.Reason = ptr.To(build.NodeSelectorNotValid)
-			b.Build.Status.Message = ptr.To(strings.Join(errs, ", "))
+			b.Build.Status.Message = ptr.To(fmt.Sprintf("Node selector key not valid: %v", strings.Join(errs, ", ")))
 		}
 		if errs := validation.IsValidLabelValue(value); len(errs) > 0 {
 			b.Build.Status.Reason = ptr.To(build.NodeSelectorNotValid)
-			b.Build.Status.Message = ptr.To(strings.Join(errs, ", "))
+			b.Build.Status.Message = ptr.To(fmt.Sprintf("Node selector value not valid: %v", strings.Join(errs, ", ")))
 		}
 	}
 
 	return nil
 }
+
+// BuildRunNodeSelector is used to validate nodeSelectors in the BuildRun object
+func BuildRunNodeSelector(nodeSelector map[string]string) (bool, string, string) {
+	for key, value := range nodeSelector {
+		if errs := validation.IsQualifiedName(key); len(errs) > 0 {
+			return false, string(build.NodeSelectorNotValid), fmt.Sprintf("Node selector key not valid: %v", strings.Join(errs, ", "))
+		}
+		if errs := validation.IsValidLabelValue(value); len(errs) > 0 {
+			return false, string(build.NodeSelectorNotValid), fmt.Sprintf("Node selector value not valid: %v", strings.Join(errs, ", "))
+		}
+	}
+	return true, "", ""
+}

pkg/validate/tolerations.go

@@ -33,7 +33,7 @@ func (b *TolerationsRef) ValidatePath(_ context.Context) error {
 		// validate Key
 		if errs := validation.IsQualifiedName(toleration.Key); errs != nil {
 			b.Build.Status.Reason = ptr.To(build.TolerationNotValid)
-			b.Build.Status.Message = ptr.To(strings.Join(errs, ", "))
+			b.Build.Status.Message = ptr.To(fmt.Sprintf("Toleration key not valid: %v", strings.Join(errs, ", ")))
 		}
 		// validate Operator
 		if !((toleration.Operator == v1.TolerationOpExists) || (toleration.Operator == v1.TolerationOpEqual)) {
@@ -43,7 +43,7 @@ func (b *TolerationsRef) ValidatePath(_ context.Context) error {
 		// validate Value
 		if errs := validation.IsValidLabelValue(toleration.Value); errs != nil {
 			b.Build.Status.Reason = ptr.To(build.TolerationNotValid)
-			b.Build.Status.Message = ptr.To(strings.Join(errs, ", "))
+			b.Build.Status.Message = ptr.To(fmt.Sprintf("Toleration value not valid: %v", strings.Join(errs, ", ")))
 		}
 		// validate Taint Effect, of which only "NoSchedule" is supported
 		if !((toleration.Effect) == "" || (toleration.Effect == v1.TaintEffectNoSchedule)) {
@@ -59,3 +59,30 @@ func (b *TolerationsRef) ValidatePath(_ context.Context) error {
 
 	return nil
 }
+
+// BuildRunTolerations is used to validate tolerations in the BuildRun object
+func BuildRunTolerations(tolerations []v1.Toleration) (bool, string, string) {
+	for _, toleration := range tolerations {
+		// validate Key
+		if errs := validation.IsQualifiedName(toleration.Key); errs != nil {
+			return false, string(build.TolerationNotValid), fmt.Sprintf("Toleration key not valid: %v", strings.Join(errs, ", "))
+		}
+		// validate Operator
+		if !((toleration.Operator == v1.TolerationOpExists) || (toleration.Operator == v1.TolerationOpEqual)) {
+			return false, string(build.TolerationNotValid), fmt.Sprintf("Toleration operator not valid. Must be one of: '%v', '%v'", v1.TolerationOpExists, v1.TolerationOpEqual)
+		}
+		// validate Value
+		if errs := validation.IsValidLabelValue(toleration.Value); errs != nil {
+			return false, string(build.TolerationNotValid), fmt.Sprintf("Toleration value not valid: %v", strings.Join(errs, ", "))
+		}
+		// validate Taint Effect, of which only "NoSchedule" is supported
+		if !((toleration.Effect) == "" || (toleration.Effect == v1.TaintEffectNoSchedule)) {
+			return false, string(build.TolerationNotValid), fmt.Sprintf("Only the '%v' toleration effect is supported.", v1.TaintEffectNoSchedule)
+		}
+		// validate TolerationSeconds, which should not be specified
+		if toleration.TolerationSeconds != nil {
+			return false, string(build.TolerationNotValid), "Specifying TolerationSeconds is not supported."
+		}
+	}
+	return true, "", ""
+}

pkg/validate/validate.go

@@ -137,6 +137,16 @@ func BuildRunFields(buildRun *build.BuildRun) (string, string) {
 			return resources.BuildRunBuildFieldOverrideForbidden,
 				"cannot use 'triggers' override in the 'BuildRun', only allowed in the 'Build'"
 		}
+
+		if len(buildRun.Spec.NodeSelector) > 0 {
+			return resources.BuildRunBuildFieldOverrideForbidden,
+				"cannot use 'nodeSelector' override and 'buildSpec' simultaneously"
+		}
+
+		if len(buildRun.Spec.Tolerations) > 0 {
+			return resources.BuildRunBuildFieldOverrideForbidden,
+				"cannot use 'tolerations' override and 'buildSpec' simultaneously"
+		}
 	}
 
 	return "", ""

test/data/v1beta1/build_buildah_tolerations_cr.yaml

@@ -0,0 +1,20 @@
+---
+apiVersion: shipwright.io/v1beta1
+kind: Build
+metadata:
+  name: buildah-tolerations-build
+spec:
+  source:
+    type: Git
+    git:
+      url: https://github.com/shipwright-io/sample-go
+    contextDir: docker-build
+  strategy:
+    name: buildah-shipwright-managed-push
+    kind: ClusterBuildStrategy
+  output:
+    image: image-registry.openshift-image-registry.svc:5000/build-examples/taxi-app
+  tolerations:
+    - key: "test-key"
+      value: "test-value"
+      operator: "Equal"

test/data/v1beta1/buildrun_buildah_tolerations_cr.yaml

@@ -0,0 +1,8 @@
+---
+apiVersion: shipwright.io/v1beta1
+kind: BuildRun
+metadata:
+  name: buildah-tolerations-buildrun
+spec:
+  build:
+    name: buildah-tolerations-build

test/e2e/v1beta1/e2e_test.go

@@ -6,6 +6,7 @@ package e2e_test
 
 import (
 	"os"
+	"time"
 
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	. "github.com/onsi/ginkgo/v2"
@@ -15,6 +16,9 @@ import (
 
 	buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1"
 	shpgit "github.com/shipwright-io/build/pkg/git"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	corev1 "k8s.io/api/core/v1"
 )
 
 var _ = Describe("For a Kubernetes cluster with Tekton and build installed", func() {
@@ -706,4 +710,64 @@ var _ = Describe("For a Kubernetes cluster with Tekton and build installed", fun
 		})
 	})
 
+	Context("when tolerations are specified", Serial, func() {
+
+		BeforeEach(func() {
+			testID = generateTestID("tolerations")
+
+			// create the build definition
+			build = createBuild(
+				testBuild,
+				testID,
+				"test/data/v1beta1/build_buildah_tolerations_cr.yaml",
+			)
+
+			// Add a taint to both of the kind worker nodes
+			nodes, err := testBuild.GetNodes()
+			Expect(err).ToNot(HaveOccurred())
+			for _, node := range nodes.Items {
+				if node.Name == "kind-worker" || node.Name == "kind-worker2" {
+					taint := corev1.Taint{Key: "test-key", Value: "test-value", Effect: "NoSchedule"}
+					err := testBuild.AddNodeTaint(node.Name, &taint)
+					Expect(err).ToNot(HaveOccurred())
+				}
+			}
+		})
+
+		AfterEach(func() {
+			nodes, err := testBuild.GetNodes()
+			Expect(err).ToNot(HaveOccurred())
+			for _, node := range nodes.Items {
+				if node.Name == "kind-worker" || node.Name == "kind-worker2" {
+					err := testBuild.RemoveNodeTaints(node.Name)
+					Expect(err).ToNot(HaveOccurred())
+				}
+			}
+		})
+
+		It("successfully runs a build when it tolerates the node taint", func() {
+			buildRun, err = buildRunTestData(testBuild.Namespace, testID, "test/data/v1beta1/buildrun_buildah_tolerations_cr.yaml")
+			Expect(err).ToNot(HaveOccurred(), "Error retrieving buildrun test data")
+
+			buildRun = validateBuildRunToSucceed(testBuild, buildRun)
+		})
+
+		It("fails to schedule when the build does not tolerate the node taint", func() {
+			// set untolerated value and a low timeout since we do not expect this to be scheduled
+			build.Spec.Tolerations[0].Value = "untolerated"
+			build.Spec.Timeout = &metav1.Duration{Duration: time.Minute}
+			testBuild.UpdateBuild(build)
+
+			buildRun, err = buildRunTestData(testBuild.Namespace, testID, "test/data/v1beta1/buildrun_buildah_tolerations_cr.yaml")
+			Expect(err).ToNot(HaveOccurred(), "Error retrieving buildrun test data")
+
+			validateBuildRunToFail(testBuild, buildRun)
+			buildRun, err = testBuild.LookupBuildRun(types.NamespacedName{Name: buildRun.Name, Namespace: testBuild.Namespace})
+
+			// Pod should fail to schedule and the BuildRun should timeout.
+			Expect(buildRun.Status.GetCondition(buildv1beta1.Succeeded).Status).To(Equal(corev1.ConditionFalse))
+			Expect(buildRun.Status.GetCondition(buildv1beta1.Succeeded).Reason).To(Equal("BuildRunTimeout"))
+			Expect(buildRun.Status.GetCondition(buildv1beta1.Succeeded).Message).To(ContainSubstring("failed to finish within"))
+		})
+	})
 })