From 56b9a190957cc0d9e90b36aa6c0d21a391eb689f Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 25 Sep 2019 16:52:30 +0200 Subject: [PATCH 1/2] add support for running builds in a fork of the repository While for most use cases running the CI builds on the same repository is fine, if a lot of people have write access to the repository there might be security concerns. To placate those this adds a new feature to Homu allowing the actual CI builds to be executed in a fork with tighter permissions. The implementation relies on how GitHub stores forks in their backends: all the forks of a repository plus the main one are actually stored in the same underlying git bare repo. This means that if we push the merge commit to the fork we can instantly fast-forward the main repo's master branch to that commit, without the need to reupload things. This simplifies the Homu logic a lot, and because of that the changes needed to implement the feature were minimal. A downside of that is the feature requires the fork to be an actual GitHub fork. Unfortunately GitHub doesn't allow the main repo to be in the same account/org as a fork of it, so we'll have to store the rustc fork in another org. Thanks to the team repo managing access to it shouldn't be a problem. Another downside is the commit history on the main repository won't show the green checkmarks, as the builds happened on the fork. This downside is mitigated by the fact that Homu will keep the history on the fork in sync with the main repo, allowing users to see the checkmarks on the fork instead. Thanks to this the UX loss will be minimal. This PR makes three different changes to homu to support the feature: - Homu will consider the fork an alias of the main repository when it receives a webhook, allowing it to process CI results from the fork. - When the feature is enabled, Homu will push the merge commit in the fork's auto and try branches instead of the main repository. - Once the build is green, in addition to fast-forwarding the main repository's master branch to the merge commit Homu will also force push the merge commit to the fork's master branch. This will allow users to follow the commit history on the fork as well. --- cfg.sample.toml | 15 +++++++++++++++ homu/main.py | 51 +++++++++++++++++++++++++++++++++++++++++-------- homu/server.py | 16 +++++++++++----- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/cfg.sample.toml b/cfg.sample.toml index 339636f..d00f994 100644 --- a/cfg.sample.toml +++ b/cfg.sample.toml @@ -101,6 +101,21 @@ try_users = [] #auto = "auto" #try = "try" +# test-on-fork allows you to run the CI builds for a project in a separate fork +# instead of the main repository, while still approving PRs and merging the +# commits in the main one. +# +# To enable test-on-fork you need to uncomment the section below and fill the +# fork's owner and repository name. The fork MUST BE AN ACTUAL GITHUB FORK for +# this feature to work. That means it will likely need to be in a separate +# GitHub organization. +# +# This only works when `local_git = true`. +# +#[repo.NAME.test-on-fork] +#owner = "" +#name = "" + [repo.NAME.github] # Arbitrary secret. You can generate one with: openssl rand -hex 20 secret = "" diff --git a/homu/main.py b/homu/main.py index 6d136fa..b5a47fb 100644 --- a/homu/main.py +++ b/homu/main.py @@ -79,6 +79,7 @@ class Repository: treeclosed = -1 treeclosed_src = None gh = None + gh_test_on_fork = None label = None db = None @@ -133,7 +134,7 @@ class PullReqState: delegate = '' def __init__(self, num, head_sha, status, db, repo_label, mergeable_que, - gh, owner, name, label_events, repos): + gh, owner, name, label_events, repos, test_on_fork): self.head_advanced('', use_db=False) self.num = num @@ -149,6 +150,7 @@ def __init__(self, num, head_sha, status, db, repo_label, mergeable_que, self.timeout_timer = None self.test_started = time.time() self.label_events = label_events + self.test_on_fork = test_on_fork def head_advanced(self, head_sha, *, use_db=True): self.head_sha = head_sha @@ -313,6 +315,22 @@ def get_repo(self): assert repo.name == self.name return repo + def get_test_on_fork_repo(self): + if not self.test_on_fork: + return None + + repo = self.repos[self.repo_label].gh_test_on_fork + if not repo: + repo = self.gh.repository( + self.test_on_fork['owner'], + self.test_on_fork['name'], + ) + self.repos[self.repo_label].gh_test_on_fork = repo + + assert repo.owner.login == self.test_on_fork['owner'] + assert repo.name == self.test_on_fork['name'] + return repo + def save(self): db_query( self.db, @@ -792,9 +810,9 @@ def handle_hook_response(state, hook_cfg, body, extra_data): def git_push(git_cmd, branch, state): merge_sha = subprocess.check_output(git_cmd('rev-parse', 'HEAD')).decode('ascii').strip() # noqa - if utils.silent_call(git_cmd('push', '-f', 'origin', branch)): + if utils.silent_call(git_cmd('push', '-f', 'test-origin', branch)): utils.logged_call(git_cmd('branch', '-f', 'homu-tmp', branch)) - utils.logged_call(git_cmd('push', '-f', 'origin', 'homu-tmp')) + utils.logged_call(git_cmd('push', '-f', 'test-origin', 'homu-tmp')) def inner(): utils.github_create_status( @@ -814,14 +832,14 @@ def fail(err): utils.retry_until(inner, fail, state) - utils.logged_call(git_cmd('push', '-f', 'origin', branch)) + utils.logged_call(git_cmd('push', '-f', 'test-origin', branch)) return merge_sha def init_local_git_cmds(repo_cfg, git_cfg): fpath = 'cache/{}/{}'.format(repo_cfg['owner'], repo_cfg['name']) - url = 'git@github.com:{}/{}.git'.format(repo_cfg['owner'], repo_cfg['name']) # noqa + genurl = lambda cfg: 'git@github.com:{}/{}.git'.format(cfg['owner'], cfg['name']) # noqa if not os.path.exists(SSH_KEY_FILE): os.makedirs(os.path.dirname(SSH_KEY_FILE), exist_ok=True) @@ -831,7 +849,17 @@ def init_local_git_cmds(repo_cfg, git_cfg): if not os.path.exists(fpath): utils.logged_call(['git', 'init', fpath]) - utils.logged_call(['git', '-C', fpath, 'remote', 'add', 'origin', url]) # noqa + + remotes = {'origin': genurl(repo_cfg), 'test-origin': genurl(repo_cfg)} + if 'test-on-fork' in repo_cfg: + remotes['test-origin'] = genurl(repo_cfg['test-on-fork']) + + for remote, url in remotes.items(): + try: + utils.logged_call(['git', '-C', fpath, 'remote', 'set-url', remote, url]) # noqa + utils.logged_call(['git', '-C', fpath, 'remote', 'set-url', '--push', remote, url]) # noqa + except subprocess.CalledProcessError: + utils.logged_call(['git', '-C', fpath, 'remote', 'add', remote, url]) # noqa return lambda *args: ['git', '-C', fpath] + list(args) @@ -1511,7 +1539,7 @@ def synchronize(repo_label, repo_cfg, logger, gh, states, repos, db, mergeable_q status = info.state break - state = PullReqState(pull.number, pull.head.sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos) # noqa + state = PullReqState(pull.number, pull.head.sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos, repo_cfg.get('test-on-fork')) # noqa state.title = pull.title state.body = pull.body state.head_ref = pull.head.repo[0] + ':' + pull.head.ref @@ -1702,6 +1730,13 @@ def main(): repo_cfgs[repo_label] = repo_cfg repo_labels[repo_cfg['owner'], repo_cfg['name']] = repo_label + # If test-on-fork is enabled point both the main repo and the fork to + # the same homu "repository". This will allow events coming from both + # GitHub repositories to be processed the same way. + if 'test-on-fork' in repo_cfg: + tof = repo_cfg['test-on-fork'] + repo_labels[tof['owner'], tof['name']] = repo_label + repo_states = {} repos[repo_label] = Repository(None, repo_label, db) @@ -1710,7 +1745,7 @@ def main(): 'SELECT num, head_sha, status, title, body, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate, merge_sha FROM pull WHERE repo = ?', # noqa [repo_label]) for num, head_sha, status, title, body, head_ref, base_ref, assignee, approved_by, priority, try_, rollup, delegate, merge_sha in db.fetchall(): # noqa - state = PullReqState(num, head_sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos) # noqa + state = PullReqState(num, head_sha, status, db, repo_label, mergeable_que, gh, repo_cfg['owner'], repo_cfg['name'], repo_cfg.get('labels', {}), repos, repo_cfg.get('test-on-fork')) # noqa state.title = title state.body = body state.head_ref = head_ref diff --git a/homu/server.py b/homu/server.py index eeca4e4..67bb00d 100644 --- a/homu/server.py +++ b/homu/server.py @@ -420,7 +420,8 @@ def github(): info['repository']['owner']['login'], info['repository']['name'], repo_cfg.get('labels', {}), - g.repos) + g.repos, + repo_cfg.get('test-on-fork')) state.title = info['pull_request']['title'] state.body = info['pull_request']['body'] state.head_ref = info['pull_request']['head']['repo']['owner']['login'] + ':' + info['pull_request']['head']['ref'] # noqa @@ -656,10 +657,16 @@ def report_build_res(succ, url, builder, state, logger, repo_cfg): merge_sha=state.merge_sha, )) state.change_labels(LabelEvent.SUCCEED) + + def set_ref(): + utils.github_set_ref(state.get_repo(), 'heads/' + + state.base_ref, state.merge_sha) + utils.github_set_ref(state.get_test_on_fork_repo(), + 'heads/' + state.base_ref, + state.merge_sha, force=True) try: try: - utils.github_set_ref(state.get_repo(), 'heads/' + - state.base_ref, state.merge_sha) + set_ref() except github3.models.GitHubError: utils.github_create_status( state.get_repo(), @@ -667,8 +674,7 @@ def report_build_res(succ, url, builder, state, logger, repo_cfg): 'success', '', 'Branch protection bypassed', context='homu') - utils.github_set_ref(state.get_repo(), 'heads/' + - state.base_ref, state.merge_sha) + set_ref() state.fake_merge(repo_cfg) From 4cd6aafad7a342134690c87c4c9e084875cac084 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 16 Oct 2019 08:33:36 +0200 Subject: [PATCH 2/2] address review comments --- homu/main.py | 7 ++++--- homu/server.py | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/homu/main.py b/homu/main.py index b5a47fb..0dce72e 100644 --- a/homu/main.py +++ b/homu/main.py @@ -850,9 +850,10 @@ def init_local_git_cmds(repo_cfg, git_cfg): if not os.path.exists(fpath): utils.logged_call(['git', 'init', fpath]) - remotes = {'origin': genurl(repo_cfg), 'test-origin': genurl(repo_cfg)} - if 'test-on-fork' in repo_cfg: - remotes['test-origin'] = genurl(repo_cfg['test-on-fork']) + remotes = { + 'origin': genurl(repo_cfg), + 'test-origin': genurl(repo_cfg.get('test-on-fork', repo_cfg)), + } for remote, url in remotes.items(): try: diff --git a/homu/server.py b/homu/server.py index 67bb00d..b9b382d 100644 --- a/homu/server.py +++ b/homu/server.py @@ -661,9 +661,10 @@ def report_build_res(succ, url, builder, state, logger, repo_cfg): def set_ref(): utils.github_set_ref(state.get_repo(), 'heads/' + state.base_ref, state.merge_sha) - utils.github_set_ref(state.get_test_on_fork_repo(), - 'heads/' + state.base_ref, - state.merge_sha, force=True) + if state.test_on_fork is not None: + utils.github_set_ref(state.get_test_on_fork_repo(), + 'heads/' + state.base_ref, + state.merge_sha, force=True) try: try: set_ref()