From 2b134158f504a040d9b3da74a246bcc7e4869e02 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 28 Jan 2014 18:29:45 -0500 Subject: [PATCH] Add issue URLs to most errors. The corresponding issue pages will be checked into the public docs repo --- endpoints/index.py | 33 ++++++++++++++++++++----------- endpoints/registry.py | 46 ++++++++++++++++++++++++++----------------- util/http.py | 19 ++++++++++++++++-- 3 files changed, 66 insertions(+), 32 deletions(-) diff --git a/endpoints/index.py b/endpoints/index.py index b740bf998..94a5d4fef 100644 --- a/endpoints/index.py +++ b/endpoints/index.py @@ -15,6 +15,7 @@ from util.email import send_confirmation_email from auth.permissions import (ModifyRepositoryPermission, UserPermission, ReadRepositoryPermission, CreateRepositoryPermission) + from util.http import abort @@ -79,14 +80,14 @@ def create_user(): model.load_token_data(password) return make_response('Verified', 201) except model.InvalidTokenException: - return make_response('Invalid access token.', 400) + abort(400, 'Invalid access token.', issue='invalid-access-token') elif '+' in username: try: model.verify_robot(username, password) return make_response('Verified', 201) except model.InvalidRobotException: - return make_response('Invalid robot account or password.', 400) + abort(400, 'Invalid robot account or password.', issue='robot-login-failure') existing_user = model.get_user(username) if existing_user: @@ -94,7 +95,8 @@ def create_user(): if verified: return make_response('Verified', 201) else: - return make_response('Invalid password.', 400) + abort(400, 'Invalid password.', issue='login-failure') + else: # New user case new_user = model.create_user(username, password, user_data['email']) @@ -146,23 +148,30 @@ def update_user(username): @generate_headers(role='write') def create_repository(namespace, repository): image_descriptions = json.loads(request.data) - repo = model.get_repository(namespace, repository) if not repo and get_authenticated_user() is None: logger.debug('Attempt to create new repository without user auth.') - abort(401) + abort(401, + message='Cannot create a repository as a guest. Please login via "docker login" first.', + issue='no-login') elif repo: permission = ModifyRepositoryPermission(namespace, repository) if not permission.can(): - abort(403) + abort(403, + message='You do not have permission to modify repository %(namespace)s/%(repository)s', + issue='no-repo-write-permission', + namespace=namespace, repository=repository) else: permission = CreateRepositoryPermission(namespace) if not permission.can(): logger.info('Attempt to create a new repo with insufficient perms.') - abort(403) + abort(403, + message='You do not have permission to create repositories in namespace "%(namespace)s"', + issue='no-create-permission', + namespace=namespace) logger.debug('Creaing repository with owner: %s' % get_authenticated_user().username) @@ -215,7 +224,7 @@ def update_images(namespace, repository): repo = model.get_repository(namespace, repository) if not repo: # Make sure the repo actually exists. - abort(404) + abort(404, message='Unknown repository', issue='unknown-repo') image_with_checksums = json.loads(request.data) @@ -263,7 +272,7 @@ def get_repository_images(namespace, repository): # We can't rely on permissions to tell us if a repo exists anymore repo = model.get_repository(namespace, repository) if not repo: - abort(404) + abort(404, message='Unknown repository', issue='unknown-repo') all_images = [] for image in model.get_repository_images(namespace, repository): @@ -311,18 +320,18 @@ def get_repository_images(namespace, repository): @parse_repository_name @generate_headers(role='write') def delete_repository_images(namespace, repository): - return make_response('Not Implemented', 501) + abort(501, 'Not Implemented', issue='not-implemented') @index.route('/repositories//auth', methods=['PUT']) @parse_repository_name def put_repository_auth(namespace, repository): - return make_response('Not Implemented', 501) + abort(501, 'Not Implemented', issue='not-implemented') @index.route('/search', methods=['GET']) def get_search(): - return make_response('Not Implemented', 501) + abort(501, 'Not Implemented', issue='not-implemented') @index.route('/_ping') diff --git a/endpoints/registry.py b/endpoints/registry.py index 4d02dfd14..fc1575c1c 100644 --- a/endpoints/registry.py +++ b/endpoints/registry.py @@ -60,7 +60,7 @@ def require_completion(f): if store.exists(store.image_mark_path(namespace, repository, kwargs['image_id'])): abort(400, 'Image %(image_id)s is being uploaded, retry later', - image_id=kwargs['image_id']) + issue='upload-in-progress', image_id=kwargs['image_id']) return f(namespace, repository, *args, **kwargs) return wrapper @@ -105,7 +105,7 @@ def get_image_layer(namespace, repository, image_id, headers): try: return Response(store.stream_read(path), headers=headers) except IOError: - abort(404, 'Image %(image_id)s not found', image_id=image_id) + abort(404, 'Image %(image_id)s not found', issue='unknown-image', image_id=image_id) abort(403) @@ -122,16 +122,20 @@ def put_image_layer(namespace, repository, image_id): json_data = store.get_content(store.image_json_path(namespace, repository, image_id)) except IOError: - abort(404, 'Image not found') + abort(404, 'Image %(image_id)s not found', issue='unknown-image', image_id=image_id) + layer_path = store.image_layer_path(namespace, repository, image_id) mark_path = store.image_mark_path(namespace, repository, image_id) + if store.exists(layer_path) and not store.exists(mark_path): - abort(409, 'Image already exists') + abort(409, 'Image %(image_id)s already exists', issue='image-exists', image_id=image_id) + input_stream = request.stream if request.headers.get('transfer-encoding') == 'chunked': # Careful, might work only with WSGI servers supporting chunked # encoding (Gunicorn) input_stream = request.environ['wsgi.input'] + # compute checksums csums = [] sr = SocketReader(input_stream) @@ -141,6 +145,7 @@ def put_image_layer(namespace, repository, image_id): sr.add_handler(sum_hndlr) store.stream_write(layer_path, sr) csums.append('sha256:{0}'.format(h.hexdigest())) + try: image_size = tmp.tell() @@ -153,6 +158,7 @@ def put_image_layer(namespace, repository, image_id): except (IOError, checksums.TarError) as e: logger.debug('put_image_layer: Error when computing tarsum ' '{0}'.format(e)) + try: checksum = store.get_content(store.image_checksum_path(namespace, repository, @@ -162,10 +168,12 @@ def put_image_layer(namespace, repository, image_id): # Not removing the mark though, image is not downloadable yet. session['checksum'] = csums return make_response('true', 200) + # We check if the checksums provided matches one the one we computed if checksum not in csums: logger.warning('put_image_layer: Wrong checksum') - abort(400, 'Checksum mismatch; ignoring the layer') + abort(400, 'Checksum mismatch; ignoring the layer for image %(image_id)s', + issue='checksum-mismatch', image_id=image_id) # Checksum is ok, we remove the marker store.remove(mark_path) @@ -192,18 +200,19 @@ def put_image_checksum(namespace, repository, image_id): checksum = request.headers.get('X-Docker-Checksum') if not checksum: - abort(400, "Missing checksum for image %(image_id)s", image_id=image_id) + abort(400, "Missing checksum for image %(image_id)s", issue='missing-checksum', image_id=image_id) if not session.get('checksum'): abort(400, 'Checksum not found in Cookie for image %(imaage_id)s', - image_id=image_id) + issue='missing-checksum-cookie', image_id=image_id) if not store.exists(store.image_json_path(namespace, repository, image_id)): - abort(404, 'Image not found: %(image_id)s', image_id=image_id) + abort(404, 'Image not found: %(image_id)s', issue='unknown-image', image_id=image_id) mark_path = store.image_mark_path(namespace, repository, image_id) if not store.exists(mark_path): - abort(409, 'Cannot set checksum for image %(image_id)s', image_id=image_id) + abort(409, 'Cannot set checksum for image %(image_id)s', + issue='image-write-error', image_id=image_id) err = store_checksum(namespace, repository, image_id, checksum) if err: @@ -213,7 +222,8 @@ def put_image_checksum(namespace, repository, image_id): logger.debug('session checksums: %s' % session.get('checksum', [])) logger.debug('client supplied checksum: %s' % checksum) logger.debug('put_image_layer: Wrong checksum') - abort(400, 'Checksum mismatch for image: %(image_id)s', image_id=image_id) + abort(400, 'Checksum mismatch for image: %(image_id)s', + issue='checksum-mismatch', image_id=image_id) # Checksum is ok, we remove the marker store.remove(mark_path) @@ -245,7 +255,7 @@ def get_image_json(namespace, repository, image_id, headers): data = store.get_content(store.image_json_path(namespace, repository, image_id)) except IOError: - abort(404, 'Image %(image_id)%s not found', image_id=image_id) + abort(404, 'Image %(image_id)%s not found', issue='unknown-image', image_id=image_id) try: size = store.get_size(store.image_layer_path(namespace, repository, @@ -278,7 +288,7 @@ def get_image_ancestry(namespace, repository, image_id, headers): data = store.get_content(store.image_ancestry_path(namespace, repository, image_id)) except IOError: - abort(404, 'Image %(image_id)s not found', image_id=image_id) + abort(404, 'Image %(image_id)s not found', issue='unknown-image', image_id=image_id) response = make_response(json.dumps(json.loads(data)), 200) response.headers.extend(headers) @@ -324,11 +334,11 @@ def put_image_json(namespace, repository, image_id): pass if not data or not isinstance(data, dict): abort(400, 'Invalid JSON for image: %(image_id)s\nJSON: %(json)s', - image_id=image_id, json=request.data) + issue='invalid-request', image_id=image_id, json=request.data) if 'id' not in data: abort(400, 'Missing key `id` in JSON for image: %(image_id)s', - image_id=image_id) + issue='invalid-request', image_id=image_id) # Read the checksum checksum = request.headers.get('X-Docker-Checksum') @@ -336,25 +346,25 @@ def put_image_json(namespace, repository, image_id): # Storing the checksum is optional at this stage err = store_checksum(namespace, repository, image_id, checksum) if err: - abort(400, err) + abort(400, err, issue='write-error') else: # We cleanup any old checksum in case it's a retry after a fail store.remove(store.image_checksum_path(namespace, repository, image_id)) if image_id != data['id']: abort(400, 'JSON data contains invalid id for image: %(image_id)s', - image_id=image_id) + issue='invalid-request', image_id=image_id) parent_id = data.get('parent') if (parent_id and not store.exists(store.image_json_path(namespace, repository, parent_id))): abort(400, 'Image %(image_id)s depends on non existing parent image %(parent_id)s', - image_id=image_id, parent_id=parent_id) + issue='invalid-request', image_id=image_id, parent_id=parent_id) json_path = store.image_json_path(namespace, repository, image_id) mark_path = store.image_mark_path(namespace, repository, image_id) if store.exists(json_path) and not store.exists(mark_path): - abort(409, 'Image %(image_id)s already exists', image_id=image_id) + abort(409, 'Image %(image_id)s already exists', issue='image-exists', image_id=image_id) # If we reach that point, it means that this is a new image or a retry # on a failed push diff --git a/util/http.py b/util/http.py index a07b1e8a7..123029960 100644 --- a/util/http.py +++ b/util/http.py @@ -13,8 +13,9 @@ DEFAULT_MESSAGE[401] = 'Unauthorized' DEFAULT_MESSAGE[403] = 'Permission Denied' DEFAULT_MESSAGE[404] = 'Not Found' DEFAULT_MESSAGE[409] = 'Conflict' +DEFAULT_MESSAGE[501] = 'Not Implemented' -def abort(status_code, message=None, **kwargs): +def abort(status_code, message=None, issue=None, **kwargs): message = (str(message) % kwargs if message else DEFAULT_MESSAGE.get(status_code, '')) @@ -38,7 +39,21 @@ def abort(status_code, message=None, **kwargs): # Log the abort. logger.error('Error %s: %s; Arguments: %s' % (status_code, message, params)) - resp = jsonify({'error': message}) + + # Calculate the issue URL (if the issue ID was supplied). + issue_url = None + if issue: + issue_url = 'http://devtable.github.io/quaydocs/issues/%s.html' % (issue) + + # Create the final response data and message. + data = {} + if issue_url: + data['info_url'] = issue_url + message = message + '. For more information: ' + issue_url + + data['error'] = message + + resp = jsonify(data) resp.status_code = status_code # Report the abort to the user.