From b48cd289260c517c0cb82a5e553a58b769fde085 Mon Sep 17 00:00:00 2001
From: git-hyagi <45576767+git-hyagi@users.noreply.github.com>
Date: Fri, 20 Sep 2024 14:04:38 -0300
Subject: [PATCH] Add os/arch/image_size fields to manifest model

closes: #1767
---
 CHANGES/1767.feature                          |  1 +
 .../commands/container-handle-image-data.py   | 42 +++++++++++++---
 ..._add_os_arch_image_size_manifest_fields.py | 49 +++++++++++++++++++
 pulp_container/app/models.py                  | 46 ++++++++++++++++-
 pulp_container/app/registry.py                |  2 +
 pulp_container/app/registry_api.py            | 13 ++---
 pulp_container/app/serializers.py             | 18 +++++++
 pulp_container/app/tasks/builder.py           |  6 ++-
 pulp_container/app/tasks/sync_stages.py       | 40 +++++++--------
 .../tests/functional/api/test_build_images.py | 10 +++-
 .../functional/api/test_pull_through_cache.py |  6 +++
 .../tests/functional/api/test_push_content.py |  4 ++
 .../tests/functional/api/test_sync.py         |  9 ++++
 pulp_container/tests/functional/conftest.py   | 11 +++++
 14 files changed, 219 insertions(+), 38 deletions(-)
 create mode 100644 CHANGES/1767.feature
 create mode 100644 pulp_container/app/migrations/0043_add_os_arch_image_size_manifest_fields.py

diff --git a/CHANGES/1767.feature b/CHANGES/1767.feature
new file mode 100644
index 000000000..5fb0797af
--- /dev/null
+++ b/CHANGES/1767.feature
@@ -0,0 +1 @@
+Added `architecture`, `os`, and `compressed_image_size` fields to Manifest.
diff --git a/pulp_container/app/management/commands/container-handle-image-data.py b/pulp_container/app/management/commands/container-handle-image-data.py
index 53c118933..facdbee22 100644
--- a/pulp_container/app/management/commands/container-handle-image-data.py
+++ b/pulp_container/app/management/commands/container-handle-image-data.py
@@ -40,12 +40,22 @@ def handle(self, *args, **options):
         manifests_updated_count = 0
 
         manifests_v1 = Manifest.objects.filter(
-            Q(media_type=MEDIA_TYPE.MANIFEST_V1), Q(data__isnull=True) | Q(type__isnull=True)
+            Q(media_type=MEDIA_TYPE.MANIFEST_V1),
+            Q(data__isnull=True)
+            | Q(type__isnull=True)
+            | Q(architecture__isnull=True)
+            | Q(os__isnull=True)
+            | Q(compressed_image_size__isnull=True),
         )
         manifests_updated_count += self.update_manifests(manifests_v1)
 
         manifests_v2 = Manifest.objects.filter(
-            Q(data__isnull=True) | Q(annotations={}, labels={}) | Q(type__isnull=True)
+            Q(data__isnull=True)
+            | Q(annotations={}, labels={})
+            | Q(type__isnull=True)
+            | Q(architecture__isnull=True)
+            | Q(os__isnull=True)
+            | Q(compressed_image_size__isnull=True)
         )
         manifests_v2 = manifests_v2.exclude(
             media_type__in=[MEDIA_TYPE.MANIFEST_LIST, MEDIA_TYPE.INDEX_OCI, MEDIA_TYPE.MANIFEST_V1]
@@ -79,6 +89,9 @@ def update_manifests(self, manifests_qs):
             "is_flatpak",
             "data",
             "type",
+            "os",
+            "architecture",
+            "compressed_image_size",
         ]
 
         for manifest in manifests_qs.iterator():
@@ -106,6 +119,7 @@ def update_manifests(self, manifests_qs):
         return manifests_updated_count
 
     def init_manifest(self, manifest):
+        updated = False
         if not manifest.data:
             manifest_artifact = manifest._artifacts.get()
             manifest_data, raw_bytes_data = get_content_data(manifest_artifact)
@@ -114,9 +128,25 @@ def init_manifest(self, manifest):
             if not (manifest.annotations or manifest.labels or manifest.type):
                 manifest.init_metadata(manifest_data)
 
