launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/turnip:enable-http-v2 into turnip:master with ~pappacena/turnip:run-v2-commands as a prerequisite.

Commit message:
Enabling protocol v2 compatibility on HTTP frontend

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/turnip/+git/turnip/+merge/389131
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/turnip:enable-http-v2 into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index 1b34ac0..a290aee 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -436,18 +436,20 @@ class PackBackendProtocol(PackServerProtocol):
 
     Invokes the reference C Git implementation.
     """
+    V2_COMPATIBLE_FRONTENDS = (b'http', )
 
     hookrpc_key = None
     expect_set_symbolic_ref = False
 
-    def getV2CommandInput(self, params):
+    def getV2CommandInput(self):
         """Reconstruct what should be sent to git's stdin from the
         parameters received."""
+        params = self.params
         cmd_input = encode_packet(b"command=%s\n" % params.get(b'command'))
-        for capability in params["capabilities"].split(b"\n"):
+        for capability in params[b"capabilities"].split(b"\n"):
             cmd_input += encode_packet(b"%s\n" % capability)
         cmd_input += DELIM_PKT
-        ignore_keys = (b'capabilities', b'version')
+        ignore_keys = (b'capabilities', b'version', b'command')
         for k, v in params.items():
             k = six.ensure_binary(k)
             if k.startswith(b"turnip-") or k in ignore_keys:
@@ -458,11 +460,18 @@ class PackBackendProtocol(PackServerProtocol):
         cmd_input += FLUSH_PKT
         return cmd_input
 
+    def isV2CompatibleRequest(self):
+        frontend = self.params.get(b'turnip-frontend')
+        return (
+            frontend in self.V2_COMPATIBLE_FRONTENDS and
+            get_capabilities_advertisement(self.params.get(b'version', 1)))
+
     @defer.inlineCallbacks
     def requestReceived(self, command, raw_pathname, params):
         self.extractRequestMeta(command, raw_pathname, params)
         self.command = command
         self.raw_pathname = raw_pathname
+        self.params = params
         self.path = compose_path(self.factory.root, self.raw_pathname)
         auth_params = self.createAuthParams(params)
 
@@ -486,7 +495,7 @@ class PackBackendProtocol(PackServerProtocol):
         cmd_input = None
         cmd_env = {}
         write_operation = False
-        if not get_capabilities_advertisement(params.get(b'version', 1)):
+        if not self.isV2CompatibleRequest():
             if command == b'git-upload-pack':
                 subcmd = b'upload-pack'
             elif command == b'git-receive-pack':
@@ -506,7 +515,7 @@ class PackBackendProtocol(PackServerProtocol):
             send_path_as_option = True
             # Do not include "advertise-refs" parameter.
             params.pop(b'turnip-advertise-refs', None)
-            cmd_input = self.getV2CommandInput(params)
+            cmd_input = self.getV2CommandInput()
 
         args = []
         if params.pop(b'turnip-stateless-rpc', None):
@@ -674,7 +683,7 @@ class PackVirtServerProtocol(PackProxyServerProtocol):
     @defer.inlineCallbacks
     def requestReceived(self, command, pathname, params):
         self.extractRequestMeta(command, pathname, params)
-        permission = 'read' if command == b'git-upload-pack' else 'write'
+        permission = 'read' if command != b'git-receive-pack' else 'write'
         proxy = xmlrpc.Proxy(self.factory.virtinfo_endpoint, allowNone=True)
         try:
             auth_params = self.createAuthParams(params)
diff --git a/turnip/pack/helpers.py b/turnip/pack/helpers.py
index 93aba33..65e27bf 100644
--- a/turnip/pack/helpers.py
+++ b/turnip/pack/helpers.py
@@ -24,7 +24,7 @@ import six
 import yaml
 
 import turnip.pack.hooks
-
+from turnip.version_info import version_info
 
 FLUSH_PKT = b'0000'
 DELIM_PKT = b'0001'
@@ -297,5 +297,14 @@ def get_capabilities_advertisement(version='1'):
 
     If no binary data is sent, no advertisement is done and we declare to
     not be compatible with that specific version."""
