Add controller flag to turn off built-in resolution

We're currently trying to work out what the best way forward for TEP-0060
(remote resolution) is for Tekton Pipelines. As part of that we're
creating a controller in the experimental repo that we can use to test out
a bunch of the Alternatives from the TEP and figure out what works best
in terms of implementation and DX.

Right now the experimental controller would race with the Pipelines controllers
since they're all going to try and "resolve" any taskRef in Taskruns and
PipelineRuns.

This commit adds a flag to the pipelines controller that explicitly switches
off resolution performed by the taskrun and pipelinerun reconcilers. This gives
us just enough room to perform resolution out-of-band and try out some of the
alternatives.

When the `-experimental-disable-in-tree-resolution` flag is set the taskrun
and pipelinerun controllers will ignore taskruns and pipelineruns that don't
have `status.taskSpec` or `status.pipelineSpec` populated.

cmd/controller/main.go

@@ -55,6 +55,10 @@ var (
 	disableHighAvailability  = flag.Bool("disable-ha", false, "Whether to disable high-availability functionality for this component.  This flag will be deprecated "+
 		"and removed when we have promoted this feature to stable, so do not pass it without filing an "+
 		"issue upstream!")
+	experimentalDisableInTreeResolution = flag.Bool(disableInTreeResolutionFlag, false,
+		"Disable resolution of taskrun and pipelinerun refs by the taskrun and pipelinerun reconcilers.")
+
+	disableInTreeResolutionFlag = "experimental-disable-in-tree-resolution"
 )
 
 func main() {
@@ -108,10 +112,20 @@ func main() {
 		log.Fatal(http.ListenAndServe(":"+port, mux))
 	}()
 
+	taskrunControllerConfig := taskrun.ControllerConfiguration{
+		Images:                   images,
+		DisableTaskRefResolution: *experimentalDisableInTreeResolution,
+	}
+
+	pipelinerunControllerConfig := pipelinerun.ControllerConfiguration{
+		Images:                       images,
+		DisablePipelineRefResolution: *experimentalDisableInTreeResolution,
+	}
+
 	ctx = filteredinformerfactory.WithSelectors(ctx, v1beta1.ManagedByLabelKey)
 	sharedmain.MainWithConfig(ctx, ControllerLogKey, cfg,
-		taskrun.NewController(*namespace, images),
-		pipelinerun.NewController(*namespace, images),
+		taskrun.NewController(*namespace, taskrunControllerConfig),
+		pipelinerun.NewController(*namespace, pipelinerunControllerConfig),
 	)
 }
 

config/controller.yaml

@@ -80,7 +80,10 @@ spec:
           "-shell-image", "gcr.io/distroless/base@sha256:aa4fd987555ea10e1a4ec8765da8158b5ffdfef1e72da512c7ede509bc9966c4",
           # for script mode to work with windows we need a powershell image
           # pinning to nanoserver tag as of July 15 2021
-          "-shell-image-win", "mcr.microsoft.com/powershell:nanoserver@sha256:b6d5ff841b78bdf2dfed7550000fd4f3437385b8fa686ec0f010be24777654d6"
+          "-shell-image-win", "mcr.microsoft.com/powershell:nanoserver@sha256:b6d5ff841b78bdf2dfed7550000fd4f3437385b8fa686ec0f010be24777654d6",
+          # Experimental. Uncomment below to disable TaskRun and PipelineRun
+          # reconcilers' built-in taskRef and pipelineRef resolution procedures.
+          # "-experimental-disable-in-tree-resolution",
         ]
         volumeMounts:
         - name: config-logging

pkg/reconciler/pipelinerun/controller.go

@@ -42,8 +42,18 @@ import (
 	"knative.dev/pkg/logging"
 )
 
+// ControllerConfiguration holds fields used to configure the
+// PipelineRun controller.
+type ControllerConfiguration struct {
+	// Images are the image references used across Tekton Pipelines.
+	Images pipeline.Images
+	// DisablePipelineRefResolution tells the controller not to perform
+	// resolution of pipeline refs from the cluster or bundles.
+	DisablePipelineRefResolution bool
+}
+
 // NewController instantiates a new controller.Impl from knative.dev/pkg/controller
