diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index faa82f92cb..ac5a41d98d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -171,7 +171,7 @@ jobs: version: v0.24.0 node_image: kindest/node:${{ matrix.kubernetes }} cluster_name: kind - config: test/kind/config.yaml + config: test/kind/config_three_node.yaml wait: 120s - name: Verify kind cluster run: | @@ -180,18 +180,21 @@ jobs: echo "# KinD nodes:" kubectl get nodes - NODE_STATUS=$(kubectl get node kind-control-plane -o json | jq -r .'status.conditions[] | select(.type == "Ready") | .status') - if [ "${NODE_STATUS}" != "True" ]; then - echo "# Node is not ready:" - kubectl describe node kind-control-plane + for nodename in $(kubectl get nodes -o name); do + kubectl wait --for=condition=Ready=true ${nodename} --timeout=60s + NODE_STATUS=$(kubectl get ${nodename} -o json | jq -r .'status.conditions[] | select(.type == "Ready") | .status') + if [ "${NODE_STATUS}" != "True" ]; then + echo "# Node is not ready:" + kubectl describe ${nodename} - echo "# Pods:" - kubectl get pod -A - echo "# Events:" - kubectl get events -A + echo "# Pods:" + kubectl get pod -A + echo "# Events:" + kubectl get events -A - exit 1 - fi + exit 1 + fi + done - name: Install Tekton env: TEKTON_VERSION: ${{ matrix.tekton }} diff --git a/deploy/crds/shipwright.io_buildruns.yaml b/deploy/crds/shipwright.io_buildruns.yaml index d2f00f59cd..9797b1387a 100644 --- a/deploy/crds/shipwright.io_buildruns.yaml +++ b/deploy/crds/shipwright.io_buildruns.yaml @@ -7535,6 +7535,45 @@ spec: Build should take to execute. format: duration type: string + tolerations: + description: If specified, the pod's tolerations. + items: + description: |- + The pod this Toleration is attached to tolerates any taint that matches + the triple using the matching operator . + properties: + effect: + description: |- + Effect indicates the taint effect to match. Empty means match all taint effects. + When specified, allowed values are NoSchedule, PreferNoSchedule and NoExecute. + type: string + key: + description: |- + Key is the taint key that the toleration applies to. Empty means match all taint keys. + If the key is empty, operator must be Exists; this combination means to match all values and all keys. + type: string + operator: + description: |- + Operator represents a key's relationship to the value. + Valid operators are Exists and Equal. Defaults to Equal. + Exists is equivalent to wildcard for value, so that a pod can + tolerate all taints of a particular category. + type: string + tolerationSeconds: + description: |- + TolerationSeconds represents the period of time the toleration (which must be + of effect NoExecute, otherwise this field is ignored) tolerates the taint. By default, + it is not set, which means tolerate the taint forever (do not evict). Zero and + negative values will be treated as 0 (evict immediately) by the system. + format: int64 + type: integer + value: + description: |- + Value is the taint value the toleration matches to. + If the operator is Exists, the value should be empty, otherwise just a regular string. + type: string + type: object + type: array trigger: description: Trigger defines the scenarios where a new build should be triggered. @@ -9753,6 +9792,45 @@ spec: description: Timeout defines the maximum run time of this BuildRun. format: duration type: string + tolerations: + description: If specified, the pod's tolerations. + items: + description: |- + The pod this Toleration is attached to tolerates any taint that matches + the triple using the matching operator . + properties: + effect: + description: |- + Effect indicates the taint effect to match. Empty means match all taint effects. + When specified, allowed values are NoSchedule, PreferNoSchedule and NoExecute. + type: string + key: + description: |- + Key is the taint key that the toleration applies to. Empty means match all taint keys. + If the key is empty, operator must be Exists; this combination means to match all values and all keys. + type: string + operator: + description: |- + Operator represents a key's relationship to the value. + Valid operators are Exists and Equal. Defaults to Equal. + Exists is equivalent to wildcard for value, so that a pod can + tolerate all taints of a particular category. + type: string + tolerationSeconds: + description: |- + TolerationSeconds represents the period of time the toleration (which must be + of effect NoExecute, otherwise this field is ignored) tolerates the taint. By default, + it is not set, which means tolerate the taint forever (do not evict). Zero and + negative values will be treated as 0 (evict immediately) by the system. + format: int64 + type: integer + value: + description: |- + Value is the taint value the toleration matches to. + If the operator is Exists, the value should be empty, otherwise just a regular string. + type: string + type: object + type: array volumes: description: |- Volumes contains volume Overrides of the BuildStrategy volumes in case those are allowed @@ -11959,6 +12037,45 @@ spec: should take to execute. format: duration type: string + tolerations: + description: If specified, the pod's tolerations. + items: + description: |- + The pod this Toleration is attached to tolerates any taint that matches + the triple using the matching operator . + properties: + effect: + description: |- + Effect indicates the taint effect to match. Empty means match all taint effects. + When specified, allowed values are NoSchedule, PreferNoSchedule and NoExecute. + type: string + key: + description: |- + Key is the taint key that the toleration applies to. Empty means match all taint keys. + If the key is empty, operator must be Exists; this combination means to match all values and all keys. + type: string + operator: + description: |- + Operator represents a key's relationship to the value. + Valid operators are Exists and Equal. Defaults to Equal. + Exists is equivalent to wildcard for value, so that a pod can + tolerate all taints of a particular category. + type: string + tolerationSeconds: + description: |- + TolerationSeconds represents the period of time the toleration (which must be + of effect NoExecute, otherwise this field is ignored) tolerates the taint. By default, + it is not set, which means tolerate the taint forever (do not evict). Zero and + negative values will be treated as 0 (evict immediately) by the system. + format: int64 + type: integer + value: + description: |- + Value is the taint value the toleration matches to. + If the operator is Exists, the value should be empty, otherwise just a regular string. + type: string + type: object + type: array trigger: description: Trigger defines the scenarios where a new build should be triggered. diff --git a/deploy/crds/shipwright.io_builds.yaml b/deploy/crds/shipwright.io_builds.yaml index ca02694d9f..9873c95947 100644 --- a/deploy/crds/shipwright.io_builds.yaml +++ b/deploy/crds/shipwright.io_builds.yaml @@ -2912,6 +2912,45 @@ spec: should take to execute. format: duration type: string + tolerations: + description: If specified, the pod's tolerations. + items: + description: |- + The pod this Toleration is attached to tolerates any taint that matches + the triple using the matching operator . + properties: + effect: + description: |- + Effect indicates the taint effect to match. Empty means match all taint effects. + When specified, allowed values are NoSchedule, PreferNoSchedule and NoExecute. + type: string + key: + description: |- + Key is the taint key that the toleration applies to. Empty means match all taint keys. + If the key is empty, operator must be Exists; this combination means to match all values and all keys. + type: string + operator: + description: |- + Operator represents a key's relationship to the value. + Valid operators are Exists and Equal. Defaults to Equal. + Exists is equivalent to wildcard for value, so that a pod can + tolerate all taints of a particular category. + type: string + tolerationSeconds: + description: |- + TolerationSeconds represents the period of time the toleration (which must be + of effect NoExecute, otherwise this field is ignored) tolerates the taint. By default, + it is not set, which means tolerate the taint forever (do not evict). Zero and + negative values will be treated as 0 (evict immediately) by the system. + format: int64 + type: integer + value: + description: |- + Value is the taint value the toleration matches to. + If the operator is Exists, the value should be empty, otherwise just a regular string. + type: string + type: object + type: array trigger: description: Trigger defines the scenarios where a new build should be triggered. diff --git a/docs/build.md b/docs/build.md index c0d685584c..3bab038d78 100644 --- a/docs/build.md +++ b/docs/build.md @@ -40,6 +40,7 @@ A `Build` resource allows the user to define: - retention - volumes - nodeSelector +- tolerations A `Build` is available within a namespace. @@ -94,6 +95,8 @@ To prevent users from triggering `BuildRun`s (_execution of a Build_) that will | TriggerInvalidPipeline | Trigger type Pipeline is invalid. | | OutputTimestampNotSupported | An unsupported output timestamp setting was used. | | OutputTimestampNotValid | The output timestamp value is not valid. | +| NodeSelectorNotValid | The specified nodeSelector is not valid. | +| TolerationNotValid | The specified tolerations are not valid. | ## Configuring a Build @@ -125,7 +128,8 @@ The `Build` definition supports the following fields: - `spec.retention.ttlAfterSucceeded` - Specifies the duration for which a successful buildrun can exist. - `spec.retention.failedLimit` - Specifies the number of failed buildrun that can exist. - `spec.retention.succeededLimit` - Specifies the number of successful buildrun can exist. - - `spec.nodeSelector` - Specifies a selector which must match a node's labels for the build pod to be scheduled on that node. + - `spec.nodeSelector` - Specifies a selector which must match a node's labels for the build pod to be scheduled on that node. If nodeSelectors are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence. + - `spec.tolerations` - Specifies the tolerations for the build pod. Only `key`, `value`, and `operator` are supported. Only `NoSchedule` taint `effect` is supported. If tolerations are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence. ### Defining the Source diff --git a/docs/buildrun.md b/docs/buildrun.md index 8b5a83eafa..35866480a5 100644 --- a/docs/buildrun.md +++ b/docs/buildrun.md @@ -75,7 +75,8 @@ The `BuildRun` definition supports the following fields: - `spec.output.timestamp` - Overrides the output timestamp configuration of the referenced build to instruct the build to change the output image creation timestamp to the specified value. When omitted, the respective build strategy tool defines the output image timestamp. - `spec.output.vulnerabilityScan` - Overrides the output vulnerabilityScan configuration of the referenced build to run the vulnerability scan for the generated image. - `spec.env` - Specifies additional environment variables that should be passed to the build container. Overrides any environment variables that are specified in the `Build` resource. The available variables depend on the tool used by the chosen build strategy. - - `spec.nodeSelector` - Specifies a selector which must match a node's labels for the build pod to be scheduled on that node. + - `spec.nodeSelector` - Specifies a selector which must match a node's labels for the build pod to be scheduled on that node. If nodeSelectors are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence. + - `spec.tolerations` - Specifies the tolerations for the build pod. Only `key`, `value`, and `operator` are supported. Only `NoSchedule` taint `effect` is supported. If tolerations are specified in both a `Build` and `BuildRun`, `BuildRun` values take precedence. **Note**: The `spec.build.name` and `spec.build.spec` are mutually exclusive. Furthermore, the overrides for `timeout`, `paramValues`, `output`, and `env` can only be combined with `spec.build.name`, but **not** with `spec.build.spec`. diff --git a/pkg/apis/build/v1beta1/build_types.go b/pkg/apis/build/v1beta1/build_types.go index 22a6e0b7f1..73822614b4 100644 --- a/pkg/apis/build/v1beta1/build_types.go +++ b/pkg/apis/build/v1beta1/build_types.go @@ -78,6 +78,8 @@ const ( OutputTimestampNotValid BuildReason = "OutputTimestampNotValid" // NodeSelectorNotValid indicates that the nodeSelector value is not valid NodeSelectorNotValid BuildReason = "NodeSelectorNotValid" + // TolerationNotValid indicates that the Toleration value is not valid + TolerationNotValid BuildReason = "TolerationNotValid" // AllValidationsSucceeded indicates a Build was successfully validated AllValidationsSucceeded = "all validations succeeded" @@ -183,6 +185,12 @@ type BuildSpec struct { // // +optional NodeSelector map[string]string `json:"nodeSelector,omitempty"` + + // If specified, the pod's tolerations. + // +optional + // +patchMergeKey=Key + // +patchStrategy=merge + Tolerations []corev1.Toleration `json:"tolerations,omitempty" patchStrategy:"merge" patchMergeKey:"Key"` } // BuildVolume is a volume that will be mounted in build pod during build step diff --git a/pkg/apis/build/v1beta1/buildrun_types.go b/pkg/apis/build/v1beta1/buildrun_types.go index 05bc7723f6..af2882fbb8 100644 --- a/pkg/apis/build/v1beta1/buildrun_types.go +++ b/pkg/apis/build/v1beta1/buildrun_types.go @@ -115,6 +115,12 @@ type BuildRunSpec struct { // // +optional NodeSelector map[string]string `json:"nodeSelector,omitempty"` + + // If specified, the pod's tolerations. + // +optional + // +patchMergeKey=Key + // +patchStrategy=merge + Tolerations []corev1.Toleration `json:"tolerations,omitempty" patchStrategy:"merge" patchMergeKey:"Key"` } // BuildRunRequestedState defines the buildrun state the user can provide to override whatever is the current state. diff --git a/pkg/reconciler/build/build.go b/pkg/reconciler/build/build.go index cfdb15db55..805d13a63f 100644 --- a/pkg/reconciler/build/build.go +++ b/pkg/reconciler/build/build.go @@ -34,6 +34,7 @@ var validationTypes = [...]string{ validate.Envs, validate.Triggers, validate.NodeSelector, + validate.Tolerations, } // ReconcileBuild reconciles a Build object diff --git a/pkg/reconciler/build/build_test.go b/pkg/reconciler/build/build_test.go index c621010fcc..c8d07d7ff8 100644 --- a/pkg/reconciler/build/build_test.go +++ b/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, "Node selector key not valid: 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, "Toleration key not valid: name part "+validation.MaxLenError(63)) statusWriter.UpdateCalls(statusCall) _, err := reconciler.Reconcile(context.TODO(), request) diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go index e17255bab7..203e8b465a 100644 --- a/pkg/reconciler/buildrun/buildrun.go +++ b/pkg/reconciler/buildrun/buildrun.go @@ -161,6 +161,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req validate.NewBuildName(build), validate.NewEnv(build), validate.NewNodeSelector(build), + validate.NewTolerations(build), ) // an internal/technical error during validation happened @@ -291,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 { diff --git a/pkg/reconciler/buildrun/buildrun_test.go b/pkg/reconciler/buildrun/buildrun_test.go index ae3ef627b5..18771f3a95 100644 --- a/pkg/reconciler/buildrun/buildrun_test.go +++ b/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) diff --git a/pkg/reconciler/buildrun/resources/taskrun.go b/pkg/reconciler/buildrun/resources/taskrun.go index 17b790368f..f93bd9b940 100644 --- a/pkg/reconciler/buildrun/resources/taskrun.go +++ b/pkg/reconciler/buildrun/resources/taskrun.go @@ -7,6 +7,7 @@ package resources import ( "fmt" "path" + "slices" "strconv" "strings" @@ -235,12 +236,27 @@ func GenerateTaskRun( }, } + taskRunPodTemplate := &pod.PodTemplate{} // Merge Build and BuildRun NodeSelectors, giving preference to BuildRun NodeSelector taskRunNodeSelector := mergeMaps(build.Spec.NodeSelector, buildRun.Spec.NodeSelector) if len(taskRunNodeSelector) > 0 { - expectedTaskRun.Spec.PodTemplate = &pod.PodTemplate{ - NodeSelector: taskRunNodeSelector, + taskRunPodTemplate.NodeSelector = taskRunNodeSelector + } + + // Merge Build and BuildRun Tolerations, giving preference to BuildRun Tolerations values + taskRunTolerations := mergeTolerations(build.Spec.Tolerations, buildRun.Spec.Tolerations) + if len(taskRunTolerations) > 0 { + for i, toleration := range taskRunTolerations { + if toleration.Effect == "" { + // set unspecified effects to TainEffectNoSchedule, as that is the only supported effect + taskRunTolerations[i].Effect = corev1.TaintEffectNoSchedule + } } + taskRunPodTemplate.Tolerations = taskRunTolerations + } + + if !(taskRunPodTemplate.Equals(&pod.PodTemplate{})) { + expectedTaskRun.Spec.PodTemplate = taskRunPodTemplate } // assign the annotations from the build strategy, filter out those that should not be propagated @@ -354,6 +370,21 @@ func effectiveTimeout(build *buildv1beta1.Build, buildRun *buildv1beta1.BuildRun return nil } +// mergeTolerations merges the values for Spec.Tolerations in the given Build and BuildRun objects, with values in the BuildRun object overriding values +// in the Build object (if present). +func mergeTolerations(buildTolerations []corev1.Toleration, buildRunTolerations []corev1.Toleration) []corev1.Toleration { + mergedTolerations := []corev1.Toleration{} + mergedTolerations = append(mergedTolerations, buildRunTolerations...) + for _, toleration := range buildTolerations { + if !slices.ContainsFunc(mergedTolerations, func(t corev1.Toleration) bool { + return t.Key == toleration.Key + }) { + mergedTolerations = append(mergedTolerations, toleration) + } + } + return mergedTolerations +} + // isPropagatableAnnotation filters the last-applied-configuration annotation from kubectl because this would break the meaning of this annotation on the target object; // also, annotations using our own custom resource domains are filtered out because we have no annotations with a semantic for both TaskRun and Pod func isPropagatableAnnotation(key string) bool { diff --git a/pkg/reconciler/buildrun/resources/taskrun_test.go b/pkg/reconciler/buildrun/resources/taskrun_test.go index 0e4b58437a..c81b073203 100644 --- a/pkg/reconciler/buildrun/resources/taskrun_test.go +++ b/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)) + }) + }) }) }) diff --git a/pkg/validate/nodeselector.go b/pkg/validate/nodeselector.go index ee4b1e1401..eee0cd01d1 100644 --- a/pkg/validate/nodeselector.go +++ b/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, "", "" +} diff --git a/pkg/validate/tolerations.go b/pkg/validate/tolerations.go new file mode 100644 index 0000000000..ec2fdae89a --- /dev/null +++ b/pkg/validate/tolerations.go @@ -0,0 +1,88 @@ +// Copyright The Shipwright Contributors +// +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "context" + "fmt" + "strings" + + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/ptr" + + build "github.com/shipwright-io/build/pkg/apis/build/v1beta1" +) + +// TolerationsRef contains all required fields +// to validate tolerations +type TolerationsRef struct { + Build *build.Build // build instance for analysis +} + +func NewTolerations(build *build.Build) *TolerationsRef { + return &TolerationsRef{build} +} + +// ValidatePath implements BuildPath interface and validates +// that tolerations key/operator/value are valid +func (b *TolerationsRef) ValidatePath(_ context.Context) error { + for _, toleration := range b.Build.Spec.Tolerations { + // validate Key + if errs := validation.IsQualifiedName(toleration.Key); errs != nil { + b.Build.Status.Reason = ptr.To(build.TolerationNotValid) + 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)) { + b.Build.Status.Reason = ptr.To(build.TolerationNotValid) + b.Build.Status.Message = ptr.To(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 { + b.Build.Status.Reason = ptr.To(build.TolerationNotValid) + 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)) { + b.Build.Status.Reason = ptr.To(build.TolerationNotValid) + b.Build.Status.Message = ptr.To(fmt.Sprintf("Only the '%v' toleration effect is supported.", v1.TaintEffectNoSchedule)) + } + // validate TolerationSeconds, which should not be specified + if toleration.TolerationSeconds != nil { + b.Build.Status.Reason = ptr.To(build.TolerationNotValid) + b.Build.Status.Message = ptr.To("Specifying TolerationSeconds is not supported.") + } + } + + 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, "", "" +} diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index d39396652a..e8228f7084 100644 --- a/pkg/validate/validate.go +++ b/pkg/validate/validate.go @@ -37,6 +37,8 @@ const ( Triggers = "triggers" // NodeSelector for validating `spec.nodeSelector` entry NodeSelector = "nodeselector" + // Tolerations for validating `spec.tolerations` entry + Tolerations = "tolerations" ) const ( @@ -79,6 +81,8 @@ func NewValidation( return &Trigger{build: build}, nil case NodeSelector: return &NodeSelectorRef{Build: build}, nil + case Tolerations: + return &TolerationsRef{Build: build}, nil default: return nil, fmt.Errorf("unknown validation type") } @@ -133,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 "", "" diff --git a/test/data/v1beta1/build_buildah_tolerations_cr.yaml b/test/data/v1beta1/build_buildah_tolerations_cr.yaml new file mode 100644 index 0000000000..026014fda2 --- /dev/null +++ b/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" \ No newline at end of file diff --git a/test/data/v1beta1/buildrun_buildah_tolerations_cr.yaml b/test/data/v1beta1/buildrun_buildah_tolerations_cr.yaml new file mode 100644 index 0000000000..d6571d03bf --- /dev/null +++ b/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 \ No newline at end of file diff --git a/test/e2e/v1beta1/e2e_test.go b/test/e2e/v1beta1/e2e_test.go index 7ea6befc39..2c9c1e082c 100644 --- a/test/e2e/v1beta1/e2e_test.go +++ b/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")) + }) + }) }) diff --git a/test/integration/build_to_taskruns_test.go b/test/integration/build_to_taskruns_test.go index 943c711532..675305e14c 100644 --- a/test/integration/build_to_taskruns_test.go +++ b/test/integration/build_to_taskruns_test.go @@ -246,4 +246,50 @@ var _ = Describe("Integration tests Build and TaskRun", func() { }) }) }) + + Context("when a build with Tolerations is defined", func() { + BeforeEach(func() { + buildSample = []byte(test.MinimalBuildWithToleration) + buildRunSample = []byte(test.MinimalBuildRun) + }) + + Context("when the TaskRun is created", func() { + It("should have the Tolerations specified in the PodTemplate", func() { + Expect(tb.CreateBuild(buildObject)).To(BeNil()) + + buildObject, err = tb.GetBuildTillValidation(buildObject.Name) + Expect(err).To(BeNil()) + Expect(*buildObject.Status.Message).To(Equal(v1beta1.AllValidationsSucceeded)) + Expect(*buildObject.Status.Registered).To(Equal(corev1.ConditionTrue)) + Expect(*buildObject.Status.Reason).To(Equal(v1beta1.SucceedStatus)) + + Expect(tb.CreateBR(buildRunObject)).To(BeNil()) + + _, err = tb.GetBRTillStartTime(buildRunObject.Name) + Expect(err).To(BeNil()) + + tr, err := tb.GetTaskRunFromBuildRun(buildRunObject.Name) + Expect(err).To(BeNil()) + Expect(buildObject.Spec.Tolerations[0].Key).To(Equal(tr.Spec.PodTemplate.Tolerations[0].Key)) + Expect(buildObject.Spec.Tolerations[0].Operator).To(Equal(tr.Spec.PodTemplate.Tolerations[0].Operator)) + Expect(buildObject.Spec.Tolerations[0].Value).To(Equal(tr.Spec.PodTemplate.Tolerations[0].Value)) + Expect(tr.Spec.PodTemplate.Tolerations[0].TolerationSeconds).To(Equal(corev1.Toleration{}.TolerationSeconds)) + Expect(tr.Spec.PodTemplate.Tolerations[0].Effect).To(Equal(corev1.TaintEffectNoSchedule)) + }) + }) + + Context("when the Toleration is invalid", func() { + It("fails the build with a proper error in Reason", func() { + // set Toleration Key to be invalid + buildObject.Spec.Tolerations[0].Key = strings.Repeat("s", 64) + Expect(tb.CreateBuild(buildObject)).To(BeNil()) + + buildObject, err = tb.GetBuildTillValidation(buildObject.Name) + Expect(err).To(BeNil()) + + Expect(*buildObject.Status.Registered).To(Equal(corev1.ConditionFalse)) + Expect(*buildObject.Status.Reason).To(Equal(v1beta1.TolerationNotValid)) + }) + }) + }) }) diff --git a/test/integration/buildruns_to_taskruns_test.go b/test/integration/buildruns_to_taskruns_test.go index 59dc9abe52..585f8f7ac9 100644 --- a/test/integration/buildruns_to_taskruns_test.go +++ b/test/integration/buildruns_to_taskruns_test.go @@ -21,6 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" ) @@ -577,9 +578,59 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() { condition := br.Status.GetCondition(v1beta1.Succeeded) Expect(condition.Status).To(Equal(corev1.ConditionFalse)) - Expect(condition.Reason).To(Equal("PodCreationFailed")) + Expect(condition.Reason).To(Equal("NodeSelectorNotValid")) Expect(condition.Message).To(ContainSubstring("must be no more than 63 characters")) }) }) }) + + Context("when a buildrun is created with a Toleration defined", func() { + BeforeEach(func() { + buildSample = []byte(test.MinimalBuild) + buildRunSample = []byte(test.MinimalBuildRunWithToleration) + }) + + Context("when the taskrun is created", func() { + It("should have the Toleration specified in the PodTemplate", func() { + Expect(tb.CreateBuild(buildObject)).To(BeNil()) + + buildObject, err = tb.GetBuildTillValidation(buildObject.Name) + Expect(err).To(BeNil()) + + Expect(tb.CreateBR(buildRunObject)).To(BeNil()) + + br, err := tb.GetBRTillCompletion(buildRunObject.Name) + Expect(err).To(BeNil()) + + tr, err := tb.GetTaskRunFromBuildRun(buildRunObject.Name) + Expect(err).To(BeNil()) + Expect(br.Spec.Tolerations[0].Key).To(Equal(tr.Spec.PodTemplate.Tolerations[0].Key)) + Expect(br.Spec.Tolerations[0].Operator).To(Equal(tr.Spec.PodTemplate.Tolerations[0].Operator)) + Expect(br.Spec.Tolerations[0].Value).To(Equal(tr.Spec.PodTemplate.Tolerations[0].Value)) + Expect(tr.Spec.PodTemplate.Tolerations[0].TolerationSeconds).To(Equal(corev1.Toleration{}.TolerationSeconds)) + Expect(tr.Spec.PodTemplate.Tolerations[0].Effect).To(Equal(corev1.TaintEffectNoSchedule)) + }) + }) + + Context("when the Toleration is invalid", func() { + It("fails the buildrun with a proper error in Reason", func() { + Expect(tb.CreateBuild(buildObject)).To(BeNil()) + + buildObject, err = tb.GetBuildTillValidation(buildObject.Name) + Expect(err).To(BeNil()) + + // set Toleration Key to be invalid + buildRunObject.Spec.Tolerations[0].Key = strings.Repeat("s", 64) + Expect(tb.CreateBR(buildRunObject)).To(BeNil()) + + br, err := tb.GetBRTillCompletion(buildRunObject.Name) + Expect(err).To(BeNil()) + + condition := br.Status.GetCondition(v1beta1.Succeeded) + Expect(condition.Status).To(Equal(corev1.ConditionFalse)) + Expect(condition.Reason).To(Equal("TolerationNotValid")) + Expect(condition.Message).To(ContainSubstring(validation.MaxLenError(63))) + }) + }) + }) }) diff --git a/test/kind/config_three_node.yaml b/test/kind/config_three_node.yaml new file mode 100644 index 0000000000..c968e608fa --- /dev/null +++ b/test/kind/config_three_node.yaml @@ -0,0 +1,17 @@ +kind: Cluster +apiVersion: kind.x-k8s.io/v1alpha4 +kubeadmConfigPatches: +- | + kind: ClusterConfiguration + metadata: + name: config + apiServer: + extraArgs: + enable-admission-plugins: PodSecurity +nodes: +- role: control-plane + extraPortMappings: + - containerPort: 32222 + hostPort: 32222 +- role: worker +- role: worker diff --git a/test/utils/v1beta1/nodes.go b/test/utils/v1beta1/nodes.go new file mode 100644 index 0000000000..e7f2071d27 --- /dev/null +++ b/test/utils/v1beta1/nodes.go @@ -0,0 +1,51 @@ +// Copyright The Shipwright Contributors +// +// SPDX-License-Identifier: Apache-2.0 + +package utils + +import ( + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + apply "k8s.io/client-go/applyconfigurations/core/v1" +) + +// GetNodes returns all Nodes for the TestBuild object +func (t *TestBuild) GetNodes() (*corev1.NodeList, error) { + client := t.Clientset.CoreV1().Nodes() + nodes, err := client.List(t.Context, metav1.ListOptions{}) + return nodes, err +} + +// AddNodeTaint sets a taint on the given Node name +func (t *TestBuild) AddNodeTaint(nodeName string, taint *corev1.Taint) error { + client := t.Clientset.CoreV1().Nodes() + taintApplyCfg := apply.Taint() + taintApplyCfg.WithKey(taint.Key) + taintApplyCfg.WithValue(taint.Value) + taintApplyCfg.WithEffect(taint.Effect) + nodeSpecApplyCfg := apply.NodeSpec() + nodeSpecApplyCfg.WithTaints(taintApplyCfg) + applyCfg := apply.Node(nodeName) + applyCfg.WithSpec(nodeSpecApplyCfg) + _, err := client.Apply(t.Context, applyCfg, metav1.ApplyOptions{FieldManager: "application/apply-patch-1}", Force: true}) + if err != nil { + return err + } + return nil +} + +// RemoveNodeTaints removes all taints on the given Node name +func (t *TestBuild) RemoveNodeTaints(nodeName string) error { + client := t.Clientset.CoreV1().Nodes() + + // explicitly set taints to null, instead of using an apply config which marshals to empty string values for the taint values. + // empty string values will fail to validate. + body := "{\"spec\":{\"taints\":null}}" + _, err := client.Patch(t.Context, nodeName, types.StrategicMergePatchType, []byte(body), metav1.PatchOptions{FieldManager: "application/apply-patch-2"}) + if err != nil { + return err + } + return nil +} diff --git a/test/v1beta1_samples/build_samples.go b/test/v1beta1_samples/build_samples.go index a46f917fe9..eff97c3258 100644 --- a/test/v1beta1_samples/build_samples.go +++ b/test/v1beta1_samples/build_samples.go @@ -565,6 +565,22 @@ spec: kubernetes.io/arch: amd64 ` +// MinimalBuildWithToleration defines a simple +// Build with a strategy, output, and a Toleration specified +const MinimalBuildWithToleration = ` +apiVersion: shipwright.io/v1beta1 +kind: Build +spec: + strategy: + kind: ClusterBuildStrategy + output: + image: image-registry.openshift-image-registry.svc:5000/example/buildpacks-app + tolerations: + - key: "build-test-key" + operator: "Equal" + value: "build-test-value" +` + // BuildWithUndefinedParameter defines a param that was not declared under the // strategy parameters const BuildWithUndefinedParam = ` diff --git a/test/v1beta1_samples/buildrun_samples.go b/test/v1beta1_samples/buildrun_samples.go index 7e55033585..cb728e0dc6 100644 --- a/test/v1beta1_samples/buildrun_samples.go +++ b/test/v1beta1_samples/buildrun_samples.go @@ -240,6 +240,21 @@ spec: kubernetes.io/arch: amd64 ` +// MinimalBuildRunWithToleration defines a minimal BuildRun +// with a reference to a not existing Build, +// and a Toleration specified +const MinimalBuildRunWithToleration = ` +apiVersion: shipwright.io/v1beta1 +kind: BuildRun +spec: + build: + name: foobar + tolerations: + - key: "buildrun-test-key" + operator: "Equal" + value: "buildrun-test-value" +` + // MinimalBuildRunWithVulnerabilityScan defines a BuildRun with // an override for the Build Output const MinimalBuildRunWithVulnerabilityScan = `