validate: Check configuration against JSON Schema #197
Conversation
|
It seems you need to add |
|
On Wed, Aug 17, 2016 at 11:43:25PM -0700, Ma Shimiao wrote:
I think I've figured that out now, although it took a few tries ;). I |
|
I'm not sure what's wrong with your PR. I can't find the changed |
|
On Wed, Aug 17, 2016 at 11:57:43PM -0700, Ma Shimiao wrote:
That has: Sorry, we could not display the entire diff because too many files I'd suggest trusting godep to have done the autogenerated commit |
|
It looks good to me. |
|
On Thu, Aug 18, 2016 at 12:18:41AM -0700, Ma Shimiao wrote:
Yeah, that's the sort of thing we'll switch on in JSONSchemaURL. |
cmd/ocitools/validate.go
Outdated
| @@ -61,6 +62,8 @@ var ( | |||
| "CAP_KILL", | |||
| "CAP_AUDIT_WRITE", | |||
| } | |||
|
|
|||
| configSchemaTemplate = "https://raw.githubusercontent.com/opencontainers/runtime-spec/v%s/schema/schema.json" | |||
There was a problem hiding this comment.
We should vendor this file rather than downloading it.
There was a problem hiding this comment.
On Thu, Aug 18, 2016 at 11:41:17AM -0700, Mrunal Patel wrote:
@@ -61,6 +62,8 @@ var (
"CAP_KILL",
"CAP_AUDIT_WRITE",
}
+
- configSchemaTemplate = "https://raw.githubusercontent.com/opencontainers/runtime-spec/v%s/schema/schema.json"
We should vendor this file rather than downloading it.
If we vendor (and I don't see a huge need for that), I'd rather vendor
with the option to download (opencontainers/image-spec#150) ;). The
drawback to only vendoring is that if we don't vendor (and
runtime-spec is consistent about the naming pattern) we can
automatically validate against future schema versions that weren't
available at compile time.
I am a fan of not requiring network activity, but I'd rather support
that with a ‘--schema $URL’ option (something like
opencontainers/runtime-spec#490) so users could run:
$ oci validate --schema file:///path/to/config-schema.json …
There was a problem hiding this comment.
If we are compiling ocitools for a particular runtime-spec version, it makes sense to use the version from the same schema by default.
There was a problem hiding this comment.
On Thu, Sep 01, 2016 at 03:37:26PM -0700, Mrunal Patel wrote:
@@ -61,6 +62,8 @@ var (
"CAP_KILL",
"CAP_AUDIT_WRITE",
}
+
- configSchemaTemplate = "https://raw.githubusercontent.com/opencontainers/runtime-spec/v%s/schema/schema.json"
If we are compiling ocitools for a particular runtime-spec version…
I'd rather not compile ocitools for a particular runtime-spec
version. By staying flexible here we have one less place where we are
pinning ourselves down.
|
+1 for this, I'll remove duplicated checks like 'mandatory', or specific enums ('SeccompArch") after this. can we have less files in vendor? For example 'testsuite' is not necessary. |
|
On Sun, Sep 11, 2016 at 06:45:56AM -0700, David Liang wrote:
I've put my godep invocation in the bf09bc2 commit message. If I |
|
Review here seems to have stalled out. I'm happy to rebase this when folks have time to take another look; just ping me here. |
|
any more comments? @opencontainers/runtime-tools-maintainers |
|
Please contribute to master branch. |
I can rebase onto master and change the target branch without filing a new PR and fragmenting discussion. Can you re-open this PR so I can do that? |
|
I don't think it's a good idea that whether you can validate bundle depends on if your network is OK or not. |
|
But if do as image-spec is not a good way either, I can't think out any better ideas. Maybe we can try to let this in first? @opencontainers/runtime-tools-maintainers |
|
On Fri, Jul 28, 2017 at 03:33:14AM +0000, Ma Shimiao wrote:
I don't think it's a good idea that whether you can validate bundle
depends on if your network is OK or not.
If you use any sort of local caching or --schema override, then
whether you can validate only depends on whether your network was
working when you decided to populate that cache. And we can provide a
nice tar of all the relevant JSON schemas (under opencontainers.org?)
if we want to make that easy for package managers and such.
|
|
@q384566678 wking already works on this year ago +1: |
|
@liangchenye and I plan to focus on this. @wking please rebase and update. |
|
ping @wking |
validate/validate.go
Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| configSchemaVersion, err := semver.Parse("1.0.0-rc2") // config.json became config-schema.json in 1.0.0-rc2 |
There was a problem hiding this comment.
should be 1.0.0?
No, rc2 is when config.jdon became config-schema.json. JSONSchemaURL works with all of our releases, not just 1.0.0.
There was a problem hiding this comment.
Sorry, I can't understand.
Our code now works based on runtime-spec v1.0.0.
Even though schema changes begin rc2, why must be rc2? It's not the latest released version.
There was a problem hiding this comment.
Even though schema changes begin rc2, why must be rc2? It's not the latest released version.
I think the long-term goal is to be able to test against multiple spec versions. See #98 for more on this. @liangchenye at least seemed on board with that idea (if not with my Python approach to it) based on his comment here. I've pushed ea82fad → 9d07098 showing a example of using CheckJSONSchema on an older release (although I didn't go back before rc2) in case that helps clarify. I understand that the rest of the Go-based validation is specific to 1.0.0, but don't think that is a reason to force CheckJSONSchema to be specific to 1.0.0. Having a flexible CheckJSONSchema means we'll have less work to do when we eventually adjust validate to work with multiple versions.
The flexibility of CheckJSONSchema is somewhat limited in the current implementation because it happens after we read JSON into Go (and in some validate workflows there may never have been JSON to begin with). That means we miss things like:
{
"ociVersion": "1.0.0-rc6",
"linux": {
"resources": {
"disableOOMKiller": "I should be a boolean"
}
}
}because since opencontainers/runtime-spec#896 there has been no way to represent linux.resources.disableOOMKiller in Go. But I expect folks will mostly be interested in 1.0.0+, and the SemVer commitment will help us avoid that sort of issue for the 1.x line.
There was a problem hiding this comment.
Ah, sorry. I misunderstand the purpose of configSchemaVersion.
There was a problem hiding this comment.
I misunderstand the purpose of configSchemaVersion.
Maybe I should rename it to configRenamedToConfigSchemaVersion or some such?
|
And CI failed |
Looks like I forgot to format the Go. Will do later today. |
runtime-spec publishes a JSON Schema covering the configuration format
(and other JSON related to runtime-spec) [1]. Reduce duplication of
effort by validating configurations against that schema. For example
this gives us lots allowed value/type checking:
$ cat config.json
{
"ociVersion": "1.0.0-rc6",
"process": {
"cwd": "/",
"args": [
"sh"
],
"user": {
"uid": 1,
"gid": 1
},
"rlimits": [{}]
},
"root": {
"path": "rootfs"
}
}
$ ./oci-runtime-tool validate
3 Errors detected:
process.rlimits.0.type: Does not match pattern '^RLIMIT_[A-Z]+$'
'POSIXRlimit.Type' should not be empty.
rlimit type "" is invalid
without us having to duplicate all the work that the runtime-spec
folks have already done for us.
Only validating the JSON Schema is not sufficient, because
--host-specific (e.g. you're running on a Linux box) and
cross-property constraits (e.g. must create a new UTS namespace if you
set hostname) are difficult/impossible to express in JSON Schema.
The 1.0.0-rc5 test is an example of pulling in JSON Schema from an
older release, since the 'process' property was required in rc5 and
optional in rc6, with opencontainers/runtime-spec@c41ea83d, config:
Make process optional, 2017-02-27, opencontainers#701) landing in between.
[1]: https://github.com/opencontainers/runtime-spec/tree/v1.0.0-rc2/schema
Signed-off-by: W. Trevor King <wking@tremily.us>
Generated with: $ godep save ./... When I originally did this with v74 I needed to move the entries from Godeps/_workspace/src to vendor/ by hand, but now that I'm using godep v77 they're added to vendor/ automatically. I'm not sure why github.com/stretchr/testify/assert and descendants weren't added to Godeps.json back in 15577bd (add runtime struct; add create test, 2017-08-24, opencontainers#447), since that's when they landed in vendor/. The fact that they weren't there means it's hard to tell whether the changes my godep call made are moving the libraries forward or backward in time, but I expect they're moving forward because I just updated them with 'go get -u ...'. Signed-off-by: W. Trevor King <wking@tremily.us>
|
LGTM |
|
@liangchenye @q384566678 @mrunalp PTAL |
|
LGTM |
runtime-spec publishes a JSON Schema covering the configuration format (and other JSON related to runtime-spec). Reduce duplication of effort by validating configurations against that schema. For example this gives us lots of range checking:
without us having to duplicate all the range-input work that the runtime-spec folks have already done for us.
Only validating the JSON Schema is not sufficient, because
--host-specific(e.g. you're running on a Linux box) and cross-property constraits (e.g. must create a new UTS namespace if you set hostname) are difficult/impossible to express in JSON Schema.See related discussion in #195.