From f7bf6a223ce457f3cc2dc3425d7a883fe26e3b5c Mon Sep 17 00:00:00 2001 From: EvB Date: Tue, 2 May 2017 15:47:59 -0400 Subject: [PATCH 1/6] feat(api/tag): generate manifest on tag --- endpoints/api/tag.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index a81e7139a..9e928df93 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -8,6 +8,7 @@ from endpoints.api import (resource, nickname, require_repo_read, require_repo_w disallow_for_app_repositories, disallow_under_trust) from endpoints.exception import NotFound from endpoints.api.image import image_view +from endpoints.v2.manifest import _generate_and_store_manifest from data import model from auth.auth_context import get_authenticated_user from util.names import TAG_ERROR, TAG_REGEX @@ -111,11 +112,16 @@ class RepositoryTag(RepositoryParamResource): model.tag.create_or_update_tag(namespace, repository, tag, image_id) username = get_authenticated_user().username - log_action('move_tag' if original_image_id else 'create_tag', namespace, - {'username': username, 'repo': repository, 'tag': tag, - 'namespace': namespace, 'image': image_id, - 'original_image': original_image_id}, - repo=model.repository.get_repository(namespace, repository)) + tag_data = {'username': username, 'repo': repository, 'tag': tag, 'namespace': namespace, + 'image': image_id, 'original_image': original_image_id} + + log_action('move_tag' if original_image_id else 'create_tag', namespace, tag_data, + repo=model.repository.get_repository(namespace, repository)) + try: + _generate_and_store_manifest(namespace, repository, tag) + except: + # log and move on since we'll attempt to store manifest on first pull as well + logger.exception('unable to store manifest for tag', extra=tag_data) return 'Updated', 201 From 70508e3692a91f1b808ba0ce94a98487029b62af Mon Sep 17 00:00:00 2001 From: EvB Date: Tue, 2 May 2017 16:13:21 -0400 Subject: [PATCH 2/6] feat(api/tag): generate manifest on revert_tag action --- endpoints/api/tag.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index 9e928df93..d3c6352c7 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -230,12 +230,8 @@ class RestoreTag(RepositoryParamResource): # Restore the tag back to the previous image. image_id = request.get_json()['image'] manifest_digest = request.get_json().get('manifest_digest', None) - if manifest_digest is not None: - existing_image = model.tag.restore_tag_to_manifest(repo, tag, manifest_digest) - else: - existing_image = model.tag.restore_tag_to_image(repo, tag, image_id) - # Log the reversion/restoration. + # Data for logging the reversion/restoration. username = get_authenticated_user().username log_data = { 'username': username, @@ -244,6 +240,17 @@ class RestoreTag(RepositoryParamResource): 'image': image_id, } + if manifest_digest is not None: + existing_image = model.tag.restore_tag_to_manifest(repo, tag, manifest_digest) + else: + existing_image = model.tag.restore_tag_to_image(repo, tag, image_id) + try: + _generate_and_store_manifest(namespace, repository, tag) + except: + # log and move on since we'll attempt to store manifest on first pull as well + logger.exception('unable to store manifest for tag', extra=log_data) + + if existing_image is not None: log_data['original_image'] = existing_image.docker_image_id From eb9db0c53be25c00f586aa3220fcd47805a1a6a9 Mon Sep 17 00:00:00 2001 From: EvB Date: Tue, 2 May 2017 18:10:20 -0400 Subject: [PATCH 3/6] test(api/tag): unit test movetag --- endpoints/api/test/test_tag.py | 57 ++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 endpoints/api/test/test_tag.py diff --git a/endpoints/api/test/test_tag.py b/endpoints/api/test/test_tag.py new file mode 100644 index 000000000..ea8c09b0e --- /dev/null +++ b/endpoints/api/test/test_tag.py @@ -0,0 +1,57 @@ +import pytest + +from mock import patch, Mock + +from endpoints.api.test.shared import client_with_identity, conduct_api_call +from endpoints.api.tag import RepositoryTag, RestoreTag +from features import FeatureNameValue + +from test.fixtures import * + +@pytest.fixture() +def get_repo_image(): + def mock_callable(namespace, repository, image_id): + img = Mock(repository='fetched_repository') if image_id == 'image1' else None + return img + with patch('endpoints.api.tag.model.image.get_repo_image', side_effect=mock_callable) as mk: + yield mk + +@pytest.fixture() +def get_repo_tag_image(): + def mock_get_repo_tag_image(repository, tag): + tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None + return tag_img + with patch('endpoints.api.tag.model.tag.get_repo_tag_image', side_effect=mock_get_repo_tag_image): + yield + +@pytest.fixture() +def create_or_update_tag(): + with patch('endpoints.api.tag.model.tag.create_or_update_tag') as mk: + yield mk + +@pytest.fixture() +def generate_manifest(): + def mock_callable(namespace, repository, tag): + if tag is 'generatemanifestfail': + raise Exception('test_failure') + with patch('endpoints.api.tag._generate_and_store_manifest', side_effect=mock_callable) as mk: + yield mk + +@pytest.fixture() +def authd_client(client): + with client_with_identity('devtable', client) as cl: + yield cl + +@pytest.mark.parametrize('test_image,test_tag,expected_status', [ + ('image1', '-INVALID-TAG-NAME', 400), + ('image1', '.INVALID-TAG-NAME', 400), + ('image1', 'INVALID-TAG_NAME-BECAUSE-THIS-IS-WAY-WAY-TOO-LOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOONG', 400), + ('nonexistantimage', 'newtag', 404), + ('image1', 'generatemanifestfail', 201), + ('image1', 'existing-tag', 201), + ('image1', 'newtag', 201), +]) +def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_repo_tag_image, create_or_update_tag, generate_manifest, authd_client): + params = {'repository': 'devtable/repo', 'tag': test_tag} + request_body = {'image': test_image} + conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) From 6e894d5f24441d88aab2f84f3a98b6675e5192f5 Mon Sep 17 00:00:00 2001 From: EvB Date: Wed, 3 May 2017 16:48:12 -0400 Subject: [PATCH 4/6] refactor(api/tag): remove try/catch --- endpoints/api/tag.py | 22 +++++++--------------- endpoints/api/test/test_tag.py | 10 +++++++--- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index d3c6352c7..55510df57 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -112,16 +112,13 @@ class RepositoryTag(RepositoryParamResource): model.tag.create_or_update_tag(namespace, repository, tag, image_id) username = get_authenticated_user().username - tag_data = {'username': username, 'repo': repository, 'tag': tag, 'namespace': namespace, - 'image': image_id, 'original_image': original_image_id} + log_action('move_tag' if original_image_id else 'create_tag', namespace, + {'username': username, 'repo': repository, 'tag': tag, + 'namespace': namespace, 'image': image_id, + 'original_image': original_image_id}, + repo=model.repository.get_repository(namespace, repository)) - log_action('move_tag' if original_image_id else 'create_tag', namespace, tag_data, - repo=model.repository.get_repository(namespace, repository)) - try: - _generate_and_store_manifest(namespace, repository, tag) - except: - # log and move on since we'll attempt to store manifest on first pull as well - logger.exception('unable to store manifest for tag', extra=tag_data) + _generate_and_store_manifest(namespace, repository, tag) return 'Updated', 201 @@ -244,12 +241,7 @@ class RestoreTag(RepositoryParamResource): existing_image = model.tag.restore_tag_to_manifest(repo, tag, manifest_digest) else: existing_image = model.tag.restore_tag_to_image(repo, tag, image_id) - try: - _generate_and_store_manifest(namespace, repository, tag) - except: - # log and move on since we'll attempt to store manifest on first pull as well - logger.exception('unable to store manifest for tag', extra=log_data) - + _generate_and_store_manifest(namespace, repository, tag) if existing_image is not None: log_data['original_image'] = existing_image.docker_image_id diff --git a/endpoints/api/test/test_tag.py b/endpoints/api/test/test_tag.py index ea8c09b0e..01e1eb22f 100644 --- a/endpoints/api/test/test_tag.py +++ b/endpoints/api/test/test_tag.py @@ -32,7 +32,7 @@ def create_or_update_tag(): @pytest.fixture() def generate_manifest(): def mock_callable(namespace, repository, tag): - if tag is 'generatemanifestfail': + if tag == 'generatemanifestfail': raise Exception('test_failure') with patch('endpoints.api.tag._generate_and_store_manifest', side_effect=mock_callable) as mk: yield mk @@ -47,11 +47,15 @@ def authd_client(client): ('image1', '.INVALID-TAG-NAME', 400), ('image1', 'INVALID-TAG_NAME-BECAUSE-THIS-IS-WAY-WAY-TOO-LOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOONG', 400), ('nonexistantimage', 'newtag', 404), - ('image1', 'generatemanifestfail', 201), + ('image1', 'generatemanifestfail', None), ('image1', 'existing-tag', 201), ('image1', 'newtag', 201), ]) def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_repo_tag_image, create_or_update_tag, generate_manifest, authd_client): params = {'repository': 'devtable/repo', 'tag': test_tag} request_body = {'image': test_image} - conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) + if expected_status is None: + with pytest.raises(Exception): + conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) + else: + conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) From 25b9d044dee1b22c94bca326a46bca2a8720e626 Mon Sep 17 00:00:00 2001 From: EvB Date: Wed, 3 May 2017 17:24:39 -0400 Subject: [PATCH 5/6] test(api/tag): test restore_tag --- endpoints/api/test/test_tag.py | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/endpoints/api/test/test_tag.py b/endpoints/api/test/test_tag.py index 01e1eb22f..9a26ecd24 100644 --- a/endpoints/api/test/test_tag.py +++ b/endpoints/api/test/test_tag.py @@ -16,6 +16,11 @@ def get_repo_image(): with patch('endpoints.api.tag.model.image.get_repo_image', side_effect=mock_callable) as mk: yield mk +@pytest.fixture() +def get_repository(): + with patch('endpoints.api.tag.model.image.get_repo_image', return_value='mock_repo') as mk: + yield mk + @pytest.fixture() def get_repo_tag_image(): def mock_get_repo_tag_image(repository, tag): @@ -24,6 +29,22 @@ def get_repo_tag_image(): with patch('endpoints.api.tag.model.tag.get_repo_tag_image', side_effect=mock_get_repo_tag_image): yield +@pytest.fixture() +def restore_tag_to_manifest(): + def mock_restore_tag_to_manifest(repository, tag, manifest_digest): + tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None + return tag_img + with patch('endpoints.api.tag.model.tag.restore_tag_to_manifest', side_effect=mock_restore_tag_to_manifest): + yield + +@pytest.fixture() +def restore_tag_to_image(): + def mock_restore_tag_to_image(repository, tag, image_id): + tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None + return tag_img + with patch('endpoints.api.tag.model.tag.restore_tag_to_image', side_effect=mock_restore_tag_to_image): + yield + @pytest.fixture() def create_or_update_tag(): with patch('endpoints.api.tag.model.tag.create_or_update_tag') as mk: @@ -59,3 +80,22 @@ def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_rep conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) else: conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) + +@pytest.mark.parametrize('test_manifest,test_tag,manifest_generated,expected_status', [ + (None, 'newtag', True, 200), + (None, 'generatemanifestfail', True, None), + ('manifest1', 'newtag', False, 200), +]) +def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_status, get_repository, restore_tag_to_manifest, restore_tag_to_image, generate_manifest, authd_client): + params = {'repository': 'devtable/repo', 'tag': test_tag} + request_body = {'image': 'image1'} + if test_manifest is not None: + request_body['manifest_digest'] = test_manifest + if expected_status is None: + with pytest.raises(Exception): + conduct_api_call(authd_client, RestoreTag, 'post', params, request_body, expected_status) + else: + conduct_api_call(authd_client, RestoreTag, 'post', params, request_body, expected_status) + + if manifest_generated: + generate_manifest.assert_called_with('devtable', 'repo', test_tag) From 467c72a9acc8fb5746976330530d7c7bea9f4d98 Mon Sep 17 00:00:00 2001 From: EvB Date: Thu, 4 May 2017 11:33:36 -0400 Subject: [PATCH 6/6] code-stye Yapf: 2 files updated --- endpoints/api/tag.py | 42 ++++++++++++++++++++-------------- endpoints/api/test/test_tag.py | 37 ++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 24 deletions(-) diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index 55510df57..adc4a60d2 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -2,10 +2,10 @@ from flask import request, abort -from endpoints.api import (resource, nickname, require_repo_read, require_repo_write, - RepositoryParamResource, log_action, validate_json_request, - path_param, parse_args, query_param, truthy_bool, - disallow_for_app_repositories, disallow_under_trust) +from endpoints.api import ( + resource, nickname, require_repo_read, require_repo_write, RepositoryParamResource, log_action, + validate_json_request, path_param, parse_args, query_param, truthy_bool, + disallow_for_app_repositories, disallow_under_trust) from endpoints.exception import NotFound from endpoints.api.image import image_view from endpoints.v2.manifest import _generate_and_store_manifest @@ -23,7 +23,8 @@ class ListRepositoryTags(RepositoryParamResource): @disallow_for_app_repositories @parse_args() @query_param('specificTag', 'Filters the tags to the specific tag.', type=str, default='') - @query_param('limit', 'Limit to the number of results to return per page. Max 100.', type=int, default=50) + @query_param('limit', 'Limit to the number of results to return per page. Max 100.', type=int, + default=50) @query_param('page', 'Page index for the results. Default 1.', type=int, default=1) @nickname('listRepoTags') def get(self, namespace, repository, parsed_args): @@ -112,11 +113,14 @@ class RepositoryTag(RepositoryParamResource): model.tag.create_or_update_tag(namespace, repository, tag, image_id) username = get_authenticated_user().username - log_action('move_tag' if original_image_id else 'create_tag', namespace, - {'username': username, 'repo': repository, 'tag': tag, - 'namespace': namespace, 'image': image_id, - 'original_image': original_image_id}, - repo=model.repository.get_repository(namespace, repository)) + log_action('move_tag' if original_image_id else 'create_tag', namespace, { + 'username': username, + 'repo': repository, + 'tag': tag, + 'namespace': namespace, + 'image': image_id, + 'original_image': original_image_id + }, repo=model.repository.get_repository(namespace, repository)) _generate_and_store_manifest(namespace, repository, tag) @@ -132,8 +136,10 @@ class RepositoryTag(RepositoryParamResource): username = get_authenticated_user().username log_action('delete_tag', namespace, - {'username': username, 'repo': repository, 'namespace': namespace, 'tag': tag}, - repo=model.repository.get_repository(namespace, repository)) + {'username': username, + 'repo': repository, + 'namespace': namespace, + 'tag': tag}, repo=model.repository.get_repository(namespace, repository)) return '', 204 @@ -143,6 +149,7 @@ class RepositoryTag(RepositoryParamResource): @path_param('tag', 'The name of the tag') class RepositoryTagImages(RepositoryParamResource): """ Resource for listing the images in a specific repository tag. """ + @require_repo_read @nickname('listTagImages') @disallow_for_app_repositories @@ -184,12 +191,13 @@ class RepositoryTagImages(RepositoryParamResource): image_map.pop(ancestor_id, None) return { - 'images': [image_view(image, image_map_all) for image in all_images - if not parsed_args['owned'] or (str(image.id) in image_map)] + 'images': [ + image_view(image, image_map_all) for image in all_images + if not parsed_args['owned'] or (str(image.id) in image_map) + ] } - @resource('/v1/repository//tag//restore') @path_param('repository', 'The full path of the repository. e.g. namespace/name') @path_param('tag', 'The name of the tag') @@ -246,8 +254,8 @@ class RestoreTag(RepositoryParamResource): if existing_image is not None: log_data['original_image'] = existing_image.docker_image_id - log_action('revert_tag', namespace, - log_data, repo=model.repository.get_repository(namespace, repository)) + log_action('revert_tag', namespace, log_data, repo=model.repository.get_repository( + namespace, repository)) return { 'image_id': image_id, diff --git a/endpoints/api/test/test_tag.py b/endpoints/api/test/test_tag.py index 9a26ecd24..0c80ef4ee 100644 --- a/endpoints/api/test/test_tag.py +++ b/endpoints/api/test/test_tag.py @@ -8,71 +8,91 @@ from features import FeatureNameValue from test.fixtures import * + @pytest.fixture() def get_repo_image(): def mock_callable(namespace, repository, image_id): img = Mock(repository='fetched_repository') if image_id == 'image1' else None return img + with patch('endpoints.api.tag.model.image.get_repo_image', side_effect=mock_callable) as mk: yield mk + @pytest.fixture() def get_repository(): with patch('endpoints.api.tag.model.image.get_repo_image', return_value='mock_repo') as mk: yield mk + @pytest.fixture() def get_repo_tag_image(): def mock_get_repo_tag_image(repository, tag): tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None return tag_img - with patch('endpoints.api.tag.model.tag.get_repo_tag_image', side_effect=mock_get_repo_tag_image): + + with patch('endpoints.api.tag.model.tag.get_repo_tag_image', + side_effect=mock_get_repo_tag_image): yield + @pytest.fixture() def restore_tag_to_manifest(): def mock_restore_tag_to_manifest(repository, tag, manifest_digest): tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None return tag_img - with patch('endpoints.api.tag.model.tag.restore_tag_to_manifest', side_effect=mock_restore_tag_to_manifest): + + with patch('endpoints.api.tag.model.tag.restore_tag_to_manifest', + side_effect=mock_restore_tag_to_manifest): yield + @pytest.fixture() def restore_tag_to_image(): def mock_restore_tag_to_image(repository, tag, image_id): tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None return tag_img - with patch('endpoints.api.tag.model.tag.restore_tag_to_image', side_effect=mock_restore_tag_to_image): + + with patch('endpoints.api.tag.model.tag.restore_tag_to_image', + side_effect=mock_restore_tag_to_image): yield + @pytest.fixture() def create_or_update_tag(): with patch('endpoints.api.tag.model.tag.create_or_update_tag') as mk: yield mk + @pytest.fixture() def generate_manifest(): def mock_callable(namespace, repository, tag): if tag == 'generatemanifestfail': raise Exception('test_failure') + with patch('endpoints.api.tag._generate_and_store_manifest', side_effect=mock_callable) as mk: yield mk + @pytest.fixture() def authd_client(client): with client_with_identity('devtable', client) as cl: yield cl + @pytest.mark.parametrize('test_image,test_tag,expected_status', [ ('image1', '-INVALID-TAG-NAME', 400), ('image1', '.INVALID-TAG-NAME', 400), - ('image1', 'INVALID-TAG_NAME-BECAUSE-THIS-IS-WAY-WAY-TOO-LOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOONG', 400), + ('image1', + 'INVALID-TAG_NAME-BECAUSE-THIS-IS-WAY-WAY-TOO-LOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOONG', + 400), ('nonexistantimage', 'newtag', 404), ('image1', 'generatemanifestfail', None), ('image1', 'existing-tag', 201), ('image1', 'newtag', 201), ]) -def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_repo_tag_image, create_or_update_tag, generate_manifest, authd_client): +def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_repo_tag_image, + create_or_update_tag, generate_manifest, authd_client): params = {'repository': 'devtable/repo', 'tag': test_tag} request_body = {'image': test_image} if expected_status is None: @@ -81,16 +101,19 @@ def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_rep else: conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) + @pytest.mark.parametrize('test_manifest,test_tag,manifest_generated,expected_status', [ (None, 'newtag', True, 200), (None, 'generatemanifestfail', True, None), ('manifest1', 'newtag', False, 200), ]) -def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_status, get_repository, restore_tag_to_manifest, restore_tag_to_image, generate_manifest, authd_client): +def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_status, get_repository, + restore_tag_to_manifest, restore_tag_to_image, generate_manifest, + authd_client): params = {'repository': 'devtable/repo', 'tag': test_tag} request_body = {'image': 'image1'} if test_manifest is not None: - request_body['manifest_digest'] = test_manifest + request_body['manifest_digest'] = test_manifest if expected_status is None: with pytest.raises(Exception): conduct_api_call(authd_client, RestoreTag, 'post', params, request_body, expected_status)