pj-rehearse: Disable rehearsals on tests where the 'restrict_network_access' config is toggled to 'false'

cmd/config-change-trigger/main.go

@@ -105,7 +105,7 @@ func main() {
 	if prConfig.CiOperator == nil || masterConfig.CiOperator == nil {
 		logger.WithError(err).Fatal("could not load ci-operator configs from base or tested revision of release repo")
 	}
-	changedCiopConfigs, _ := diffs.GetChangedCiopConfigs(masterConfig.CiOperator, prConfig.CiOperator, logger)
+	changedCiopConfigs, _, _ := diffs.GetChangedCiopConfigs(masterConfig.CiOperator, prConfig.CiOperator, logger)
 	changedImagesPostsubmits := diffs.GetImagesPostsubmitsForCiopConfigs(prConfig.Prow, changedCiopConfigs)
 
 	namespace := prConfig.Prow.ProwJobNamespace

cmd/pj-rehearse/main.go

@@ -172,7 +172,7 @@ func dryRun(o options, logger *logrus.Entry) error {
 	candidatePath := dro.dryRunPath
 	candidate := rehearse.RehearsalCandidateFromPullRequest(pr, pr.Base.SHA)
 
-	presubmits, periodics, changedTemplates, changedClusterProfiles, err := rc.DetermineAffectedJobs(candidate, candidatePath, logger)
+	presubmits, periodics, changedTemplates, changedClusterProfiles, _, err := rc.DetermineAffectedJobs(candidate, candidatePath, logger)
 	if err != nil {
 		return fmt.Errorf("error determining affected jobs: %w: %s", err, "ERROR: pj-rehearse: misconfiguration")
 	}

cmd/pj-rehearse/server.go

@@ -165,7 +165,7 @@ func (s *server) respondToNewPR(pullRequest *github.PullRequest, logger *logrus.
 	repo := pullRequest.Base.Repo.Name
 	number := pullRequest.Number
 	user := pullRequest.User.Login
-	presubmits, periodics, err := s.getAffectedJobs(pullRequest, logger)
+	presubmits, periodics, disabledDueToNetworkAccessToggle, err := s.getAffectedJobs(pullRequest, logger)
 	if err != nil {
 		comment := "unable to determine affected jobs. This could be due to a branch that needs to be rebased."
 		s.reportFailure(comment, err, org, repo, user, number, false, true, logger)
@@ -177,6 +177,7 @@ func (s *server) respondToNewPR(pullRequest *github.PullRequest, logger *logrus.
 	}
 
 	lines, jobCount := s.getJobsTableLines(presubmits, periodics, user)
+	lines = append(lines, s.getDisabledRehearsalsLines(disabledDueToNetworkAccessToggle)...)
 	if foundJobsToRehearse {
 		if jobCount > s.rehearsalConfig.MaxLimit {
 			fileLocation := s.dumpAffectedJobsToGCS(pullRequest, presubmits, periodics, jobCount, logger)
@@ -226,7 +227,7 @@ func (s *server) handleNewPush(l *logrus.Entry, event github.PullRequestEvent) {
 			}
 		}
 
-		presubmits, periodics, err := s.getAffectedJobs(pullRequest, logger)
+		presubmits, periodics, disabledDueToNetworkAccessToggle, err := s.getAffectedJobs(pullRequest, logger)
 		user := pullRequest.User.Login
 		if err != nil {
 			comment := "unable to determine affected jobs. This could be due to a branch that needs to be rebased."
@@ -249,6 +250,7 @@ func (s *server) handleNewPush(l *logrus.Entry, event github.PullRequestEvent) {
 			fileLocation := s.dumpAffectedJobsToGCS(pullRequest, presubmits, periodics, jobCount, logger)
 			jobTableLines = append(jobTableLines, fmt.Sprintf("A full list of affected jobs can be found [here](%s%s)", s.rehearsalConfig.GCSBrowserPrefix, fileLocation))
 		}
+		jobTableLines = append(jobTableLines, s.getDisabledRehearsalsLines(disabledDueToNetworkAccessToggle)...)
 		jobTableLines = append(jobTableLines, s.getUsageDetailsLines()...)
 		if err := s.ghc.CreateComment(org, repo, number, strings.Join(jobTableLines, "\n")); err != nil {
 			logger.WithError(err).Error("failed to create comment")
@@ -345,7 +347,7 @@ func (s *server) handlePotentialCommands(pullRequest *github.PullRequest, commen
 				//TODO(DPTP-2888): this is the point at which we can use repoClient.RevParse() to see if we even need to load the configs at all, and also prune the set of loaded configs to only the changed files
 
 				candidatePath := repoClient.Directory()
-				presubmits, periodics, changedTemplates, changedClusterProfiles, err := rc.DetermineAffectedJobs(candidate, candidatePath, logger)
+				presubmits, periodics, changedTemplates, changedClusterProfiles, _, err := rc.DetermineAffectedJobs(candidate, candidatePath, logger)
 				if err != nil {
 					logger.WithError(err).Error("couldn't determine affected jobs")
 					s.reportFailure("unable to determine affected jobs", err, org, repo, user, number, true, false, logger)
@@ -409,14 +411,14 @@ func (s *server) handlePotentialCommands(pullRequest *github.PullRequest, commen
 	}
 }
 
-func (s *server) getAffectedJobs(pullRequest *github.PullRequest, logger *logrus.Entry) (config.Presubmits, config.Periodics, error) {
+func (s *server) getAffectedJobs(pullRequest *github.PullRequest, logger *logrus.Entry) (config.Presubmits, config.Periodics, []string, error) {
 	rc := s.rehearsalConfig
 	org := pullRequest.Base.Repo.Owner.Login
 	repo := pullRequest.Base.Repo.Name
 	repoClient, err := s.getRepoClient(org, repo)
 	if err != nil {
 		logger.WithError(err).Error("couldn't create repo client")
-		return nil, nil, fmt.Errorf("couldn't create repo client: %w", err)
+		return nil, nil, nil, fmt.Errorf("couldn't create repo client: %w", err)
 	}
 	defer func() {
 		if err := repoClient.Clean(); err != nil {
@@ -427,14 +429,14 @@ func (s *server) getAffectedJobs(pullRequest *github.PullRequest, logger *logrus
 	candidate, err := s.prepareCandidate(repoClient, pullRequest)
 	if err != nil {
 		logger.WithError(err).Error("couldn't prepare candidate")
-		return nil, nil, fmt.Errorf("couldn't prepare candidate: %w", err)
+		return nil, nil, nil, fmt.Errorf("couldn't prepare candidate: %w", err)
 	}
 
 	//TODO(DPTP-2888): this is the point at which we can use repoClient.RevParse() to see if we even need to load the configs at all, and also prune the set of loaded configs to only the changed files
 
 	candidatePath := repoClient.Directory()
-	presubmits, periodics, _, _, err := rc.DetermineAffectedJobs(candidate, candidatePath, logger)
-	return presubmits, periodics, err
+	presubmits, periodics, _, _, disabledDueToNetworkAccessToggle, err := rc.DetermineAffectedJobs(candidate, candidatePath, logger)
+	return presubmits, periodics, disabledDueToNetworkAccessToggle, err
 }
 
 func (s *server) reportFailure(message string, err error, org, repo, user string, number int, addContact, addUsageDetails bool, l *logrus.Entry) {
@@ -551,6 +553,20 @@ func (s *server) getUsageDetailsLines() []string {
 	}
 }
 
+func (s *server) getDisabledRehearsalsLines(disabledDueToNetworkAccessToggle []string) []string {
+	var lines []string
+	if len(disabledDueToNetworkAccessToggle) > 0 {
+		lines = []string{
+			"The following jobs are not rehearsable due to the `restrict_network_access` field being set to `false` in this PR. You must first merge this PR, and then subsequent changes to the job will be rehearsable: ",
+			"",
+			"Test name |",
+			"--- |",
+		}
+		lines = append(lines, disabledDueToNetworkAccessToggle...)
+	}
+	return lines
+}
+
 func (s *server) acknowledgeRehearsals(org, repo string, number int, logger *logrus.Entry) {
 	if err := s.ghc.AddLabel(org, repo, number, rehearse.RehearsalsAckLabel); err != nil {
 		logger.WithError(err).Errorf("failed to add '%s' label", rehearse.RehearsalsAckLabel)

pkg/diffs/diffs.go

@@ -30,17 +30,17 @@ const (
 
 // GetChangedCiopConfigs identifies CI Operator configurations that are new or have changed and
 // determines for each which jobs are impacted if job-specific changes were made
-func GetChangedCiopConfigs(masterConfig, prConfig config.DataByFilename, logger *logrus.Entry) (config.DataByFilename, map[string]sets.Set[string]) {
-	ret := config.DataByFilename{}
-	affectedJobs := map[string]sets.Set[string]{}
+func GetChangedCiopConfigs(masterConfig, prConfig config.DataByFilename, logger *logrus.Entry) (configs config.DataByFilename, affectedJobs map[string]sets.Set[string], disabledDueToNetworkAccessToggle []string) {
+	configs = config.DataByFilename{}
+	affectedJobs = map[string]sets.Set[string]{}
 
 	for filename, newConfig := range prConfig {
 		oldConfig, ok := masterConfig[filename]
 		jobs := sets.New[string]()
 
 		// new ciop config
 		if !ok {
-			ret[filename] = newConfig
+			configs[filename] = newConfig
 			logger.WithField(logCiopConfig, filename).Info(newCiopConfigMsg)
 			continue
 		}
@@ -57,26 +57,43 @@ func GetChangedCiopConfigs(masterConfig, prConfig config.DataByFilename, logger
 
 		if !equality.Semantic.DeepEqual(withoutTests(oldConfig.Configuration), withoutTests(newConfig.Configuration)) {
 			logger.WithField(logCiopConfig, filename).Info(changedCiopConfigMsg)
-			ret[filename] = newConfig
+			configs[filename] = newConfig
 			continue
 		}
 
 		oldTests := getTestsByName(oldConfig.Configuration.Tests)
 		newTests := getTestsByName(newConfig.Configuration.Tests)
 
 		for as, test := range newTests {
-			if !equality.Semantic.DeepEqual(oldTests[as], test) {
-				logger.WithField(logCiopConfig, filename).Info(changedCiopConfigMsg)
-				ret[filename] = newConfig
-				jobs.Insert(as)
+			oldTest := oldTests[as]
+			if !equality.Semantic.DeepEqual(oldTest, test) {
+				testLogger := logger.WithField(logCiopConfig, filename)
+				testLogger.Info(changedCiopConfigMsg)
+				configs[filename] = newConfig
+
+				// We don't allow rehearsals of tests that specifically toggle 'restrict_network_access' off
+				nonRehearsableBecauseNetworkRestrictionOff := false
+				if oldTest.RestrictNetworkAccess != test.RestrictNetworkAccess {
+					nonRehearsableBecauseNetworkRestrictionOff = test.RestrictNetworkAccess != nil && !*test.RestrictNetworkAccess
+				}
+				if nonRehearsableBecauseNetworkRestrictionOff {
+					testLogger.Debug("new test configuration has 'restrict_network_access' set to false, not rehearsable")
+					prefix := jobconfig.PresubmitPrefix
+					if test.IsPeriodic() {
+						prefix = jobconfig.PeriodicPrefix
+					}
+					disabledDueToNetworkAccessToggle = append(disabledDueToNetworkAccessToggle, newConfig.Info.JobName(prefix, as))
+				} else {
+					jobs.Insert(as)
+				}
 			}
 		}
 
 		if len(jobs) > 0 {
 			affectedJobs[filename] = jobs
 		}
 	}
-	return ret, affectedJobs
+	return
 }
 
 // GetChangedPresubmits returns a mapping of repo to presubmits to execute.

pkg/diffs/diffs_test.go

@@ -12,6 +12,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/util/sets"
+	utilpointer "k8s.io/utils/pointer"
 	pjapi "sigs.k8s.io/prow/pkg/apis/prowjobs/v1"
 	prowconfig "sigs.k8s.io/prow/pkg/config"
 
@@ -62,10 +63,11 @@ func TestGetChangedCiopConfigs(t *testing.T) {
 	}
 
 	testCases := []struct {
-		name                 string
-		configGenerator      func(*testing.T) (before, after config.DataByFilename)
-		expected             func() config.DataByFilename
-		expectedAffectedJobs map[string]sets.Set[string]
+		name                                     string
+		configGenerator                          func(*testing.T) (before, after config.DataByFilename)
+		expected                                 func() config.DataByFilename
+		expectedAffectedJobs                     map[string]sets.Set[string]
+		expectedDisabledDueToNetworkAccessToggle []string
 	}{{
 		name: "no changes",
 		configGenerator: func(t *testing.T) (config.DataByFilename, config.DataByFilename) {
@@ -110,72 +112,132 @@ func TestGetChangedCiopConfigs(t *testing.T) {
 			return config.DataByFilename{"org-repo-branch.yaml": expected}
 		},
 		expectedAffectedJobs: map[string]sets.Set[string]{},
-	},
-		{
-			name: "changed tests",
-			configGenerator: func(t *testing.T) (config.DataByFilename, config.DataByFilename) {
-				before := config.DataByFilename{"org-repo-branch.yaml": baseCiopConfig}
-				afterConfig := config.DataWithInfo{}
-				if err := deepcopy.Copy(&afterConfig, &baseCiopConfig); err != nil {
-					t.Fatal(err)
-				}
-				afterConfig.Configuration.Tests[0].Commands = "changed commands"
-				after := config.DataByFilename{"org-repo-branch.yaml": afterConfig}
-				return before, after
-			},
-			expected: func() config.DataByFilename {
-				expected := config.DataWithInfo{}
-				if err := deepcopy.Copy(&expected, &baseCiopConfig); err != nil {
-					t.Fatal(err)
-				}
-				expected.Configuration.Tests[0].Commands = "changed commands"
-				return config.DataByFilename{"org-repo-branch.yaml": expected}
+	}, {
+		name: "changed tests",
+		configGenerator: func(t *testing.T) (config.DataByFilename, config.DataByFilename) {
+			before := config.DataByFilename{"org-repo-branch.yaml": baseCiopConfig}
+			afterConfig := config.DataWithInfo{}
+			if err := deepcopy.Copy(&afterConfig, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			afterConfig.Configuration.Tests[0].Commands = "changed commands"
+			after := config.DataByFilename{"org-repo-branch.yaml": afterConfig}
+			return before, after
+		},
+		expected: func() config.DataByFilename {
+			expected := config.DataWithInfo{}
+			if err := deepcopy.Copy(&expected, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			expected.Configuration.Tests[0].Commands = "changed commands"
+			return config.DataByFilename{"org-repo-branch.yaml": expected}
+		},
+		expectedAffectedJobs: map[string]sets.Set[string]{"org-repo-branch.yaml": {"unit": sets.Empty{}}},
+	}, {
+		name: "changed multiple tests",
+		configGenerator: func(t *testing.T) (config.DataByFilename, config.DataByFilename) {
+			before := config.DataByFilename{"org-repo-branch.yaml": baseCiopConfig}
+			afterConfig := config.DataWithInfo{}
+			if err := deepcopy.Copy(&afterConfig, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			afterConfig.Configuration.Tests[0].Commands = "changed commands"
+			afterConfig.Configuration.Tests[1].Commands = "changed commands"
+			after := config.DataByFilename{"org-repo-branch.yaml": afterConfig}
+			return before, after
+		},
+		expected: func() config.DataByFilename {
+			expected := config.DataWithInfo{}
+			if err := deepcopy.Copy(&expected, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			expected.Configuration.Tests[0].Commands = "changed commands"
+			expected.Configuration.Tests[1].Commands = "changed commands"
+			return config.DataByFilename{"org-repo-branch.yaml": expected}
+		},
+		expectedAffectedJobs: map[string]sets.Set[string]{
+			"org-repo-branch.yaml": {
+				"unit": sets.Empty{},
+				"e2e":  sets.Empty{},
 			},
-			expectedAffectedJobs: map[string]sets.Set[string]{"org-repo-branch.yaml": {"unit": sets.Empty{}}},
 		},
-		{
-			name: "changed multiple tests",
-			configGenerator: func(t *testing.T) (config.DataByFilename, config.DataByFilename) {
-				before := config.DataByFilename{"org-repo-branch.yaml": baseCiopConfig}
-				afterConfig := config.DataWithInfo{}
-				if err := deepcopy.Copy(&afterConfig, &baseCiopConfig); err != nil {
-					t.Fatal(err)
-				}
-				afterConfig.Configuration.Tests[0].Commands = "changed commands"
-				afterConfig.Configuration.Tests[1].Commands = "changed commands"
-				after := config.DataByFilename{"org-repo-branch.yaml": afterConfig}
-				return before, after
-			},
-			expected: func() config.DataByFilename {
-				expected := config.DataWithInfo{}
-				if err := deepcopy.Copy(&expected, &baseCiopConfig); err != nil {
-					t.Fatal(err)
-				}
-				expected.Configuration.Tests[0].Commands = "changed commands"
-				expected.Configuration.Tests[1].Commands = "changed commands"
-				return config.DataByFilename{"org-repo-branch.yaml": expected}
-			},
-			expectedAffectedJobs: map[string]sets.Set[string]{
-				"org-repo-branch.yaml": {
-					"unit": sets.Empty{},
-					"e2e":  sets.Empty{},
-				},
+	}, {
+		name: "one potential rehearsal disabled due to un-restricted network access toggle 'true' to 'false'",
+		configGenerator: func(t *testing.T) (config.DataByFilename, config.DataByFilename) {
+			before := config.DataByFilename{"org-repo-branch.yaml": baseCiopConfig}
+			afterConfig := config.DataWithInfo{}
+			if err := deepcopy.Copy(&afterConfig, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			afterConfig.Configuration.Tests[0].RestrictNetworkAccess = utilpointer.Bool(false)
+			afterConfig.Configuration.Tests[1].RestrictNetworkAccess = utilpointer.Bool(true)
+			after := config.DataByFilename{"org-repo-branch.yaml": afterConfig}
+			return before, after
+		},
+		expected: func() config.DataByFilename {
+			expected := config.DataWithInfo{}
+			if err := deepcopy.Copy(&expected, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			expected.Configuration.Tests[0].RestrictNetworkAccess = utilpointer.Bool(false)
+			expected.Configuration.Tests[1].RestrictNetworkAccess = utilpointer.Bool(true)
+			return config.DataByFilename{"org-repo-branch.yaml": expected}
+		},
+		expectedAffectedJobs: map[string]sets.Set[string]{
+			"org-repo-branch.yaml": {
+				"e2e": sets.Empty{},
 			},
 		},
+		expectedDisabledDueToNetworkAccessToggle: []string{"pull-ci-org-repo-branch-unit"},
+	}, {
+		name: "potential rehearsal for new test disabled due to un-restricted network access set to 'false'",
+		configGenerator: func(t *testing.T) (config.DataByFilename, config.DataByFilename) {
+			before := config.DataByFilename{"org-repo-branch.yaml": baseCiopConfig}
+			afterConfig := config.DataWithInfo{}
+			if err := deepcopy.Copy(&afterConfig, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			afterConfig.Configuration.Tests = append(afterConfig.Configuration.Tests, cioperatorapi.TestStepConfiguration{
+				As:                    "lint",
+				Commands:              "make lint",
+				RestrictNetworkAccess: utilpointer.Bool(false),
+			})
+			after := config.DataByFilename{"org-repo-branch.yaml": afterConfig}
+			return before, after
+		},
+		expected: func() config.DataByFilename {
+			expected := config.DataWithInfo{}
+			if err := deepcopy.Copy(&expected, &baseCiopConfig); err != nil {
+				t.Fatal(err)
+			}
+			expected.Configuration.Tests = append(expected.Configuration.Tests, cioperatorapi.TestStepConfiguration{
+				As:                    "lint",
+				Commands:              "make lint",
+				RestrictNetworkAccess: utilpointer.Bool(false),
+			})
+			return config.DataByFilename{"org-repo-branch.yaml": expected}
+		},
+		expectedAffectedJobs:                     map[string]sets.Set[string]{},
+		expectedDisabledDueToNetworkAccessToggle: []string{"pull-ci-org-repo-branch-lint"},
+	},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			before, after := tc.configGenerator(t)
-			actual, affectedJobs := GetChangedCiopConfigs(before, after, logrus.NewEntry(logrus.New()))
+			actual, affectedJobs, disabledDueToNetworkAccessToggle := GetChangedCiopConfigs(before, after, logrus.NewEntry(logrus.New()))
 			expected := tc.expected()
 
-			if !reflect.DeepEqual(expected, actual) {
-				t.Errorf("Detected changed ci-operator config changes differ from expected:\n%s", cmp.Diff(expected, actual))
+			if diff := cmp.Diff(expected, actual); diff != "" {
+				t.Errorf("Detected changed ci-operator config changes differ from expected:\n%s", diff)
+			}
+
+			if diff := cmp.Diff(tc.expectedAffectedJobs, affectedJobs, ignoreUnexported); diff != "" {
+				t.Errorf("Affected jobs differ from expected:\n%s", diff)
 			}
 
-			if !reflect.DeepEqual(tc.expectedAffectedJobs, affectedJobs) {
-				t.Errorf("Affected jobs differ from expected:\n%s", cmp.Diff(tc.expectedAffectedJobs, affectedJobs, ignoreUnexported))
+			if diff := cmp.Diff(tc.expectedDisabledDueToNetworkAccessToggle, disabledDueToNetworkAccessToggle); diff != "" {
+				t.Errorf("Actual disabledDueToNetworkAccessToggle differs from expected:\n%s", diff)
 			}
 		})
 	}