-    # XXX pappacena 2020-08-11: Return the correct data for protocol v2.
-    return b""
+    if version != '2':
+        return b""
+    turnip_version = six.ensure_binary(version_info.get("revision_id", '-1'))
+    return (
+        encode_packet(b"version 2\n") +
+        encode_packet(b"agent=turnip/%s\n" % turnip_version) +
+        encode_packet(b"ls-refs\n") +
+        encode_packet(b"fetch=shallow\n") +
+        encode_packet(b"server-option\n") +
+        FLUSH_PKT
+    )
diff --git a/turnip/pack/http.py b/turnip/pack/http.py
index c0becf0..9e0661c 100644
--- a/turnip/pack/http.py
+++ b/turnip/pack/http.py
@@ -29,6 +29,7 @@ from paste.auth.cookie import (
     decode as decode_cookie,
     encode as encode_cookie,
     )
+import six
 from twisted.internet import (
     defer,
     error,
@@ -58,8 +59,8 @@ from turnip.pack.helpers import (
     encode_packet,
     encode_request,
     translate_xmlrpc_fault,
-    TurnipFaultCode,
-    )
+    TurnipFaultCode, get_capabilities_advertisement,
+)
 try:
     from turnip.version_info import version_info
 except ImportError:
@@ -80,6 +81,16 @@ def fail_request(request, message, code=http.INTERNAL_SERVER_ERROR):
     return b''
 
 
+def get_protocol_version_from_request(request):
+    version_header = request.requestHeaders.getRawHeaders(
+        'git-protocol', ['version=1'])[0]
+    try:
+        return version_header.split('version=', 1)[1]
+    except IndexError:
+        pass
+    return 1
+
+
 class HTTPPackClientProtocol(PackProtocol):
     """Abstract bridge between a Git pack connection and a smart HTTP request.
 
@@ -99,7 +110,11 @@ class HTTPPackClientProtocol(PackProtocol):
 
     def startGoodResponse(self):
         """Prepare the HTTP response for forwarding from the backend."""
-        raise NotImplementedError()
+        self.factory.http_request.write(
+            get_capabilities_advertisement(self.getProtocolVersion()))
+
+    def getProtocolVersion(self):
+        return get_protocol_version_from_request(self.factory.http_request)
 
     def backendConnected(self):
         """Called when the backend is connected and has sent a good packet."""
@@ -209,6 +224,7 @@ class HTTPPackClientRefsProtocol(HTTPPackClientProtocol):
         self.factory.http_request.setHeader(
             b'Content-Type',
             b'application/x-%s-advertisement' % self.factory.command)
+        super(HTTPPackClientRefsProtocol, self).startGoodResponse()
 
     def backendConnected(self):
         HTTPPackClientProtocol.backendConnected(self)
@@ -221,10 +237,11 @@ class HTTPPackClientCommandProtocol(HTTPPackClientProtocol):
 
     def startGoodResponse(self):
         """Prepare the HTTP response for forwarding from the backend."""
-        self.factory.http_request.setResponseCode(http.OK)
-        self.factory.http_request.setHeader(
-            b'Content-Type',
-            b'application/x-%s-result' % self.factory.command)
+        if self.getProtocolVersion() != b'2':
+            self.factory.http_request.setResponseCode(http.OK)
+            self.factory.http_request.setHeader(
+                b'Content-Type',
+                b'application/x-%s-result' % self.factory.command)
 
 
 class HTTPPackClientFactory(protocol.ClientFactory):
@@ -280,8 +297,11 @@ class BaseSmartHTTPResource(resource.Resource):
         by the virt service, if any.
         """
         params = {
+            b'turnip-frontend': b'http',
             b'turnip-can-authenticate': b'yes',
             b'turnip-request-id': str(uuid.uuid4()),
+            b'version': six.ensure_binary(
+                get_protocol_version_from_request(request))
             }
         authenticated_params = yield self.authenticateUser(request)
         for key, value in authenticated_params.items():
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 27bc26e..c811b2a 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -815,16 +815,19 @@ class TestSmartHTTPFrontendWithAuthFunctional(TestSmartHTTPFrontendFunctional):
         test_root = self.useFixture(TempDir()).path
         clone = os.path.join(test_root, 'clone')
         yield self.assertCommandSuccess((b'git', b'clone', self.ro_url, clone))
