From 4ce563f882b41c658621a9a57737aeb8ff849510 Mon Sep 17 00:00:00 2001 From: jonathanmetzman <31354670+jonathanmetzman@users.noreply.github.com> Date: Sun, 31 Oct 2021 20:36:07 -0400 Subject: Fix diffing on non-GitHub and improve config (#6707) Fix diffing on non-GitHub and improve config 1. Remove obsolete comments. 2. Remove unused function get_pr_ref 3. Correct comment on git_sha 4. Rename commit_sha to git_sha 5. Make base_commit, pr_ref, and base_ref non-github specific and move to CiEnvironment. 6. Hoist get_diff_base to base class to allow diffing 7. Fix downloading coverage on non-Github. 8. Add TODO so that we don't assume github actions is run on github.com (enterprise users) 9. Rename repo_url to git_url. 10. Add missing git_url method base class. 11. Clarify what git_url, git_sha and pr_ref are for and leave TODOs about how we can eliminate them. 12. Fix typos. --- infra/cifuzz/build_fuzzers.py | 10 +- infra/cifuzz/build_fuzzers_entrypoint.py | 14 +- infra/cifuzz/build_fuzzers_test.py | 20 +- infra/cifuzz/cifuzz_combined_entrypoint.py | 17 +- infra/cifuzz/clusterfuzz_deployment.py | 1 + infra/cifuzz/clusterfuzz_deployment_test.py | 11 +- infra/cifuzz/config_utils.py | 245 ++++++++++++++++----- infra/cifuzz/config_utils_test.py | 42 ++-- infra/cifuzz/continuous_integration.py | 52 ++--- .../github_actions/github_actions_test.py | 3 +- infra/cifuzz/run_fuzzers_entrypoint.py | 9 +- infra/cifuzz/run_fuzzers_test.py | 2 +- 12 files changed, 266 insertions(+), 160 deletions(-) (limited to 'infra/cifuzz') diff --git a/infra/cifuzz/build_fuzzers.py b/infra/cifuzz/build_fuzzers.py index 58f60f7d..c35115c4 100644 --- a/infra/cifuzz/build_fuzzers.py +++ b/infra/cifuzz/build_fuzzers.py @@ -154,16 +154,10 @@ def build_fuzzers(config): """Builds all of the fuzzers for a specific OSS-Fuzz project. Args: - project_name: The name of the OSS-Fuzz project being built. - project_repo_name: The name of the project's repo. - workspace: The location in a shared volume to store a git repo and build - artifacts. - pr_ref: The pull request reference to be built. - commit_sha: The commit sha for the project to be built at. - sanitizer: The sanitizer the fuzzers should be built with. + config: The configuration object for building fuzzers. Returns: - True if build succeeded or False on failure. + True if build succeeded. """ # Do some quick validation. if config.project_src_path and not check_project_src_path( diff --git a/infra/cifuzz/build_fuzzers_entrypoint.py b/infra/cifuzz/build_fuzzers_entrypoint.py index 5d701cf5..2a5bec6b 100644 --- a/infra/cifuzz/build_fuzzers_entrypoint.py +++ b/infra/cifuzz/build_fuzzers_entrypoint.py @@ -38,7 +38,7 @@ def build_fuzzers_entrypoint(): if not build_fuzzers.build_fuzzers(config): logging.error('Error building fuzzers for (commit: %s, pr_ref: %s).', - config.commit_sha, config.pr_ref) + config.git_sha, config.pr_ref) return returncode return 0 @@ -50,18 +50,6 @@ def main(): Note: The resulting fuzz target binaries of this build are placed in the directory: ${GITHUB_WORKSPACE}/out - Required environment variables: - OSS_FUZZ_PROJECT_NAME: The name of OSS-Fuzz project. - GITHUB_REPOSITORY: The name of the Github repo that called this script. - GITHUB_SHA: The commit SHA that triggered this script. - GITHUB_EVENT_NAME: The name of the hook event that triggered this script. - GITHUB_EVENT_PATH: - The path to the file containing the POST payload of the webhook: - https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners#filesystems-on-github-hosted-runners - GITHUB_WORKSPACE: The shared volume directory where input artifacts are. - DRY_RUN: If true, no failures will surface. - SANITIZER: The sanitizer to use when running fuzzers. - Returns: 0 on success or nonzero on failure. """ diff --git a/infra/cifuzz/build_fuzzers_test.py b/infra/cifuzz/build_fuzzers_test.py index 3bef0d40..31c055e4 100644 --- a/infra/cifuzz/build_fuzzers_test.py +++ b/infra/cifuzz/build_fuzzers_test.py @@ -93,7 +93,7 @@ class InternalGithubBuildTest(unittest.TestCase): """Tests for building OSS-Fuzz projects on GitHub actions.""" PROJECT_REPO_NAME = 'myproject' SANITIZER = 'address' - COMMIT_SHA = 'fake' + GIT_SHA = 'fake' PR_REF = 'fake' def _create_builder(self, tmp_dir, oss_fuzz_project_name='myproject'): @@ -103,7 +103,7 @@ class InternalGithubBuildTest(unittest.TestCase): project_repo_name=self.PROJECT_REPO_NAME, workspace=tmp_dir, sanitizer=self.SANITIZER, - commit_sha=self.COMMIT_SHA, + git_sha=self.GIT_SHA, pr_ref=self.PR_REF, is_github=True) ci_system = continuous_integration.get_ci(config) @@ -180,7 +180,7 @@ class BuildFuzzersIntegrationTest(unittest.TestCase): project_repo_name=project_repo_name, workspace=self.workspace, git_url=git_url, - commit_sha='HEAD', + git_sha='HEAD', is_github=True, base_commit='HEAD^1') self.assertTrue(build_fuzzers.build_fuzzers(config)) @@ -202,7 +202,7 @@ class BuildFuzzersIntegrationTest(unittest.TestCase): workspace=self.workspace, git_url=git_url, filestore='no_filestore', - commit_sha='HEAD', + git_sha='HEAD', project_src_path=project_src_path, base_commit='HEAD^1') self.assertTrue(build_fuzzers.build_fuzzers(config)) @@ -215,7 +215,7 @@ class BuildFuzzersIntegrationTest(unittest.TestCase): oss_fuzz_project_name=EXAMPLE_PROJECT, project_repo_name='oss-fuzz', workspace=self.workspace, - commit_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523', + git_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523', base_commit='da0746452433dc18bae699e355a9821285d863c8', is_github=True) self.assertTrue(build_fuzzers.build_fuzzers(config)) @@ -252,7 +252,7 @@ class BuildFuzzersIntegrationTest(unittest.TestCase): oss_fuzz_project_name='not_a_valid_project', project_repo_name='oss-fuzz', workspace=self.workspace, - commit_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523') + git_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523') self.assertFalse(build_fuzzers.build_fuzzers(config)) def test_invalid_repo_name(self): @@ -261,16 +261,16 @@ class BuildFuzzersIntegrationTest(unittest.TestCase): oss_fuzz_project_name=EXAMPLE_PROJECT, project_repo_name='not-real-repo', workspace=self.workspace, - commit_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523') + git_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523') self.assertFalse(build_fuzzers.build_fuzzers(config)) - def test_invalid_commit_sha(self): + def test_invalid_git_sha(self): """Tests building fuzzers with invalid commit SHA.""" config = test_helpers.create_build_config( oss_fuzz_project_name=EXAMPLE_PROJECT, project_repo_name='oss-fuzz', workspace=self.workspace, - commit_sha='', + git_sha='', is_github=True) with self.assertRaises(AssertionError): build_fuzzers.build_fuzzers(config) @@ -281,7 +281,7 @@ class BuildFuzzersIntegrationTest(unittest.TestCase): oss_fuzz_project_name=EXAMPLE_PROJECT, project_repo_name='oss-fuzz', workspace=os.path.join(self.workspace, 'not', 'a', 'dir'), - commit_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523') + git_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523') self.assertFalse(build_fuzzers.build_fuzzers(config)) diff --git a/infra/cifuzz/cifuzz_combined_entrypoint.py b/infra/cifuzz/cifuzz_combined_entrypoint.py index 008ce108..920e32e4 100644 --- a/infra/cifuzz/cifuzz_combined_entrypoint.py +++ b/infra/cifuzz/cifuzz_combined_entrypoint.py @@ -22,23 +22,10 @@ import run_fuzzers_entrypoint def main(): """Builds and runs fuzzers for CI tools. - NOTE: Any crash report will be in the filepath: - ${GITHUB_WORKSPACE}/out/testcase - This can be used with GitHub's upload-artifact action to surface the logs. + NOTE: Any crash report will be in the filepath: $WORKSPACE/out/testcase + This can be used with GitHub's upload-artifact action to surface the logs. Required environment variables: - OSS_FUZZ_PROJECT_NAME: The name of OSS-Fuzz project. - GITHUB_REPOSITORY: The name of the Github repo that called this script. - GITHUB_SHA: The commit SHA that triggered this script. - GITHUB_EVENT_NAME: The name of the hook event that triggered this script. - GITHUB_EVENT_PATH: - The path to the file containing the POST payload of the webhook: - https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners#filesystems-on-github-hosted-runners - GITHUB_WORKSPACE: The shared volume directory where input artifacts are. - DRY_RUN: If true, no failures will surface. - SANITIZER: The sanitizer to use when running fuzzers. - FUZZ_SECONDS: The length of time in seconds that fuzzers are to be run. - Returns: 0 on success or 1 on failure. """ diff --git a/infra/cifuzz/clusterfuzz_deployment.py b/infra/cifuzz/clusterfuzz_deployment.py index d95f80d3..74ad4867 100644 --- a/infra/cifuzz/clusterfuzz_deployment.py +++ b/infra/cifuzz/clusterfuzz_deployment.py @@ -203,6 +203,7 @@ class ClusterFuzzLite(BaseClusterFuzzDeployment): def get_coverage(self, repo_path): """Returns the project coverage object for the project.""" + _make_empty_dir_if_nonexistent(self.workspace.clusterfuzz_coverage) try: if not self.filestore.download_coverage( self.COVERAGE_NAME, self.workspace.clusterfuzz_coverage): diff --git a/infra/cifuzz/clusterfuzz_deployment_test.py b/infra/cifuzz/clusterfuzz_deployment_test.py index e0e94095..a7cbf3d2 100644 --- a/infra/cifuzz/clusterfuzz_deployment_test.py +++ b/infra/cifuzz/clusterfuzz_deployment_test.py @@ -255,12 +255,13 @@ class GetClusterFuzzDeploymentTest(unittest.TestCase): return_value=platform, new_callable=mock.PropertyMock): with mock.patch('filestore_utils.get_filestore', return_value=None): - config = _create_config() - workspace = workspace_utils.Workspace(config) + with mock.patch('config_utils._get_event_data', return_value={}): + config = _create_config() + workspace = workspace_utils.Workspace(config) - self.assertIsInstance( - clusterfuzz_deployment.get_clusterfuzz_deployment( - config, workspace), expected_deployment_cls) + self.assertIsInstance( + clusterfuzz_deployment.get_clusterfuzz_deployment( + config, workspace), expected_deployment_cls) if __name__ == '__main__': diff --git a/infra/cifuzz/config_utils.py b/infra/cifuzz/config_utils.py index d756a30d..b29f3095 100644 --- a/infra/cifuzz/config_utils.py +++ b/infra/cifuzz/config_utils.py @@ -40,12 +40,6 @@ DEFAULT_ARCHITECTURE = 'x86_64' # that are supposed to be strings. -def _get_pr_ref(event): - if event == 'pull_request': - return os.getenv('GITHUB_REF') - return None - - def _get_sanitizer(): return os.getenv('SANITIZER', constants.DEFAULT_SANITIZER).lower() @@ -74,22 +68,31 @@ class BaseCiEnvironment: @property def workspace(self): """Returns the workspace.""" - raise NotImplementedError('Child class must implment method.') + raise NotImplementedError('Child class must implement method.') @property def git_sha(self): - """Returns the Git SHA to diff against.""" - raise NotImplementedError('Child class must implment method.') + """Returns the Git SHA to checkout and fuzz. This is used only by GitHub + projects when commit fuzzing. It is not used when PR fuzzing. It is + definitely needed by OSS-Fuzz on GitHub since they have no copy of the repo + on the host and the repo on the builder image is a clone from main/master. + Right now it is needed by external on GitHub because we need to clone a new + repo because the copy they give us doesn't work for diffing. + + TODO(metzman): Try to eliminate the need for this by 1. Making the clone + from external github projects usable. 2. Forcing OSS-Fuzz on Github to clone + before starting CIFuzz.""" + raise NotImplementedError('Child class must implement method.') @property def actor(self): """Name of the actor for the CI.""" - raise NotImplementedError('Child class must implment method.') + raise NotImplementedError('Child class must implement method.') @property def token(self): """Returns the CI API token.""" - raise NotImplementedError('Child class must implment method.') + raise NotImplementedError('Child class must implement method.') @property def project_src_path(self): @@ -104,6 +107,43 @@ class BaseCiEnvironment: logging.debug('PROJECT_SRC_PATH set: %s.', path) return path + @property + def base_commit(self): + """Returns the base commit to diff against (commit fuzzing).""" + raise NotImplementedError('Child class must implement method.') + + @property + def pr_ref(self): + """Returns the pull request to checkout and fuzz. This is used only by + GitHub projects when PR fuzzing. It is not used when commit fuzzing. It is + definitely needed by OSS-Fuzz on GitHub since they have no copy of the repo + on the host and the repo on the builder image is a clone from main/master. + Right now it is needed by external on GitHub because we need to clone a new + repo because the copy they give us doesn't work for diffing. + + TODO(metzman): Try to eliminate the need for this by 1. Making the clone + from external github projects usable. 2. Forcing OSS-Fuzz on Github to clone + before starting CIFuzz.""" + raise NotImplementedError('Child class must implement method.') + + @property + def base_ref(self): + """Returns the base branch to diff against (pr fuzzing).""" + raise NotImplementedError('Child class must implement method.') + + @property + def git_url(self): + """Returns the repo URL. This is only used by GitHub users. Right now it is + needed by external on GitHub because we need to clone a new repo because the + copy they give us doesn't work for diffing. It isn't used by OSS-Fuzz on + github users since the Git URL is determined using repo detection. + + TODO(metzman): Try to eliminate the need for this by making the clone + from external github projects usable. + TODO(metzman): As an easier goal, maybe make OSS-Fuzz GitHub use this too + for: 1. Consistency 2. Maybe it will allow use on forks.""" + raise NotImplementedError('Child class must implement method.') + class GenericCiEnvironment(BaseCiEnvironment): """CI Environment for generic CI systems.""" @@ -115,8 +155,17 @@ class GenericCiEnvironment(BaseCiEnvironment): @property def git_sha(self): - """Returns the Git SHA to diff against.""" - return os.getenv('GIT_SHA') + """Returns the Git SHA to checkout and fuzz. This is used only by GitHub + projects when commit fuzzing. It is not used when PR fuzzing. It is + definitely needed by OSS-Fuzz on GitHub since they have no copy of the repo + on the host and the repo on the builder image is a clone from main/master. + Right now it is needed by external on GitHub because we need to clone a new + repo because the copy they give us doesn't work for diffing. + + TODO(metzman): Try to eliminate the need for this by 1. Making the clone + from external github projects usable. 2. Forcing OSS-Fuzz on Github to clone + before starting CIFuzz.""" + return None @property def token(self): @@ -136,14 +185,56 @@ class GenericCiEnvironment(BaseCiEnvironment): return None, repository @property - def repo_url(self): - """Returns the repo URL.""" - return os.getenv('REPOSITORY_URL') + def git_url(self): + """Returns the repo URL. This is only used by GitHub users. Right now it is + needed by external on GitHub because we need to clone a new repo because the + copy they give us doesn't work for diffing. It isn't used by OSS-Fuzz on + github users since the Git URL is determined using repo detection. + + TODO(metzman): Try to eliminate the need for this by making the clone + from external github projects usable. + TODO(metzman): As an easier goal, maybe make OSS-Fuzz GitHub use this too + for: 1. Consistency 2. Maybe it will allow use on forks.""" + return None + + @property + def base_commit(self): + """Returns the base commit to diff against (commit fuzzing).""" + return os.getenv('GIT_BASE_COMMIT') + + @property + def pr_ref(self): + """Returns the pull request to checkout and fuzz. This is used only by + GitHub projects when PR fuzzing. It is not used when commit fuzzing. It is + definitely needed by OSS-Fuzz on GitHub since they have no copy of the repo + on the host and the repo on the builder image is a clone from main/master. + Right now it is needed by external on GitHub because we need to clone a new + repo because the copy they give us doesn't work for diffing. + + TODO(metzman): Try to eliminate the need for this by 1. Making the clone + from external github projects usable. 2. Forcing OSS-Fuzz on Github to clone + before starting CIFuzz.""" + return None + + @property + def base_ref(self): + """Returns the base branch to diff against (pr fuzzing).""" + return os.getenv('GIT_BASE_REF') + + +def _get_event_data(): + github_event_path = _get_github_event_path() + with open(github_event_path, encoding='utf-8') as file_handle: + return json.load(file_handle) class GithubEnvironment(BaseCiEnvironment): """CI environment for GitHub.""" + def __init__(self): + self._event_data = _get_event_data() + self._event = os.getenv('GITHUB_EVENT_NAME') + @property def workspace(self): """Returns the workspace.""" @@ -151,7 +242,16 @@ class GithubEnvironment(BaseCiEnvironment): @property def git_sha(self): - """Returns the Git SHA to diff against.""" + """Returns the Git SHA to checkout and fuzz. This is used only by GitHub + projects when commit fuzzing. It is not used when PR fuzzing. It is + definitely needed by OSS-Fuzz on GitHub since they have no copy of the repo + on the host and the repo on the builder image is a clone from main/master. + Right now it is needed by external on GitHub because we need to clone a new + repo because the copy they give us doesn't work for diffing. + + TODO(metzman): Try to eliminate the need for this by 1. Making the clone + from external github projects usable. 2. Forcing OSS-Fuzz on Github to clone + before starting CIFuzz.""" return os.getenv('GITHUB_SHA') @property @@ -186,16 +286,66 @@ class GithubEnvironment(BaseCiEnvironment): return os.path.split(repository) @property - def repo_url(self): - """Returns the GitHub repo URL.""" + def git_url(self): + """Returns the repo URL. This is only used by GitHub users. Right now it is + needed by external on GitHub because we need to clone a new repo because the + copy they give us doesn't work for diffing. It isn't used by OSS-Fuzz on + github users since the Git URL is determined using repo detection. + + TODO(metzman): Try to eliminate the need for this by making the clone + from external github projects usable. + TODO(metzman): As an easier goal, maybe make OSS-Fuzz GitHub use this too + for: 1. Consistency 2. Maybe it will allow use on forks.""" repository = os.getenv('GITHUB_REPOSITORY') + # TODO(metzman): Probably need to change this to github.server_url. return f'https://github.com/{repository}.git' + @property + def base_commit(self): + """Returns the base commit to diff against (commit fuzzing).""" + base_commit = None + if self._event == 'push': + base_commit = self._event_data['before'] + logging.debug('base_commit: %s', base_commit) + return base_commit + + @property + def pr_ref(self): + """Returns the pull request to checkout and fuzz. This is used only by + GitHub projects when PR fuzzing. It is not used when commit fuzzing. It is + definitely needed by OSS-Fuzz on GitHub since they have no copy of the repo + on the host and the repo on the builder image is a clone from main/master. + Right now it is needed by external on GitHub because we need to clone a new + repo because the copy they give us doesn't work for diffing. + + TODO(metzman): Try to eliminate the need for this by 1. Making the clone + from external github projects usable. 2. Forcing OSS-Fuzz on Github to clone + before starting CIFuzz.""" + if self._event == 'pull_request': + pr_ref = f'refs/pull/{self._event_data["pull_request"]["number"]}/merge' + logging.debug('pr_ref: %s', pr_ref) + return pr_ref + return None + + @property + def base_ref(self): + """Returns the base branch to diff against (pr fuzzing).""" + return os.getenv('GITHUB_BASE_REF') + class ConfigError(Exception): """Error for invalid configuration.""" +def _is_github(): + """Returns True if running on Github Actions.""" + return bool(_get_github_event_path()) + + +def _get_github_event_path(): + return os.getenv('GITHUB_EVENT_PATH') + + class BaseConfig: """Object containing constant configuration for CIFuzz.""" @@ -206,34 +356,16 @@ class BaseConfig: INTERNAL_GENERIC_CI = 2 # OSS-Fuzz on any CI. EXTERNAL_GENERIC_CI = 3 # Non-OSS-Fuzz on any CI. - def _get_config_from_event_path(self): - event = os.getenv('GITHUB_EVENT_NAME') - if not self._github_event_path or not os.path.exists( - self._github_event_path): - return - with open(self._github_event_path, encoding='utf-8') as file_handle: - event_data = json.load(file_handle) - if event == 'push': - self.base_commit = event_data['before'] - logging.debug('base_commit: %s', self.base_commit) - elif event == 'pull_request': - self.pr_ref = f'refs/pull/{event_data["pull_request"]["number"]}/merge' - logging.debug('pr_ref: %s', self.pr_ref) - def __init__(self): # Need to set these before calling self.platform. - self._github_event_path = os.getenv('GITHUB_EVENT_PATH') - self.is_github = bool(self._github_event_path) - + self.is_github = _is_github() logging.debug('Is github: %s.', self.is_github) - self.oss_fuzz_project_name = os.getenv('OSS_FUZZ_PROJECT_NAME') - - self.base_commit = None - self.pr_ref = None - self.base_ref = os.getenv('GITHUB_BASE_REF') - self._get_config_from_event_path() + self.oss_fuzz_project_name = os.getenv('OSS_FUZZ_PROJECT_NAME') self._ci_env = _get_ci_environment(self.platform) + self.base_commit = self._ci_env.base_commit + self.base_ref = self._ci_env.base_ref + self.pr_ref = self._ci_env.pr_ref self.workspace = self._ci_env.workspace self.project_repo_owner, self.project_repo_name = ( @@ -368,22 +500,29 @@ class BuildFuzzersConfig(BaseConfig): """Get the configuration from CIFuzz from the environment. These variables are set by GitHub or the user.""" super().__init__() - self.commit_sha = self._ci_env.git_sha - self.git_url = self._ci_env.repo_url + self.git_sha = self._ci_env.git_sha + self.git_url = self._ci_env.git_url self.allowed_broken_targets_percentage = os.getenv( 'ALLOWED_BROKEN_TARGETS_PERCENTAGE') self.bad_build_check = environment.get_bool('BAD_BUILD_CHECK', True) - # pylint: disable=consider-using-ternary - self.keep_unaffected_fuzz_targets = ( - # Not from a commit or PR. - (not self.base_ref and not self.base_commit) or - environment.get_bool('KEEP_UNAFFECTED_FUZZERS')) + + self.keep_unaffected_fuzz_targets = environment.get_bool( + 'KEEP_UNAFFECTED_FUZZERS') + self.upload_build = environment.get_bool('UPLOAD_BUILD', False) - if self.upload_build: - logging.info('Keeping all fuzzers because we are uploading build.') - self.keep_unaffected_fuzz_targets = True + if not self.keep_unaffected_fuzz_targets: + has_base_for_diff = (self.base_ref or self.base_commit) + if not has_base_for_diff: + logging.info( + 'Keeping all fuzzers because there is nothing to diff against.') + self.keep_unaffected_fuzz_targets = True + elif self.upload_build: + logging.info('Keeping all fuzzers because we are uploading build.') + self.keep_unaffected_fuzz_targets = True + elif self.sanitizer == 'coverage': + logging.info('Keeping all fuzzers because we are doing coverage.') + self.keep_unaffected_fuzz_targets = True if self.sanitizer == 'coverage': - self.keep_unaffected_fuzz_targets = True self.bad_build_check = False diff --git a/infra/cifuzz/config_utils_test.py b/infra/cifuzz/config_utils_test.py index 9d6fc8f3..a54bb9d1 100644 --- a/infra/cifuzz/config_utils_test.py +++ b/infra/cifuzz/config_utils_test.py @@ -11,7 +11,7 @@ # 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. -"""Module for getting the configuration CIFuzz needs to run.""" +"""Tests for config_utils.""" import os import unittest from unittest import mock @@ -20,7 +20,7 @@ import config_utils import constants import test_helpers -# pylint: disable=no-self-use,protected-access +# pylint: disable=no-self-use,protected-access,arguments-differ class BaseConfigTest(unittest.TestCase): @@ -105,10 +105,20 @@ class BuildFuzzersConfigTest(unittest.TestCase): def _create_config(self): return config_utils.BuildFuzzersConfig() - def test_base_ref(self): + @mock.patch('config_utils._get_event_data', return_value={}) + def test_github_base_ref(self, _): """Tests that base_ref is set properly.""" expected_base_ref = 'expected_base_ref' os.environ['GITHUB_BASE_REF'] = expected_base_ref + os.environ['GITHUB_EVENT_PATH'] = '/event' + os.environ['GITHUB_REPOSITORY'] = 'owner/repo' + config = self._create_config() + self.assertEqual(config.base_ref, expected_base_ref) + + def test_base_ref(self): + """Tests that base_ref is set properly.""" + expected_base_ref = 'expected_base_ref' + os.environ['GIT_BASE_REF'] = expected_base_ref config = self._create_config() self.assertEqual(config.base_ref, expected_base_ref) @@ -120,7 +130,7 @@ class BuildFuzzersConfigTest(unittest.TestCase): def test_keep_unaffected_defaults_to_false_when_pr(self): """Tests that keep_unaffected_fuzz_targets defaults to false when from a pr.""" - os.environ['GITHUB_BASE_REF'] = 'base-ref' + os.environ['GIT_BASE_REF'] = 'base-ref' config = self._create_config() self.assertFalse(config.keep_unaffected_fuzz_targets) @@ -167,7 +177,8 @@ class RunFuzzersConfigTest(unittest.TestCase): class GetProjectRepoOwnerAndNameTest(unittest.TestCase): """Tests for BaseCiEnv.get_project_repo_owner_and_name.""" - def setUp(self): + @mock.patch('config_utils._get_event_data', return_value={}) + def setUp(self, _): test_helpers.patch_environ(self) self.repo_owner = 'repo-owner' self.repo_name = 'repo-name' @@ -201,30 +212,31 @@ class GetProjectRepoOwnerAndNameTest(unittest.TestCase): (None, self.repo_name)) -class GetRepoUrlTest(unittest.TestCase): - """Tests for GenericCiEnvironment.repo_url.""" +class GetGitUrlTest(unittest.TestCase): + """Tests for GenericCiEnvironment.git_url.""" - def setUp(self): + @mock.patch('config_utils._get_event_data', return_value={}) + def setUp(self, _): test_helpers.patch_environ(self) self.github_env = config_utils.GithubEnvironment() self.generic_ci_env = config_utils.GenericCiEnvironment() def test_unset_repository(self): """Tests that the correct result is returned when repository is not set.""" - self.assertEqual(self.generic_ci_env.repo_url, None) + self.assertEqual(self.generic_ci_env.git_url, None) def test_github_repository(self): """Tests that the correct result is returned when repository contains the owner and repo name (as it does on GitHub).""" os.environ['GITHUB_REPOSITORY'] = 'repo/owner' self.assertEqual('https://github.com/repo/owner.git', - self.github_env.repo_url) + self.github_env.git_url) def test_nongithub_repository(self): """Tests that the correct result is returned when repository contains the just the repo name (as it does outside of GitHub).""" - os.environ['REPOSITORY_URL'] = 'https://repo/url' - self.assertEqual('https://repo/url', self.generic_ci_env.repo_url) + os.environ['GITHUB_REPOSITORY'] = 'repo/owner' + self.assertEqual(None, self.generic_ci_env.git_url) class GetSanitizerTest(unittest.TestCase): @@ -259,13 +271,15 @@ class ProjectSrcPathTest(unittest.TestCase): self.project_src_dir_name = 'project-src' - def test_unset(self): + @mock.patch('config_utils._get_event_data', return_value={}) + def test_github_unset(self, _): """Tests that project_src_path returns None when no PROJECT_SRC_PATH is set.""" github_env = config_utils.GithubEnvironment() self.assertIsNone(github_env.project_src_path) - def test_github(self): + @mock.patch('config_utils._get_event_data', return_value={}) + def test_github(self, _): """Tests that project_src_path returns the correct result on GitHub.""" os.environ['PROJECT_SRC_PATH'] = self.project_src_dir_name expected_project_src_path = os.path.join(self.workspace, diff --git a/infra/cifuzz/continuous_integration.py b/infra/cifuzz/continuous_integration.py index 8836023f..230d2c96 100644 --- a/infra/cifuzz/continuous_integration.py +++ b/infra/cifuzz/continuous_integration.py @@ -69,12 +69,23 @@ class BaseCi: def get_diff_base(self): """Returns the base to diff against with git to get the change under test.""" - raise NotImplementedError('Child class must implement method.') + if self.config.base_ref: + logging.debug('Diffing against base_ref: %s.', self.config.base_ref) + return self.config.base_ref + if self.config.base_commit: + logging.debug('Diffing against base_commit: %s.', self.config.base_commit) + return self.config.base_commit + # TODO(metzman): Do we want this at all? What purpose does it serve? I guess + # it is a decent fallback when there is no base_commit or base_ref. + logging.debug('Diffing against origin.') + return 'origin' def get_changed_code_under_test(self, repo_manager_obj): """Returns the changed files that need to be tested.""" - base = self.get_diff_base() + if self.config.base_ref: + repo_manager_obj.fetch_branch(self.config.base_ref) fix_git_repo_for_diff(repo_manager_obj) + base = self.get_diff_base() logging.info('Diffing against %s.', base) # git diff ... is equivalent to # git diff $(git merge-base HEAD) @@ -127,18 +138,18 @@ def get_ci(config): return InternalGithub(config) -def checkout_specified_commit(repo_manager_obj, pr_ref, commit_sha): +def checkout_specified_commit(repo_manager_obj, pr_ref, git_sha): """Checks out the specified commit or pull request using |repo_manager_obj|.""" try: if pr_ref: repo_manager_obj.checkout_pr(pr_ref) else: - repo_manager_obj.checkout_commit(commit_sha) + repo_manager_obj.checkout_commit(git_sha) except (RuntimeError, ValueError): logging.error( 'Can not check out requested state %s. ' - 'Using current repo state', pr_ref or commit_sha) + 'Using current repo state', pr_ref or git_sha) class GithubCiMixin: @@ -169,21 +180,6 @@ class GithubCiMixin: return repo_path - def get_diff_base(self): - """Returns the base to diff against with git to get the change under - test.""" - if self.config.base_ref: - logging.debug('Diffing against base_ref: %s.', self.config.base_ref) - return self.config.base_ref - logging.debug('Diffing against base_commit: %s.', self.config.base_commit) - return self.config.base_commit - - def get_changed_code_under_test(self, repo_manager_obj): - """Returns the changed files that need to be tested.""" - if self.config.base_ref: - repo_manager_obj.fetch_branch(self.config.base_ref) - return super().get_changed_code_under_test(repo_manager_obj) - class InternalGithub(GithubCiMixin, BaseCi): """Class representing CI for an OSS-Fuzz project on Github Actions.""" @@ -192,7 +188,7 @@ class InternalGithub(GithubCiMixin, BaseCi): """Builds the fuzzer builder image, checks out the pull request/commit and returns the BuildPreparationResult.""" logging.info('Building OSS-Fuzz project on Github Actions.') - assert self.config.pr_ref or self.config.commit_sha + assert self.config.pr_ref or self.config.git_sha # detect_main_repo builds the image as a side effect. inferred_url, image_repo_path = (build_specified_commit.detect_main_repo( self.config.oss_fuzz_project_name, @@ -216,8 +212,7 @@ class InternalGithub(GithubCiMixin, BaseCi): repo_name=image_repo_name, username=self.config.actor, password=self.config.token) - checkout_specified_commit(manager, self.config.pr_ref, - self.config.commit_sha) + checkout_specified_commit(manager, self.config.pr_ref, self.config.git_sha) return BuildPreparationResult(success=True, image_repo_path=image_repo_path, @@ -266,9 +261,6 @@ class InternalGeneric(BaseCi): image_repo_path=image_repo_path, repo_manager=manager) - def get_diff_base(self): - return 'origin' - def get_build_command(self, host_repo_path, image_repo_path): # pylint: disable=no-self-use """Returns the command for building the project that is run inside the project builder container. Command also replaces |image_repo_path| with @@ -300,9 +292,6 @@ class ExternalGeneric(BaseCi): returned otherwise.""" return self._repo_dir - def get_diff_base(self): - return 'origin' - def prepare_for_fuzzer_build(self): logging.info('ExternalGeneric: preparing for fuzzer build.') manager = repo_manager.RepoManager(self.config.project_src_path) @@ -311,7 +300,7 @@ class ExternalGeneric(BaseCi): if not build_external_project_docker_image(manager.repo_dir, build_integration_abs_path): logging.error('Failed to build external project: %s.', - self.config.oss_fuzz_project_name) + self.config.project_repo_name) return BuildPreparationResult(success=False, image_repo_path=None, repo_manager=None) @@ -346,8 +335,7 @@ class ExternalGithub(GithubCiMixin, BaseCi): repo_name=self.config.project_repo_name, username=self.config.actor, password=self.config.token) - checkout_specified_commit(manager, self.config.pr_ref, - self.config.commit_sha) + checkout_specified_commit(manager, self.config.pr_ref, self.config.git_sha) build_integration_abs_path = os.path.join( manager.repo_dir, self.config.build_integration_path) diff --git a/infra/cifuzz/filestore/github_actions/github_actions_test.py b/infra/cifuzz/filestore/github_actions/github_actions_test.py index 6c57dd1e..8aafa4c8 100644 --- a/infra/cifuzz/filestore/github_actions/github_actions_test.py +++ b/infra/cifuzz/filestore/github_actions/github_actions_test.py @@ -36,7 +36,8 @@ import test_helpers class GithubActionsFilestoreTest(fake_filesystem_unittest.TestCase): """Tests for GithubActionsFilestore.""" - def setUp(self): + @mock.patch('config_utils._get_event_data', return_value={}) + def setUp(self, _): # pylint: disable=arguments-differ test_helpers.patch_environ(self) self.token = 'example githubtoken' self.owner = 'exampleowner' diff --git a/infra/cifuzz/run_fuzzers_entrypoint.py b/infra/cifuzz/run_fuzzers_entrypoint.py index ee748363..4777786c 100644 --- a/infra/cifuzz/run_fuzzers_entrypoint.py +++ b/infra/cifuzz/run_fuzzers_entrypoint.py @@ -75,7 +75,7 @@ def main(): """Runs project's fuzzers for CI tools. This is the entrypoint for the run_fuzzers github action. - NOTE: libFuzzer binaries must be located in the ${GITHUB_WORKSPACE}/out + NOTE: libFuzzer binaries must be located in the $WORKSPACE/build-out directory in order for this action to be used. This action will only fuzz the binaries that are located in that directory. It is recommended that you add the build_fuzzers action preceding this one. @@ -85,13 +85,6 @@ def main(): This can be used in parallel with the upload-artifact action to surface the logs. - Required environment variables: - FUZZ_SECONDS: The length of time in seconds that fuzzers are to be run. - GITHUB_WORKSPACE: The shared volume directory where input artifacts are. - DRY_RUN: If true, no failures will surface. - OSS_FUZZ_PROJECT_NAME: The name of the relevant OSS-Fuzz project. - SANITIZER: The sanitizer to use when running fuzzers. - Returns: 0 on success or nonzero on failure. """ diff --git a/infra/cifuzz/run_fuzzers_test.py b/infra/cifuzz/run_fuzzers_test.py index 7ccc3ffb..6f352303 100644 --- a/infra/cifuzz/run_fuzzers_test.py +++ b/infra/cifuzz/run_fuzzers_test.py @@ -335,7 +335,7 @@ class CoverageReportIntegrationTest(unittest.TestCase): oss_fuzz_project_name=EXAMPLE_PROJECT, project_repo_name='oss-fuzz', workspace=temp_dir, - commit_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523', + git_sha='0b95fe1039ed7c38fea1f97078316bfc1030c523', base_commit='da0746452433dc18bae699e355a9821285d863c8', sanitizer=self.SANITIZER, is_github=True, -- cgit v1.2.3