Fix parent rewrite bug in schema1 manifest code and add a bunch more tests

Before this change, if you ended up writing a middle layer whose parent is not in the database, the manifest would fail to rewrite. We now just lookup the parent image in the manifest given to us, ignoring whether it is in the database or not (as it doesn't actually matter if not present; it'll be created if necessary).

image/docker/schema1.py

@@ -259,13 +259,9 @@ class DockerSchema1Manifest(object):
       updated_id_map[extracted_v1_metadata.image_id] = working_image_id
 
       # Lookup the parent image for the layer, if any.
-      parent_image_id = None
+      parent_image_id = extracted_v1_metadata.parent_image_id
-      if extracted_v1_metadata.parent_image_id is not None:
+      if parent_image_id is not None:
-        parent_image = images_map.get(extracted_v1_metadata.parent_image_id, None)
+        parent_image_id = updated_id_map.get(parent_image_id, parent_image_id)
-        if parent_image is None:
-          raise MalformedSchema1Manifest('parent not found with image ID: %s' %
-                                         extracted_v1_metadata.parent_image_id)
-        parent_image_id = updated_id_map.get(parent_image.image_id, parent_image.image_id)
 
       # Synthesize and store the v1 metadata in the db.
       v1_metadata_json = layer.raw_v1_metadata
@@ -285,7 +281,6 @@ class DockerSchema1Manifest(object):
         content_checksum=digest_str,
       )
 
-      images_map[updated_image.image_id] = updated_image
       yield updated_image
 
 

test/registry_tests.py

@@ -38,7 +38,7 @@ from endpoints.csrf import generate_csrf_token
 from endpoints.v1 import v1_bp
 from endpoints.v2 import v2_bp
 from endpoints.verbs import verbs
-from image.docker.schema1 import DockerSchema1ManifestBuilder
+from image.docker.schema1 import DockerSchema1ManifestBuilder, DockerSchema1Manifest
 from initdb import wipe_database, initialize_database, populate_database
 from jsonschema import validate as validate_schema
 from util.security.registry_jwt import decode_bearer_header
@@ -72,6 +72,14 @@ def set_fakestorage_directdownload(enabled):
   return 'OK'
 
 
+@testbp.route('/deleteimage/<image_id>', methods=['POST'])
+def delete_image(image_id):
+  image = Image.get(docker_image_id=image_id)
+  image.docker_image_id = 'DELETED'
+  image.save()
+  return 'OK'
+
+
 @testbp.route('/storagerepentry/<image_id>', methods=['GET'])
 def get_storage_replication_entry(image_id):
   image = Image.get(docker_image_id=image_id)
@@ -342,7 +350,7 @@ class V1RegistryPushMixin(V1RegistryMixin):
   push_version = 'v1'
 
   def do_push(self, namespace, repository, username, password, images=None, expect_failure=None,
-              munge_shas=False, tag_names=None):
+              munge_shas=[], tag_names=None, head_check=True):
     images = images or self._get_default_images()
     auth = (username, password)
     repo_name = _get_repo_name(namespace, repository)
@@ -399,7 +407,7 @@ class V1RegistryPullMixin(V1RegistryMixin):
   pull_version = 'v1'
 
   def do_pull(self, namespace, repository, username=None, password='password', expect_failure=None,
-              images=None, munge_shas=False):
+              images=None, munge_shas=[]):
     images = images or self._get_default_images()
     repo_name = _get_repo_name(namespace, repository)
 
@@ -422,10 +430,11 @@ class V1RegistryPullMixin(V1RegistryMixin):
     tags_result = json.loads(self.conduct('GET', prefix + 'tags', auth='sig').text)
     self.assertEquals(1, len(tags_result.values()))
 
-    # Ensure we do (or do not) have a matching image ID.
     tag_image_id = tags_result['latest']
-    known_ids = [item['id'] for item in images]
+    if not munge_shas:
-    self.assertEquals(not munge_shas, tag_image_id in known_ids)
+      # Ensure we have a matching image ID.
+      known_ids = [item['id'] for item in images]
+      self.assertTrue(tag_image_id in known_ids)
 
     # Retrieve the ancestry of the tag image.
     image_prefix = '/v1/images/%s/' % tag_image_id
@@ -527,7 +536,8 @@ class V2RegistryPushMixin(V2RegistryMixin):
   push_version = 'v2'
 
   def do_push(self, namespace, repository, username, password, images=None, tag_names=None,
-              cancel=False, invalid=False, expect_failure=None, scopes=None, munge_shas=False):
+              cancel=False, invalid=False, expect_failure=None, scopes=None, munge_shas=[],
+              head_check=True):
     images = images or self._get_default_images()
     repo_name = _get_repo_name(namespace, repository)
 
