Update ggcr, use remote.Head

[mattmoor]

Some additional context: https://knative.slack.com/archives/CDJ8M6R34/p1600293665001500

This is to pull in google/go-containerregistry#770 and enable the use of remote.Head, which bypasses the rate limiting that DockerHub is instituting.

Uses HEAD rather than GET for resolving image digests, which avoids counting image digest resolution against dockerhub pull quotas.

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/revision/resolve.go	95.5%	90.9%	-4.5

[jonjohnsonjr]

/lgtm
/approve

[mattmoor]

I had to update the tests 😇

[jonjohnsonjr]

/lgtm
/approve

[vagababov]

/lgtm
/hold
if you wanna fix the nits

[mattmoor]

RFAL

[mattmoor]

/unhold

[vagababov]

/lgtm

[mattmoor]

/retest

[mattmoor]

This is a legit failure...

errorcondition_test.go:86: Reason: RevisionFailed; Message: Revision "container-error-msg-iggovilu-kpd7r" failed with message: Unable to fetch image "gcr.io/knative-boskos-84/serving-e2e-img/997/invalidhelloworld:latest": failed to resolve image to digest: HEAD https://gcr.io/v2/knative-boskos-84/serving-e2e-img/997/invalidhelloworld/manifests/latest: unsupported status code 404.; Status False

[mattmoor]

/hold

So this is definitely breaking, it breaks with GCR. 🙃

@jonjohnsonjr ping me once a fix has been rolled out for the couple issues I sent you and we can /retest this, but this shouldn't land until early next release.

[knative-test-reporter-robot]

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	2020-09-17 02:15:04.923 +0000 UTC 2020-09-17 03:03:05.029 +0000 UTC 2020-09-17 03:52:05.006 +0000 UTC 2020-09-17 04:41:05.43 +0000 UTC	3/3

Job pull-knative-serving-integration-tests expended all 3 retries without success.

[julz]

Should we add something to the release note in the description for this? Seems worth making sure we remember to mention

[jonjohnsonjr]

I'd argue for just changing the test, or checking the status code in the resolver and setting the error to something stable based on that.

Per https://tools.ietf.org/html/rfc7231#section-4.3.2

The HEAD method is identical to GET except that the server MUST NOT
send a message body in the response (i.e., the response terminates at
the end of the header section). The server SHOULD send the same
header fields in response to a HEAD request as it would have sent if
the request had been a GET, except that the payload header fields
(Section 3.3) MAY be omitted. This method can be used for obtaining
metadata about the selected representation without transferring the
representation data and is often used for testing hypertext links for
validity, accessibility, and recent modification.

As far as I can tell from testing, both GCR and Docker Hub return an empty response body for HEAD requests.

I think checking the specific error message you get back from ggcr is going to be fragile, but you could consider looking for "404" too?

[jonjohnsonjr]

Should we add something to the release note in the description for this? Seems worth making sure we remember to mention

+1

1. If anyone is concerned about the rate limiting, seeing this is helpful (at least we don't make the situation worse).
2. If this breaks against certain registries, having release notes can help identify the issue.

[mattmoor]

As far as I can tell from testing, both GCR and Docker Hub return an empty response body for HEAD requests.

Actually one of the bugs I found last night is that GCR is returning a body with HEAD. Ack that the semantics of HEAD should be no body, but I guess that dogma on 4xx and 5xx status codes surprises me a bit.

[mattmoor]

I added your release notes @julz (thanks!) 😉

[mattmoor]

Changed it to look for a 404 instead, which is about the only useful information we get with this request type 🤷

[jonjohnsonjr]

Actually one of the bugs I found last night is that GCR is returning a body with HEAD. Ack that the semantics of HEAD should be no body, but I guess that dogma on 4xx and 5xx status codes surprises me a bit.

I couldn't repro this when I tried -- I can get the same curl error as you but I'm not sure that means there was a body?

$ curl -X HEAD https://gcr.io/v2/distroless/static/manifests/latest
Warning: Setting custom HTTP method to HEAD with -X/--request may not work the 
Warning: way you want. Consider using -I/--head instead.
curl: (18) transfer closed with 526 bytes remaining to read

Presumably, these warnings are exactly about this scenario. I would guess --head causes curl to interpret the content-length headers differently than just doing -X HEAD:

$ curl --head https://gcr.io/v2/distroless/static/manifests/latest
HTTP/2 200 
docker-distribution-api-version: registry/2.0
content-type: application/vnd.docker.distribution.manifest.v2+json
content-length: 526
docker-content-digest: sha256:08cce4d65b19780e10b555578a0f9c9af51310308ec31e648577122d40e6922a
date: Thu, 17 Sep 2020 18:33:36 GMT
server: Docker Registry
...

[jonjohnsonjr]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonjohnsonjr, mattmoor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mattmoor]

/unhold