From ac18f629d4dd9edf15040d9adf095c2e9f343eb7 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Thu, 10 Mar 2022 21:47:42 +0100 Subject: [PATCH] Introduce embedded `BuildSpec` for `BuildRun`s Make `BuildRef` optional and generate CRDs and fix all occurrences. Introduce code in `GetBuildObject` to support both the `BuildRef` approach and the new `BuildSpec` option. Return a transient in-memory Build resource in case the BuildRun has an embedded BuildSpec. Add optional `BuildSpec` in BuildRun spec. Add E2E test cases for embedded builds in buildruns. Add new BuildRun validation option in docs. Remove variable name for `context` to make it undefined (linter warning). Add empty lines for readability. Sort imports based on common kube/others order. Replace copy/paste description with specific one for bundle tests. Add `Expect`s to give human readable error in case test environment variables are not set. Remove duplicate import of `k8s.io/api/core/v1`. Simplify code by removing the `if` check. Fix warning that variable declaration could shadow previous variable value by refactoring the respective `if` block. Add or remove empty lines in `buildrun_test.go` to increase readability and to keep the same style as in other test cases. Refactor code to fix variable value shadow warning. Replace generic `errors.New("not found")` with Kube specific one. Introduce empty status call stub (not not fail in status update case). Refactor return statement to be more simple. Introduce `BuildSpec` in BuildRun spec Refactor build reconciler logic by inlining `UpdateBuildStatusAndRetreat` function that is only used once. Add test case to cover special case for build resource not being in the system. Refactor validation code by moving out validation list into a global variable. Add convenience function to get the name of the referenced build. Use `resources.GetBuildObject` instead of manual look-up in test cases. Introduce controller-runtime Kubernetes client in test handle. Add functions to create the validation structs in a consistent way. Add convenience function to validate a Build with validation varargs. Introduce validation code for field permutations. Add test cases for field validation. Fix markdown linter warnings and indention. --- README.md | 37 +- deploy/crds/shipwright.io_buildruns.yaml | 373 +++++++++++++++++- docs/buildrun.md | 44 ++- pkg/apis/build/v1alpha1/buildrun_types.go | 26 +- .../build/v1alpha1/zz_generated.deepcopy.go | 11 +- pkg/reconciler/build/build.go | 59 ++- pkg/reconciler/build/build_test.go | 45 ++- pkg/reconciler/buildrun/buildrun.go | 118 ++++-- pkg/reconciler/buildrun/buildrun_test.go | 315 +++++++++++++-- pkg/reconciler/buildrun/resources/build.go | 24 +- .../buildrun/resources/build_test.go | 71 ++-- .../buildrun/resources/conditions.go | 3 + .../buildrun/resources/conditions_test.go | 8 +- .../buildrun/resources/service_accounts.go | 15 +- .../resources/service_accounts_test.go | 8 +- .../buildrun/resources/strategies_test.go | 4 +- pkg/reconciler/buildrun/resources/taskrun.go | 19 +- pkg/validate/buildname.go | 4 + pkg/validate/secrets.go | 4 + pkg/validate/sourceurl.go | 4 + pkg/validate/strategies.go | 4 + pkg/validate/validate.go | 50 +++ test/catalog.go | 18 +- test/e2e/common_suite_test.go | 22 +- test/e2e/common_test.go | 11 +- test/e2e/e2e_bundle_test.go | 2 +- test/e2e/e2e_one_off_builds_test.go | 110 ++++++ test/e2e/e2e_suite_test.go | 3 +- test/e2e/validators_test.go | 7 +- test/utils/environment.go | 8 + test/utils/lookup.go | 1 - 31 files changed, 1218 insertions(+), 210 deletions(-) create mode 100644 test/e2e/e2e_one_off_builds_test.go diff --git a/README.md b/README.md index 8d4dda0cfe..8b0aa26694 100644 --- a/README.md +++ b/README.md @@ -32,27 +32,27 @@ Shipwright supports any tool that can build container images in Kubernetes clust ## Try It! -* We assume you already have a Kubernetes cluster (v1.21+). If you don't, you can use [KinD](https://kind.sigs.k8s.io), which you can install by running [`./hack/install-kind.sh`](./hack/install-kind.sh). +- We assume you already have a Kubernetes cluster (v1.21+). If you don't, you can use [KinD](https://kind.sigs.k8s.io), which you can install by running [`./hack/install-kind.sh`](./hack/install-kind.sh). -* We also require a Tekton installation (v0.30+). To install the newest supported version, run: +- We also require a Tekton installation (v0.30+). To install the newest supported version, run: ```bash kubectl apply --filename https://storage.googleapis.com/tekton-releases/pipeline/previous/v0.34.1/release.yaml ``` -* Install the Shipwright deployment. To install the latest version, run: +- Install the Shipwright deployment. To install the latest version, run: ```bash kubectl apply --filename https://github.com/shipwright-io/build/releases/download/v0.8.0/release.yaml ``` -* Install the Shipwright strategies. To install the latest version, run: +- Install the Shipwright strategies. To install the latest version, run: ```bash kubectl apply --filename https://github.com/shipwright-io/build/releases/download/v0.8.0/sample-strategies.yaml ``` -* Generate a secret to access your container registry, such as one on [Docker Hub](https://hub.docker.com/) or [Quay.io](https://quay.io/): +- Generate a secret to access your container registry, such as one on [Docker Hub](https://hub.docker.com/) or [Quay.io](https://quay.io/): ```bash REGISTRY_SERVER=https://index.docker.io/v1/ REGISTRY_USER= REGISTRY_PASSWORD= @@ -63,7 +63,7 @@ Shipwright supports any tool that can build container images in Kubernetes clust --docker-email= ``` -* Create a *Build* object, replacing `` with the registry username your `push-secret` secret have access to: +- Create a *Build* object, replacing `` with the registry username your `push-secret` secret have access to: ```bash REGISTRY_ORG= @@ -86,15 +86,16 @@ Shipwright supports any tool that can build container images in Kubernetes clust EOF ``` -To view the *Build* which you just created: + To view the *Build* which you just created: - ``` - $ kubectl get builds + ```bash + $ kubectl get builds NAME REGISTERED REASON BUILDSTRATEGYKIND BUILDSTRATEGYNAME CREATIONTIME buildpack-nodejs-build True Succeeded ClusterBuildStrategy buildpacks-v3 68s ``` -* Submit your *BuildRun*: + +- Submit your *BuildRun*: ```bash cat <'']`, + `metadata.annotations['''']`, spec.nodeName, + spec.serviceAccountName, status.hostIP, status.podIP, + status.podIPs.' + properties: + apiVersion: + description: Version of the schema the FieldPath + is written in terms of, defaults to "v1". + type: string + fieldPath: + description: Path of the field to select in the + specified API version. + type: string + required: + - fieldPath + type: object + resourceFieldRef: + description: 'Selects a resource of the container: only + resources limits and requests (limits.cpu, limits.memory, + limits.ephemeral-storage, requests.cpu, requests.memory + and requests.ephemeral-storage) are currently supported.' + properties: + containerName: + description: 'Container name: required for volumes, + optional for env vars' + type: string + divisor: + anyOf: + - type: integer + - type: string + description: Specifies the output format of the + exposed resources, defaults to "1" + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + resource: + description: 'Required: resource to select' + type: string + required: + - resource + type: object + secretKeyRef: + description: Selects a key of a secret in the pod's + namespace + properties: + key: + description: The key of the secret to select from. Must + be a valid secret key. + type: string + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, + uid?' + type: string + optional: + description: Specify whether the Secret or its key + must be defined + type: boolean + required: + - key + type: object + type: object + required: + - name + type: object + type: array + output: + description: Output refers to the location where the built image + would be pushed. + properties: + annotations: + additionalProperties: + type: string + description: Annotations references the additional annotations + to be applied on the image + type: object + credentials: + description: Credentials references a Secret that contains + credentials to access the image registry. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + image: + description: Image is the reference of the image. + type: string + labels: + additionalProperties: + type: string + description: Labels references the additional labels to be + applied on the image + type: object + required: + - image + type: object + paramValues: + description: Params is a list of key/value that could be used + to set strategy parameters + items: + description: ParamValue is a key/value that populates a strategy + parameter used in the execution of the strategy steps + properties: + configMapValue: + description: The ConfigMap value of the parameter + properties: + format: + description: An optional format to add pre- or suffix + to the object value. For example 'KEY=${SECRET_VALUE}' + or 'KEY=${CONFIGMAP_VALUE}' depending on the context. + type: string + key: + description: Key inside the object + type: string + name: + description: Name of the object + type: string + required: + - key + - name + type: object + name: + description: Name of the parameter + type: string + secretValue: + description: The secret value of the parameter + properties: + format: + description: An optional format to add pre- or suffix + to the object value. For example 'KEY=${SECRET_VALUE}' + or 'KEY=${CONFIGMAP_VALUE}' depending on the context. + type: string + key: + description: Key inside the object + type: string + name: + description: Name of the object + type: string + required: + - key + - name + type: object + value: + description: The value of the parameter + type: string + values: + description: Values of an array parameter + items: + description: The value type contains the properties for + a value, this allows for an easy extension in the future + to support more kinds + properties: + configMapValue: + description: The ConfigMap value of the parameter + properties: + format: + description: An optional format to add pre- or + suffix to the object value. For example 'KEY=${SECRET_VALUE}' + or 'KEY=${CONFIGMAP_VALUE}' depending on the + context. + type: string + key: + description: Key inside the object + type: string + name: + description: Name of the object + type: string + required: + - key + - name + type: object + secretValue: + description: The secret value of the parameter + properties: + format: + description: An optional format to add pre- or + suffix to the object value. For example 'KEY=${SECRET_VALUE}' + or 'KEY=${CONFIGMAP_VALUE}' depending on the + context. + type: string + key: + description: Key inside the object + type: string + name: + description: Name of the object + type: string + required: + - key + - name + type: object + value: + description: The value of the parameter + type: string + type: object + type: array + required: + - name + type: object + type: array + source: + description: Source refers to the Git repository containing the + source code to be built. + properties: + bundleContainer: + description: BundleContainer + properties: + image: + description: Image reference, i.e. quay.io/org/image:tag + type: string + required: + - image + type: object + contextDir: + description: ContextDir is a path to subfolder in the repo. + Optional. + type: string + credentials: + description: Credentials references a Secret that contains + credentials to access the repository. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + revision: + description: "Revision describes the Git revision (e.g., branch, + tag, commit SHA, etc.) to fetch. \n If not defined, it will + fallback to the repository's default branch." + type: string + url: + description: URL describes the URL of the Git repository. + type: string + type: object + sources: + description: Sources slice of BuildSource, defining external build + artifacts complementary to VCS (`.spec.source`) data. + items: + description: BuildSource remote artifact definition, also known + as "sources". Simple "name" and "url" pairs, initially without + "credentials" (authentication) support yet. + properties: + name: + description: Name instance entry. + type: string + timeout: + description: Timeout how long the BuildSource execution + must take. + type: string + type: + description: Type is the BuildSource qualifier, the type + of the data-source. + type: string + url: + description: URL remote artifact location. + type: string + required: + - name + type: object + type: array + strategy: + description: Strategy references the BuildStrategy to use to build + the container image. + properties: + apiVersion: + description: API version of the referent + type: string + kind: + description: BuildStrategyKind indicates the kind of the buildstrategy, + namespaced or cluster scoped. + type: string + name: + description: 'Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names' + type: string + required: + - name + type: object + timeout: + description: Timeout defines the maximum amount of time the Build + should take to execute. + format: duration + type: string + required: + - output + - source + - strategy + type: object env: description: Env contains additional environment variables that should be passed to the build container @@ -353,8 +724,6 @@ spec: description: Timeout defines the maximum run time of this BuildRun. format: duration type: string - required: - - buildRef type: object status: description: BuildRunStatus defines the observed state of BuildRun diff --git a/docs/buildrun.md b/docs/buildrun.md index 5c706052ba..b1b20d7285 100644 --- a/docs/buildrun.md +++ b/docs/buildrun.md @@ -10,6 +10,7 @@ SPDX-License-Identifier: Apache-2.0 - [BuildRun Controller](#buildrun-controller) - [Configuring a BuildRun](#configuring-a-buildrun) - [Defining the BuildRef](#defining-the-buildref) + - [Defining the BuildSpec](#defining-the-buildspec) - [Defining ParamValues](#defining-paramvalues) - [Defining the ServiceAccount](#defining-the-serviceaccount) - [Canceling a `BuildRun`](#canceling-a-buildrun) @@ -56,9 +57,10 @@ The `BuildRun` definition supports the following fields: - [`apiVersion`](https://kubernetes.io/docs/concepts/overview/working-with-objects/kubernetes-objects/#required-fields) - Specifies the API version, for example `shipwright.io/v1alpha1`. - [`kind`](https://kubernetes.io/docs/concepts/overview/working-with-objects/kubernetes-objects/#required-fields) - Specifies the Kind type, for example `BuildRun`. - [`metadata`](https://kubernetes.io/docs/concepts/overview/working-with-objects/kubernetes-objects/#required-fields) - Metadata that identify the CRD instance, for example the name of the `BuildRun`. - - `spec.buildRef` - Specifies an existing `Build` resource instance to use. - Optional: + - `spec.buildRef` - Specifies an existing `Build` resource instance to use. Cannot be used together with `buildSpec`. + - `spec.buildSpec` - Specified an embedded (transient) Build resource to use. Cannot be used together with `buildRef`. - `spec.serviceAccount` - Refers to the SA to use when building the image. (_defaults to the `default` SA_) - `spec.timeout` - Defines a custom timeout. The value needs to be parsable by [ParseDuration](https://golang.org/pkg/time/#ParseDuration), for example `5m`. The value overwrites the value that is defined in the `Build`. - `spec.paramValues` - Refers to a name-value(s) list to specify values for `parameters` defined in the `BuildStrategy`. This overwrites values defined with the same name in the Build. @@ -66,6 +68,8 @@ The `BuildRun` definition supports the following fields: - `spec.output.credentials.name` - Reference an existing secret to get access to the container registry. This secret will be added to the service account along with the ones requested by the `Build`. - `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 that is being used by the chosen build strategy. +_Note:_ The `BuildRef` and `BuildSpec` are mutually exclusive. Futhermore, the overrides for `timeout`, `paramValues`, `output`, and `env` can only be used in combination with `buildRef`, but **not** with `buildSpec`. + ### Defining the BuildRef A `BuildRun` resource can reference a `Build` resource, that indicates what image to build. For example: @@ -80,6 +84,27 @@ spec: name: buildpack-nodejs-build-namespaced ``` +### Defining the BuildSpec + +Alternatively to `BuildRef`, a complete `BuildSpec` can be embedded into the `BuildRun` to be used for the build. + +```yaml +apiVersion: shipwright.io/v1alpha1 +kind: BuildRun +metadata: + name: standalone-buildrun +spec: + buildSpec: + source: + url: https://github.com/shipwright-io/sample-go.git + contextDir: source-build + strategy: + kind: ClusterBuildStrategy + name: buildpacks-v3 + output: + image: foo/bar:latest +``` + ### Defining ParamValues A `BuildRun` resource can define _paramValues_ for parameters specified in the build strategy. If a value has been provided for a parameter with the same name in the `Build` already, then the value from the `BuildRun` will have precedence. @@ -247,13 +272,13 @@ The `status.conditions` hosts different fields, like `status`, `reason` and `mes The following table illustrates the different states a BuildRun can have under its `status.conditions`: -| Status | Reason | CompletionTime is set | Description | -| --- | --- | --- | --- | -| Unknown | Pending | No | The BuildRun is waiting on a Pod in status Pending. | -| Unknown | Running | No | The BuildRun has been validate and started to perform its work. |l -| Unknown | Running | No | The BuildRun has been validate and started to perform its work. | -| Unknown | BuildRunCanceled | No | The user requested the BuildRun to be canceled. This results in the BuildRun controller requesting the TaskRun be canceled. Cancellation has not been done yet. | -| True | Succeeded | Yes | The BuildRun Pod is done. | +| Status | Reason | CompletionTime is set | Description | +| --- | --- | --- | --- | +| Unknown | Pending | No | The BuildRun is waiting on a Pod in status Pending. | +| Unknown | Running | No | The BuildRun has been validate and started to perform its work. | +| Unknown | Running | No | The BuildRun has been validate and started to perform its work. | +| Unknown | BuildRunCanceled | No | The user requested the BuildRun to be canceled. This results in the BuildRun controller requesting the TaskRun be canceled. Cancellation has not been done yet. | +| True | Succeeded | Yes | The BuildRun Pod is done. | | False | Failed | Yes | The BuildRun failed in one of the steps. | | False | BuildRunTimeout | Yes | The BuildRun timed out. | | False | UnknownStrategyKind | Yes | The Build specified strategy Kind is unknown. (_options: ClusterBuildStrategy or BuildStrategy_) | @@ -275,6 +300,9 @@ The following table illustrates the different states a BuildRun can have under i | False | BuildNotFound | Yes | The related Build in the BuildRun was not found. | | False | BuildRunCanceled | Yes | The BuildRun and underlying TaskRun were canceled successfully. | | False | BuildRunNameInvalid | Yes | The defined `BuildRun` name (`metadata.name`) is invalid. The `BuildRun` name should be a [valid label value](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set). | +| False | BuildRunNoRefOrSpec | Yes | BuildRun does not have either `BuildRef` or `BuildSpec` defined. There is no connection to a Build specification. | +| False | BuildRunAmbiguousBuild | Yes | The defined `BuildRun` uses both `BuildRef` and `BuildSpec`. Only one of them is allowed at the same time.| +| False | BuildRunBuildFieldOverrideForbidden | Yes | The defined `BuildRun` uses an override (e.g. `timeout`, `paramValues`, `output`, or `env`) in combination with `BuildSpec`, which is not allowed. Use the `BuildSpec` to directly specify the respective value. | | False | PodEvicted | Yes | The BuildRun Pod was evicted from the node it was running on. See [API-initiated Eviction](https://kubernetes.io/docs/concepts/scheduling-eviction/api-eviction/) and [Node-pressure Eviction](https://kubernetes.io/docs/concepts/scheduling-eviction/node-pressure-eviction/) for more information. | _Note_: We heavily rely on the Tekton TaskRun [Conditions](https://github.com/tektoncd/pipeline/blob/main/docs/taskruns.md#monitoring-execution-status) for populating the BuildRun ones, with some exceptions. diff --git a/pkg/apis/build/v1alpha1/buildrun_types.go b/pkg/apis/build/v1alpha1/buildrun_types.go index 2cae3aabbf..4a01e6e20d 100644 --- a/pkg/apis/build/v1alpha1/buildrun_types.go +++ b/pkg/apis/build/v1alpha1/buildrun_types.go @@ -22,8 +22,15 @@ const ( // BuildRunSpec defines the desired state of BuildRun type BuildRunSpec struct { + // BuildSpec refers to an embedded build specification + // + // +optional + BuildSpec *BuildSpec `json:"buildSpec,omitempty"` + // BuildRef refers to the Build - BuildRef BuildRef `json:"buildRef"` + // + // +optional + BuildRef *BuildRef `json:"buildRef,omitempty"` // Sources slice of BuildSource, defining external build artifacts complementary to VCS // (`.spec.source`) data. @@ -34,29 +41,35 @@ type BuildRunSpec struct { // ServiceAccount refers to the kubernetes serviceaccount // which is used for resource control. // Default serviceaccount will be set if it is empty + // // +optional ServiceAccount *ServiceAccount `json:"serviceAccount,omitempty"` // Timeout defines the maximum run time of this BuildRun. + // // +optional // +kubebuilder:validation:Format=duration Timeout *metav1.Duration `json:"timeout,omitempty"` // Params is a list of key/value that could be used // to set strategy parameters + // // +optional ParamValues []ParamValue `json:"paramValues,omitempty"` // Output refers to the location where the generated // image would be pushed to. It will overwrite the output image in build spec + // // +optional Output *Image `json:"output,omitempty"` // State is used for canceling a buildrun (and maybe more later on). + // // +optional State *BuildRunRequestedState `json:"state,omitempty"` // Env contains additional environment variables that should be passed to the build container + // // +optional Env []corev1.EnvVar `json:"env,omitempty"` } @@ -351,3 +364,14 @@ func (brs *BuildRunStatus) SetCondition(condition *Condition) { brs.Conditions = append(brs.Conditions, *condition) } } + +// BuildName returns the name of the associated build, which can be a referened +// build resource or an embedded build specification +func (buildrunSpec *BuildRunSpec) BuildName() string { + if buildrunSpec.BuildRef != nil { + return buildrunSpec.BuildRef.Name + } + + // Only BuildRuns with a BuildRef can actually return a proper Build name + return "" +} diff --git a/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go index d919ef3d25..dab8d4977f 100644 --- a/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go @@ -161,7 +161,16 @@ func (in *BuildRunList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *BuildRunSpec) DeepCopyInto(out *BuildRunSpec) { *out = *in - in.BuildRef.DeepCopyInto(&out.BuildRef) + if in.BuildSpec != nil { + in, out := &in.BuildSpec, &out.BuildSpec + *out = new(BuildSpec) + (*in).DeepCopyInto(*out) + } + if in.BuildRef != nil { + in, out := &in.BuildRef, &out.BuildRef + *out = new(BuildRef) + (*in).DeepCopyInto(*out) + } if in.Sources != nil { in, out := &in.Sources, &out.Sources *out = make([]BuildSource, len(*in)) diff --git a/pkg/reconciler/build/build.go b/pkg/reconciler/build/build.go index 2a96ed1c21..9d6f2d8ba4 100644 --- a/pkg/reconciler/build/build.go +++ b/pkg/reconciler/build/build.go @@ -22,6 +22,17 @@ import ( "github.com/shipwright-io/build/pkg/validate" ) +// build a list of current validation types +var validationTypes = [...]string{ + validate.OwnerReferences, + validate.SourceURL, + validate.Secrets, + validate.Strategies, + validate.Sources, + validate.BuildName, + validate.Envs, +} + // ReconcileBuild reconciles a Build object type ReconcileBuild struct { // This client, initialized using mgr.Client() above, is a split client @@ -52,29 +63,19 @@ func (r *ReconcileBuild) Reconcile(ctx context.Context, request reconcile.Reques ctxlog.Debug(ctx, "start reconciling Build", namespace, request.Namespace, name, request.Name) b := &build.Build{} - err := r.client.Get(ctx, request.NamespacedName, b) - if err != nil && !apierrors.IsNotFound(err) { + if err := r.client.Get(ctx, request.NamespacedName, b); err != nil { + if apierrors.IsNotFound(err) { + ctxlog.Debug(ctx, "finish reconciling build. build was not found", namespace, request.Namespace, name, request.Name) + return reconcile.Result{}, nil + } + return reconcile.Result{}, err - } else if apierrors.IsNotFound(err) { - ctxlog.Debug(ctx, "finish reconciling build. build was not found", namespace, request.Namespace, name, request.Name) - return reconcile.Result{}, nil } // Populate the status struct with default values b.Status.Registered = build.ConditionStatusPtr(corev1.ConditionFalse) b.Status.Reason = build.BuildReasonPtr(build.SucceedStatus) - // build a list of current validation types - validationTypes := []string{ - validate.OwnerReferences, - validate.SourceURL, - validate.Secrets, - validate.Strategies, - validate.Sources, - validate.BuildName, - validate.Envs, - } - // trigger all current validations for _, validationType := range validationTypes { v, err := validate.NewValidation(validationType, b, r.client, r.scheme) @@ -89,22 +90,30 @@ func (r *ReconcileBuild) Reconcile(ctx context.Context, request reconcile.Reques if validationType == validate.Secrets || validationType == validate.Strategies { return reconcile.Result{}, err } + if validationType == validate.OwnerReferences { // we do not want to bail out here if the owerreference validation fails, we ignore this error on purpose // In case we just created the Build, we want the Build reconcile logic to continue, in order to // validate the Build references ( e.g secrets, strategies ) - ctxlog.Info(ctx, "unexpected error during ownership reference validation", namespace, request.Namespace, name, request.Name, "error", err) + ctxlog.Info(ctx, "unexpected error during ownership reference validation", + namespace, b.Namespace, + name, b.Name, + "error", err) } } + if b.Status.Reason == nil || *b.Status.Reason != build.SucceedStatus { - return r.UpdateBuildStatusAndRetreat(ctx, b) + if err := r.client.Status().Update(ctx, b); err != nil { + return reconcile.Result{}, err + } + + return reconcile.Result{}, nil } } b.Status.Registered = build.ConditionStatusPtr(corev1.ConditionTrue) b.Status.Message = pointer.String(build.AllValidationsSucceeded) - err = r.client.Status().Update(ctx, b) - if err != nil { + if err := r.client.Status().Update(ctx, b); err != nil { return reconcile.Result{}, err } @@ -114,13 +123,3 @@ func (r *ReconcileBuild) Reconcile(ctx context.Context, request reconcile.Reques ctxlog.Debug(ctx, "finishing reconciling Build", namespace, request.Namespace, name, request.Name) return reconcile.Result{}, nil } - -// UpdateBuildStatusAndRetreat returns an error if an update fails, this should force -// a new reconcile until the API call succeeds. If return is nil, no further reconciliations -// will take place -func (r *ReconcileBuild) UpdateBuildStatusAndRetreat(ctx context.Context, b *build.Build) (reconcile.Result, error) { - if err := r.client.Status().Update(ctx, b); err != nil { - return reconcile.Result{}, err - } - return reconcile.Result{}, nil -} diff --git a/pkg/reconciler/build/build_test.go b/pkg/reconciler/build/build_test.go index 5707e07211..d3701a32d3 100644 --- a/pkg/reconciler/build/build_test.go +++ b/pkg/reconciler/build/build_test.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" @@ -55,7 +56,7 @@ var _ = Describe("Reconcile Build", func() { // Fake the client GET calls when reconciling, // in order to get our Build CRD instance client = &fakes.FakeClient{} - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -103,7 +104,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -155,7 +156,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -191,7 +192,7 @@ var _ = Describe("Reconcile Build", func() { It("succeed when the secret exists", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -237,7 +238,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -261,7 +262,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -305,7 +306,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -346,7 +347,7 @@ var _ = Describe("Reconcile Build", func() { It("default to BuildStrategy and succeed if the strategy exists", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -403,7 +404,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -428,7 +429,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -455,7 +456,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -485,7 +486,7 @@ var _ = Describe("Reconcile Build", func() { // Fake some client Get calls and ensure we populate all // different resources we could get during reconciliation - client.GetCalls(func(context context.Context, nn types.NamespacedName, object crc.Object) error { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -592,5 +593,25 @@ var _ = Describe("Reconcile Build", func() { }) }) + + Context("when build object is not in the cluster (anymore)", func() { + It("should finish reconciling when the build cannot be found", func() { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, o crc.Object) error { + return errors.NewNotFound(build.Resource("build"), nn.Name) + }) + + _, err := reconciler.Reconcile(context.TODO(), request) + Expect(err).To(BeNil()) + }) + + It("should finish reconciling with an error when looking up the build fails with an unexpected error", func() { + client.GetCalls(func(_ context.Context, nn types.NamespacedName, o crc.Object) error { + return errors.NewBadRequest("foobar") + }) + + _, err := reconciler.Reconcile(context.TODO(), request) + Expect(err).ToNot(BeNil()) + }) + }) }) }) diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go index 93027aa66b..5611ac8d7a 100644 --- a/pkg/reconciler/buildrun/buildrun.go +++ b/pkg/reconciler/buildrun/buildrun.go @@ -66,7 +66,6 @@ func NewReconciler(c *config.Config, mgr manager.Manager, ownerRef setOwnerRefer // Reconcile reads that state of the cluster for a Build object and makes changes based on the state read // and what is in the Build.Spec func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { - var buildRun *buildv1alpha1.BuildRun var build *buildv1alpha1.Build @@ -98,20 +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 - if updateErr := resources.UpdateConditionWithFalseStatus( - ctx, - r.client, - buildRun, - strings.Join(errs, ", "), - resources.BuildRunNameInvalid, - ); updateErr != nil { - return reconcile.Result{}, updateErr + // 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, + ) } - return reconcile.Result{}, nil + // 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 @@ -122,10 +131,8 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req // for existing TaskRuns update the BuildRun Status, if there is no TaskRun, then create one if getTaskRunErr != nil { if apierrors.IsNotFound(getTaskRunErr) { - build = &buildv1alpha1.Build{} - err := resources.GetBuildObject(ctx, r.client, buildRun, build) - if err != nil { + if err := resources.GetBuildObject(ctx, r.client, buildRun, build); err != nil { if !resources.IsClientStatusUpdateError(err) && buildRun.Status.IsFailed(buildv1alpha1.Succeeded) { return reconcile.Result{}, nil } @@ -135,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 { @@ -183,16 +229,19 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req updateBuildRunRequired = true } - buildGeneration := strconv.FormatInt(build.Generation, 10) - if buildRun.GetLabels()[buildv1alpha1.LabelBuild] != build.Name || buildRun.GetLabels()[buildv1alpha1.LabelBuildGeneration] != buildGeneration { - buildRun.Labels[buildv1alpha1.LabelBuild] = build.Name - buildRun.Labels[buildv1alpha1.LabelBuildGeneration] = buildGeneration - ctxlog.Info(ctx, "updating BuildRun labels", namespace, request.Namespace, name, request.Name) - updateBuildRunRequired = true + // Add missing build name and generation labels to BuildRun (unless it is an embedded build) + if build.Name != "" && build.Generation != 0 { + buildGeneration := strconv.FormatInt(build.Generation, 10) + if buildRun.GetLabels()[buildv1alpha1.LabelBuild] != build.Name || buildRun.GetLabels()[buildv1alpha1.LabelBuildGeneration] != buildGeneration { + buildRun.Labels[buildv1alpha1.LabelBuild] = build.Name + buildRun.Labels[buildv1alpha1.LabelBuildGeneration] = buildGeneration + ctxlog.Info(ctx, "updating BuildRun labels", namespace, request.Namespace, name, request.Name) + updateBuildRunRequired = true + } } if updateBuildRunRequired { - if err = r.client.Update(ctx, buildRun); err != nil { + if err := r.client.Update(ctx, buildRun); err != nil { return reconcile.Result{}, err } ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, request.Namespace, name, request.Name) @@ -201,7 +250,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req // Set the Build spec in the BuildRun status buildRun.Status.BuildSpec = &build.Spec ctxlog.Info(ctx, "updating BuildRun status", namespace, request.Namespace, name, request.Name) - if err = r.client.Status().Update(ctx, buildRun); err != nil { + if err := r.client.Status().Update(ctx, buildRun); err != nil { return reconcile.Result{}, err } @@ -263,7 +312,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req buildmetrics.BuildRunCountInc( buildRun.Status.BuildSpec.StrategyName(), buildRun.Namespace, - buildRun.Spec.BuildRef.Name, + buildRun.Spec.BuildName(), buildRun.Name, ) @@ -271,7 +320,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req buildmetrics.BuildRunRampUpDurationObserve( buildRun.Status.BuildSpec.StrategyName(), buildRun.Namespace, - buildRun.Spec.BuildRef.Name, + buildRun.Spec.BuildName(), buildRun.Name, generatedTaskRun.CreationTimestamp.Time.Sub(buildRun.CreationTimestamp.Time), ) @@ -343,7 +392,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req buildmetrics.BuildRunEstablishObserve( buildRun.Status.BuildSpec.StrategyName(), buildRun.Namespace, - buildRun.Spec.BuildRef.Name, + buildRun.Spec.BuildName(), buildRun.Name, buildRun.Status.StartTime.Time.Sub(buildRun.CreationTimestamp.Time), ) @@ -356,7 +405,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req buildmetrics.BuildRunCompletionObserve( buildRun.Status.BuildSpec.StrategyName(), buildRun.Namespace, - buildRun.Spec.BuildRef.Name, + buildRun.Spec.BuildName(), buildRun.Name, buildRun.Status.CompletionTime.Time.Sub(buildRun.CreationTimestamp.Time), ) @@ -374,7 +423,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req buildmetrics.TaskRunPodRampUpDurationObserve( buildRun.Status.BuildSpec.StrategyName(), buildRun.Namespace, - buildRun.Spec.BuildRef.Name, + buildRun.Spec.BuildName(), buildRun.Name, lastInitPod.State.Terminated.FinishedAt.Sub(pod.CreationTimestamp.Time), ) @@ -385,7 +434,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req buildmetrics.TaskRunRampUpDurationObserve( buildRun.Status.BuildSpec.StrategyName(), buildRun.Namespace, - buildRun.Spec.BuildRef.Name, + buildRun.Spec.BuildName(), buildRun.Name, pod.CreationTimestamp.Time.Sub(lastTaskRun.CreationTimestamp.Time), ) @@ -406,10 +455,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req // GetBuildRunObject retrieves an existing BuildRun based on a name and namespace func (r *ReconcileBuildRun) GetBuildRunObject(ctx context.Context, objectName string, objectNS string, buildRun *buildv1alpha1.BuildRun) error { - if err := r.client.Get(ctx, types.NamespacedName{Name: objectName, Namespace: objectNS}, buildRun); err != nil { - return err - } - return nil + return r.client.Get(ctx, types.NamespacedName{Name: objectName, Namespace: objectNS}, buildRun) } // VerifyRequestName parse a Reconcile request name and looks for an associated BuildRun name diff --git a/pkg/reconciler/buildrun/buildrun_test.go b/pkg/reconciler/buildrun/buildrun_test.go index 0500b4c606..bd7828a033 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" @@ -61,7 +63,7 @@ var _ = Describe("Reconcile BuildRun", func() { // Basic stubs that simulate the output of all client calls in the Reconciler logic. // This applies only for a Build and BuildRun client get. - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -362,7 +364,7 @@ var _ = Describe("Reconcile BuildRun", func() { cancelPatchCalled := false cancelUpdateCalled := false // override the updateClientStub so we can see the update on the BuildRun condition - stubUpdateCalls := func(context context.Context, object crc.Object, opts ...crc.UpdateOption) error { + stubUpdateCalls := func(_ context.Context, object crc.Object, opts ...crc.UpdateOption) error { switch v := object.(type) { case *build.BuildRun: c := v.Status.GetCondition(build.Succeeded) @@ -374,7 +376,7 @@ var _ = Describe("Reconcile BuildRun", func() { return nil } statusWriter.UpdateCalls(stubUpdateCalls) - stubPatchCalls := func(context context.Context, object crc.Object, patch crc.Patch, opts ...crc.PatchOption) error { + stubPatchCalls := func(_ context.Context, object crc.Object, patch crc.Patch, opts ...crc.PatchOption) error { switch v := object.(type) { case *v1beta1.TaskRun: if v.Name == taskRunSample.Name { @@ -547,7 +549,7 @@ var _ = Describe("Reconcile BuildRun", func() { cancelUpdateCalled := false // override the updateClientStub so we can see the update on the BuildRun condition - stubUpdateCalls := func(context context.Context, object crc.Object, opts ...crc.UpdateOption) error { + stubUpdateCalls := func(_ context.Context, object crc.Object, opts ...crc.UpdateOption) error { switch v := object.(type) { case *build.BuildRun: c := v.Status.GetCondition(build.Succeeded) @@ -572,7 +574,7 @@ var _ = Describe("Reconcile BuildRun", func() { // override the initial getClientStub, and generate a new stub // that only contains a Buildrun - stubGetCalls := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + stubGetCalls := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: return k8serrors.NewNotFound(schema.GroupResource{}, nn.Name) @@ -594,7 +596,7 @@ var _ = Describe("Reconcile BuildRun", func() { // override the initial getClientStub, and generate a new stub // that only contains a BuildRun - stubGetCalls := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + stubGetCalls := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: return k8serrors.NewNotFound(schema.GroupResource{}, nn.Name) @@ -622,10 +624,9 @@ var _ = Describe("Reconcile BuildRun", func() { }) It("fails on a TaskRun creation due to service account not found", func() { - // override the initial getClientStub, and generate a new stub // that only contains a Build and Buildrun, none TaskRun - stubGetCalls := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + stubGetCalls := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -661,12 +662,12 @@ var _ = Describe("Reconcile BuildRun", func() { Expect(client.GetCallCount()).To(Equal(4)) Expect(client.StatusCallCount()).To(Equal(2)) }) - It("fails on a TaskRun creation due to issues when retrieving the service account", func() { + It("fails on a TaskRun creation due to issues when retrieving the service account", func() { // override the initial getClientStub, and generate a new stub // that only contains a Build, BuildRun and a random error when // retrieving a service account - stubGetCalls := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + stubGetCalls := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -691,7 +692,6 @@ var _ = Describe("Reconcile BuildRun", func() { }) It("fails on a TaskRun creation due to namespaced buildstrategy not found", func() { - // override the Build to use a namespaced BuildStrategy buildSample = ctl.DefaultBuild(buildName, strategyName, build.NamespacedBuildStrategyKind) @@ -725,12 +725,11 @@ var _ = Describe("Reconcile BuildRun", func() { }) It("fails on a TaskRun creation due to issues when retrieving the buildstrategy", func() { - // override the Build to use a namespaced BuildStrategy buildSample = ctl.DefaultBuild(buildName, strategyName, build.NamespacedBuildStrategyKind) // stub get calls so that on namespaced strategy retrieval, we throw a random error - stubGetCalls := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + stubGetCalls := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -793,12 +792,11 @@ var _ = Describe("Reconcile BuildRun", func() { }) It("fails on a TaskRun creation due to issues when retrieving the clusterbuildstrategy", func() { - // override the Build to use a namespaced BuildStrategy buildSample = ctl.DefaultBuild(buildName, strategyName, build.ClusterBuildStrategyKind) // stub get calls so that on cluster strategy retrieval, we throw a random error - stubGetCalls := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + stubGetCalls := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -973,7 +971,7 @@ var _ = Describe("Reconcile BuildRun", func() { ) // Stub the create calls for a TaskRun - client.CreateCalls(func(context context.Context, object crc.Object, _ ...crc.CreateOption) error { + client.CreateCalls(func(_ context.Context, object crc.Object, _ ...crc.CreateOption) error { switch object := object.(type) { case *v1beta1.TaskRun: ctl.DefaultTaskRunWithStatus(taskRunName, buildRunName, ns, corev1.ConditionTrue, "Succeeded").DeepCopyInto(object) @@ -1002,7 +1000,7 @@ var _ = Describe("Reconcile BuildRun", func() { ) // Stub the create calls for a TaskRun - client.CreateCalls(func(context context.Context, object crc.Object, _ ...crc.CreateOption) error { + client.CreateCalls(func(_ context.Context, object crc.Object, _ ...crc.CreateOption) error { switch object := object.(type) { case *v1beta1.TaskRun: ctl.DefaultTaskRunWithStatus(taskRunName, buildRunName, ns, corev1.ConditionTrue, "Succeeded").DeepCopyInto(object) @@ -1013,10 +1011,11 @@ var _ = Describe("Reconcile BuildRun", func() { _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) Expect(err).ToNot(HaveOccurred()) }) + It("stops creation when a FALSE registered status of the build occurs", func() { // Init the Build with registered status false buildSample = ctl.DefaultBuildWithFalseRegistered(buildName, strategyName, build.ClusterBuildStrategyKind) - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -1084,7 +1083,7 @@ var _ = Describe("Reconcile BuildRun", func() { ) // Stub the create calls for a TaskRun - client.CreateCalls(func(context context.Context, object crc.Object, _ ...crc.CreateOption) error { + client.CreateCalls(func(_ context.Context, object crc.Object, _ ...crc.CreateOption) error { switch object := object.(type) { case *v1beta1.TaskRun: ctl.DefaultTaskRunWithStatus(taskRunName, buildRunName, ns, corev1.ConditionTrue, "Succeeded").DeepCopyInto(object) @@ -1099,7 +1098,6 @@ var _ = Describe("Reconcile BuildRun", func() { }) It("updates Build with error when BuildRun is already owned", func() { - fakeOwnerName := "fakeOwner" // Set the build spec @@ -1216,6 +1214,274 @@ var _ = Describe("Reconcile BuildRun", func() { Expect(err).ToNot(BeNil()) Expect(resources.IsClientStatusUpdateError(err)).To(BeTrue()) }) + + It("should mark buildrun succeeded false when BuildRun name is too long", func() { + buildRunSample = ctl.BuildRunWithoutSA("f"+strings.Repeat("o", 63)+"bar", buildName) + + 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: + condition := buildRun.Status.GetCondition(build.Succeeded) + Expect(condition.Reason).To(Equal(resources.BuildRunNameInvalid)) + Expect(condition.Message).To(Equal("must be no more than 63 characters")) + } + + return nil + }) + + _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(statusWriter.UpdateCallCount()).To(Equal(1)) + }) + + It("should mark buildrun succeeded false when BuildRun name contains illegal runes", func() { + buildRunSample = ctl.BuildRunWithoutSA("fööbar", buildName) + + 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: + condition := buildRun.Status.GetCondition(build.Succeeded) + Expect(condition.Reason).To(Equal(resources.BuildRunNameInvalid)) + Expect(condition.Message).To(ContainSubstring("a valid label must be an empty string or consist of alphanumeric characters")) + } + + return nil + }) + + _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) + Expect(err).To(BeNil()) + Expect(statusWriter.UpdateCallCount()).To(Equal(1)) + }) + + It("should fail the reconcile if an update call failed during a validation error", func() { + buildRunSample = ctl.BuildRunWithoutSA("fööbar", buildName) + + client.GetCalls(ctl.StubBuildRun(buildRunSample)) + statusWriter.UpdateCalls(func(_ context.Context, _ crc.Object, _ ...crc.UpdateOption) error { + return k8serrors.NewInternalError(fmt.Errorf("something bad happened")) + }) + + _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) + Expect(err).To(HaveOccurred()) + Expect(statusWriter.UpdateCallCount()).To(Equal(1)) + }) + }) + + 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) + }) + + statusWriter.UpdateCalls(func(_ context.Context, o crc.Object, _ ...crc.UpdateOption) error { + switch buildRun := o.(type) { + case *build.BuildRun: + Expect(buildRun.Labels).ToNot(HaveKey(build.LabelBuild), "no build name label is suppose to be set") + Expect(buildRun.Labels).ToNot(HaveKey(build.LabelBuildGeneration), "no build generation label is suppose to be set") + return nil + } + + return nil + }) + + var taskRunCreates int + client.CreateCalls(func(_ context.Context, o crc.Object, _ ...crc.CreateOption) error { + switch taskRun := o.(type) { + case *v1beta1.TaskRun: + taskRunCreates++ + + Expect(taskRun.Labels).ToNot(HaveKey(build.LabelBuild), "no build name label is suppose to be set") + Expect(taskRun.Labels).ToNot(HaveKey(build.LabelBuildGeneration), "no build generation label is suppose to be set") + } + + return nil + }) + + _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(statusWriter.UpdateCallCount()).To(Equal(2)) + Expect(taskRunCreates).To(Equal(1)) + }) + }) }) Context("when environment variables are specified", func() { @@ -1233,8 +1499,8 @@ var _ = Describe("Reconcile BuildRun", func() { _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) Expect(err).To(BeNil()) Expect(statusWriter.UpdateCallCount()).To(Equal(1)) - }) + It("fails when the name is blank using valueFrom", func() { buildRunSample.Spec.Env = []corev1.EnvVar{ { @@ -1253,8 +1519,8 @@ var _ = Describe("Reconcile BuildRun", func() { _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) Expect(err).To(BeNil()) Expect(statusWriter.UpdateCallCount()).To(Equal(1)) - }) + It("fails when both value and valueFrom are specified", func() { buildRunSample.Spec.Env = []corev1.EnvVar{ { @@ -1274,8 +1540,8 @@ var _ = Describe("Reconcile BuildRun", func() { _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) Expect(err).To(BeNil()) Expect(statusWriter.UpdateCallCount()).To(Equal(1)) - }) + It("succeeds with compliant env var using Value", func() { buildRunSample.Spec.Env = []corev1.EnvVar{ { @@ -1290,8 +1556,8 @@ var _ = Describe("Reconcile BuildRun", func() { _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) Expect(err).To(BeNil()) Expect(statusWriter.UpdateCallCount()).To(Equal(1)) - }) + It("succeeds with compliant env var using ValueFrom", func() { buildRunSample.Spec.Env = []corev1.EnvVar{ { @@ -1310,7 +1576,6 @@ var _ = Describe("Reconcile BuildRun", func() { _, err := reconciler.Reconcile(context.TODO(), buildRunRequest) Expect(err).To(BeNil()) Expect(statusWriter.UpdateCallCount()).To(Equal(1)) - }) }) }) diff --git a/pkg/reconciler/buildrun/resources/build.go b/pkg/reconciler/buildrun/resources/build.go index cc2b6c9022..72e8e419d1 100644 --- a/pkg/reconciler/buildrun/resources/build.go +++ b/pkg/reconciler/buildrun/resources/build.go @@ -8,17 +8,20 @@ import ( "context" "fmt" - buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + + buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" ) // GetBuildObject retrieves an existing Build based on a name and namespace func GetBuildObject(ctx context.Context, client client.Client, buildRun *buildv1alpha1.BuildRun, build *buildv1alpha1.Build) error { - err := client.Get(ctx, types.NamespacedName{Name: buildRun.Spec.BuildRef.Name, Namespace: buildRun.Namespace}, build) - if err != nil { + // Option #1: BuildRef is specified + // An actual Build resource is specified by name and needs to be looked up in the cluster. + if buildRun.Spec.BuildRef != nil { + err := client.Get(ctx, types.NamespacedName{Name: buildRun.Spec.BuildName(), Namespace: buildRun.Namespace}, build) if apierrors.IsNotFound(err) { // stop reconciling and mark the BuildRun as Failed // we only reconcile again if the status.Update call fails @@ -26,9 +29,22 @@ func GetBuildObject(ctx context.Context, client client.Client, buildRun *buildv1 return HandleError("build object not found", err, updateErr) } } + + return err + } + + // Option #2: BuildSpec is specified + // The build specification is embedded in the BuildRun itself, create a transient Build resource. + if buildRun.Spec.BuildSpec != nil { + build.Name = "" + build.Namespace = buildRun.Namespace + build.Status = buildv1alpha1.BuildStatus{} + buildRun.Spec.BuildSpec.DeepCopyInto(&build.Spec) + return nil } - return err + // Bail out hard in case of an invalid state + return fmt.Errorf("invalid BuildRun resource that neither has a BuildRef nor an embedded BuildSpec") } // IsOwnedByBuild checks if the controllerReferences contains a well known owner Kind diff --git a/pkg/reconciler/buildrun/resources/build_test.go b/pkg/reconciler/buildrun/resources/build_test.go index 295c6a79f9..edbc3eb98f 100644 --- a/pkg/reconciler/buildrun/resources/build_test.go +++ b/pkg/reconciler/buildrun/resources/build_test.go @@ -6,32 +6,32 @@ package resources_test import ( "context" - "errors" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" - "github.com/shipwright-io/build/pkg/controller/fakes" - "github.com/shipwright-io/build/pkg/reconciler/buildrun/resources" - "github.com/shipwright-io/build/test" + + v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" crc "sigs.k8s.io/controller-runtime/pkg/client" + + build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" + "github.com/shipwright-io/build/pkg/controller/fakes" + "github.com/shipwright-io/build/pkg/reconciler/buildrun/resources" + "github.com/shipwright-io/build/test" ) var _ = Describe("Build Resource", func() { - var ( - client *fakes.FakeClient - ctl test.Catalog - buildName string + client *fakes.FakeClient + ctl test.Catalog ) Context("Operating on Build resources", func() { // init vars - buildName = "foobuild" + buildName := "foobuild" client = &fakes.FakeClient{} buildRun := &build.BuildRun{ ObjectMeta: metav1.ObjectMeta{ @@ -39,9 +39,7 @@ var _ = Describe("Build Resource", func() { Namespace: "bar", }, Spec: build.BuildRunSpec{ - // buildRef is a mandatory field, - // therefore we can assume is always present - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, }, @@ -51,7 +49,7 @@ var _ = Describe("Build Resource", func() { buildSample := ctl.DefaultBuild(buildName, "foostrategy", build.ClusterBuildStrategyKind) // stub a GET API call with buildSample contents - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *build.Build: buildSample.DeepCopyInto(object) @@ -66,21 +64,21 @@ var _ = Describe("Build Resource", func() { buildObject := &build.Build{} Expect(resources.GetBuildObject(context.TODO(), client, buildRun, buildObject)).To(BeNil()) }) + It("should not retrieve a missing build object when missing", func() { - // stub a GET API call with buildSample contents that returns "not found" - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { - switch object.(type) { - case *build.Build: - return errors.New("not found") - } + // stub a GET API call that returns "not found" + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object) error { return k8serrors.NewNotFound(schema.GroupResource{}, nn.Name) - } - // fake the calls with the above stub - client.GetCalls(getClientStub) + }) + + client.StatusCalls(func() crc.StatusWriter { + return &fakes.FakeStatusWriter{} + }) build := &build.Build{} Expect(resources.GetBuildObject(context.TODO(), client, buildRun, build)).ToNot(BeNil()) }) + It("should be able to verify valid ownerships", func() { managingController := true @@ -104,6 +102,7 @@ var _ = Describe("Build Resource", func() { // Assert that our Build is owned by an owner Expect(resources.IsOwnedByBuild(buildSample, fakeOwnerRef)).To(BeTrue()) }) + It("should be able to verify invalid ownerships", func() { managingController := true @@ -128,4 +127,30 @@ var _ = Describe("Build Resource", func() { Expect(resources.IsOwnedByBuild(buildSample, fakeOwnerRef)).To(BeFalse()) }) }) + + Context("Operating on embedded Build(Spec) resources", func() { + client = &fakes.FakeClient{} + buildRun := &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + Spec: build.BuildRunSpec{ + BuildSpec: &build.BuildSpec{ + Env: []v1.EnvVar{{Name: "foo", Value: "bar"}}, + }, + }, + } + + It("should be able to retrieve an embedded build object if it exists", func() { + build := &build.Build{} + err := resources.GetBuildObject(context.TODO(), client, buildRun, build) + + Expect(err).To(BeNil()) + Expect(build).ToNot(BeNil()) + Expect(build.Spec).ToNot(BeNil()) + Expect(build.Spec.Env).ToNot(BeNil()) + Expect(build.Spec.Env).To(ContainElement(v1.EnvVar{Name: "foo", Value: "bar"})) + }) + }) }) 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/reconciler/buildrun/resources/conditions_test.go b/pkg/reconciler/buildrun/resources/conditions_test.go index 4f5405fe8c..714f9dad78 100644 --- a/pkg/reconciler/buildrun/resources/conditions_test.go +++ b/pkg/reconciler/buildrun/resources/conditions_test.go @@ -144,7 +144,7 @@ var _ = Describe("Conditions", func() { It("updates BuildRun condition when TaskRun fails and pod not found", func() { // stub a GET API call that fails with not found - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object.(type) { case *corev1.Pod: return k8serrors.NewNotFound(schema.GroupResource{}, nn.Name) @@ -201,7 +201,7 @@ var _ = Describe("Conditions", func() { } // stub a GET API call with taskRunGeneratedPod - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *corev1.Pod: taskRunGeneratedPod.DeepCopyInto(object) @@ -247,7 +247,7 @@ var _ = Describe("Conditions", func() { } // stub a GET API call with to pass the created pod - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *corev1.Pod: failedTaskRunEvictedPod.DeepCopyInto(object) @@ -290,7 +290,7 @@ var _ = Describe("Conditions", func() { } // stub a GET API call with the above taskRunGeneratedPod spec - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *corev1.Pod: taskRunGeneratedPod.DeepCopyInto(object) diff --git a/pkg/reconciler/buildrun/resources/service_accounts.go b/pkg/reconciler/buildrun/resources/service_accounts.go index 43298ca4f2..38cd815f3d 100644 --- a/pkg/reconciler/buildrun/resources/service_accounts.go +++ b/pkg/reconciler/buildrun/resources/service_accounts.go @@ -127,24 +127,18 @@ func getDefaultNamespaceSA(ctx context.Context, client client.Client, buildRun * // RetrieveServiceAccount provides either a default sa with a referenced secret or it will generate a new sa on the fly. // When not using the generate feature, it will modify and return the default sa from a k8s namespace, which is "default" // or the default sa inside an openshift namespace, which is "pipeline". -func RetrieveServiceAccount(ctx context.Context, client client.Client, build *buildv1alpha1.Build, buildRun *buildv1alpha1.BuildRun) (*corev1.ServiceAccount, error) { - - var ( - err error - ) - +func RetrieveServiceAccount(ctx context.Context, client client.Client, build *buildv1alpha1.Build, buildRun *buildv1alpha1.BuildRun) (serviceAccount *corev1.ServiceAccount, err error) { // generate or retrieve an existing autogenerated sa if IsGeneratedServiceAccountUsed(buildRun) { return GenerateSA(ctx, client, build, buildRun) } - serviceAccount := &corev1.ServiceAccount{} - if buildRun.Spec.ServiceAccount != nil && buildRun.Spec.ServiceAccount.Name != nil { serviceAccountName := *buildRun.Spec.ServiceAccount.Name - err := client.Get(ctx, types.NamespacedName{Name: serviceAccountName, Namespace: buildRun.Namespace}, serviceAccount) + // here we might need to update Status Conditions and Fail the BR - if err != nil { + serviceAccount = &corev1.ServiceAccount{} + if err = client.Get(ctx, types.NamespacedName{Name: serviceAccountName, Namespace: buildRun.Namespace}, serviceAccount); err != nil { if apierrors.IsNotFound(err) { if updateErr := UpdateConditionWithFalseStatus(ctx, client, buildRun, fmt.Sprintf("service account %s not found", serviceAccountName), ConditionServiceAccountNotFound); updateErr != nil { return nil, HandleError("failed to retrieve service account", err, updateErr) @@ -153,6 +147,7 @@ func RetrieveServiceAccount(ctx context.Context, client client.Client, build *bu return nil, err } + } else { // we default to pipeline/default sa serviceAccount, err = getDefaultNamespaceSA(ctx, client, buildRun) diff --git a/pkg/reconciler/buildrun/resources/service_accounts_test.go b/pkg/reconciler/buildrun/resources/service_accounts_test.go index 92c14fcc1d..0573443586 100644 --- a/pkg/reconciler/buildrun/resources/service_accounts_test.go +++ b/pkg/reconciler/buildrun/resources/service_accounts_test.go @@ -39,7 +39,7 @@ var _ = Describe("Operating service accounts", func() { // stub client GET calls and return a initialized sa when asking for a sa var generateGetSAStub = func(saName string) func(context context.Context, nn types.NamespacedName, object crc.Object) error { - return func(context context.Context, nn types.NamespacedName, object crc.Object) error { + return func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *corev1.ServiceAccount: ctl.DefaultServiceAccount(saName).DeepCopyInto(object) @@ -51,7 +51,7 @@ var _ = Describe("Operating service accounts", func() { // stub client GET calls and return an error when asking for a service account var generateGetSAStubWithError = func(customError error) func(context context.Context, nn types.NamespacedName, object crc.Object) error { - return func(context context.Context, nn types.NamespacedName, object crc.Object) error { + return func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object.(type) { case *corev1.ServiceAccount: return customError @@ -111,7 +111,7 @@ var _ = Describe("Operating service accounts", func() { client.StatusCalls(func() crc.StatusWriter { statusWriter := &fakes.FakeStatusWriter{} - statusWriter.UpdateCalls(func(ctx context.Context, object crc.Object, _ ...crc.UpdateOption) error { + statusWriter.UpdateCalls(func(_ context.Context, object crc.Object, _ ...crc.UpdateOption) error { switch object.(type) { case *buildv1alpha1.BuildRun: return fmt.Errorf("failed") @@ -142,7 +142,7 @@ var _ = Describe("Operating service accounts", func() { mountTokenVal := false - client.CreateCalls(func(context context.Context, object crc.Object, _ ...crc.CreateOption) error { + client.CreateCalls(func(_ context.Context, object crc.Object, _ ...crc.CreateOption) error { switch object := object.(type) { case *corev1.ServiceAccount: Expect(len(object.Secrets)).To(Equal(1)) diff --git a/pkg/reconciler/buildrun/resources/strategies_test.go b/pkg/reconciler/buildrun/resources/strategies_test.go index c2fdf4d04e..c82567f8b5 100644 --- a/pkg/reconciler/buildrun/resources/strategies_test.go +++ b/pkg/reconciler/buildrun/resources/strategies_test.go @@ -31,7 +31,7 @@ var _ = Describe("Operating Build strategies", func() { It("should return a cluster buildstrategy", func() { // stub a GET API call with a cluster strategy - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *buildv1alpha1.ClusterBuildStrategy: ctl.DefaultClusterBuildStrategy().DeepCopyInto(object) @@ -49,7 +49,7 @@ var _ = Describe("Operating Build strategies", func() { It("should return a namespaced buildstrategy", func() { // stub a GET API call with a namespace strategy - getClientStub := func(context context.Context, nn types.NamespacedName, object crc.Object) error { + getClientStub := func(_ context.Context, nn types.NamespacedName, object crc.Object) error { switch object := object.(type) { case *buildv1alpha1.BuildStrategy: ctl.DefaultNamespacedBuildStrategy().DeepCopyInto(object) diff --git a/pkg/reconciler/buildrun/resources/taskrun.go b/pkg/reconciler/buildrun/resources/taskrun.go index d791a6b6b1..cbfd791030 100644 --- a/pkg/reconciler/buildrun/resources/taskrun.go +++ b/pkg/reconciler/buildrun/resources/taskrun.go @@ -267,16 +267,23 @@ func GenerateTaskRun( return nil, err } + // Add BuildRun name reference to the TaskRun labels + taskRunLabels := map[string]string{ + buildv1alpha1.LabelBuildRun: buildRun.Name, + buildv1alpha1.LabelBuildRunGeneration: strconv.FormatInt(buildRun.Generation, 10), + } + + // Add Build name reference unless it is an embedded Build (empty build name) + if build.Name != "" { + taskRunLabels[buildv1alpha1.LabelBuild] = build.Name + taskRunLabels[buildv1alpha1.LabelBuildGeneration] = strconv.FormatInt(build.Generation, 10) + } + expectedTaskRun := &v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ GenerateName: buildRun.Name + "-", Namespace: buildRun.Namespace, - Labels: map[string]string{ - buildv1alpha1.LabelBuild: build.Name, - buildv1alpha1.LabelBuildGeneration: strconv.FormatInt(build.Generation, 10), - buildv1alpha1.LabelBuildRun: buildRun.Name, - buildv1alpha1.LabelBuildRunGeneration: strconv.FormatInt(buildRun.Generation, 10), - }, + Labels: taskRunLabels, }, Spec: v1beta1.TaskRunSpec{ ServiceAccountName: serviceAccountName, diff --git a/pkg/validate/buildname.go b/pkg/validate/buildname.go index 51f58f9d85..1eaa83c1f5 100644 --- a/pkg/validate/buildname.go +++ b/pkg/validate/buildname.go @@ -20,6 +20,10 @@ type BuildNameRef struct { Build *build.Build // build instance for analysis } +func NewBuildName(build *build.Build) *BuildNameRef { + return &BuildNameRef{build} +} + // ValidatePath implements BuildPath interface and validates // that build name is a valid label value func (b *BuildNameRef) ValidatePath(_ context.Context) error { diff --git a/pkg/validate/secrets.go b/pkg/validate/secrets.go index cdbffd308e..5e3692e0c6 100644 --- a/pkg/validate/secrets.go +++ b/pkg/validate/secrets.go @@ -26,6 +26,10 @@ type Credentials struct { Client client.Client } +func NewCredentials(client client.Client, build *build.Build) *Credentials { + return &Credentials{build, client} +} + // ValidatePath implements BuildPath interface and validates // that all referenced secrets under spec exists func (s Credentials) ValidatePath(ctx context.Context) error { diff --git a/pkg/validate/sourceurl.go b/pkg/validate/sourceurl.go index a084d36137..c724d423b7 100644 --- a/pkg/validate/sourceurl.go +++ b/pkg/validate/sourceurl.go @@ -23,6 +23,10 @@ type SourceURLRef struct { Client client.Client } +func NewSourceURL(client client.Client, build *build.Build) *SourceURLRef { + return &SourceURLRef{build, client} +} + // ValidatePath implements BuildPath interface and validates // that the spec.source.url exists. This validation only applies // to endpoints that do not require authentication. diff --git a/pkg/validate/strategies.go b/pkg/validate/strategies.go index 7c0483c8bb..107503b408 100644 --- a/pkg/validate/strategies.go +++ b/pkg/validate/strategies.go @@ -24,6 +24,10 @@ type Strategy struct { Client client.Client } +func NewStrategies(client client.Client, build *build.Build) *Strategy { + return &Strategy{build, client} +} + // ValidatePath implements BuildPath interface and validates // that the referenced strategy exists. This applies to both // namespaced or cluster scoped strategies diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index 1d392a5495..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 ( @@ -67,3 +68,52 @@ func NewValidation( return nil, fmt.Errorf("unknown validation type") } } + +// 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 + } + } + + 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/catalog.go b/test/catalog.go index b7d967bec0..32d48e2765 100644 --- a/test/catalog.go +++ b/test/catalog.go @@ -758,7 +758,7 @@ func (c *Catalog) DefaultBuildRun(buildRunName string, buildName string) *build. Name: buildRunName, }, Spec: build.BuildRunSpec{ - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, }, @@ -803,7 +803,7 @@ func (c *Catalog) BuildRunWithBuildSnapshot(buildRunName string, buildName strin }, }, Spec: build.BuildRunSpec{ - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, }, @@ -828,7 +828,7 @@ func (c *Catalog) BuildRunWithExistingOwnerReferences(buildRunName string, build OwnerReferences: []metav1.OwnerReference{fakeOwnerRef}, }, Spec: build.BuildRunSpec{ - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, }, @@ -844,7 +844,7 @@ func (c *Catalog) BuildRunWithFakeNamespace(buildRunName string, buildName strin Namespace: "foobarns", }, Spec: build.BuildRunSpec{ - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, }, @@ -933,7 +933,7 @@ func (c *Catalog) BuildRunWithSA(buildRunName string, buildName string, saName s Name: buildRunName, }, Spec: build.BuildRunSpec{ - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, ServiceAccount: &build.ServiceAccount{ @@ -952,7 +952,7 @@ func (c *Catalog) BuildRunWithoutSA(buildRunName string, buildName string) *buil Name: buildRunName, }, Spec: build.BuildRunSpec{ - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, ServiceAccount: &build.ServiceAccount{ @@ -970,7 +970,7 @@ func (c *Catalog) BuildRunWithSAGenerate(buildRunName string, buildName string) Name: buildRunName, }, Spec: build.BuildRunSpec{ - BuildRef: build.BuildRef{ + BuildRef: &build.BuildRef{ Name: buildName, }, ServiceAccount: &build.ServiceAccount{ @@ -1028,14 +1028,14 @@ func (c *Catalog) LoadBuildRunFromBytes(d []byte) (*build.BuildRun, error) { } // LoadBRWithNameAndRef returns a populated BuildRun with a name and a referenced Build -func (c *Catalog) LoadBRWithNameAndRef(name string, buildRef string, d []byte) (*build.BuildRun, error) { +func (c *Catalog) LoadBRWithNameAndRef(name string, buildName string, d []byte) (*build.BuildRun, error) { b := &build.BuildRun{} err := yaml.Unmarshal(d, b) if err != nil { return nil, err } b.Name = name - b.Spec.BuildRef.Name = buildRef + b.Spec.BuildRef = &build.BuildRef{Name: buildName} return b, nil } diff --git a/test/e2e/common_suite_test.go b/test/e2e/common_suite_test.go index d9a19c29ea..c6ffb6f314 100644 --- a/test/e2e/common_suite_test.go +++ b/test/e2e/common_suite_test.go @@ -15,7 +15,6 @@ import ( . "github.com/onsi/ginkgo/v2" core "k8s.io/api/core/v1" - v1 "k8s.io/api/core/v1" meta "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/pointer" @@ -67,7 +66,7 @@ func (b *buildPrototype) ClusterBuildStrategy(name string) *buildPrototype { func (b *buildPrototype) SourceCredentials(name string) *buildPrototype { if name != "" { - b.build.Spec.Source.Credentials = &v1.LocalObjectReference{Name: name} + b.build.Spec.Source.Credentials = &core.LocalObjectReference{Name: name} } return b @@ -235,12 +234,17 @@ func (b buildPrototype) Create() (build *buildv1alpha1.Build, err error) { return false, err } - return build.Status.Registered != nil && *build.Status.Registered == v1.ConditionTrue, nil + return build.Status.Registered != nil && *build.Status.Registered == core.ConditionTrue, nil }) return } +// BuildSpec returns the BuildSpec of this Build (no cluster resource is created) +func (b buildPrototype) BuildSpec() (build *buildv1alpha1.BuildSpec) { + return &b.build.Spec +} + func NewBuildRunPrototype() *buildRunPrototype { return &buildRunPrototype{buildRun: buildv1alpha1.BuildRun{}} } @@ -250,12 +254,22 @@ func (b *buildRunPrototype) Name(name string) *buildRunPrototype { return b } +func (b *buildRunPrototype) Namespace(namespace string) *buildRunPrototype { + b.buildRun.Namespace = namespace + return b +} + func (b *buildRunPrototype) ForBuild(build *buildv1alpha1.Build) *buildRunPrototype { - b.buildRun.Spec.BuildRef = buildv1alpha1.BuildRef{Name: build.Name} + b.buildRun.Spec.BuildRef = &buildv1alpha1.BuildRef{Name: build.Name} b.buildRun.ObjectMeta.Namespace = build.Namespace return b } +func (b *buildRunPrototype) WithBuildSpec(buildSpec *buildv1alpha1.BuildSpec) *buildRunPrototype { + b.buildRun.Spec.BuildSpec = buildSpec + return b +} + func (b *buildRunPrototype) GenerateServiceAccount() *buildRunPrototype { if b.buildRun.Spec.ServiceAccount == nil { b.buildRun.Spec.ServiceAccount = &buildv1alpha1.ServiceAccount{} diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go index f5ab444e43..ebd6b6b1d4 100644 --- a/test/e2e/common_test.go +++ b/test/e2e/common_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/util/rand" buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" + "github.com/shipwright-io/build/pkg/reconciler/buildrun/resources" "github.com/shipwright-io/build/test/utils" ) @@ -130,15 +131,13 @@ func retrieveBuildAndBuildRun(testBuild *utils.TestBuild, namespace string, buil return nil, nil, err } - buildName := buildRun.Spec.BuildRef.Name - - build, err := testBuild.LookupBuild(types.NamespacedName{Name: buildName, Namespace: namespace}) - if err != nil { - Logf("Failed to get Build %s: %s", buildName, err) + var build buildv1alpha1.Build + if err := resources.GetBuildObject(testBuild.Context, testBuild.ControllerRuntimeClient, buildRun, &build); err != nil { + Logf("Failed to get Build from BuildRun %s: %s", buildRunName, err) return nil, buildRun, err } - return build, buildRun, nil + return &build, buildRun, nil } // printTestFailureDebugInfo will output the status of Build, BuildRun, TaskRun and Pod, also print logs of Pod diff --git a/test/e2e/e2e_bundle_test.go b/test/e2e/e2e_bundle_test.go index 83f97676f9..7eafeeb6e3 100644 --- a/test/e2e/e2e_bundle_test.go +++ b/test/e2e/e2e_bundle_test.go @@ -22,7 +22,7 @@ import ( buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" ) -var _ = Describe("For a Kubernetes cluster with Tekton and build installed", func() { +var _ = Describe("Test local source code (bundle) functionality", func() { var ( testID string err error 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) + }) + }) +}) diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index dbde65f3c8..ff8448cf6a 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -35,7 +35,8 @@ var ( Expect(err).ToNot(HaveOccurred()) testBuild.Namespace, ok = os.LookupEnv("TEST_NAMESPACE") - Expect(ok).To(BeTrue()) + Expect(ok).To(BeTrue(), "TEST_NAMESPACE should be set") + Expect(testBuild.Namespace).ToNot(BeEmpty()) // create the pipeline service account Logf("Creating the pipeline service account") diff --git a/test/e2e/validators_test.go b/test/e2e/validators_test.go index 3e1f3c8a3a..cc837030db 100644 --- a/test/e2e/validators_test.go +++ b/test/e2e/validators_test.go @@ -44,7 +44,10 @@ const ( // createPipelineServiceAccount reads the TEST_E2E_SERVICEACCOUNT_NAME environment variable. If the value is "generated", then nothing is done. // Otherwise it will create the service account. No error occurs if the service account already exists. func createPipelineServiceAccount(testBuild *utils.TestBuild) { - serviceAccountName := os.Getenv(EnvVarServiceAccountName) + serviceAccountName, ok := os.LookupEnv(EnvVarServiceAccountName) + Expect(ok).To(BeTrue(), "environment variable "+EnvVarServiceAccountName+" is not set") + Expect(serviceAccountName).ToNot(BeEmpty()) + if serviceAccountName == "generated" { Logf("Skipping creation of service account, generated one will be used per build run.") return @@ -329,7 +332,7 @@ func buildRunTestData(ns string, identifier string, filePath string) (*buildv1al buildRun.SetNamespace(ns) buildRun.SetName(identifier) - buildRun.Spec.BuildRef.Name = identifier + buildRun.Spec.BuildRef = &buildv1alpha1.BuildRef{Name: identifier} serviceAccountName := os.Getenv(EnvVarServiceAccountName) if serviceAccountName == "generated" { diff --git a/test/utils/environment.go b/test/utils/environment.go index 58359dbe7c..b38d0e8d69 100644 --- a/test/utils/environment.go +++ b/test/utils/environment.go @@ -20,6 +20,7 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth/oidc" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" + "sigs.k8s.io/controller-runtime/pkg/client" buildClient "github.com/shipwright-io/build/pkg/client/clientset/versioned" "github.com/shipwright-io/build/pkg/ctxlog" @@ -46,6 +47,7 @@ type TestBuild struct { StopBuildControllers context.CancelFunc BuildClientSet *buildClient.Clientset PipelineClientSet *tektonClient.Clientset + ControllerRuntimeClient client.Client Catalog test.Catalog Context context.Context BuildControllerLogBuffer *bytes.Buffer @@ -78,6 +80,11 @@ func NewTestBuild() (*TestBuild, error) { return nil, err } + controllerRuntimeClient, err := client.New(restConfig, client.Options{}) + if err != nil { + return nil, err + } + ctx, cancelFn := context.WithCancel(ctx) return &TestBuild{ @@ -89,6 +96,7 @@ func NewTestBuild() (*TestBuild, error) { Namespace: testNamespace, BuildClientSet: buildClientSet, PipelineClientSet: pipelineClientSet, + ControllerRuntimeClient: controllerRuntimeClient, Context: ctx, BuildControllerLogBuffer: logBuffer, StopBuildControllers: cancelFn, diff --git a/test/utils/lookup.go b/test/utils/lookup.go index 432b8fb8d6..a0220b7bed 100644 --- a/test/utils/lookup.go +++ b/test/utils/lookup.go @@ -88,7 +88,6 @@ func (t *TestBuild) LookupTaskRunUsingBuildRun(buildRun *buildv1alpha1.BuildRun) List(t.Context, metav1.ListOptions{ LabelSelector: labels.SelectorFromSet( map[string]string{ - buildv1alpha1.LabelBuild: buildRun.Spec.BuildRef.Name, buildv1alpha1.LabelBuildRun: buildRun.Name, }).String(), })