Merge ~andersson123/autopkgtest-cloud:exit_code_suite_autopkgtest-web into autopkgtest-cloud:master

[Tim Andersson]

[Tim Andersson (andersson123)]

Tested in staging and works!

[Brian Murray (brian-murray)]

This generally looks great but I have a couple of questions in line.

review: Needs Fixing

[Paride Legovini (paride)]

review: Needs Fixing (cursory)

[Tim Andersson (andersson123)]

Amended. Please take another look.

[Brian Murray (brian-murray)]

I noticed an issue which needs addressing (commented in-line) which would end up "fixing" bug 2028801.

review: Needs Fixing

[Tim Andersson (andersson123)]

I'm going to test this in staging now.

[Brian Murray (brian-murray)]

Has this been tested in staging now?

review: Needs Information

[Tim Andersson (andersson123)]

I can't remember how far I got with this, so I'll re-test now

[Brian Murray (brian-murray)]

This seems to have failed some CI checks:

:: ************* Module request.submit
:: charms/focal/autopkgtest-web/webcontrol/request/submit.py:618:0: C0301: Line too long (162/100) (line-too-long)
:: charms/focal/autopkgtest-web/webcontrol/request/submit.py:621:0: C0301: Line too long (130/100) (line-too-long)

I also have one in line comment.

While we just talked about utilizing the unit testing in autopkgtest-web, I think waiting to merge this until we have unit testing for the changes would be appropriate.

[Tim Andersson (andersson123)]

Totally understandable, let's do that :)

[Brian Murray (brian-murray)]

I see a conflict in this branch due to the changes merge in https://git.launchpad.net/autopkgtest-cloud/commit/?id=4a98a49a612f804f69ae54bafaf9e4b9de861fc9.

[Tim Andersson (andersson123)]

Amended

[Tim Andersson (andersson123)]

I just tested this again and I think it's good to go.

[Tim Andersson (andersson123)]

We're waiting for the unit testing MP to be merged before merging this. Once it's merged we'll add a unit test for this and then look to merge this one.

[Paride Legovini (paride)]

We should definitely add tests for new features/bugfixes, but given that the unit test suite needs work, maybe we can land this and then concentrate on fixing that.

I'd prefer input validation commit to be in a separate MP. That should be easy to do.

I have an inline comment on the input validation comment, but overall this LGTM.

review: Needs Fixing

[Tim Andersson (andersson123)]

separate mp for input validation:
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/449624

[Paride Legovini (paride)]

This LGTM, but I'd like CI to pass, so let's not merge right away. I want to figure out why we're getting flaky CI now.

review: Approve

diff --git a/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py b/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py
new file mode 100644
index 0000000..9f4f745
--- /dev/null
+++ b/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py
@@ -0,0 +1,80 @@
+"""
+Http exceptions for autopkgtest-web
+"""
+
+
+class WebControlException(Exception):
+    def __init__(self, message, exit_code):
+        super().__init__(message)
+        self._code = exit_code
+
+    def exit_code(self):
+        return self._code
+
+
+class RequestInQueue(WebControlException):
+    def __init__(self, release, package, arch, triggers):
+        super().__init__(
+            "Test already queued:\nrelease: %s\npkg: %s\narch: %s\ntriggers: %s"
+            % (release, package, arch, ",".join(triggers)),
+            403,
+        )
+
+
+class RequestRunning(WebControlException):
+    def __init__(self, release, package, arch, triggers):
+        super().__init__(
+            "Test already running:\nrelease: %s\npkg: %s\narch: %s\ntriggers: %s"
+            % (release, package, arch, ",".join(triggers)),
+            403,
+        )
+
+
+class BadRequest(WebControlException):
+    def __init__(self, msg=None):
+        if msg is None:
+            super().__init__(
+                "Bad request - unacceptable passed variables", 400
+            )
+        else:
+            super().__init__(msg, 400)
+
+
+class Unauthorized(WebControlException):
+    def __init__(self):
+        super().__init__("Authorization failure", 401)
+
+
+class ForbiddenRequest(WebControlException):
+    def __init__(self, package, trigger):
+        super().__init__(
+            (
+                "You are not allowed to upload %s or %s to Ubuntu, "
+                + "thus you are not allowed to use this service."
+            )
+            % (package, trigger),
+            403,
+        )
+
+
+class NotFound(WebControlException):
+    def __init__(self, element_name, element, msg=None):
+        if msg is None:
+            super().__init__(
+                "%s %s not found" % (element_name, element),
+                404,
+            )
+        else:
+            super().__init__(
+                "%s %s %s" % (element_name, element, msg),
+                404,
+            )
+
+
+class TooManyRequests(WebControlException):
+    def __init__(self, requester):
+        super().__init__(
+            "You, %s, have requested too many tests. Please try again later."
+            % requester,
+            429,
+        )
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/app.py b/charms/focal/autopkgtest-web/webcontrol/request/app.py
index 758f05e..73fb0dc 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/app.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/app.py
@@ -8,8 +8,9 @@ from html import escape as _escape
 
 from flask import Flask, redirect, request, session
 from flask_openid import OpenID
