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

Update 1.0 branch for 1.0.3 release #955

Closed
wants to merge 88 commits into from

Conversation

sudo-bmitch
Copy link
Contributor

@sudo-bmitch sudo-bmitch commented Sep 16, 2022

This is a cherry-pick of commits from the v1.0.1 release to just before we merged in the working group results, and a few additional Go and CI fixes after that. The v1.0.1 release was picked because there were changes during the past 5 years that were not included in the v1.0.2 security fix.

The following should be on the list of PR's included in this update:

decdb15 Merge pull request #951 from sudo-bmitch/pr-go-1.17
d2cc46d Merge pull request #944 from austinvazquez/remove-ioutil-dep
50b6194 Merge pull request #941 from sudo-bmitch/pr-spec-components
ff02571 Merge pull request #934 from oci-playground/pr
d265d74 Merge pull request #897 from vbatts/another_adopter
c27355a Merge pull request #932 from sudo-bmitch/pr-implementation-regclient
f3096cf Merge pull request #911 from jdolitsky/sajay
2014771 Merge pull request #909 from jdolitsky/brandon
6ad7100 Merge pull request #919 from nishakm/912-maintainer-template
7b36cea Merge pull request #917 from sudo-bmitch/pr-gha-lint
8685486 Merge pull request #915 from sudo-bmitch/pr-https
37d82a2 Merge pull request #910 from jdolitsky/old-maintainers
02efb9a Merge pull request #900 from michaelb990/main
8205360 Merge pull request #885 from stevvooe/use-embed
9716dee Merge pull request #898 from sudo-bmitch/pr-broken-link
bac4452 Merge pull request #826 from jonjohnsonjr/data
1f308fd Merge pull request #896 from vbatts/implementations
ea0209f Merge pull request #887 from sanshirookazaki/fix-readme
eacdcc1 Merge pull request #871 from wyckster/patch-2
a9fdc37 Merge pull request #884 from vbatts/fix_lint
a5463b7 Merge pull request #883 from vbatts/carry_810
6793483 Merge pull request #882 from bloodorangeio/why-u-pull-quay
b52c172 Merge pull request #879 from opencontainers/lint-fixes
c5a74bc Merge pull request #878 from opencontainers/v1.0
43a7dee Merge pull request #869 from SteveLasker/add-acr
9a7a987 Merge pull request #875 from SteveLasker/docker-distribution
54a822e Merge pull request #873 from jpetazzo/fix-config-example
5ad6f50 Merge pull request #817 from AkihiroSuda/docker-oci-diff
8e42a01 Merge pull request #822 from imjasonh/patch-1
fe0a249 Merge pull request #809 from cpuguy83/image-platform
91bc345 Merge pull request #863 from vbatts/rm-pullapprove
5ced465 Merge pull request #856 from vsoch/patch-1
d9aa673 Merge pull request #793 from vsoch/fix/descriptor-branch-comments
fe7b394 Merge pull request #824 from AidanDelaney/patch-1
93e69bc Merge pull request #800 from zhsj/remove-go4
083f635 Merge pull request #860 from lucianoqueiroz/add-background-to-png-imgs
85c28d7 Merge pull request #858 from vbatts/codeowners
f18daf2 Merge pull request #855 from jonjohnsonjr/typo
81270cd Merge pull request #813 from Iduoad/master
8d2a44c Merge pull request #846 from vbatts/oci-container
32e130c Merge pull request #848 from vsoch/add/github-workflows
daf5154 Merge pull request #849 from jonboulle/master
269c1c5 Merge pull request #832 from ImJasonH/patch-2
859973e Merge pull request #814 from vbatts/change_image
775207b Merge pull request #790 from vrothberg/add-zstd-constants
9f4348a Merge pull request #788 from giuseppe/zstd
d110911 Merge pull request #786 from estesp/meeting-ref
6943c5f Merge pull request #757 from unasuke/fix_typo
ae1010f Merge pull request #782 from odinuge/lint-issue
da296dc Merge pull request #772 from woernfl/patch-1
243ea08 Merge pull request #759 from jzelinskie/loosen-mediatypes
09950c5 Merge pull request #753 from HaraldNordgren/go_versions
b6e51fa Merge pull request #740 from francescomari/fix-link
e562b04 Merge pull request #716 from AkihiroSuda/index-is-required
1492521 Merge pull request #739 from mtrmac/id-based-loader
577479e Merge pull request #736 from AkihiroSuda/bump-up
89b51c7 Merge pull request #728 from q384566678/add-ln

