From d0a10534c90fd6a8a4950abe271d7314a5602bee Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Tue, 14 Sep 2021 16:59:54 -0700 Subject: [PATCH] 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! --- cmd/controller/main.go | 54 ++++-------------- pkg/apis/pipeline/images.go | 42 +++++++++++--- pkg/apis/pipeline/images_test.go | 55 ++++++++++--------- pkg/reconciler/pipelinerun/controller.go | 16 +----- .../pipelinerun/pipelinerun_test.go | 3 +- pkg/reconciler/taskrun/controller.go | 16 +----- pkg/reconciler/taskrun/taskrun_test.go | 3 +- 7 files changed, 81 insertions(+), 108 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index f844adb53a8..a4e24a5800c 100644 --- 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 index e0c81c9869c..9732ee37ee4 100644 --- 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 index c4c4e766321..6fba96f0ed5 100644 --- 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 index 04c2fb5c95b..509218af371 100644 --- 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 index 42152cdd74b..ffa4ce302dc 100644 --- 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 index 3739deb3ca2..0a97bb0620f 100644 --- 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 index 48908f854c2..f6de3ff5ff1 100644 --- 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) }