+            if self.needs_os_arch_size_update(manifest):
+                self.init_manifest_os_arch_size(manifest)
             manifest._artifacts.clear()
-            return True
+            updated = True
 
-        elif not manifest.type:
-            return manifest.init_image_nature()
-        return False
+        if not manifest.type:
+            updated = manifest.init_image_nature()
+
+        if self.needs_os_arch_size_update(manifest):
+            self.init_manifest_os_arch_size(manifest)
+            updated = True
+
+        return updated
+
+    def needs_os_arch_size_update(self, manifest):
+        return manifest.media_type not in [MEDIA_TYPE.MANIFEST_LIST, MEDIA_TYPE.INDEX_OCI] and not (
+            manifest.architecture or manifest.os or manifest.compressed_image_size
+        )
+
+    def init_manifest_os_arch_size(self, manifest):
+        manifest.init_architecture_and_os()
+        manifest.init_compressed_image_size()
diff --git a/pulp_container/app/migrations/0043_add_os_arch_image_size_manifest_fields.py b/pulp_container/app/migrations/0043_add_os_arch_image_size_manifest_fields.py
new file mode 100644
index 000000000..d4e20d4b2
--- /dev/null
+++ b/pulp_container/app/migrations/0043_add_os_arch_image_size_manifest_fields.py
@@ -0,0 +1,49 @@
+# Generated by Django 4.2.16 on 2024-10-30 11:09
+import warnings
+
+from django.db import migrations, models
+
+def print_warning_for_updating_manifest_fields(apps, schema_editor):
+    warnings.warn(
+        "Run 'pulpcore-manager container-handle-image-data' to update the manifests' "
+        "os, architecture, and compressed_image_size fields."
+    )
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('container', '0042_add_manifest_nature_field'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='manifest',
+            name='architecture',
+            field=models.TextField(null=True),
+        ),
+        migrations.AddField(
+            model_name='manifest',
+            name='compressed_image_size',
+            field=models.IntegerField(null=True),
+        ),
+        migrations.AddField(
+            model_name='manifest',
+            name='os',
+            field=models.TextField(null=True),
+        ),
+        migrations.AlterField(
+            model_name='manifestlistmanifest',
+            name='architecture',
+            field=models.TextField(blank=True, default=''),
+        ),
+        migrations.AlterField(
+            model_name='manifestlistmanifest',
+            name='os',
+            field=models.TextField(blank=True, default=''),
+        ),
+        migrations.RunPython(
+            print_warning_for_updating_manifest_fields,
+            reverse_code=migrations.RunPython.noop,
+            elidable=True,
+        ),
+    ]
diff --git a/pulp_container/app/models.py b/pulp_container/app/models.py
index 35d3afd55..a20ee91f8 100644
--- a/pulp_container/app/models.py
+++ b/pulp_container/app/models.py
@@ -85,6 +85,11 @@ class Manifest(Content):
         labels (models.JSONField): Metadata stored inside the image configuration.
         is_bootable (models.BooleanField): Indicates whether the image is bootable or not.
         is_flatpak (models.BooleanField): Indicates whether the image is a flatpak package or not.
+        architecture (models.TextField): CPU architecture for which the binaries in the image are
+            designed to run.
+        os (models.TextField): Operating System which the image is built to run on.
+        compressed_image_size (models.IntegerField): Sum of the sizes, in bytes, of all compressed
+            layers.
 
     Relations:
         blobs (models.ManyToManyField): Many-to-many relationship with Blob.
@@ -112,6 +117,9 @@ class Manifest(Content):
 
     annotations = models.JSONField(default=dict)
     labels = models.JSONField(default=dict)
+    architecture = models.TextField(null=True)
+    os = models.TextField(null=True)
+    compressed_image_size = models.IntegerField(null=True)
 
     # DEPRECATED: this field is deprecated and will be removed in a future release.
     is_bootable = models.BooleanField(default=False)
@@ -212,6 +220,31 @@ def init_manifest_nature(self):
 
         return False
 
