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

Check Docker-Distribution-API-Version header in v2 check #5676

Merged
merged 1 commit into from Nov 4, 2015

Conversation

smarterclayton
Copy link
Contributor

Guard against servers that return non-json for the /v2/ check like
quay.io started doing today. Reverts skipping quay.io

@smarterclayton smarterclayton added approved Indicates a PR has been approved by an approver from all required OWNERS files. component/imageregistry labels Nov 4, 2015
@smarterclayton
Copy link
Contributor Author

[test]

@@ -311,6 +318,11 @@ func (c *connection) checkV2() (bool, error) {
// handle auth challenges on individual repositories
case code >= 300 || resp.StatusCode < 200:
return false, nil
default:
if mediatype, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type")); err == nil && mediatype != "application/json" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're sure all registries return content type correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All v2 registries that I can find (all 2 of them) do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec doesn't say that there is any content. Unfortunately, quay.io violates the spec.

Actually, going to make this docker distribution header. One sec.

if err != nil {
return spec
}
if ref.Registry != "v1.docker.io" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flip to show special handling of v1.docker.io?:

if err == nil && ref.Registry == "v1.docker.io" {
  ref.Registry = ""
  return ref.Exact()
}
return spec

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flipped. Make a call now.

@liggitt
Copy link
Contributor

liggitt commented Nov 4, 2015

"Docker-Distribution-API-Version" check is fine, I want more time to think about the v1.docker.io change

like quay.io started doing today.  Reverts skipping quay.io
@liggitt liggitt changed the title Allow 'v1.docker.io' to force v1 against docker Check Docker-Distribution-API-Version header in v2 check Nov 4, 2015
@liggitt
Copy link
Contributor

liggitt commented Nov 4, 2015

LGTM, clean unit and integration test locally before merging

@liggitt liggitt self-assigned this Nov 4, 2015
@liggitt liggitt added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2015
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 2de8fcf

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6899/)

@smarterclayton
Copy link
Contributor Author

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6899/) (Image: devenv-rhel7_2638)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 2de8fcf

openshift-bot pushed a commit that referenced this pull request Nov 4, 2015
@openshift-bot openshift-bot merged commit d512cd0 into openshift:master Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/imageregistry lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants