config variable in backwards compatibility test

[xiekeyang]

The variable name and media type in HTTP header should be config, but not manifest.

Signed-off-by: xiekeyang [email protected]

[wking]

0d477e4 looks good to me, although I'd be happier about having the cURL comment there if it had actually been tested ;). I don't remember enough about Docker's auth to know what to put in the the auth header. Are there no configs we can fetch from Docker's registry without auth?

[xiekeyang]

@wking
Maybe I'm misunderstanding what you mean. https://github.com/opencontainers/image-spec/blob/master/schema/manifest_backwards_compatibility_test.go#L134 seems enough to consumer, do you need more?
Docker Hub enable the authorization, so the auth header of Json Web Token(JWT) is necessary. JWT generation requires repository name, action, account name,password, and so on.

[wking]

On Wed, Oct 05, 2016 at 10:33:45PM -0700, xiekeyang wrote:

Maybe I'm misunderstanding what you
mean. https://github.com/opencontainers/image-spec/blob/master/schema/manifest_backwards_compatibility_test.go#L134
seems enough to consumer, do you need more?

The ‘Accept: application/vnd.docker.distribution.manifest.v2+json’
you're updating came in with #152, but the payload is clearly an
application/vnd.docker.container.image.v1+json (and you're fixing a
lot of that in this PR). I expect the stuff you're fixing was
originally a copy/paste error when #152 was being written. To avoid
uncertainty going forward, I'd like to have someone actually run the
curl comment and check the returned payload against the test's config
entry. That way we know we're free of further typos.

Docker Hub enable the authorization, so the auth header of Json Web
Token(JWT) is necessary. JWT generation requires repository name,
action, account name,password, and so on.

And I'm not familiar enough with all of that to be able to run the
curl comment myself. Is someone who is more familiar able to confirm
your new curl comment works as advertised? Are there any configs in
the Docker registry which you can retrieve anonymously, or does the
registry not have a concept of public blobs?

[xiekeyang]

@wking
Docker hub skip checking Accept header by client HTTP. The line in spec:

-H "Accept: application/vnd.docker.container.image.v1+json"

That is ineffective. But I think it is OK to keep this line.
If you want to test and observe the response, please use the commend as the below:

curl -H "Authorization: Bearer $(curl https://auth.docker.io/token\?service\=registry.docker.io\&scope\=repository:library/busybox:pull | python -m json.tool |grep token |awk '{print $2}' |awk -F \" '{print $2}')" https://index.docker.io/v2/library/busybox/manifests/latest

It fetch Bearer Token, and get config content.

[wking]

On Tue, Oct 11, 2016 at 01:41:47AM -0700, xiekeyang wrote:

Docker hub skip checking Accept header by client HTTP.

Then we should probably skip that Accept header in the comment and
just show the media type of the response. But the docs claim that the
Accept header is important for picking which manifest type you get
1.

curl -H "Authorization: Bearer $(curl
https://auth.docker.io/token\?service\=registry.docker.io\&scope\=repository:library/busybox:pull
| python -m json.tool |grep token |awk '{print $2}' |awk -F "
'{print $2}')"
https://index.docker.io/v2/library/busybox/manifests/latest

Ah, thanks :). Shuffling that around a bit and applying it to the
config case, I think we want to put something like this in the
comment:

