Add support for OCI Image Index v1

Danielku15 commented 2 months ago

This issue is a split from https://github.com/dotnet/sdk-container-builds/issues/527

Goal

Officially support the OCI Image Index v1 format which might be returned by registries like JFrog Artifactory and Sonatype Nexus.

Spec: https://github.com/opencontainers/image-spec/blob/main/image-index.md MimeType: application/vnd.oci.image.index.v1+json

Details

Luckily the OCI JSON schema aligns with the current ManifestListV2. There might be some differences in the specs, but the properties used currently by the .net SDK seem to be matching. Similarly the DockerManifestV2 and OciManifestV1 also match.

Main changes / Implementation Details

The following list points out some basic changes we have to do. These were already verified in a local build testing against JFrog Artifactory.

https://github.com/dotnet/sdk/blob/cf8c24575410adf397c0823fd7061f9451049ea1/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs#L157

-            SchemaTypes.DockerManifestListV2 => await PickBestImageFromManifestListAsync(
+            SchemaTypes.DockerManifestListV2 or SchemaTypes.OciImageIndexV1 => await PickBestImageFromManifestListAsync(

https://github.com/dotnet/sdk/blob/cf8c24575410adf397c0823fd7061f9451049ea1/src/Containers/Microsoft.NET.Build.Containers/Registry/SchemaTypes.cs#L7-L14

+    internal const string OciImageIndexV1 = "application/vnd.oci.image.index.v1+json";

https://github.com/dotnet/sdk/blob/cf8c24575410adf397c0823fd7061f9451049ea1/src/Containers/Microsoft.NET.Build.Containers/Registry/HttpExtensions.cs#L15

+        request.Headers.Accept.Add(new(SchemaTypes.OciImageIndexV1));

Tests

To test reliably that both formats work we have to ensure the registry image used in tests can be forced to deliver application/vnd.oci.image.index.v1+json (maybe by specifying the Accept header differently).

baronfel commented 2 months ago

We should support this, but I agree that testing will be important. I took at look at the source code for the registry, but it does look like it supports OCI Image Indexes.

It's nice that a simple change to add the schema works, but I'm not sure exactly how compatible the two formats are - I'd prefer to put in ground work to model the OCI Image Indexes as a separate domain type to keep the fidelity of the transport structure.

Danielku15 commented 2 months ago

It's nice that a simple change to add the schema works, but I'm not sure exactly how compatible the two formats are

Maybe it can be tackled separately to have dedicated OCI and Docker models ot not block/delay this feature? This was already the path for the image layers.

I understand the concern in the difference but the impact in the codebase might be bigger than expected. I'm thinking:

Adding base interfaces to reuse logic like PickBestImageFromManifestListAsync (DRY)
Translating between OCI and Docker formats when reading/writing images (annotations, mimetypes, archives etc).

baronfel commented 2 months ago

I'm worried about a few different aspects of just trying to paper over the schema differences:

If we change nothing about the Targets interfaces:

How should we treat the annotations property of the image index and the nested manifests? Without considering that we're 'lossy'.
How should we handle an image manifest that contains 'nested' image manifests (i.e. a manifests array entry with mediaType of application/vnd.oci.image.index.v1+json)? For purposes of best-matching I'd expect that we recursively traverse any such manifests and use the complete set of individual application/vnd.oci.image.manifest.v1+json items for doing RID comparisons, etc.
How should the ContainerLabels property map to the Image Index vs the Image Manifest items?

I don't have answers for all of these (though we should discuss and answer them in this issue!) and I'd want answers before we just go do a thing. This is the kind of thing I'm worried about papering over with a simple change. I understand it means more work, but supporting a new container type is not as conceptually simple as the layer format change you mention for this reason.

schmitch commented 2 months ago

actually for:

it would probably be better to actually throw an unsupported exception for the first version and add support for it later? I've not seen too deeply nested manifests (yet)

for 1. and 3. aren't they basically the same things? shouldn't the containerslabels add the annotations? or am I wrong?

Danielku15 commented 2 months ago

@baronfel What do you think about haveing a separate issue for supporting the OCI Image Manifest correctly and an epic/umbrella for overall OCI support?

Then we can keep this issue about the OCI Image Index. Supporting the index of image selection sounds simpler than handling OCI Images correctly. The spec doesn't prohibit listing Docker Image Manifests on an OCI image index.

This would bring the immediate benefit that people can load images from Artifactory and Nexus registries.

On the OCI image topic: You are fully right with your concerns about the differences in the image specs. We are lucky that the differences are mainly on the annotations where we have a data loss when creating the manifest for the next layer. This loss might also be in our interest as many annotations are not valid anymore on the next layer. Also Container Runtimes seem to be fine with mixing OCI and "Docker" Layers (as far I could see).

dotnet / sdk-container-builds