From 636c13f81814109f280b1cd1eae055719f0b7433 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Mon, 17 Jul 2023 10:36:48 -0400 Subject: [PATCH] build: do not attempt to push unnamed service images When building, if images are being pushed, ensure that only named images (i.e. services with a populated `image` field) are attempted to be pushed. Services without `image` get an auto-generated name, which will be a "Docker library" reference since they're in the format `$project-$service`, which is implicitly the same as `docker.io/library/$project-$service`. A push for that is never desirable / will always fail. The key here is that we cannot overwrite the `.image` field when doing builds, as we need to be able to check for its presence to determine whether a push makes sense. Fixes #10813. Signed-off-by: Milas Bowman --- cmd/compose/config.go | 6 +----- pkg/api/api.go | 1 - pkg/compose/build.go | 26 ++++++++++++++------------ pkg/compose/build_classic.go | 5 +++-- pkg/compose/pull.go | 11 +++++++++-- pkg/compose/viz.go | 6 +++--- pkg/e2e/build_test.go | 7 ++++++- 7 files changed, 36 insertions(+), 26 deletions(-) diff --git a/cmd/compose/config.go b/cmd/compose/config.go index 22d02358fc..16031ffeb7 100644 --- a/cmd/compose/config.go +++ b/cmd/compose/config.go @@ -233,11 +233,7 @@ func runConfigImages(streams api.Streams, opts configOptions, services []string) return err } for _, s := range project.Services { - if s.Image != "" { - fmt.Fprintln(streams.Out(), s.Image) - } else { - fmt.Fprintf(streams.Out(), "%s%s%s\n", project.Name, api.Separator, s.Name) - } + fmt.Fprintln(streams.Out(), api.GetImageNameOrDefault(s, project.Name)) } return nil } diff --git a/pkg/api/api.go b/pkg/api/api.go index a693abb501..09f2858d2f 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -145,7 +145,6 @@ func (o BuildOptions) Apply(project *types.Project) error { if service.Build == nil { continue } - service.Image = GetImageNameOrDefault(service, project.Name) if platform != "" { if len(service.Build.Platforms) > 0 && !utils.StringContains(service.Build.Platforms, platform) { return fmt.Errorf("service %q build.platforms does not support value set by DOCKER_DEFAULT_PLATFORM: %s", service.Name, platform) diff --git a/pkg/compose/build.go b/pkg/compose/build.go index f513b309a8..9840b79c90 100644 --- a/pkg/compose/build.go +++ b/pkg/compose/build.go @@ -77,7 +77,7 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opti return nil, err } - builtIDs := make([]string, len(project.Services)) + builtDigests := make([]string, len(project.Services)) err = InDependencyOrder(ctx, project, func(ctx context.Context, name string) error { if len(options.Services) > 0 && !utils.Contains(options.Services, name) { return nil @@ -94,11 +94,11 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opti } else { service.Build.Args = service.Build.Args.OverrideBy(args) } - id, err := s.doBuildClassic(ctx, service, options) + id, err := s.doBuildClassic(ctx, project.Name, service, options) if err != nil { return err } - builtIDs[idx] = id + builtDigests[idx] = id if options.Push { return s.push(ctx, project, api.PushOptions{}) @@ -120,7 +120,7 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opti if err != nil { return err } - builtIDs[idx] = digest + builtDigests[idx] = digest return nil }, func(traversal *graphTraversal) { @@ -137,9 +137,10 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opti } imageIDs := map[string]string{} - for i, d := range builtIDs { - if d != "" { - imageIDs[project.Services[i].Image] = d + for i, imageDigest := range builtDigests { + if imageDigest != "" { + imageRef := api.GetImageNameOrDefault(project.Services[i], project.Name) + imageIDs[imageRef] = imageDigest } } return imageIDs, err @@ -222,7 +223,8 @@ func (s *composeService) prepareProjectForBuild(project *types.Project, images m continue } - _, localImagePresent := images[service.Image] + image := api.GetImageNameOrDefault(service, project.Name) + _, localImagePresent := images[image] if localImagePresent && service.PullPolicy != types.PullPolicyBuild { service.Build = nil project.Services[i] = service @@ -292,8 +294,6 @@ func (s *composeService) getLocalImagesDigests(ctx context.Context, project *typ } func (s *composeService) toBuildOptions(project *types.Project, service types.ServiceConfig, options api.BuildOptions) (build.Options, error) { - tags := []string{service.Image} - buildArgs := flatten(service.Build.Args.Resolve(envResolver(project.Environment))) for k, v := range storeutil.GetProxyConfig(s.dockerCli) { @@ -335,6 +335,7 @@ func (s *composeService) toBuildOptions(project *types.Project, service types.Se sessionConfig = append(sessionConfig, secretsProvider) } + tags := []string{api.GetImageNameOrDefault(service, project.Name)} if len(service.Build.Tags) > 0 { tags = append(tags, service.Build.Tags...) } @@ -345,18 +346,19 @@ func (s *composeService) toBuildOptions(project *types.Project, service types.Se imageLabels := getImageBuildLabels(project, service) + push := options.Push && service.Image != "" exports := []bclient.ExportEntry{{ Type: "docker", Attrs: map[string]string{ "load": "true", - "push": fmt.Sprint(options.Push), + "push": fmt.Sprint(push), }, }} if len(service.Build.Platforms) > 1 { exports = []bclient.ExportEntry{{ Type: "image", Attrs: map[string]string{ - "push": fmt.Sprint(options.Push), + "push": fmt.Sprint(push), }, }} } diff --git a/pkg/compose/build_classic.go b/pkg/compose/build_classic.go index e0d2fb32c0..2f08837a69 100644 --- a/pkg/compose/build_classic.go +++ b/pkg/compose/build_classic.go @@ -45,7 +45,7 @@ import ( ) //nolint:gocyclo -func (s *composeService) doBuildClassic(ctx context.Context, service types.ServiceConfig, options api.BuildOptions) (string, error) { +func (s *composeService) doBuildClassic(ctx context.Context, projectName string, service types.ServiceConfig, options api.BuildOptions) (string, error) { var ( buildCtx io.ReadCloser dockerfileCtx io.ReadCloser @@ -160,7 +160,8 @@ func (s *composeService) doBuildClassic(ctx context.Context, service types.Servi authConfigs[k] = registry.AuthConfig(auth) } buildOptions := imageBuildOptions(service.Build) - buildOptions.Tags = append(buildOptions.Tags, service.Image) + imageName := api.GetImageNameOrDefault(service, projectName) + buildOptions.Tags = append(buildOptions.Tags, imageName) buildOptions.Dockerfile = relDockerfile buildOptions.AuthConfigs = authConfigs buildOptions.Memory = options.Memory diff --git a/pkg/compose/pull.go b/pkg/compose/pull.go index 96191c39d8..41fd6d7687 100644 --- a/pkg/compose/pull.go +++ b/pkg/compose/pull.go @@ -313,8 +313,15 @@ func isServiceImageToBuild(service types.ServiceConfig, services []types.Service return true } - for _, depService := range services { - if depService.Image == service.Image && depService.Build != nil { + if service.Image == "" { + // N.B. this should be impossible as service must have either `build` or `image` (or both) + return false + } + + // look through the other services to see if another has a build definition for the same + // image name + for _, svc := range services { + if svc.Image == service.Image && svc.Build != nil { return true } } diff --git a/pkg/compose/viz.go b/pkg/compose/viz.go index f87056856d..25ea7a2be3 100644 --- a/pkg/compose/viz.go +++ b/pkg/compose/viz.go @@ -52,7 +52,7 @@ func (s *composeService) Viz(_ context.Context, project *types.Project, opts api // dot is the perfect layout for this use case since graph is directed and hierarchical graphBuilder.WriteString(opts.Indentation + "layout=dot;\n") - addNodes(&graphBuilder, graph, &opts) + addNodes(&graphBuilder, graph, project.Name, &opts) graphBuilder.WriteByte('\n') addEdges(&graphBuilder, graph, &opts) @@ -63,7 +63,7 @@ func (s *composeService) Viz(_ context.Context, project *types.Project, opts api // addNodes adds the corresponding graphviz representation of all the nodes in the given graph to the graphBuilder // returns the same graphBuilder -func addNodes(graphBuilder *strings.Builder, graph vizGraph, opts *api.VizOptions) *strings.Builder { +func addNodes(graphBuilder *strings.Builder, graph vizGraph, projectName string, opts *api.VizOptions) *strings.Builder { for serviceNode := range graph { // write: // "service name" [style="filled" label<service name @@ -107,7 +107,7 @@ func addNodes(graphBuilder *strings.Builder, graph vizGraph, opts *api.VizOption if opts.IncludeImageName { graphBuilder.WriteString("") graphBuilder.WriteString("

