Refactor the way the Pipelines controllers receive flags.

This change has a few parts, but the overarching goal is to make it easier for downstreams to repackage Tekton Pipelines across updates to the flags that are parsed and passed into the controllers. 1. Instead of defining `var`s for the flags, and then extracting the flags into the `pipeline.Images` struct, this change takes advantage of `flag.NewFooVar` to pass in the address of the variable to update. This lets us have flag parsing process things directly into its final variable. 2. Wrap `Images` in `FlagOptions`, which is passed to controllers. I'm sort of ambivalent about the nested struct vs. renaming Images, but opted for the former because so many files reference `pipeline.Images` and I wanted to reduce the scope of the change. 3. For flags only used in `main()` use a local flag definition (vs. `FlagOptions`). It turns out the `*namespace` we were passing to the `NewController` methods was unused! 4. For globals we've turned into flags, use the `flag.NewFooVar` form to update the global directly. 5. Colocate the `flag` setup with the validation setup. This allows us to get coverage of the flag processing code, and it turns out there were some bugs in the names of flags emitted from validation!
tektoncd · Sep 20, 2021 · d0a1053 · d0a1053
1 parent 0fcdd39
commit d0a1053
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 108 deletions.
diff --git a/cmd/controller/main.go b/cmd/controller/main.go
@@ -40,42 +40,18 @@ const (
 	ControllerLogKey = "tekton-pipelines-controller"
 )
 
