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

schema: Completely drop our JSON Schema 'id' properties #945

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

wking
Copy link
Contributor

@wking wking commented Jan 5, 2018

We're using JSON Schema draft-04, as declared by our $schema properties. In draft-04, the id keyword alters the resolution scope. But our current $ref values use JSON Pointers with relative references like defs-linux.json#/definitions/Device that ignore the ids.

By draft-07, id has become $id, and the docs say:

The root schema of a JSON Schema document SHOULD contain an "$id" keyword with a URI (containing a scheme).

But since xeipuuv/gojsonschema@83a7f6369d5, including any URI that cannot be retrieved generates an error:

$ ./validate config-schema.json test/config/good/minimal.json
Could not read schema from HTTP, response status is 404 Not Found

While a root id entry would be nice, we don't currently host these anywhere with a useful URI. We could use GitHub links like this, but then testing pull requests would be difficult.

By draft-07, the purpose of internal $id entries is clearly explained:

Providing a plain name fragment enables a subschema to be relocated within a schema without requiring that JSON Pointer references are updated.

We don't need that, because we control all the references. In the infrequent event of a subschema move, we can update the consuming references in the same commit.

The draft-07 $ref docs also explain that $ref targets may be URNs:

The URI is not a network locator, only an identifier. A schema need not be downloadable from the address if it is a network-addressable URL, and implementations SHOULD NOT assume they should perform a network operation when they encounter a network-addressable URI.

I haven't found analogous wording for $id, but it's possible that gojsonschema is being overly agressive with its attempted retrievals.

This commit removes all of our id entries. The resulting JSON Schema is valid (regardless of where you host it) and does not generate the 404s.

If/when we merge/release this it should address opencontainers/runc#1680.

@wking wking force-pushed the json-schema-minimal-ids branch from 25d6c81 to a3f89ae Compare January 5, 2018 18:20
We're using JSON Schema draft-04 [1], as declared by our '$schema'
properties [2].  In draft-04, the 'id' keyword alters the resolution
scope.  But our current '$ref' values use JSON Pointers [3,4] with
relative references like 'defs-linux.json#/definitions/Device' that
ignore the 'id's.

By draft-07, 'id' has become '$id', and [5]:

  The root schema of a JSON Schema document SHOULD contain an "$id"
  keyword with a URI (containing a scheme).

But since [6], including any URI that cannot be retrieved generates an
error:

  $ ./validate config-schema.json test/config/good/minimal.json
  Could not read schema from HTTP, response status is 404 Not Found

While a root 'id' entry would be nice, we don't currently host these
anywhere with a useful URI.  We could use [7], but then testing pull
requests would be difficult.

By draft-07, the purpose of internal '$id' entries is clearly
explained [5]:

  Providing a plain name fragment enables a subschema to be relocated
  within a schema without requiring that JSON Pointer references are
  updated.

We don't need that, because we control all the references.  In the
infrequent event of a subschema move, we can update the consuming
references in the same commit.

The draft-07 $ref docs also explain that $ref targets may be URNs [8]:

  The URI is not a network locator, only an identifier.  A schema need
  not be downloadable from the address if it is a network-addressable
  URL, and implementations SHOULD NOT assume they should perform a
  network operation when they encounter a network-addressable URI.

I haven't found analogous wording for $id, but it's possible that
gojsonschema is being overly agressive with its attempted retrievals.

This commit removes all of our 'id' entries.  The resulting JSON
Schema is valid (regardless of where you host it) and does not
generate the 404s.

Reported by Tom Godkin [9] and William Martin [10].

[1]: https://tools.ietf.org/html/draft-zyp-json-schema-04#section-7.2
[2]: https://tools.ietf.org/html/draft-zyp-json-schema-04#section-6
[3]: https://tools.ietf.org/html/draft-ietf-appsawg-json-pointer-07
[4]: https://tools.ietf.org/html/rfc6901
[5]: https://tools.ietf.org/html/draft-handrews-json-schema-00#section-9.2
[6]: xeipuuv/gojsonschema@83a7f63
[7]: https://raw.githubusercontent.com/opencontainers/runtime-spec/v1.0.1/schema/config-schema.json
[8]: https://tools.ietf.org/html/draft-handrews-json-schema-00#section-8
[9]: opencontainers/runc#1680
[10]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/L9ME-YRPmmc
      Subject: runtime-spec validation questions
      Date: Thu, 4 Jan 2018 15:47:50 +0000
      Message-ID: <CAMp6QwMTJab5K25=CVy=6OZV6NRX0s-nMLGwqC8ZMpFEp5bF_Q@mail.gmail.com>

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

crosbymichael commented Feb 12, 2018

LGTM

Approved with PullApprove

1 similar comment
@tianon
Copy link
Member

tianon commented Feb 15, 2018

LGTM

Approved with PullApprove

@tianon tianon merged commit 520c634 into opencontainers:master Feb 15, 2018
@wking wking deleted the json-schema-minimal-ids branch February 15, 2018 21:21
@vbatts vbatts mentioned this pull request Jan 9, 2020
kolyshkin added a commit to kolyshkin/runtime-tools that referenced this pull request Oct 19, 2021
New(er) xeipuuv/gojsonschema package is trying to fetch id fields,
which in the spec were looking like this:

	"id": "https://opencontainers.org/schema/bundle/linux"

