Malicious or malformed LLB definitions could reference negative or
out-of-range op input indices, or omit required diff lower/upper
inputs, causing the daemon to panic on a slice index or nil pointer
dereference during load and cache-key computation.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
(cherry picked from commit 2177ce16335f943e82458958902f9f21f6993d8d)
(cherry picked from commit ed777881d1)
Keep exec network modes limited to sandbox, host, and none, and pass proxy
network configuration separately through solve and executor runtime state.
Proxy execs now use bridge-style egress by default, host egress only for host
network mode with entitlement, and no proxy for none mode. Add integration
coverage for bridge, host, and none proxy behavior across OCI and containerd
workers.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Record each proxied exec request and print a redacted method and URL list in
the exec progress logs after the process completes.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Record successful GET responses through the exec proxy as provenance
materials and report incomplete material coverage as a typed solve error.
Thread proxy policy and capture state through typed executor/network options.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
When an ExecOp exec fails, ExecOp.Exec commits each output mutable
snapshot to an immutable ref, appends it to the results returned to the
solver, AND stores it (via the error-decoration defer) into the
ExecError. The error path stored the SAME *workerRefResult into
ExecError.Mounts that was already handed to the solver, so a single
committed output ref was owned through two independent channels but
backed by only one counted cache ref.
When the error owner is released (e.g. gateway/solver error cleanup),
that single release drains the shared refcount to zero, cascading into
equalMutable.release and deleting the mutable record's lease. The active
snapshot is protected only by that lease, so a subsequent containerd
metadata GC collects it. The surviving solver-side owner is then used as
the upper parent of a Diff, whose Finalize -> Snapshotter.Commit runs on
the already-collected active snapshot, producing:
failed to finalize upper parent during diff: failed to commit <active>
to <final> during finalize: failed to stat active key during commit:
snapshot <active> does not exist: not found
The fix is twofold:
- exec.go: clone the result (res.Clone()) before embedding it in
ExecError.Mounts so the error owner has its own counted ref.
- worker/result.go: make workerRefResult.Clone produce an independent
*WorkerRef (and thus an independent cache ref) instead of sharing the
embedded one and orphaning the original.
Both are required: the call-site clone is what stops the over-release,
and the Clone-method fix is what makes that clone actually independent
(otherwise it aliases the shared *WorkerRef and leaks the original ref).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
The error defer's active-commit loop wrapped the commit error into the
outer err but skipped releasing active.Ref via `continue`. The mutable
was neither put into execMounts nor released, leaking on each failed
Commit.
Release before continue.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
ExecOp.Exec's post-exec commit loop returned (nil, err) when
mutable.Commit failed mid-loop, leaving the already-committed outputs
in the named `results` return clobbered to nil with no holder -- a
permanent leak.
Release them inline instead. An internal commit failure is not a
user-facing exec error, so they don't belong in ExecError.Mounts
either.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Update non-generated code for the newer lint recommendations by using typed
atomic values, strings.Cut, and slices.Backward where applicable.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Generalize session.Group to JobContext so it
can be updated with additional job properties in
addition to retrieving the current session.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
This allows opt-in to cache key debug database on
daemon startup.
If enabled, all cache keys generated by builds are
saved into this database together with the plaintexts
of the original data so a reverse lookup can be performed
later to compare two checksums and find out their original
difference. If checksum contains other checksums internally
then these are saved as well. For storage constraints, the
plaintext of file content is not saved but the metadata
portion can be still looked up.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
The PATH is very critical during container runs on
Windows. Windows stores the PATH details in its
registry hive, while in unix, this is often
stored in the image's config. See further details
at #5445
Setting a default path like we do on Linux
(which is mostly not used since the PATH is
already set in the configs), works against
the expected build experience, especially when
it comes to installers and commands like `setx`.
Therefore, we skip setting the default PATH on
Windows, and leave it for the OS to load it
from its registry hive.
This also further supports backward compatibilitiy
with the current experience with docker classic
builder.
Users wishing to explicitly store this in the
configs can opt-in by using the ENV PATH= ..
in the Dockerfile, etc.
See also the same practices on Docker Engine
and Containerd:
- da3b31fb2c/oci/defaults.go (L24-L33)
- 041743e8af/pkg/oci/spec_opts_windows.go (L66-L69)closes#5445
Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
The current logic was incorrect in some places so that if first
session randomly chosen by `Any()` returned NotFound then other
sessions were not attempted.
For the main use case of mounting secrets as files the logic
was correct, but it was incorrect for example for the case of
adding secrets as environment variables.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
I came across a number of calls to `platforms.DefaultSpec()`
that were not necessary. That prompted me to take a look
various other similar places.
At least on Windows, the call is a has some cost till
doing a sycall `RtlGetNtVersionNumbers()`.
Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
vtproto is an extra protobuf compiler that generates special methods
suffixed with `VT` that create typed and unrolled marshal and unmarshal
functions similar to gogo that can be used for performance sensitive
code. These extensions are optional for code to use but buildkit uses
them.
A codec is also included to utilize vtproto for grpc code. If the
package `github.com/moby/buildkit/util/grpcutil/encoding/proto` is
imported then vtproto will be used if it exists and otherwise it will
use the standard marshaling and unmarshaling methods.
This codec has an important difference from the default codec. The
default codec will always reset messages before unmarshaling. In most
cases, this is unnecessary and is only relevant for `RecvMsg` on
streams. In most cases, if we are passing in an existing message to this
method, we want to reuse the buffers. This codec will always merge the
message when unmarshaling instead of resetting the input message.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Remove gogoproto in favor of the standard protobuf compiler. This
removes any nonstandard extensions that were part of gogoproto such as
the custom types.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Secret, SSH and Tmpfs mounts never have an input vertex
and zero value for input should not be considered and input
at index 0 .
Currently, if for example secret mount had a input=0, it
caused content based checksum from first input(rootfs),
that could take quite a lot of time when image was big
and had lots of files. The computation result was cached
but if there was a cache invalidation in previous commands
it caused checksum to recomputed again for the command with
the secret mount.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Switch to use github.com/containerd/platforms module, because containerd's
platforms package has moved to a separate module. This allows updating the
platforms parsing independent of the containerd module itself.
The package in containerd is deprecated, but kept as an alias to provide
compatibility between codebases.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This allows LLB-directed content-based cache enablement for each mount.
Some mounts may not be explicitly unabled (because it would be unsafe) -
for these cases we explicitly error out.
Signed-off-by: Justin Chadwell <me@jedevc.com>
These mounts are actually safe, as suggested by Erik on slack:
> Is it correct that this wouldn’t be a problem in the case where the
> selector of the mount is just “/“? Because then there’s no “hidden”
> files.
>
> If so, maybe there’s a path to enabling content cache for rw mounts
> that are from “/“. Then you could also get the same end behavior by
> not using selectors and instead always copying the subdir you want to
> mount to scratch.
This patch adds a check for this case, and explicitly enables
content-based cache for these cases.
Co-authored-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Justin Chadwell <me@jedevc.com>
This refactors the content-based cache to be that little bit tidier. In
addition to adding comments that explain *why* we're even bothering,
this restructures the code to avoid being unclear.
To explain the changes in a little more detail (since it's not
abundantly clear why this translation is valid), the initial condition
looks like:
if (!m.Readonly || m.Dest == pb.RootMount) && m.Output != -1 {
deps[m.Input].NoContentBasedHash = true
We can apply De Morgan's law recursively to invert the condition and the
result:
deps[m.Input].NoContentBasedHash = true
if (m.Readonly && m.Dest != pb.RootMount) || m.Output == -1 {
deps[m.Input].NoContentBasedHash = false
With all the juggling of NoContentBasedCache, we invert the variable
name to be ContentBasedCache (and invert everywhere it's used as well):
if (m.Readonly && m.Dest != pb.RootMount) || m.Output == -1 {
deps[m.Input].ContentBasedHash = true
This reads a bit easier, but now we split this into two separate
branches for readability (and so we can comment each one in more detail
separately):
if m.Readonly && m.Dest != pb.RootMount {
deps[m.Input].ContentBasedHash = true
}
if m.Output == -1 {
deps[m.Input].ContentBasedHash = true
}
While this has been the behavior for ages, I think it makes sense to
deliberately this behavior slightly. It doesn't make sense to me that we
should only disallow read-only root mounts, but no-output root mounts
are allowed - there's no reason these shouldn't behave identically, by
splitting these out:
if m.Readonly {
deps[m.Input].ContentBasedHash = true
}
if m.Output == -1 {
deps[m.Input].ContentBasedHash = true
}
if m.Dest == pb.RootMount {
deps[m.Input].ContentBasedHash = false
}
There is a small chance that this is a breaking change for some users,
however, 1. SkipOutput (-1) is very rare and not often used in the wild
(except for dockerfiles, where it's only used for non-root mounts), and 2.
will only cause a cache miss.
Signed-off-by: Justin Chadwell <me@jedevc.com>
A cache ID should not have any impact on whether or not a step should be
re-run any more than the content of that cache does (or rather,
doesn't).
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This updates #4270 to add an integration test and also merge some of the
logic for how the selectors are created. Now, `toSelectors` will perform
the root path detection instead of some custom logic in `getMountDeps`.
`dedupePaths` has also been updated to check if the number of paths is 1
or less so it can avoid an allocation when the function is a no-op.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Trivially created by looking for every reference to .Variant and adding
OSVersion and OSFeatures, except the ones related to the string
representation of a Platform instance.
I then went through and ensured every assignment of OSFeatures that
might leak out, i.e., not local-only or for marhsalling purposes, uses
the append-to-nil idiom to avoid sharing the slice storage and allowing
accidental mutation after-the-fact.
Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Correctly set the content based selector when multiple bind mounts refer
to the same source. Previously, a selector that referred to the root
filesystem would be ignored. This is because a blank selector refers to
the root filesystem.
When two bind mounts referred to the same dependency, one mount would
add a selector while the other would be skipped. This caused the cache
key to be only computed based on the more narrow filesystem which caused
erroneous cache hits.
Now, the creation of the selector includes the root filesystem for
consideration. It fills in `/` as the selector and then removes it later
so that we don't narrow the selection in an invalid way.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
This change properly handles paths on different platforms. In short, this
change checks the target platform we're building an image for and applies
normalization steps to make sure the file paths are valid. This makes buildkit
properly handle paths on both *nix systems and on Windows.
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This allows a frontend to request a specific for stubs removal.
By default, if not specified, this will revert to the previous
behaviour. New gateway clients however will set the property to the
desired recursive removal mode.
This property needs to be set for both components that call the
executor: for ExecOp, as well as for the StartContainer API.
Signed-off-by: Justin Chadwell <me@jedevc.com>
Change how provenance information is captured from builds.
While previously frontend passed the buildinfo
sources with metadata, now all information is captured
through buildkit. A frontend does not need to implement
buildinfo and can't set incorrect/incomplete buildinfo
for a build result.
All LLB operations can now collect as much provenance
info as they like that will be used when making the
attestation. Previously this was limited to a single Pin
value. For example now we also detect secrets and SSH IDs
that the build uses, or if it accesses network, if local
sources are used etc.. The new design makes sure this
can be easily extended in the future.
Provenance capture can now detect builds that do
multiple separate subsolves in sequence. For example,
first subsolve gathers the sources for the build and
second one builds from immutable sources without a
network connection. If first solve does not participate
in final build result it does not end up in provenance.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
This centralizes the location where ProgressKey gets set, which works
because it only needs information about the vertex, nothing op-specific.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Before this, the worker's FromRemote method only partially intialized
progress controllers, leaving out the vertex digest. This meant that
only status updates would be sent and vertex start/stops would not be
sent.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