+    def init_architecture_and_os(self):
+        # schema1 has the architecture/os definition in the Manifest (not in the ConfigBlob)
+        # and none of these fields are required
+        if self.json_manifest.get("architecture", None) or self.json_manifest.get("os", None):
+            self.architecture = self.json_manifest.get("architecture", None)
+            self.os = self.json_manifest.get("os", None)
+            return
+
+        config_artifact = self.config_blob._artifacts.get()
+        config_data, _ = get_content_data(config_artifact)
+        self.architecture = config_data.get("architecture", None)
+        self.os = config_data.get("os", None)
+
+    def init_compressed_image_size(self):
+        # manifestv2 schema1 has only blobSum definition for each layer
+        if self.json_manifest.get("fsLayers", None):
+            self.compressed_image_size = 0
+            return
+
+        layers = self.json_manifest.get("layers")
+        compressed_size = 0
+        for layer in layers:
+            compressed_size += layer.get("size")
+        self.compressed_image_size = compressed_size
+
     def is_bootable_image(self):
         return (
             self.annotations.get("containers.bootc") == "1"
@@ -301,8 +334,9 @@ class ManifestListManifest(models.Model):
         manifest_list (models.ForeignKey): Many-to-one relationship with ManifestList.
     """
 
-    architecture = models.TextField()
-    os = models.TextField()
+    # in oci-index spec, platform is an optional field
+    architecture = models.TextField(default="", blank=True)
+    os = models.TextField(default="", blank=True)
     os_version = models.TextField(default="", blank=True)
     os_features = models.TextField(default="", blank=True)
     features = models.TextField(default="", blank=True)
@@ -315,6 +349,14 @@ class ManifestListManifest(models.Model):
         Manifest, related_name="manifest_lists", on_delete=models.CASCADE
     )
 
+    def set_platform_configs(self, platform):
+        self.architecture = platform["architecture"]
+        self.os = platform["os"]
+        self.features = platform.get("features", "")
+        self.variant = platform.get("variant", "")
+        self.os_version = platform.get("os.version", "")
+        self.os_features = platform.get("os.features", "")
+
     class Meta:
         unique_together = ("image_manifest", "manifest_list")
 
diff --git a/pulp_container/app/registry.py b/pulp_container/app/registry.py
index f907ef98d..be6b1cced 100644
--- a/pulp_container/app/registry.py
+++ b/pulp_container/app/registry.py
@@ -454,10 +454,12 @@ async def init_pending_content(self, digest, manifest_data, media_type, raw_text
             config_blob=config_blob,
             data=raw_text_data,
         )
+        await sync_to_async(manifest.init_architecture_and_os)()
 
         # skip if media_type of schema1
         if media_type in (MEDIA_TYPE.MANIFEST_V2, MEDIA_TYPE.MANIFEST_OCI):
             await sync_to_async(manifest.init_metadata)(manifest_data=manifest_data)
+        await sync_to_async(manifest.init_compressed_image_size)()
 
         try:
             await manifest.asave()
diff --git a/pulp_container/app/registry_api.py b/pulp_container/app/registry_api.py
index 49f1347ae..72e416ab3 100644
--- a/pulp_container/app/registry_api.py
+++ b/pulp_container/app/registry_api.py
@@ -1204,7 +1204,7 @@ def put(self, request, path, pk=None):
         if is_manifest_list:
             manifests = {}
             for manifest in content_data.get("manifests"):
-                manifests[manifest["digest"]] = manifest["platform"]
+                manifests[manifest["digest"]] = manifest.get("platform", None)
 
             digests = set(manifests.keys())
 
@@ -1219,17 +1219,12 @@ def put(self, request, path, pk=None):
 
             manifests_to_list = []
             for manifest in found_manifests:
-                platform = manifests[manifest.digest]
                 manifest_to_list = models.ManifestListManifest(
                     manifest_list=manifest,
                     image_manifest=manifest_list,
-                    architecture=platform["architecture"],
-                    os=platform["os"],
-                    features=platform.get("features", ""),
-                    variant=platform.get("variant", ""),
-                    os_version=platform.get("os.version", ""),
-                    os_features=platform.get("os.features", ""),
                 )
+                if platform := manifests[manifest.digest]:
+                    manifest_to_list.set_platform_configs(platform)
                 manifests_to_list.append(manifest_to_list)
 
             models.ManifestListManifest.objects.bulk_create(
@@ -1300,6 +1295,8 @@ def put(self, request, path, pk=None):
             config_blob = found_config_blobs.first()
             manifest = self._init_manifest(manifest_digest, media_type, raw_text_data, config_blob)
             manifest.init_metadata(manifest_data=content_data)
+            manifest.init_architecture_and_os()
+            manifest.init_compressed_image_size()
 
             manifest = self._save_manifest(manifest)
 
diff --git a/pulp_container/app/serializers.py b/pulp_container/app/serializers.py
index 6a767d857..32ca51778 100644
--- a/pulp_container/app/serializers.py
+++ b/pulp_container/app/serializers.py
@@ -116,6 +116,21 @@ class ManifestSerializer(NoArtifactContentSerializer):
             "[deprecated] check type field instead"
         ),
     )
+    architecture = serializers.CharField(
+        help_text="The CPU architecture which the binaries in this image are built to run on.",
+        required=False,
+        default=None,
+    )
+    os = serializers.CharField(
+        help_text="The name of the operating system which the image is built to run on.",
+        required=False,
+        default=None,
+    )
+    compressed_image_size = serializers.IntegerField(
+        help_text="Specifies the sum of the sizes, in bytes, of all compressed layers",
+        required=False,
+        default=None,
+    )
 
     class Meta:
         fields = NoArtifactContentSerializer.Meta.fields + (
@@ -130,6 +145,9 @@ class Meta:
             "is_bootable",
             "is_flatpak",
             "type",
+            "architecture",
+            "os",
+            "compressed_image_size",
         )
         model = models.Manifest
 
diff --git a/pulp_container/app/tasks/builder.py b/pulp_container/app/tasks/builder.py
index 0739242aa..530e64480 100644
--- a/pulp_container/app/tasks/builder.py
+++ b/pulp_container/app/tasks/builder.py
@@ -87,11 +87,15 @@ def add_image_from_directory_to_repository(path, repository, tag):
 
         config_blob = get_or_create_blob(manifest_json["config"], manifest, path)
         manifest.config_blob = config_blob
-        manifest.save()
+        manifest.init_architecture_and_os()
 
         pks_to_add = []
+        compressed_size = 0
         for layer in manifest_json["layers"]:
+            compressed_size += layer.get("size")
             pks_to_add.append(get_or_create_blob(layer, manifest, path).pk)
+        manifest.compressed_image_size = compressed_size
+        manifest.save()
 
         pks_to_add.extend([manifest.pk, tag.pk, config_blob.pk])
         new_repo_version.add_content(Content.objects.filter(pk__in=pks_to_add))
diff --git a/pulp_container/app/tasks/sync_stages.py b/pulp_container/app/tasks/sync_stages.py
index c78b77476..3b3163532 100644
--- a/pulp_container/app/tasks/sync_stages.py
+++ b/pulp_container/app/tasks/sync_stages.py
@@ -281,6 +281,7 @@ async def resolve_flush(self):
                 manifest_dc.content.config_blob = await config_blob_dc.resolution()
                 await sync_to_async(manifest_dc.content.init_labels)()
                 manifest_dc.content.init_image_nature()
+                await sync_to_async(manifest_dc.content.init_architecture_and_os)()
             for blob_dc in manifest_dc.extra_data["blob_dcs"]:
                 # Just await here. They will be associated in the post_save hook.
                 await blob_dc.resolution()
@@ -334,12 +335,15 @@ async def handle_blobs(self, manifest_dc, content_data):
         Handle blobs.
         """
         manifest_dc.extra_data["blob_dcs"] = []
+        compressed_size = 0
         for layer in content_data.get("layers") or content_data.get("fsLayers"):
             if not self._include_layer(layer):
                 continue
+            compressed_size += layer.get("size", 0)
             blob_dc = self.create_blob(layer)
             manifest_dc.extra_data["blob_dcs"].append(blob_dc)
             await self.put(blob_dc)
+        manifest_dc.content.compressed_image_size = compressed_size
         layer = content_data.get("config", None)
         if layer:
             blob_dc = self.create_blob(layer, deferred_download=False)
@@ -392,6 +396,8 @@ def create_manifest(self, manifest_data, raw_text_data, media_type, digest=None)
             media_type=media_type,
             data=raw_text_data,
             annotations=manifest_data.get("annotations", {}),
+            architecture=manifest_data.get("architecture", None),
+            os=manifest_data.get("os", None),
         )
 
         manifest.init_manifest_nature()