@@ -551,8 +561,9 @@ class V2RegistryPushMixin(V2RegistryMixin):
     manifests = {}
     full_contents = {}
     for image_data in reversed(images):
-      full_contents[image_data['id']] = _get_full_contents(image_data,
+      image_id = image_data['id']
-                                                           additional_fields=munge_shas)
+      full_contents[image_id] = _get_full_contents(image_data,
+                                                   additional_fields=image_id in munge_shas)
 
     # Build a fake manifest.
     for tag_name in tag_names:
@@ -577,8 +588,10 @@ class V2RegistryPushMixin(V2RegistryMixin):
 
       # Layer data should not yet exist.
       checksum = 'sha256:' + hashlib.sha256(layer_bytes).hexdigest()
-      self.conduct('HEAD', '/v2/%s/blobs/%s' % (repo_name, checksum),
+
-                   expected_code=404, auth='jwt')
+      if head_check:
+        self.conduct('HEAD', '/v2/%s/blobs/%s' % (repo_name, checksum),
+                     expected_code=404, auth='jwt')
 
       # If we expected a non-404 status code, then the HEAD operation has failed and we cannot
       # continue performing the push.
@@ -657,7 +670,7 @@ class V2RegistryPullMixin(V2RegistryMixin):
   pull_version = 'v2'
 
   def do_pull(self, namespace, repository, username=None, password='password', expect_failure=None,
-              manifest_id=None, images=None, munge_shas=False):
+              manifest_id=None, images=None, munge_shas=[]):
     images = images or self._get_default_images()
     repo_name = _get_repo_name(namespace, repository)
 
@@ -709,7 +722,7 @@ class V2RegistryPullMixin(V2RegistryMixin):
     for image in images:
       self.assertIn(image['id'], found_v1_layers)
 
-    return blobs
+    return blobs, manifest_data
 
 
 class V1RegistryLoginMixin(object):
@@ -749,6 +762,120 @@ class V2RegistryLoginMixin(object):
 
 
 class RegistryTestsMixin(object):
+  def test_middle_layer_different_sha(self):
+    if self.push_version == 'v1':
+      # No SHAs to munge in V1.
+      return
+
+    images = [
+      {
+        'id': 'rootid',
+        'contents': 'The root image',
+      },
+      {
+        'id': 'baseid',
+        'contents': 'The base image',
+      },
+    ]
+
+    # Push a new repository with two layers.
+    self.do_push('public', 'newrepo', 'public', 'password', images=images)
+
+    # Pull the repository to verify.
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images)
+
+    # Push again, munging the middle layer to ensure it gets assigned a different ID.
+    images = [
+      {
+        'id': 'rootid',
+        'contents': 'The root image',
+      },
+      {
+        'id': 'baseid',
+        'contents': 'The base image',
+      },
+      {
+        'id': 'latestid',
+        'contents': 'the latest image',
+        'parent': 'baseid',
+      },
+    ]
+
+    munged_shas = ['baseid']
+
+    # Push the repository.
+    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas,
+                 head_check=False)
+
+    # Pull the repository to verify.
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas)
+
+    # Ensures we don't hit weird tag overwrite issues.
+    time.sleep(1)
+
+    # Delete the baseid image.
+    self.conduct('POST', '/__test/deleteimage/baseid')
+
+    images = [
+      {
+        'id': 'rootid',
+        'contents': 'The root image',
+      },
+      {
+        'id': 'baseid',
+        'contents': 'The base image',
+      },
+      {
+        'id': 'latestid',
+        'contents': 'the latest image',
+        'parent': 'baseid',
+      },
+    ]
+
+    # Push the repository again, this time munging the root layer. Since the baseid does not exist
+    # anymore (since we deleted it above), this will have to look in the layer metadata itself
+    # to work (which didn't before).
+    munged_shas = ['rootid']
+    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas,
+                 head_check=False)
+
+    # Pull the repository to verify.
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas)
+
+
+  def test_push_same_ids_different_base_sha(self):
+    if self.push_version == 'v1':
+      # No SHAs to munge in V1.
+      return
+
+    images = [
+      {
+        'id': 'baseid',
+        'contents': 'The base image',
+      },
+      {
+        'id': 'latestid',
+        'contents': 'the latest image',
+        'parent': 'baseid',
+      },
+    ]
+
+    munged_shas = ['baseid']
+
+    # Push a new repository.
+    self.do_push('public', 'newrepo', 'public', 'password', images=images)
+
+    # Pull the repository.
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images)
+
+    # Push a the repository again, but with different SHAs.
+    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas,
+                 head_check=False)
+
+    # Pull the repository.
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas)
+
+
   def test_push_same_ids_different_sha(self):
     if self.push_version == 'v1':
       # No SHAs to munge in V1.
