tests: ensure that pre-submits get additional reviews

Makefile

@@ -59,6 +59,13 @@ update-go-deps:
 .PHONY: verify-go-deps
 verify-go-deps:
 	hack/make-rules/verify/go-deps.sh
+.PHONY: update-unit
+.PHONY: update-go-unit
+update-unit: update-go-unit
+	UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh
+.PHONY: update-config-fixture
+update-config-fixture:
+	UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh ./config/tests/...
 # ======================== Image Building/Publishing ===========================
 # Build and publish miscellaneous images that get pushed to the specified REGISTRY.
 # The full set of images covered by these targets is configured in

config/jobs/kubernetes/sig-api-machinery/sig-api-machinery-config.yaml

@@ -80,6 +80,7 @@ presubmits:
     cluster: k8s-infra-prow-build
     always_run: false
     run_if_changed: '^(cluster/gce/manifests/konnectivity-server.yaml$|cluster/gce/addons/konnectivity-agent)'
+    optional: true # Doesn't always run and regressions shouldn't block merging PRs where it gets run.
     labels:
       preset-service-account: "true"
       preset-k8s-ssh: "true"

config/testgrids/kubernetes/presubmits/config.yaml

@@ -48,9 +48,6 @@ dashboards:
   - name: pull-kubernetes-dependencies
     test_group_name: pull-kubernetes-dependencies
     base_options: width=10
-  - name: pull-kubernetes-e2e-gce-network-proxy-http-connect
-    test_group_name: pull-kubernetes-e2e-gce-network-proxy-http-connect
-    base_options: width=10
   - name: pull-kubernetes-conformance-kind-ga-only-parallel
     test_group_name: pull-kubernetes-conformance-kind-ga-only-parallel
     base_options: width=10

config/tests/jobs/README.md

@@ -8,4 +8,7 @@ To run via bazel: `bazel test //config/tests/jobs/...`
 
 To run via go: `go test .`
 
+If tests fail, re-run with the `UPDATE_FIXTURE_DATA=true` env variable
+and include the modified files in the PR which updates the jobs.
+
 [prow.k8s.io]: https://prow.k8s.io

config/tests/jobs/policy/OWNERS

@@ -0,0 +1,17 @@
+# See the OWNERS docs at https://go.k8s.io/owners
+
+options:
+  no_parent_owners: true
+
+reviewers:
+- test-infra-oncall # oncall
+- BenTheElder # lead
+- aojea # lead
+approvers:
+- test-infra-oncall # oncall
+- BenTheElder # lead
+- aojea # lead
+
+labels:
+- sig/testing
+- sig/infra

config/tests/jobs/policy/README.md

@@ -0,0 +1,13 @@
+The project has certain guidelines around jobs which are meant to ensure that
+there's a balance between test coverage and costs for running the CI. For
+example, non-blocking jobs that get trigger automatically for PRs should be
+used judiciously.
+
+Because SIG leads are not necessarily familiar with those policies, SIG Testing
+and SIG Infra need to be involved before merging jobs that fall into those
+sensitive areas. This is achieved with tests and additional files in this
+directory and a separate OWNERS file.
+
+To check whether jobs are okay, run the Go tests in this directory.
+If tests fail, re-run with the `UPDATE_FIXTURE_DATA=true` env variable
+and include the modified files in the PR which updates the jobs.

config/tests/jobs/policy/policy_test.go

