From d32f477e0b37a78379a2e86f141163881d05decf Mon Sep 17 00:00:00 2001 From: "Brian C. Lane" Date: Wed, 27 Feb 2019 17:00:19 -0800 Subject: [PATCH] lorax-composer: Return UnknownBlueprint errors when using deleted blueprints Reading a blueprint wasn't checking to see if it had been deleted so it was returning the most recent commit before it had been deleted. This allowed things like starting a compose with a blueprint that technically doesn't exist. One exception to this is the /changes/ route, it must be available so that you can use the commit hash to undo a delete. This also adds tests for the various operations. Resolves: rhbz#1682113 --- src/pylorax/api/recipes.py | 24 ++++++++++ src/pylorax/api/v0.py | 45 ++++++++++++++---- tests/pylorax/test_server.py | 88 ++++++++++++++++++++++++++++++++++-- 3 files changed, 143 insertions(+), 14 deletions(-) diff --git a/src/pylorax/api/recipes.py b/src/pylorax/api/recipes.py index 617044f5..3dc76f0e 100644 --- a/src/pylorax/api/recipes.py +++ b/src/pylorax/api/recipes.py @@ -408,6 +408,9 @@ def read_recipe_commit(repo, branch, recipe_name, commit=None): If no commit is passed the master:filename is returned, otherwise it will be commit:filename """ + if not repo_file_exists(repo, branch, recipe_filename(recipe_name)): + raise RecipeFileError("Unknown blueprint") + (_, recipe_toml) = read_commit(repo, branch, recipe_filename(recipe_name), commit) return recipe_from_toml(recipe_toml) @@ -631,6 +634,9 @@ def tag_recipe_commit(repo, branch, recipe_name): Uses tag_file_commit() """ + if not repo_file_exists(repo, branch, recipe_filename(recipe_name)): + raise RecipeFileError("Unknown blueprint") + return tag_file_commit(repo, branch, recipe_filename(recipe_name)) def tag_file_commit(repo, branch, filename): @@ -920,3 +926,21 @@ def recipe_diff(old_recipe, new_recipe): diffs.extend(diff_items("Group", old_recipe["groups"], new_recipe["groups"])) return diffs + +def repo_file_exists(repo, branch, filename): + """Return True if the filename exists on the branch + + :param repo: Open repository + :type repo: Git.Repository + :param branch: Branch name + :type branch: str + :param filename: Filename to check + :type filename: str + :returns: True if the filename exists on the HEAD of the branch, False otherwise. + :rtype: bool + """ + commit = head_commit(repo, branch).get_id().to_string() + commit_id = Git.OId.new_from_string(commit) + commit_obj = repo.lookup(commit_id, Git.Commit) + tree = commit_obj.get_tree() + return tree.get_by_name(filename) is not None diff --git a/src/pylorax/api/v0.py b/src/pylorax/api/v0.py index d9e52844..081ca14f 100644 --- a/src/pylorax/api/v0.py +++ b/src/pylorax/api/v0.py @@ -1007,7 +1007,7 @@ from pylorax.api.queue import queue_status, build_status, uuid_delete, uuid_stat from pylorax.api.queue import uuid_tar, uuid_image, uuid_cancel, uuid_log from pylorax.api.recipes import RecipeError, list_branch_files, read_recipe_commit, recipe_filename, list_commits from pylorax.api.recipes import recipe_from_dict, recipe_from_toml, commit_recipe, delete_recipe, revert_recipe -from pylorax.api.recipes import tag_recipe_commit, recipe_diff +from pylorax.api.recipes import tag_recipe_commit, recipe_diff, RecipeFileError from pylorax.api.regexes import VALID_API_STRING from pylorax.api.workspace import workspace_read, workspace_write, workspace_delete @@ -1028,12 +1028,21 @@ def take_limits(iterable, offset, limit): return iterable[offset:][:limit] def blueprint_exists(api, branch, blueprint_name): + """Return True if the blueprint exists + + :param api: flask object + :type api: Flask + :param branch: Branch name + :type branch: str + :param recipe_name: Recipe name to read + :type recipe_name: str + """ try: with api.config["GITLOCK"].lock: read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name) return True - except RecipeError: + except (RecipeError, RecipeFileError): return False def v0_api(api): @@ -1092,6 +1101,10 @@ def v0_api(api): try: with api.config["GITLOCK"].lock: git_blueprint = read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name) + except RecipeFileError as e: + # Adding an exception would be redundant, skip it + git_blueprint = None + log.error("(v0_blueprints_info) %s", str(e)) except Exception as e: git_blueprint = None exceptions.append(str(e)) @@ -1150,20 +1163,19 @@ def v0_api(api): errors = [] for blueprint_name in [n.strip() for n in blueprint_names.split(",")]: filename = recipe_filename(blueprint_name) - - if not blueprint_exists(api, branch, blueprint_name): - errors.append({"id": UNKNOWN_BLUEPRINT, "msg": "Unknown blueprint name: %s" % blueprint_name}) - continue - try: with api.config["GITLOCK"].lock: commits = list_commits(api.config["GITLOCK"].repo, branch, filename) - limited_commits = take_limits(list_commits(api.config["GITLOCK"].repo, branch, filename), offset, limit) except Exception as e: errors.append({"id": BLUEPRINTS_ERROR, "msg": "%s: %s" % (blueprint_name, str(e))}) log.error("(v0_blueprints_changes) %s", str(e)) else: - blueprints.append({"name":blueprint_name, "changes":limited_commits, "total":len(commits)}) + if commits: + limited_commits = take_limits(commits, offset, limit) + blueprints.append({"name":blueprint_name, "changes":limited_commits, "total":len(commits)}) + else: + # no commits means there is no blueprint in the branch + errors.append({"id": UNKNOWN_BLUEPRINT, "msg": "%s" % blueprint_name}) blueprints = sorted(blueprints, key=lambda r: r["name"].lower()) @@ -1312,6 +1324,9 @@ def v0_api(api): try: with api.config["GITLOCK"].lock: tag_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name) + except RecipeFileError as e: + log.error("(v0_blueprints_tag) %s", str(e)) + return jsonify(status=False, errors=[{"id": UNKNOWN_BLUEPRINT, "msg": str(e)}]), 400 except Exception as e: log.error("(v0_blueprints_tag) %s", str(e)) return jsonify(status=False, errors=[{"id": BLUEPRINTS_ERROR, "msg": str(e)}]), 400 @@ -1336,6 +1351,9 @@ def v0_api(api): if VALID_API_STRING.match(branch) is None: return jsonify(status=False, errors=[{"id": INVALID_CHARS, "msg": "Invalid characters in branch argument"}]), 400 + if not blueprint_exists(api, branch, blueprint_name): + return jsonify(status=False, errors=[{"id": UNKNOWN_BLUEPRINT, "msg": "Unknown blueprint name: %s" % blueprint_name}]) + try: if from_commit == "NEWEST": with api.config["GITLOCK"].lock: @@ -1402,6 +1420,9 @@ def v0_api(api): try: with api.config["GITLOCK"].lock: blueprint = read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name) + except RecipeFileError as e: + # adding an error here would be redundant, skip it + log.error("(v0_blueprints_freeze) %s", str(e)) except Exception as e: errors.append({"id": BLUEPRINTS_ERROR, "msg": "%s: %s" % (blueprint_name, str(e))}) log.error("(v0_blueprints_freeze) %s", str(e)) @@ -1462,6 +1483,9 @@ def v0_api(api): try: with api.config["GITLOCK"].lock: blueprint = read_recipe_commit(api.config["GITLOCK"].repo, branch, blueprint_name) + except RecipeFileError as e: + # adding an error here would be redundant, skip it + log.error("(v0_blueprints_depsolve) %s", str(e)) except Exception as e: errors.append({"id": BLUEPRINTS_ERROR, "msg": "%s: %s" % (blueprint_name, str(e))}) log.error("(v0_blueprints_depsolve) %s", str(e)) @@ -1794,6 +1818,9 @@ def v0_api(api): if VALID_API_STRING.match(blueprint_name) is None: errors.append({"id": INVALID_CHARS, "msg": "Invalid characters in API path"}) + if not blueprint_exists(api, branch, blueprint_name): + errors.append({"id": UNKNOWN_BLUEPRINT, "msg": "Unknown blueprint name: %s" % blueprint_name}) + if errors: return jsonify(status=False, errors=errors), 400 diff --git a/tests/pylorax/test_server.py b/tests/pylorax/test_server.py index 045c38a9..79f6a15a 100644 --- a/tests/pylorax/test_server.py +++ b/tests/pylorax/test_server.py @@ -206,13 +206,11 @@ class ServerTestCase(unittest.TestCase): def test_03_blueprints_info_none(self): """Test the /api/v0/blueprints/info route with an unknown blueprint""" - info_dict_3 = {"changes":[], - "errors":[{"id": UNKNOWN_BLUEPRINT, "msg": "missing-blueprint: No commits for missing-blueprint.toml on the master branch."}], - "blueprints":[] - } resp = self.server.get("/api/v0/blueprints/info/missing-blueprint") data = json.loads(resp.data) - self.assertEqual(data, info_dict_3) + self.assertNotEqual(data, None) + self.assertTrue(len(data["errors"]) > 0) + self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint") def test_04_blueprints_changes(self): """Test the /api/v0/blueprints/changes route""" @@ -403,6 +401,21 @@ class ServerTestCase(unittest.TestCase): # Make sure the workspace file is gone self.assertEqual(os.path.exists(joinpaths(self.repo_dir, "git/workspace/master/example-glusterfs.toml")), False) + # This has to run after the above test + def test_10_blueprints_delete_2(self): + """Test running a compose with the deleted blueprint""" + # Trying to start a compose with a deleted blueprint should fail + test_compose = {"blueprint_name": "example-glusterfs", + "compose_type": "tar", + "branch": "master"} + + resp = self.server.post("/api/v0/compose?test=2", + data=json.dumps(test_compose), + content_type="application/json") + data = json.loads(resp.data) + self.assertNotEqual(data, None) + self.assertEqual(data["status"], False, "Compose of deleted blueprint did not fail: %s" % data) + def test_11_blueprints_undo(self): """Test POST /api/v0/blueprints/undo//""" resp = self.server.get("/api/v0/blueprints/changes/example-glusterfs") @@ -1399,6 +1412,71 @@ class ServerTestCase(unittest.TestCase): resp = self.server.get("/api/v0/compose/log/" + UTF8_TEST_STRING) self.assertInputError(resp) + # A series of tests for dealing with deleted blueprints + def test_deleted_bp_00_setup(self): + """Setup a deleted blueprint for use in the tests""" + # Start by creating a new blueprint for this series of tests and then + # deleting it. + test_blueprint = {"description": "A blueprint that has been deleted", + "name":"deleted-blueprint", + "version": "0.0.1", + "modules":[{"name":"glusterfs", "version":"5.*"}, + {"name":"glusterfs-cli", "version":"5.*"}], + "packages":[{"name":"samba", "version":"4.*.*"}, + {"name":"tmux", "version":"2.8"}], + "groups": []} + + resp = self.server.post("/api/v0/blueprints/new", + data=json.dumps(test_blueprint), + content_type="application/json") + data = json.loads(resp.data) + self.assertEqual(data, {"status":True}) + + resp = self.server.delete("/api/v0/blueprints/delete/deleted-blueprint") + data = json.loads(resp.data) + self.assertEqual(data, {"status":True}) + + def test_deleted_bp_01_show(self): + """Test blueprint show with deleted blueprint""" + resp = self.server.get("/api/v0/blueprints/info/deleted-blueprint") + data = json.loads(resp.data) + self.assertNotEqual(data, None) + self.assertTrue(len(data["errors"]) > 0) + self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint") + + def test_deleted_bp_02_depsolve(self): + """Test blueprint depsolve with deleted blueprint""" + resp = self.server.get("/api/v0/blueprints/depsolve/deleted-blueprint") + data = json.loads(resp.data) + self.assertNotEqual(data, None) + self.assertTrue(len(data["errors"]) > 0) + self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint") + + def test_deleted_bp_03_diff(self): + """Test blueprint diff with deleted blueprint""" + resp = self.server.get("/api/v0/blueprints/diff/deleted-blueprint/NEWEST/WORKSPACE") + data = json.loads(resp.data) + self.assertNotEqual(data, None) + self.assertTrue(len(data["errors"]) > 0) + self.assertEqual(data["status"], False) + self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint") + + def test_deleted_bp_04_freeze(self): + """Test blueprint freeze with deleted blueprint""" + resp = self.server.get("/api/v0/blueprints/freeze/deleted-blueprint") + data = json.loads(resp.data) + self.assertNotEqual(data, None) + self.assertTrue(len(data["errors"]) > 0) + self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint") + + def test_deleted_bp_05_tag(self): + """Test blueprint tag with deleted blueprint""" + resp = self.server.post("/api/v0/blueprints/tag/deleted-blueprint") + data = json.loads(resp.data) + self.assertNotEqual(data, None) + self.assertTrue(len(data["errors"]) > 0) + self.assertEqual(data["errors"][0]["id"], "UnknownBlueprint") + @contextmanager def in_tempdir(prefix='tmp'): """Execute a block of code with chdir in a temporary location"""