The big change here is to pass around an explicit `*BuildOptions` object
as part of Compose operations like `up` & `run` that may or may not do
builds. If the options object is `nil`, no builds whatsoever will be
attempted.
Motivation is to allow for partial rebuilds in the context of an `up`
for watch. This was broken and tricky to accomplish because various parts
of the Compose APIs mutate the `*Project` for convenience in ways that
make it unusable afterwards. (For example, it might set `service.Build = nil`
because it's not going to build that service right _then_. But we might
still want to build it later!)
NOTE: This commit does not actually touch the watch logic. This is all
in preparation to make it possible.
As part of this, a bunch of code moved around and I eliminated a bunch
of partially redundant logic, mostly around multi-platform. Several
edge cases have been addressed as part of this:
* `DOCKER_DEFAULT_PLATFORM` was _overriding_ explicitly set platforms
in some cases, this is no longer true, and it behaves like the Docker
CLI now
* It was possible for Compose to build an image for one platform and
then try to run it for a different platform (and fail)
* Errors are no longer returned if a local image exists but for the
wrong platform - the correct platform will be fetched/built (if
possible).
Because there's a LOT of subtlety and tricky logic here, I've also tried
to add an excessive amount of explanatory comments.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
If running `up` in foreground mode (i.e. not `-d`),
when exiting via `Ctrl-C`, Compose stops all the
services it launched directly as part of that `up`
command.
In one of the E2E tests (`TestUpDependenciesNotStopped`),
this was occasionally flaking because the stop
behavior was racy: the return might not block on
the stop operation because it gets added to the
error group in a goroutine. As a result, it was
possible for no services to get terminated on exit.
There were a few other related pieces here that
I uncovered and tried to fix while stressing this.
For example, the printer could cause a deadlock if
an event was sent to it after it stopped.
Also, an error group wasn't really appropriate here;
each goroutine is a different operation for printing,
signal-handling, etc. If one part fails, we don't
actually want printing to stop, for example. This has
been switched to a `multierror.Group`, which has the
same API but coalesces errors instead of canceling a
context the moment the first one fails and returning
that single error.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
The uuid package in distribution was created as a utility for the distribution
project itself, to cut down external dependencies (see [1][1]).
For compose, this has the reverse effect, as it now brings all the dependencies
of the distribution module with it.
This patch switches to the uuid generation to crypto/rand to produce a random
id. I was considering using a different uuid implementation, or docker's
"stringid.GenerateRandomID", but all of those are doing more than needed,
so keep it simple.
Currently, this change has little effect, because compose also uses the
distribution module for other purposes, but the distribution project is
in the process of moving the "reference" package to a separate module,
in which case we don't want to depend on the distribution module only for
the uuid package.
[1]: 36e34a55ad
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
* Use unique project name prefixes (some of these tests assert
on output using the project name as a magic string, so could
be impacted by other tests with the same project name prefix)
* Tear down port range project before starting to try and avoid
race conditions with the engine and port assignment
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
This is a good place to start introducing (local) exclusivity
to Compose. Now, when `alpha watch` launches, it will check for
the existence of a PID file in the user XDG runtime directory,
and create one if the existing one is stale or does not exist.
If the PID file exists and is valid, an error is returned and
Compose exits.
A slight tweak to the experimental remote Git loader has been
made to use the XDG package for consistency.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
We can't assume we receive container logs line by line. Some framework won't buffer output and will send char by char, and we also can receive looong lines which get buffered to 32kb and then cut into multiple logs.
This assumes we will catch container streams being closed before we receive a die event for container, which could be subject to race condition, but at least the impact here is minimal and the fix works for reproduction examples provided in linked issues.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
The BuildKit dockerignore package was integrated in the patternmatcher
repository / module. This patch updates our uses of the BuildKit package
with its new location.
A small local change was made to keep the format of the existing error message,
because the "ignorefile" package is slightly more agnostic in that respect
and doesn't include ".dockerignore" in the error message.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
By default, `compose up` attaches to all services (i.e.
shows log output from every associated container). If
a service is specified, e.g. `compose up foo`, then
only `foo`'s logs are tailed. The `--attach-dependencies`
flag can also be used, so that if `foo` depended upon
`bar`, then `bar`'s logs would also be followed. It's
also possible to use `--no-attach` to filter out one
or more services explicitly, e.g. `compose up --no-attach=noisy`
would launch all services, including `noisy`, and would
show log output from every service _except_ `noisy`.
Lastly, it's possible to use `up --attach` to explicitly
restrict to a subset of services (or their dependencies).
How these flags interact with each other is also worth
thinking through.
There were a few different connected issues here, but
the primary issue was that running `compose up foo` was
always attaching dependencies regardless of `--attach-dependencies`.
The filtering logic here has been updated so that it
behaves predictably both when launching all services
(`compose up`) or a subset (`compose up foo`) as well
as various flag combinations on top of those.
Notably, this required making some changes to how it
watches containers. The logic here between attaching
for logs and monitoring for lifecycle changes is
tightly coupled, so some changes were needed to ensure
that the full set of services being `up`'d are _watched_
and the subset that should have logs shown are _attached_.
(This does mean faking the attach with an event but not
actually doing it.)
While handling that, I adjusted the context lifetimes
here, which improves error handling that gets shown to
the user and should help avoid potential leaks by getting
rid of a `context.Background()`.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Refactor to use a consistent code path for determining the build
args for a service image regardless of whether BuildKit or the
classic builder is being used.
After recent changes, these code paths had diverged, so the classic
builder was missing the proxy variables from the Docker client
config.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
This was left over from debugging, but we should not block.
OTel will handle the connection in the background.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Swap the default implementation now that batching is merged.
Keeping the `docker cp` based implementation around for the
moment, but it needs to be _explicitly_ disabled now by setting
`COMPOSE_EXPERIMENTAL_WATCH_TAR=0`.
After the next release, we should remove the `docker cp`
implementation entirely.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
If an optional dependency exits successfully (exit code of 0),
with a service condition of `service_completed_successfully`,
don't log a warning.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
* When waiting for dependencies, `select` on the context as well
as the ticker
* Write multiple progress events "transactionally" (i.e. hold the
lock for the duration to avoid other events being interleaved)
* Do not change "finished" steps back to "in progress" to prevent
flickering
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Move builder and nodes initialization code up, avoiding to recreate/load them for every service build.
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Adjust the debouncing logic so that it applies to all inbound file
events, regardless of whether they match a sync or rebuild rule.
When the batch is flushed out, if any event for the service is a
rebuild event, then the service is rebuilt and all sync events for
the batch are ignored. If _all_ events in the batch are sync events,
then a sync is triggered, passing the entire batch at once. This
provides a substantial performance win for the new `tar`-based
implementation, as it can efficiently transfer the changes in bulk.
Additionally, this helps with jitter, e.g. it's not uncommon for
there to be double-writes in quick succession to a file, so even if
there's not many files being modified at once, it can still prevent
some unnecessary transfers.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Support services with scale > 1 for the tar watch sync.
Add a "lossy" multi-writer specific to pipes that writes the
tar data to each `io.PipeWriter`, which is connected to `stdin`
for the `tar` process being exec'd in the container.
The data is written serially to each writer. This could be
adjusted to do concurrent writes but that will rapidly increase
the I/O load, so is not done here - in general, 99% of the
time you'll be developing (and thus using watch/sync) with a
single replica of a service.
If a write fails, the corresponding `io.PipeWriter` is removed
from the active set and closed with an error.
This means that a single container copy failing won't stop
writes to the others that are succeeding. Of course, they will
be in an inconsistent state afterwards still, but that's a
different problem.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
We cannot guarantee the exact value of `CapEff` across
environments, and this test has started failing some places,
e.g. Docker Desktop, and now GitHub Actions (likely due to
a kernel upgrade on the runners or similar).
By setting `privileged: true` on the build, we're asking for
the `security.insecure` entitlement on the build. A safe
assumption is that will include `CAP_SYS_ADMIN`, which won't
be present otherwise, so mask the `CapEff` value and check
for that.
It's worth noting that realistically, the build won't even
be able to complete without the correct entitlement, since the
`Dockerfile` uses `RUN --security=insecure`, so this is really
an additional sanity check.
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Includes a fix for CVE-2023-29409
go1.20.7 (released 2023-08-01) includes a security fix to the crypto/tls
package, as well as bug fixes to the assembler and the compiler. See the
Go 1.20.7 milestone on our issue tracker for details:
- https://github.com/golang/go/issues?q=milestone%3AGo1.20.7+label%3ACherryPickApproved
- full diff: https://github.com/golang/go/compare/go1.20.6...go1.20.7
From the mailing list announcement:
[security] Go 1.20.7 and Go 1.19.12 are released
Hello gophers,
We have just released Go versions 1.20.7 and 1.19.12, minor point releases.
These minor releases include 1 security fixes following the security policy:
- crypto/tls: restrict RSA keys in certificates to <= 8192 bits
Extremely large RSA keys in certificate chains can cause a client/server
to expend significant CPU time verifying signatures. Limit this by
restricting the size of RSA keys transmitted during handshakes to <=
8192 bits.
Based on a survey of publicly trusted RSA keys, there are currently only
three certificates in circulation with keys larger than this, and all
three appear to be test certificates that are not actively deployed. It
is possible there are larger keys in use in private PKIs, but we target
the web PKI, so causing breakage here in the interests of increasing the
default safety of users of crypto/tls seems reasonable.
Thanks to Mateusz Poliwczak for reporting this issue.
View the release notes for more information:
https://go.dev/doc/devel/release#go1.20.7
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Milas Bowman
Sebastiaan van Stijn
Bilal Khan
Nicolas De Loof
dependabot[bot]
Guillaume Lours
Silvin Lubecki
relrelb
Praful Gupta
Silvin Lubecki