From 054eccc2fc07c2368fbd9dc20dde16c4eaa0639b Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Wed, 20 Dec 2017 11:36:19 +0100 Subject: [PATCH] Set remote url on every getRepo in merger In case we run on github with apps we have an access token in the git url. This access token is only valid for a specific period of time. Currently the merger caches its repos and never changes the git url on subsequent merge or cat requests. After timeout the merger then faces an exception [1]. The fix is to update the remote url on every getRepo call in the merger. This makes sure that we have a recent access token on every merge or cat call. Further the executor has the same problems and gets fixed by this too as it also uses getRepo from the merger. [1] Exception: 2017-12-20 07:00:00,874 ERROR zuul.Merger: Unable to update github/sandbox/sandbox Traceback (most recent call last): File "/usr/lib/python3.6/site-packages/zuul/merger/merger.py", line 382, in updateRepo repo.reset() File "/usr/lib/python3.6/site-packages/zuul/merger/merger.py", line 143, in reset self.update() File "/usr/lib/python3.6/site-packages/zuul/merger/merger.py", line 293, in update self._git_fetch(repo, 'origin', tags=True) File "/usr/lib/python3.6/site-packages/zuul/merger/merger.py", line 133, in _git_fetch **kwargs) File "/usr/lib/python3.6/site-packages/git/cmd.py", line 551, in return lambda *args, **kwargs: self._call_process(name, *args, **kwargs) File "/usr/lib/python3.6/site-packages/git/cmd.py", line 1010, in _call_process return self.execute(call, **exec_kwargs) File "/usr/lib/python3.6/site-packages/git/cmd.py", line 821, in execute raise GitCommandError(command, status, stderr_value, stdout_value) git.exc.GitCommandError: Cmd('git') failed due to: exit code(128) cmdline: git fetch --tags origin stderr: 'remote: Invalid username or password. fatal: Authentication failed for 'https://x-access-token:v1.c8ec09e233b16871b64843adeec5fb048a383fba@github.example.com/sandbox/sandbox/'' Change-Id: If990dc48e6c10c24b6b32db5f5711fc3608bdfe4 --- tests/base.py | 22 ++++++++---- tests/unit/test_merger_repo.py | 62 +++++++++++++++++++++++++++++++++- zuul/merger/merger.py | 20 +++++++++-- 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/tests/base.py b/tests/base.py index f68f59a581..c13519ee96 100755 --- a/tests/base.py +++ b/tests/base.py @@ -941,7 +941,7 @@ class FakeGithubConnection(githubconnection.GithubConnection): log = logging.getLogger("zuul.test.FakeGithubConnection") def __init__(self, driver, connection_name, connection_config, rpcclient, - changes_db=None, upstream_root=None): + changes_db=None, upstream_root=None, git_url_with_auth=False): super(FakeGithubConnection, self).__init__(driver, connection_name, connection_config) self.connection_name = connection_name @@ -953,6 +953,7 @@ class FakeGithubConnection(githubconnection.GithubConnection): self.merge_not_allowed_count = 0 self.reports = [] self.github_client = tests.fakegithub.FakeGithub(changes_db) + self.git_url_with_auth = git_url_with_auth self.rpcclient = rpcclient def getGithubClient(self, @@ -1045,7 +1046,13 @@ class FakeGithubConnection(githubconnection.GithubConnection): return 'read' def getGitUrl(self, project): - return os.path.join(self.upstream_root, str(project)) + if self.git_url_with_auth: + auth_token = ''.join( + random.choice(string.ascii_lowercase) for x in range(8)) + prefix = 'file://x-access-token:%s@' % auth_token + else: + prefix = '' + return prefix + os.path.join(self.upstream_root, str(project)) def real_getGitUrl(self, project): return super(FakeGithubConnection, self).getGitUrl(project) @@ -1901,6 +1908,7 @@ class ZuulTestCase(BaseTestCase): run_ansible = False create_project_keys = False use_ssl = False + git_url_with_auth = False def _startMerger(self): self.merge_server = zuul.merger.server.MergeServer(self.config, @@ -2070,10 +2078,12 @@ class ZuulTestCase(BaseTestCase): def getGithubConnection(driver, name, config): server = config.get('server', 'github.com') db = self.github_changes_dbs.setdefault(server, {}) - con = FakeGithubConnection(driver, name, config, - self.rpcclient, - changes_db=db, - upstream_root=self.upstream_root) + con = FakeGithubConnection( + driver, name, config, + self.rpcclient, + changes_db=db, + upstream_root=self.upstream_root, + git_url_with_auth=self.git_url_with_auth) self.event_queues.append(con.event_queue) setattr(self, 'fake_' + name, con) return con diff --git a/tests/unit/test_merger_repo.py b/tests/unit/test_merger_repo.py index fb2f1999ea..984644fc56 100644 --- a/tests/unit/test_merger_repo.py +++ b/tests/unit/test_merger_repo.py @@ -22,7 +22,7 @@ import git import testtools from zuul.merger.merger import Repo -from tests.base import ZuulTestCase, FIXTURE_DIR +from tests.base import ZuulTestCase, FIXTURE_DIR, simple_layout class TestMergerRepo(ZuulTestCase): @@ -116,3 +116,63 @@ class TestMergerRepo(ZuulTestCase): # This is created on the second fetch self.assertTrue(os.path.exists(os.path.join( self.workspace_root, 'stamp2'))) + + +class TestMergerWithAuthUrl(ZuulTestCase): + config_file = 'zuul-github-driver.conf' + + git_url_with_auth = True + + @simple_layout('layouts/merging-github.yaml', driver='github') + def test_changing_url(self): + """ + This test checks that if getGitUrl returns different urls for the same + repo (which happens if an access token is part of the url) then the + remote urls are changed in the merger accordingly. This tests directly + the merger. + """ + + merger = self.executor_server.merger + repo = merger.getRepo('github', 'org/project') + first_url = repo.remote_url + + repo = merger.getRepo('github', 'org/project') + second_url = repo.remote_url + + # the urls should differ + self.assertNotEqual(first_url, second_url) + + @simple_layout('layouts/merging-github.yaml', driver='github') + def test_changing_url_end_to_end(self): + """ + This test checks that if getGitUrl returns different urls for the same + repo (which happens if an access token is part of the url) then the + remote urls are changed in the merger accordingly. This is an end to + end test. + """ + + A = self.fake_github.openFakePullRequest('org/project', 'master', + 'PR title') + self.fake_github.emitEvent(A.getCommentAddedEvent('merge me')) + self.waitUntilSettled() + self.assertTrue(A.is_merged) + + # get remote url of org/project in merger + repo = self.executor_server.merger.repos.get('github.com/org/project') + self.assertIsNotNone(repo) + git_repo = git.Repo(repo.local_path) + first_url = list(git_repo.remotes[0].urls)[0] + + B = self.fake_github.openFakePullRequest('org/project', 'master', + 'PR title') + self.fake_github.emitEvent(B.getCommentAddedEvent('merge me again')) + self.waitUntilSettled() + self.assertTrue(B.is_merged) + + repo = self.executor_server.merger.repos.get('github.com/org/project') + self.assertIsNotNone(repo) + git_repo = git.Repo(repo.local_path) + second_url = list(git_repo.remotes[0].urls)[0] + + # the urls should differ + self.assertNotEqual(first_url, second_url) diff --git a/zuul/merger/merger.py b/zuul/merger/merger.py index 07f3e6990f..63bb5d5054 100644 --- a/zuul/merger/merger.py +++ b/zuul/merger/merger.py @@ -79,6 +79,8 @@ class Repo(object): self.retry_interval = retry_interval try: self._ensure_cloned() + self._git_set_remote_url( + git.Repo(self.local_path), self.remote_url) except Exception: self.log.exception("Unable to initialize repo for %s" % remote) @@ -112,8 +114,7 @@ class Repo(object): config_writer.set_value('user', 'name', self.username) config_writer.write() if rewrite_url: - with repo.remotes.origin.config_writer as config_writer: - config_writer.set('url', self.remote_url) + self._git_set_remote_url(repo, self.remote_url) self._initialized = True def isInitialized(self): @@ -154,6 +155,10 @@ class Repo(object): else: raise + def _git_set_remote_url(self, repo, url): + with repo.remotes.origin.config_writer as config_writer: + config_writer.set('url', url) + def createRepoObject(self): self._ensure_cloned() repo = git.Repo(self.local_path) @@ -358,6 +363,13 @@ class Repo(object): repo = self.createRepoObject() repo.delete_remote(repo.remotes[remote]) + def setRemoteUrl(self, url): + if self.remote_url == url: + return + self.log.debug("Set remote url to %s" % url) + self.remote_url = url + self._git_set_remote_url(self.createRepoObject(), self.remote_url) + class Merger(object): def __init__(self, working_root, connections, email, username, @@ -405,7 +417,9 @@ class Merger(object): url = source.getGitUrl(project) key = '/'.join([hostname, project_name]) if key in self.repos: - return self.repos[key] + repo = self.repos[key] + repo.setRemoteUrl(url) + return repo sshkey = self.connections.connections.get(connection_name).\ connection_config.get('sshkey') if not url: