Merge pull request kubernetes#14415 from cjwagner/plugin-validation

Improvements to plugin config validation, plugin imports, and serving plugin config from Deck.
yannizhang2019 · Sep 20, 2019 · dcbb5ef · dcbb5ef
2 parents c8941d1 + a1f2768
commit dcbb5ef
Show file tree

Hide file tree

Showing 21 changed files with 196 additions and 148 deletions.
diff --git a/prow/cmd/checkconfig/BUILD.bazel b/prow/cmd/checkconfig/BUILD.bazel
@@ -15,7 +15,7 @@ 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/hook/plugin-imports:go_default_library",
         "//prow/labels:go_default_library",
         "//prow/logrusutil:go_default_library",
         "//prow/plugins:go_default_library",

diff --git a/prow/cmd/checkconfig/main.go b/prow/cmd/checkconfig/main.go
@@ -40,7 +40,7 @@ import (
 	needsrebase "k8s.io/test-infra/prow/external-plugins/needs-rebase/plugin"
 	"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/labels"
 	"k8s.io/test-infra/prow/logrusutil"
 	"k8s.io/test-infra/prow/plugins"
@@ -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
diff --git a/prow/cmd/deck/main_test.go b/prow/cmd/deck/main_test.go
@@ -160,7 +160,7 @@ func TestHandleLog(t *testing.T) {
 			code: http.StatusNotFound,
 		},
 	}
-	handler := handleLog(flc(0))
+	handler := handleLog(flc(0), logrus.WithField("handler", "/log"))
 	for _, tc := range testcases {
 		req, err := http.NewRequest(http.MethodGet, "", nil)
 		if err != nil {
@@ -234,7 +234,7 @@ func TestProwJob(t *testing.T) {
 			State: prowapi.PendingState,
 		},
 	})
