From 6283fee6a249c514dd5b9e886ead3a2aecd1a8ec Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 3 Jul 2018 02:31:05 +0000 Subject: [PATCH] api: Change Platform field back to string (temporary workaround) This partially reverts https://github.com/moby/moby/pull/37350 Although specs.Platform is desirable in the API, there is more work to be done on helper functions, namely containerd's platforms.Parse that assumes the default platform of the Go runtime. That prevents a client to use the recommended Parse function to retrieve a specs.Platform object. With this change, no parsing is expected from the client. Signed-off-by: Tibor Vass Upstream-commit: facad557440a0c955beb615495b8d0175f25e4e3 Component: engine --- .../api/server/router/build/build_routes.go | 16 ++-------- components/engine/api/types/client.go | 5 ++- .../engine/builder/builder-next/builder.go | 14 +++++++-- .../engine/builder/dockerfile/builder.go | 31 ++++++++++++++++--- components/engine/builder/dockerfile/copy.go | 2 +- .../engine/builder/dockerfile/dispatchers.go | 4 +-- .../builder/dockerfile/dispatchers_test.go | 6 ++-- .../engine/builder/dockerfile/internals.go | 10 +++--- components/engine/client/image_build.go | 15 ++++----- 9 files changed, 60 insertions(+), 43 deletions(-) diff --git a/components/engine/api/server/router/build/build_routes.go b/components/engine/api/server/router/build/build_routes.go index 827ba3a713..c4699f3d86 100644 --- a/components/engine/api/server/router/build/build_routes.go +++ b/components/engine/api/server/router/build/build_routes.go @@ -14,7 +14,6 @@ import ( "strings" "sync" - "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/server/httputils" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" @@ -24,8 +23,7 @@ import ( "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/progress" "github.com/docker/docker/pkg/streamformatter" - "github.com/docker/docker/pkg/system" - "github.com/docker/go-units" + units "github.com/docker/go-units" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -72,17 +70,7 @@ func newImageBuildOptions(ctx context.Context, r *http.Request) (*types.ImageBui options.Target = r.FormValue("target") options.RemoteContext = r.FormValue("remote") if versions.GreaterThanOrEqualTo(version, "1.32") { - apiPlatform := r.FormValue("platform") - if apiPlatform != "" { - sp, err := platforms.Parse(apiPlatform) - if err != nil { - return nil, err - } - if err := system.ValidatePlatform(sp); err != nil { - return nil, err - } - options.Platform = &sp - } + options.Platform = r.FormValue("platform") } if r.Form.Get("shmsize") != "" { diff --git a/components/engine/api/types/client.go b/components/engine/api/types/client.go index 33bc98e0bb..3b698c2c24 100644 --- a/components/engine/api/types/client.go +++ b/components/engine/api/types/client.go @@ -7,8 +7,7 @@ import ( "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/filters" - "github.com/docker/go-units" - specs "github.com/opencontainers/image-spec/specs-go/v1" + units "github.com/docker/go-units" ) // CheckpointCreateOptions holds parameters to create a checkpoint from a container @@ -181,7 +180,7 @@ type ImageBuildOptions struct { ExtraHosts []string // List of extra hosts Target string SessionID string - Platform *specs.Platform + Platform string // Version specifies the version of the unerlying builder to use Version BuilderVersion // BuildID is an optional identifier that can be passed together with the diff --git a/components/engine/builder/builder-next/builder.go b/components/engine/builder/builder-next/builder.go index 73520db9fd..b1d31a5225 100644 --- a/components/engine/builder/builder-next/builder.go +++ b/components/engine/builder/builder-next/builder.go @@ -14,6 +14,7 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/daemon/images" "github.com/docker/docker/pkg/streamformatter" + "github.com/docker/docker/pkg/system" controlapi "github.com/moby/buildkit/api/services/control" "github.com/moby/buildkit/control" "github.com/moby/buildkit/identity" @@ -208,8 +209,17 @@ func (b *Builder) Build(ctx context.Context, opt backend.BuildConfig) (*builder. frontendAttrs["no-cache"] = "" } - if opt.Options.Platform != nil { - frontendAttrs["platform"] = platforms.Format(*opt.Options.Platform) + if opt.Options.Platform != "" { + // same as in newBuilder in builder/dockerfile.builder.go + // TODO: remove once opt.Options.Platform is of type specs.Platform + sp, err := platforms.Parse(opt.Options.Platform) + if err != nil { + return nil, err + } + if err := system.ValidatePlatform(sp); err != nil { + return nil, err + } + frontendAttrs["platform"] = opt.Options.Platform } exporterAttrs := map[string]string{} diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go index 1a0b680c37..ee95f23689 100644 --- a/components/engine/builder/dockerfile/builder.go +++ b/components/engine/builder/dockerfile/builder.go @@ -10,6 +10,7 @@ import ( "strings" "time" + "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" @@ -25,6 +26,7 @@ import ( "github.com/moby/buildkit/frontend/dockerfile/parser" "github.com/moby/buildkit/frontend/dockerfile/shell" "github.com/moby/buildkit/session" + specs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sync/syncmap" @@ -111,7 +113,11 @@ func (bm *BuildManager) Build(ctx context.Context, config backend.BuildConfig) ( PathCache: bm.pathCache, IDMappings: bm.idMappings, } - return newBuilder(ctx, builderOptions).build(source, dockerfile) + b, err := newBuilder(ctx, builderOptions) + if err != nil { + return nil, err + } + return b.build(source, dockerfile) } func (bm *BuildManager) initializeClientSession(ctx context.Context, cancel func(), options *types.ImageBuildOptions) (builder.Source, error) { @@ -175,10 +181,11 @@ type Builder struct { pathCache pathCache containerManager *containerManager imageProber ImageProber + platform *specs.Platform } // newBuilder creates a new Dockerfile builder from an optional dockerfile and a Options. -func newBuilder(clientCtx context.Context, options builderOptions) *Builder { +func newBuilder(clientCtx context.Context, options builderOptions) (*Builder, error) { config := options.Options if config == nil { config = new(types.ImageBuildOptions) @@ -199,7 +206,20 @@ func newBuilder(clientCtx context.Context, options builderOptions) *Builder { containerManager: newContainerManager(options.Backend), } - return b + // same as in Builder.Build in builder/builder-next/builder.go + // TODO: remove once config.Platform is of type specs.Platform + if config.Platform != "" { + sp, err := platforms.Parse(config.Platform) + if err != nil { + return nil, err + } + if err := system.ValidatePlatform(sp); err != nil { + return nil, err + } + b.platform = &sp + } + + return b, nil } // Build 'LABEL' command(s) from '--label' options and add to the last stage @@ -365,9 +385,12 @@ func BuildFromConfig(config *container.Config, changes []string, os string) (*co return nil, errdefs.InvalidParameter(err) } - b := newBuilder(context.Background(), builderOptions{ + b, err := newBuilder(context.Background(), builderOptions{ Options: &types.ImageBuildOptions{NoCache: true}, }) + if err != nil { + return nil, err + } // ensure that the commands are valid for _, n := range dockerfile.AST.Children { diff --git a/components/engine/builder/dockerfile/copy.go b/components/engine/builder/dockerfile/copy.go index 7e9dc6036a..74e245bdc4 100644 --- a/components/engine/builder/dockerfile/copy.go +++ b/components/engine/builder/dockerfile/copy.go @@ -87,7 +87,7 @@ func copierFromDispatchRequest(req dispatchRequest, download sourceDownloader, i pathCache: req.builder.pathCache, download: download, imageSource: imageSource, - platform: req.builder.options.Platform, + platform: req.builder.platform, } } diff --git a/components/engine/builder/dockerfile/dispatchers.go b/components/engine/builder/dockerfile/dispatchers.go index f59b7c844c..6ee2b17cda 100644 --- a/components/engine/builder/dockerfile/dispatchers.go +++ b/components/engine/builder/dockerfile/dispatchers.go @@ -146,7 +146,7 @@ func (d *dispatchRequest) getImageMount(imageRefOrID string) (*imageMount, error imageRefOrID = stage.Image localOnly = true } - return d.builder.imageSources.Get(imageRefOrID, localOnly, d.builder.options.Platform) + return d.builder.imageSources.Get(imageRefOrID, localOnly, d.builder.platform) } // FROM [--platform=platform] imagename[:tag | @digest] [AS build-stage-name] @@ -238,7 +238,7 @@ func (d *dispatchRequest) getImageOrStage(name string, platform *specs.Platform) } if platform == nil { - platform = d.builder.options.Platform + platform = d.builder.platform } // Windows cannot support a container with no base image unless it is LCOW. diff --git a/components/engine/builder/dockerfile/dispatchers_test.go b/components/engine/builder/dockerfile/dispatchers_test.go index 047a8742e8..c61a45b03a 100644 --- a/components/engine/builder/dockerfile/dispatchers_test.go +++ b/components/engine/builder/dockerfile/dispatchers_test.go @@ -6,7 +6,6 @@ import ( "runtime" "testing" - "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" @@ -23,8 +22,7 @@ import ( func newBuilderWithMockBackend() *Builder { mockBackend := &MockBackend{} - defaultPlatform := platforms.DefaultSpec() - opts := &types.ImageBuildOptions{Platform: &defaultPlatform} + opts := &types.ImageBuildOptions{} ctx := context.Background() b := &Builder{ options: opts, @@ -116,7 +114,7 @@ func TestFromScratch(t *testing.T) { err := initializeStage(sb, cmd) if runtime.GOOS == "windows" && !system.LCOWSupported() { - assert.Check(t, is.Error(err, "Windows does not support FROM scratch")) + assert.Check(t, is.Error(err, "Linux containers are not supported on this system")) return } diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index 5e2c286d75..1b3a5b0f03 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -169,7 +169,7 @@ func (b *Builder) performCopy(req dispatchRequest, inst copyInstruction) error { return err } - imageMount, err := b.imageSources.Get(state.imageID, true, req.builder.options.Platform) + imageMount, err := b.imageSources.Get(state.imageID, true, req.builder.platform) if err != nil { return errors.Wrapf(err, "failed to get destination image %q", state.imageID) } @@ -416,7 +416,9 @@ func (b *Builder) probeAndCreate(dispatchState *dispatchState, runConfig *contai func (b *Builder) create(runConfig *container.Config) (string, error) { logrus.Debugf("[BUILDER] Command to be executed: %v", runConfig.Cmd) - hostConfig := hostConfigFromOptions(b.options) + + isWCOW := runtime.GOOS == "windows" && b.platform != nil && b.platform.OS == "windows" + hostConfig := hostConfigFromOptions(b.options, isWCOW) container, err := b.containerManager.Create(runConfig, hostConfig) if err != nil { return "", err @@ -429,7 +431,7 @@ func (b *Builder) create(runConfig *container.Config) (string, error) { return container.ID, nil } -func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConfig { +func hostConfigFromOptions(options *types.ImageBuildOptions, isWCOW bool) *container.HostConfig { resources := container.Resources{ CgroupParent: options.CgroupParent, CPUShares: options.CPUShares, @@ -457,7 +459,7 @@ func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConf // is too small for builder scenarios where many users are // using RUN statements to install large amounts of data. // Use 127GB as that's the default size of a VHD in Hyper-V. - if runtime.GOOS == "windows" && options.Platform != nil && options.Platform.OS == "windows" { + if isWCOW { hc.StorageOpt = make(map[string]string) hc.StorageOpt["size"] = "127GB" } diff --git a/components/engine/client/image_build.go b/components/engine/client/image_build.go index e5013176a2..9add3c10b3 100644 --- a/components/engine/client/image_build.go +++ b/components/engine/client/image_build.go @@ -8,8 +8,8 @@ import ( "net/http" "net/url" "strconv" + "strings" - "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" ) @@ -30,12 +30,6 @@ func (cli *Client) ImageBuild(ctx context.Context, buildContext io.Reader, optio } headers.Add("X-Registry-Config", base64.URLEncoding.EncodeToString(buf)) - if options.Platform != nil { - if err := cli.NewVersionError("1.32", "platform"); err != nil { - return types.ImageBuildResponse{}, err - } - query.Set("platform", platforms.Format(*options.Platform)) - } headers.Set("Content-Type", "application/x-tar") serverResp, err := cli.postRaw(ctx, "/build", query, buildContext, headers) @@ -130,8 +124,11 @@ func (cli *Client) imageBuildOptionsToQuery(options types.ImageBuildOptions) (ur if options.SessionID != "" { query.Set("session", options.SessionID) } - if options.Platform != nil { - query.Set("platform", platforms.Format(*options.Platform)) + if options.Platform != "" { + if err := cli.NewVersionError("1.32", "platform"); err != nil { + return query, err + } + query.Set("platform", strings.ToLower(options.Platform)) } if options.BuildID != "" { query.Set("buildid", options.BuildID)