Obviously, this results in HTTP 404s, and multiple test failures.

This was fixed by opencontainers/runtime-spec#945
which ended up in runtime-spec v1.0.2.

Now, if we want to bump xeipuuv/gojsonschema (we do), we need to test
against at least v1.0.2 of runtime-spec, for the reason explained above.

Bump the spec version in all test cases, remove or fix some test cases.

In particular:

 * remove "process is required" as it needed v1.0.0-rc5 version of spec.
 * remove "args is required" as args are no longer required since commit
   opencontainers/runtime-spec@deb4d954eafc4fc.
 * fixup "invalid seccomp action" error as it now also has SCMP_ACT_LOG.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runtime-tools that referenced this pull request Oct 19, 2021
New(er) xeipuuv/gojsonschema package is trying to fetch id fields,
which in the spec were looking like this:

	"id": "https://opencontainers.org/schema/bundle/linux"

Obviously, this results in HTTP 404s, and multiple test failures.

This was fixed by opencontainers/runtime-spec#945
which ended up in runtime-spec v1.0.2.

Now, if we want to bump xeipuuv/gojsonschema (we do), we need to test
against at least v1.0.2 of runtime-spec, for the reason explained above.

Bump the spec version in all test cases, remove or fix some test cases.

In particular:

 * remove "process is required" as it needed v1.0.0-rc5 version of spec.
 * remove "args is required" as args are no longer required since commit
   opencontainers/runtime-spec@deb4d954eafc4fc.
 * fixup "invalid seccomp action" error as it now also has SCMP_ACT_LOG.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runtime-tools that referenced this pull request Oct 20, 2021
New(er) xeipuuv/gojsonschema package is trying to fetch id fields,
which in the spec were looking like this:

	"id": "https://opencontainers.org/schema/bundle/linux"

Obviously, this results in HTTP 404s, and multiple test failures.

This was fixed by opencontainers/runtime-spec#945
which ended up in runtime-spec v1.0.2.

Now, if we want to bump xeipuuv/gojsonschema (we do), we need to test
against at least v1.0.2 of runtime-spec, for the reason explained above.

Bump the spec version in all test cases, remove or fix some test cases.

In particular:

 * remove "process is required" as it needed v1.0.0-rc5 version of spec.
 * remove "args is required" as args are no longer required since commit
   opencontainers/runtime-spec@deb4d954eafc4fc.
 * fixup "invalid seccomp action" error as it now also has SCMP_ACT_LOG.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runtime-tools that referenced this pull request Oct 20, 2021
New(er) xeipuuv/gojsonschema package is trying to fetch id fields,
which in the spec were looking like this:

	"id": "https://opencontainers.org/schema/bundle/linux"

Obviously, this results in HTTP 404s, and multiple test failures.

This was fixed by opencontainers/runtime-spec#945
which ended up in runtime-spec v1.0.2.

Now, if we want to bump xeipuuv/gojsonschema (we do), we need to test
against at least v1.0.2 of runtime-spec, for the reason explained above.

Bump the spec version in all test cases, remove or fix some test cases.

In particular:

 * remove "process is required" as it needed v1.0.0-rc5 version of spec.
 * remove "args is required" as args are no longer required since commit
   opencontainers/runtime-spec@deb4d954eafc4fc.
 * fixup "invalid seccomp action" error as it now also has SCMP_ACT_LOG.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runtime-tools that referenced this pull request Oct 20, 2021
New(er) xeipuuv/gojsonschema package is trying to fetch id fields,
which in the spec were looking like this:

	"id": "https://opencontainers.org/schema/bundle/linux"

Obviously, this results in HTTP 404s, and multiple test failures.

This was fixed by opencontainers/runtime-spec#945
which ended up in runtime-spec v1.0.2.

This essentially means with newer xeipuuv/gojsonschema we are no longer
able to validate against runtime-spec < 1.0.2.

To adopt for a new xeipuuv/gojsonschema, do the following:

1. Add the version check, add a test case for it.

2. Remove some test cases:

   - "process is required" as it needed v1.0.0-rc5 version of spec.
   - "args is required" as args are no longer required since commit
      opencontainers/runtime-spec@deb4d954eafc4fc.

3. Bump the spec version in all test cases.

4. Fix "invalid seccomp action" error as it now also has SCMP_ACT_LOG.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin added a commit to kolyshkin/runtime-tools that referenced this pull request Oct 20, 2021
New(er) xeipuuv/gojsonschema package is trying to fetch id fields,
which in the spec were looking like this:

	"id": "https://opencontainers.org/schema/bundle/linux"

Obviously, this results in HTTP 404s, and multiple test failures.

This was fixed by opencontainers/runtime-spec#945
which ended up in runtime-spec v1.0.2.

This essentially means with newer xeipuuv/gojsonschema we are no longer
able to validate against runtime-spec < 1.0.2.

To adopt for a new xeipuuv/gojsonschema, do the following:

1. Add the version check, add a test case for it.

2. Remove some test cases:

   - "process is required" as it needed v1.0.0-rc5 version of spec.
   - "args is required" as args are no longer required since commit
      opencontainers/runtime-spec@deb4d954eafc4fc.

3. Bump the spec version in all test cases.

4. Fix "invalid seccomp action" error as it now also has SCMP_ACT_LOG.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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.

3 participants