Merge lp:~allenap/pyjuju/maas-anon-urls-2 into lp:pyjuju

[Gavin Panella]

When requesting a file from MAAS using the new-style URLs, it's necessary to differentiate between a 404 response that means "this file does not exist" and "no handler found for this path". In the latter case it means that we ought to fall back to the legacy URL (?op=get&filename=...); in the former it means we ought to immediately report the 404.

This was a result of a long discussion between Raphaël and myself. We haven't documented this yet, so please ask me questions (it is late here so I'm too lazy to document the whole story). However, I suspect this is not the end of the story...

[Kapil Thangavelu (hazmat)]

get_url is a required method of the storage interface, its used when deploying charms to store a reference in the charm state for download by agents. it was happy coincidence that the change to deferred return get_url worked in jeroen's branches. The removal gives me some concern, Has this code branch been tested in practice?

The use of a response header to distinguish framework vs handler response is fine.. but speling bug numbers as part of the protocol values feels quite strange.

should jeroen's branches not be in review given this branch?

review: Needs Fixing

[Gavin Panella (allenap)]

> get_url is a required method of the storage interface, its used when
> deploying charms to store a reference in the charm state for
> download by agents.

Ah yes, so it is.

> The use of a response header to distinguish framework vs handler
> response is fine.. but speling bug numbers as part of the protocol
> values feels quite strange.

We've been talking a lot about how to do this in another way. We're
stuck with a tricky problem that is proving hard to resolve. You can
read more at http://goo.gl/9r3nD (Google Docs, Canonical only) if
you're interested. It is a work in progress.

I'll mark this branch and Jeroen's as work in progress too. We may
resurrect them later, but for now I'll move them out of the limelight.

Thanks for your comments Kapil!

Unmerged revisions

652. By Gavin Panella on 2013-02-25

Better docs, clean-ups.

651. By Gavin Panella on 2013-02-25

Remove MAASFileStorage.get_url; it is unused.

650. By Gavin Panella on 2013-02-25

Reimplement MAASFileStorage.get using fetch.

649. By Gavin Panella on 2013-02-25

New method MAASFileStorage.fetch, which will soon replace get and get_url.

648. By Jeroen T. Vermeulen on 2013-02-25

Hide the FakeFileStorage factory for 404s in another factory. Sigh.

647. By Jeroen T. Vermeulen on 2013-02-25

A single 404 isn't enough to trigger an exception on a get() any more. Patch out the get_url call where 404s are ignored.

646. By Jeroen T. Vermeulen on 2013-02-25

Tiny fixup.

645. By Jeroen T. Vermeulen on 2013-02-25

Merge trunk.

644. By Jeroen T. Vermeulen on 2013-02-25

Simplified Twisted http error handling, as per review.

643. By Jeroen T. Vermeulen on 2013-02-25

anon_url has become anon_resource_uri.

=== modified file 'juju/providers/maas/files.py'
--- juju/providers/maas/files.py	2013-02-25 22:23:23 +0000
+++ juju/providers/maas/files.py	2013-02-25 22:23:23 +0000
@@ -3,6 +3,7 @@
 
 """File Storage API client for MAAS."""
 
+from base64 import b64decode
 from cStringIO import StringIO
 import httplib
 import json
@@ -131,38 +132,34 @@
             returnValue(body)
 
     @inlineCallbacks
-    def get_url(self, name):
-        """Return a URL that can be used to access a stored file.
+    def fetch(self, name):
+        """Get a file's URL and contents from the MAAS server.
 
