pyjuju

Currently when the MAAS provider is asked for the download URL of an uploaded file, it just computes a standard URL for that file on the MAAS API. But in order to be able to separate namespaces for different users (and juju environments), we need to give MAAS more room to assign its own URLs.

And so, this change makes get_url() ask the MAAS API for the correct URL. If it finds it's talking to an older MAAS which doesn't support that, it still falls back to the old way of doing things. But if MAAS provides a URL for the file, that's the one that it will pass on to nodes that need access.

I went with inlineCallbacks here, even though many people dislike it for its unhelpful tracebacks. I did that because it allowed me to break up functions along traditional lines of abstraction. I'm adding just enough complexity that a hodgepodge of nested function definitions and clever chaining was making it hard to see what was going on.

You'll notice that get_url now returns Deferred, instead of the URL itself. Luckily the code (well, outside the MAAS provider itself of course!) does not make assumptions about this. It correctly runs the function through the Twisted reactor. There is no documentation to say that that is wrong, so I can only infer that this was always intended to be possible.

Possible, yes, but not optimal. There would be other ways of getting at the file data when talking to a new MAAS (it's actually contained in the file information that get_url downloads). And we might cache the knowledge of which MAAS we're talking to, and we might cache URLs, and so on. But we believe those to be secondary concerns. Our reasons are:

1. juju-core will not have any of these problems. It will go straight to the file information and return the file's contents, or its URL, immediately.

2. We urgently need to fix an inability to run two juju environments on one MAAS, so that takes precedence.

3. Amdahl's Law: if this isn't the bottleneck, it's not the right place to optimize.

4. Knuth's Law: optimizing the process would also increase complexity and thus risk.

My biggest worry is in the error handling. Development was strictly TDD, but with this much layercake in the way there is a risk of "circular reasoning" between the test expectations and the test doubles. We'll have to test thoroughly in the Q/A lab. In particular, the "not found" case will be exercised when we test the new provider code against an old MAAS. Since both of the "404" situations that come up go through the same code, that one scenario will validate both.

Jeroen

Revision history for this message

Raphaël Badin (rvb) wrote on 2013-02-22:

158 + meta_url = self._base_url + name

This generates urls like "/MAAS/api/1.0/files/provider-state" (note the missing slash at the end of the url) that gets redirected to "/MAAS/api/1.0/files/provider-state/". (See this extract from apache2's log: http://paste.ubuntu.com/1705131/). It's probably better to generate the proper url directly.

Revision history for this message

Raphaël Badin (rvb) wrote on 2013-02-22:

[1]

166 + attributes = json.loads(body)
167 + anon_url = attributes.get('anon_url')
168 + if anon_url is None:
169 + # MAAS doesn't provide an anonymous download URL. Generate it
170 + # the old way. If the file was uploaded by a legacy provider,
171 + # MAAS will honour this legacy URL.
172 + returnValue(compose_legacy_file_url(self._base_url, name))
173 + else:
174 + # Success. Return the anonymous file download URL that MAAS
175 + # sent us.
176 + returnValue(anon_url)

The 'anon_url' does not include the host part (see the discussion on bug 1131692). You need to change that part to something along the lines of https://pastebin.canonical.com/85388/ plus s/anon_url/anon_resource_uri/ (to account for the fix I just committed in lp:~rvb/maas/bug-1123986-key-api).

[2]

With that change, I was able to successfully deploy bootrap an environment and deploy a mysql and a mediawiki charms. But look at the record of the calls to the file storage API endpoint: https://pastebin.canonical.com/85392/. Line 13 and 27, you can see the bootstrap node still using the old version of the anonymous urls. Note that the nodes themselves use the new version of the anonymous urls.

Revision history for this message

Gavin Panella (allenap) wrote on 2013-02-23:

Download full text (3.6 KiB)

Looks good! [1] and [2] were head-scratchers but I don't want to block
you on resolving these, hence +1.

[1]

Would this last conditional be needed if this block were moved up to
where the `pass` is made?

        try:
            body = yield self.dispatch_query(url)
        except Error:
            if hasattr(self.client, 'status'):
                # This is an error response from the server. We'll
                # handle it by looking at the http status code.
                status = int(self.client.status)
                if status == httplib.NOT_FOUND:
                    raise FileNotFound(url)
                else:
                    # Something else went wrong. Pass it on.
                    raise ProviderError(...
            else:
                raise

[2]

This got me wondering...

Error always has a status attribute. In twisted.web.client it only
ever appears to be raised when there's a status to report, which
implies that some response has been received.

Also, client.status (where client is an instance of HTTPClientFactory)
is set when twisted.web.http.HTTPClient.lineReceived calls
gotStatus(), when the first line back from the server is received, but
is not reset when there's a 30[123] redirect, so it's not a wholly
reliable test for a client-side failure.

Might the following do what you need?

        try:
            body = yield self.dispatch_query(url)
        except Error as error:
            if int(error.status) == httplib.NOT_FOUND:
                raise FileNotFound(url)
            else:
                # Something else went wrong. Pass it on.
                raise ProviderError(
                    "%s: unexpected HTTP %s" % (intention, status))
        except Exception as e:
            raise convert_unknown_error(e)

returnValue(body)

Btw, I like the intention stuff. I think it'll make user's lives a
little nicer.

[3]

+ anon_url = attributes.get('anon_url')

The handling of this will need to change; rvba's changing this to
anon_resource_uri (iirc, better check), which also means you'll need
to join it to self._base_url.

[4]

+ try:
+ yield storage.get_url("kaboom")
+ except ProviderError:
+ ...

Looks good! [1] and [2] were head-scratchers but I don't want to block
you on resolving these, hence +1.

[1]

+    def _download(self, url, intention=None):
...
+        try:
+            body = yield self.dispatch_query(url)
+        except Error:
+            if hasattr(self.client, 'status'):
+                # This is an error response from the server.  We'll
+                # handle it by looking at the http status code.
+                pass
...
+        status = int(self.client.status)
+        if status == httplib.NOT_FOUND:
+            raise FileNotFound(url)
+        if (status < 200) or (status > 299):
+            # Something else went wrong.  Pass it on.
+            raise ProviderError("%s: unexpected HTTP %s" % (intention, status))

Would this last conditional be needed if this block were moved up to
where the `pass` is made?

try:
            body = yield self.dispatch_query(url)
        except Error:
            if hasattr(self.client, 'status'):
                # This is an error response from the server.  We'll
                # handle it by looking at the http status code.
                status = int(self.client.status)
                if status == httplib.NOT_FOUND:
                    raise FileNotFound(url)
                else:
                    # Something else went wrong.  Pass it on.
                    raise ProviderError(...
            else:
                raise

[2]

+        except Error:
+            if hasattr(self.client, 'status'):
+                # This is an error response from the server.  We'll
+                # handle it by looking at the http status code.
+                pass
+            else:
+                # This is a client-side failure.  Pass it on.
+                raise

This got me wondering...

Error always has a status attribute. In twisted.web.client it only
ever appears to be raised when there's a status to report, which
implies that some response has been received.

Might the following do what you need?

try:
            body = yield self.dispatch_query(url)
        except Error as error:
            if int(error.status) == httplib.NOT_FOUND:
                raise FileNotFound(url)
            else:
                # Something else went wrong.  Pass it on.
                raise ProviderError(
                    "%s: unexpected HTTP %s" % (intention, status))
        except Exception as e:
            raise convert_unknown_error(e)

returnValue(body)

Btw, I like the intention stuff. I think it'll make user's lives a
little nicer.

[3]

+        anon_url = attributes.get('anon_url')

The handling of this will need to change; rvba's changing this to
anon_resource_uri (iirc, better check), which also means you'll need
to join it to self._base_url.

[4]

+        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.")

Fwiw, testtools.ExpectedException can help here, assuming we're
allowed testtools (ah, maybe not):

with ExpectedException(ProviderError):
            yield storage.get_url("kaboom")

review: Approve

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2013-02-25:

> Looks good! [1] and [2] were head-scratchers but I don't want to block
> you on resolving these, hence +1.

Thanks. It's a holiday today but people are waiting for this so I'm pushing it through now, and then I will enjoy my evening in a guilt-free fashion. :)

—

> + def _download(self, url, intention=None):

> + if (status < 200) or (status > 299):
> + # Something else went wrong. Pass it on.
> + raise ProviderError("%s: unexpected HTTP %s" % (intention,
> status))
>
> Would this last conditional be needed if this block were moved up to
> where the `pass` is made?

Probably not, actually! Twisted's documentation has a way of not answering important questions, so I was a bit extra conservative with this. But now that I've looked at the code, I think it'd be OK to do error processing only in the exception case.

My remaining worry is that as far as I can see, Twisted would also raise an error for "No content" and a bunch of other 2xx responses. But as far as I know we don't need that anywhere, and an unwarranted error that we can make the code ignore is a lot more benign than a silent failure that we should be checking for.

—

> Error always has a status attribute. In twisted.web.client it only
> ever appears to be raised when there's a status to report, which
> implies that some response has been received.
>
> Also, client.status (where client is an instance of HTTPClientFactory)
> is set when twisted.web.http.HTTPClient.lineReceived calls
> gotStatus(), when the first line back from the server is received, but
> is not reset when there's a 30[123] redirect, so it's not a wholly
> reliable test for a client-side failure.
>
> Might the following do what you need?

[…]

Yup, that looks like what I need. Thanks — I used it.

—

> Btw, I like the intention stuff. I think it'll make user's lives a
> little nicer.

Glad to hear it. Error reporting is a field that gets nowhere near the attention it deserves, whether in theory or in practice.

—

> + anon_url = attributes.get('anon_url')
>
> The handling of this will need to change; rvba's changing this to
> anon_resource_uri (iirc, better check), which also means you'll need
> to join it to self._base_url.

Yes, Raphaël already noted that and I've made the necessary changes.

—

> Fwiw, testtools.ExpectedException can help here, assuming we're
> allowed testtools (ah, maybe not):

That's what I thought at the time I wrote it, but alas: no testtools!

Jeroen

> Looks good! [1] and [2] were head-scratchers but I don't want to block
> you on resolving these, hence +1.

Thanks.  It's a holiday today but people are waiting for this so I'm pushing it through now, and then I will enjoy my evening in a guilt-free fashion.  :)

—

> +    def _download(self, url, intention=None):

> +        if (status < 200) or (status > 299):
> +            # Something else went wrong.  Pass it on.
> +            raise ProviderError("%s: unexpected HTTP %s" % (intention,
> status))
> 
> Would this last conditional be needed if this block were moved up to
> where the `pass` is made?

Probably not, actually!  Twisted's documentation has a way of not answering important questions, so I was a bit extra conservative with this.  But now that I've looked at the code, I think it'd be OK to do error processing only in the exception case.

My remaining worry is that as far as I can see, Twisted would also raise an error for "No content" and a bunch of other 2xx responses.  But as far as I know we don't need that anywhere, and an unwarranted error that we can make the code ignore is a lot more benign than a silent failure that we should be checking for.

—

> Error always has a status attribute. In twisted.web.client it only
> ever appears to be raised when there's a status to report, which
> implies that some response has been received.
> 
> Also, client.status (where client is an instance of HTTPClientFactory)
> is set when twisted.web.http.HTTPClient.lineReceived calls
> gotStatus(), when the first line back from the server is received, but
> is not reset when there's a 30[123] redirect, so it's not a wholly
> reliable test for a client-side failure.
> 
> Might the following do what you need?

[…]

Yup, that looks like what I need.  Thanks — I used it.

—

> Btw, I like the intention stuff. I think it'll make user's lives a
> little nicer.

Glad to hear it.  Error reporting is a field that gets nowhere near the attention it deserves, whether in theory or in practice.

—

> +        anon_url = attributes.get('anon_url')
> 
> The handling of this will need to change; rvba's changing this to
> anon_resource_uri (iirc, better check), which also means you'll need
> to join it to self._base_url.

Yes, Raphaël already noted that and I've made the necessary changes.

—

> Fwiw, testtools.ExpectedException can help here, assuming we're
> allowed testtools (ah, maybe not):

That's what I thought at the time I wrote it, but alas: no testtools!

Jeroen

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2013-02-25:

The review changes exposed a weakness in the test setup: all instances of FakeFileStorageReturning404 used the same Deferred instance (the one that failed with an http not-found error). Once that had failed once, any other test calls using the same fake file storage would set their client status to 404, but would not return/raise (depending on your choice of Twisted execution model) an error. That was enough to make the old code see a 404, but not the new code which relies on these errors.

One instance went away when I patched out a call to get_url that was irrelevant to the test (reducing the number of 404s incurred from two to one). The other required me to hide the fake filestorage factory in another factory. There are probably nicer ways of doing this, such as making it a class, or refactoring the test helpers yet again. For now, I went with the simplest change that sprang to mind.

Unmerged revisions

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.
642. By Jeroen T. Vermeulen on 2013-02-25: Review changes.
641. By Jeroen T. Vermeulen on 2013-02-21: Tweaks to error handling.
640. By Jeroen T. Vermeulen on 2013-02-21: Extract download method.
639. By Jeroen T. Vermeulen on 2013-02-21: Get lint-checking etc. to work.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Beber

Benjamin Saller

Chuck Short

Gustavo Niemeyer

James Page

Jeroen T. Vermeulen

Jim Baker

John A Meinel

Jorge Castro

Kapil Thangavelu

Marius B. Kotsbak

liuxing

to status/vote changes:

Akapo

 === modified file 'Makefile'
 --- Makefile	2012-09-27 01:43:32 +0000
 +++ Makefile	2013-02-25 09:42:19 +0000
@@ -21,24 +21,34 @@
  etags:
  	@ctags -e --python-kinds=-iv -R juju
--modified=$(shell bzr status -S  |grep -P '^\s*M' | awk '{print $$2;}'| grep -P ".py$$")
--check:
--	@test -n "$(modified)" && echo $(modified) | xargs $(PEP8) --repeat
--	@test -n "$(modified)" && echo $(modified) | xargs pyflakes
--
--
--modified=$(shell bzr status -S -r ancestor:$(JUJU_TRUNK) |grep -P '^\s*M' | awk '{print $$2;}'| grep -P ".py$$")
--review:
--	#@test -n "$(modified)" && echo $(modified) | xargs $(PEP8) --repeat
--	@test -n "$(modified)" && echo $(modified) | xargs pyflakes
--
--
--modified=$(shell bzr status -S -r branch::prev  |grep -P '^\s*\+?[MN]' | awk '{print $$2;}'| grep -P "test_.*\.py$$")
++
++present_pep8=$(shell which $(PEP8))
++present_pyflakes=$(shell which pyflakes)
++warn_missing_linters:
++	@test -n "$(present_pep8)" || echo "WARNING: $(PEP8) not installed."
++	@test -n "$(present_pyflakes)" || echo "WARNING: pyflakes not installed."
++
++
++# "check": Check uncommitted changes for lint.
++check_changes=$(shell bzr status -S | grep '^[ +]*[MN]' | awk '{print $$2;}' | grep "\\.py$$")
++check: warn_missing_linters
++	@test -z $(present_pep8) || (echo $(check_changes) | xargs -r $(PEP8) --repeat)
++	@test -z $(present_pyflakes) || (echo $(check_changes) | xargs -r pyflakes)
++
++
++# "review": Check all changes compared to trunk for lint.
++review_changes=$(shell bzr status -S -r ancestor:$(JUJU_TRUNK) | grep '^[ +]*[MN]' | awk '{print $$2;}' | grep "\\.py$$")
++review: warn_missing_linters
++	#@test -z $(present_pep8) || (echo $(review_changes) | xargs -r $(PEP8) --repeat)
++	@test -z $(present_pyflakes) || (echo $(review_changes) | xargs -r pyflakes)
++
++
++ptests_changes=$(shell bzr status -S -r branch::prev | grep -P '^[ +]*[MN]' | awk '{print $$2;}'| grep "test_.*\\.py$$")
  ptests:
--	@test -n "$(modified)" && echo $(modified) | xargs ./test
++	@echo $(ptests_changes) | xargs -r ./test
--modified=$(shell  bzr status -S -r ancestor:$(JUJU_TRUNK)/|grep -P 'test.*\.py' |awk '{print $$2;}')
++btests_changes=$(shell bzr status -S -r ancestor:$(JUJU_TRUNK)/ | grep "test.*\\.py$$" | awk '{print $$2;}')
  btests:
--	@./test $(modified)
++	@./test $(btests_changes)
--.PHONY: tags check review
++.PHONY: tags check review warn_missing_linters
 === modified file 'README'
 --- README	2012-07-01 18:36:56 +0000
 +++ README	2013-02-25 09:42:19 +0000
@@ -9,7 +9,7 @@
  The juju bug tracker is at https://bugs.launchpad.net/juju
--Documentation for getting setup and running juju can be found at
++Documentation for getting set up and running juju can be found at
  http://juju.ubuntu.com/docs
  ====
 === modified file 'juju/providers/maas/files.py'
 --- juju/providers/maas/files.py	2012-03-27 11:26:50 +0000
 +++ juju/providers/maas/files.py	2013-02-25 09:42:19 +0000
@@ -5,9 +5,11 @@
  from cStringIO import StringIO
  import httplib
++import json
  import mimetypes
  import random
  import string
++from twisted.internet.defer import (inlineCallbacks, returnValue)
  from twisted.web.error import Error
  import urllib
  from urlparse import urljoin
@@ -79,6 +81,17 @@
      return body, headers
++def compose_legacy_file_url(base_url, filename):
++    """Produce a legacy anonymous URL for an uploaded file.
++
++    With newer MAAS versions, the region controller provides the anonymous
++    download URL.
++    """
++    params = {"op": "get", "filename": filename}
++    param_string = urllib.urlencode(params)
++    return base_url + "?" + param_string
++
++
  class MAASFileStorage(MAASOAuthConnection):
      """A file storage abstraction for MAAS."""
@@ -90,6 +103,34 @@
          self._auth = config["maas-oauth"]
          super(MAASFileStorage, self).__init__(self._auth)
++    @inlineCallbacks
++    def _download(self, url, intention=None):
++        """Download the resource at the given URL, using a http "GET".
++
++        Pass an "intention" string to tell a reader of the logs, in the
++        even of an error, what it was you were trying to do.
++
++        Returns the response body if successful.  Otherwise, returns either
++        FileNotFound on the URL (if the server responded with "not found"),
++        or a Twisted Error.
++        """
++        if intention is None:
++            intention = "GET %s" % url
++
++        try:
++            body = yield self.dispatch_query(url)
++        except Error as e:
++            if int(e.status) == httplib.NOT_FOUND:
++                raise FileNotFound(url)
++            else:
++                raise ProviderError(
++                    "%s: unexpected HTTP %s" % (intention, e.status))
++        except Exception as e:
++            raise convert_unknown_error(e)
++        else:
++            returnValue(body)
++
++    @inlineCallbacks
      def get_url(self, name):
          """Return a URL that can be used to access a stored file.
@@ -97,10 +138,33 @@
          :return: a URL
          :rtype: str
          """
--        params = {"op": "get", "filename": name}
--        param_string = urllib.urlencode(params)
--        return self._base_url + "?" + param_string
--
++        # 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 + "/"
++        try:
++            body = yield self._download(meta_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))
++        else:
++            # Success.  Return the anonymous file download URL that MAAS
++            # sent us.
++            returnValue(urljoin(self._base_url, anon_url))
++
++    @inlineCallbacks
      def get(self, name):
          """Get a file object from the MAAS server.
@@ -109,12 +173,17 @@
          :rtype: :class:`twisted.internet.defer.Deferred`
          :raises: :exc:`juju.errors.FileNotFound` if the file doesn't exist
          """
--        url = self.get_url(name)
--        d = self.dispatch_query(url)
--        d.addCallback(StringIO)
--        error_tab = {httplib.NOT_FOUND: FileNotFound(url)}
--        d.addErrback(_convert_error, "GET", url, error_tab)
--        return d
++        url = yield self.get_url(name)
++
++        try:
++            body = yield self._download(url, "Download %s" % 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
++            # on nobody knowing the URL itself.  Cite the filename instead.
++            raise FileNotFound(name)
++
++        returnValue(StringIO(body))
      def put(self, name, file_object):
          """Upload a file to MAAS.
 === modified file 'juju/providers/maas/tests/test_files.py'
 --- juju/providers/maas/tests/test_files.py	2012-03-27 11:26:50 +0000
 +++ juju/providers/maas/tests/test_files.py	2013-02-25 09:42:19 +0000
@@ -3,13 +3,16 @@
  """Tests for juju.providers.maas.files"""
++from functools import partial
  import httplib
  from io import BytesIO
++import json
  import re
  from textwrap import dedent
  from twisted.internet import defer
  from twisted.web import error
--from urlparse import urlparse
++from urllib import urlencode
++from urlparse import parse_qs, urlparse
  from juju.errors import (
      FileNotFound, ProviderError, ProviderInteractionError)
@@ -20,41 +23,42 @@
  class FakeFileStorage(object):
      """A fake http client to MAAS so MAASFileStorage tests can operate."""
--    def __init__(self, url, method='GET', postdata=None, headers=None):
++    def __init__(self, url, method='GET', postdata=None, headers=None,
++                 fake_status=httplib.OK, fake_deferred=None):
++        """This constructor is written with partial application in mind.
++
++        The fake_status and fake_deferred parameters determine the outcome
++        of any request: "status" is set to fake_status (if given) and
++        "deferred" is set to fake_deferred (if given, or defaults to success).
++
++        In cases where you need a factory of FakeFileStorage objects with
++        these attributes set, use functools.partial to generate a factory with
++        the fake_* parameters pre-set.
++        """
          # Store passed data for later inspection.
          self.headers = headers
          self.url = url
          self.data = postdata
          self.action = method
--        func = getattr(self, method.lower())
--        self.deferred = func()
--
--    def get(self):
--        return defer.succeed("blah")
--
--    def post(self):
--        return defer.succeed("blah")
--
--
--class FakeFileStorageReturning404(FakeFileStorage):
--
--    def get(self):
--        self.status = str(httplib.NOT_FOUND)
--        return defer.fail(error.Error(self.status, "this is a 404", ""))
--
--
--class FakeFileStorageReturningUnexpectedError(FakeFileStorage):
--
--    def get(self):
--        return defer.fail(ZeroDivisionError("numpty"))
--
--
--class FakeFileStorageWithErrorOnAddingFile(FakeFileStorage):
--
--    def post(self):
--        self.status = str(httplib.UNAUTHORIZED)
--        return defer.fail(error.Error(self.status, "this is a 401", ""))
++        self.status = str(fake_status)
++        if fake_deferred is None:
++            self.deferred = defer.succeed("{}")
++        else:
++            self.deferred = fake_deferred
++
++
++def set_up_for_404():
++    """Return a FakeFileStorage factory that simulates a "Not Found" response.
++    """
++    # This could be done with a simple global variable, except each
++    # instance of the factory must have its own instance of the Deferred
++    # that produces the error, or the client code won't fail after the
++    # Deferred's first run.
++    return partial(
++        FakeFileStorage, fake_status=httplib.NOT_FOUND,
++        fake_deferred=defer.fail(
++            error.Error(str(httplib.NOT_FOUND), "this is a 404", "")))
  class TestMAASFileAPIFunctions(TestCase):
@@ -96,16 +100,114 @@
          self.assertEqual(expected_headers, headers)
++def compute_legacy_file_url(maas_path, filename):
++    """Return (path, query) for a legacy file URL as computed by the provider.
++
++    :param maas_path: Base path (no networking part!) for the MAAS API.  Must
++        represent a directory, and so end in a slash.
++    :param filename: Name of the file you want a URL for.
++    :return: Full absolute path part of the file's URL, and a query dict.
++    """
++    assert maas_path.endswith('/')
++    return (
++        maas_path + 'files/',
++        {'op': ['get'], 'filename': [filename]},
++        )
++
++
++def compute_anon_file_url(maas_path, file_key):
++    """Return (path, query) for an anonymous file URL.
++
++    :param maas_path: Base path (no networking part!) for the MAAS API.  Must
++        represent a directory, and so end in a slash.
++    :param file_key: Unique key of the file you want a URL for.
++    :return: Full absolute path part of the file's anonymous download URL, and
++        a query dict.
++    """
++    assert maas_path.endswith('/')
++    return (
++        maas_path + 'files/',
++        {'op': ['get_by_key'], 'key': [file_key]},
++        )
++
++
++def parse_file_url(url):
++    """Return (path, query) for a file URL."""
++    urlparts = urlparse(url)
++    return urlparts.path, parse_qs(urlparts.query)
++
++
++def compose_file_url(path, query):
++    """Put the pieces (path and query) of a file URL back together."""
++    return "%s?%s" % (path, urlencode(query, doseq=True))
++
++
  class TestMAASFileStorage(TestCase):
--    def test_get_url(self):
--        # get_url should return the base URL plus the op params for a
--        # file name.
--        storage = MAASFileStorage(CONFIG)
--        url = storage.get_url("foofile")
--        urlparts = urlparse(url)
--        self.assertEqual("/maas/api/1.0/files/", urlparts.path)
--        self.assertEqual("filename=foofile&op=get", urlparts.query)
++    @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),
++        }
++        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)
@@ -125,16 +227,24 @@
          d.addErrback(self.fail)
          return d
--    def test_get_with_bad_filename(self):
--        self.setup_connection(MAASFileStorage, FakeFileStorageReturning404)
++    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):
--        self.setup_connection(
--            MAASFileStorage, FakeFileStorageReturningUnexpectedError)
++        unexpected_error = partial(
++            FakeFileStorage,
++            fake_deferred=defer.fail(ZeroDivisionError("numpty")))
++
++        self.setup_connection(MAASFileStorage, unexpected_error)
          storage = MAASFileStorage(CONFIG)
          d = storage.get("foo")
@@ -151,8 +261,12 @@
          return d
      def test_put_with_error_returned(self):
--        self.setup_connection(
--            MAASFileStorage, FakeFileStorageWithErrorOnAddingFile)
++        status = str(httplib.UNAUTHORIZED)
++        response = defer.fail(error.Error(status, "this is a 401", ""))
++        post_error = partial(
++            FakeFileStorage, fake_status=status, fake_deferred=response)
++        self.setup_connection(MAASFileStorage, post_error)
++
          storage = MAASFileStorage(CONFIG)
          fileObj = BytesIO("some data")
          d = storage.put("foo", fileObj)