launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad-buildd:oci-can-be-layer-symlinks into launchpad-buildd:master.

Commit message:
Handle layer symlinks in OCI builds

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1925791 in Launchpad itself: "OCI recipe build failure with "cannot extract (sym)link as file object""
  https://bugs.launchpad.net/launchpad/+bug/1925791

For more details, see:
https://code.launchpad.net/~twom/launchpad-buildd/+git/launchpad-buildd/+merge/401710

Occasionally, docker can produce a saved image tarball that contains symlinks between layers.
This causes the buildd to fail to extract from the image as you can't dereference in a streamed tarfile object.

Instead, keep track of symlinks and then use the paths that are available to copy the existing file to the target place.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad-buildd:oci-can-be-layer-symlinks into launchpad-buildd:master.

diff --git a/lpbuildd/oci.py b/lpbuildd/oci.py
index cf8d1d7..1196c1d 100644
--- a/lpbuildd/oci.py
+++ b/lpbuildd/oci.py
@@ -8,6 +8,7 @@ __metaclass__ = type
 import hashlib
 import json
 import os
+import shutil
 import tarfile
 import tempfile
 
@@ -161,13 +162,14 @@ class OCIBuildManager(SnapBuildProxyMixin, DebianBuildManager):
             proc = self.backend.run(
                 ['docker', 'save', self.name],
                 get_output=True, return_process=True)
-            tar = tarfile.open(fileobj=proc.stdout, mode="r|")
+            tar = tarfile.open(fileobj=proc.stdout, mode="r|", dereference=True)
         except Exception as e:
             self._builder.log("Unable to save image: {}".format(e))
             raise
 
         current_dir = ''
         directory_tar = None
+        symlinks = []
         try:
             # The tarfile is a stream and must be processed in order
             for file in tar:
@@ -183,7 +185,14 @@ class OCIBuildManager(SnapBuildProxyMixin, DebianBuildManager):
                     directory_tar = tarfile.open(
                         os.path.join(
                             extract_path, '{}.tar.gz'.format(file.name)),
-                        'w|gz')
+                        'w|gz', dereference=True)
+                if file.issym():
+                    # symlinks can't be extracted or derefenced from a stream
+                    # as you can't seek backwards.
+                    # Work out what the symlink is referring to, then
+                    # we can deal with it later
+                    symlinks.append(file)
+                    continue
                 if current_dir and file.name.endswith('layer.tar'):
                     # This is the actual layer data, we want to add it to
                     # the directory gzip
@@ -203,6 +212,21 @@ class OCIBuildManager(SnapBuildProxyMixin, DebianBuildManager):
             if directory_tar is not None:
                 directory_tar.close()
 
+        # deal with any symlinks we had
+        for symlink in symlinks:
+            # These are paths that finish in "<layer_id>/layer.tar"
+            # we want the directory name, which should always be
+            # the second component
+            source_name = os.path.join(
+                extract_path,
+                "{}.tar.gz".format(symlink.linkpath.split('/')[-2]))
+            target_name = os.path.join(
+                extract_path,
+                '{}.tar.gz'.format(symlink.name.split('/')[-2]))
+            # Do a copy to dereference the symlink
+            shutil.copy(source_name, target_name)
+
+
         # We need these mapping files
         sha_directory = tempfile.mkdtemp()
         # This can change depending on the kernel options / docker package
diff --git a/lpbuildd/tests/oci_tarball.py b/lpbuildd/tests/oci_tarball.py
index 70c055b..21ec013 100644
--- a/lpbuildd/tests/oci_tarball.py
+++ b/lpbuildd/tests/oci_tarball.py
@@ -17,14 +17,18 @@ class OCITarball:
     @property
     def config(self):
         return self._makeFile(
-            {"rootfs": {"diff_ids": ["sha256:diff1", "sha256:diff2"]}},
+            {"rootfs": {"diff_ids":
+                ["sha256:diff1", "sha256:diff2", "sha256:diff3"]}},
             'config.json')
 
     @property
     def manifest(self):
         return self._makeFile(
             [{"Config": "config.json",
-              "Layers": ["layer-1/layer.tar", "layer-2/layer.tar"]}],
+              "Layers": [
+                  "layer-1/layer.tar",
+                  "layer-2/layer.tar",
+                  "layer-3/layer.tar"]}],
             'manifest.json')
 
     @property