-        :param unicode name: the file path for which to provide a URL
-        :return: a URL
-        :rtype: str
+        :param unicode name: path to the desired file
+        :return: a byte string of the file's contents
+        :rtype: :class:`twisted.internet.defer.Deferred`
+        :raises: :exc:`juju.errors.FileNotFound` if the file doesn't exist
         """
-        # Request metadata for the file, including its anonymous URL.
-        # This is a later addition to the MAAS API, so it may not be
-        # available.  If it isn't, we fall back on the "legacy" way of
-        # composing URLs right here in the provider.
-        # Adding a trailing slash here saves a step as the API would
-        # otherwise redirect to this same URL anyway.
-        meta_url = self._base_url + name + "/"
+        url = self._base_url + name + "/"
         try:
-            body = yield self._download(meta_url, "Get file information")
+            body = yield self._download(url, "Get file & information")
         except FileNotFound:
-            # This MAAS version doesn't provide anonymous file URLs.
-            # Fall back to the legacy URL.
-            returnValue(compose_legacy_file_url(self._base_url, name))
-
-        attributes = json.loads(body)
-        anon_url = attributes.get('anon_resource_uri')
-        if anon_url is None:
-            # MAAS doesn't provide an anonymous download URL.  Generate it
-            # the old way.  If the file was uploaded by a legacy provider,
-            # MAAS will honour this legacy URL.
-            returnValue(compose_legacy_file_url(self._base_url, name))
+            workarounds = self.client.response_headers.get("Workaround", "")
+            if "bug1123986" in workarounds.split():
+                # This is a recent MAAS server. This 404 was generated by a
+                # handler in MAAS that did not find the requested file, not by
+                # Django itself, unable to route the request.
+                raise
+            else:
+                # This is an older MAAS server. This 404 was generated because
+                # MAAS did not know how to route the request.
+                url = compose_legacy_file_url(self._base_url, name)
+                body = yield self._download(url, "Get file content")
+                returnValue((url, body))
         else:
-            # Success.  Return the anonymous file download URL that MAAS
-            # sent us.
-            returnValue(urljoin(self._base_url, anon_url))
+            attributes = json.loads(body)
+            content = attributes["content"]
+            returnValue((url, b64decode(content)))
 
     @inlineCallbacks
     def get(self, name):
@@ -173,10 +170,8 @@
         :rtype: :class:`twisted.internet.defer.Deferred`
         :raises: :exc:`juju.errors.FileNotFound` if the file doesn't exist
         """
-        url = yield self.get_url(name)
-
         try:
-            body = yield self._download(url, "Download %s" % name)
+            url, body = yield self.fetch(name)
         except FileNotFound:
             # Don't raise a FileNotFound with the full URL in it.  Anonymous
             # URLs can be accessed by anyone, and privacy of the files relies

=== modified file 'juju/providers/maas/tests/test_files.py'
--- juju/providers/maas/tests/test_files.py	2013-02-25 22:23:23 +0000
+++ juju/providers/maas/tests/test_files.py	2013-02-25 22:23:23 +0000
@@ -3,6 +3,7 @@
 
 """Tests for juju.providers.maas.files"""
 
+from base64 import b64encode
 from functools import partial
 import httplib
 from io import BytesIO
@@ -14,8 +15,7 @@
 from urllib import urlencode
 from urlparse import parse_qs, urlparse
 
-from juju.errors import (
-    FileNotFound, ProviderError, ProviderInteractionError)
+from juju.errors import FileNotFound, ProviderError
 from juju.providers.maas.files import encode_multipart_data, MAASFileStorage
 from juju.providers.maas.tests.testing import CONFIG, TestCase
 
@@ -24,7 +24,8 @@
     """A fake http client to MAAS so MAASFileStorage tests can operate."""
 
     def __init__(self, url, method='GET', postdata=None, headers=None,
-                 fake_status=httplib.OK, fake_deferred=None):
+                 fake_status=httplib.OK, fake_deferred=None,
+                 fake_response_headers=None):
         """This constructor is written with partial application in mind.
 
         The fake_status and fake_deferred parameters determine the outcome
@@ -47,6 +48,10 @@
         else:
             self.deferred = fake_deferred
 
+        # Don't set a default; let tests blow up with AttributeError instead.
+        if fake_response_headers is not None:
+            self.response_headers = fake_response_headers
+
 
 def set_up_for_404():
     """Return a FakeFileStorage factory that simulates a "Not Found" response.
@@ -145,110 +150,96 @@
 class TestMAASFileStorage(TestCase):
 
     @defer.inlineCallbacks
