Artifact push URLs must redirect to the corresponding pull URL

This change is part of issue #112 ("Artifact configuration is confusing
and fragile, need canonical push/pull urls.")

It updates the bst-artifact-receive program to send a pull URL to
clients who access it over SSH.

This requires extra configuration in the artifact cache server, so that
it knows the correct pull URL. Versions of BuildStream which contain
this commit will not be able to communicate over SSH with artifact
caches that have not been updated to the same version.

Currently this is just used to double-check user configuration, but we
can now simplify the user facing configuration for artifact caches
completely.

3 files changed, 58 insertions, 16 deletions

diff --git a/buildstream/_artifactcache/ostreecache.py b/buildstream/_artifactcache/ostreecache.py
index e2e623e78..6b2411e7f 100644
--- a/buildstream/_artifactcache/ostreecache.py
+++ b/buildstream/_artifactcache/ostreecache.py
@@ -29,7 +29,7 @@ from ..element import _KeyStrength
 from .._ostree import OSTreeError
 
 from . import ArtifactCache
-from .pushreceive import check_push_connection
+from .pushreceive import initialize_push_connection
 from .pushreceive import push as push_artifact
 from .pushreceive import PushException
 
@@ -82,8 +82,12 @@ class OSTreeCache(ArtifactCache):
     def preflight(self):
         if self.can_push() and not self.artifact_push.startswith("/"):
             try:
-                check_push_connection(self.artifact_push,
-                                      self.artifact_push_port)
+                pull_url = initialize_push_connection(self.artifact_push,
+                                                      self.artifact_push_port)
+                if pull_url != self.artifact_pull:
+                    raise ArtifactError(
+                        "This cache reports its pull URL as {}, but user "
+                        "configuration specifies {}.".format(pull_url, self.artifact_pull))
             except PushException as e:
                 raise ArtifactError("BuildStream will be unable to push artifacts "
                                     "to the shared cache: {}".format(e))
diff --git a/buildstream/_artifactcache/pushreceive.py b/buildstream/_artifactcache/pushreceive.py
index 9aef842a8..18905fed3 100644
--- a/buildstream/_artifactcache/pushreceive.py
+++ b/buildstream/_artifactcache/pushreceive.py
@@ -136,8 +136,8 @@ class PushMessageWriter(object):
         self.file.flush()
 
     def send_hello(self):
-        # The 'hello' message is used to check connectivity, and is actually
-        # an empty info request in order to keep the receiver code simple.
+        # The 'hello' message is used to check connectivity and discover the
+        # cache's pull URL. It's actually transmitted as an empty info request.
         args = {
             'mode': GLib.Variant('i', 0),
             'refs': GLib.Variant('a{ss}', {})
@@ -145,7 +145,7 @@ class PushMessageWriter(object):
         command = PushCommand(PushCommandType.info, args)
         self.write(command)
 
-    def send_info(self, repo, refs):
+    def send_info(self, repo, refs, pull_url=None):
         cmdtype = PushCommandType.info
         mode = repo.get_mode()
 
@@ -161,6 +161,12 @@ class PushMessageWriter(object):
             'mode': GLib.Variant('i', mode),
             'refs': GLib.Variant('a{ss}', ref_map)
         }
+
+        # The server sends this so clients can discover the correct pull URL
+        # for this cache without requiring end-users to specify it.
+        if pull_url:
+            args['pull_url'] = GLib.Variant('s', pull_url)
+
         command = PushCommand(cmdtype, args)
         self.write(command)
 
@@ -511,9 +517,16 @@ class OSTreePusher(object):
         return self.close()
 
 
+# OSTreeReceiver is on the receiving end of an OSTree push.
+#
+# Args:
+#     repopath (str): On-disk location of the receiving repository.
+#     pull_url (str): Redirection for clients who want to pull, not push.
+#
 class OSTreeReceiver(object):
-    def __init__(self, repopath):
+    def __init__(self, repopath, pull_url):
         self.repopath = repopath
+        self.pull_url = pull_url
 
         if self.repopath is None:
             self.repo = OSTree.Repo.new_default()
@@ -552,7 +565,8 @@ class OSTreeReceiver(object):
         remote_refs = args['refs']
 
         # Send info back