+        expected_requests = 1 if self.protocol_version == '1' else 2
         self.assertEqual(
-            [(b'test-user', b'test-password')], self.virtinfo.authentications)
-        self.assertThat(self.virtinfo.translations, MatchesListwise([
-            MatchesListwise([
+            [(b'test-user', b'test-password')] * expected_requests,
+            self.virtinfo.authentications)
+        self.assertEqual(expected_requests, len(self.virtinfo.translations))
+        for translation in self.virtinfo.translations:
+            self.assertThat(translation, MatchesListwise([
                 Equals(b'/test'), Equals(b'read'),
                 MatchesDict({
                     b'can-authenticate': Is(True),
                     b'request-id': Not(Is(None)),
-                    b'user': Equals(b'test-user'),
-                    })])]))
+                    b'user': Equals(b'test-user')})
+                ]))
 
     @defer.inlineCallbacks
     def test_authenticated_push(self):
diff --git a/turnip/pack/tests/test_git.py b/turnip/pack/tests/test_git.py
index b69049c..ec8876f 100644
--- a/turnip/pack/tests/test_git.py
+++ b/turnip/pack/tests/test_git.py
@@ -296,6 +296,7 @@ class TestPackBackendProtocol(TestCase):
         self.proto.requestReceived(
             b'git-upload-pack', b'/foo.git', OrderedDict([
                 (b'turnip-x', b'yes'),
+                (b'turnip-frontend', b'http'),
                 (b'turnip-request-id', b'123'),
                 (b'version', b'2'),
                 (b'command', b'ls-refs'),
@@ -313,7 +314,7 @@ class TestPackBackendProtocol(TestCase):
         stdin_content = (
             b'0014command=ls-refs\n'
             b'0015agent=git/2.25.1\n'
-            b'00010014command ls-refs\n'
+            b'0001'
             b'0009peel\n'
             b'000csymrefs\n'
             b'0014ref-prefix HEAD\n'
diff --git a/turnip/pack/tests/test_http.py b/turnip/pack/tests/test_http.py
index 26e2754..9c547c1 100644
--- a/turnip/pack/tests/test_http.py
+++ b/turnip/pack/tests/test_http.py
@@ -24,7 +24,10 @@ from turnip.pack import (
     helpers,
     http,
     )
+from turnip.pack.helpers import encode_packet
 from turnip.pack.tests.fake_servers import FakeVirtInfoService
+from turnip.tests.compat import mock
+from turnip.version_info import version_info
 
 
 class LessDummyRequest(requesthelper.DummyRequest):
@@ -74,12 +77,14 @@ class FakeRoot(object):
 
     def __init__(self):
         self.backend_transport = None
+        self.client_factory = None
         self.backend_connected = defer.Deferred()
 
     def authenticateWithPassword(self, user, password):
         return {}
 
     def connectToBackend(self, client_factory):
+        self.client_factory = client_factory
         self.backend_transport = testing.StringTransportWithDisconnection()
         p = client_factory.buildProtocol(None)
         self.backend_transport.protocol = p
@@ -186,6 +191,37 @@ class TestSmartHTTPRefsResource(ErrorTestMixin, TestCase):
             'And I am raw, since we got a good packet to start with.',
             self.request.value)
 
+    @defer.inlineCallbacks
+    def test_good_v2_included_version_and_capabilities(self):
+        self.request.requestHeaders.addRawHeader("Git-Protocol", "version=2")
+        yield self.performRequest(
+            helpers.encode_packet(b'I am git protocol data.') +
+            b'And I am raw, since we got a good packet to start with.')
+        self.assertEqual(200, self.request.responseCode)
+        self.assertEqual(self.root.client_factory.params, {
+            'version': '2',
+            'turnip-frontend': 'http',
+            'turnip-advertise-refs': 'yes',
+            'turnip-can-authenticate': 'yes',
+            'turnip-request-id': mock.ANY,
+            'turnip-stateless-rpc': 'yes'})
+
+        ver = version_info["revision_id"]
+        capabilities = (
+            encode_packet(b'version 2\n') +
+            encode_packet(b'agent=turnip/%s\n' % ver) +
+            encode_packet(b'ls-refs\n') +
+            encode_packet(b'fetch=shallow\n') +
+            encode_packet(b'server-option\n') +
+            b'0000'
+            )
+        self.assertEqual(
+            capabilities +
+            '001e# service=git-upload-pack\n'
+            '0000001bI am git protocol data.'
+            'And I am raw, since we got a good packet to start with.',
+            self.request.value)
+
 
 class TestSmartHTTPCommandResource(ErrorTestMixin, TestCase):