validate: Check configuration against JSON Schema by wking · Pull Request #197 · opencontainers/runtime-tools

wking · 2016-08-18T06:31:10Z

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:

$ ocitools validate
...
1. linux.resources.oomScoreAdj: Must be greater than or equal to -1000
...

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.

Mashimiao · 2016-08-18T06:43:22Z

It seems you need to add gojsonschema to godeps

wking · 2016-08-18T06:50:00Z

On Wed, Aug 17, 2016 at 11:43:25PM -0700, Ma Shimiao wrote:

It seems you need to add gojsonschema to godeps

I think I've figured that out now, although it took a few tries ;). I
don't write much Go :p.

Mashimiao · 2016-08-18T06:57:43Z

I'm not sure what's wrong with your PR. I can't find the changed validate.go in https://github.com/opencontainers/ocitools/pull/197/files

wking · 2016-08-18T07:01:40Z

On Wed, Aug 17, 2016 at 11:57:43PM -0700, Ma Shimiao wrote:

I'm not sure what's wrong with your PR. I can't find the changed
validate.go in
https://github.com/opencontainers/ocitools/pull/197/files

That has:

Sorry, we could not display the entire diff because too many files
(392) changed.

I'd suggest trusting godep to have done the autogenerated commit
correctly, and then just reviewing 1.

Mashimiao · 2016-08-18T07:18:40Z

It looks good to me.
Except, schema.json has been changed to config-shema.json in rumtime's upstream. It seems we have to change URL based on spec version in the later.

wking · 2016-08-18T16:05:13Z

On Thu, Aug 18, 2016 at 12:18:41AM -0700, Ma Shimiao wrote:

Except, schema.json has been changed to config-shema.json in
rumtime's upstream. It seems we have to change URL based on spec
version in the later.

Yeah, that's the sort of thing we'll switch on in JSONSchemaURL.

mrunalp · 2016-08-18T18:41:16Z

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

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 …

If we are compiling ocitools for a particular runtime-spec version, it makes sense to use the version from the same schema by default.

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.

wking · 2016-08-18T20:09:14Z

