sdk-container-builds icon indicating copy to clipboard operation
sdk-container-builds copied to clipboard

Add support for OCI Image Index v1

Open Danielku15 opened this issue 1 year ago • 5 comments

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).

Danielku15 avatar Apr 15 '24 13:04 Danielku15

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.

baronfel avatar Apr 15 '24 15:04 baronfel

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).

Danielku15 avatar Apr 16 '24 13:04 Danielku15

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.

baronfel avatar Apr 16 '24 14:04 baronfel

actually for:

  1. 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?

schmitch avatar Apr 18 '24 11:04 schmitch

@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).

Danielku15 avatar Apr 20 '24 09:04 Danielku15

November's 8.0.400 and the 9.0.100 GA release will both support reading image manifests. https://github.com/dotnet/sdk/pull/43085 coming later this month will add support for authoring these manifests as well.

baronfel avatar Oct 09 '24 14:10 baronfel

Added support for authoring application/vnd.oci.image.index.v1+json in https://github.com/dotnet/sdk/pull/43085

surayya-MS avatar Oct 22 '24 16:10 surayya-MS