-        self.writer.send_info(self.repo, list(remote_refs.keys()))
+        self.writer.send_info(self.repo, list(remote_refs.keys()),
+                              pull_url=self.pull_url)
 
         # Wait for update or done command
         cmdtype, args = self.reader.receive([PushCommandType.update,
@@ -606,19 +620,28 @@ class OSTreeReceiver(object):
         return 0
 
 
-# check_push_connection()
+# initialize_push_connection()
+#
+# Test that we can connect to the remote bst-artifact-receive program, and
+# receive the pull URL for this artifact cache.
 #
-# Test that we can connect to the remote bst-artifact-receive program.
 # We don't want to make the user wait until the first artifact has been built
-# to discover that they actually cannot push.
+# to discover that they actually cannot push, so this should be called early.
+#
+# The SSH push protocol doesn't allow pulling artifacts. We don't want to
+# require users to specify two URLs for a single cache, so we have the push
+# protocol return the corresponding pull URL as part of the 'hello' response.
 #
 # Args:
 #   remote: The ssh remote url to push to
 #   remote_port: The ssh port at the remote url
 #
+# Returns:
+#   (str): The URL that should be used for pushing to this cache.
+#
 # Raises:
 #   PushException if there was an issue connecting to the remote.
-def check_push_connection(remote, remote_port):
+def initialize_push_connection(remote, remote_port):
     remote_host, remote_user, remote_repo, remote_port = parse_remote_location(remote, remote_port)
     ssh_cmd = ssh_commandline(remote_host, remote_user, remote_port)
 
@@ -632,7 +655,18 @@ def check_push_connection(remote, remote_port):
                            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
     writer = PushMessageWriter(ssh.stdin)
+    reader = PushMessageReader(ssh.stdout)
+
     writer.send_hello()
+    args = reader.receive_info()
+
+    if 'pull_url' in args:
+        pull_url = args['pull_url']
+    else:
+        raise PushException(
+            "Remote cache did not tell us its pull URL. This cache probably "
+            "requires updating to a newer version of `bst-artifact-receive`.")
+
     writer.send_done()
 
     ssh.wait()
@@ -640,6 +674,8 @@ def check_push_connection(remote, remote_port):
         error = ssh.stderr.read().decode('unicode-escape')
         raise PushException(error)
 
+    return pull_url
+
 
 # push()
 #
@@ -691,8 +727,10 @@ def push(repo, remote, remote_port, branches, output):
 @click.command(short_help="Receive pushed artifacts over ssh")
 @click.option('--verbose', '-v', is_flag=True, default=False, help="Verbose mode")
 @click.option('--debug', '-d', is_flag=True, default=False, help="Debug mode")
+@click.option('--pull-url', type=str, required=True,
+              help="Clients who try to pull over SSH will be redirected here")
 @click.argument('repo')
-def receive_main(verbose, debug, repo):
+def receive_main(verbose, debug, pull_url, repo):
     """A BuildStream sister program for receiving artifacts send to a shared artifact cache
     """
     loglevel = logging.WARNING
@@ -703,5 +741,5 @@ def receive_main(verbose, debug, repo):
     logging.basicConfig(format='%(module)s: %(levelname)s: %(message)s',
                         level=loglevel, stream=sys.stderr)
 
-    receiver = OSTreeReceiver(repo)
+    receiver = OSTreeReceiver(repo, pull_url)
     return receiver.run()
diff --git a/doc/source/artifacts.rst b/doc/source/artifacts.rst
index 8ad2f600c..575f51a51 100644
--- a/doc/source/artifacts.rst
+++ b/doc/source/artifacts.rst
@@ -122,8 +122,8 @@ For this you will want something like the following in your ``/etc/ssh/sshd_conf
 	# bst-artifact-receive program, note that the full
 	# command must be specified here; 'artifacts' is
 	# the HOME relative path to the artifact cache.
-	#
-        ForceCommand bst-artifact-receive --verbose artifacts
+	# The exact pull URL must also be specified.
+        ForceCommand bst-artifact-receive --pull-url https://example.com/artifacts --verbose artifacts
 
 
 Summary file updates