Fix bug around pushing manifest lists that refer to the same manifest twice as children

data/model/oci/manifest.py

@@ -59,7 +59,7 @@ def _create_manifest(repository_id, manifest_interface_instance, storage):
   # Load, parse and get/create the child manifests, if any.
   retriever = RepositoryContentRetriever.for_repository(repository_id, storage)
   child_manifest_refs = manifest_interface_instance.child_manifests(retriever)
-  child_manifest_rows = []
+  child_manifest_rows = {}
   child_manifest_label_dicts = []
 
   if child_manifest_refs is not None:
@@ -90,7 +90,7 @@ def _create_manifest(repository_id, manifest_interface_instance, storage):
         logger.error('Could not get/create child manifest')
         return None
 
-      child_manifest_rows.append(child_manifest_info.manifest)
+      child_manifest_rows[child_manifest_info.manifest.digest] = child_manifest_info.manifest
       child_manifest_label_dicts.append(labels)
 
   # Ensure all the blobs in the manifest exist.
@@ -147,7 +147,7 @@ def _create_manifest(repository_id, manifest_interface_instance, storage):
     if child_manifest_rows:
       children_to_insert = [dict(manifest=manifest, child_manifest=child_manifest,
                                  repository=repository_id)
-                            for child_manifest in child_manifest_rows]
+                            for child_manifest in child_manifest_rows.values()]
       ManifestChild.insert_many(children_to_insert).execute()
 
   # Define the labels for the manifest (if any).

data/model/oci/test/test_oci_manifest.py

@@ -241,6 +241,80 @@ def test_get_or_create_manifest_list(initialized_db):
   assert child_manifests[v2_manifest.digest].media_type.name == v2_manifest.media_type
 
 
+def test_get_or_create_manifest_list_duplicate_child_manifest(initialized_db):
+  repository = create_repository('devtable', 'newrepo', None)
+
+  expected_labels = {
+    'Foo': 'Bar',
+    'Baz': 'Meh',
+  }
+
+  layer_json = json.dumps({
+    'id': 'somelegacyid',
+    'config': {
+      'Labels': expected_labels,
+    },
+    "rootfs": {
+      "type": "layers",
+      "diff_ids": []
+    },
+    "history": [
+      {
+        "created": "2018-04-03T18:37:09.284840891Z",
+        "created_by": "do something",
+      },
+    ],
+  })
+
+  # Create a legacy image.
+  find_create_or_link_image('somelegacyid', repository, 'devtable', {}, 'local_us')
+
+  # Add a blob containing the config.
+  _, config_digest = _populate_blob(layer_json)
+
+  # Add a blob of random data.
+  random_data = 'hello world'
+  _, random_digest = _populate_blob(random_data)
+
+  # Build the manifest.
+  v2_builder = DockerSchema2ManifestBuilder()
+  v2_builder.set_config_digest(config_digest, len(layer_json))
+  v2_builder.add_layer(random_digest, len(random_data))
+  v2_manifest = v2_builder.build()
+
+  # Write the manifest.
+  v2_created = get_or_create_manifest(repository, v2_manifest, storage)
+  assert v2_created
+  assert v2_created.manifest.digest == v2_manifest.digest
+
+  # Build the manifest list, with the child manifest repeated.
+  list_builder = DockerSchema2ManifestListBuilder()
+  list_builder.add_manifest(v2_manifest, 'amd64', 'linux')
+  list_builder.add_manifest(v2_manifest, 'amd32', 'linux')
+  manifest_list = list_builder.build()
+
+  # Write the manifest list, which should also write the manifests themselves.
+  created_tuple = get_or_create_manifest(repository, manifest_list, storage)
+  assert created_tuple is not None
+
+  created_list = created_tuple.manifest
+  assert created_list
+  assert created_list.media_type.name == manifest_list.media_type
+  assert created_list.digest == manifest_list.digest
+
+  # Ensure the child manifest links exist.
+  child_manifests = {cm.child_manifest.digest: cm.child_manifest
+                     for cm in ManifestChild.select().where(ManifestChild.manifest == created_list)}
+  assert len(child_manifests) == 1
+  assert v2_manifest.digest in child_manifests
+  assert child_manifests[v2_manifest.digest].media_type.name == v2_manifest.media_type
+
+  # Try to create again and ensure we get back the same manifest list.
+  created2_tuple = get_or_create_manifest(repository, manifest_list, storage)
+  assert created2_tuple is not None
+  assert created2_tuple.manifest == created_list
+
+
 def test_get_or_create_manifest_with_remote_layers(initialized_db):
   repository = create_repository('devtable', 'newrepo', None)
 

test/registry/registry_tests.py

@@ -1493,3 +1493,41 @@ def test_push_pull_manifest_list_missing_manifest(v22_protocol, basic_images, li
                          [], blobs,
                          credentials=credentials, options=options,
                          expected_failure=Failures.INVALID_MANIFEST)
+
+
+def test_push_pull_manifest_list_again(v22_protocol, basic_images, different_images,
+                                       liveserver_session, app_reloader, data_model):
+  """ Test: Push a new tag with a manifest list containing two manifests, push it again, and pull
+      it.
+  """
+  if data_model != 'oci_model':
+    return
+
+  credentials = ('devtable', 'password')
+  options = ProtocolOptions()
+
+  # Build the manifests that will go in the list.
+  blobs = {}
+
+  first_manifest = v22_protocol.build_schema2(basic_images, blobs, options)
+  second_manifest = v22_protocol.build_schema2(different_images, blobs, options)
+
+  # Create and push the manifest list.
+  builder = DockerSchema2ManifestListBuilder()
+  builder.add_manifest(first_manifest, 'amd64', 'linux')
+  builder.add_manifest(second_manifest, 'arm', 'linux')
+  manifestlist = builder.build()
+
+  v22_protocol.push_list(liveserver_session, 'devtable', 'newrepo', 'latest', manifestlist,
+                         [first_manifest, second_manifest], blobs,
+                         credentials=credentials, options=options)
+
+  # Push the manifest list again. This should more or less no-op.
+  options.skip_head_checks = True
+  v22_protocol.push_list(liveserver_session, 'devtable', 'newrepo', 'latest', manifestlist,
+                         [first_manifest, second_manifest], blobs,
+                         credentials=credentials, options=options)
+
+  # Pull and verify the manifest list.
+  v22_protocol.pull_list(liveserver_session, 'devtable', 'newrepo', 'latest', manifestlist,
+                         credentials=credentials, options=options)