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,23 @@ func (s *server) getUsageDetailsLines() []string {
 	}
 }
 
+func (s *server) getDisabledRehearsalsLines(disabledDueToNetworkAccessToggle []string) []string {
+	var lines []string
+	if len(disabledDueToNetworkAccessToggle) > 0 {
+		lines = append(lines, "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: ")
+		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 |",
+			"--- |",
+		}
+		for _, disabled := range disabledDueToNetworkAccessToggle {
+			lines = append(lines, disabled)
+		}
+	}
+	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.