+from helpers.exceptions import WebControlException
 from helpers.utils import setup_key
-from request.submit import DuplicateRequestException, Submit
+from request.submit import Submit
 from werkzeug.middleware.proxy_fix import ProxyFix
 
 # map multiple GET vars to AMQP JSON request parameter list
@@ -184,12 +185,10 @@ def index_root():
                     github_params["number"],
                 )
             s.validate_git_request(**params)
-        except (ValueError, TypeError) as e:
-            return invalid(e)
+        except WebControlException as e:
+            return invalid(e, e.exit_code())
         except KeyError as e:
             return invalid("Missing field in JSON data: %s" % e)
-        except DuplicateRequestException as e:
-            return invalid(e, 403)
 
         s.send_amqp_request(context="upstream", **params)
         # write status file for pending test
@@ -237,10 +236,8 @@ def index_root():
         s = Submit()
         try:
             s.validate_distro_request(**params)
-        except (ValueError, TypeError) as e:
-            return invalid(e)
-        except DuplicateRequestException as e:
-            return invalid(e, 403)
+        except WebControlException as e:
+            return invalid(e, e.exit_code())
 
         if params.get("delete"):
             del params["delete"]
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/submit.py b/charms/focal/autopkgtest-web/webcontrol/request/submit.py
index f0eca73..79018ea 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/submit.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/submit.py
@@ -17,11 +17,13 @@ from urllib.error import HTTPError
 
 import amqplib.client_0_8 as amqp
 from distro_info import UbuntuDistroInfo
-
-
-class DuplicateRequestException(Exception):
-    pass
-
+from helpers.exceptions import (
+    BadRequest,
+    ForbiddenRequest,
+    NotFound,
+    RequestInQueue,
+    RequestRunning,
+)
 
 # Launchpad REST API base
 LP = "https://api.launchpad.net/1.0/"
