From f79ec3bf540d6b953c7fb4800e779e2a2b72455a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20=27sil2100=27=20Zemczak?= Date: Wed, 16 Jun 2021 09:57:15 +0200 Subject: [PATCH] Revert "Switch from swiftclient for private PPAs to using HTTP with X-Auth-Token instead." This reverts commit 7ff150ced70d0db565378683909e22dfac5383e1. Sadly we can't use the X-Auth-Token approach due to implementational details (the token is valid only for an hour). So we need to switch back to using swiftclient. --- britney.conf | 8 +- britney.conf.template | 8 +- britney2/policies/autopkgtest.py | 140 +++++++++++++++++++++---------- tests/__init__.py | 8 +- tests/mock_swift.py | 34 ++++---- tests/mock_swiftclient.py | 70 ++++++++++++++++ tests/test_autopkgtest.py | 18 ++-- tests/test_policy.py | 4 +- 8 files changed, 214 insertions(+), 76 deletions(-) create mode 100644 tests/mock_swiftclient.py diff --git a/britney.conf b/britney.conf index 5701701..ee4fc89 100644 --- a/britney.conf +++ b/britney.conf @@ -102,10 +102,12 @@ ADT_SHARED_RESULTS_CACHE = # Swift base URL with the results (must be publicly readable and browsable) # or file location if results are pre-fetched ADT_SWIFT_URL = https://autopkgtest.ubuntu.com/results/ -# Swift authentication user and token for access to private containers +# Swift identity with read access to the private result container # (this is required whenever a private PPA is used for testing) -ADT_SWIFT_USER = -ADT_SWIFT_TOKEN = +ADT_SWIFT_USER = +ADT_SWIFT_PASS = +ADT_SWIFT_AUTH_URL = +ADT_SWIFT_TENANT = # List of launchpad users/teams that should have read access to any private # result logs ADT_PRIVATE_SHARED = diff --git a/britney.conf.template b/britney.conf.template index 4a10ca2..7c21e5f 100644 --- a/britney.conf.template +++ b/britney.conf.template @@ -124,10 +124,12 @@ ADT_SHARED_RESULTS_CACHE = # or file location if results are pre-fetched #ADT_SWIFT_URL = https://example.com/some/url ADT_SWIFT_URL = file:///path/to/britney/state/debci.json -# Swift authentication user and token for access to private containers +# Swift identity with read access to the private result container # (this is required whenever a private PPA is used for testing) -ADT_SWIFT_USER = -ADT_SWIFT_TOKEN = +ADT_SWIFT_USER = +ADT_SWIFT_PASS = +ADT_SWIFT_AUTH_URL = +ADT_SWIFT_TENANT = # List of launchpad users/teams that should have read access to any private # result logs ADT_PRIVATE_SHARED = diff --git a/britney2/policies/autopkgtest.py b/britney2/policies/autopkgtest.py index e34684f..bbfc2f3 100644 --- a/britney2/policies/autopkgtest.py +++ b/britney2/policies/autopkgtest.py @@ -32,7 +32,7 @@ import sys import time import urllib.parse from urllib.error import HTTPError -from urllib.request import Request, urlopen +from urllib.request import urlopen import apt_pkg @@ -208,9 +208,11 @@ class AutopkgtestPolicy(BasePolicy): # log into swift in case we need to fetch some private results # this is optional - if there are no credentials present, results will # be fetched without authentication - if self.options.adt_swift_token: - if self.options.adt_swift_url.startswith('file://'): - raise RuntimeError('Token authentication requires swift remote storage') + if self.options.adt_swift_user: + if (not self.options.adt_swift_pass or + not self.options.adt_swift_auth_url or + not self.options.adt_swift_tenant): + raise RuntimeError('Incomplete swift credentials given') # once swift credentials are given, the results will be published # to a private container @@ -223,10 +225,28 @@ class AutopkgtestPolicy(BasePolicy): # TODO: write a test for this if '@' in ppa and not re.match(r'^.+:.+@.+:.+$', ppa): raise RuntimeError('Private PPA %s not following required format (user:token@team/name:fingerprint)', ppa) + + import swiftclient + + if '/v2.0' in self.options.adt_swift_auth_url: + auth_version = '2.0' + else: + auth_version = '1' + + self.logger.info('Creating an authenticated swift connection for user %s', self.options.adt_swift_user) + self.swift_conn = swiftclient.Connection( + authurl=self.options.adt_swift_auth_url, + user=self.options.adt_swift_user, + key=self.options.adt_swift_pass, + tenant_name=self.options.adt_swift_tenant, + auth_version=auth_version + ) else: if any('@' in ppa for ppa in self.options.adt_ppas): raise RuntimeError('Private PPA configured but no swift credentials given') + self.swift_conn = None + # read in the new results if self.options.adt_swift_url.startswith('file://'): debci_file = self.options.adt_swift_url[7:] @@ -835,12 +855,10 @@ class AutopkgtestPolicy(BasePolicy): latest_run_for_package._cache = collections.defaultdict(dict) - def download_retry(self, url, token=None): - headers = {'X-Auth-Token': token} if token else {} - request = Request(url, headers=headers) + def download_retry(self, url): for retry in range(5): try: - req = urlopen(request, timeout=30) + req = urlopen(url, timeout=30) code = req.getcode() if 200 <= code < 300: return req @@ -885,32 +903,54 @@ class AutopkgtestPolicy(BasePolicy): query['marker'] = query['prefix'] + latest_run_id # request new results from swift - url = os.path.join(swift_url, self.swift_container) - url += '?' + urllib.parse.urlencode(query) - f = None - try: - f = self.download_retry(url, self.options.adt_swift_token) - if f.getcode() == 200: - result_paths = f.read().decode().strip().splitlines() - elif f.getcode() == 204: # No content - result_paths = [] - else: - # we should not ever end up here as we expect a HTTPError in - # other cases; e. g. 3XX is something that tells us to adjust - # our URLS, so fail hard on those - raise NotImplementedError('fetch_swift_results(%s): cannot handle HTTP code %i' % - (url, f.getcode())) - except IOError as e: - # 401 "Unauthorized" is swift's way of saying "container does not exist" - if hasattr(e, 'code') and e.code == 401: - self.logger.info('fetch_swift_results: %s does not exist yet or is inaccessible', url) - return - # same as above in the swift authenticated case - self.logger.error('Failure to fetch swift results from %s: %s', url, str(e)) - sys.exit(1) - finally: - if f is not None: - f.close() + if self.swift_conn: + # when we have an authenticated swift connection, use that to + # fetch the result_path list as we might be fetching from an + # otherwise unaccessible container + from swiftclient.exceptions import ClientException + + try: + _, result_paths = self.swift_conn.get_container( + self.swift_container, + query_string=urllib.parse.urlencode(query)) + except ClientException as e: + # 401 "Unauthorized" is swift's way of saying "container does not exist" + if e.http_status == '401' or e.http_status == '404': + self.logger.info('fetch_swift_results: %s does not exist yet or is inaccessible', self.swift_container) + return + # Other status codes are usually a transient + # network/infrastructure failure. Ignoring this can lead to + # re-requesting tests which we already have results for, so + # fail hard on this and let the next run retry. + self.logger.error('Failure to fetch swift results from %s: %s', self.swift_container, str(e)) + sys.exit(1) + else: + url = os.path.join(swift_url, self.swift_container) + url += '?' + urllib.parse.urlencode(query) + f = None + try: + f = self.download_retry(url) + if f.getcode() == 200: + result_paths = f.read().decode().strip().splitlines() + elif f.getcode() == 204: # No content + result_paths = [] + else: + # we should not ever end up here as we expect a HTTPError in + # other cases; e. g. 3XX is something that tells us to adjust + # our URLS, so fail hard on those + raise NotImplementedError('fetch_swift_results(%s): cannot handle HTTP code %i' % + (url, f.getcode())) + except IOError as e: + # 401 "Unauthorized" is swift's way of saying "container does not exist" + if hasattr(e, 'code') and e.code == 401: + self.logger.info('fetch_swift_results: %s does not exist yet or is inaccessible', url) + return + # same as above in the swift authenticated case + self.logger.error('Failure to fetch swift results from %s: %s', url, str(e)) + sys.exit(1) + finally: + if f is not None: + f.close() for p in result_paths: self.fetch_one_result( @@ -926,13 +966,29 @@ class AutopkgtestPolicy(BasePolicy): f = None try: - url = os.path.join(swift_url, container, path, name) - f = self.download_retry(url, self.options.adt_swift_token) - if f.getcode() == 200: - tar_bytes = io.BytesIO(f.read()) + if self.swift_conn: + from swiftclient.exceptions import ClientException + + # We don't need any additional retry logic as swiftclient + # already performs retries (5 by default). + url = os.path.join(path, name) + try: + _, contents = self.swift_conn.get_object(container, url) + except ClientException as e: + self.logger.error('Failure to fetch %s from container %s: %s', + url, container, str(e)) + if e.http_status == '404': + return + sys.exit(1) + tar_bytes = io.BytesIO(contents) else: - raise NotImplementedError('fetch_one_result(%s): cannot handle HTTP code %i' % - (url, f.getcode())) + url = os.path.join(swift_url, container, path, name) + f = self.download_retry(url) + if f.getcode() == 200: + tar_bytes = io.BytesIO(f.read()) + else: + raise NotImplementedError('fetch_one_result(%s): cannot handle HTTP code %i' % + (url, f.getcode())) except IOError as e: self.logger.error('Failure to fetch %s: %s', url, str(e)) # we tolerate "not found" (something went wrong on uploading the @@ -1060,7 +1116,7 @@ class AutopkgtestPolicy(BasePolicy): qname = 'debci-%s-%s' % (self.options.series, arch) params['submit-time'] = datetime.strftime(datetime.utcnow(), '%Y-%m-%d %H:%M:%S%z') - if self.options.adt_swift_user: + if self.swift_conn: params['swiftuser'] = self.options.adt_swift_user if self.options.adt_private_shared: params['readable-by'] = self.options.adt_private_shared @@ -1273,7 +1329,7 @@ class AutopkgtestPolicy(BasePolicy): 'log.gz') else: # Private runs have a different base url - results_url = self.options.adt_private_url if self.options.adt_swift_user else self.options.adt_swift_url + results_url = self.options.adt_private_url if self.swift_conn else self.options.adt_swift_url url = os.path.join(results_url, self.swift_container, self.options.series, diff --git a/tests/__init__.py b/tests/__init__.py index 7fab7b9..acbe475 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -395,9 +395,11 @@ ADT_PPAS = ADT_SHARED_RESULTS_CACHE = ADT_SWIFT_URL = http://localhost:18085 -ADT_SWIFT_USER = -ADT_SWIFT_TOKEN = -ADT_PRIVATE_SHARED = +ADT_SWIFT_USER = +ADT_SWIFT_PASS = +ADT_SWIFT_AUTH_URL = +ADT_SWIFT_TENANT = +ADT_PRIVATE_SHARED = ADT_PRIVATE_URL = ADT_CI_URL = https://autopkgtest.ubuntu.com/ ADT_HUGE = 20 diff --git a/tests/mock_swift.py b/tests/mock_swift.py index 1d5c0db..0fc1a4c 100644 --- a/tests/mock_swift.py +++ b/tests/mock_swift.py @@ -32,7 +32,6 @@ class SwiftHTTPRequestHandler(BaseHTTPRequestHandler): ''' # map container -> result.tar path -> (exitcode, testpkg-version[, testinfo]) results = {} - expected_token = 'SomeSecretToken' def do_GET(self): p = urlparse(self.path) @@ -44,22 +43,7 @@ class SwiftHTTPRequestHandler(BaseHTTPRequestHandler): else: self.list_container(container, parse_qs(p.query)) - def check_if_authorized(self, container): - status = True - if container.startswith('private-'): - # private container, so we need authentication - token = self.headers.get('X-Auth-Token') - if not token: - self.send_error(401, 'Unauthorized') - status = False - elif self.expected_token != token: - self.send_error(403, 'Forbidden') - status = False - return status - def serve_file(self, container, path): - if not self.check_if_authorized(container): - return if os.path.basename(path) != 'result.tar': self.send_error(404, 'File not found (only result.tar supported)') return @@ -101,8 +85,6 @@ class SwiftHTTPRequestHandler(BaseHTTPRequestHandler): self.wfile.write(tar.getvalue()) def list_container(self, container, query): - if not self.check_if_authorized(container): - return try: objs = set(['%s/result.tar' % r for r in self.results[container]]) except KeyError: @@ -146,7 +128,15 @@ class AutoPkgTestSwiftServer: ''' SwiftHTTPRequestHandler.results = results - def start(self): + def start(self, swiftclient=False): + if swiftclient: + # since we're running britney directly, the only way to reliably + # mock out the swiftclient module is to override it in the local + # path with the dummy version we created + src = os.path.join(TESTS_DIR, 'mock_swiftclient.py') + dst = os.path.join(PROJECT_DIR, 'swiftclient.py') + os.symlink(src, dst) + assert self.server_pid is None, 'already started' if self.log: self.log.close() @@ -170,6 +160,12 @@ class AutoPkgTestSwiftServer: sys.exit(0) def stop(self): + # in case we were 'mocking out' swiftclient, remove the symlink we + # created earlier during start() + swiftclient_mod = os.path.join(PROJECT_DIR, 'swiftclient.py') + if os.path.islink(swiftclient_mod): + os.unlink(swiftclient_mod) + assert self.server_pid, 'not running' os.kill(self.server_pid, 15) os.waitpid(self.server_pid, 0) diff --git a/tests/mock_swiftclient.py b/tests/mock_swiftclient.py new file mode 100644 index 0000000..5f419ac --- /dev/null +++ b/tests/mock_swiftclient.py @@ -0,0 +1,70 @@ +# Mock the swiftclient Python library, the bare minimum for ADT purposes +# Author: Łukasz 'sil2100' Zemczak + +import os +import sys + +from urllib.request import urlopen + + +# We want to use this single Python module file to mock out the exception +# module as well. +sys.modules["swiftclient.exceptions"] = sys.modules[__name__] + + +class ClientException(Exception): + def __init__(self, msg, http_status=''): + super(ClientException, self).__init__(msg) + self.msg = msg + self.http_status = http_status + + +class Connection: + def __init__(self, authurl, user, key, tenant_name, auth_version): + self._mocked_swift = 'http://localhost:18085' + + def get_container(self, container, marker=None, limit=None, prefix=None, + delimiter=None, end_marker=None, path=None, + full_listing=False, headers=None, query_string=None): + url = os.path.join(self._mocked_swift, container) + '?' + query_string + req = None + try: + req = urlopen(url, timeout=30) + code = req.getcode() + if code == 200: + result_paths = req.read().decode().strip().splitlines() + elif code == 204: # No content + result_paths = [] + else: + raise ClientException('MockedError', http_status=str(code)) + except IOError as e: + # 401 "Unauthorized" is swift's way of saying "container does not exist" + # But here we just assume swiftclient handles this via the usual + # ClientException. + raise ClientException('MockedError', http_status=str(e.code) if hasattr(e, 'code') else '') + finally: + if req is not None: + req.close() + + return (None, result_paths) + + def get_object(self, container, obj): + url = os.path.join(self._mocked_swift, container, obj) + req = None + try: + req = urlopen(url, timeout=30) + code = req.getcode() + if code == 200: + contents = req.read() + else: + raise ClientException('MockedError', http_status=str(code)) + except IOError as e: + # 401 "Unauthorized" is swift's way of saying "container does not exist" + # But here we just assume swiftclient handles this via the usual + # ClientException. + raise ClientException('MockedError', http_status=str(e.code) if hasattr(e, 'code') else '') + finally: + if req is not None: + req.close() + + return (None, contents) diff --git a/tests/test_autopkgtest.py b/tests/test_autopkgtest.py index 74b4766..281889a 100644 --- a/tests/test_autopkgtest.py +++ b/tests/test_autopkgtest.py @@ -117,7 +117,7 @@ class TestAutopkgtestBase(TestBase): with open(email_path, 'w', encoding='utf-8') as email: email.write(json.dumps(self.email_cache)) - self.swift.start() + self.swift.start(swiftclient=True) (excuses_yaml, excuses_html, out) = self.run_britney() self.swift.stop() @@ -2556,8 +2556,12 @@ class AT(TestAutopkgtestBase): print('ADT_PPAS = first/ppa user:password@joe/foo:DEADBEEF') elif line.startswith('ADT_SWIFT_USER'): print('ADT_SWIFT_USER = user') - elif line.startswith('ADT_SWIFT_TOKEN'): - print('ADT_SWIFT_TOKEN = SomeSecretToken') + elif line.startswith('ADT_SWIFT_PASS'): + print('ADT_SWIFT_PASS = pass') + elif line.startswith('ADT_SWIFT_TENANT'): + print('ADT_SWIFT_TENANT = tenant') + elif line.startswith('ADT_SWIFT_AUTH_URL'): + print('ADT_SWIFT_AUTH_URL = http://127.0.0.1:5000/v2.0/') elif line.startswith('ADT_PRIVATE_SHARED'): print('ADT_PRIVATE_SHARED = user1 team2') elif line.startswith('ADT_PRIVATE_URL'): @@ -2624,8 +2628,12 @@ class AT(TestAutopkgtestBase): for line in fileinput.input(self.britney_conf, inplace=True): if line.startswith('ADT_SWIFT_USER'): print('ADT_SWIFT_USER = user') - elif line.startswith('ADT_SWIFT_TOKEN'): - print('ADT_SWIFT_TOKEN = SomeSecretToken') + elif line.startswith('ADT_SWIFT_PASS'): + print('ADT_SWIFT_PASS = pass') + elif line.startswith('ADT_SWIFT_TENANT'): + print('ADT_SWIFT_TENANT = tenant') + elif line.startswith('ADT_SWIFT_AUTH_URL'): + print('ADT_SWIFT_AUTH_URL = http://127.0.0.1:5000/v2.0/') elif line.startswith('ADT_PRIVATE_URL'): print('ADT_PRIVATE_URL = http://localhost:18085/private-results/') else: diff --git a/tests/test_policy.py b/tests/test_policy.py index d7c411a..c6600c0 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -44,7 +44,9 @@ def initialize_policy(test_name, policy_class, *args, **kwargs): adt_swift_url='file://' + debci_data, adt_ci_url='', adt_swift_user='', - adt_swift_token='', + adt_swift_pass='', + adt_swift_tenant='', + adt_swift_auth_url='', adt_private_shared=[], adt_private_url='', adt_success_bounty=3,