@@ -434,6 +440,8 @@ async def _download_and_instantiate_manifest(self, manifest_url, digest):
             media_type=media_type,
             data=raw_text_data,
             annotations=content_data.get("annotations", {}),
+            architecture=content_data.get("architecture", None),
+            os=content_data.get("os", None),
         )
         return content_data, manifest
 
@@ -472,14 +480,11 @@ async def create_listed_manifest(self, manifest_data):
                 manifest_url, digest
             )
 
-        platform = {}
-        p = manifest_data["platform"]
-        platform["architecture"] = p["architecture"]
-        platform["os"] = p["os"]
-        platform["features"] = p.get("features", "")
-        platform["variant"] = p.get("variant", "")
-        platform["os.version"] = p.get("os.version", "")
-        platform["os.features"] = p.get("os.features", "")
+        # in oci-index spec, platform is an optional field
+        platform = manifest_data.get("platform", None)
+        if platform:
+            manifest.os = platform["os"]
+            manifest.architecture = platform["architecture"]
         man_dc = DeclarativeContent(content=manifest)
         return {"manifest_dc": man_dc, "platform": platform, "content_data": content_data}
 
@@ -629,19 +634,14 @@ def _post_save(self, batch):
                 manifest_lists.append(dc.content)
                 for listed_manifest in dc.extra_data["listed_manifests"]:
                     manifest_dc = listed_manifest["manifest_dc"]
