Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: add support for org.opencontainers.image annotations #1197

Merged
merged 1 commit into from Dec 3, 2023

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Apr 19, 2023

These annotations are specified as part of the official conversion process from an OCI image configuration to an OCI runtime configuration since v1.0.0-rc7 of the image-spec but they were never officially specified in

The fact these are not allowed by the current runtime-spec causes some issues with inter-spec compatibility. In order for tools like umoci to be able to do this generation properly, we need to allow this namespace to be used.

Ref: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc2/conversion.md
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

@cyphar
Copy link
Member Author

cyphar commented Apr 19, 2023

This is a minimal implementation of support for the org.opencontainers.image namespace of annotations (as defined in and used by the image-spec).

However, given that the list of annotations might grow in the future, it might make far more sense to copy the current set of annotations into the runtime-spec and require any additions to the list of annotations to be co-ordinated between specifications? That way we don't need to hard-code a reference to a specific image-spec version in the runtime-spec...

@cyphar
Copy link
Member Author

cyphar commented Apr 19, 2023

(One reason we need this PR is that umoci has been generating these configs for ages, but the version of oci-runtime-tool used to validate the generated config.json didn't support validating annotations. I've only just gotten around to updating the ancient version of oci-runtime-tool and it turns out this conversion causes verification failures.)

@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Apr 19, 2023
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AkihiroSuda AkihiroSuda modified the milestones: v1.2.0, v1.1.1 Apr 20, 2023
@cyphar cyphar marked this pull request as draft April 20, 2023 16:12
@cyphar
Copy link
Member Author

cyphar commented Apr 20, 2023

I will switch to having a copy of the annotation list here, since otherwise it'll be difficult to guarantee compatibility with a specific runtime-spec version (what version of image-spec should be used?). We don't add annotations to the list very regularly so having to do updates in lock-step probably isn't too much of a hassle.

@giuseppe
Copy link
Member

Would these annotations matter anyway for an OCI runtime?

@cyphar
Copy link
Member Author

cyphar commented Apr 20, 2023

I think some runtimes might make use of them (org.opencontainers.image.stopSignal is the most obvious one that a runtime might care about).

The issue is that at the moment technically you're forbidden to use them for anything according to the runtime-spec, but the image-spec explicitly requires you to use them when generating a config.json. If we want to have proper validation in runtime-tools (rather than just allowing all of org.opencontainers.image.* without restrictions) I think we should at least have a list of the allowed annotations (and point to the image-spec for their semantics).

cyphar added a commit to cyphar/umoci that referenced this pull request Apr 21, 2023
We also need to switch to "go install" entirely because we are about to
update to a newer Go version that has disallowed "go get" outside of
repos.

For both tools, we need to pin an older version in order to make sure
our tests function properly:

 * oci-runtime-tool since v0.6.0 validates that reserved annotations are
   not used. However, due to an oversight the OCI image-spec defined
   org.opencontainers.image.* annotations are considered "reserved" by
   the OCI runtime-spec. [1] should resolve this issue (once it is
   merged and oci-runtime-tool is updated with the relevant fixes) but
   in the meantime we need to use an older version of this tool. So we
   need to use v0.5.0 for the time being.

 * oci-image-tool appears to have become non-functional for our needs
   since v1.0.0-rc1, where support for scanning an entire image layout
   (i.e. complete OCI image) appears to have been removed. We need to
   use v0.3.0 for the time being.

[1]: opencontainers/runtime-spec#1197

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
These annotations are specified as part of the official conversion
process from an OCI image configuration to an OCI runtime configuration
since v1.0.0-rc7 of the image-spec but they were never officially
specified in the runtime-spec.

The fact these are not allowed by the current runtime-spec causes some
issues with inter-spec compatibility. In order for tools like umoci to
be able to do this generation properly, we need to allow this namespace
to be used.

Ref: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc2/conversion.md
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@cyphar cyphar marked this pull request as ready for review April 21, 2023 13:15
@cyphar cyphar requested a review from giuseppe April 21, 2023 13:15
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

| `org.opencontainers.image.created` | Indicates the date and time when the container image was created. The annotation value MUST have a valid value for the `created` property as defined in [the OCIimage specification][oci-image-config-properties]. This annotation SHOULD only be used in accordance with the [OCI image specification's runtime conversion specification][oci-image-conversion]. |
| `org.opencontainers.image.stopSignal` | Indicates signal that SHOULD be sent by the container runtimes to [kill the container](runtime.md#kill). The annotation value MUST have a valid value for the `config.StopSignal` property as defined in [the OCI image specification][oci-image-config-properties]. This annotation SHOULD only be used in accordance with the [OCI image specification's runtime conversion specification][oci-image-conversion]. |

All other keys in the `org.opencontainers` namespace not specified in this above table are reserved and MUST NOT be used by subsequent specifications.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"subsequent specifications" here seems a little weird - that almost sounds like it's saying we won't use any others in the future, which I don't think is a promise we should make 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wording in the current spec so I tried to mirror it, but I agree that it sounds very wrong. I suspect that "subsequent specifications" is meant to mean "other specifications" or "other organisations" (I haven't looked at the git history but it's possible this comes from when we thought AppC might end up being an extension of runtime-spec.)

I'll figure out some nicer text.

@AkihiroSuda AkihiroSuda merged commit 68346ed into opencontainers:main Dec 3, 2023
3 checks passed
@cyphar cyphar deleted the image-spec-annotations branch December 3, 2023 13:37
@utam0k utam0k mentioned this pull request Jan 5, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants