From 54904cfd6ef65ff186cea5a43dd14a00ea6e82a7 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Mon, 19 Nov 2018 18:26:22 +0200 Subject: [PATCH] Fix bug around pushing manifest lists that refer to the same manifest twice as children --- data/model/oci/manifest.py | 6 +- data/model/oci/test/test_oci_manifest.py | 74 ++++++++++++++++++++++++ test/registry/registry_tests.py | 38 ++++++++++++ 3 files changed, 115 insertions(+), 3 deletions(-) diff --git a/data/model/oci/manifest.py b/data/model/oci/manifest.py index e1023881b..d094e2ac5 100644 --- a/data/model/oci/manifest.py +++ b/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). diff --git a/data/model/oci/test/test_oci_manifest.py b/data/model/oci/test/test_oci_manifest.py index 60f9f1c8e..e85657ab6 100644 --- a/data/model/oci/test/test_oci_manifest.py +++ b/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) diff --git a/test/registry/registry_tests.py b/test/registry/registry_tests.py index 37039a592..8e915ea96 100644 --- a/test/registry/registry_tests.py +++ b/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)