From bfa54081d4133a272a0e3271ffb45a00f71884ea Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Thu, 10 Aug 2023 08:57:28 -0400 Subject: [PATCH] build: fix missing proxy build args for classic builder (#10887) 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 --- pkg/compose/build.go | 75 +++++++++++++++++++----------------- pkg/compose/build_classic.go | 13 ++++--- pkg/e2e/build_test.go | 23 +++++------ 3 files changed, 60 insertions(+), 51 deletions(-) diff --git a/pkg/compose/build.go b/pkg/compose/build.go index 3b19c75a..cf2f8a36 100644 --- a/pkg/compose/build.go +++ b/pkg/compose/build.go @@ -22,18 +22,19 @@ import ( "os" "path/filepath" - "github.com/docker/buildx/builder" - "github.com/docker/compose/v2/internal/tracing" - - "github.com/docker/buildx/controller/pb" - "github.com/compose-spec/compose-go/types" "github.com/containerd/containerd/platforms" "github.com/docker/buildx/build" - _ "github.com/docker/buildx/driver/docker" // required to get default driver registered + "github.com/docker/buildx/builder" + "github.com/docker/buildx/controller/pb" "github.com/docker/buildx/store/storeutil" "github.com/docker/buildx/util/buildflags" xprogress "github.com/docker/buildx/util/progress" + "github.com/docker/cli/cli/command" + "github.com/docker/compose/v2/internal/tracing" + "github.com/docker/compose/v2/pkg/api" + "github.com/docker/compose/v2/pkg/progress" + "github.com/docker/compose/v2/pkg/utils" "github.com/docker/docker/builder/remotecontext/urlutil" bclient "github.com/moby/buildkit/client" "github.com/moby/buildkit/session" @@ -45,9 +46,8 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" - "github.com/docker/compose/v2/pkg/api" - "github.com/docker/compose/v2/pkg/progress" - "github.com/docker/compose/v2/pkg/utils" + // required to get default driver registered + _ "github.com/docker/buildx/driver/docker" ) func (s *composeService) Build(ctx context.Context, project *types.Project, options api.BuildOptions) error { @@ -61,9 +61,8 @@ func (s *composeService) Build(ctx context.Context, project *types.Project, opti }, s.stdinfo(), "Building") } -func (s *composeService) build(ctx context.Context, project *types.Project, options api.BuildOptions) (map[string]string, error) { //nolint:gocyclo - args := options.Args.Resolve(envResolver(project.Environment)) - +//nolint:gocyclo +func (s *composeService) build(ctx context.Context, project *types.Project, options api.BuildOptions) (map[string]string, error) { buildkitEnabled, err := s.dockerCli.BuildKitEnabled() if err != nil { return nil, err @@ -119,12 +118,7 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opti } if !buildkitEnabled { - if service.Build.Args == nil { - service.Build.Args = args - } else { - service.Build.Args = service.Build.Args.OverrideBy(args) - } - id, err := s.doBuildClassic(ctx, project.Name, service, options) + id, err := s.doBuildClassic(ctx, project, service, options) if err != nil { return err } @@ -144,7 +138,6 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opti if err != nil { return err } - buildOptions.BuildArgs = mergeArgs(buildOptions.BuildArgs, flatten(args)) digest, err := s.doBuildBuildkit(ctx, service.Name, buildOptions, w, nodes) if err != nil { @@ -337,15 +330,37 @@ func (s *composeService) getLocalImagesDigests(ctx context.Context, project *typ return images, nil } -func (s *composeService) toBuildOptions(project *types.Project, service types.ServiceConfig, options api.BuildOptions) (build.Options, error) { - buildArgs := flatten(service.Build.Args.Resolve(envResolver(project.Environment))) +// resolveAndMergeBuildArgs returns the final set of build arguments to use for the service image build. +// +// First, args directly defined via `build.args` in YAML are considered. +// Then, any explicitly passed args in opts (e.g. via `--build-arg` on the CLI) are merged, overwriting any +// keys that already exist. +// Next, any keys without a value are resolved using the project environment. +// +// Finally, standard proxy variables based on the Docker client configuration are added, but will not overwrite +// any values if already present. +func resolveAndMergeBuildArgs( + dockerCli command.Cli, + project *types.Project, + service types.ServiceConfig, + opts api.BuildOptions, +) types.MappingWithEquals { + result := make(types.MappingWithEquals). + OverrideBy(service.Build.Args). + OverrideBy(opts.Args). + Resolve(envResolver(project.Environment)) - for k, v := range storeutil.GetProxyConfig(s.dockerCli) { - if _, ok := buildArgs[k]; !ok { - buildArgs[k] = v + // proxy arguments do NOT override and should NOT have env resolution applied, + // so they're handled last + for k, v := range storeutil.GetProxyConfig(dockerCli) { + if _, ok := result[k]; !ok { + result[k] = &v } } + return result +} +func (s *composeService) toBuildOptions(project *types.Project, service types.ServiceConfig, options api.BuildOptions) (build.Options, error) { plats, err := addPlatforms(project, service) if err != nil { return build.Options{}, err @@ -418,7 +433,7 @@ func (s *composeService) toBuildOptions(project *types.Project, service types.Se CacheTo: pb.CreateCaches(cacheTo), NoCache: service.Build.NoCache, Pull: service.Build.Pull, - BuildArgs: buildArgs, + BuildArgs: flatten(resolveAndMergeBuildArgs(s.dockerCli, project, service, options)), Tags: tags, Target: service.Build.Target, Exports: exports, @@ -445,16 +460,6 @@ func flatten(in types.MappingWithEquals) types.Mapping { return out } -func mergeArgs(m ...types.Mapping) types.Mapping { - merged := types.Mapping{} - for _, mapping := range m { - for key, val := range mapping { - merged[key] = val - } - } - return merged -} - func dockerFilePath(ctxName string, dockerfile string) string { if dockerfile == "" { return "" diff --git a/pkg/compose/build_classic.go b/pkg/compose/build_classic.go index 2f08837a..243ffb17 100644 --- a/pkg/compose/build_classic.go +++ b/pkg/compose/build_classic.go @@ -26,6 +26,8 @@ import ( "runtime" "strings" + "github.com/docker/cli/cli/command" + "github.com/docker/docker/api/types/registry" "github.com/compose-spec/compose-go/types" @@ -45,7 +47,7 @@ import ( ) //nolint:gocyclo -func (s *composeService) doBuildClassic(ctx context.Context, projectName string, service types.ServiceConfig, options api.BuildOptions) (string, error) { +func (s *composeService) doBuildClassic(ctx context.Context, project *types.Project, service types.ServiceConfig, options api.BuildOptions) (string, error) { var ( buildCtx io.ReadCloser dockerfileCtx io.ReadCloser @@ -159,8 +161,8 @@ func (s *composeService) doBuildClassic(ctx context.Context, projectName string, for k, auth := range creds { authConfigs[k] = registry.AuthConfig(auth) } - buildOptions := imageBuildOptions(service.Build) - imageName := api.GetImageNameOrDefault(service, projectName) + buildOptions := imageBuildOptions(s.dockerCli, project, service, options) + imageName := api.GetImageNameOrDefault(service, project.Name) buildOptions.Tags = append(buildOptions.Tags, imageName) buildOptions.Dockerfile = relDockerfile buildOptions.AuthConfigs = authConfigs @@ -215,14 +217,15 @@ func isLocalDir(c string) bool { return err == nil } -func imageBuildOptions(config *types.BuildConfig) dockertypes.ImageBuildOptions { +func imageBuildOptions(dockerCli command.Cli, project *types.Project, service types.ServiceConfig, options api.BuildOptions) dockertypes.ImageBuildOptions { + config := service.Build return dockertypes.ImageBuildOptions{ Version: dockertypes.BuilderV1, Tags: config.Tags, NoCache: config.NoCache, Remove: true, PullParent: config.Pull, - BuildArgs: config.Args, + BuildArgs: resolveAndMergeBuildArgs(dockerCli, project, service, options), Labels: config.Labels, NetworkMode: config.Network, ExtraHosts: config.ExtraHosts.AsList(), diff --git a/pkg/e2e/build_test.go b/pkg/e2e/build_test.go index 0f4702c3..7e6d5590 100644 --- a/pkg/e2e/build_test.go +++ b/pkg/e2e/build_test.go @@ -38,8 +38,8 @@ func TestLocalComposeBuild(t *testing.T) { t.Run(env+" build named and unnamed images", func(t *testing.T) { // ensure local test run does not reuse previously build image - c.RunDockerOrExitError(t, "rmi", "build-test-nginx") - c.RunDockerOrExitError(t, "rmi", "custom-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "build-test-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "custom-nginx") res := c.RunDockerComposeCmd(t, "--project-directory", "fixtures/build-test", "build") @@ -50,8 +50,8 @@ func TestLocalComposeBuild(t *testing.T) { t.Run(env+" build with build-arg", func(t *testing.T) { // ensure local test run does not reuse previously build image - c.RunDockerOrExitError(t, "rmi", "build-test-nginx") - c.RunDockerOrExitError(t, "rmi", "custom-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "build-test-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "custom-nginx") c.RunDockerComposeCmd(t, "--project-directory", "fixtures/build-test", "build", "--build-arg", "FOO=BAR") @@ -61,8 +61,8 @@ func TestLocalComposeBuild(t *testing.T) { t.Run(env+" build with build-arg set by env", func(t *testing.T) { // ensure local test run does not reuse previously build image - c.RunDockerOrExitError(t, "rmi", "build-test-nginx") - c.RunDockerOrExitError(t, "rmi", "custom-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "build-test-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "custom-nginx") icmd.RunCmd(c.NewDockerComposeCmd(t, "--project-directory", @@ -72,7 +72,7 @@ func TestLocalComposeBuild(t *testing.T) { "FOO"), func(cmd *icmd.Cmd) { cmd.Env = append(cmd.Env, "FOO=BAR") - }) + }).Assert(t, icmd.Success) res := c.RunDockerCmd(t, "image", "inspect", "build-test-nginx") res.Assert(t, icmd.Expected{Out: `"FOO": "BAR"`}) @@ -92,8 +92,9 @@ func TestLocalComposeBuild(t *testing.T) { }) t.Run(env+" build as part of up", func(t *testing.T) { - c.RunDockerOrExitError(t, "rmi", "build-test-nginx") - c.RunDockerOrExitError(t, "rmi", "custom-nginx") + // ensure local test run does not reuse previously build image + c.RunDockerOrExitError(t, "rmi", "-f", "build-test-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "custom-nginx") res := c.RunDockerComposeCmd(t, "--project-directory", "fixtures/build-test", "up", "-d") t.Cleanup(func() { @@ -130,8 +131,8 @@ func TestLocalComposeBuild(t *testing.T) { t.Run(env+" cleanup build project", func(t *testing.T) { c.RunDockerComposeCmd(t, "--project-directory", "fixtures/build-test", "down") - c.RunDockerCmd(t, "rmi", "build-test-nginx") - c.RunDockerCmd(t, "rmi", "custom-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "build-test-nginx") + c.RunDockerOrExitError(t, "rmi", "-f", "custom-nginx") }) }