-	handler := handleProwJob(fakeProwJobClient.ProwV1().ProwJobs("prowjobs"))
+	handler := handleProwJob(fakeProwJobClient.ProwV1().ProwJobs("prowjobs"), logrus.WithField("handler", "/prowjob"))
 	req, err := http.NewRequest(http.MethodGet, "/prowjob?prowjob=wowsuch", nil)
 	if err != nil {
 		t.Fatalf("Error making request: %v", err)
@@ -432,7 +432,7 @@ func TestRerun(t *testing.T) {
 			ghc := mockGitHubConfigGetter{githubLogin: tc.login}
 			rc := &fakegithub.FakeClient{OrgMembers: map[string][]string{"org": {"org-member"}}}
 			pca := plugins.NewFakeConfigAgent()
-			handler := handleRerun(fakeProwJobClient.ProwV1().ProwJobs("prowjobs"), tc.rerunCreatesJob, configGetter, goa, ghc, rc, &pca)
+			handler := handleRerun(fakeProwJobClient.ProwV1().ProwJobs("prowjobs"), tc.rerunCreatesJob, configGetter, goa, ghc, rc, &pca, logrus.WithField("handler", "/rerun"))
 			handler.ServeHTTP(rr, req)
 			if rr.Code != tc.httpCode {
 				t.Fatalf("Bad error code: %d", rr.Code)
@@ -505,7 +505,7 @@ func TestTide(t *testing.T) {
 	if ta.pools[0].Org != "o" {
 		t.Errorf("Wrong org in pool. Got %s, expected o in %v", ta.pools[0].Org, ta.pools)
 	}
-	handler := handleTidePools(ca.Config, &ta)
+	handler := handleTidePools(ca.Config, &ta, logrus.WithField("handler", "/tide.js"))
 	req, err := http.NewRequest(http.MethodGet, "/tide.js", nil)
 	if err != nil {
 		t.Fatalf("Error making request: %v", err)
@@ -564,7 +564,7 @@ func TestTideHistory(t *testing.T) {
 		t.Fatalf("Expected tideAgent history:\n%#v\n,but got:\n%#v\n", testHist, ta.history)
 	}
 
-	handler := handleTideHistory(&ta)
+	handler := handleTideHistory(&ta, logrus.WithField("handler", "/tide-history.js"))
 	req, err := http.NewRequest(http.MethodGet, "/tide-history.js", nil)
 	if err != nil {
 		t.Fatalf("Error making request: %v", err)
@@ -609,7 +609,7 @@ func TestHelp(t *testing.T) {
 	ha := &helpAgent{
 		path: s.URL,
 	}
-	handler := handlePluginHelp(ha)
+	handler := handlePluginHelp(ha, logrus.WithField("handler", "/plugin-help.js"))
 	handleAndCheck := func() {
 		req, err := http.NewRequest(http.MethodGet, "/plugin-help.js", nil)
 		if err != nil {
@@ -1024,7 +1024,7 @@ func TestHandleConfig(t *testing.T) {
 	configGetter := func() *config.Config {
 		return &c
 	}
-	handler := handleConfig(configGetter)
+	handler := handleConfig(configGetter, logrus.WithField("handler", "/config"))
 	req, err := http.NewRequest(http.MethodGet, "/config", nil)
 	if err != nil {
 		t.Fatalf("Error making request: %v", err)
@@ -1066,7 +1066,7 @@ func TestHandlePluginConfig(t *testing.T) {
 	}
 	pluginAgent := &plugins.ConfigAgent{}
 	pluginAgent.Set(&c)
-	handler := handlePluginConfig(pluginAgent)
+	handler := handlePluginConfig(pluginAgent, logrus.WithField("handler", "/plugin-config"))
 	req, err := http.NewRequest(http.MethodGet, "/config", nil)
 	if err != nil {
 		t.Fatalf("Error making request: %v", err)

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/hook/BUILD.bazel b/prow/hook/BUILD.bazel
@@ -26,59 +26,14 @@ go_library(
     srcs = [
         "events.go",
         "metrics.go",
-        "plugins.go",
         "server.go",
     ],
     importpath = "k8s.io/test-infra/prow/hook",
     deps = [
         "//prow/config:go_default_library",
         "//prow/github:go_default_library",
+        "//prow/hook/plugin-imports:go_default_library",
         "//prow/plugins:go_default_library",
-        "//prow/plugins/approve:go_default_library",
-        "//prow/plugins/assign:go_default_library",
-        "//prow/plugins/blockade:go_default_library",
-        "//prow/plugins/blunderbuss:go_default_library",
-        "//prow/plugins/branchcleaner:go_default_library",
-        "//prow/plugins/bugzilla:go_default_library",
-        "//prow/plugins/buildifier:go_default_library",
-        "//prow/plugins/cat:go_default_library",
-        "//prow/plugins/cherrypickunapproved:go_default_library",
-        "//prow/plugins/cla:go_default_library",
-        "//prow/plugins/dco:go_default_library",
-        "//prow/plugins/docs-no-retest:go_default_library",
-        "//prow/plugins/dog:go_default_library",
-        "//prow/plugins/golint:go_default_library",
-        "//prow/plugins/heart:go_default_library",
-        "//prow/plugins/help:go_default_library",
-        "//prow/plugins/hold:go_default_library",
-        "//prow/plugins/invalidcommitmsg:go_default_library",
-        "//prow/plugins/label:go_default_library",
-        "//prow/plugins/lgtm:go_default_library",
-        "//prow/plugins/lifecycle:go_default_library",
-        "//prow/plugins/mergecommitblocker:go_default_library",
-        "//prow/plugins/milestone:go_default_library",
-        "//prow/plugins/milestoneapplier:go_default_library",
-        "//prow/plugins/milestonestatus:go_default_library",
-        "//prow/plugins/override:go_default_library",
-        "//prow/plugins/owners-label:go_default_library",
-        "//prow/plugins/pony:go_default_library",
-        "//prow/plugins/project:go_default_library",
-        "//prow/plugins/releasenote:go_default_library",
-        "//prow/plugins/require-matching-label:go_default_library",
-        "//prow/plugins/requiresig:go_default_library",
-        "//prow/plugins/retitle:go_default_library",
-        "//prow/plugins/shrug:go_default_library",
-        "//prow/plugins/sigmention:go_default_library",
-        "//prow/plugins/size:go_default_library",
-        "//prow/plugins/skip:go_default_library",
-        "//prow/plugins/slackevents:go_default_library",
-        "//prow/plugins/stage:go_default_library",
-        "//prow/plugins/trigger:go_default_library",
-        "//prow/plugins/updateconfig:go_default_library",
-        "//prow/plugins/verify-owners:go_default_library",
-        "//prow/plugins/welcome:go_default_library",
-        "//prow/plugins/wip:go_default_library",
-        "//prow/plugins/yuks:go_default_library",
         "@com_github_prometheus_client_golang//prometheus:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
     ],
@@ -93,6 +48,9 @@ filegroup(
 
 filegroup(
     name = "all-srcs",
-    srcs = [":package-srcs"],
+    srcs = [
+        ":package-srcs",
+        "//prow/hook/plugin-imports:all-srcs",
+    ],
     tags = ["automanaged"],
 )
diff --git a/prow/hook/plugin-imports/BUILD.bazel b/prow/hook/plugin-imports/BUILD.bazel
@@ -0,0 +1,69 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["plugin-imports.go"],
+    importpath = "k8s.io/test-infra/prow/hook/plugin-imports",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//prow/plugins/approve:go_default_library",
+        "//prow/plugins/assign:go_default_library",
+        "//prow/plugins/blockade:go_default_library",
+        "//prow/plugins/blunderbuss:go_default_library",
+        "//prow/plugins/branchcleaner:go_default_library",
+        "//prow/plugins/bugzilla:go_default_library",
+        "//prow/plugins/buildifier:go_default_library",
+        "//prow/plugins/cat:go_default_library",
+        "//prow/plugins/cherrypickunapproved:go_default_library",
+        "//prow/plugins/cla:go_default_library",
+        "//prow/plugins/dco:go_default_library",
+        "//prow/plugins/docs-no-retest:go_default_library",
+        "//prow/plugins/dog:go_default_library",
+        "//prow/plugins/golint:go_default_library",
+        "//prow/plugins/heart:go_default_library",
+        "//prow/plugins/help:go_default_library",
+        "//prow/plugins/hold:go_default_library",
+        "//prow/plugins/invalidcommitmsg:go_default_library",
+        "//prow/plugins/label:go_default_library",
+        "//prow/plugins/lgtm:go_default_library",
+        "//prow/plugins/lifecycle:go_default_library",
+        "//prow/plugins/mergecommitblocker:go_default_library",
+        "//prow/plugins/milestone:go_default_library",
+        "//prow/plugins/milestoneapplier:go_default_library",
+        "//prow/plugins/milestonestatus:go_default_library",
+        "//prow/plugins/override:go_default_library",
+        "//prow/plugins/owners-label:go_default_library",
+        "//prow/plugins/pony:go_default_library",
+        "//prow/plugins/project:go_default_library",
+        "//prow/plugins/releasenote:go_default_library",
+        "//prow/plugins/require-matching-label:go_default_library",
+        "//prow/plugins/requiresig:go_default_library",
+        "//prow/plugins/retitle:go_default_library",
+        "//prow/plugins/shrug:go_default_library",
+        "//prow/plugins/sigmention:go_default_library",
+        "//prow/plugins/size:go_default_library",
+        "//prow/plugins/skip:go_default_library",
+        "//prow/plugins/slackevents:go_default_library",
+        "//prow/plugins/stage:go_default_library",
+        "//prow/plugins/trigger:go_default_library",
+        "//prow/plugins/updateconfig:go_default_library",
+        "//prow/plugins/verify-owners:go_default_library",
+        "//prow/plugins/welcome:go_default_library",
+        "//prow/plugins/wip:go_default_library",
+        "//prow/plugins/yuks:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+    visibility = ["//visibility:public"],
+)
diff --git a/prow/hook/plugins.go → prow/hook/plugin-imports/plugin-imports.go b/prow/hook/plugins.go → prow/hook/plugin-imports/plugin-imports.go
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package hook
+package pluginimports
 
 // We need to empty import all enabled plugins so that they will be linked into
 // any hook binary.

diff --git a/prow/hook/server.go b/prow/hook/server.go
@@ -31,6 +31,7 @@ import (
 
 	"k8s.io/test-infra/prow/config"
 	"k8s.io/test-infra/prow/github"
+	_ "k8s.io/test-infra/prow/hook/plugin-imports"
 	"k8s.io/test-infra/prow/plugins"
 )
 

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",