[MERGE/RFC] Add transport jail to bzr smart server; add ignore_fallbacks option to BzrDir.open_branch

[Andrew Bennetts]

First, apologies for the length of this reply.

Robert Collins wrote:
> On Fri, 2009-03-20 at 13:16 +1100, Andrew Bennetts wrote:
> > To be paranoid we perhaps should make sure the jail state propagates to
> > new threads, although very little bzrlib code uses threads at the
> > moment.
> 
> Possibly, OTOH we generally avoid threads today, so perhaps this is just
> something to document clearly in the writing-plugins, and server docs.

Right.  I'm not worried about doing it right now, it's just something we may
want to do in the future.

> > This may cause some trouble for server-side plugins that actually do
> > want to access branches outside the jail, e.g. to automatically mirror a
> > branch to another location.  We should spend a bit of time making sure
> > we have a reasonable API for those plugins to use to do that (a API to
> > extend the jail, or circumvent it?).
> 
> I'm not against making a clean API for this, but do suggest its optional
> - wait for people doing it then pick the cleanest solution they come up
> with :).

Well, that's ok, so as it's actually *possible*.  I *think* it's possible
with something ugly like requiring that the hook callable registered by the
plugin does something like:

   orig_jail = relax_jail()
   try:
      actual_work()
   finally:
      restore_jail(orig_jail)

Which is pretty ugly (especially because the plugin would need to write
{relax,restore}_jail first), but I suppose it's workable.

(As a bit of a digression: I also don't think it's quite reasonable as a
policy to leave it totally up to the plugin authors to design our APIs if we
haven't thought about them yet.  Their use cases do need to inform our API
design, but API design can influence underlying design, and plugin authors
can't do anything directly about that.  In other words, I think it's ok to
say “it's up to someone else to build a good API for this” only so long as
there is first a good foundation to allow them to build it.)

> > In support of that, this patch also adds an ignore_fallbacks option to
> > BzrDir.open_branch (and BranchFormat.open, and so on).  This allows
> > the
> > smart server methods that do need to open a branch to do so without
> > opening fallback branches that are forbidden (and unnecessary to the
> > implementation of the server request).  I think this is potentially
> > useful
> > in other cases as well.
> > 
> > There are some outstanding issues:
> > 
> >  * Tests fail. 
> 
> Andrew and I have been talking about these tests already, broadly we
> feel that clone|sprout on a reference containing bzr dir isn't something
> we do in the UI, and its safe to just make it error; Andrew is working
> on this at the moment, I think.

Right.  I've attached an updated bundle that does that.  There's minor
fallout for 1.13 clients, but not with any of the core commands, and
probably not with any plugins either.

The change is that BzrDir.cloning_metadir RPC now raises a (new) error if
called on a branch reference, and to minimise test fallout the RemoteBzrDir
client catches that, opens the referenced branch, and tries again with that
branch's bzrdir.  Normal code paths don't try to do this, so aren't
affected, e.g. the RPC count ratchets haven't gone up.

> >  * Robert would rather that open_branch didn't have a new flag.
> > Instead he
> >    proposes (Robert please jump in if I'm misrepresenting this in any
> > way)
> >    that Branch itself should check if it's being opened while in a
> > jail, and
> >    if so automatically skip opening the fallbacks.
> 
> You're misrepresenting it :). I'm happy with a new flag - and suspect we
> will need it from time to time in other legitimate code.

Thanks for correcting me :)

> I'm not happy having to explicitly use it in the smart server. So one
> way would be to say I want a three-valued flag:
> auto - open fallbacks outside of the server, don't inside the server
> open - always open, fail if unable to
> never - never open.

To be clear, if we had that flag, I assume you'd like the default to be
"auto"?

> The main reason for this is that in the server, for our code, we *never*
> want to open a fallback. And any api that opens a branch for us (like
> cloning metadir) has to honour this desire. We can either propogate the
> flag everywhere (ugh), or have it Just Work when we are inside the
> server. I'm entirely happy with people overriding it in either direction
> inside the server. But I'm not happy with it potentially propogating all
> over the place. Global state is generally bad... but not always.

I guess I don't see is as being such a big deal to ask callers to be
explicit about whether they want a full branch or not.  The effort to update
the affected server-side code in core was tiny.  So in our server-side code
we never want to open a fallback, so all two places in the server that did
so now pass ignore_fallbacks=True in my patch.

I'm actually tempted to make open_branch(ignore_fallbacks=False) always fail
server-side, even for unstacked branches, just to make potentially
server-unsafe code fail as early as possible, but that's probably a bit
extreme.

I'm not sure that we should be trying to hide the fact that the server-side
is fundamentally different for a plugin (or core bzrlib code) than the
client-side.  If the difference was something we could transparently hide
from the caller, then I'd be in favour, but it isn't.  I'm a bit reminded of
how naïve remote object protocols that assume remote objects and method
calls can always be transparently substituted for local ones suck, because
there are differences that simply cannot be hidden (unlike local objects,
network connections can fail to be accessable at any time, and remote
methods calls have very different performance characteristics to calls made
in local memory).

Plugins that want to work server-side on possibly stacked branches cannot
avoid dealing with the issue.  Even post_change_branch_tip hooks that don't
open their own branch will need to cope with the passed in branch possibly
being a stacked branch with an inaccessible fallback.  Adding magic to
branch opening won't make any difference to this case; the plugin needs to
anticipate the issue or it will error on non-trivial operations like
generating diffs.

I guess in a nutshell my feeling is adding magic to branch open doesn't
really offer much practical benefit to callers (adding a simple boolean flag
to the open_branch call is the least of your worries in dealing with a
branch with an inaccessible fallback), and the cost is quite high (because
it's magic).

> >   So the server could continue to use plain bzrdir.open_branch().  I
> >   don't like the idea of a method giving a different type of result (a
> >   fully working branch vs. a partially working branch) based on global
> >   state; Robert doesn't like adding yet another flag to open_branch, and
> >   forcing people like plugin authors to decide if they need to open a
> >   fallback-enabled branch or not.  If branches were more lazy about when
> >   they opened fallback locations, perhaps we'd both be happy, but
> >   implementing that would probably be non-trivial.
> 
> The bit above here is fine though - I just wanted to ensure it was clear
> that I'm ok with a parameter to open_branch, as long as its *not needed
> in the common case*.

That's good to know.  Thanks for being clear :)

So, given that the flag is useful anyway, why block this patch on the
question of whether it should be necessary server-side?  If we do decide
that magic is appropriate we can add that later.  It's still relatively
early in the release cycle, so there's plenty of time for feedback.

> > Robert and I would very much like to hear other people's opinions on the
> > explicit-flag vs. automatic-check-for-jail question, because we haven't
> > yet been able to convince each other.
> 
> Indeed :).

This still holds!  People who aren't Robert: share your thoughts!

> > I'd also like feedback on the patch in general; despite the open
> > issues it's
> > actually quite close to being ready to land.
> 
> Parameters added to def open() deserve docstrings :)

Done.

> +                #format =
> self._bzrdir.find_branch_format().network_name()
> 
> Either do it, or delete it :)

Deleted :)

> Other than those comments, the lack of tests on ignore_fallbacks, and
> our continuing deadlock on defaulting-in-the-server, its bb:approve for
> me.
> 
> bb:resubmit

:)

Here's an updated patch.  The two issues you give there aren't addressed so
it's still in limbo, but the others should be, including test failures
(although I need to do another full test run to be 100% sure).

-Andrew.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzrdir-open-gaol-round-2.patch
Type: text/x-diff
Size: 38754 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090323/a939cd8d/attachment-0001.bin