-                    platform = listed_manifest["platform"]
-                    manifest_list_manifests.append(
-                        ManifestListManifest(
-                            manifest_list=manifest_dc.content,
-                            image_manifest=dc.content,
-                            architecture=platform["architecture"],
-                            os=platform["os"],
-                            features=platform.get("features"),
-                            variant=platform.get("variant"),
-                            os_version=platform.get("os.version"),
-                            os_features=platform.get("os.features"),
-                        )
+                    manifest_list_manifest = ManifestListManifest(
+                        manifest_list=manifest_dc.content,
+                        image_manifest=dc.content,
                     )
+                    if platform := listed_manifest.get("platform", None):
+                        manifest_list_manifest.set_platform_configs(platform)
+                    manifest_list_manifests.append(manifest_list_manifest)
+
         if blob_manifests:
             BlobManifest.objects.bulk_create(blob_manifests, ignore_conflicts=True)
         if manifest_list_manifests:
diff --git a/pulp_container/tests/functional/api/test_build_images.py b/pulp_container/tests/functional/api/test_build_images.py
index 3980246b0..992c0777f 100644
--- a/pulp_container/tests/functional/api/test_build_images.py
+++ b/pulp_container/tests/functional/api/test_build_images.py
@@ -209,7 +209,13 @@ def test_invalid_containerfile_from_build_context(
 
 
 def test_without_build_context(
-    build_image, container_distribution_api, container_repo, gen_object_with_cleanup, local_registry
+    build_image,
+    check_manifest_arch_os_size,
+    container_distribution_api,
+    container_manifest_api,
+    container_repo,
+    gen_object_with_cleanup,
+    local_registry,
 ):
     """Test build with only a Containerfile (no additional files)"""
 
@@ -237,3 +243,5 @@ def containerfile_without_context_files():
     local_registry.pull(distribution.base_path)
     image = local_registry.inspect(distribution.base_path)
     assert image[0]["Config"]["Cmd"] == ["ls", "/"]
+    manifest = container_manifest_api.list(digest=image[0]["Digest"])
+    check_manifest_arch_os_size(manifest)
diff --git a/pulp_container/tests/functional/api/test_pull_through_cache.py b/pulp_container/tests/functional/api/test_pull_through_cache.py
index 3eb170e50..72a4f9b53 100644
--- a/pulp_container/tests/functional/api/test_pull_through_cache.py
+++ b/pulp_container/tests/functional/api/test_pull_through_cache.py
@@ -40,6 +40,8 @@ def pull_and_verify(
     anonymous_user,
     add_pull_through_entities_to_cleanup,
     check_manifest_fields,
+    check_manifest_arch_os_size,
+    container_manifest_api,
     container_pull_through_distribution_api,
     container_distribution_api,
     container_repository_api,
@@ -85,6 +87,10 @@ def _pull_and_verify(images, pull_through_distribution):
             remote_image = registry_client.inspect(remote_image_path)
             assert local_image[0]["Id"] == remote_image[0]["Id"]
 
+            # 2.1. check pulp manifest model fields
+            manifest = container_manifest_api.list(digest=local_image[0]["Digest"])
+            check_manifest_arch_os_size(manifest)
+
             # 3. check if the repository version has changed
             for _ in range(5):
                 repository = container_repository_api.list(name=path).results[0]
diff --git a/pulp_container/tests/functional/api/test_push_content.py b/pulp_container/tests/functional/api/test_push_content.py
index 07898ba34..4033c277e 100644
--- a/pulp_container/tests/functional/api/test_push_content.py
+++ b/pulp_container/tests/functional/api/test_push_content.py
@@ -37,9 +37,11 @@
 
 def test_push_using_registry_client_admin(
     add_to_cleanup,
+    check_manifest_arch_os_size,
     registry_client,
     local_registry,
     check_manifest_fields,
+    container_manifest_api,
     container_namespace_api,
 ):
     """Test push with official registry client and logged in as admin."""
@@ -56,6 +58,8 @@ def test_push_using_registry_client_admin(
         manifest_filters={"digest": local_image[0]["Digest"]},
         fields={"type": MANIFEST_TYPE.IMAGE},
     )
+    manifest = container_manifest_api.list(digest=local_image[0]["Digest"])
+    check_manifest_arch_os_size(manifest)
 
     # ensure that same content can be pushed twice without permission errors
     local_registry.tag_and_push(image_path, local_url)
diff --git a/pulp_container/tests/functional/api/test_sync.py b/pulp_container/tests/functional/api/test_sync.py
index d2d534fe7..f36f7fe23 100644
--- a/pulp_container/tests/functional/api/test_sync.py
+++ b/pulp_container/tests/functional/api/test_sync.py
@@ -43,10 +43,12 @@ def _synced_container_repository_factory(
 @pytest.mark.parallel
 def test_basic_sync(
     check_manifest_fields,
+    check_manifest_arch_os_size,
     container_repo,
     container_remote,
     container_repository_api,
     container_sync,
+    container_manifest_api,
 ):
     repo_version = container_sync(container_repo, container_remote).created_resources[0]
     repository = container_repository_api.read(container_repo.pulp_href)
@@ -71,6 +73,13 @@ def test_basic_sync(
 
     assert repository.latest_version_href == latest_version_href
 
+    manifest = container_manifest_api.list(
+        repository_version=repo_version,
+        media_type=[MEDIA_TYPE.MANIFEST_V2],
+        digest=PULP_HELLO_WORLD_LINUX_AMD64_DIGEST,
+    )
+    check_manifest_arch_os_size(manifest)
+
 
 @pytest.mark.parallel
 def test_sync_labelled_image(
diff --git a/pulp_container/tests/functional/conftest.py b/pulp_container/tests/functional/conftest.py
index bcae14793..9e926b688 100644
--- a/pulp_container/tests/functional/conftest.py
+++ b/pulp_container/tests/functional/conftest.py
@@ -474,3 +474,14 @@ def _check_manifest_fields(**kwargs):
         return True
 
     return _check_manifest_fields
+
+
+@pytest.fixture
+def check_manifest_arch_os_size():
+    def _check_manifest_arch_os_size(manifest):
+        manifests = manifest.to_dict()["results"]
+        assert any("amd64" in manifest["architecture"] for manifest in manifests)
+        assert any("linux" in manifest["os"] for manifest in manifests)
+        assert any(manifest["compressed_image_size"] > 0 for manifest in manifests)
+
+    return _check_manifest_arch_os_size