I just pushed 9564b11 → bf09bc2 bumping the commit timestamp to get a
new hash, since GitHub seems to be confused by 9564b11 (never ran
Travis and think's @mrunalp is commenting on an outdated diff 1).

initlove · 2016-09-11T13:45:55Z

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

wking · 2016-09-14T06:19:02Z

On Sun, Sep 11, 2016 at 06:45:56AM -0700, David Liang wrote:

can we have less files in vendor? For example 'testsuite' is not
necessary.

I've put my godep invocation in the bf09bc2 commit message. If I
should be running something else to update godeps, let me know.

wking · 2016-11-07T22:00:22Z

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.

Mashimiao · 2016-11-08T03:34:04Z

any more comments? @opencontainers/runtime-tools-maintainers

Mashimiao · 2017-07-19T07:18:45Z

Please contribute to master branch.

wking · 2017-07-19T14:53:37Z

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?

wking · 2017-07-25T17:24:32Z

I've pushed afd025f → cfb2f1c to shift the vendored content from Godeps/_workspace/src to vendor, although I'm not sure why my godep didn't put them there on its own.

Mashimiao · 2017-07-28T03:33:12Z

I don't think it's a good idea that whether you can validate bundle depends on if your network is OK or not.

Mashimiao · 2017-07-28T03:36:06Z

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

wking · 2017-07-28T04:39:26Z

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.

liangchenye · 2017-08-30T06:37:10Z

@q384566678 wking already works on this year ago +1:
we can review and work on this.

Mashimiao · 2017-09-01T09:03:44Z

@liangchenye and I plan to focus on this. @wking please rebase and update.

Mashimiao · 2017-09-07T01:37:31Z

ping @wking

wking · 2017-09-09T05:32:21Z

Rebased and added some tests with cfb2f1c → 17713b5.

Mashimiao · 2017-09-09T06:31:29Z

+	if err != nil {
+ return "", err
+	}
+	configSchemaVersion, err := semver.Parse("1.0.0-rc2") // config.json became config-schema.json in 1.0.0-rc2


should be 1.0.0?

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.

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.

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.

Ah, sorry. I misunderstand the purpose of configSchemaVersion.

I misunderstand the purpose of configSchemaVersion.
Maybe I should rename it to configRenamedToConfigSchemaVersion or some such?

I think it will be better

I think it will be better
Renamed in 9d07098 → 3935592.

Mashimiao · 2017-09-09T06:32:07Z

And CI failed

wking · 2017-09-09T13:52:06Z

And CI failed

Looks like I forgot to format the Go. Will do later today.

wking · 2017-09-11T04:19:09Z

On Sat, Sep 09, 2017 at 01:52:07PM +0000, W. Trevor King wrote: Looks like I forgot to format the Go.

Fixed the formatting with 17713b5 → ea82fad.

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>

Mashimiao · 2017-09-12T01:17:24Z

LGTM

Mashimiao · 2017-09-12T01:46:46Z

@liangchenye @q384566678 @mrunalp PTAL

zhouhao3 · 2017-09-14T01:24:34Z

LGTM

wking force-pushed the json-schema branch from d81eb6f to d4b604cCompare August 18, 2016 06:31

wking mentioned this pull request Aug 18, 2016
validation: add linux resource check #195
Merged

wking force-pushed the json-schema branch from 75dd401 to 9564b11Compare August 18, 2016 06:43

mrunalp reviewed Aug 18, 2016
View reviewed changes

wking force-pushed the json-schema branch from 9564b11 to bf09bc2Compare August 18, 2016 20:05

wking mentioned this pull request Aug 20, 2016
Range-checking for generate options #186
Open

wking mentioned this pull request Sep 22, 2016
Drop image tools cmd/ and image/ opencontainers/image-spec#337
Merged

wking mentioned this pull request Nov 7, 2016
schema/config-linux: add min/max limit for resource memory and cpu opencontainers/runtime-spec#533
Closed

wking mentioned this pull request Jan 24, 2017
validate: add validation for OOMScoreAdj #316
Closed

Mashimiao modified the milestone: 0.7Apr 12, 2017

wking mentioned this pull request Apr 14, 2017
config: Require strictly-positive timeout values opencontainers/runtime-spec#764
Merged

wking mentioned this pull request Apr 28, 2017
validate: add the validation of Resources.devices #374
Merged

wking mentioned this pull request May 23, 2017
config-linux: Forbid the empty string for mountLabel opencontainers/runtime-spec#843
Closed

Mashimiao closed this Jul 19, 2017

Mashimiao reopened this Jul 19, 2017

wking changed the base branch from v1.0.0.rc1 to masterJuly 19, 2017 15:32

wking force-pushed the json-schema branch from 3392ea6 to cfb2f1cCompare July 25, 2017 17:23

wking mentioned this pull request Aug 30, 2017
validate config.json by https://github.com/opencontainers/runtime-spec/tree/master/schema #453
Closed

Mashimiao mentioned this pull request Sep 1, 2017
validate: Increase the valid value of devices.type #459
Closed

wking force-pushed the json-schema branch from cfb2f1c to 17713b5Compare September 9, 2017 05:31

Mashimiao reviewed Sep 9, 2017
View reviewed changes

wking force-pushed the json-schema branch from 17713b5 to ea82fadCompare September 11, 2017 04:16

wking force-pushed the json-schema branch from ea82fad to 9d07098Compare September 11, 2017 08:04

wking added 2 commits September 11, 2017 03:01

wking force-pushed the json-schema branch from 9d07098 to 3935592Compare September 11, 2017 10:01

Mashimiao merged commit 4882fc1 into opencontainers:masterSep 14, 2017

zhouhao3 mentioned this pull request Sep 14, 2017
validate: Delete the extra validation #473
Merged

wking deleted the json-schema branch September 14, 2017 16:37

Mashimiao removed this from the v0.7.0 milestone Sep 15, 2017

wking mentioned this pull request Oct 26, 2017
validation requires a network connection #499
Open

wking mentioned this pull request Dec 15, 2017
Recursive propagation flags should be legal to use #543
Merged

Uh oh!

Conversation

wking commented Aug 18, 2016

Uh oh!

Mashimiao commented Aug 18, 2016

Uh oh!

wking commented Aug 18, 2016

Uh oh!

Mashimiao commented Aug 18, 2016

Uh oh!

wking commented Aug 18, 2016

Uh oh!

Mashimiao commented Aug 18, 2016

Uh oh!

wking commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 18, 2016

Uh oh!

initlove commented Sep 11, 2016

Uh oh!

wking commented Sep 14, 2016

Uh oh!

wking commented Nov 7, 2016

Uh oh!

Mashimiao commented Nov 8, 2016

Uh oh!

Mashimiao commented Jul 19, 2017

Uh oh!

wking commented Jul 19, 2017

Uh oh!

wking commented Jul 25, 2017

Uh oh!

Mashimiao commented Jul 28, 2017

Uh oh!

Mashimiao commented Jul 28, 2017

Uh oh!

wking commented Jul 28, 2017 via email

Uh oh!

liangchenye commented Aug 30, 2017

Uh oh!

Mashimiao commented Sep 1, 2017

Uh oh!

Mashimiao commented Sep 7, 2017

Uh oh!

wking commented Sep 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mashimiao commented Sep 9, 2017

Uh oh!

wking commented Sep 9, 2017

Uh oh!

wking commented Sep 11, 2017 via email

Uh oh!

Mashimiao commented Sep 12, 2017• edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mashimiao commented Sep 12, 2017

Mashimiao commented Sep 12, 2017•
edited by caniszczyk
Loading

zhouhao3 commented Sep 14, 2017•
edited by caniszczyk
Loading