We also need an additional check to avoid setting both the error and
response which can create a race where they can arrive in the receiving
thread in either order.
If we hit an error, we don't need to send the response.
> There is a condition where the registry (unexpectedly, not to spec)
> returns 201 or 204 on the put before the body is fully written. I would
> expect that the http library would issue close and could fall into a
> deadlock here. We could just read respC and call setResponse. In that
> case ErrClosedPipe would get returned and Commit shouldn't be called
> anyway.
Signed-off-by: Justin Chadwell <me@jedevc.com>
If sending two messages from goroutine X:
a <- 1
b <- 2
And receiving them in goroutine Y:
select {
case <- a:
case <- b:
}
Either branch of the select can trigger first - so when we call
.setError and .Close next to each other, we don't know whether the done
channel will close first or the error channel will receive first - so
sometimes, we get an incorrect error message.
We resolve this by not sending both signals - instead, we can have
.setError *imply* .Close, by having the pushWriter call .Close on
itself, after receiving an error.
Signed-off-by: Justin Chadwell <me@jedevc.com>
If we get io.ErrClosedPipe in pushWriter.Write, there are three possible
scenarios:
- The request has failed, we need to attempt a reset, so we can expect a
new pipe incoming on pipeC.
- The request has failed, we don't need to attempt a reset, so we can
expect an incoming error on errC.
- Something else externally has called Close, so we can expect the done
channel to be closed.
This patch ensures that we block for as long as possible (while still
handling each of the above cases, so we avoid hanging), to make sure
that we properly return an appropriate error message each time.
Signed-off-by: Justin Chadwell <me@jedevc.com>
If Close is called externally before a request is attempted, then we
will accidentally attempt to send to a closed channel, causing a panic.
To avoid this, we can check to see if Close has been called, using a
done channel. If this channel is ever done, we drop any incoming errors,
requests or pipes - we don't need them, since we're done.
Signed-off-by: Justin Chadwell <me@jedevc.com>
io.Pipe produces a PipeReader and a PipeWriter - a close on the write
side, causes an error on both the read and write sides, while a close on
the read side causes an error on only the read side. Previously, we
explicitly prohibited closing from the read side.
However, http.Request.Body requires that "calling Close should unblock a
Read waiting for input". Our reader will not do this - calling close
becomes a no-op. This can cause a deadlock because client.Do may never
terminate in some circumstances.
We need the Reader side to close its side of the pipe as well, which it
already does using the go standard library - otherwise, we can hang
forever, writing to a pipe that will never be closed.
Allowing the requester to close the body should be safe - we never reuse
the same reader between requests, as the result of body() will never be
reused by the guarantees of the standard library.
Signed-off-by: Justin Chadwell <me@jedevc.com>
All components need to use a consistent `semconv` version or OTel
will emit errors about "cannot merge resource due to conflicting Schema URL".
Switch to the appropriate semconv version, which requires dropping
usage of `httpconv`. Instead, the upstream HTTP client hooks are
used directly. (The lower-level functions are no longer exported by
OTel.)
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
(cherry picked from commit ad3b806a4f)
Signed-off-by: Derek McGowan <derek@mcg.dev>
Close connection if no more data. It's to fix false alert filed by image
pull progress.
```
dst = OpenWriter (--> Content Store)
src = Fetch
Open (--> Registry)
Mark it as active request
Copy(dst, src) (--> Keep updating total received bytes)
^
| (Active Request > 0, but total received bytes won't be updated)
v
defer src.Close()
content.Commit(dst)
```
Before migrating to transfer service, CRI plugin doesn't limit global
concurrent downloads for ImagePulls. Each ImagePull requests have 3 concurrent
goroutines to download blob and 1 goroutine to unpack blob. Like ext4
filesystem [1][1], the fsync from content.Commit may sync unrelated dirty pages
into disk. The host is running under IO pressure, and then the content.Commit
will take long time and block other goroutines. If httpreadseeker
doesn't close the connection after io.EOF, this connection will be
considered as active. The pull progress reporter reports there is no
bytes transfered and cancels the ImagePull.
The original 1-minute timeout[2][2] is from kubelet settting. Since CRI-plugin
can't limit the total concurrent downloads, this patch is to update 1-minute
to 5-minutes to prevent from unexpected cancel.
[1]: https://lwn.net/Articles/842385/
[2]: https://github.com/kubernetes/kubernetes/blob/release-1.23/pkg/kubelet/config/flags.go#L45-L48
Signed-off-by: Wei Fu <fuweid89@gmail.com>
When the HTTP fallback is used, the scheme changes from HTTPS to HTTP
which can cause a mismatch on redirect, causing the authorizer to get
stripped out. Since the redirect host must match the redirect host in
this case, credentials are only sent to the same origin host that
returned the redirect.
This fixes an issue for a push getting a 401 unauthorized on the PUT
request even though credentials are available.
Signed-off-by: Derek McGowan <derek@mcg.dev>
(cherry picked from commit 466ee870d5)
Signed-off-by: Derek McGowan <derek@mcg.dev>
The TLS fallback should only be used when the protocol is ambiguous due
to provided TLS configurations and defaulting to http. Do not add TLS
configurations when defaulting to http. When the port is 80 or will be
defaulted to 80, there is no protocol ambiguity and TLS fallback should
not be used.
Signed-off-by: Derek McGowan <derek@mcg.dev>
(cherry picked from commit d48ceb6065)
Signed-off-by: Derek McGowan <derek@mcg.dev>
When a credential handler is provided but no basic auth credentials
are provided, handle the error specifically rather than treating
the credentials as not implemented. This allows a clearer error to
be provided to users rather than a confusing not implemented error
or generic unauthorized error.
Add unit tests for the basic auth case.
Signed-off-by: Derek McGowan <derek@mcg.dev>
(cherry picked from commit 51c9ffe468)
Signed-off-by: Derek McGowan <derek@mcg.dev>
`MountedFrom` was prefixed with the whole target repository instead of
just the registry hostname.
Also adjust the test cases to use the registry hostname.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
(cherry picked from commit 859c2651e5)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
When a endpoint is configured for http and has a tls configuration,
always try to the tls connection and fallback to http when the tls
connections fails from receiving an http response. This fixes an issue
with default localhost endpoints which get defaulted to http with
insecure tls also configured but are using tls.
Signed-off-by: Derek McGowan <derek@mcg.dev>
(cherry picked from commit 79772a0dd4)
Signed-off-by: Derek McGowan <derek@mcg.dev>
This makes it possible to check whether content didn't actually need to
be pushed to the remote registry and was cross-repo mounted or already
existed.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
(cherry picked from commit dfc7590d5a)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Go deprecation comments must be formatted to have an empty comment line before
them. Fix the formatting to make sure linters and editors detect that these
are deprecated.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 52997ea983)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Add a public const for "containerd.io/distribution.source" in `labels`
package and replace hardcoded usages.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
In the refactor from 926b9c72f6, the error
handling was substantially reworked, and changed the types of errors
returned.
Notably, in the case of a network error, instead of propogating the
error through to return from pushWriter.Write (as previously), it would
be propagated through to pushWriter.Commit - however, this is too late,
since we've already closed the io.Pipe by the time we would have reached
this function. Therefore, we get the generic error message "io:
read/write on closed pipe" for *every network error*.
This patch corrects this behavior to ensure that the correct error
object is always returned as early as possible, by checking the error
result after writing and detecting a closed pipe.
Additionally, we do some additional hardening - specifically we prevent
falling through when resetting the content or detecting errors, and
update the tests to explicitly check for the ErrReset message.
Signed-off-by: Justin Chadwell <me@jedevc.com>
Fetching blobs without foreknown descriptors is useful for using a registry as a general-purpose CAS.
Related: `oras blob fetch` (ORAS v0.15.0)
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Signed-off-by: Swagat Bora <sbora@amazon.com>
Add spans around image unpack operations
Use image.ref to denote image name and image.id for the image config digest
Add top-level spand and record errors in the CRI instrumentation service
when a put request is retried due to the response from registry,
the body of the request should be seekable. A dynamic pipe is added
to the body so that the content of the body can be read again.
Currently a maximum of 5 resets are allowed, above which will fail the
request. A new error ErrReset is introduced which informs that a
reset has occured and request needs to be retried.
also added tests for Copy() and push() to test the new functionality
Signed-off-by: Akhil Mohan <makhil@vmware.com>
沈陵