From 1b6ecb6c1c59ca575fe1972588b699129f4608d2 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 9 Nov 2017 16:21:40 -0500 Subject: [PATCH] Fix bug in listing owned tags We were indexing into a map using the docker_image_id, but the ancestors use the *image id*. Also cleans up the code and adds some tests. Fixes https://jira.prod.coreos.systems/browse/QS-55 --- endpoints/api/tag.py | 22 +-- endpoints/api/tag_models_interface.py | 3 +- endpoints/api/tag_models_pre_oci.py | 5 +- endpoints/api/test/test_tag.py | 16 ++- endpoints/api/test/test_tag_models_pre_oci.py | 126 ------------------ 5 files changed, 27 insertions(+), 145 deletions(-) diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index 106d051f0..df5861096 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -172,14 +172,11 @@ class RepositoryTagImages(RepositoryParamResource): if tag_image is None: raise NotFound() + # Find all the parent images for the tag. parent_images = model.get_parent_images(namespace, repository, tag_image.docker_image_id) - image_map = {str(tag_image.docker_image_id): tag_image} - - for image in parent_images: - image_map[str(image.docker_image_id)] = image - - image_map_all = dict(image_map) all_images = [tag_image] + list(parent_images) + image_map = {image.docker_image_id: image for image in all_images} + skip_set = set() # Filter the images returned to those not found in the ancestry of any of the other tags in # the repository. @@ -189,18 +186,13 @@ class RepositoryTagImages(RepositoryParamResource): if current_tag.name == tag: continue - # Remove the tag's image ID. - tag_image_id = str(current_tag.docker_image_id) - image_map.pop(tag_image_id, None) - - # Remove any ancestors: - for ancestor_id in current_tag.image.ancestors.split('/'): - image_map.pop(ancestor_id, None) + skip_set.add(current_tag.image.ancestor_id) + skip_set = skip_set | set(current_tag.image.ancestor_id_list) return { 'images': [ - image.to_dict(image_map_all) for image in all_images - if not parsed_args['owned'] or (str(image.docker_image_id) in image_map) + image.to_dict(image_map) for image in all_images + if not parsed_args['owned'] or (image.ancestor_id not in skip_set) ] } diff --git a/endpoints/api/tag_models_interface.py b/endpoints/api/tag_models_interface.py index a005aed9b..82e5c4ca2 100644 --- a/endpoints/api/tag_models_interface.py +++ b/endpoints/api/tag_models_interface.py @@ -61,7 +61,7 @@ class Repository(namedtuple('Repository', ['namespace_name', 'repository_name']) class Image( namedtuple('Image', [ 'docker_image_id', 'created', 'comment', 'command', 'storage_image_size', - 'storage_uploading', 'ancestor_id_list' + 'storage_uploading', 'ancestor_id_list', 'ancestor_id' ])): """ Image @@ -72,6 +72,7 @@ class Image( :type storage_image_size: int :type storage_uploading: boolean :type ancestor_id_list: [int] + :type ancestor_id: int """ def to_dict(self, image_map, include_ancestors=True): diff --git a/endpoints/api/tag_models_pre_oci.py b/endpoints/api/tag_models_pre_oci.py index 99adf9a57..72297d593 100644 --- a/endpoints/api/tag_models_pre_oci.py +++ b/endpoints/api/tag_models_pre_oci.py @@ -120,11 +120,12 @@ def convert_image(database_image): comment=database_image.comment, command=database_image.command, storage_image_size=database_image.storage.image_size, storage_uploading=database_image.storage.uploading, - ancestor_id_list=database_image.ancestor_id_list()) + ancestor_id_list=database_image.ancestor_id_list(), + ancestor_id=database_image.id) def convert_tag(tag, manifest_list=None): - return Tag(name=tag.name, image=tag.image, reversion=tag.reversion, + return Tag(name=tag.name, image=convert_image(tag.image), reversion=tag.reversion, lifetime_start_ts=tag.lifetime_start_ts, lifetime_end_ts=tag.lifetime_end_ts, manifest_list=manifest_list, docker_image_id=tag.image.docker_image_id) diff --git a/endpoints/api/test/test_tag.py b/endpoints/api/test/test_tag.py index abc0adf9e..316b9fbd3 100644 --- a/endpoints/api/test/test_tag.py +++ b/endpoints/api/test/test_tag.py @@ -8,7 +8,7 @@ from data.model import DataModelException from endpoints.api.tag_models_interface import RepositoryTagHistory, Tag from endpoints.api.test.shared import conduct_api_call from endpoints.test.shared import client_with_identity -from endpoints.api.tag import RepositoryTag, RestoreTag, ListRepositoryTags +from endpoints.api.tag import RepositoryTag, RestoreTag, ListRepositoryTags, RepositoryTagImages from features import FeatureNameValue @@ -281,3 +281,17 @@ def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_statu if manifest_generated: generate_manifest.assert_called_with('devtable', 'simple', test_tag) + +@pytest.mark.parametrize('repository, tag, owned, expect_images', [ + ('devtable/simple', 'prod', False, True), + ('devtable/simple', 'prod', True, False), + ('devtable/simple', 'latest', False, True), + ('devtable/simple', 'latest', True, False), + + ('devtable/complex', 'prod', False, True), + ('devtable/complex', 'prod', True, True), +]) +def test_list_tag_images(repository, tag, owned, expect_images, authd_client): + params = {'repository': repository, 'tag': tag, 'owned': owned} + result = conduct_api_call(authd_client, RepositoryTagImages, 'get', params, None, 200).json + assert bool(result['images']) == expect_images diff --git a/endpoints/api/test/test_tag_models_pre_oci.py b/endpoints/api/test/test_tag_models_pre_oci.py index 5ddb34ab3..88303c17e 100644 --- a/endpoints/api/test/test_tag_models_pre_oci.py +++ b/endpoints/api/test/test_tag_models_pre_oci.py @@ -34,78 +34,6 @@ def mock_out_get_repository(monkeypatch, namespace_name, repository_name): monkeypatch.setattr(model.repository, 'get_repository', return_function) -def create_mock_tag(name, reversion, lifetime_start_ts, lifetime_end_ts, mock_id, docker_image_id, - manifest_list): - tag_mock = Mock() - tag_mock.name = name - image_mock = Mock() - image_mock.docker_image_id = docker_image_id - tag_mock.image = image_mock - tag_mock.reversion = reversion - tag_mock.lifetime_start_ts = lifetime_start_ts - tag_mock.lifetime_end_ts = lifetime_end_ts - tag_mock.id = mock_id - tag_mock.manifest_list = manifest_list - tag = Tag(name=name, reversion=reversion, image=image_mock, docker_image_id=docker_image_id, - lifetime_start_ts=lifetime_start_ts, lifetime_end_ts=lifetime_end_ts, - manifest_list=manifest_list) - return tag_mock, tag - - -first_mock, first_tag = create_mock_tag('tag1', 'rev1', 'start1', 'end1', 'id1', - 'docker_image_id1', []) -second_mock, second_tag = create_mock_tag('tag2', 'rev2', 'start2', 'end2', 'id2', - 'docker_image_id2', ['manifest']) - - -def mock_out_list_repository_tag_history(monkeypatch, namespace_name, repository_name, page, size, - specific_tag): - def list_empty_tag_history(repository, page, size, specific_tag): - return [], {}, False - - def list_filled_tag_history(repository, page, size, specific_tag): - tags = [first_mock, second_mock] - return tags, { - first_mock.id: first_mock.manifest_list, - second_mock.id: second_mock.manifest_list - }, len(tags) > size - - def list_only_second_tag(repository, page, size, specific_tag): - tags = [second_mock] - return tags, {second_mock.id: second_mock.manifest_list}, len(tags) > size - - if namespace_name == EMPTY_NAMESPACE or repository_name == EMPTY_REPOSITORY: - return_function = list_empty_tag_history - else: - if specific_tag == 'tag2': - return_function = list_only_second_tag - else: - return_function = list_filled_tag_history - - monkeypatch.setattr(model.tag, 'list_repository_tag_history', return_function) - - -@pytest.mark.parametrize( - 'expected, namespace_name, repository_name, page, size, specific_tag', [ - (None, BAD_NAMESPACE_NAME, 'repository_name', 1, 100, None), - (None, 'namespace_name', BAD_REPOSITORY_NAME, 1, 100, None), - (RepositoryTagHistory(tags=[], more=False), EMPTY_NAMESPACE, EMPTY_REPOSITORY, 1, 100, None), - (RepositoryTagHistory(tags=[first_tag, second_tag], more=False), 'namespace', 'repository', 1, - 100, None), - (RepositoryTagHistory(tags=[first_tag, second_tag], more=True), 'namespace', 'repository', 1, - 1, None), - (RepositoryTagHistory(tags=[second_tag], more=False), 'namespace', 'repository', 1, 100, - 'tag2'), - ]) -def test_list_repository_tag_history(expected, namespace_name, repository_name, page, size, - specific_tag, get_monkeypatch): - mock_out_get_repository(get_monkeypatch, namespace_name, repository_name) - mock_out_list_repository_tag_history(get_monkeypatch, namespace_name, repository_name, page, - size, specific_tag) - assert pre_oci_model.list_repository_tag_history(namespace_name, repository_name, page, size, - specific_tag) == expected - - def get_repo_mock(monkeypatch, return_value): def return_return_value(namespace_name, repository_name): return return_value @@ -238,60 +166,6 @@ def test_get_parent_images_empty_parent_images(get_monkeypatch): assert images == [] -def compare_images(parent_image, attr_dict): - for field in parent_image._fields: - assert getattr(parent_image, field) == getattr(attr_dict, field) - - -def test_get_parent_images(get_monkeypatch): - get_image_by_id_mock = Mock() - get_monkeypatch.setattr(model.image, 'get_image_by_id', get_image_by_id_mock) - - def fake_list(): - return [] - - image_one = AttrDict({ - 'docker_image_id': 'docker_image_id', - 'created': 'created_one', - 'comment': 'comment_one', - 'command': 'command_one', - 'storage': AttrDict({ - 'image_size': 'image_size_one', - 'uploading': 'uploading_one' - }), - 'ancestors': 'one/two/three', - 'ancestor_id_list': fake_list - }) - image_two = AttrDict({ - 'docker_image_id': 'docker_image_id_two', - 'created': 'created', - 'comment': 'comment', - 'command': 'command', - 'storage': AttrDict({ - 'image_size': 'image_size', - 'uploading': 'uploading' - }), - 'ancestors': 'four/five/six', - 'ancestor_id_list': fake_list - }) - - get_parent_images_mock = Mock(return_value=[image_one, image_two]) - get_monkeypatch.setattr(model.image, 'get_parent_images', get_parent_images_mock) - get_monkeypatch.setattr(model.image, 'get_image_by_id', Mock()) - - images = pre_oci_model.get_parent_images('namespace_name', 'repository_name', 'tag_name') - image_one.ancestor_id_list = [] - image_one.storage_image_size = 'image_size_one' - image_one.storage_uploading = 'uploading_one' - image_one.ancestor_length = 13 - image_two.ancestor_id_list = [] - image_two.storage_uploading = 'uploading' - image_two.storage_image_size = 'image_size' - image_two.ancestor_length = 13 - compare_images(images[0], image_one) - compare_images(images[1], image_two) - - def test_list_repository_tags(get_monkeypatch): mock = Mock(return_value=[])