From 2d918b69dafa9d1cb92c4f7be4bad27687473c77 Mon Sep 17 00:00:00 2001 From: Cole Wagner Date: Thu, 19 Sep 2019 16:52:41 -0700 Subject: [PATCH] Make unknown plugin validation optional and avoid in Deck. --- prow/cmd/checkconfig/main.go | 2 +- prow/cmd/config-bootstrapper/BUILD.bazel | 2 +- prow/cmd/config-bootstrapper/main.go | 4 +- prow/cmd/deck/localdata/.gitignore | 2 + prow/cmd/deck/main.go | 2 +- prow/cmd/deck/runlocal | 5 ++- prow/cmd/hook/main.go | 2 +- prow/cmd/hook/main_test.go | 2 +- prow/cmd/status-reconciler/BUILD.bazel | 1 - prow/cmd/status-reconciler/main.go | 3 +- .../external-plugins/needs-rebase/BUILD.bazel | 1 - prow/external-plugins/needs-rebase/main.go | 5 +-- prow/plugins/BUILD.bazel | 1 + prow/plugins/config.go | 37 +++++++++++-------- prow/plugins/plugins.go | 19 +++++++--- 15 files changed, 51 insertions(+), 37 deletions(-) diff --git a/prow/cmd/checkconfig/main.go b/prow/cmd/checkconfig/main.go index 623b46a0dcef..9b16a8b5c473 100644 --- a/prow/cmd/checkconfig/main.go +++ b/prow/cmd/checkconfig/main.go @@ -184,7 +184,7 @@ func main() { pluginAgent := plugins.ConfigAgent{} var pcfg *plugins.Configuration if o.pluginConfig != "" { - if err := pluginAgent.Load(o.pluginConfig); err != nil { + if err := pluginAgent.Load(o.pluginConfig, true); err != nil { logrus.WithError(err).Fatal("Error loading Prow plugin config.") } pcfg = pluginAgent.Config() diff --git a/prow/cmd/config-bootstrapper/BUILD.bazel b/prow/cmd/config-bootstrapper/BUILD.bazel index 75f857ed2cc8..26267be0a473 100644 --- a/prow/cmd/config-bootstrapper/BUILD.bazel +++ b/prow/cmd/config-bootstrapper/BUILD.bazel @@ -11,7 +11,7 @@ go_library( "//prow/config:go_default_library", "//prow/flagutil:go_default_library", "//prow/github:go_default_library", - "//prow/hook:go_default_library", + "//prow/hook/plugin-imports:go_default_library", "//prow/logrusutil:go_default_library", "//prow/plugins:go_default_library", "//prow/plugins/updateconfig:go_default_library", diff --git a/prow/cmd/config-bootstrapper/main.go b/prow/cmd/config-bootstrapper/main.go index c4919720b9c6..a60e910ac801 100644 --- a/prow/cmd/config-bootstrapper/main.go +++ b/prow/cmd/config-bootstrapper/main.go @@ -28,7 +28,7 @@ import ( "k8s.io/test-infra/prow/config" prowflagutil "k8s.io/test-infra/prow/flagutil" "k8s.io/test-infra/prow/github" - _ "k8s.io/test-infra/prow/hook" + _ "k8s.io/test-infra/prow/hook/plugin-imports" "k8s.io/test-infra/prow/logrusutil" "k8s.io/test-infra/prow/plugins" "k8s.io/test-infra/prow/plugins/updateconfig" @@ -89,7 +89,7 @@ func main() { } pluginAgent := &plugins.ConfigAgent{} - if err := pluginAgent.Start(o.pluginConfig); err != nil { + if err := pluginAgent.Start(o.pluginConfig, true); err != nil { logrus.WithError(err).Fatal("Error starting plugin configuration agent.") } diff --git a/prow/cmd/deck/localdata/.gitignore b/prow/cmd/deck/localdata/.gitignore index 342dda7490c3..9714365d443f 100644 --- a/prow/cmd/deck/localdata/.gitignore +++ b/prow/cmd/deck/localdata/.gitignore @@ -4,3 +4,5 @@ tide.js tide-history.js branding.js pr-data.js +config.yaml +plugins.yaml diff --git a/prow/cmd/deck/main.go b/prow/cmd/deck/main.go index bfc1340cbb3b..4823f9fa073d 100644 --- a/prow/cmd/deck/main.go +++ b/prow/cmd/deck/main.go @@ -297,7 +297,7 @@ func main() { var pluginAgent *plugins.ConfigAgent if o.pluginConfig != "" { pluginAgent = &plugins.ConfigAgent{} - if err := pluginAgent.Start(o.pluginConfig); err != nil { + if err := pluginAgent.Start(o.pluginConfig, false); err != nil { logrus.WithError(err).Fatal("Error loading Prow plugin config.") } } else { diff --git a/prow/cmd/deck/runlocal b/prow/cmd/deck/runlocal index b050ec64c085..4b10954c4f0d 100755 --- a/prow/cmd/deck/runlocal +++ b/prow/cmd/deck/runlocal @@ -25,4 +25,7 @@ curl "${HOST}/tide.js?var=tideData" > tide.js curl "${HOST}/tide-history.js?var=tideHistory" > tide-history.js curl "${HOST}/plugin-help.js?var=allHelp" > plugin-help.js curl "${HOST}/pr-data.js" > pr-data.js -bazel run //prow/cmd/deck:deck -- --pregenerated-data=${DIR}/localdata --static-files-location=./prow/cmd/deck/static --template-files-location=./prow/cmd/deck/template --spyglass-files-location=./prow/spyglass/lenses --config-path ${DIR}/../../../config/prow/config.yaml --spyglass \ No newline at end of file +# Use config that matches the data instead of whatever is checked out locally. +curl "${HOST}/config" > config.yaml +curl "${HOST}/plugin-config" > plugins.yaml +bazel run //prow/cmd/deck:deck -- --pregenerated-data=${DIR}/localdata --static-files-location=./prow/cmd/deck/static --template-files-location=./prow/cmd/deck/template --spyglass-files-location=./prow/spyglass/lenses --config-path ${DIR}/localdata/config.yaml --spyglass --plugin-config ${DIR}/localdata/plugins.yaml diff --git a/prow/cmd/hook/main.go b/prow/cmd/hook/main.go index 9b520b71f3cb..b16f2e6bd081 100644 --- a/prow/cmd/hook/main.go +++ b/prow/cmd/hook/main.go @@ -124,7 +124,7 @@ func main() { } pluginAgent := &plugins.ConfigAgent{} - if err := pluginAgent.Start(o.pluginConfig); err != nil { + if err := pluginAgent.Start(o.pluginConfig, true); err != nil { logrus.WithError(err).Fatal("Error starting plugins.") } diff --git a/prow/cmd/hook/main_test.go b/prow/cmd/hook/main_test.go index 672cf358282a..71f21d744f09 100644 --- a/prow/cmd/hook/main_test.go +++ b/prow/cmd/hook/main_test.go @@ -31,7 +31,7 @@ import ( // Make sure that our plugins are valid. func TestPlugins(t *testing.T) { pa := &plugins.ConfigAgent{} - if err := pa.Load("../../../config/prow/plugins.yaml"); err != nil { + if err := pa.Load("../../../config/prow/plugins.yaml", true); err != nil { t.Fatalf("Could not load plugins: %v.", err) } } diff --git a/prow/cmd/status-reconciler/BUILD.bazel b/prow/cmd/status-reconciler/BUILD.bazel index d8b15d76ed6e..8cae2a3c8cf1 100644 --- a/prow/cmd/status-reconciler/BUILD.bazel +++ b/prow/cmd/status-reconciler/BUILD.bazel @@ -24,7 +24,6 @@ go_library( "//prow/config:go_default_library", "//prow/config/secret:go_default_library", "//prow/flagutil:go_default_library", - "//prow/hook:go_default_library", "//prow/interrupts:go_default_library", "//prow/logrusutil:go_default_library", "//prow/pjutil:go_default_library", diff --git a/prow/cmd/status-reconciler/main.go b/prow/cmd/status-reconciler/main.go index a465ede7326c..69d205d7a1e5 100644 --- a/prow/cmd/status-reconciler/main.go +++ b/prow/cmd/status-reconciler/main.go @@ -29,7 +29,6 @@ import ( "k8s.io/test-infra/prow/config" "k8s.io/test-infra/prow/config/secret" prowflagutil "k8s.io/test-infra/prow/flagutil" - _ "k8s.io/test-infra/prow/hook" "k8s.io/test-infra/prow/logrusutil" "k8s.io/test-infra/prow/pjutil" "k8s.io/test-infra/prow/plugins" @@ -114,7 +113,7 @@ func main() { } pluginAgent := &plugins.ConfigAgent{} - if err := pluginAgent.Start(o.pluginConfig); err != nil { + if err := pluginAgent.Start(o.pluginConfig, false); err != nil { logrus.WithError(err).Fatal("Error starting plugin configuration agent.") } diff --git a/prow/external-plugins/needs-rebase/BUILD.bazel b/prow/external-plugins/needs-rebase/BUILD.bazel index f4a219bd1522..394a075b0a65 100644 --- a/prow/external-plugins/needs-rebase/BUILD.bazel +++ b/prow/external-plugins/needs-rebase/BUILD.bazel @@ -18,7 +18,6 @@ go_library( "//prow/external-plugins/needs-rebase/plugin:go_default_library", "//prow/flagutil:go_default_library", "//prow/github:go_default_library", - "//prow/hook:go_default_library", "//prow/interrupts:go_default_library", "//prow/labels:go_default_library", "//prow/pluginhelp/externalplugins:go_default_library", diff --git a/prow/external-plugins/needs-rebase/main.go b/prow/external-plugins/needs-rebase/main.go index 2dcf73e72abd..d3b373ad99b8 100644 --- a/prow/external-plugins/needs-rebase/main.go +++ b/prow/external-plugins/needs-rebase/main.go @@ -35,9 +35,6 @@ import ( "k8s.io/test-infra/prow/github" "k8s.io/test-infra/prow/labels" - // TODO: Remove the need for this import; it's currently required to allow the plugin config loader to function correctly (it expects plugins to be initialised) - // See https://github.com/kubernetes/test-infra/pull/8933#issuecomment-411511180 - _ "k8s.io/test-infra/prow/hook" "k8s.io/test-infra/prow/pluginhelp/externalplugins" "k8s.io/test-infra/prow/plugins" ) @@ -97,7 +94,7 @@ func main() { } pa := &plugins.ConfigAgent{} - if err := pa.Start(o.pluginConfig); err != nil { + if err := pa.Start(o.pluginConfig, false); err != nil { log.WithError(err).Fatalf("Error loading plugin config from %q.", o.pluginConfig) } diff --git a/prow/plugins/BUILD.bazel b/prow/plugins/BUILD.bazel index c966c2876602..d034db542e64 100644 --- a/prow/plugins/BUILD.bazel +++ b/prow/plugins/BUILD.bazel @@ -35,6 +35,7 @@ go_library( "//prow/client/clientset/versioned/typed/prowjobs/v1:go_default_library", "//prow/commentpruner:go_default_library", "//prow/config:go_default_library", + "//prow/errorutil:go_default_library", "//prow/git:go_default_library", "//prow/github:go_default_library", "//prow/labels:go_default_library", diff --git a/prow/plugins/config.go b/prow/plugins/config.go index ad01a3d49ad1..a1382723f3a8 100644 --- a/prow/plugins/config.go +++ b/prow/plugins/config.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/test-infra/prow/bugzilla" + "k8s.io/test-infra/prow/errorutil" "k8s.io/test-infra/prow/labels" ) @@ -783,30 +784,34 @@ func (c *Configuration) setDefaults() { } } -// validatePlugins will return error if -// there are unknown or duplicated plugins. -func validatePlugins(plugins map[string][]string) error { - var errors []string - for _, configuration := range plugins { - for _, plugin := range configuration { - if _, ok := pluginHelp[plugin]; !ok { - errors = append(errors, fmt.Sprintf("unknown plugin: %s", plugin)) - } - } - } +// validatePluginsDupes will return an error if there are duplicated plugins. +// It is sometimes a sign of misconfiguration and is always useless for a +// plugin to be specified at both the org and repo levels. +func validatePluginsDupes(plugins map[string][]string) error { + var errors []error for repo, repoConfig := range plugins { if strings.Contains(repo, "/") { org := strings.Split(repo, "/")[0] if dupes := findDuplicatedPluginConfig(repoConfig, plugins[org]); len(dupes) > 0 { - errors = append(errors, fmt.Sprintf("plugins %v are duplicated for %s and %s", dupes, repo, org)) + errors = append(errors, fmt.Errorf("plugins %v are duplicated for %s and %s", dupes, repo, org)) } } } + return errorutil.NewAggregate(errors...) +} - if len(errors) > 0 { - return fmt.Errorf("invalid plugin configuration:\n\t%v", strings.Join(errors, "\n\t")) +// ValidatePluginsUnknown will return an error if there are any unrecognized +// plugins configured. +func (c *Configuration) ValidatePluginsUnknown() error { + var errors []error + for _, configuration := range c.Plugins { + for _, plugin := range configuration { + if _, ok := pluginHelp[plugin]; !ok { + errors = append(errors, fmt.Errorf("unknown plugin: %s", plugin)) + } + } } - return nil + return errorutil.NewAggregate(errors...) } func validateSizes(size Size) error { @@ -962,7 +967,7 @@ func (c *Configuration) Validate() error { return err } - if err := validatePlugins(c.Plugins); err != nil { + if err := validatePluginsDupes(c.Plugins); err != nil { return err } if err := validateExternalPlugins(c.ExternalPlugins); err != nil { diff --git a/prow/plugins/plugins.go b/prow/plugins/plugins.go index 45444b340e17..d9f561db3f9a 100644 --- a/prow/plugins/plugins.go +++ b/prow/plugins/plugins.go @@ -19,11 +19,11 @@ package plugins import ( "errors" "fmt" - "github.com/prometheus/client_golang/prometheus" "io/ioutil" "sync" "time" + "github.com/prometheus/client_golang/prometheus" "github.com/sirupsen/logrus" "k8s.io/client-go/kubernetes" "k8s.io/test-infra/prow/bugzilla" @@ -218,7 +218,9 @@ func NewFakeConfigAgent() ConfigAgent { // Load attempts to load config from the path. It returns an error if either // the file can't be read or the configuration is invalid. -func (pa *ConfigAgent) Load(path string) error { +// If checkUnknownPlugins is true, unrecognized plugin names will make config +// loading fail. +func (pa *ConfigAgent) Load(path string, checkUnknownPlugins bool) error { b, err := ioutil.ReadFile(path) if err != nil { return err @@ -230,6 +232,11 @@ func (pa *ConfigAgent) Load(path string) error { if err := np.Validate(); err != nil { return err } + if checkUnknownPlugins { + if err := np.ValidatePluginsUnknown(); err != nil { + return err + } + } pa.Set(np) return nil @@ -254,14 +261,16 @@ func (pa *ConfigAgent) Set(pc *Configuration) { // Start starts polling path for plugin config. If the first attempt fails, // then start returns the error. Future errors will halt updates but not stop. -func (pa *ConfigAgent) Start(path string) error { - if err := pa.Load(path); err != nil { +// If checkUnknownPlugins is true, unrecognized plugin names will make config +// loading fail. +func (pa *ConfigAgent) Start(path string, checkUnknownPlugins bool) error { + if err := pa.Load(path, checkUnknownPlugins); err != nil { return err } ticker := time.Tick(1 * time.Minute) go func() { for range ticker { - if err := pa.Load(path); err != nil { + if err := pa.Load(path, checkUnknownPlugins); err != nil { logrus.WithField("path", path).WithError(err).Error("Error loading plugin config.") } }