From ed03a7a355a1209d416b10694b7c249f12c8bcc7 Mon Sep 17 00:00:00 2001 From: dlorenc Date: Fri, 12 Oct 2018 12:53:19 -0700 Subject: [PATCH] Implement Task/TaskRun Input Parameters. This adds support for Task/TaskRun Input Parameters using the 'ApplyReplacements' function from knative/build. Reusing this function ensures consistency in variable replacement between the projects. This change assumes that the supplied TaskRun.Spec.Inputs.Params have been valided against the Task.Inputs.Params elsewhere. We may want to revisit this and perform additional validation. This change does not introduce support for Resources or PipelineParams yet. --- Gopkg.lock | 7 +- Gopkg.toml | 2 +- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 20 +- .../v1alpha1/taskrun/taskrun_test.go | 204 +++++++++++++----- .../knative/build/pkg/builder/common.go | 121 +++++++++++ .../knative/build/pkg/builder/interface.go | 116 ++++++++++ 6 files changed, 417 insertions(+), 53 deletions(-) create mode 100644 vendor/github.com/knative/build/pkg/builder/common.go create mode 100644 vendor/github.com/knative/build/pkg/builder/interface.go diff --git a/Gopkg.lock b/Gopkg.lock index 5bb65196ff2..0d743231051 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -137,11 +137,12 @@ revision = "f2b4162afba35581b6d4a50d3b8f34e33c144682" [[projects]] - digest = "1:1626441bf44173cc41161cbdc5cbf42309aa9488d282cfff5a00e8ea8b7eecb5" + digest = "1:7d4fb1e9f425e206c3f491edf55dea318080d4c7a2bd8d9bfb113f0bdd1ba90e" name = "github.com/knative/build" packages = [ "pkg/apis/build", "pkg/apis/build/v1alpha1", + "pkg/builder", "pkg/client/clientset/versioned", "pkg/client/clientset/versioned/fake", "pkg/client/clientset/versioned/scheme", @@ -154,7 +155,7 @@ "pkg/client/listers/build/v1alpha1", ] pruneopts = "NUT" - revision = "7360ec685b35802d4703b31e6c0be2cc04b4503c" + revision = "92a1258647b272a3a4505616b102d6e8f86be082" [[projects]] branch = "master" @@ -741,6 +742,7 @@ input-imports = [ "github.com/google/go-cmp/cmp", "github.com/knative/build/pkg/apis/build/v1alpha1", + "github.com/knative/build/pkg/builder", "github.com/knative/build/pkg/client/clientset/versioned", "github.com/knative/build/pkg/client/clientset/versioned/fake", "github.com/knative/build/pkg/client/clientset/versioned/typed/build/v1alpha1", @@ -768,6 +770,7 @@ "k8s.io/api/core/v1", "k8s.io/apimachinery/pkg/api/equality", "k8s.io/apimachinery/pkg/api/errors", + "k8s.io/apimachinery/pkg/api/resource", "k8s.io/apimachinery/pkg/apis/meta/v1", "k8s.io/apimachinery/pkg/labels", "k8s.io/apimachinery/pkg/runtime", diff --git a/Gopkg.toml b/Gopkg.toml index 07f983f3f8a..8d2c2a12b3c 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -62,7 +62,7 @@ required = [ [[constraint]] name = "github.com/knative/build" # HEAD as of 2018-10-09 - revision = "7360ec685b35802d4703b31e6c0be2cc04b4503c" + revision = "92a1258647b272a3a4505616b102d6e8f86be082" [prune] go-tests = true diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index a8cd6ca1680..a8c4e334d02 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -20,6 +20,8 @@ import ( "fmt" "reflect" + "github.com/knative/build/pkg/builder" + "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" "github.com/knative/build-pipeline/pkg/reconciler" resources "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources" @@ -242,6 +244,9 @@ func (c *Reconciler) makeBuild(tr *v1alpha1.TaskRun, logger *zap.SugaredLogger) return nil, err } + // Apply parameters from the taskrun. + build = applyTemplates(b, t, tr) + createdBuild, err := c.BuildClientSet.BuildV1alpha1().Builds(tr.Namespace).Create(build) if err != nil { logger.Errorf("Failed to create build for taskrun %s, %v", tr.Name, err) @@ -258,6 +263,19 @@ func (c *Reconciler) makeBuild(tr *v1alpha1.TaskRun, logger *zap.SugaredLogger) logger.Errorf("Failed to create tracker for build %s for taskrun %s: %v", buildRef, tr.Name, err) return nil, err } - return createdBuild, nil } + +func applyTemplates(b *buildv1alpha1.Build, t *v1alpha1.Task, tr *v1alpha1.TaskRun) *buildv1alpha1.Build { + // This assumes that the TaskRun inputs have been validated against what the Task requests. + replacements := map[string]string{} + for _, p := range tr.Spec.Inputs.Params { + replacements[fmt.Sprintf("inputs.params.%s", p.Name)] = p.Value + } + + return builder.ApplyReplacements(b, replacements) +} + +func (c *Reconciler) now() metav1.Time { + return metav1.Now() +} diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index c71237e6f46..fae5f776948 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -18,6 +18,8 @@ import ( "testing" "time" + corev1 "k8s.io/api/core/v1" + "github.com/google/go-cmp/cmp" "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" fakepipelineclientset "github.com/knative/build-pipeline/pkg/client/clientset/versioned/fake" @@ -26,7 +28,6 @@ import ( buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" fakebuildclientset "github.com/knative/build/pkg/client/clientset/versioned/fake" buildinformers "github.com/knative/build/pkg/client/informers/externalversions" - "github.com/knative/pkg/controller" "go.uber.org/zap" "go.uber.org/zap/zaptest/observer" @@ -35,8 +36,49 @@ import ( ktesting "k8s.io/client-go/testing" ) -func TestReconcile(t *testing.T) { - taskname := "test-task" +var kanikoTask = &v1alpha1.Task{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-task", + Namespace: "foo", + }, + Spec: v1alpha1.TaskSpec{ + BuildSpec: &buildv1alpha1.BuildSpec{ + Template: &buildv1alpha1.TemplateInstantiationSpec{ + Name: "kaniko", + Arguments: []buildv1alpha1.ArgumentSpec{ + buildv1alpha1.ArgumentSpec{ + Name: "DOCKERFILE", + Value: "${PATH_TO_DOCKERFILE}", + }, + buildv1alpha1.ArgumentSpec{ + Name: "REGISTRY", + Value: "${REGISTRY}", + }, + }, + }, + }, + }, +} + +var templatedTask = &v1alpha1.Task{ + ObjectMeta: metav1.ObjectMeta{ + Name: "task-with-templating", + Namespace: "foo", + }, + Spec: v1alpha1.TaskSpec{ + BuildSpec: &buildv1alpha1.BuildSpec{ + Steps: []corev1.Container{ + { + Name: "mycontainer", + Image: "myimage", + Args: []string{"--my-arg=${inputs.params.myarg}"}, + }, + }, + }, + }, +} + +func TestReconcileBuildsCreated(t *testing.T) { taskruns := []*v1alpha1.TaskRun{ { ObjectMeta: metav1.ObjectMeta{ @@ -45,77 +87,141 @@ func TestReconcile(t *testing.T) { }, Spec: v1alpha1.TaskRunSpec{ TaskRef: v1alpha1.TaskRef{ - Name: taskname, + Name: "test-task", + APIVersion: "a1", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-templating", + Namespace: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + TaskRef: v1alpha1.TaskRef{ + Name: "task-with-templating", APIVersion: "a1", }, - }}, + Inputs: v1alpha1.TaskRunInputs{ + Params: []v1alpha1.Param{ + { + Name: "myarg", + Value: "foo", + }, + }, + }, + }, + }, } - buildSpec := buildv1alpha1.BuildSpec{ - Template: &buildv1alpha1.TemplateInstantiationSpec{ - Name: "kaniko", - Arguments: []buildv1alpha1.ArgumentSpec{ - buildv1alpha1.ArgumentSpec{ - Name: "DOCKERFILE", - Value: "${PATH_TO_DOCKERFILE}", + d := testData{ + taskruns: taskruns, + tasks: []*v1alpha1.Task{kanikoTask, templatedTask}, + } + testcases := []struct { + name string + taskRun string + wantedBuildSpec buildv1alpha1.BuildSpec + }{ + { + name: "success", + taskRun: "foo/test-taskrun-run-success", + wantedBuildSpec: buildv1alpha1.BuildSpec{ + Template: &buildv1alpha1.TemplateInstantiationSpec{ + Name: "kaniko", + Arguments: []buildv1alpha1.ArgumentSpec{ + buildv1alpha1.ArgumentSpec{ + Name: "DOCKERFILE", + Value: "${PATH_TO_DOCKERFILE}", + }, + buildv1alpha1.ArgumentSpec{ + Name: "REGISTRY", + Value: "${REGISTRY}", + }, + }, }, - buildv1alpha1.ArgumentSpec{ - Name: "REGISTRY", - Value: "${REGISTRY}", + }, + }, + { + name: "params", + taskRun: "foo/test-taskrun-templating", + wantedBuildSpec: buildv1alpha1.BuildSpec{ + Steps: []corev1.Container{ + { + Name: "mycontainer", + Image: "myimage", + Args: []string{"--my-arg=foo"}, + }, }, - }}, + }, + }, } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + c, _, client := getController(d) + if err := c.Reconciler.Reconcile(context.Background(), tc.taskRun); err != nil { + t.Errorf("expected no error. Got error %v", err) + } - tasks := []*v1alpha1.Task{ - { + if len(client.Actions()) == 0 { + t.Errorf("Expected actions to be logged in the buildclient, got none") + } + build := client.Actions()[0].(ktesting.CreateAction).GetObject().(*buildv1alpha1.Build) + if d := cmp.Diff(build.Spec, tc.wantedBuildSpec); d != "" { + t.Errorf("buildspec doesn't match, diff: %s", d) + } + }) + } +} + +func TestReconcileBuildCreationErrors(t *testing.T) { + taskRuns := []*v1alpha1.TaskRun{ + &v1alpha1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ - Name: taskname, + Name: "notaskrun", Namespace: "foo", }, - Spec: v1alpha1.TaskSpec{ - BuildSpec: &buildSpec, - }}, + Spec: v1alpha1.TaskRunSpec{ + TaskRef: v1alpha1.TaskRef{ + Name: "notask", + APIVersion: "a1", + }, + }, + }, + } + + tasks := []*v1alpha1.Task{ + kanikoTask, } + d := testData{ - taskruns: taskruns, + taskruns: taskRuns, tasks: tasks, } + testcases := []struct { - name string - taskRun string - shdErr bool - shdMakebuild bool - log string + name string + taskRun string }{ - {"success", "foo/test-taskrun-run-success", false, true, ""}, + { + name: "task run with no task", + taskRun: "foo/notaskrun", + }, } + for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - c, logs, client := getController(d) - err := c.Reconciler.Reconcile(context.Background(), tc.taskRun) - if tc.shdErr != (err != nil) { - t.Errorf("expected to see error %t. Got error %v", tc.shdErr, err) + c, _, client := getController(d) + if err := c.Reconciler.Reconcile(context.Background(), tc.taskRun); err == nil { + t.Error("Expected reconcile to error. Got nil") } - if tc.log == "" && logs.Len() > 0 { - t.Errorf("expected to see no error log. However found errors in logs: %v", logs) - } else if tc.log != "" && logs.FilterMessage(tc.log).Len() == 0 { - m := getLogMessages(logs) - t.Errorf("Log lines diff %s", cmp.Diff(tc.log, m)) - } else if tc.shdMakebuild { - if err == nil { - if len(client.Actions()) == 0 { - t.Errorf("Expected actions to be logged in the buildclient, got none") - } - build := client.Actions()[0].(ktesting.CreateAction).GetObject().(*buildv1alpha1.Build) - if d := cmp.Diff(build.Spec, buildSpec); d != "" { - t.Errorf("Expected created resource to be %v, but was %v", build.Spec, buildSpec) - } - } + if len(client.Actions()) != 0 { + t.Errorf("expected no actions to be created by the reconciler, got %v", client.Actions()) } }) } -} +} func getController(d testData) (*controller.Impl, *observer.ObservedLogs, *fakebuildclientset.Clientset) { pipelineClient := fakepipelineclientset.NewSimpleClientset() buildClient := fakebuildclientset.NewSimpleClientset() diff --git a/vendor/github.com/knative/build/pkg/builder/common.go b/vendor/github.com/knative/build/pkg/builder/common.go new file mode 100644 index 00000000000..ba515472577 --- /dev/null +++ b/vendor/github.com/knative/build/pkg/builder/common.go @@ -0,0 +1,121 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package builder provides common methods for Builder implementations. +package builder + +import ( + "fmt" + "strings" + + corev1 "k8s.io/api/core/v1" + + "github.com/knative/build/pkg/apis/build/v1alpha1" +) + +// ApplyTemplate applies the values in the template to the build, and replaces +// placeholders for declared parameters with the build's matching arguments. +func ApplyTemplate(u *v1alpha1.Build, tmpl v1alpha1.BuildTemplateInterface) (*v1alpha1.Build, error) { + build := u.DeepCopy() + if tmpl == nil { + return build, nil + } + tmpl = tmpl.Copy() + build.Spec.Steps = tmpl.TemplateSpec().Steps + build.Spec.Volumes = append(build.Spec.Volumes, tmpl.TemplateSpec().Volumes...) + + // Apply template arguments or parameter defaults. + replacements := map[string]string{} + if tmpl != nil { + for _, p := range tmpl.TemplateSpec().Parameters { + if p.Default != nil { + replacements[p.Name] = *p.Default + } + } + } + if build.Spec.Template != nil { + for _, a := range build.Spec.Template.Arguments { + replacements[a.Name] = a.Value + } + } + + build = ApplyReplacements(build, replacements) + return build, nil +} + +// ApplyReplacements replaces placeholders for declared parameters with the specified replacements. +func ApplyReplacements(build *v1alpha1.Build, replacements map[string]string) *v1alpha1.Build { + build = build.DeepCopy() + + applyReplacements := func(in string) string { + for k, v := range replacements { + in = strings.Replace(in, fmt.Sprintf("${%s}", k), v, -1) + } + return in + } + + // Apply variable expansion to steps fields. + steps := build.Spec.Steps + for i := range steps { + steps[i].Name = applyReplacements(steps[i].Name) + steps[i].Image = applyReplacements(steps[i].Image) + for ia, a := range steps[i].Args { + steps[i].Args[ia] = applyReplacements(a) + } + for ie, e := range steps[i].Env { + steps[i].Env[ie].Value = applyReplacements(e.Value) + } + steps[i].WorkingDir = applyReplacements(steps[i].WorkingDir) + for ic, c := range steps[i].Command { + steps[i].Command[ic] = applyReplacements(c) + } + for iv, v := range steps[i].VolumeMounts { + steps[i].VolumeMounts[iv].Name = applyReplacements(v.Name) + steps[i].VolumeMounts[iv].MountPath = applyReplacements(v.MountPath) + steps[i].VolumeMounts[iv].SubPath = applyReplacements(v.SubPath) + } + } + + if buildTmpl := build.Spec.Template; buildTmpl != nil && len(buildTmpl.Env) > 0 { + // Apply variable expansion to the build's overridden + // environment variables + for i, e := range buildTmpl.Env { + buildTmpl.Env[i].Value = applyReplacements(e.Value) + } + + for i := range steps { + steps[i].Env = applyEnvOverride(steps[i].Env, buildTmpl.Env) + } + } + return build +} + +func applyEnvOverride(src, override []corev1.EnvVar) []corev1.EnvVar { + result := make([]corev1.EnvVar, 0, len(src)+len(override)) + overrides := make(map[string]bool) + + for _, env := range override { + overrides[env.Name] = true + } + + for _, env := range src { + if _, present := overrides[env.Name]; !present { + result = append(result, env) + } + } + + return append(result, override...) +} diff --git a/vendor/github.com/knative/build/pkg/builder/interface.go b/vendor/github.com/knative/build/pkg/builder/interface.go new file mode 100644 index 00000000000..96555d09ae2 --- /dev/null +++ b/vendor/github.com/knative/build/pkg/builder/interface.go @@ -0,0 +1,116 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package builder + +import ( + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + v1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" +) + +// Operation defines the interface for interacting with an Operation of a particular BuildProvider. +type Operation interface { + // Name provides the unique name for this operation, see OperationFromStatus. + Name() string + + // Checkpoint augments the provided BuildStatus with sufficient state to be + // restored by OperationFromStatus on an appropriate BuildProvider. + // + // This takes into account necessary information about the provided Build. + Checkpoint(*v1alpha1.Build, *v1alpha1.BuildStatus) error + + // Wait blocks until the Operation completes, returning either a status for the build or an error. + // TODO(mattmoor): This probably shouldn't be BuildStatus, but some sort of smaller-scope thing. + Wait() (*v1alpha1.BuildStatus, error) + + // Terminate cleans up this particular operation and returns an error if it fails + Terminate() error +} + +// Build defines the interface for launching a build and getting an Operation by which to track it to completion. +type Build interface { + // Execute launches this particular build and returns an Operation to track it's progress. + Execute() (Operation, error) +} + +// Interface defines the set of operations that all builders must implement. +type Interface interface { + // Which builder are we? + Builder() v1alpha1.BuildProvider + + // Validate a Build for this flavor of builder. + Validate(*v1alpha1.Build) error + + // Construct a Build for this flavor of builder from our CRD specification. + BuildFromSpec(*v1alpha1.Build) (Build, error) + + // Construct an Operation for this flavor of builder from a BuildStatus. + OperationFromStatus(*v1alpha1.BuildStatus) (Operation, error) +} + +// IsDone returns true if the build's status indicates the build is done. +func IsDone(status *v1alpha1.BuildStatus) bool { + if status == nil || len(status.Conditions) == 0 { + return false + } + for _, cond := range status.Conditions { + if cond.Type == v1alpha1.BuildSucceeded { + return cond.Status != corev1.ConditionUnknown + } + } + return false +} + +// IsTimeout returns true if the build's execution time is greater than +// specified build spec timeout. +func IsTimeout(status *v1alpha1.BuildStatus, buildTimeout *metav1.Duration) bool { + var timeout time.Duration + var defaultTimeout = 10 * time.Minute + + if status == nil { + return false + } + + if buildTimeout == nil { + // Set default timeout to 10 minute if build timeout is not set + timeout = defaultTimeout + } else { + timeout = buildTimeout.Duration + } + + // If build has not started timeout, startTime should be zero. + if status.StartTime.Time.IsZero() { + return false + } + return time.Since(status.StartTime.Time).Seconds() > timeout.Seconds() +} + +// ErrorMessage returns the error message from the status. +func ErrorMessage(status *v1alpha1.BuildStatus) (string, bool) { + if status == nil || len(status.Conditions) == 0 { + return "", false + } + for _, cond := range status.Conditions { + if cond.Type == v1alpha1.BuildSucceeded && cond.Status == corev1.ConditionFalse { + return cond.Message, true + } + } + return "", false +}