@@ -94,11 +96,7 @@ class Submit:
         Raise ValueError with error messsage if the request is invalid,
         otherwise return.
         """
-        not_needed, msg = self.is_request_queued_or_running(
-            release, arch, package, triggers, kwargs
-        )
-        if not_needed:
-            raise DuplicateRequestException(msg)
+        self.is_request_queued_or_running(release, arch, package, triggers)
 
         can_upload_any_trigger = False
 
@@ -125,12 +123,12 @@ class Submit:
             raise ValueError("Invalid argument %s" % list(kwargs)[0])
 
         if release not in self.releases:
-            raise ValueError("Unknown release " + release)
+            raise NotFound("release", release)
         if arch not in self.architectures:
-            raise ValueError("Unknown architecture " + arch)
+            raise NotFound("arch", arch)
         for ppa in ppas:
             if not self.is_valid_ppa(ppa):
-                raise ValueError("Unknown PPA " + ppa)
+                raise NotFound("ppa", ppa)
         # allow kernel tests for EOL vivid
         skip_result_check = (
             release == "vivid" and triggers and triggers[0].startswith("linux")
@@ -138,32 +136,33 @@ class Submit:
         if not self.is_valid_package_with_results(
             None if (ppas or skip_result_check) else release, arch, package
         ):
-            raise ValueError(
-                "Package %s does not have any test results for %s on %s"
-                % (package, release, arch)
+            raise NotFound(
+                "package", package, "does not have any test results"
             )
 
         if "migration-reference/0" in triggers:
             if len(triggers) != 1:
-                raise ValueError(
+                raise BadRequest(
                     "Cannot use additional triggers with migration-reference/0"
                 )
             if ppas:
-                raise ValueError("Cannot use PPAs with migration-reference/0")
+                raise BadRequest("Cannot use PPAs with migration-reference/0")
             if "all-proposed" in kwargs:
-                raise ValueError(
+                raise BadRequest(
                     'Cannot use "all-proposed" with migration-reference/0'
                 )
         for trigger in triggers:
             try:
                 trigsrc, trigver = trigger.split("/")
-            except ValueError as exc:
-                raise ValueError(
+            except ValueError as e:
+                raise BadRequest(
                     "Malformed trigger, must be srcpackage/version"
-                ) from exc
+                ) from e
             # Debian Policy 5.6.1 and 5.6.12
             if not NAME.match(trigsrc) or not VERSION.match(trigver):
-                raise ValueError("Malformed trigger")
+                raise BadRequest(
+                    "Malformed trigger: %s\nversion: %s" % (trigsrc, trigver)
+                )
 
             # Special snowflake
             if trigger in ("qemu-efi-noacpi/0", "migration-reference/0"):
@@ -173,7 +172,7 @@ class Submit:
                 if not self.is_valid_package_version(
                     release, trigsrc, trigver, ppas and ppas[-1] or None
                 ):
-                    raise ValueError(
+                    raise BadRequest(
                         "%s is not published in PPA %s %s"
                         % (trigger, ppas[-1], release)
                     )
@@ -188,7 +187,7 @@ class Submit:
                     release, trigsrc, trigver
                 )
                 if not trigsrc_component:
-                    raise ValueError(
+                    raise BadRequest(
                         "%s is not published in %s" % (trigger, release)
                     )
 
@@ -205,7 +204,7 @@ class Submit:
                 release, package, None
             )
             if not package_component:
-                raise ValueError(
+                raise BadRequest(
                     "%s is not published in %s" % (package, release)
                 )
 
@@ -216,11 +215,7 @@ class Submit:
             and requester not in ALLOWED_USERS_PERPACKAGE.get(package, [])
             and not self.in_allowed_team(requester, package)
         ):
-            raise ValueError(
-                "You are not allowed to upload %s or %s to "
-                "Ubuntu, thus you are not allowed to use this "
-                "service." % (package, trigsrc)
-            )
+            raise ForbiddenRequest(package, ",".join(triggers))
 
     # pylint: disable=dangerous-default-value
     def validate_git_request(
@@ -246,40 +241,36 @@ class Submit:
                 else:
                     triggers.append(env_var.split("=")[1])
 
-        not_needed, msg = self.is_request_queued_or_running(
-            release, arch, package, triggers, kwargs
-        )
-        if not_needed:
-            raise DuplicateRequestException(msg)
+        self.is_request_queued_or_running(release, arch, package, triggers)
 
         if release not in self.releases:
-            raise ValueError("Unknown release " + release)
+            raise NotFound("release", release)
         if arch not in self.architectures:
-            raise ValueError("Unknown architecture " + arch)
+            raise NotFound("arch", arch)
         if not NAME.match(package):
-            raise ValueError("Malformed package")
+            raise NotFound("package", package)
         if not ppas:
-            raise ValueError(
+            raise BadRequest(
                 "Must specify at least one PPA (to associate results with)"
             )
         for ppa in ppas:
             if not self.is_valid_ppa(ppa):
-                raise ValueError("Unknown PPA " + ppa)
+                raise NotFound("ppa", ppa)
         for e in env:
             if not ENV.match(e):
-                raise ValueError(
+                raise BadRequest(
                     'Invalid environment variable format "%s"' % e
                 )
         # we should only be called in this mode
         assert "build-git" in kwargs
         if not GIT.match(kwargs["build-git"]):
-            raise ValueError("Malformed build-git")
+            raise BadRequest("Malformed build-git")
         if "testname" in kwargs and not NAME.match(kwargs["testname"]):
-            raise ValueError("Malformed testname")
+            raise BadRequest("Malformed testname")
 
         unsupported_keys = set(kwargs.keys()) - {"build-git", "testname"}
         if unsupported_keys:
-            raise ValueError(
+            raise BadRequest(
                 "Unsupported arguments: %s" % " ".join(unsupported_keys)
             )
 
@@ -634,20 +625,13 @@ class Submit:
         if self.is_test_running(
             req_series, req_arch, req_package, req_triggers, kwargs
         ):
-            o_msg = "Requested test is already running."
-            o_msg += "\nRelease: " + req_series
-            o_msg += "\nArchitecture: " + req_arch
-            o_msg += "\nPackage: " + req_package
-            o_msg += "\nTriggers: " + ",".join(req_triggers)
-            return True, o_msg
+            raise RequestRunning(
+                req_series, req_package, req_arch, req_triggers
+            )
 
         if self.is_test_in_queue(
             req_series, req_arch, req_package, req_triggers, kwargs
         ):
-            o_msg = "Requested test is already queued."
-            o_msg += "\nRelease: " + req_series
-            o_msg += "\nArchitecture: " + req_arch
-            o_msg += "\nPackage: " + req_package
-            o_msg += "\nTriggers: " + ",".join(req_triggers)
-            return True, o_msg
-        return False, ""
+            raise RequestInQueue(
+                req_series, req_package, req_arch, req_triggers
+            )