-    def test_get_url_uses_anon_resource_uri_from_server(self):
-        # MAAS normally provides a URL for the file, as one of the file
-        # object's attributes, and get_url returns it.
-        file_path, file_params = compute_anon_file_url('/maas/api/1.0/', "987")
-
-        # Simulated FileStorage that contains an anon_resource_uri entry.
-        response = {
-            'anon_resource_uri': compose_file_url(file_path, file_params),
-        }
+    def test_fetch_returns_url_and_content_from_new_maas(self):
+        # Recent MAAS versions have a different way of accessing the file
+        # storage, and return a JSON document containing file meta data and
+        # its content.
+        response = {'content': b64encode("fred")}
         fake_storage = partial(
             FakeFileStorage, fake_deferred=defer.succeed(json.dumps(response)))
         self.setup_connection(MAASFileStorage, fake_storage)
-
-        url = yield MAASFileStorage(CONFIG).get_url("foofile")
-
-        self.assertEqual((file_path, file_params), parse_file_url(url))
-
-    @defer.inlineCallbacks
-    def test_get_url_reports_failure_while_asking_maas(self):
-        status = str(httplib.NOT_IMPLEMENTED)
-        response = defer.fail(error.Error(status, "deliberate failure", ""))
-        get_error = partial(
-            FakeFileStorage, fake_status=status, fake_deferred=response)
-        self.setup_connection(MAASFileStorage, get_error)
-        storage = MAASFileStorage(CONFIG)
-
-        try:
-            yield storage.get_url("kaboom")
-        except ProviderError:
-            # This is what we expect: an error response from the server means
-            # that the Deferred returned by get_url fails.
-            pass
-        except:
-            # Any other error?  That's unexpected.  Pass it on.
-            raise
-        else:
-            self.fail("Error was not propagated.")
-
-    @defer.inlineCallbacks
-    def test_get_url_falls_back_on_legacy_url_with_older_maas_api(self):
-        self.setup_connection(MAASFileStorage, set_up_for_404())
-        storage = MAASFileStorage(CONFIG)
-
-        url = yield storage.get_url("foofile")
-
-        self.assertEqual(
-            compute_legacy_file_url('/maas/api/1.0/', 'foofile'),
-            parse_file_url(url))
-
-    @defer.inlineCallbacks
-    def test_get_url_gives_legacy_url_if_server_omits_anon_resource_uri(self):
-        # If the server does not return a URL for a file, get_url composes
-        # its own "legacy" URL, for compatibility with existing uploads.
-        # The legacy URL consists of the base URL plus the op params for
-        # retrieving the named file.
-        self.setup_connection(MAASFileStorage, FakeFileStorage)
-        storage = MAASFileStorage(CONFIG)
-
-        url = yield storage.get_url("foofile")
-
-        self.assertEqual(
-            compute_legacy_file_url('/maas/api/1.0/', 'foofile'),
-            parse_file_url(url))
-
-    def test_get_succeeds(self):
-        self.setup_connection(MAASFileStorage, FakeFileStorage)
-        storage = MAASFileStorage(CONFIG)
-        d = storage.get("foo")
-
-        def check(value):
-            # The underlying code returns a StringIO but because
-            # implementations of StringIO and cStringIO are completely
-            # different the only reasonable thing to do here is to
-            # check to see if the returned object has a "read" method.
-            attr = getattr(value, "read")
-            self.assertIsNot(None, attr)
-            self.assertTrue(value.read)
-
-        d.addCallback(check)
-        d.addErrback(self.fail)
-        return d
-
-    def test_get_fails_if_file_does_not_exist(self):
-        self.setup_connection(MAASFileStorage, set_up_for_404())
-        storage = MAASFileStorage(CONFIG)
-        # Patch out get_url.  It will swallow the 404 that our fake file
-        # storage returns.  We want the 404 to hit the later request for
-        # the actual file.
-        self.patch(storage, 'get_url', lambda name: "/file/info/")
-
-        d = storage.get("foo")
-
-        return self.assertFailure(d, FileNotFound)
-
-    def test_get_with_unexpected_response(self):
-        unexpected_error = partial(
+        url, content = yield MAASFileStorage(CONFIG).fetch("foofile")
+        self.assertEqual(
+            ("/maas/api/1.0/files/foofile/", {}),
+            parse_file_url(url))
+        self.assertEqual("fred", content)
+
+    def test_fetch_from_new_maas_not_found(self):
+        # Recent MAAS versions include a workaround header so that clients can
+        # discern the difference between a routing failure and a file that has
+        # looked for but not found.
+        fake_storage = partial(
             FakeFileStorage,
-            fake_deferred=defer.fail(ZeroDivisionError("numpty")))
-
-        self.setup_connection(MAASFileStorage, unexpected_error)
-        storage = MAASFileStorage(CONFIG)
-        d = storage.get("foo")
-
-        return self.assertFailure(d, ProviderInteractionError)
+            fake_status=httplib.NOT_FOUND,
+            fake_deferred=defer.fail(FileNotFound("foo")),
+            fake_response_headers={"Workaround": "bug1123986"})
+        self.setup_connection(MAASFileStorage, fake_storage)
+        return self.assertFailure(
+            MAASFileStorage(CONFIG).fetch("foofile"),
+            FileNotFound)
+
+    @defer.inlineCallbacks
+    def test_fetch_returns_url_and_content_from_old_maas(self):
+        # An older MAAS instance returns a 404 to the first request because
+        # there is no handler for the path. Juju falls back to the query
+        # parameter based URL and tries again.
+        fake_storage = [
+            partial(
+                FakeFileStorage,
+                fake_status=httplib.NOT_FOUND,
+                fake_deferred=defer.fail(FileNotFound("foo")),
+                fake_response_headers={}),
+            partial(
+                FakeFileStorage,
+                fake_deferred=defer.succeed("fred")),
+            ]
+        fake_storage_factory = lambda *args, **kwargs: (
+            fake_storage.pop(0)(*args, **kwargs))
+        self.setup_connection(MAASFileStorage, fake_storage_factory)
+        url, content = yield MAASFileStorage(CONFIG).fetch("foofile")
+        self.assertEqual(
+            ("/maas/api/1.0/files/",
+             {"filename": ["foofile"], "op": ["get"]}),
+            parse_file_url(url))
+        self.assertEqual("fred", content)
+
+    def test_fetch_from_old_maas_not_found(self):
+        # An older MAAS instance returns a 404 to the first request because
+        # there is no handler for the path. Juju falls back to the query
+        # parameter based URL and tries again.
+        fake_storage = [
+            partial(
+                FakeFileStorage,
+                fake_status=httplib.NOT_FOUND,
+                fake_deferred=defer.fail(FileNotFound("foo")),
+                fake_response_headers={}),
+            partial(
+                FakeFileStorage,
+                fake_status=httplib.NOT_FOUND,
+                fake_deferred=defer.fail(FileNotFound("foo"))),
+            ]
+        fake_storage_factory = lambda *args, **kwargs: (
+            fake_storage.pop(0)(*args, **kwargs))
+        self.setup_connection(MAASFileStorage, fake_storage_factory)
+        self.assertFailure(
+            MAASFileStorage(CONFIG).fetch("foofile"),
+            FileNotFound)
+
+    @defer.inlineCallbacks
+    def test_get_is_simple_wrapper_around_fetch(self):
+        storage = MAASFileStorage(CONFIG)
+        storage.fetch = lambda name: defer.succeed(
+            ("/url/of/" + name, "Content of " + name))
+        reader = yield storage.get("some-name")
+        self.assertEqual("Content of some-name", reader.read())
+
+    @defer.inlineCallbacks
+    def test_get_returns_only_file_name_when_file_not_found(self):
+        storage = MAASFileStorage(CONFIG)
+        storage.fetch = lambda name: defer.fail(FileNotFound("/path/file"))
+        error = yield self.assertFailure(
+            storage.get("some-name"), FileNotFound)
+        self.assertEqual("some-name", error.path)
 
     def test_put_succeeds(self):
         self.setup_connection(MAASFileStorage, FakeFileStorage)