Patchwork [V2] wireproto: support disabling bundle1 only if repo is generaldelta

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 2, 2016, 8:28 p.m.
Message ID <ab4f909ad85632312e1a.1451766537@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/12488/
State Accepted
Headers show

Comments

Gregory Szorc - Jan. 2, 2016, 8:28 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1450641384 28800
#      Sun Dec 20 11:56:24 2015 -0800
# Node ID ab4f909ad85632312e1af5493cdf4915d7dc586d
# Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
wireproto: support disabling bundle1 only if repo is generaldelta

I recently implemented the server.bundle1* options to control whether
bundle1 exchange is allowed.

After thinking about Mozilla's strategy for handling generaldelta
rollout a bit more, I think server operators need an additional
lever: disable bundle1 if and only if the repo is generaldelta.

bundle1 exchange for non-generaldelta repos will not have the potential
for CPU explosion that generaldelta repos do. Therefore, it makes sense
for server operators to continue to allow bundle1 exchange for
non-generaldelta repos without having to set a per-repo hgrc option
to change the policy depending on whether the repo is generaldelta.

This patch introduces a new set of options to control bundle1 behavior
for generaldelta repos. These options enable server operators to limit
bundle1 restrictions to the class of repos that can be performance
issues. It also allows server operators to tie bundle1 access to store
format. In many server environments (including Mozilla's), legacy repos
will not be generaldelta and new repos will or might be. New repos often
aren't bound by legacy access requirements, so setting a global policy
that disallows access to new/generaldelta repos via bundle1 could be a
reasonable policy in many server environments. This patch makes this
policy very easy to implement (modify global hgrc, add options to
existing generaldelta repos to grandfather them in).
Augie Fackler - Jan. 5, 2016, 5:14 p.m.
On Sat, Jan 02, 2016 at 12:28:57PM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1450641384 28800
> #      Sun Dec 20 11:56:24 2015 -0800
> # Node ID ab4f909ad85632312e1af5493cdf4915d7dc586d
> # Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
> wireproto: support disabling bundle1 only if repo is generaldelta


Queued this, thanks.

>
> I recently implemented the server.bundle1* options to control whether
> bundle1 exchange is allowed.
>
> After thinking about Mozilla's strategy for handling generaldelta
> rollout a bit more, I think server operators need an additional
> lever: disable bundle1 if and only if the repo is generaldelta.
>
> bundle1 exchange for non-generaldelta repos will not have the potential
> for CPU explosion that generaldelta repos do. Therefore, it makes sense
> for server operators to continue to allow bundle1 exchange for
> non-generaldelta repos without having to set a per-repo hgrc option
> to change the policy depending on whether the repo is generaldelta.
>
> This patch introduces a new set of options to control bundle1 behavior
> for generaldelta repos. These options enable server operators to limit
> bundle1 restrictions to the class of repos that can be performance
> issues. It also allows server operators to tie bundle1 access to store
> format. In many server environments (including Mozilla's), legacy repos
> will not be generaldelta and new repos will or might be. New repos often
> aren't bound by legacy access requirements, so setting a global policy
> that disallows access to new/generaldelta repos via bundle1 could be a
> reasonable policy in many server environments. This patch makes this
> policy very easy to implement (modify global hgrc, add options to
> existing generaldelta repos to grandfather them in).
>
> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -1340,24 +1340,36 @@ Controls generic server settings.
>  ``maxhttpheaderlen``
>      Instruct HTTP clients not to send request headers longer than this
>      many bytes. (default: 1024)
>
>  ``bundle1``
>      Whether to allow clients to push and pull using the legacy bundle1
>      exchange format. (default: True)
>
> +``bundle1gd``
> +    Like ``bundle1` but only used if the repository is using the
> +    *generaldelta* storage format. (default: True)
> +
>  ``bundle1.push``
>      Whether to allow clients to push using the legacy bundle1 exchange
>      format. (default: True)
>
> +``bundle1gd.push``
> +    Like ``bundle1.push` but only used if the repository is using the
> +    *generaldelta* storage format. (default: True)
> +
>  ``bundle1.pull``
>      Whether to allow clients to pull using the legacy bundle1 exchange
>      format. (default: True)
>
> +``bundle1gd.pull``
> +    Like ``bundle1.pull` but only used if the repository is using the
> +    *generaldelta* storage format. (default: True)
> +
>      Large repositories using the *generaldelta* storage format should
>      consider setting this option because converting *generaldelta*
>      repositories to the exchange format required by the bundle1 data
>      format can consume a lot of CPU.
>
>  ``smtp``
>  --------
>
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -486,22 +486,43 @@ def options(cmd, keys, others):
>          if k in others:
>              opts[k] = others[k]
>              del others[k]
>      if others:
>          sys.stderr.write("warning: %s ignored unexpected arguments %s\n"
>                           % (cmd, ",".join(others)))
>      return opts
>
> -def bundle1allowed(ui, action):
> -    """Whether a bundle1 operation is allowed from the server."""
> +def bundle1allowed(repo, action):
> +    """Whether a bundle1 operation is allowed from the server.
> +
> +    Priority is:
> +
> +    1. server.bundle1gd.<action> (if generaldelta active)
> +    2. server.bundle1.<action>
> +    3. server.bundle1gd (if generaldelta active)
> +    4. server.bundle1
> +    """
> +    ui = repo.ui
> +    gd = 'generaldelta' in repo.requirements
> +
> +    if gd:
> +        v = ui.configbool('server', 'bundle1gd.%s' % action, None)
> +        if v is not None:
> +            return v
> +
>      v = ui.configbool('server', 'bundle1.%s' % action, None)
>      if v is not None:
>          return v
>
> +    if gd:
> +        v = ui.configbool('server', 'bundle1gd', None)
> +        if v is not None:
> +            return v
> +
>      return ui.configbool('server', 'bundle1', True)
>
>  # list of commands
>  commands = {}
>
>  def wireprotocommand(name, args=''):
>      """decorator for wire protocol command"""
>      def register(func):
> @@ -660,17 +681,17 @@ def getbundle(repo, proto, others):
>              if v == '0':
>                  opts[k] = False
>              else:
>                  opts[k] = bool(v)
>          elif keytype != 'plain':
>              raise KeyError('unknown getbundle option type %s'
>                             % keytype)
>
> -    if not bundle1allowed(repo.ui, 'pull'):
> +    if not bundle1allowed(repo, 'pull'):
>          if not exchange.bundle2requested(opts.get('bundlecaps')):
>              return ooberror(bundle2required)
>
>      cg = exchange.getbundle(repo, 'serve', **opts)
>      return streamres(proto.groupchunks(cg))
>
>  @wireprotocommand('heads')
>  def heads(repo, proto):
> @@ -776,17 +797,17 @@ def unbundle(repo, proto, heads):
>          fd, tempname = tempfile.mkstemp(prefix='hg-unbundle-')
>          fp = os.fdopen(fd, 'wb+')
>          r = 0
>          try:
>              proto.getfile(fp)
>              fp.seek(0)
>              gen = exchange.readbundle(repo.ui, fp, None)
>              if (isinstance(gen, changegroupmod.cg1unpacker)
> -                and not bundle1allowed(repo.ui, 'push')):
> +                and not bundle1allowed(repo, 'push')):
>                  return ooberror(bundle2required)
>
>              r = exchange.unbundle(repo, gen, their_heads, 'serve',
>                                    proto._client())
>              if util.safehasattr(r, 'addpart'):
>                  # The return looks streamable, we are in the bundle2 case and
>                  # should return a stream.
>                  return streamres(r.getchunks())
> diff --git a/tests/test-bundle2-exchange.t b/tests/test-bundle2-exchange.t
> --- a/tests/test-bundle2-exchange.t
> +++ b/tests/test-bundle2-exchange.t
> @@ -972,16 +972,61 @@ Servers can disable bundle1 for clone/pu
>
>    $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2
>    requesting all changes
>    abort: remote error:
>    incompatible Mercurial client; bundle2 required
>    (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
>    [255]
>    $ killdaemons.py
> +  $ cd ..
> +
> +bundle1 can still pull non-generaldelta repos when generaldelta bundle1 disabled
> +
> +  $ hg --config format.usegeneraldelta=false init notgdserver
> +  $ cd notgdserver
> +  $ cat > .hg/hgrc << EOF
> +  > [server]
> +  > bundle1gd.pull = false
> +  > EOF
> +
> +  $ touch foo
> +  $ hg -q commit -A -m initial
> +  $ hg serve -p $HGPORT -d --pid-file=hg.pid
> +  $ cat hg.pid >> $DAEMON_PIDS
> +
> +  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2-1
> +  requesting all changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files
> +  updating to branch default
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +
> +  $ killdaemons.py
> +  $ cd ../bundle2onlyserver
> +
> +bundle1 pull can be disabled for generaldelta repos only
> +
> +  $ cat > .hg/hgrc << EOF
> +  > [server]
> +  > bundle1gd.pull = false
> +  > EOF
> +
> +  $ hg serve -p $HGPORT -d --pid-file=hg.pid
> +  $ cat hg.pid >> $DAEMON_PIDS
> +  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2
> +  requesting all changes
> +  abort: remote error:
> +  incompatible Mercurial client; bundle2 required
> +  (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
> +  [255]
> +
> +  $ killdaemons.py
>
>  Verify the global server.bundle1 option works
>
>    $ cat > .hg/hgrc << EOF
>    > [server]
>    > bundle1 = false
>    > EOF
>    $ hg serve -p $HGPORT -d --pid-file=hg.pid
> @@ -989,16 +1034,52 @@ Verify the global server.bundle1 option
>    $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT not-bundle2
>    requesting all changes
>    abort: remote error:
>    incompatible Mercurial client; bundle2 required
>    (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
>    [255]
>    $ killdaemons.py
>
> +  $ cat > .hg/hgrc << EOF
> +  > [server]
> +  > bundle1gd = false
> +  > EOF
> +  $ hg serve -p $HGPORT -d --pid-file=hg.pid
> +  $ cat hg.pid >> $DAEMON_PIDS
> +
> +  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2
> +  requesting all changes
> +  abort: remote error:
> +  incompatible Mercurial client; bundle2 required
> +  (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
> +  [255]
> +
> +  $ killdaemons.py
> +
> +  $ cd ../notgdserver
> +  $ cat > .hg/hgrc << EOF
> +  > [server]
> +  > bundle1gd = false
> +  > EOF
> +  $ hg serve -p $HGPORT -d --pid-file=hg.pid
> +  $ cat hg.pid >> $DAEMON_PIDS
> +
> +  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2-2
> +  requesting all changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files
> +  updating to branch default
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +
> +  $ killdaemons.py
> +  $ cd ../bundle2onlyserver
> +
>  Verify bundle1 pushes can be disabled
>
>    $ cat > .hg/hgrc << EOF
>    > [server]
>    > bundle1.push = false
>    > [web]
>    > allow_push = *
>    > push_ssl = false
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Jan. 7, 2016, 10:05 p.m.
On Sat, Jan 2, 2016 at 12:29 PM Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> +``bundle1gd``
> +    Like ``bundle1` but only used if the repository is using the
>

There is only one closing backticks here and below, which breaks the build.
I'll fix in the clowncopter.

Patch

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -1340,24 +1340,36 @@  Controls generic server settings.
 ``maxhttpheaderlen``
     Instruct HTTP clients not to send request headers longer than this
     many bytes. (default: 1024)
 
 ``bundle1``
     Whether to allow clients to push and pull using the legacy bundle1
     exchange format. (default: True)
 
+``bundle1gd``
+    Like ``bundle1` but only used if the repository is using the
+    *generaldelta* storage format. (default: True)
+
 ``bundle1.push``
     Whether to allow clients to push using the legacy bundle1 exchange
     format. (default: True)
 
+``bundle1gd.push``
+    Like ``bundle1.push` but only used if the repository is using the
+    *generaldelta* storage format. (default: True)
+
 ``bundle1.pull``
     Whether to allow clients to pull using the legacy bundle1 exchange
     format. (default: True)
 
+``bundle1gd.pull``
+    Like ``bundle1.pull` but only used if the repository is using the
+    *generaldelta* storage format. (default: True)
+
     Large repositories using the *generaldelta* storage format should
     consider setting this option because converting *generaldelta*
     repositories to the exchange format required by the bundle1 data
     format can consume a lot of CPU.
 
 ``smtp``
 --------
 
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -486,22 +486,43 @@  def options(cmd, keys, others):
         if k in others:
             opts[k] = others[k]
             del others[k]
     if others:
         sys.stderr.write("warning: %s ignored unexpected arguments %s\n"
                          % (cmd, ",".join(others)))
     return opts
 
-def bundle1allowed(ui, action):
-    """Whether a bundle1 operation is allowed from the server."""
+def bundle1allowed(repo, action):
+    """Whether a bundle1 operation is allowed from the server.
+
+    Priority is:
+
+    1. server.bundle1gd.<action> (if generaldelta active)
+    2. server.bundle1.<action>
+    3. server.bundle1gd (if generaldelta active)
+    4. server.bundle1
+    """
+    ui = repo.ui
+    gd = 'generaldelta' in repo.requirements
+
+    if gd:
+        v = ui.configbool('server', 'bundle1gd.%s' % action, None)
+        if v is not None:
+            return v
+
     v = ui.configbool('server', 'bundle1.%s' % action, None)
     if v is not None:
         return v
 
+    if gd:
+        v = ui.configbool('server', 'bundle1gd', None)
+        if v is not None:
+            return v
+
     return ui.configbool('server', 'bundle1', True)
 
 # list of commands
 commands = {}
 
 def wireprotocommand(name, args=''):
     """decorator for wire protocol command"""
     def register(func):
@@ -660,17 +681,17 @@  def getbundle(repo, proto, others):
             if v == '0':
                 opts[k] = False
             else:
                 opts[k] = bool(v)
         elif keytype != 'plain':
             raise KeyError('unknown getbundle option type %s'
                            % keytype)
 
-    if not bundle1allowed(repo.ui, 'pull'):
+    if not bundle1allowed(repo, 'pull'):
         if not exchange.bundle2requested(opts.get('bundlecaps')):
             return ooberror(bundle2required)
 
     cg = exchange.getbundle(repo, 'serve', **opts)
     return streamres(proto.groupchunks(cg))
 
 @wireprotocommand('heads')
 def heads(repo, proto):
@@ -776,17 +797,17 @@  def unbundle(repo, proto, heads):
         fd, tempname = tempfile.mkstemp(prefix='hg-unbundle-')
         fp = os.fdopen(fd, 'wb+')
         r = 0
         try:
             proto.getfile(fp)
             fp.seek(0)
             gen = exchange.readbundle(repo.ui, fp, None)
             if (isinstance(gen, changegroupmod.cg1unpacker)
-                and not bundle1allowed(repo.ui, 'push')):
+                and not bundle1allowed(repo, 'push')):
                 return ooberror(bundle2required)
 
             r = exchange.unbundle(repo, gen, their_heads, 'serve',
                                   proto._client())
             if util.safehasattr(r, 'addpart'):
                 # The return looks streamable, we are in the bundle2 case and
                 # should return a stream.
                 return streamres(r.getchunks())
diff --git a/tests/test-bundle2-exchange.t b/tests/test-bundle2-exchange.t
--- a/tests/test-bundle2-exchange.t
+++ b/tests/test-bundle2-exchange.t
@@ -972,16 +972,61 @@  Servers can disable bundle1 for clone/pu
 
   $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2
   requesting all changes
   abort: remote error:
   incompatible Mercurial client; bundle2 required
   (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
   [255]
   $ killdaemons.py
+  $ cd ..
+
+bundle1 can still pull non-generaldelta repos when generaldelta bundle1 disabled
+
+  $ hg --config format.usegeneraldelta=false init notgdserver
+  $ cd notgdserver
+  $ cat > .hg/hgrc << EOF
+  > [server]
+  > bundle1gd.pull = false
+  > EOF
+
+  $ touch foo
+  $ hg -q commit -A -m initial
+  $ hg serve -p $HGPORT -d --pid-file=hg.pid
+  $ cat hg.pid >> $DAEMON_PIDS
+
+  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2-1
+  requesting all changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  updating to branch default
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+  $ killdaemons.py
+  $ cd ../bundle2onlyserver
+
+bundle1 pull can be disabled for generaldelta repos only
+
+  $ cat > .hg/hgrc << EOF
+  > [server]
+  > bundle1gd.pull = false
+  > EOF
+
+  $ hg serve -p $HGPORT -d --pid-file=hg.pid
+  $ cat hg.pid >> $DAEMON_PIDS
+  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2
+  requesting all changes
+  abort: remote error:
+  incompatible Mercurial client; bundle2 required
+  (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
+  [255]
+
+  $ killdaemons.py
 
 Verify the global server.bundle1 option works
 
   $ cat > .hg/hgrc << EOF
   > [server]
   > bundle1 = false
   > EOF
   $ hg serve -p $HGPORT -d --pid-file=hg.pid
@@ -989,16 +1034,52 @@  Verify the global server.bundle1 option 
   $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT not-bundle2
   requesting all changes
   abort: remote error:
   incompatible Mercurial client; bundle2 required
   (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
   [255]
   $ killdaemons.py
 
+  $ cat > .hg/hgrc << EOF
+  > [server]
+  > bundle1gd = false
+  > EOF
+  $ hg serve -p $HGPORT -d --pid-file=hg.pid
+  $ cat hg.pid >> $DAEMON_PIDS
+
+  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2
+  requesting all changes
+  abort: remote error:
+  incompatible Mercurial client; bundle2 required
+  (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
+  [255]
+
+  $ killdaemons.py
+
+  $ cd ../notgdserver
+  $ cat > .hg/hgrc << EOF
+  > [server]
+  > bundle1gd = false
+  > EOF
+  $ hg serve -p $HGPORT -d --pid-file=hg.pid
+  $ cat hg.pid >> $DAEMON_PIDS
+
+  $ hg --config experimental.bundle2-exp=false clone http://localhost:$HGPORT/ not-bundle2-2
+  requesting all changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  updating to branch default
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+  $ killdaemons.py
+  $ cd ../bundle2onlyserver
+
 Verify bundle1 pushes can be disabled
 
   $ cat > .hg/hgrc << EOF
   > [server]
   > bundle1.push = false
   > [web]
   > allow_push = *
   > push_ssl = false