Make unknown plugin validation optional and avoid in Deck.

yannizhang2019 · Sep 20, 2019 · 2d918b6 · 2d918b6
1 parent 21b2f1e
commit 2d918b6
Show file tree

Hide file tree

Showing 15 changed files with 51 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+# 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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.")
 			}
 		}