@@ -32,16 +36,32 @@ class OCITarball:
         return self._makeFile([], 'repositories')
 
     def layer_file(self, directory, layer_name):
+        layer_directory = os.path.join(directory, layer_name)
+        os.mkdir(layer_directory)
         contents = "{}-contents".format(layer_name)
         tarinfo = tarfile.TarInfo(contents)
         tarinfo.size = len(contents)
         layer_contents = io.BytesIO(contents.encode("UTF-8"))
         layer_tar_path = os.path.join(
-            directory, '{}.tar.gz'.format(layer_name))
-        layer_tar = tarfile.open(layer_tar_path, 'w:gz')
+            layer_directory, 'layer.tar')
+        layer_tar = tarfile.open(layer_tar_path, 'w')
         layer_tar.addfile(tarinfo, layer_contents)
         layer_tar.close()
-        return layer_tar_path
+        return layer_directory
+
+    def add_symlink_layer(self, directory, source_layer, target_layer):
+        target_layer_directory = os.path.join(directory, target_layer)
+        source_layer_directory = os.path.join(directory, source_layer)
+
+        target = os.path.join(target_layer_directory, "layer.tar")
+        source = os.path.join(source_layer_directory, "layer.tar")
+
+        os.mkdir(target_layer_directory)
+        os.symlink(
+            os.path.relpath(source, target_layer_directory),
+            os.path.join(target_layer_directory, "layer.tar")
+        )
+        return target_layer_directory
 
     def build_tar_file(self):
         tar_directory = tempfile.mkdtemp()
@@ -53,7 +73,12 @@ class OCITarball:
 
         for layer_name in ['layer-1', 'layer-2']:
             layer = self.layer_file(tar_directory, layer_name)
-            tar.add(layer, arcname='{}.tar.gz'.format(layer_name))
+            tar.add(layer, arcname=layer_name)
+
+        # add a symlink for 'layer-3'
+        target_layer = self.add_symlink_layer(
+            tar_directory, 'layer-2', 'layer-3')
+        tar.add(target_layer, arcname="layer-3")
 
         tar.close()
 
diff --git a/lpbuildd/tests/test_oci.py b/lpbuildd/tests/test_oci.py
index 15a4d64..78b0dbb 100644
--- a/lpbuildd/tests/test_oci.py
+++ b/lpbuildd/tests/test_oci.py
@@ -43,7 +43,8 @@ class MockBuildManager(OCIBuildManager):
 class MockOCITarSave():
     @property
     def stdout(self):
-        return io.open(OCITarball().build_tar_file(), 'rb')
+        tar_path = OCITarball().build_tar_file()
+        return io.open(tar_path, 'rb')
 
 
 class TestOCIBuildManagerIteration(TestCase):
@@ -147,6 +148,7 @@ class TestOCIBuildManagerIteration(TestCase):
             'manifest.json',
             'layer-1.tar.gz',
             'layer-2.tar.gz',
+            'layer-3.tar.gz',
             'digests.json',
             'config.json',
             ]
@@ -167,6 +169,11 @@ class TestOCIBuildManagerIteration(TestCase):
                 "source": "",
                 "digest": "testsha",
                 "layer_id": "layer-2"
+            },
+            "sha256:diff3": {
+                "source": "",
+                "digest": "testsha",
+                "layer_id": "layer-3"
             }
         }]
         self.assertEqual(digests_expected, json.loads(digests_contents))
@@ -233,6 +240,7 @@ class TestOCIBuildManagerIteration(TestCase):
             'manifest.json',
             'layer-1.tar.gz',
             'layer-2.tar.gz',
+            'layer-3.tar.gz',
             'digests.json',
             'config.json',
             ]
@@ -253,6 +261,11 @@ class TestOCIBuildManagerIteration(TestCase):
                 "source": "",
                 "digest": "testsha",
                 "layer_id": "layer-2"
+            },
+            "sha256:diff3": {
+                "source": "",
+                "digest": "testsha",
+                "layer_id": "layer-3"
             }
         }]
         self.assertEqual(digests_expected, json.loads(digests_contents))