Image:
") - graphBuilder.WriteString(serviceNode.Image) + graphBuilder.WriteString(api.GetImageNameOrDefault(*serviceNode, projectName)) graphBuilder.WriteString("") } diff --git a/pkg/e2e/build_test.go b/pkg/e2e/build_test.go index 76b8c6427d..243f5ee5f8 100644 --- a/pkg/e2e/build_test.go +++ b/pkg/e2e/build_test.go @@ -111,7 +111,7 @@ func TestLocalComposeBuild(t *testing.T) { t.Run(env+" no rebuild when up again", func(t *testing.T) { res := c.RunDockerComposeCmd(t, "--project-directory", "fixtures/build-test", "up", "-d") - assert.Assert(t, !strings.Contains(res.Stdout(), "COPY static"), res.Stdout()) + assert.Assert(t, !strings.Contains(res.Stdout(), "COPY static")) }) t.Run(env+" rebuild when up --build", func(t *testing.T) { @@ -121,6 +121,11 @@ func TestLocalComposeBuild(t *testing.T) { res.Assert(t, icmd.Expected{Out: "COPY static2 /usr/share/nginx/html"}) }) + t.Run(env+" build --push ignored for unnamed images", func(t *testing.T) { + res := c.RunDockerComposeCmd(t, "--workdir", "fixtures/build-test", "build", "--push", "nginx") + assert.Assert(t, !strings.Contains(res.Stdout(), "failed to push"), res.Stdout()) + }) + 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")