@@ -0,0 +1,254 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package policy
+
+// This file validates Kubernetes's jobs configs against policies.
+
+import (
+	"bytes"
+	"flag"
+	"fmt"
+	"os"
+	"path/filepath"
+	"slices"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	yaml "sigs.k8s.io/yaml/goyaml.v3"
+
+	cfg "sigs.k8s.io/prow/pkg/config"
+)
+
+var configPath = flag.String("config", "../../../../config/prow/config.yaml", "Path to prow config")
+var jobConfigPath = flag.String("job-config", "../../../jobs", "Path to prow job config")
+
+// Loaded at TestMain.
+var c *cfg.Config
+
+func TestMain(m *testing.M) {
+	flag.Parse()
+	if *configPath == "" {
+		fmt.Println("--config must set")
+		os.Exit(1)
+	}
+
+	conf, err := cfg.Load(*configPath, *jobConfigPath, nil, "")
+	if err != nil {
+		fmt.Printf("Could not load config: %v", err)
+		os.Exit(1)
+	}
+	c = conf
+
+	os.Exit(m.Run())
+}
+
+func TestKubernetesPresubmitJobs(t *testing.T) {
+	jobs := c.AllStaticPresubmits([]string{"kubernetes/kubernetes"})
+	var expected presubmitJobs
+
+	for _, job := range jobs {
+		if !job.AlwaysRun && job.RunIfChanged == "" {
+			// Manually triggered, no additional review needed.
+			continue
+		}
+
+		// Mirror those attributes of the job which must trigger additional reviews
+		// or are needed to identify the job.
+		j := presubmitJob{
+			Name:         job.Name,
+			SkipBranches: job.SkipBranches,
+			Branches:     job.Branches,
+
+			RunIfChanged:      job.RunIfChanged,
+			SkipIfOnlyChanged: job.SkipIfOnlyChanged,
+		}
+
+		// This uses separate top-level fields instead of job attributes to
+		// make it more obvious when run_if_changed is used.
+		if job.AlwaysRun {
+			expected.AlwaysRun = append(expected.AlwaysRun, j)
+		} else {
+			expected.RunIfChanged = append(expected.RunIfChanged, j)
+
+			if !job.Optional {
+				// Absolute path is more user-friendly than ../../config/...
+				t.Errorf("Policy violation: %s in %s should use `optional: true` or `alwaysRun: true`.", job.Name, maybeAbsPath(job.SourcePath))
+			}
+		}
+
+	}
+	expected.Normalize()
+
+	// Encode the expected content.
+	var expectedData bytes.Buffer
+	if _, err := expectedData.Write([]byte(`# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT!
+
+`)); err != nil {
+		t.Fatalf("unexpected error writing into buffer: %v", err)
+	}
+
+	encoder := yaml.NewEncoder(&expectedData)
+	encoder.SetIndent(4)
+	if err := encoder.Encode(expected); err != nil {
+		t.Fatalf("unexpected error encoding %s: %v", presubmitsFile, err)
+	}
+
+	// Compare. This proceeds on read or decoding errors because
+	// the file might get re-generated below.
+	var actual presubmitJobs
+	actualData, err := os.ReadFile(presubmitsFile)
+	if err != nil && !os.IsNotExist(err) {
+		t.Errorf("unexpected error: %v", err)
+	}
+	if err := yaml.Unmarshal(actualData, &actual); err != nil {
+		t.Errorf("unexpected error decoding %s: %v", presubmitsFile, err)
+	}
+
+	// First check the in-memory structs. The diff is nicer for them (more context).
+	diff := cmp.Diff(actual, expected)
+	if diff == "" {
+		// Next check the encoded data. This should only be different on test updates.
+		diff = cmp.Diff(string(actualData), expectedData.String(), cmpopts.AcyclicTransformer("SplitLines", func(s string) []string {
+			return strings.Split(s, "\n")
+		}))
+	}
+
+	if diff != "" {
+		if value, _ := os.LookupEnv("UPDATE_FIXTURE_DATA"); value == "true" {
+			if err := os.WriteFile(presubmitsFile, expectedData.Bytes(), 0644); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			t.Logf(`
+%s was out-dated. Updated as requested with the following changes (- actual, + expected):
+%s
+`, maybeAbsPath(presubmitsFile), diff)
+		} else {
+			t.Errorf(`
+%s is out-dated. Detected differences (- actual, + expected):
+%s
+
+Blocking pre-submit jobs must be for stable, important features.
+Non-blocking pre-submit jobs should only be run automatically if they meet
+the criteria outlined in https://github.com/kubernetes/community/pull/8196.
+
+To ensure that this is considered when defining pre-submit jobs, they
+need to be listed in %s. If the pre-submit job is really needed,
+re-run the test with UPDATE_FIXTURE_DATA=true and include the modified
+file. The following command can be used:
+
+   make update-config-fixture
+`, presubmitsFile, diff, presubmitsFile)
+		}
+	}
+}
+
+// presubmitsFile contains the following struct.
+const presubmitsFile = "presubmit-jobs.yaml"
+
+type presubmitJobs struct {
+	AlwaysRun    []presubmitJob `yaml:"always_run"`
+	RunIfChanged []presubmitJob `yaml:"run_if_changed"`
+}
+type presubmitJob struct {
+	Name              string   `yaml:"name"`
+	SkipBranches      []string `yaml:"skip_branches,omitempty"`
+	Branches          []string `yaml:"branches,omitempty"`
+	RunIfChanged      string   `yaml:"run_if_changed,omitempty"`
+	SkipIfOnlyChanged string   `yaml:"skip_if_only_changed,omitempty"`
+}
+
+func (p *presubmitJobs) Normalize() {
+	sortJobs(&p.AlwaysRun)
+	sortJobs(&p.RunIfChanged)
+}
+
+func sortJobs(jobs *[]presubmitJob) {
+	for _, job := range *jobs {
+		sort.Strings(job.SkipBranches)
+		sort.Strings(job.Branches)
+	}
+	sort.Slice(*jobs, func(i, j int) bool {
+		switch strings.Compare((*jobs)[i].Name, (*jobs)[j].Name) {
+		case -1:
+			return true
+		case 1:
+			return false
+		}
+		switch slices.Compare((*jobs)[i].SkipBranches, (*jobs)[j].SkipBranches) {
+		case -1:
+			return true
+		case 1:
+			return false
+		}
+		switch slices.Compare((*jobs)[i].Branches, (*jobs)[j].Branches) {
+		case -1:
+			return true
+		case 1:
+			return false
+		}
+		return false
+	})
+
+	// If a job has the same settings regardless of the branch, then
+	// we can reduce to a single entry without the branch info.
+	shorterJobs := make([]presubmitJob, 0, len(*jobs))
+	for i := 0; i < len(*jobs); {
+		job := (*jobs)[i]
+		job.Branches = nil
+		job.SkipBranches = nil
+
+		if sameSettings(*jobs, job) {
+			shorterJobs = append(shorterJobs, job)
+			// Fast-forward to next job.
+			for i < len(*jobs) && (*jobs)[i].Name == job.Name {
+				i++
+			}
+		} else {
+			// Keep all of the different entries.
+			for i < len(*jobs) && (*jobs)[i].Name == job.Name {
+				shorterJobs = append(shorterJobs, (*jobs)[i])
+			}
+		}
+	}
+	*jobs = shorterJobs
+}
+
+func sameSettings(jobs []presubmitJob, ref presubmitJob) bool {
+	for _, job := range jobs {
+		if job.Name != ref.Name {
+			continue
+		}
+		if job.RunIfChanged != ref.RunIfChanged ||
+			job.SkipIfOnlyChanged != ref.SkipIfOnlyChanged {
+			return false
+		}
+	}
+	return true
+}
+
+// maybeAbsPath tries to make a path absolute. This is useful because
+// relative paths in test output tend to be confusing when the user
+// invoked the test outside of the test's directory.
+func maybeAbsPath(path string) string {
+	if path, err := filepath.Abs(path); err == nil {
+		return path
+	}
+	return path
+}