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: Do not allow runtimes to ignore properties defined by the spec #680

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wking
Copy link
Contributor

@wking wking commented Feb 7, 2017

Otherwise a runtime could silently ignore a property it didn't want to implement, which would be confusing for runtime callers. This closes a potential loophole in the restriction from #559.

@wking
Copy link
Contributor Author

wking commented Feb 7, 2017

This would benefit from the clearer platform/protocol slug language in #570, so if #570 lands first I'll rebase this PR around it and tighten the “target platform” wording here.

@wking
Copy link
Contributor Author

wking commented Apr 7, 2017

This would benefit from the clearer platform/protocol slug language in #570

#570 landed, and I've floated some work tying settings to platforms in #747. I recommend reviewing #747 first, and then I'll reroll this if/when that lands.

@crosbymichael
Copy link
Member

We are going to figure out a better way to word this.

config.md Outdated
@@ -415,6 +415,7 @@ Values MAY be an empty string.
## Extensibility
Implementations that are reading/processing this configuration file MUST NOT generate an error if they encounter an unknown property.
Instead they MUST ignore unknown properties.
Properties defined for the [target platform](#platform) by the [declared version](#specification-version) of this specification MUST NOT be ignored.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tianon questioned the “for the target platform” portion of this in #819, saying he would be surprised if the runtime did not error when the config set a property that this spec only defined for another platform (e.g. the Windows-only process.user.username in a Linux config). I, on the other hand, would be very surprised if the runtime cared about those properties at all (and I see no code in runC checking Process.Username). So whatever we end up doing to clarify the intended behavior, we'll want to cover what happens in situations like that.

@vbatts
Copy link
Member

vbatts commented May 24, 2017

let's make a decision on whether this is required for v1.0.0

@tianon
Copy link
Member

tianon commented May 24, 2017

IMO we can decide on the specifics of this edge case post-1.0

@crosbymichael what's your opinion on clarifying this being a 1.0 or 1.1 concern?

@wking
Copy link
Contributor Author

wking commented May 24, 2017 via email

@wking wking mentioned this pull request May 24, 2017
Otherwise a runtime could silently ignore a property it didn't want to
implement, which would be confusing for runtime callers [1].  This
closes a potential loophole in the restriction from 766abd6
(runtime.md: Require 'create' to fail if config.json asks for the
impossible, 2016-09-08, opencontainers#559).

[1]: opencontainers#472 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Jun 16, 2017

Rebased around #850 with 19ed44cffa1718, changing the “target platform” link from #platform to spec.md#platforms.

@justincormack
Copy link
Contributor

justincormack commented Jun 27, 2017

Does this mean that everything that cannot be defined for one platform must be moved to platform specific code or explicitly annotated?

eg currently root.readonly is not annotated as every platform except Windows.

@wking
Copy link
Contributor Author

wking commented Jul 10, 2017 via email

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

house keeping:
is this still an interested topic?

@h-vetinari h-vetinari mentioned this pull request Feb 3, 2020
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