@@ -766,6 +893,8 @@ class RegistryTestsMixin(object):
       },
     ]
 
+    munged_shas = ['latestid']
+
     # Push a new repository.
     self.do_push('public', 'newrepo', 'public', 'password', images=images)
 
@@ -773,10 +902,44 @@ class RegistryTestsMixin(object):
     self.do_pull('public', 'newrepo', 'public', 'password', images=images)
 
     # Push a the repository again, but with different SHAs.
-    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=True)
+    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas,
+                 head_check=False)
 
     # Pull the repository.
-    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=True)
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas)
+
+
+  def test_push_same_ids_different_sha_both_layers(self):
+    if self.push_version == 'v1':
+      # No SHAs to munge in V1.
+      return
+
+    images = [
+      {
+        'id': 'baseid',
+        'contents': 'The base image',
+      },
+      {
+        'id': 'latestid',
+        'contents': 'the latest image',
+        'parent': 'baseid',
+      },
+    ]
+
+    munged_shas = ['baseid', 'latestid']
+
+    # Push a new repository.
+    self.do_push('public', 'newrepo', 'public', 'password', images=images)
+
+    # Pull the repository.
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images)
+
+    # Push a the repository again, but with different SHAs.
+    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas,
+                 head_check=False)
+
+    # Pull the repository.
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas)
 
 
   def test_push_same_ids_different_sha_with_unicode(self):
@@ -797,6 +960,8 @@ class RegistryTestsMixin(object):
       },
     ]
 
+    munged_shas = ['latestid', 'baseid']
+
     # Push a new repository.
     self.do_push('public', 'newrepo', 'public', 'password', images=images)
 
@@ -804,10 +969,11 @@ class RegistryTestsMixin(object):
     self.do_pull('public', 'newrepo', 'public', 'password', images=images)
 
     # Push a the repository again, but with different SHAs.
-    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=True)
+    self.do_push('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas,
+                 head_check=False)
 
     # Pull the repository.
-    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=True)
+    self.do_pull('public', 'newrepo', 'public', 'password', images=images, munge_shas=munged_shas)
 
 
   def test_push_pull_logging(self):
@@ -1363,7 +1529,7 @@ class V2RegistryTests(V2RegistryPullMixin, V2RegistryPushMixin, RegistryTestsMix
     self.do_push('devtable', 'newrepo', 'devtable', 'password', images=images)
 
     # Pull the image back and verify the contents.
-    blobs = self.do_pull('devtable', 'newrepo', 'devtable', 'password', images=images)
+    blobs, _ = self.do_pull('devtable', 'newrepo', 'devtable', 'password', images=images)
     self.assertEquals(len(blobs.items()), 1)
     self.assertEquals(blobs.items()[0][1], contents)
 
@@ -1384,7 +1550,7 @@ class V2RegistryTests(V2RegistryPullMixin, V2RegistryPushMixin, RegistryTestsMix
     self.do_push('devtable', 'newrepo', 'devtable', 'password', images=images)
 
     # Pull the image back and verify the contents.
-    blobs = self.do_pull('devtable', 'newrepo', 'devtable', 'password', images=images)
+    blobs, _ = self.do_pull('devtable', 'newrepo', 'devtable', 'password', images=images)
     self.assertEquals(len(blobs.items()), 1)
     self.assertEquals(blobs.items()[0][1], contents)
 
@@ -1406,7 +1572,7 @@ class V2RegistryTests(V2RegistryPullMixin, V2RegistryPushMixin, RegistryTestsMix
     self.do_push('devtable', 'newrepo', 'devtable', 'password', images=images)
 
     # Pull the image back and verify the contents.
-    blobs = self.do_pull('devtable', 'newrepo', 'devtable', 'password', images=images)
+    blobs, _ = self.do_pull('devtable', 'newrepo', 'devtable', 'password', images=images)
     self.assertEquals(len(blobs.items()), 1)
     self.assertEquals(blobs.items()[0][1], contents)
 
@@ -1583,8 +1749,8 @@ class TorrentTestMixin(V2RegistryPullMixin):
     self.do_push('devtable', 'newrepo', 'devtable', 'password', images=initial_images)
 
     # Retrieve the manifest for the tag.
-    blobs = self.do_pull('devtable', 'newrepo', 'devtable', 'password', manifest_id='latest',
+    blobs, _ = self.do_pull('devtable', 'newrepo', 'devtable', 'password', manifest_id='latest',
-                         images=initial_images)
+                            images=initial_images)
     self.assertEquals(1, len(list(blobs.keys())))
     blobsum = list(blobs.keys())[0]