$ TOKEN=$(curl 'https://auth.docker.io/token?service=registry.docker.io&scope=repository:library/docker:pull' | jq -r .token)
$ CONFIG=$(curl -H "Authorization: Bearer ${TOKEN}" -H 'Accept: application/vnd.docker.distribution.manifest.v2+json' https://index.docker.io/v2/library/docker/manifests/1.12.1 | jq -r .config.digest)
$ echo "${CONFIG}"
sha256:a059ea7356d5b5a9e0f6352bfa463e7bd4721c2ade3ef168603826e0de6fe54b
$ curl -LH "Authorization: Bearer ${TOKEN}" "https://index.docker.io/v2/library/docker/blobs/${CONFIG}"

And that we want to use the raw
sha256:a059ea7356d5b5a9e0f6352bfa463e7bd4721c2ade3ef168603826e0de6fe54b
content in the first entry here. I'm not sure where the current
sha256:5359… content came from, maybe a different version than
library/docker v1.12.1?

[xiekeyang]

@wking If we really need to put the detail HTTP requests here? It seems to be giving introduction to consumers, that is unnecessary( in my personal opinion).

[wking]

On Wed, Oct 12, 2016 at 05:04:31AM -0700, xiekeyang wrote:

If we really need to put the detail HTTP requests here? It seems to
be giving introduction to consumers, that is unnecessary( in my
personal opinion).

I'm fine dropping the curl comments, but if we do that I'd like to
have the curl sequence I outlined above 1 in the commit message (for
commits that remove curl comments or commits that add new entries).
For content that is not being written by the commit author, it should
be obvious from the commit (message or landed comment) how to
regenerate that content. What I'd like to avoid is comments that give
you some information about reproducing the content but without enough
detail to actually work.

[xiekeyang]

@wking Updated it. PTAL. The fetching commends looks almost as same as you mentioned. I tried them on my local side, and it works correctly.

[wking]

I was using a leading $ to represent a shell prompt. If you drop the space, you are dereferencing the left-hand side of the assignment, which is not what you want:

$ $TOKEN=a
bash: =a: command not found

I also prefer wrapping quotes to backslash escapes in the URLs, but that's not a big deal either way.

[wking]

This is the config digest, not the manifest. If you want to split the manifest out as a separate step, you could use:

$ MANIFEST="$(curl -H "Authorization: Bearer ${TOKEN}" -H 'Accept: application/vnd.docker.distribution.manifest.v2+json' https://index.docker.io/v2/library/docker/manifests/1.12.1)"
$ CONFIG_DIGEST=$(echo "${MANIFEST}" | jq -r .config.digest)

[xiekeyang]

@wking Updated, PTAL, thanks.

[wking]

On Thu, Oct 13, 2016 at 08:01:49PM -0700, xiekeyang wrote:

@wking Updated, PTAL, thanks.

If you keep the ‘$’ prompts, you'll want to use ‘$ ’ (with a trailing
space) 1.

[xiekeyang]

@wking I've dropped $. Now PR looks clearer. PTAL, thanks.

[wking]

When you dropped the prompt $, you went back to MANIFEST for the variable name. I still think that this variable should be CONFIG or CONFIG_DIGEST.

[xiekeyang]

@wking All right, fixed it.

[wking]

On Mon, Oct 17, 2016 at 10:35:01PM -0700, xiekeyang wrote:

@wking All right, fixed it.

Not quite ;). Here's a history of your tip:

1. 0d477e4: Just Changed manifest → config, which is fine, but I was
   concerned about the comment I couldn't test 1.
2. e3d5585: Updated the comment, but used both ‘$TOKEN=…’ 2 and a
   variable named MANIFEST for holding the config digest 3.
3. c89918c: Changed the config-digest variable from MANIFEST to
   CONFIG_DIGEST, but still had ‘$TOKEN=…’ 4.
4. ad8c8e4: Removed the prompts, but changed CONFIG_DIGEST back to
   MANIFEST 5.
5. 0a16aee: Restored the prompts (now with a space, so ‘$ TOKEN=…’,
   which is fine) but kept MANIFEST for the config digest.

What I'd like to see for the troublesome comment is either:

// config pulled from docker hub blob store
//
// $ TOKEN=$(curl https://auth.docker.io/token\?service\=registry.docker.io\&scope\=repository:library/docker:pull | jq -r .token)
// $ CONFIG_DIGEST=$(curl -H "Authorization: Bearer ${TOKEN}" -H 'Accept: application/vnd.docker.distribution.manifest.v2+json' https://index.docker.io/v2/library/docker/manifests/1.12.1 | jq -r .config.digest)
// $ curl -LH "Authorization: Bearer ${TOKEN}" "https://index.docker.io/v2/library/docker/blobs/${CONFIG_DIGEST}"

or without prompts:

// config pulled from docker hub blob store
//
// TOKEN=$(curl https://auth.docker.io/token\?service\=registry.docker.io\&scope\=repository:library/docker:pull | jq -r .token)
// CONFIG_DIGEST=$(curl -H "Authorization: Bearer ${TOKEN}" -H 'Accept: application/vnd.docker.distribution.manifest.v2+json' https://index.docker.io/v2/library/docker/manifests/1.12.1 | jq -r .config.digest)
// curl -LH "Authorization: Bearer ${TOKEN}" "https://index.docker.io/v2/library/docker/blobs/${CONFIG_DIGEST}"

[xiekeyang]

@wking Sorry for my fault. That should be corrected.

[wking]

a39089c looks good to me :).

[xiekeyang]

@philips PTAL.

[vbatts]

LGTM

[vbatts]

@opencontainers/image-spec-maintainers PTAL

[jonboulle]

lgtm