Add text about extensions #510
Conversation
| @@ -317,6 +320,10 @@ Annotations are key-value maps. | |||
| } | |||
| ``` | |||
|
|
|||
| ## Extensibility | |||
| The `annotations` property MAY be used as an extensibility point to include additional information that is not defined as part of this specification. | |||
| Excluding the `annotations` property, implementations that are reading/processing this configuration file MUST generate an error if they encounter an unkown property. | |||
There was a problem hiding this comment.
This seems unnecessarily strict. The ociVersion field uniquely (ish, #371) declares semantics for fields defined by this spec. Maybe the additional properties are optional features (folks have talked about having both Solaris and Linux sections in the same config, although that particular use-case doesn't make much sense to me). In any case, I think this should be relaxed to SHOULD, and would recommend runtimes provide an --unknown-properties=(ignore|warn|error) flag or some such to allow callers to adjust the behavior.
More generally, I think JSON-LD is a good way to assign semantics to all properties in the config, whether they are stock OCI properties or extensions. That would give callers a better chance of figuring out whether a given extension was ignorable or not by making it easier to track down its semantics.
There was a problem hiding this comment.
I'd prefer we either ignore or fault unknown properties - no reason to open up interoperability issues with SHOULDs/MAYs/etc... I picked the fault path because that's where more people seemed to be headed during the OCI F2F last week. So this issue (and opencontainers/image-spec#164 ) were designed to have the discussion - once we pick one we just need to make the same decision in both spots.
There was a problem hiding this comment.
On Sun, Jun 26, 2016 at 05:37:21PM -0700, Doug Davis wrote:
+## Extensibility
+Theannotationsproperty MAY be used as an extensibility point to include additional information that is not defined as part of this specification.
+Excluding theannotationsproperty, implementations that are reading/processing this configuration file MUST generate an error if they encounter an unkown property.I'd prefer we either ignore or fault unknown properties - no reason
to open up interoperability issues with SHOULDs/MAYs/etc...
It's hard to know whether a property should be optional or not without
details of the property itself. For example, runC feels like the
version check is unnecessary 1, and ccon feels like the platform
check is unnecessary 2. In the absence of detail about the property
in question, the conservative course is to error out (so I'm in favor
of SHOULDing that approach). But “the runtime doesn't know what that
property is for” doesn't mean “the caller doesn't know what that
property is for”, and I'd rather allow users to be able to say “don't
worry about unrecognized properties, they're not important for this
container” (so I'm not in favor of MUSTing that approach). And a
SHOULD would avoid name migration issues as new features are
standardized 3.
On the other hand, I'm generally in favor of for moving non-standard
things out of the config (e.g. hooks.json, if we drop hooks 4), and
a MUST here would certainly help with that ;).
Subject: Re: Hooks and all-in-one operation
Date: Thu, 9 Jun 2016 14:50:49 -0700
Message-ID: <20160609215049.GK8801@odin.tremily.us>
| @@ -308,15 +308,21 @@ The semantics are the same as `Path`, `Args` and `Env` in [golang Cmd](https://g | |||
|
|
|||
| This OPTIONAL property contains arbitrary metadata for the container. | |||
| This information MAY be structured or unstructured. | |||
| Annotations are key-value maps. | |||
| Annotations MUST be key-value maps. | |||
There was a problem hiding this comment.
nit: Specify explicitly that the key and value are of type string and that the specification is not expecting any structure like DNS naming, etc..
There was a problem hiding this comment.
@vishh what do you mean by not expecting any structure? People can put json in the value string if they want right?
|
Updated based on today's conf call |
| Keys MUST be unique, and best practice is to namespace the keys. | ||
| Keys starting with the `opencontainers.org` namespace are reserved and MUST NOT be used. | ||
| If there are no annotations then this property MAY either be absent or an empty map. | ||
| Implementations that are reading/processing this configuration file MUST NOT generate an error if they encounter an unknown annotation key. |
There was a problem hiding this comment.
“MUST NOT generate an error” doesn't say how they should handle unknown entries. I think we should follow HTML's:
The
HTMLUnknownElementinterface must be used for HTML elements that are not defined by this specification (or other applicable specifications).
In our case, maybe wording like:
Implementations consuming this configuration MAY log unrecognized annotation keys but MUST otherwise ignore them.
There was a problem hiding this comment.
And while I abbreviated the “Implementations that are reading/processing this configuration” wording in my suggestion above, I think we should probably just go with “Runtimes”. Do you have other consumers in mind with your more general wording?
| @@ -308,15 +308,26 @@ The semantics are the same as `Path`, `Args` and `Env` in [golang Cmd](https://g | |||
|
|
|||
| This OPTIONAL property contains arbitrary metadata for the container. | |||
| This information MAY be structured or unstructured. | |||
| Annotations are key-value maps. | |||
| Annotations MUST be key-value maps where both the key and value MUST be strings. | |||
| While the key and value MUST be present, they MAY be empty strings. | |||
There was a problem hiding this comment.
Empty-string values are fine, but empty-string keys sounds like a bad idea to me.
duglin
force-pushed
the
extension
branch
2 times, most recently
from
8987aad to
44e2135
Compare
There are other APIs described in this specification (e.g. the state JSON format, and the in-flight command-line API [1]), but this string covers the configuration file and referenced objects (e.g. the filesystem at root.path). As additional, backwards compatible features are added to the spec (leading to 1.1, 1.2, etc. releases) and supported by runtimes, those runtimes will *still* stupport 1.0 configs. Once a 2.0 spec is cut, runtimes that only support 2.0 (and nothing in the 1.0 line) will no longer support the 1.0 config. My preferred approach here would be to use JSON-LD [2,3,4] to explicitly document the intended semantics for each field, which would allow us to drop the config-wide version and version each field independently. That would mean a breaking change on a particular field would only break compatibility for folks who were using that field. Unfortunately, I haven't had much luck pushing the consensus in that direction. This commit does not add wording about how the runtime and other consumers should handle an incompatible version. We can address that once the command-line API lands. [1]: opencontainers#513 [2]: opencontainers#371 (comment) [3]: opencontainers/image-spec#111 (comment) [4]: opencontainers#510 (comment)
There are other APIs described in this specification (e.g. the state JSON format, and the in-flight command-line API [1]), but this string covers the configuration file and referenced objects (e.g. the filesystem at root.path). As additional, backwards compatible features are added to the spec (leading to 1.1, 1.2, etc. releases) and supported by runtimes, those runtimes will *still* stupport 1.0 configs. Once a 2.0 spec is cut, runtimes that only support 2.0 (and nothing in the 1.0 line) will no longer support the 1.0 config. My preferred approach here would be to use JSON-LD [2,3,4] to explicitly document the intended semantics for each field, which would allow us to drop the config-wide version and version each field independently. That would mean a breaking change on a particular field would only break compatibility for folks who were using that field. Unfortunately, I haven't had much luck pushing the consensus in that direction. This commit does not add wording about how the runtime and other consumers should handle an incompatible version. We can address that once the command-line API lands. [1]: opencontainers#513 [2]: opencontainers#371 (comment) [3]: opencontainers/image-spec#111 (comment) [4]: opencontainers#510 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
|
FYI the PR was written to use reverse-DNS as was agreed in the last f2f meeting |
|
Thanks @duglin. |
|
if people are ok with it I can add a sentence about how they SHOULD use reverse-DNS notation. |
|
@duglin I know @RobDolinMS is wishing to avoid SHOULDs as they make for wider interpretation. I'm not sure how best to write it as an encouragement but not an invalidating factor |
|
On Tue, Aug 30, 2016 at 10:21:09AM -0700, Vincent Batts wrote:
Ignoring a SHOULD doesn't make your implementation invalid, it just However, this key-namespacing suggestion is targetted at external spec |
|
hmm, I'm not sure why a SHOULD would be bad since its pretty much what we want - an encouragement (we can use RECOMMEND if we want too) - w/o making it a hard requirement. @RobDolinMS what's the issue with SHOULDs? |
|
On Tue, Aug 30, 2016 at 05:38:07PM -0700, Doug Davis wrote:
To make 1 clearer, the line you recently removed from the image-spec Keys SHOULD be named using a reverse domain notation - Assuming you add a line like that here, how would you test compliance Ocitools currently tests configurations and runtime implementations |
|
On Tue, Aug 30, 2016 at 08:59:26PM -0700, Doug Davis wrote:
This is #543, which I'm not in favor of, but that's not the point…
This (and your (1)) are generic SHOULD stuff. I'm more concerned
Do you have implementation suggestions for those (I don't)? If not, |
|
I feel like I'm missing something. Generic statements:
Testing a SHOULD is no different than testing a MUST - either you can or can't do it with the test tools. The only difference is how you deal with the detection of them not following the rule. MUST always results in an error ; SHOULDs are either ignored, generate warnings, or generate an error if there's some kind of "strict" option. |
|
On Wed, Aug 31, 2016 at 04:43:28AM -0700, Doug Davis wrote:
The line I think you're considering adding 1: Keys SHOULD be named using a reverse domain notation - is a requirement placed on external specs. It is not a requirement If you see it as a runtime or configuration requirement, could you
We certainly could have a SHOULD requirement for external specs. |
duglin
force-pushed
the
extension
branch
2 times, most recently
from
8f3f59e to
1bc036e
Compare
Mimic opencontainers/image-spec#164 and they should be kept in-sync Signed-off-by: Doug Davis <dug@us.ibm.com>
|
updated |
| While the value MUST be present, it MAY be an empty string. | ||
| Keys MUST be unique within this map, and best practice is to namespace the keys. | ||
| Keys SHOULD be named using a reverse domain notation - e.g. `com.example.myKey`. | ||
| Keys using the `org.opencontainers` namespace are reserved and MUST NOT be used by subsequent specifications. |
There was a problem hiding this comment.
Missing the trailing dot for org.opencontainers..
“subsequent specifications” is unclear to me. Can we follow image-spec and use “… are reserved for future versions of the specification.”?
|
LGTM |
|
The use of SHOULD in line 330 of this PR looks good to me. /cc @duglin, @wking, @vbatts I also agree with @duglin's comment about how test tools interpret: |
|
LGTM |
There's an outside change that these are intentional, since I pointed one of these out earlier [1] and it wasn't fixed. But I haven't seen " : " used intentionally outside of this project, and don't think we want to break ground in that direction ;). [1]: opencontainers#510 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
Add text about extensions Signed-off-by: Deng Guangxing <dengguangxing@huawei.com>
There are other APIs described in this specification (e.g. the state JSON format, and the in-flight command-line API [1]), but this string covers the configuration file and referenced objects (e.g. the filesystem at root.path). As additional, backwards compatible features are added to the spec (leading to 1.1, 1.2, etc. releases) and supported by runtimes, those runtimes will *still* stupport 1.0 configs. Once a 2.0 spec is cut, runtimes that only support 2.0 (and nothing in the 1.0 line) will no longer support the 1.0 config. My preferred approach here would be to use JSON-LD [2,3,4] to explicitly document the intended semantics for each field, which would allow us to drop the config-wide version and version each field independently. That would mean a breaking change on a particular field would only break compatibility for folks who were using that field. Unfortunately, I haven't had much luck pushing the consensus in that direction. This commit does not add wording about how the runtime and other consumers should handle an incompatible version. We can address that once the command-line API lands. [1]: opencontainers#513 [2]: opencontainers#371 (comment) [3]: opencontainers/image-spec#111 (comment) [4]: opencontainers#510 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
…ssible
We don't want to silently ignore settings that we understand but
cannot implement [1] (we *do* want to ignore settings that we don't
understand [2], but that's a separate issue).
This raises a slightly sticky certification issue. If a runtime
*always* exits 'create' with an error:
func create() err {
return fmt.Errorf("nope, I cannot create that container either.")
}
it would be neither complaint nor non-compliant. It would not fail
any MUSTs, but availing itself of the "cannot create the maintainer"
option specified in this commit would mean the test suite could not
test the deeper requirements around the config properties themselves.
So with this change, making Microsoft certifiable will still need an
explicit weakening around root.path. The easiest way to do that might
be to have separate annotations for whether a setting is optional for
config authors and whether it's optional for runtime authors
(supported):
* **`readonly`** (bool, config:optional, support:optional) ...
But I'll leave hashing that out to a later commit.
[1]: https://github.com/opencontainers/runtime-spec/pull/476/files/9b8e21826cc9887f51f095604120cfbb788078b2#r65400731
Subject: [ Config | Root Config ] Clarify readonly
[2]: opencontainers#510
Subject: Add text about extensions
Signed-off-by: W. Trevor King <wking@tremily.us>
…ssible
We don't want to silently ignore settings that we understand but
cannot implement [1] (we *do* want to ignore settings that we don't
understand [2], but that's a separate issue).
This raises a slightly sticky certification issue. If a runtime
*always* exits 'create' with an error:
func create() err {
return fmt.Errorf("nope, I cannot create that container either.")
}
it would be neither complaint nor non-compliant. It would not fail
any MUSTs, but availing itself of the "cannot create the maintainer"
option specified in this commit would mean the test suite could not
test the deeper requirements around the config properties themselves.
So with this change, making Microsoft certifiable will still need an
explicit weakening around root.path. The easiest way to do that might
be to have separate annotations for whether a setting is optional for
config authors and whether it's optional for runtime authors
(supported):
* **`readonly`** (bool, config:optional, support:optional) ...
But I'll leave hashing that out to a later commit. Regardless of the
certification impact, we want to be clear that silently ignoring known
parameters is wrong.
[1]: https://github.com/opencontainers/runtime-spec/pull/476/files/9b8e21826cc9887f51f095604120cfbb788078b2#r65400731
Subject: [ Config | Root Config ] Clarify readonly
[2]: opencontainers#510
Subject: Add text about extensions
Signed-off-by: W. Trevor King <wking@tremily.us>
…ssible
We don't want to silently ignore settings that we understand but
cannot implement [1] (we *do* want to ignore settings that we don't
understand [2], but that's a separate issue).
This raises a slightly sticky certification issue. If a runtime
*always* exits 'create' with an error:
func create() err {
return fmt.Errorf("nope, I cannot create that container either.")
}
it would be neither complaint nor non-compliant. It would not fail
any MUSTs, but availing itself of the "cannot create the maintainer"
option specified in this commit would mean the test suite could not
test the deeper requirements around the config properties themselves.
So with this change, making Microsoft certifiable will still need an
explicit weakening around root.path. The easiest way to do that might
be to have separate annotations for whether a setting is optional for
config authors and whether it's optional for runtime authors
(supported):
* **`readonly`** (bool, config:optional, support:optional) ...
But I'll leave hashing that out to a later commit. Regardless of the
certification impact, we want to be clear that silently ignoring known
parameters is wrong.
[1]: https://github.com/opencontainers/runtime-spec/pull/476/files/9b8e21826cc9887f51f095604120cfbb788078b2#r65400731
Subject: [ Config | Root Config ] Clarify readonly
[2]: opencontainers#510
Subject: Add text about extensions
Signed-off-by: W. Trevor King <wking@tremily.us>
There are other APIs described in this specification (e.g. the state JSON format, and the in-flight command-line API [1]), but this string covers the configuration file and referenced objects (e.g. the filesystem at root.path). As additional, backwards compatible features are added to the spec (leading to 1.1, 1.2, etc. releases) and supported by runtimes, those runtimes will *still* stupport 1.0 configs. Once a 2.0 spec is cut, runtimes that only support 2.0 (and nothing in the 1.0 line) will no longer support the 1.0 config. My preferred approach here would be to use JSON-LD [2,3,4] to explicitly document the intended semantics for each field, which would allow us to drop the config-wide version and version each field independently. That would mean a breaking change on a particular field would only break compatibility for folks who were using that field. Unfortunately, I haven't had much luck pushing the consensus in that direction. This commit does not add wording about how the runtime and other consumers should handle an incompatible version. We can address that once the command-line API lands. [1]: opencontainers#513 [2]: opencontainers#371 (comment) [3]: opencontainers/image-spec#111 (comment) [4]: opencontainers#510 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
…ssible
We don't want to silently ignore settings that we understand but
cannot implement [1] (we *do* want to ignore settings that we don't
understand [2], but that's a separate issue).
This raises a slightly sticky certification issue. If a runtime
*always* exits 'create' with an error:
func create() err {
return fmt.Errorf("nope, I cannot create that container either.")
}
it would be neither complaint nor non-compliant. It would not fail
any MUSTs, but availing itself of the "cannot create the maintainer"
option specified in this commit would mean the test suite could not
test the deeper requirements around the config properties themselves.
So with this change, making Microsoft certifiable will still need an
explicit weakening around root.path. The easiest way to do that might
be to have separate annotations for whether a setting is optional for
config authors and whether it's optional for runtime authors
(supported):
* **`readonly`** (bool, config:optional, support:optional) ...
But I'll leave hashing that out to a later commit. Regardless of the
certification impact, we want to be clear that silently ignoring known
parameters is wrong.
[1]: https://github.com/opencontainers/runtime-spec/pull/476/files/9b8e21826cc9887f51f095604120cfbb788078b2#r65400731
Subject: [ Config | Root Config ] Clarify readonly
[2]: opencontainers#510
Subject: Add text about extensions
Signed-off-by: W. Trevor King <wking@tremily.us>
This condition landed in 27a05de (Add text about extensions, 2016-06-26, opencontainers#510) with subsequent wording tweaks in 3f0440b (config.md: add empty limit for key of annotations, Dec 28 10:35:19 2016, opencontainers#645) and 2c8feeb (config: Bring "unique... within this map" back together, 2017-01-12, opencontainers#654). However, since eeaccfa (glossary: Make objects explicitly unordered and forbid duplicate names, 2016-09-27, opencontainers#584) we forbid duplicate keys on *all* objects (not just annotations), so this PR removes the redundant annotation-specific condition. Signed-off-by: W. Trevor King <wking@tremily.us>
This condition landed in 27a05de (Add text about extensions, 2016-06-26, opencontainers#510) with subsequent wording tweaks in 3f0440b (config.md: add empty limit for key of annotations, Dec 28 10:35:19 2016, opencontainers#645) and 2c8feeb (config: Bring "unique... within this map" back together, 2017-01-12, opencontainers#654). However, since eeaccfa (glossary: Make objects explicitly unordered and forbid duplicate names, 2016-09-27, opencontainers#584) we forbid duplicate keys on *all* objects (not just annotations), so this PR removes the redundant annotation-specific condition. Signed-off-by: W. Trevor King <wking@tremily.us>
Generating an error seems like one potential violation of the requirement to ignore unknown properties. Compliance testing for the ignore requirement can cite the MUST I've written here for any noticeable runtime activity around the unknown property without needing a error-specific MUST. We've had the two MUSTs since 27a05de (Add text about extensions, 2016-06-26, opencontainers#510), citing [1]. I'd asked for consolidated phrasing then [2,3], but hadn't followed up after the commit landed. I've left a line mentioning the error activity as non-normative clarification, but am also happy to drop that line completely. Also: * Update the unknown annotation entry to reference the generic extensibility section, because there's nothing annotation-specific in how we want runtimes to handle unknown keys. * Remove "reading or processing" language. This initially landed in 27a05de with a bump in b92cf90 (consistency and style fix, 2017-05-12, opencontainers#811). Some thought was put into this phrasing there [4,5] and earlier in opencontainers#510 [6], but we never got around to dropping this qualifier. However, the purpose of this qualifier is unclear to me. What is the point of compliance requirements for runtimes which don't read or process a configuration? [1]: opencontainers/image-spec#164 [2]: opencontainers#510 (comment) [3]: opencontainers#510 (comment) [4]: opencontainers#811 (comment) [5]: opencontainers#811 (comment) [6]: opencontainers#510 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
Mimic opencontainers/image-spec#164
and they should be kept in-sync
Signed-off-by: Doug Davis dug@us.ibm.com