Define a new `remote.DefaultTransport`. by mattmoor · Pull Request #1165 · google/go-containerregistry

mattmoor · 2021-11-04T23:27:11Z

The first commit here reverts #1163 which went in before I could change it to the transport-based option.

Previously we used http.DefaultTransport (on which this is based),
but this uses a default dial timeout of 30s, which when we (by default)
wrap things in 5x retries can lead to ~150s delays simply pinging
an http-based registry.

If folks encounter issues with this via the library they can restore
the current behavior with:

remote.WithTransport(http.DefaultTransport)

If folks encounter issues with this via crane they can restore the
current behavior with:

--dial-timeout 30s

google-cla · 2021-11-04T23:27:17Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mattmoor · 2021-11-04T23:36:28Z

@googlebot I fixed it.

google-cla · 2021-11-04T23:36:50Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mattmoor · 2021-11-04T23:51:01Z

@googlebot I fixed it.

google-cla · 2021-11-04T23:51:09Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mattmoor · 2021-11-04T23:52:00Z

@googlebot I fixed it.

google-cla · 2021-11-04T23:52:08Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mattmoor · 2021-11-04T23:53:28Z

omg I hate you CLA bot

mattmoor · 2021-11-04T23:57:20Z

Switched to the old email 🙃

mattmoor · 2021-11-05T00:05:11Z

Fixed the gcrane test that assumed http.DefaultTransport was the default transport to use WithTransport.

codecov-commenter · 2021-11-05T00:06:07Z

Codecov Report

Merging #1165 (3997975) into main (6cb23fb) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1165      +/-   ##
==========================================
- Coverage   75.24%   75.23%   -0.01%     
==========================================
  Files         108      108              
  Lines        7857     7855       -2     
==========================================
- Hits         5912     5910       -2     
  Misses       1379     1379              
  Partials      566      566

Impacted Files	Coverage Δ
pkg/v1/remote/options.go	`69.23% <100.00%> (ø)`
pkg/v1/remote/transport/ping.go	`91.46% <100.00%> (-0.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cb23fb...3997975. Read the comment docs.

imjasonh

This looks okay to me. I'm not super in love with making crane users think about the dial timeout flag, but I guess it's okay to let them get back to previous behavior if they want it.

imjasonh · 2021-11-10T21:17:57Z

Friendly ping. If we do a release soon do we feel like we need to get this included in it?

This reverts commit 080751a.

mattmoor · 2021-11-10T22:43:30Z

Rebasing now, then I'll add a commit for the xcr.io thing.

Previously we used `http.DefaultTransport` (on which this is based), but this uses a default dial timeout of 30s, which when we (by default) wrap things in 5x retries can lead to ~150s delays simply pinging an http-based registry. If folks encounter issues with this via the library they can restore the current behavior with: ```go remote.WithTransport(http.DefaultTransport) ``` If folks encounter issues with this via `crane` they can restore the current behavior with: ``` --dial-timeout 30s ```

This implements a "happy eyeballs" (RFC 6555) style of race in order to speed up http fallback during registry pings. This heavily borrows code from net/dial.go which implements this same kind of race for dual stack DNS. Rather than waiting for the initial "https" ping to time out completely before attempting to use "http", we will instead start a second fallback ping after 300ms and return whichever response comes back first. This partially reverts the portion of google#1165 that reduced the dial timeout to workaround this issue.

This implements a "happy eyeballs" (RFC 6555) style of race in order to speed up http fallback during registry pings. This heavily borrows code from net/dial.go which implements this same kind of race for dual stack DNS. Rather than waiting for the initial "https" ping to time out completely before attempting to use "http", we will instead start a second fallback ping after 300ms and return whichever response comes back first. This partially reverts the portion of google#1165 that reduced the dial timeout to workaround this issue. Also, drop unused parseChallenge to appease the linter.

This implements a "happy eyeballs" (RFC 6555) style of race in order to speed up http fallback during registry pings. This heavily borrows code from net/dial.go which implements this same kind of race for dual stack DNS. Rather than waiting for the initial "https" ping to time out completely before attempting to use "http", we will instead start a second fallback ping after 300ms and return whichever response comes back first. This partially reverts the portion of #1165 that reduced the dial timeout to workaround this issue. Also, drop unused parseChallenge to appease the linter.

mattmoor requested review from imjasonh and jonjohnsonjr November 4, 2021 23:27

mattmoor force-pushed the switch-to-custom-transport branch from 31bbe05 to 4c81250 Compare November 4, 2021 23:56

mattmoor force-pushed the switch-to-custom-transport branch from 4c81250 to 13cab84 Compare November 5, 2021 00:04

imjasonh reviewed Nov 6, 2021

View reviewed changes

Comment thread pkg/gcrane/copy_test.go

Revert "Give the ping context a timeout. (google#1163)"

396766a

This reverts commit 080751a.

mattmoor force-pushed the switch-to-custom-transport branch from 13cab84 to 7df8286 Compare November 10, 2021 22:44

Switch to an impossible domain name

32a378d

imjasonh reviewed Nov 11, 2021

View reviewed changes

Comment thread cmd/crane/cmd/root.go Outdated

Back out the crane flag

3997975

jonjohnsonjr approved these changes Nov 11, 2021

View reviewed changes

imjasonh approved these changes Nov 11, 2021

View reviewed changes

imjasonh merged commit 7a6ee45 into google:main Nov 11, 2021

hasheddan mentioned this pull request Jan 5, 2022

Update go-containerregistry to v0.7.0 crossplane/crossplane#2814

Merged

3 tasks

jonjohnsonjr mentioned this pull request Feb 7, 2022

Local registry times out unless --allow_insecure_registry is used sigstore/cosign#1405

Closed

This was referenced Dec 27, 2022

Race http fallback ping #1521

Merged

Fix DNS lookup failure caused by too-short dial timeout #1517

Closed

Conversation

mattmoor commented Nov 4, 2021

Uh oh!

google-cla Bot commented Nov 4, 2021

Uh oh!

mattmoor commented Nov 4, 2021

Uh oh!

google-cla Bot commented Nov 4, 2021

Uh oh!

mattmoor commented Nov 4, 2021

Uh oh!

google-cla Bot commented Nov 4, 2021

Uh oh!

mattmoor commented Nov 4, 2021

Uh oh!

google-cla Bot commented Nov 4, 2021

Uh oh!

mattmoor commented Nov 4, 2021

Uh oh!

mattmoor commented Nov 4, 2021

Uh oh!

mattmoor commented Nov 5, 2021

Uh oh!

codecov-commenter commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

imjasonh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imjasonh commented Nov 10, 2021

Uh oh!

mattmoor commented Nov 10, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Nov 5, 2021 •

edited

Loading