-var (
-	entrypointImage          = flag.String("entrypoint-image", "", "The container image containing our entrypoint binary.")
-	nopImage                 = flag.String("nop-image", "", "The container image used to stop sidecars")
-	gitImage                 = flag.String("git-image", "", "The container image containing our Git binary.")
-	kubeconfigWriterImage    = flag.String("kubeconfig-writer-image", "", "The container image containing our kubeconfig writer binary.")
-	shellImage               = flag.String("shell-image", "", "The container image containing a shell")
-	shellImageWin            = flag.String("shell-image-win", "", "The container image containing a windows shell")
-	gsutilImage              = flag.String("gsutil-image", "", "The container image containing gsutil")
-	prImage                  = flag.String("pr-image", "", "The container image containing our PR binary.")
-	imageDigestExporterImage = flag.String("imagedigest-exporter-image", "", "The container image containing our image digest exporter binary.")
-	namespace                = flag.String("namespace", corev1.NamespaceAll, "Namespace to restrict informer to. Optional, defaults to all namespaces.")
-	threadsPerController     = flag.Int("threads-per-controller", controller.DefaultThreadsPerController, "Threads (goroutines) to create per controller")
-	disableHighAvailability  = flag.Bool("disable-ha", false, "Whether to disable high-availability functionality for this component.  This flag will be deprecated "+
+func main() {
+	flag.IntVar(&controller.DefaultThreadsPerController, "threads-per-controller", controller.DefaultThreadsPerController, "Threads (goroutines) to create per controller")
+	namespace := flag.String("namespace", corev1.NamespaceAll, "Namespace to restrict informer to. Optional, defaults to all namespaces.")
+	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"
-)
+	fo := pipeline.NewFlagOptions(flag.CommandLine)
 
-func main() {
+	// This parses flags.
 	cfg := injection.ParseAndGetRESTConfigOrDie()
-	controller.DefaultThreadsPerController = *threadsPerController
-	images := pipeline.Images{
-		EntrypointImage:          *entrypointImage,
-		NopImage:                 *nopImage,
-		GitImage:                 *gitImage,
-		KubeconfigWriterImage:    *kubeconfigWriterImage,
-		ShellImage:               *shellImage,
-		ShellImageWin:            *shellImageWin,
-		GsutilImage:              *gsutilImage,
-		PRImage:                  *prImage,
-		ImageDigestExporterImage: *imageDigestExporterImage,
-	}
-	if err := images.Validate(); err != nil {
+
+	if err := fo.Images.Validate(); err != nil {
 		log.Fatal(err)
 	}
 	if cfg.QPS == 0 {
@@ -112,20 +88,10 @@ 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, taskrunControllerConfig),
-		pipelinerun.NewController(*namespace, pipelinerunControllerConfig),
+		taskrun.NewController(fo),
+		pipelinerun.NewController(fo),
 	)
 }
 

diff --git a/pkg/apis/pipeline/images.go b/pkg/apis/pipeline/images.go
@@ -17,10 +17,34 @@ limitations under the License.
 package pipeline
 
 import (
+	"flag"
 	"fmt"
 	"sort"
 )
 
+// FlagOptions holds options passed to the Tekton Pipeline controllers via command-line flags.
+type FlagOptions struct {
+	Images                        Images
+	ExperimentalDisableResolution bool
+}
+
+func NewFlagOptions(fs *flag.FlagSet) *FlagOptions {
+	fo := &FlagOptions{}
+	fs.StringVar(&fo.Images.EntrypointImage, "entrypoint-image", "", "The container image containing our entrypoint binary.")
+	fs.StringVar(&fo.Images.NopImage, "nop-image", "", "The container image used to stop sidecars")
+	fs.StringVar(&fo.Images.GitImage, "git-image", "", "The container image containing our Git binary.")
+	fs.StringVar(&fo.Images.KubeconfigWriterImage, "kubeconfig-writer-image", "", "The container image containing our kubeconfig writer binary.")
+	fs.StringVar(&fo.Images.ShellImage, "shell-image", "", "The container image containing a shell")
+	fs.StringVar(&fo.Images.ShellImageWin, "shell-image-win", "", "The container image containing a windows shell")
+	fs.StringVar(&fo.Images.GsutilImage, "gsutil-image", "", "The container image containing gsutil")
+	fs.StringVar(&fo.Images.PRImage, "pr-image", "", "The container image containing our PR binary.")
+	fs.StringVar(&fo.Images.ImageDigestExporterImage, "imagedigest-exporter-image", "", "The container image containing our image digest exporter binary.")
+	fs.BoolVar(&fo.ExperimentalDisableResolution, "experimental-disable-in-tree-resolution", false,
+		"Disable resolution of taskrun and pipelinerun refs by the taskrun and pipelinerun reconcilers.")
+
+	return fo
+}
+
 // Images holds the images reference for a number of container images used
 // across tektoncd pipelines.
 type Images struct {
@@ -52,15 +76,15 @@ func (i Images) Validate() error {
 	for _, f := range []struct {
 		v, name string
 	}{
-		{i.EntrypointImage, "entrypoint"},
-		{i.NopImage, "nop"},
-		{i.GitImage, "git"},
-		{i.KubeconfigWriterImage, "kubeconfig-writer"},
-		{i.ShellImage, "shell"},
-		{i.ShellImageWin, "windows-shell"},
-		{i.GsutilImage, "gsutil"},
-		{i.PRImage, "pr"},
-		{i.ImageDigestExporterImage, "imagedigest-exporter"},
+		{i.EntrypointImage, "entrypoint-image"},
+		{i.NopImage, "nop-image"},
+		{i.GitImage, "git-image"},
+		{i.KubeconfigWriterImage, "kubeconfig-writer-image"},
+		{i.ShellImage, "shell-image"},
+		{i.ShellImageWin, "shell-image-win"},
+		{i.GsutilImage, "gsutil-image"},
+		{i.PRImage, "pr-image"},
+		{i.ImageDigestExporterImage, "imagedigest-exporter-image"},
 	} {
 		if f.v == "" {
 			unset = append(unset, f.name)

diff --git a/pkg/apis/pipeline/images_test.go b/pkg/apis/pipeline/images_test.go
@@ -1,40 +1,45 @@
 package pipeline_test
 
 import (
+	"flag"
 	"testing"
 
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 )
 
 func TestValidate(t *testing.T) {
-	valid := pipeline.Images{
-		EntrypointImage:          "set",
-		NopImage:                 "set",
-		GitImage:                 "set",
-		KubeconfigWriterImage:    "set",
-		ShellImage:               "set",
-		ShellImageWin:            "set",
-		GsutilImage:              "set",
-		PRImage:                  "set",
-		ImageDigestExporterImage: "set",
-	}
-	if err := valid.Validate(); err != nil {
+	fs := flag.FlagSet{}
+	valid := pipeline.NewFlagOptions(&fs)
+	fs.Parse([]string{
+		"-entrypoint-image", "set",
+		"-nop-image", "set",
+		"-git-image", "set",
+		"-kubeconfig-writer-image", "set",
+		"-shell-image", "set",
+		"-shell-image-win", "set",
+		"-gsutil-image", "set",
+		"-pr-image", "set",
+		"-imagedigest-exporter-image", "set",
+	})
+	if err := valid.Images.Validate(); err != nil {
 		t.Errorf("valid Images returned error: %v", err)
 	}
 
-	invalid := pipeline.Images{
-		EntrypointImage:          "set",
-		NopImage:                 "set",
-		GitImage:                 "", // unset!
-		KubeconfigWriterImage:    "set",
-		ShellImage:               "", // unset!
-		ShellImageWin:            "set",
-		GsutilImage:              "set",
-		PRImage:                  "", // unset!
-		ImageDigestExporterImage: "set",
-	}
-	wantErr := "found unset image flags: [git pr shell]"
-	if err := invalid.Validate(); err == nil {
+	fs = flag.FlagSet{}
+	invalid := pipeline.NewFlagOptions(&fs)
+	fs.Parse([]string{
+		"-entrypoint-image", "set",
+		"-nop-image", "set",
+		// "-git-image", "unset",
+		"-kubeconfig-writer-image", "set",
+		// "-shell-image", "unset",
+		"-shell-image-win", "set",
+		"-gsutil-image", "set",
+		// "-pr-image", "unset",
+		"-imagedigest-exporter-image", "set",
+	})
+	wantErr := "found unset image flags: [git-image pr-image shell-image]"
+	if err := invalid.Images.Validate(); err == nil {
 		t.Error("invalid Images expected error, got nil")
 	} else if err.Error() != wantErr {
 		t.Errorf("Unexpected error message: got %q, want %q", err.Error(), wantErr)

diff --git a/pkg/reconciler/pipelinerun/controller.go b/pkg/reconciler/pipelinerun/controller.go
@@ -42,18 +42,8 @@ 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, conf ControllerConfiguration) func(context.Context, configmap.Watcher) *controller.Impl {
+func NewController(fo *pipeline.FlagOptions) 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)
@@ -72,7 +62,7 @@ func NewController(namespace string, conf ControllerConfiguration) func(context.
 		c := &Reconciler{
 			KubeClientSet:     kubeclientset,
 			PipelineClientSet: pipelineclientset,
-			Images:            conf.Images,
+			Images:            fo.Images,
 			pipelineRunLister: pipelineRunInformer.Lister(),
 			pipelineLister:    pipelineInformer.Lister(),
 			taskLister:        taskInformer.Lister(),
@@ -84,7 +74,7 @@ func NewController(namespace string, conf ControllerConfiguration) func(context.
 			cloudEventClient:  cloudeventclient.Get(ctx),
 			metrics:           pipelinerunmetrics.Get(ctx),
 			pvcHandler:        volumeclaim.NewPVCHandler(kubeclientset, logger),
-			disableResolution: conf.DisablePipelineRefResolution,
+			disableResolution: fo.ExperimentalDisableResolution,
 		}
 		impl := pipelinerunreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options {
 			return controller.Options{

diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -70,7 +70,6 @@ import (
 )
 
 var (
-	namespace                = ""
 	ignoreLastTransitionTime = cmpopts.IgnoreTypes(apis.Condition{}.LastTransitionTime.Inner.Time)
 	images                   = pipeline.Images{
 		EntrypointImage:          "override-with-entrypoint:latest",
@@ -159,7 +158,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, ControllerConfiguration{Images: images})(ctx, configMapWatcher)
+	ctl := NewController(&pipeline.FlagOptions{Images: images})(ctx, configMapWatcher)
 
 	if la, ok := ctl.Reconciler.(reconciler.LeaderAware); ok {
 		la.Promote(reconciler.UniversalBucket(), func(reconciler.Bucket, types.NamespacedName) {})

diff --git a/pkg/reconciler/taskrun/controller.go b/pkg/reconciler/taskrun/controller.go
@@ -41,18 +41,8 @@ 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, conf ControllerConfiguration) func(context.Context, configmap.Watcher) *controller.Impl {
+func NewController(fo *pipeline.FlagOptions) 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)
@@ -74,7 +64,7 @@ func NewController(namespace string, conf ControllerConfiguration) func(context.
 		c := &Reconciler{
 			KubeClientSet:     kubeclientset,
 			PipelineClientSet: pipelineclientset,
-			Images:            conf.Images,
+			Images:            fo.Images,
 			taskRunLister:     taskRunInformer.Lister(),
 			taskLister:        taskInformer.Lister(),
 			clusterTaskLister: clusterTaskInformer.Lister(),
@@ -84,7 +74,7 @@ func NewController(namespace string, conf ControllerConfiguration) func(context.
 			metrics:           taskrunmetrics.Get(ctx),
 			entrypointCache:   entrypointCache,
 			pvcHandler:        volumeclaim.NewPVCHandler(kubeclientset, logger),
-			disableResolution: conf.DisableTaskRefResolution,
+			disableResolution: fo.ExperimentalDisableResolution,
 		}
 		impl := taskrunreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options {
 			return controller.Options{

diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go
@@ -76,7 +76,6 @@ const (
 )
 
 var (
-	namespace                    = "" // all namespaces
 	defaultActiveDeadlineSeconds = int64(config.DefaultTimeoutMinutes * 60 * 1.5)
 	images                       = pipeline.Images{
 		EntrypointImage:          "override-with-entrypoint:latest",
@@ -335,7 +334,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, ControllerConfiguration{Images: images})(ctx, configMapWatcher)
+	ctl := NewController(&pipeline.FlagOptions{Images: images})(ctx, configMapWatcher)
 	if err := configMapWatcher.Start(ctx.Done()); err != nil {
 		t.Fatalf("error starting configmap watcher: %v", err)
 	}