Note the DCO issue will need to be ignored because it exists in our main branch today for a minor update to email addresses.

Fixes #918

cyphar and others added 30 commits September 25, 2022 10:23
In b6d5a8c ("Change platform ref from runtime-spec"), the
conversion to runtime-spec for the "os" and "architecture" fields was
removed (as the fields had also been removed in runtime-spec). Re-add
the conversion as an annotation field rather than a verbatim field.

Signed-off-by: Aleksa Sarai <[email protected]>
Signed-off-by: zhouhao <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
This only commits the result of (make schema-fs) and is otherwise
unrelated to the rest of the PR.

Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Francesco Mari <[email protected]>
After updating gojsonschema to include
xeipuuv/gojsonschema#171 , tests fail with
> unable to validate: Could not read schema from HTTP, response status is 404 Not Found

Before that gojsonschema change, "$ref" links were interpreted by taking
the current schema source file's URI as a base, and treating "$ref"
as relative to this.

For example, starting with the [file://]/image-manifest-schema.json
URI, as used by Validator.Validate (based on the "specs" map), the
>  "$ref": "content-descriptor.json"
reference used to evaluate to file:///content-descriptor.json.
gojsonschema.jsonReferenceLoader would then load these file:///*.json
URIs via _escFS.

After the gojsonschema change, "$ref" links are evaluated relative to
a URI base specified by the "id" attribute inside the schema source,
regardless of the "external" URI passed to the gojsonschema.JSONLoader.

This is consistent with
http://json-schema.org/latest/json-schema-core.html#rfc.section.8 and
http://json-schema.org/latest/json-schema-core.html#rfc.section.9.2
(apart from the "id" vs. "$id" attribute name).

In the same example, [file://]/image-manifest-schema.json URI contains
>  "id": "https://opencontainers.org/schema/image/manifest",
so the same
>  "$ref": "content-descriptor.json"
now evaluates to
"https://opencontainers.org/schema/image/content-descriptor.json",
which is not found by gojsonschema.jsonReferenceLoader (it uses
_escFS only for file:/// URIs), resulting in the 404 quoted above.

This is a minimal fix, making the schema files available to
gojsonschema at the https:// URIs, while continuing to read them from
_escFS.

Because gojsonschema.jsonReferenceLoader can only use the provided fs
for file:/// URIs, we are forced to implement our own
gojsonschema.JSONLoaderFactory and gojsonschema.JSONLoader; something
like this might be more generally useful and should therefore instead
be provided by the gojsonschema library.

This particular JSONLoader{Factory,} implementation, though, is
image-spec specific because it locally works around various
inconsistencies in the image-spec JSON schemas, and thus is not suitable
for gojsonschema as is.

Namely, the specs/*.json schema files use URIs with two URI path prefixes,
https://opencontainers.org/schema/{,image/}
in the top-level "id" attributes, and the nested "id" attributes along
with "$ref" references use _several more_ URI path prefixes, e.g.
>       "id": "https://opencontainers.org/schema/image/manifest/annotations",
>      "$ref": "defs-descriptor.json#/definitions/annotations"
in image-manifest-schema.json specifies the
https://opencontainers.org/schema/image/manifest/defs-descriptor.json
URI.

In fact, defs-descriptor.json references use all of the following URIs:
> https://opencontainers.org/schema/defs-descriptor.json
> https://opencontainers.org/schema/image/defs-descriptor.json
> https://opencontainers.org/schema/image/descriptor/defs-descriptor.json
> https://opencontainers.org/schema/image/index/defs-descriptor.json
> https://opencontainers.org/schema/image/manifest/defs-descriptor.json

So, this commit introduces a loader which preserves the original _escFS
layout by recognizing and stripping all of these prefixes, and using
the same /*.json paths for _escFS lookups as before; this is clearly
unsuitable for gojsonschema inclusion.

Finally, the reason this commit uses such a fairly hacky loader is that merely
changing the _escFS structure is still not sufficient to get consistent
schema: the schema/*.json paths in this repository, and the "$ref" values,
do not match the "id" values inside the schemas at all.  E.g.
image-manifest-schema.json refers to
https://opencontainers.org/schema/image/manifest/content-descriptor.json ,
while content-descriptor.json identifies itself as
https://opencontainers.org/schema/descriptor , matching neither the path prefix
nor the file name.

Overall, it is completely unclear to me which of the URIs is the canonical URI
of the "content descriptor" schema, and the owner of the URI namespace
needs to decide on the canonical schema URIs.  Only afterwards can the
code be cleanly modified to match the specification; until then, this
commit at least keeps the tests passing, and the validator usable
by external callers who want to use the public
image-spec/schema.ValidateMediaType*.Validate() API.

Signed-off-by: Miloslav Trmač <[email protected]>
The "id" values in JSON schema files must be unique, per RFC draft 8.3.1:
> A schema MAY (and likely will) have multiple URIs, but there is no
> way for a URI to identify more than one schema.
and recent gojsonschema fails when handling such inputs (fairly
nontransparently, it silently fails to resolve $ref references to
absolute URIs and reports something like
> Reference defs.json#/definitions/mapStringString must be canonical
.)

In particular, the https://opencontainers.org/schema/image/descriptor/annotations
id value had three definitions.  To resolve this:
- Leave the definition in image-index-schema.json; although using the /descriptor
  subnamespace for the "manifests" array is a bit surprising, the /image/ part
  clearly belongs to image-index-schema.json
- Rename the id definition in content-descriptor.json, to use the generic
  "blob descriptor" namespace.
- Remove the definition in defs-descriptor.json; that seems to be an "utility"
  schema file describing common structures, but it's better for users to
  reference schema fragments by purpose than by common structure (so that
  we can let the structure diverge in the future if necessary).

Finally, changing the content-descriptor.json "id" value changes the
resolved absolute value of the reference to defs-descriptor.json,
so add another namespace to be handled by fsLoaderFactory.

Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Harald Nordgren <[email protected]>
Move "prefix" to out of the code backtick.

Signed-off-by: Yusuke Nakamura <[email protected]>
Signed-off-by: Vanessa Sochat <[email protected]>
Make error string non-capitalized, to fix linting problem

Signed-off-by: Odin Ugedal <[email protected]>
Signed-off-by: Odin Ugedal <[email protected]>
Point to one canonical location for meeting details, frequency, minutes,
agenda, etc.

Signed-off-by: Phil Estes <[email protected]>
Add go constants for the zstd MIME types to make them usable by
consumers of the go package.

Signed-off-by: Valentin Rothberg <[email protected]>
I have not been active in the project for quite some time so I am
stepping down.

Thank you for all of the work over the years to standardize container
formats everyone!

Signed-off-by: Brandon Philips <[email protected]>
The offset to line/col translation is easy to implement. Not worthy
to bring a third-party dependency.

PS. I think this code is never used, but removing it is too aggressive
since it exposes public api.

Signed-off-by: Shengjing Zhu <[email protected]>
Signed-off-by: Mohammed Daoudi <[email protected]>
Switch approved by Jason Bouzane via email to OCI TOB mailing list.

Signed-off-by: Phil Estes <[email protected]>
vbatts and others added 22 commits September 25, 2022 10:25
Signed-off-by: Vincent Batts <[email protected]>
The OCI scope table no-longer exists.

Fixes opencontainers#812

Signed-off-by: sanshirookazaki <[email protected]>
having seen opencontainers#895, it's worth ensuring these other languages are listed
implementations

Signed-off-by: Vincent Batts <[email protected]>
Signed-off-by: Brandon Mitchell <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
Signed-off-by: Brandon Mitchell <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
This is a PR template to propose adding new maintainers

Fixes opencontainers#912

Signed-off-by: nisha <[email protected]>
Signed-off-by: Brandon Mitchell <[email protected]>
io/ioutil is deprecated since Go 1.16

Signed-off-by: Austin Vazquez <[email protected]>
Signed-off-by: Brandon Mitchell <[email protected]>
@thaJeztah
Copy link
Member

Was there there a reason not to tag the next release from the main branch? I think the v1.0 branch was purely created (temporarily) to do a security release without pulling in upcoming changes from main; while it may serve purpose to backport specific critical fixes to a v1.0.x release, I would consider tagging the next release from main instead

@sudo-bmitch
Copy link
Contributor Author

We'd still need a branch for some Go and CI fixes that can't be tagged on main without pulling in the working group changes. Given that even v1.0.1 was released from the branch, I'm still trying to understand the normal release process.

@thaJeztah
Copy link
Member

can't be tagged on main without pulling in the working group changes

Does that work contain backward incompatible changes, or is that because it's not yet complete? Mostly wondering if there's a need to have an "LTS-like" v1.0 branch if a v1.1.0 release is being worked on. Go modules doesn't work well (or at all) with release branches (if bases any pseudo version it creates on "last semver(ish) tag on the default branch"). Tagging releases from a release branch, not from the "default" branch, will make those tags be ignored, so (e.g.) when tagging v1.1.0 from the v1 branch, the main branch will still be versioned v1.0.1-<date>-<commit>.

Given that even v1.0.1 was released from the branch

It wasn't created from the branch; d600991 is in main (and was merged as part of #734). It probably shows up as "part of" the v1.0 branch because that branch was created later on from the v1.0.1 tag ( in preparation of v1.0.2).

If I recall correctly, the v1 branch was created specific for the security release; normal releases would come from the default branch, but as v1.0.2 was a security release, the intent was to only contain those changes, so the (temporary) branch was created from the v1.0.1 tag. The intent at the time was to merge the security fix into the v1.0 branch, then merge the v1.0.2 tag back into main, which happened in #878 (after which the v1.0 branch probably should've been deleted).

Unfortunately something went wrong in the process of publishing the advisory, as the temporary GitHub security fork contained two PR's; one targeting the v1.0 branch, and one targeting main (which should've been closed); when publishing the advisory, both PR's were merged automatically, so there were now two distinct commits with the security fix;

git log --grep GHSA-77vh-xpmg-72qh
commit 0126d30a90249ba624d134266b662e1d661381f5 (upstream/v1.0)
Merge: d600991 e3885ce
Author: Vincent Batts <[email protected]>
Date:   Wed Nov 17 13:12:56 2021 -0500

    Merge pull request from GHSA-77vh-xpmg-72qh

    Advisory fix 2

commit 693428a734f5bab1a84bd2f990d92ef1111cd60c
Merge: 9a7a987 6ced3bd
Author: Vincent Batts <[email protected]>
Date:   Wed Nov 17 13:12:55 2021 -0500

    Merge pull request from GHSA-77vh-xpmg-72qh

    *.md: bring mediaType out of reserved status

@sudo-bmitch
Copy link
Contributor Author

can't be tagged on main without pulling in the working group changes

Does that work contain backward incompatible changes, or is that because it's not yet complete? Mostly wondering if there's a need to have an "LTS-like" v1.0 branch if a v1.1.0 release is being worked on.

We want to give v1.1.0 some time in RC to allow implementations to be written and prove it out. And the changes we want to include in v1.0.3 after the working group commit will fix CI from breaking. My hope is this is the last v1.0.x release.

Go modules doesn't work well (or at all) with release branches (if bases any pseudo version it creates on "last semver(ish) tag on the default branch"). Tagging releases from a release branch, not from the "default" branch, will make those tags be ignored, so (e.g.) when tagging v1.1.0 from the v1 branch, the main branch will still be versioned v1.0.1-<date>-<commit>.

Do you have more details on this. My google foo is failing and I'm not seeing it on my own projects where I do tagging on release branches. I see that when a project pulls from a newer commit, so if they update to @main, they'll see the last tag on the main branch + the date/commit hash (confusing but not unusable). But if they pull the most recent v1 using @latest, I think they'd get the latest tag even if it's on a branch. Maybe we're saying the same thing, I just want to be sure "ignored" doesn't mean more than that.

@vbatts
Copy link
Member

vbatts commented Feb 12, 2023

@sudo-bmitch can we close this PR? what's the status here?

@sudo-bmitch
Copy link
Contributor Author

I can think of 3 options:

  1. Refuse Please release 1.0.3 #918 and close this.
  2. Accept this and tag a v1.0.3.
  3. Move the1.0 branch off of main rather than cherry picking into the branch. There would still be some cherry picked commits to include from after the WG additions to allow CI to pass.

@sudo-bmitch sudo-bmitch closed this May 4, 2023
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.