-func NewController(namespace string, images pipeline.Images) func(context.Context, configmap.Watcher) *controller.Impl {
+func NewController(namespace string, conf ControllerConfiguration) func(context.Context, configmap.Watcher) *controller.Impl {
 	return func(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
 		logger := logging.FromContext(ctx)
 		kubeclientset := kubeclient.Get(ctx)
@@ -62,7 +72,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex
 		c := &Reconciler{
 			KubeClientSet:     kubeclientset,
 			PipelineClientSet: pipelineclientset,
-			Images:            images,
+			Images:            conf.Images,
 			pipelineRunLister: pipelineRunInformer.Lister(),
 			pipelineLister:    pipelineInformer.Lister(),
 			taskLister:        taskInformer.Lister(),
@@ -74,6 +84,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex
 			cloudEventClient:  cloudeventclient.Get(ctx),
 			metrics:           pipelinerunmetrics.Get(ctx),
 			pvcHandler:        volumeclaim.NewPVCHandler(kubeclientset, logger),
+			disableResolution: conf.DisablePipelineRefResolution,
 		}
 		impl := pipelinerunreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options {
 			return controller.Options{

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -132,11 +132,21 @@ type Reconciler struct {
 	cloudEventClient  cloudevent.CEClient
 	metrics           *pipelinerunmetrics.Recorder
 	pvcHandler        volumeclaim.PvcHandler
+
+	// disableResolution is a flag to the reconciler that it should
+	// not be performing resolution of pipelineRefs.
+	// TODO(sbwsg): Once we've agreed on a way forward for TEP-0060
+	// this can be removed in favor of whatever that chosen solution
+	// is.
+	disableResolution bool
 }
 
 var (
 	// Check that our Reconciler implements pipelinerunreconciler.Interface
 	_ pipelinerunreconciler.Interface = (*Reconciler)(nil)
+
+	// Indicates pipelinerun resolution hasn't occurred yet.
+	errResourceNotResolved = fmt.Errorf("pipeline ref has not been resolved")
 )
 
 // ReconcileKind compares the actual state with the desired, and attempts to
@@ -232,6 +242,13 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun)
 		logger.Errorf("Reconcile error: %v", err.Error())
 	}
 
+	if c.disableResolution && err == errResourceNotResolved {
+		// This is not an error: an out-of-band process can
+		// still resolve the PipelineRun, at which point
+		// reconciliation can continue as normal.
+		err = nil
+	}
+
 	if err = c.finishReconcileUpdateEmitEvents(ctx, pr, before, err); err != nil {
 		return err
 	}
@@ -336,6 +353,10 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
 		return nil
 	}
 
+	if c.disableResolution && pr.Status.PipelineSpec == nil {
+		return errResourceNotResolved
+	}
+
 	pipelineMeta, pipelineSpec, err := resources.GetPipelineData(ctx, pr, getPipelineFunc)
 	if err != nil {
 		logger.Errorf("Failed to determine Pipeline spec to use for pipelinerun %s: %v", pr.Name, err)

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -159,7 +159,7 @@ func getPipelineRunController(t *testing.T, d test.Data) (test.Assets, func()) {
 	c, informers := test.SeedTestData(t, ctx, d)
 	configMapWatcher := cminformer.NewInformedWatcher(c.Kube, system.Namespace())
 
-	ctl := NewController(namespace, images)(ctx, configMapWatcher)
+	ctl := NewController(namespace, ControllerConfiguration{Images: images})(ctx, configMapWatcher)
 
 	if la, ok := ctl.Reconciler.(reconciler.LeaderAware); ok {
 		la.Promote(reconciler.UniversalBucket(), func(reconciler.Bucket, types.NamespacedName) {})

pkg/reconciler/taskrun/controller.go

@@ -40,8 +40,18 @@ import (
 	"knative.dev/pkg/logging"
 )
 
+// ControllerConfiguration holds fields used to configure the
+// TaskRun controller.
+type ControllerConfiguration struct {
+	// Images are the image references used across Tekton Pipelines.
+	Images pipeline.Images
+	// DisableTaskRefResolution tells the controller not to perform
+	// resolution of task refs from the cluster or bundles.
+	DisableTaskRefResolution bool
+}
+
 // NewController instantiates a new controller.Impl from knative.dev/pkg/controller
-func NewController(namespace string, images pipeline.Images) func(context.Context, configmap.Watcher) *controller.Impl {
+func NewController(namespace string, conf ControllerConfiguration) func(context.Context, configmap.Watcher) *controller.Impl {
 	return func(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
 		logger := logging.FromContext(ctx)
 		kubeclientset := kubeclient.Get(ctx)
@@ -62,7 +72,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex
 		c := &Reconciler{
 			KubeClientSet:     kubeclientset,
 			PipelineClientSet: pipelineclientset,
-			Images:            images,
+			Images:            conf.Images,
 			taskRunLister:     taskRunInformer.Lister(),
 			taskLister:        taskInformer.Lister(),
 			clusterTaskLister: clusterTaskInformer.Lister(),
@@ -71,6 +81,7 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex
 			metrics:           taskrunmetrics.Get(ctx),
 			entrypointCache:   entrypointCache,
 			pvcHandler:        volumeclaim.NewPVCHandler(kubeclientset, logger),
+			disableResolution: conf.DisableTaskRefResolution,
 		}
 		impl := taskrunreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options {
 			return controller.Options{

pkg/reconciler/taskrun/taskrun.go

@@ -74,10 +74,22 @@ type Reconciler struct {
 	entrypointCache   podconvert.EntrypointCache
 	metrics           *taskrunmetrics.Recorder
 	pvcHandler        volumeclaim.PvcHandler
+
+	// disableResolution is a flag to the reconciler that it should
+	// not be performing resolution of taskRefs.
+	// TODO(sbwsg): Once we've agreed on a way forward for TEP-0060
+	// this can be removed in favor of whatever that chosen solution
+	// is.
+	disableResolution bool
 }
 
 // Check that our Reconciler implements taskrunreconciler.Interface
-var _ taskrunreconciler.Interface = (*Reconciler)(nil)
+var (
+	_ taskrunreconciler.Interface = (*Reconciler)(nil)
+
+	// Indicates taskrun resolution hasn't occurred yet.
+	errResourceNotResolved = fmt.Errorf("task ref has not been resolved")
+)
 
 // ReconcileKind compares the actual state with the desired, and attempts to
 // converge the two. It then updates the Status block of the Task Run
@@ -168,25 +180,30 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg
 	// taskrun, runs API convertions. Errors that come out of prepare are
 	// permanent one, so in case of error we update, emit events and return
 	_, rtr, err := c.prepare(ctx, tr)
-	if err != nil {
-		logger.Errorf("TaskRun prepare error: %v", err.Error())
-		// We only return an error if update failed, otherwise we don't want to
-		// reconcile an invalid TaskRun anymore
-		return c.finishReconcileUpdateEmitEvents(ctx, tr, nil, err)
-	}
+	if c.disableResolution && err == errResourceNotResolved {
+		// This is not an error - the taskrun is still expected
+		// to be resolved out-of-band.
+	} else {
+		if err != nil {
+			logger.Errorf("TaskRun prepare error: %v", err.Error())
+			// We only return an error if update failed, otherwise we don't want to
+			// reconcile an invalid TaskRun anymore
+			return c.finishReconcileUpdateEmitEvents(ctx, tr, nil, err)
+		}
 
-	// Store the condition before reconcile
-	before = tr.Status.GetCondition(apis.ConditionSucceeded)
+		// Store the condition before reconcile
+		before = tr.Status.GetCondition(apis.ConditionSucceeded)
 
-	// Reconcile this copy of the task run and then write back any status
-	// updates regardless of whether the reconciliation errored out.
-	if err = c.reconcile(ctx, tr, rtr); err != nil {
-		logger.Errorf("Reconcile: %v", err.Error())
-	}
+		// Reconcile this copy of the task run and then write back any status
+		// updates regardless of whether the reconciliation errored out.
+		if err = c.reconcile(ctx, tr, rtr); err != nil {
+			logger.Errorf("Reconcile: %v", err.Error())
+		}
 
-	// Emit events (only when ConditionSucceeded was changed)
-	if err = c.finishReconcileUpdateEmitEvents(ctx, tr, before, err); err != nil {
-		return err
+		// Emit events (only when ConditionSucceeded was changed)
+		if err = c.finishReconcileUpdateEmitEvents(ctx, tr, before, err); err != nil {
+			return err
+		}
 	}
 
 	if tr.Status.StartTime != nil {
@@ -270,6 +287,10 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1
 	// and may not have had all of the assumed default specified.
 	tr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx))
 
+	if c.disableResolution && tr.Status.TaskSpec == nil {
+		return nil, nil, errResourceNotResolved
+	}
+
 	getTaskfunc, err := resources.GetTaskFuncFromTaskRun(ctx, c.KubeClientSet, c.PipelineClientSet, tr)
 	if err != nil {
 		logger.Errorf("Failed to fetch task reference %s: %v", tr.Spec.TaskRef.Name, err)

pkg/reconciler/taskrun/taskrun_test.go

@@ -329,7 +329,7 @@ func getTaskRunController(t *testing.T, d test.Data) (test.Assets, func()) {
 	c, informers := test.SeedTestData(t, ctx, d)
 	configMapWatcher := cminformer.NewInformedWatcher(c.Kube, system.Namespace())
 
-	ctl := NewController(namespace, images)(ctx, configMapWatcher)
+	ctl := NewController(namespace, ControllerConfiguration{Images: images})(ctx, configMapWatcher)
 	if err := configMapWatcher.Start(ctx.Done()); err != nil {
 		t.Fatalf("error starting configmap watcher: %v", err)
 	}