auth: Enable ticket cancellation

Previously we removed tickets without checking the ticket status. Now
removing a ticket will cancel the ticket and wait until the ticket is
unused.

If the ticket is still used after the remove timeout, the request will
fail with "409 Conflict". The caller need to poll the ticket status
until the number of connections reaches zero, and then remove the ticket
again. A simpler option is to retry removing the ticket until the
operation succeeds.

When a ticket is canceled, authorizing a ticket will fail. Add tests for
the Authorizer that previously was test only by the functional tests.

Ticket remove timeout can be modified using new config option:

    [control]
    remove_timeout = 60

Testing canceling upload using 4 connections and local storage show that
cancelling takes 30-650 milliseconds.

More tests are needed:

- functional tests for canceling tickets an monitoring number of
  connections during cancellation.
- functional tests for cancel timeout
- stress test for cancelling tickets

Change-Id: I0c9462e1fc927a8e4a572fefc62b4d6adeccb3ab
Bug-Url: https://bugzilla.redhat.com/1524184
Signed-off-by: Nir Soffer <nsoffer@redhat.com>

daemon/data/daemon.conf.sample

@@ -131,6 +131,13 @@
 # dual stack system.
 #   prefer_ipv4 = true
 
+# Number of seconds to wait when removing a ticket. If ticket cannot be removed
+# within this timeout, the request will fail with "409 Conflict", and the user
+# need to retry the request again. A ticket can be removed only when the number
+# of connections using the ticket is zero.
+# The default value:
+#   remove_timeout = 60
+
 [profile]
 # Filename for storing profile data. Profiling requires the "yappi"
 # package. Version 0.93 is recommended for best performance.

daemon/ovirt_imageio/_internal/auth.py

@@ -352,7 +352,8 @@ def _validate(key, value, type):
 
 class Authorizer:
 
-    def __init__(self):
+    def __init__(self, config):
+        self._config = config
         self._tickets = {}
 
     def add(self, ticket_dict):
@@ -365,7 +366,18 @@ def add(self, ticket_dict):
         self._tickets[ticket.uuid] = ticket
 
     def remove(self, ticket_id):
-        del self._tickets[ticket_id]
+        try:
+            ticket = self._tickets[ticket_id]
+        except KeyError:
+            log.debug("Ticket %s does not exist", ticket_id)
+            return
+
+        # Cancel the ticket and wait until the ticket is unused. Will raise
+        # errors.TicketCancelTimeout if the ticket could not be canceled within
+        # the timeout.
+        if ticket.cancel(self._config.control.remove_timeout):
+            # Ticket is unused now, so it is safe to remove it.
+            del self._tickets[ticket_id]
 
     def clear(self):
         self._tickets.clear()
@@ -388,6 +400,10 @@ def authorize(self, ticket_id, op):
             raise errors.AuthorizationError(
                 "No such ticket {}".format(ticket_id))
 
+        if ticket.canceled:
+            raise errors.AuthorizationError(
+                "Ticket {} was canceled".format(ticket_id))
+
         if ticket.expires <= util.monotonic_time():
             raise errors.AuthorizationError(
                 "Ticket {} expired".format(ticket_id))

daemon/ovirt_imageio/_internal/config.py

@@ -126,6 +126,12 @@ class control:
     # dual stack system.
     prefer_ipv4 = True
 
+    # Number of seconds to wait when removing a ticket. If ticket cannot be
+    # removed within this timeout, the request will fail with "409 Conflict",
+    # and the user need to retry the request again. A ticket can be removed
+    # only when the number of connections using the ticket is zero.
+    remove_timeout = 60
+
 
 class profile:
 

daemon/ovirt_imageio/_internal/server.py

@@ -106,7 +106,7 @@ class Server:
     def __init__(self, config):
         self.config = config
         self.running = False
-        self.auth = auth.Authorizer()
+        self.auth = auth.Authorizer(config)
         self.remote_service = services.RemoteService(self.config, self.auth)
         self.local_service = None
         if config.local.enable:

daemon/ovirt_imageio/_internal/tickets.py

@@ -98,8 +98,11 @@ def delete(self, req, resp, ticket_id):
         if ticket_id:
             try:
                 self.auth.remove(ticket_id)
-            except KeyError:
-                log.debug("Ticket %s does not exists", ticket_id)
+            except errors.TicketCancelTimeout as e:
+                # The ticket is still used by some connection so we cannot
+                # remove it. The caller can retry the call again when the
+                # number connections reach zero.
+                raise http.Error(http.CONFLICT, str(e))
         else:
             self.auth.clear()
 

daemon/test/auth_test.py

@@ -14,9 +14,10 @@
 
 from six.moves import xrange
 
+from ovirt_imageio._internal import config
 from ovirt_imageio._internal import errors
 from ovirt_imageio._internal import util
-from ovirt_imageio._internal.auth import Ticket
+from ovirt_imageio._internal.auth import Ticket, Authorizer
 
 from test import testutil
 
@@ -544,3 +545,163 @@ def close(self):
 
     info = ticket.info()
     assert info["connections"] == 0
