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

Don't use http.DefaultTransport in the CLI #77

Closed
dprotaso opened this issue Apr 19, 2018 · 3 comments · Fixed by #471
Closed

Don't use http.DefaultTransport in the CLI #77

dprotaso opened this issue Apr 19, 2018 · 3 comments · Fixed by #471
Labels
good first issue Good for newcomers

Comments

@dprotaso
Copy link
Contributor

It has no timeouts.

See: golang/go#24138 and https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/

@dekkagaijin
Copy link
Contributor

I'm planning on adding several nesting transports/clients that we've added to the python client (for good reasons). I can add a default timeout at the same time as that refactor.

@mattmoor
Copy link
Collaborator

FWIW, http.DefaultTransport = foo is a thing :)

@jonjohnsonjr
Copy link
Collaborator

Given that the issue above doesn't get much attention

@adrcunha continuing here

I think that issue doesn't get much attention because for most people (at least for me), using ko is an interactive thing -- network flakes are easy to deal with by just re-running the command, so it's not really a big deal because at worst you've wasted a few seconds, and re-running benefits from the go build cache.

For the integration tests, a network flake means we have to teardown/recreate the entire universe and rebuild everything :(

If this really is an issue for y'all, we happily accept pull requests and bribes 😉

The would be to replace this with something simple that will retry low-level network errors once or twice:

transport: http.DefaultTransport,

jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 19, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Also, stop using the default transport. It doesn't set timeouts for
headers, so it can hang forever and cause us to get stuck.

Fixes google#77
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 19, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes google#77
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 20, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes google#77
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 20, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes google#77
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 20, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes google#77
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 20, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes google#77
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 20, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes google#77
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this issue Jun 21, 2019
We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes google#77
jonjohnsonjr added a commit that referenced this issue Jun 21, 2019
* Add retries for temporary network issues

We pretty commonly see this issue when communicating with a registry:
```
Get https://gcr.io/v2/: net/http: TLS handshake timeout
```

This causes a huge amount of pain if it's a failure within e.g. a large
CI job, since the entire thing would fail.

Fixes #77

* Rename options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants