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

fix typo and regular expression of algorithim #221

Merged
merged 1 commit into from
Sep 9, 2016

Conversation

x1022as
Copy link

@x1022as x1022as commented Aug 30, 2016

regular expression of algorithim in defs-image.json:
"pattern": "^[a-z0-9]+:[a-fA-F0-9]+$"
make descriptor.md consistent with it.

Signed-off-by: Deng Guangxing dengguangxing@huawei.com

@wking
Copy link
Contributor

wking commented Aug 30, 2016 via email

@philips
Copy link
Contributor

philips commented Aug 30, 2016

LGTM

Approved with PullApprove

@stevvooe
Copy link
Contributor

defs-image.json is not the specification.

This was provide to allow case-insensitivity (although, we don't clarify this). If we do this, we would also have to remove [A-F] from the hex portion. In general, we should maintain that lower(digest) == upper(digest).

@wking
Copy link
Contributor

wking commented Aug 31, 2016

On Wed, Aug 31, 2016 at 01:35:01PM -0700, Stephen Day wrote:

This was provide to allow case-insensitivity (although, we don't
clarify this).

Case-insensitivity isn't hard to support, but I don't see a reason
to support it. Can't we just require lowercase hex?

@wking
Copy link
Contributor

wking commented Aug 31, 2016

On Wed, Aug 31, 2016 at 01:44:38PM -0700, W. Trevor King wrote:

Wed, Aug 31, 2016 at 01:35:01PM -0700, Stephen Day:

This was provide to allow case-insensitivity (although, we don't
clarify this).

Case-insensitivity isn't hard to support, but I don't see a reason
to support it. Can't we just require lowercase hex?

Actually, I'm pretty sure we don't want to support case insensitivity,
because allowing it increases the chances of two otherwise-identical
descriptor (or descriptor-containing) blobs having different hashes.
You could make the APIs case-insensitive if you really want to, but by
the time stuff makes it down to image-layout or the wire, digests
should be all lowercase. I'd rather just require that at the API too
(with engines backed by directories on case-insensitive filesystems
taking care to downcase filenames as they read them off the disk).

It would be nice to have case-sensitive ref names too, but I don't see
an easy way to do that in storage backed by a case-insensitive
filesystem without #174.

@vbatts
Copy link
Member

vbatts commented Sep 7, 2016

LGTM
please rebase

Approved with PullApprove

@x1022as
Copy link
Author

x1022as commented Sep 8, 2016

rebased, waiting for the CI ;)

@jonboulle
Copy link
Contributor

jonboulle commented Sep 8, 2016

LGTM

Approved with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Sep 8, 2016

Sorry guys, not LGTM

The <algorithm> portion of digest needs to support tarsum.v1+sha256 definitions to at least reject them with a proper error message. We also need to be able to handle digest variations, such as sha3-256, blake2b-512 or git+sha1.

Digests in this class are valid syntactically but may not be supported.

This PR should only remove the uppercase components. We should also add a statement that a digest MUST be case flattened before parsing and verifying.

@stevvooe
Copy link
Contributor

stevvooe commented Sep 8, 2016

Sorry I missed the above in the initial review.

@wking
Copy link
Contributor

wking commented Sep 8, 2016

On Thu, Sep 08, 2016 at 12:40:17PM -0700, Stephen Day wrote:

The <algorithm> portion of digest needs to support
tarsum.v1+sha256 definitions to at least reject them with a proper
error message. We also need to be able to handle digest variations,
such as sha3-256, blake2b-512 or git+sha1.

With af00de0, this PR updates descriptor.md to match defs-image.json.
We want that consistency either way, but it sounds like you're asking
for [a-f0-9_+.-]+. While we're adjusting the set of allowed
characters, are there any others that should be included?

@stevvooe
Copy link
Contributor

stevvooe commented Sep 8, 2016

With af00de0, this PR updates descriptor.md to match defs-image.json.

Yes, but as I said before, defs-image.json is not the specification.

I'm asking that [a-z0-9_+.-]+ (note that we cover to z here) be accepted here. This character set is similar to what is allowed by semver, so I don't see a need for expanding this further.