+
+
+def test_authorizer_add():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=["read"])
+    auth.add(ticket_info)
+
+    ticket = auth.get(ticket_info["uuid"])
+    assert ticket.uuid == ticket_info["uuid"]
+
+
+def test_authorizer_remove_unused():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=["read"])
+    auth.add(ticket_info)
+
+    # Ticket is unused so it will be removed.
+    auth.remove(ticket_info["uuid"])
+    with pytest.raises(KeyError):
+        auth.get(ticket_info["uuid"])
+
+
+def test_authorizer_remove_timeout():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=["read"])
+    auth.add(ticket_info)
+
+    ticket = auth.get(ticket_info["uuid"])
+    ticket.add_context(1, Context())
+    assert ticket.info()["connections"] == 1
+
+    # Use short timeout to keep the tests fast.
+    cfg.control.remove_timeout = 0.001
+
+    # Ticket cannot be removed since it is used by connection 1.
+    with pytest.raises(errors.TicketCancelTimeout):
+        auth.remove(ticket.uuid)
+
+    # Ticket was not removed.
+    assert auth.get(ticket.uuid) is ticket
+
+    # The connection was closed, the ticket can be removed now.
+    ticket.remove_context(1)
+    assert ticket.info()["connections"] == 0
+
+    auth.remove(ticket.uuid)
+
+    # Ticket was removed.
+    with pytest.raises(KeyError):
+        auth.get(ticket.uuid)
+
+
+def test_authorizer_remove_async():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=["read"])
+    auth.add(ticket_info)
+
+    ticket = auth.get(ticket_info["uuid"])
+    ticket.add_context(1, Context())
+    assert ticket.info()["connections"] == 1
+
+    # Disable the timeout, so removing a ticket cancel the ticket without
+    # waiting, and requiring polling the ticket status.
+    cfg.control.remove_timeout = 0
+
+    # Ticket is canceled, but not removed.
+    auth.remove(ticket.uuid)
+    assert ticket.canceled
+    assert ticket.info()["connections"] == 1
+
+    # Ticket was not removed.
+    assert auth.get(ticket.uuid) is ticket
+
+    # The connection was closed, the ticket can be removed now.
+    ticket.remove_context(1)
+    assert ticket.info()["connections"] == 0
+
+    auth.remove(ticket.uuid)
+
+    # Ticket was removed.
+    with pytest.raises(KeyError):
+        auth.get(ticket.uuid)
+
+
+def test_authorizer_remove_mising():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    # Removing missing ticket does not raise.
+    auth.remove("no-such-ticket")
+
+
+def test_authorize_read():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=["read"])
+    auth.add(ticket_info)
+
+    ticket = auth.get(ticket_info["uuid"])
+    assert auth.authorize(ticket.uuid, "read") == ticket
+
+    with pytest.raises(errors.AuthorizationError):
+        auth.authorize(ticket.uuid, "write")
+
+
+def test_authorize_write():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=["write"])
+    auth.add(ticket_info)
+
+    ticket = auth.get(ticket_info["uuid"])
+    assert auth.authorize(ticket.uuid, "write") == ticket
+
+    # "write" implies also "read".
+    assert auth.authorize(ticket.uuid, "read") == ticket
+
+
+def test_authorizer_no_ticket():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    with pytest.raises(errors.AuthorizationError):
+        auth.authorize("no-such-ticket", "read")
+
+
+@pytest.mark.parametrize("ops,allowed", [
+    (["read"], ["read"]),
+    (["write"], ["read", "write"]),
+])
+def test_authorizer_canceled(ops, allowed):
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=ops)
+    auth.add(ticket_info)
+    ticket = auth.get(ticket_info["uuid"])
+
+    # Cancelling the ticket disables any operation.
+    ticket.cancel()
+
+    for op in allowed:
+        with pytest.raises(errors.AuthorizationError):
+            auth.authorize(ticket.uuid, op)
+
+
+def test_authorizer_expired():
+    cfg = config.load(["test/conf.d/daemon.conf"])
+    auth = Authorizer(cfg)
+    ticket_info = testutil.create_ticket(ops=["write"])
+    auth.add(ticket_info)
+    ticket = auth.get(ticket_info["uuid"])
+
+    # Extending with zero timeout expire the ticket.
+    ticket.extend(0)
+
+    for op in ("read", "write"):
+        with pytest.raises(errors.AuthorizationError):
+            auth.authorize(ticket.uuid, op)

daemon/test/services_test.py

@@ -15,17 +15,17 @@
 
 @pytest.mark.parametrize("port", [-1, 65536])
 def test_invalid_remote_port(port):
-    authorizer = auth.Authorizer()
     cfg = config.load(["test/conf/daemon.conf"])
+    authorizer = auth.Authorizer(cfg)
     cfg.remote.port = port
     with pytest.raises(errors.InvalidConfig):
         services.RemoteService(cfg, authorizer)
 
 
 @pytest.mark.parametrize("port", [-1, 65536])
 def test_invalid_control_port(port):
-    authorizer = auth.Authorizer()
     cfg = config.load(["test/conf/proxy.conf"])
+    authorizer = auth.Authorizer(cfg)
     cfg.control.port = port
     with pytest.raises(errors.InvalidConfig):
         services.ControlService(cfg, authorizer)

daemon/test/ssl_test.py

@@ -28,7 +28,7 @@ def on_centos(version=""):
 def remote_service(config_file):
     path = os.path.join("test/conf", config_file)
     cfg = config.load([path])
-    authorizer = auth.Authorizer()
+    authorizer = auth.Authorizer(cfg)
     s = services.RemoteService(cfg, authorizer)
     s.start()
     try: