diff --git a/omps/api/v1/push.py b/omps/api/v1/push.py index b740705..c6978b3 100644 --- a/omps/api/v1/push.py +++ b/omps/api/v1/push.py @@ -3,7 +3,6 @@ # see the LICENSE file for license # -from distutils import dir_util from functools import partial import logging import os @@ -11,8 +10,7 @@ import zipfile from flask import jsonify, current_app, request -from operatorcourier.api import flatten -from operatorcourier.errors import OpCourierError +from yaml import safe_load from . import API from omps.api.common import extract_auth_token, replace_registries @@ -24,12 +22,11 @@ OMPSInvalidVersionFormat, OMPSUploadedFileError, OMPSExpectedFileError, + PackageValidationError, QuayPackageNotFound, - raise_for_courier_exception, ) from omps.greenwave import GREENWAVE from omps.koji_util import KOJI -from omps.manifests_util import ManifestBundle from omps.quay import ReleaseVersion, ORG_MANAGER logger = logging.getLogger(__name__) @@ -170,20 +167,22 @@ def get_package_version(quay_org, repo, version=None): def _get_reponame_from_manifests(source_dir): - bundle = ManifestBundle.from_dir(source_dir) - return bundle.package_name - - -def _flatten_manifest_structure(source_dir, dest_dir): - try: - flatten(source_dir, dest_dir) - except OpCourierError as e: - raise_for_courier_exception(e) - - if not os.listdir(dest_dir): - # if dest dir is empty, it means that flatten did noop and source dir - # has already flat structure - dir_util.copy_tree(source_dir, dest_dir) + for filename in sorted(os.listdir(source_dir)): + filename = os.path.join(source_dir, filename) + if filename.endswith('.yaml') or filename.endswith('.yml'): + try: + with open(filename, 'r') as f: + contents = safe_load(f.read()) + if 'packageName' in contents: + name = contents['packageName'] + logger.info("Found packageName %s in %s", name, filename) + return name + except Exception: + message = "Failed to parse yaml file %s" % filename[len(source_dir):] + logger.exception(message) + raise PackageValidationError(message) + + raise PackageValidationError("Could not find packageName in manifests.") def _dir_files(dir_path): @@ -222,19 +221,15 @@ def _zip_flow(*, organization, repo, version, extract_manifest_func, logger.info("Extracted files: %s", extracted_files) data['extracted_files'] = extracted_files - with TemporaryDirectory() as tmpdir_flatten: - # operator-courier supports only flat dir structure - _flatten_manifest_structure(tmpdir, tmpdir_flatten) - - if repo is None: - repo = _get_reponame_from_manifests(tmpdir_flatten) + if repo is None: + repo = _get_reponame_from_manifests(tmpdir) - version = get_package_version(quay_org, repo, version) - logger.info("Using release version: %s", version) + version = get_package_version(quay_org, repo, version) + logger.info("Using release version: %s", version) - replace_registries(quay_org, tmpdir_flatten) + replace_registries(quay_org, tmpdir) - quay_org.push_operator_manifest(repo, version, tmpdir_flatten) + quay_org.push_operator_manifest(repo, version, tmpdir) data.update({ 'organization': organization, diff --git a/omps/manifests_util.py b/omps/manifests_util.py deleted file mode 100644 index ab60cf9..0000000 --- a/omps/manifests_util.py +++ /dev/null @@ -1,59 +0,0 @@ -# -# Copyright (C) 2019 Red Hat, Inc -# see the LICENSE file for license -# - -"""Helper classes/functions for operator manifests""" - -import logging -import io - -import yaml - -from operatorcourier.api import build_and_verify -from omps.errors import raise_for_courier_exception - -logger = logging.getLogger(__name__) - - -class ManifestBundle: - """Provides metadata of operator bundle""" - - @classmethod - def from_dir(cls, source_dir_path): - """Build bundle from specified directory - - :param source_dir_path: path to directory with operator manifest - :rtype: ManifestBundle - :return: object - """ - try: - bundle = build_and_verify(source_dir_path) - except Exception as e: - msg = "Operator courier failed: {}".format(e) - raise_for_courier_exception(e, new_msg=msg) - return cls(bundle) - - def __init__(self, bundle): - """ - :param bundle: bundle built by operator-courier - """ - self._bundle = bundle - - @property - def bundle(self): - """Returns operator bundle""" - return self._bundle - - @property - def package_name(self): - """Returns defined package name from operator bundle""" - # op. courier do verification, this should be never empty - if hasattr(self.bundle, 'bundle'): - # New, op. courier >= 2.0.0 - pkgs_yaml = self.bundle.bundle['data']['packages'] - else: - # Old, op. courier < 2.0.0 - pkgs_yaml = self.bundle['data']['packages'] - pkgs = yaml.safe_load(io.StringIO(pkgs_yaml)) - return pkgs[0]['packageName'] diff --git a/tests/api/v2/test_api_push.py b/tests/api/v2/test_api_push.py index 9083f0b..efbb3c5 100644 --- a/tests/api/v2/test_api_push.py +++ b/tests/api/v2/test_api_push.py @@ -107,7 +107,7 @@ def test_push_zipfile_encrypted( def test_push_koji_nvr( client, endpoint_push_koji, mocked_quay_io, mocked_op_courier_push, auth_header, mocked_koji_archive_download, mocked_greenwave): - """Test REST API for pushing operators form koji by NVR""" + """Test REST API for pushing operators from koji by NVR""" archive = mocked_koji_archive_download rv = client.post( endpoint_push_koji.url_path, @@ -125,6 +125,23 @@ def test_push_koji_nvr( mocked_greenwave.assert_called_once_with(endpoint_push_koji.nvr) +def test_push_invalid_manifests( + client, endpoint_push_koji, mocked_quay_io, mocked_op_courier_push, + auth_header, mocked_bad_koji_archive_download, mocked_greenwave): + """Test REST API for failing to push operators with bad manifests """ + rv = client.post( + endpoint_push_koji.url_path, + headers=auth_header + ) + assert rv.status_code == requests.codes.bad_request, rv.get_json() + rv_json = rv.get_json() + assert rv_json['error'] == 'PackageValidationError' + assert rv_json['message'] in ( + 'Could not find packageName in manifests.', + 'Failed to parse yaml file /not_yaml.yaml', + ) + + def test_push_koji_unauthorized(client, endpoint_push_koji): """Test if api properly refuses unauthorized requests""" rv = client.post(endpoint_push_koji.url_path) diff --git a/tests/conftest.py b/tests/conftest.py index 138f3de..1eeab61 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -59,7 +59,7 @@ def datadir(tmpdir): ('etcd_op_nested', 'etcd'), ]) def valid_manifest_dir(request, datadir): - """Return metadata and path to manifest""" + """Return metadata and path to valid manifest""" manifest_dir_name, pkg_name = request.param path = os.path.join(datadir, manifest_dir_name) return ManifestDirMeta( @@ -69,6 +69,21 @@ def valid_manifest_dir(request, datadir): ) +@pytest.fixture(params=[ + ('no_package_name', 'no_package_name'), + ('not_yaml', 'not_yaml'), +]) +def invalid_manifest_dir(request, datadir): + """Return metadata and path to invalid manifest""" + manifest_dir_name, pkg_name = request.param + path = os.path.join(datadir, manifest_dir_name) + return ManifestDirMeta( + path=path, + pkg_name=pkg_name, + valid=False + ) + + @pytest.fixture def valid_manifest_flatten_dir(valid_manifest_dir): """Most operator-courier operations require flatten dir structure""" @@ -86,12 +101,11 @@ def valid_manifest_flatten_dir(valid_manifest_dir): ) -@pytest.fixture -def valid_manifests_archive(datadir, tmpdir, valid_manifest_dir): +def _manifests_archive(datadir, tmpdir, manifest_dir): """Construct valid operator manifest data zip archive""" path = os.path.join(tmpdir, 'test_archive.zip') - start = valid_manifest_dir.path + start = manifest_dir.path res_files = [] with zipfile.ZipFile(path, 'w') as zip_archive: @@ -105,8 +119,20 @@ def valid_manifests_archive(datadir, tmpdir, valid_manifest_dir): return ArchiveMeta( path=path, files=sorted(res_files), - pkg_name=valid_manifest_dir.pkg_name, - valid=valid_manifest_dir.valid) + pkg_name=manifest_dir.pkg_name, + valid=manifest_dir.valid) + + +@pytest.fixture +def valid_manifests_archive(datadir, tmpdir, valid_manifest_dir): + """Construct valid operator manifest data zip archive""" + return _manifests_archive(datadir, tmpdir, valid_manifest_dir) + + +@pytest.fixture +def invalid_manifests_archive(datadir, tmpdir, invalid_manifest_dir): + """Construct invalid operator manifest data zip archive""" + return _manifests_archive(datadir, tmpdir, invalid_manifest_dir) @pytest.fixture @@ -181,22 +207,32 @@ def __exit__(self, *args): return CourierPushCM -@pytest.fixture -def mocked_koji_archive_download(valid_manifests_archive): - """Mock KojiUtil.koji_download_manifest_archive to return valid archive""" +def _koji_archive_download(manifests_archive): def fake_download(nvr, target_fd): - with open(valid_manifests_archive.path, 'rb') as zf: + with open(manifests_archive.path, 'rb') as zf: target_fd.write(zf.read()) target_fd.flush() orig = KOJI.download_manifest_archive try: KOJI.download_manifest_archive = fake_download - yield valid_manifests_archive + yield manifests_archive finally: KOJI.download_manifest_archive = orig +@pytest.fixture +def mocked_koji_archive_download(valid_manifests_archive): + """Mock KojiUtil.koji_download_manifest_archive to return valid archive""" + yield from _koji_archive_download(valid_manifests_archive) + + +@pytest.fixture +def mocked_bad_koji_archive_download(invalid_manifests_archive): + """Mock KojiUtil.koji_download_manifest_archive to return invalid archive""" + yield from _koji_archive_download(invalid_manifests_archive) + + @pytest.fixture def mocked_koji_get_api_version(): """Mock global KOJI.get_api_version to return valid version""" diff --git a/tests/data/no_package_name/package.yaml b/tests/data/no_package_name/package.yaml new file mode 100644 index 0000000..02104ee --- /dev/null +++ b/tests/data/no_package_name/package.yaml @@ -0,0 +1,2 @@ +# Note there is no packageName defined here, making this invalid. +key: value \ No newline at end of file diff --git a/tests/data/not_yaml/not_yaml.yaml b/tests/data/not_yaml/not_yaml.yaml new file mode 100644 index 0000000..dab3f92 --- /dev/null +++ b/tests/data/not_yaml/not_yaml.yaml @@ -0,0 +1,2 @@ +foo: bar +baz \ No newline at end of file diff --git a/tests/data/not_yaml/package.yaml b/tests/data/not_yaml/package.yaml new file mode 100644 index 0000000..84391bd --- /dev/null +++ b/tests/data/not_yaml/package.yaml @@ -0,0 +1 @@ +packageName: not_yaml \ No newline at end of file diff --git a/tests/test_manifest_util.py b/tests/test_manifest_util.py deleted file mode 100644 index 3693f6a..0000000 --- a/tests/test_manifest_util.py +++ /dev/null @@ -1,30 +0,0 @@ -# -# Copyright (C) 2019 Red Hat, Inc -# see the LICENSE file for license -# - -import tempfile - -import pytest - -from omps.errors import PackageValidationError -from omps.manifests_util import ManifestBundle - - -class TestManifestBundle: - """Tests of ManifestBundle class""" - - def test_invalid_bundle(self): - """Test if proper exception is raised when source data are invalid. - Uses empty dir to force operator-courier to raise an exception - """ - with pytest.raises(PackageValidationError) as exc_info, \ - tempfile.TemporaryDirectory() as tmpdir: - ManifestBundle.from_dir(tmpdir) - - assert str(exc_info.value).startswith('Operator courier failed: ') - - def test_package_name(self, valid_manifest_flatten_dir): - """Test of property which provides package name""" - mb = ManifestBundle.from_dir(valid_manifest_flatten_dir.path) - assert mb.package_name == valid_manifest_flatten_dir.pkg_name