This provides for sufficient future proofing to defer algorithm to supportability validations rather than syntax validation. This will make for a much smoother transition if we ever encounter the dreaded break of sha256.

@wking
Copy link
Contributor

wking commented Sep 8, 2016

On Thu, Sep 08, 2016 at 01:05:07PM -0700, Stephen Day wrote:

With af00de0, this PR updates descriptor.md to match defs-image.json.

Yes, but as I said before, defs-image.json is not the specification.

Agreed, but i think everyone also agrees that the specificaiton in
descriptor.md should be changed. I'm trying to figure out if we have
a consensus on what it should be changed to ;).

I'm asking that [a-z0-9_+.-]+ (note that we cover to z here) be
accepted here.

Sounds good. And I'd missed the ‘z’ earlier, so thanks for pointing
it out :).

@x1022as
Copy link
Author

x1022as commented Sep 9, 2016

On Thu, Sep 08, 2016 at 12:40:17PM -0700, Stephen Day wrote:
The <algorithm> portion of digest needs to support
tarsum.v1+sha256 definitions to at least reject them with a proper
error message. We also need to be able to handle digest variations,
such as sha3-256, blake2b-512 or git+sha1.

this makes [a-z0-9_+.-]+ reasonable, and thus we need to change the def-images.json as well. I think we should make them match anyway. what do others think about it?

change regular expression of algorithim in defs-image.json to:
	"pattern": "^[a-z0-9_+.-]+"
and make descriptor.md consistent with it.

Signed-off-by: Deng Guangxing <dengguangxing@huawei.com>
@jonboulle
Copy link
Contributor

jonboulle commented Sep 9, 2016

LGTM

Approved with PullApprove

@wking
Copy link
Contributor

wking commented Sep 9, 2016 via email

@stevvooe
Copy link
Contributor

stevvooe commented Sep 9, 2016

LGTM

Approved with PullApprove

@stevvooe stevvooe merged commit 5553ed5 into opencontainers:master Sep 9, 2016
wking added a commit to wking/image-spec that referenced this pull request Sep 9, 2016
Now that 75a51ed (fix regular expression of algorithim, 2016-08-30,
opencontainers#221) has made the Markdown and JSON Schema consistent (and required
lowercase algorithms), make digest comparisons easier by also
requiring lowercase hex.

This also:

* Makes it easier to serve blobs out of a case-insensitive filesystem
  store.
* Avoids having two otherwise-identical descriptor (or
  descriptor-containing) blobs with different hashes because they
  picked differend hex-casing.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/image-spec that referenced this pull request Sep 9, 2016
Now that 75a51ed (fix regular expression of algorithim, 2016-08-30,
opencontainers#221) has made the Markdown and JSON Schema consistent (and required
lowercase algorithms), make digest comparisons easier by also
requiring lowercase hex.

This also:

* Makes it easier to serve blobs out of a case-insensitive filesystem
  store.
* Avoids having two otherwise-identical descriptor (or
  descriptor-containing) blobs with different hashes because they
  picked differend hex-casing.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/image-spec that referenced this pull request Sep 9, 2016
Now that 75a51ed (fix regular expression of algorithim, 2016-08-30,
opencontainers#221) has made the Markdown and JSON Schema consistent (and required
lowercase algorithms), make digest comparisons easier by also
requiring lowercase hex.

This also:

* Makes it easier to serve blobs out of a case-insensitive filesystem
  store.
* Avoids having two otherwise-identical descriptor (or
  descriptor-containing) blobs with different hashes because they
  picked differend hex-casing.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/image-spec that referenced this pull request Sep 9, 2016
Now that 75a51ed (fix regular expression of algorithim, 2016-08-30,
opencontainers#221) has made the Markdown and JSON Schema consistent (and required
lowercase algorithms), make digest comparisons easier by also
requiring lowercase hex.

This also:

* Makes it easier to serve blobs out of a case-insensitive filesystem
  store.
* Avoids having two otherwise-identical descriptor (or
  descriptor-containing) blobs with different hashes because they
  picked differend hex-casing.

Signed-off-by: W. Trevor King <wking@tremily.us>
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.

6 participants