Conversation
This helps clarify the difference cases for parsing ctxName, and for getting an image. Signed-off-by: Daniel Nephin <dnephin@docker.com>
Rename the methods to match my understanding of the behaviour. Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
dnephin
left a comment
There was a problem hiding this comment.
If you are ok with these changes I can squash my commits.
Unit tests pass, I haven't tried the integration test yet. I will try it later today
| } | ||
|
|
||
| // ImageMounter is able to mount images by id | ||
| type ImageMounter interface { |
There was a problem hiding this comment.
I split this out so that imageMounts can depend on a much smaller interface
| b.cancel() | ||
| } | ||
|
|
||
| func (b *Builder) pullOrGetImage(name string) (builder.Image, error) { |
There was a problem hiding this comment.
I moved this to Builder because it's tightly coupled, It depends on 4 fields on the Builder. It makes it easier to expose this function from imageContexts (as an interface with a single method)
| return im, nil | ||
| } | ||
| // TODO: what is this case? | ||
| return nil, nil |
There was a problem hiding this comment.
This exists in your original branch as well. I don't think I understand this case. When would the imageContexts have a named context without an image ID?
There was a problem hiding this comment.
That happens in the case of FROM scratch.
| return im, nil | ||
| } | ||
|
|
||
| func pullOrGetImage(b *Builder, name string) (builder.Image, error) { |
| } | ||
|
|
||
| // mountByRef creates an imageMount from a reference. pulling the image if needed. | ||
| func mountByRef(b *Builder, name string) (*imageMount, error) { |
| cache *pathCache | ||
| } | ||
|
|
||
| func (ic *imageContexts) new(name string, increment bool) (*imageMount, error) { |
There was a problem hiding this comment.
The call that was using increment was replaced with a call to newImageMountWithID()
| return nil, errors.Errorf("could not copy from empty context") | ||
| } | ||
| p, release, err := im.ic.b.docker.MountImage(im.id) | ||
| p, release, err := im.mounter.MountImage(im.id) |
There was a problem hiding this comment.
Some of this refactor was to enable this change. So that this function doesn't have to reach two levels into a dependency (ref: https://en.wikipedia.org/wiki/Law_of_Demeter)
|
The main problem here seems to be that Moving things to their own function(parseContextName, getFromImage) while that function is only called once seem like optional to me and could be a refactoring later. While I'm not particularly proud of the original code I don't think this improves the situation much. Plus it is longer. I think separating. |
|
Cool, I'm going to LGTM your PR, and I can submit this as PR to docker/docker after it merges |
|
Thanks! |
This subtle bug keeps lurking in because error checking for `Mkdir()`
and `MkdirAll()` is slightly different wrt to `EEXIST`/`IsExist`:
- for `Mkdir()`, `IsExist` error should (usually) be ignored
(unless you want to make sure directory was not there before)
as it means "the destination directory was already there"
- for `MkdirAll()`, `IsExist` error should NEVER be ignored.
Mostly, this commit just removes ignoring the IsExist error, as it
should not be ignored.
Also, there are a couple of cases then IsExist is handled as
"directory already exist" which is wrong. As a result, some code
that never worked as intended is now removed.
NOTE that `idtools.MkdirAndChown()` behaves like `os.MkdirAll()`
rather than `os.Mkdir()` -- so its description is amended accordingly,
and its usage is handled as such (i.e. IsExist error is not ignored).
For more details, a quote from my runc commit 6f82d4b (July 2015):
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.
Quoting MkdirAll documentation:
> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.
This means two things:
1. If a directory to be created already exists, no error is
returned.
2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.
The above is a theory, based on quoted documentation and my UNIX
knowledge.
3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in #2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.
Because of #1, IsExist check after MkdirAll is not needed.
Because of #2 and #3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.
Note this error is all over the tree, I guess due to copy-paste,
or trying to follow the same usage pattern as for Mkdir(),
or some not quite correct examples on the Internet.
[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In particular, these two: > daemon/daemon_unix.go:1129: Wrapf format %v reads arg #1, but call has 0 args > daemon/kill.go:111: Warn call has possible formatting directive %s and a few more. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Fix the following go-1.11beta1 build error: > daemon/graphdriver/aufs/aufs.go:376: Wrapf format %s reads arg #1, but call has 0 args While at it, change '%s' to %q. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I had a CI run fail to "Upload reports":
Exponential backoff for retry #1. Waiting for 4565 milliseconds before continuing the upload at offset 0
Finished backoff for retry #1, continuing with upload
Total file count: 211 ---- Processed file moby#160 (75.8%)
...
Total file count: 211 ---- Processed file moby#164 (77.7%)
Total file count: 211 ---- Processed file moby#164 (77.7%)
Total file count: 211 ---- Processed file moby#164 (77.7%)
A 503 status code has been received, will attempt to retry the upload
##### Begin Diagnostic HTTP information #####
Status Code: 503
Status Message: Service Unavailable
Header Information: {
"content-length": "592",
"content-type": "application/json; charset=utf-8",
"date": "Mon, 21 Aug 2023 14:08:10 GMT",
"server": "Kestrel",
"cache-control": "no-store,no-cache",
"pragma": "no-cache",
"strict-transport-security": "max-age=2592000",
"x-tfs-processid": "b2fc902c-011a-48be-858d-c62e9c397cb6",
"activityid": "49a48b53-0411-4ff3-86a7-4528e3f71ba2",
"x-tfs-session": "49a48b53-0411-4ff3-86a7-4528e3f71ba2",
"x-vss-e2eid": "49a48b53-0411-4ff3-86a7-4528e3f71ba2",
"x-vss-senderdeploymentid": "63be6134-28d1-8c82-e969-91f4e88fcdec",
"x-frame-options": "SAMEORIGIN"
}
###### End Diagnostic HTTP information ######
Retry limit has been reached for chunk at offset 0 to https://pipelinesghubeus5.actions.githubusercontent.com/Y2huPMnV2RyiTvKoReSyXTCrcRyxUdSDRZYoZr0ONBvpl5e9Nu/_apis/resources/Containers/8331549?itemPath=integration-reports%2Fubuntu-22.04-systemd%2Fbundles%2Ftest-integration%2FTestInfoRegistryMirrors%2Fd20ac12e48cea%2Fdocker.log
Warning: Aborting upload for /tmp/reports/ubuntu-22.04-systemd/bundles/test-integration/TestInfoRegistryMirrors/d20ac12e48cea/docker.log due to failure
Error: aborting artifact upload
Total file count: 211 ---- Processed file moby#165 (78.1%)
A 503 status code has been received, will attempt to retry the upload
Exponential backoff for retry #1. Waiting for 5799 milliseconds before continuing the upload at offset 0
As a result, the "Download reports" continued retrying:
...
Total file count: 1004 ---- Processed file moby#436 (43.4%)
Total file count: 1004 ---- Processed file moby#436 (43.4%)
Total file count: 1004 ---- Processed file moby#436 (43.4%)
An error occurred while attempting to download a file
Error: Request timeout: /Y2huPMnV2RyiTvKoReSyXTCrcRyxUdSDRZYoZr0ONBvpl5e9Nu/_apis/resources/Containers/8331549?itemPath=integration-reports%2Fubuntu-20.04%2Fbundles%2Ftest-integration%2FTestCreateWithDuplicateNetworkNames%2Fd47798cc212d1%2Fdocker.log
at ClientRequest.<anonymous> (/home/runner/work/_actions/actions/download-artifact/v3/dist/index.js:3681:26)
at Object.onceWrapper (node:events:627:28)
at ClientRequest.emit (node:events:513:28)
at TLSSocket.emitRequestTimeout (node:_http_client:839:9)
at Object.onceWrapper (node:events:627:28)
at TLSSocket.emit (node:events:525:35)
at TLSSocket.Socket._onTimeout (node:net:550:8)
at listOnTimeout (node:internal/timers:559:17)
at processTimers (node:internal/timers:502:7)
Exponential backoff for retry #1. Waiting for 5305 milliseconds before continuing the download
Total file count: 1004 ---- Processed file moby#436 (43.4%)
And, it looks like GitHub doesn't allow cancelling the job, possibly
because it is defined with `if: always()`?
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…f v1.5.4 full diffs: - protocolbuffers/protobuf-go@v1.31.0...v1.33.0 - golang/protobuf@v1.5.3...v1.5.4 From the Go security announcement list; > Version v1.33.0 of the google.golang.org/protobuf module fixes a bug in > the google.golang.org/protobuf/encoding/protojson package which could cause > the Unmarshal function to enter an infinite loop when handling some invalid > inputs. > > This condition could only occur when unmarshaling into a message which contains > a google.protobuf.Any value, or when the UnmarshalOptions.UnmarshalUnknown > option is set. Unmarshal now correctly returns an error when handling these > inputs. > > This is CVE-2024-24786. In a follow-up post; > A small correction: This vulnerability applies when the UnmarshalOptions.DiscardUnknown > option is set (as well as when unmarshaling into any message which contains a > google.protobuf.Any). There is no UnmarshalUnknown option. > > In addition, version 1.33.0 of google.golang.org/protobuf inadvertently > introduced an incompatibility with the older github.com/golang/protobuf > module. (golang/protobuf#1596) Users of the older > module should update to github.com/golang/protobuf@v1.5.4. govulncheck results in our code: govulncheck ./... Scanning your code and 1221 packages across 204 dependent modules for known vulnerabilities... === Symbol Results === Vulnerability #1: GO-2024-2611 Infinite loop in JSON unmarshaling in google.golang.org/protobuf More info: https://pkg.go.dev/vuln/GO-2024-2611 Module: google.golang.org/protobuf Found in: google.golang.org/protobuf@v1.31.0 Fixed in: google.golang.org/protobuf@v1.33.0 Example traces found: #1: daemon/logger/gcplogs/gcplogging.go:154:18: gcplogs.New calls logging.Client.Ping, which eventually calls json.Decoder.Peek #2: daemon/logger/gcplogs/gcplogging.go:154:18: gcplogs.New calls logging.Client.Ping, which eventually calls json.Decoder.Read #3: daemon/logger/gcplogs/gcplogging.go:154:18: gcplogs.New calls logging.Client.Ping, which eventually calls protojson.Unmarshal Your code is affected by 1 vulnerability from 1 module. This scan found no other vulnerabilities in packages you import or modules you require. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
api: Make EnableIPv6 optional (impl #1 - pointer-based)
contains a fix for CVE-2024-45338 / https://go.dev/issue/70906, but it doesn't affect our codebase: govulncheck -show=verbose ./... Scanning your code and 1260 packages across 211 dependent modules for known vulnerabilities... ... Vulnerability #1: GO-2024-3333 Non-linear parsing of case-insensitive content in golang.org/x/net/html More info: https://pkg.go.dev/vuln/GO-2024-3333 Module: golang.org/x/net Found in: golang.org/x/net@v0.32.0 Fixed in: golang.org/x/net@v0.33.0 Your code is affected by 0 vulnerabilities. This scan also found 0 vulnerabilities in packages you import and 1 vulnerability in modules you require, but your code doesn't appear to call these vulnerabilities. full diff: golang/net@v0.32.0...v0.33.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I was looking over the named-context PR, and instead of making a bunch of comments, I'm opening this PR to see if you agree with some of the refactoring.
I split it into a few commits. There is one open question that I'm not sure about, and I think the first commit makes the case more obvious.