Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dial-stdio command #2112

Merged
merged 1 commit into from
Feb 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions build/dial.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package build

import (
"context"
stderrors "errors"
"net"

"github.com/containerd/containerd/platforms"
"github.com/docker/buildx/builder"
"github.com/docker/buildx/util/progress"
v1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
)

func Dial(ctx context.Context, nodes []builder.Node, pw progress.Writer, platform *v1.Platform) (net.Conn, error) {
nodes, err := filterAvailableNodes(nodes)
if err != nil {
return nil, err
}

if len(nodes) == 0 {
return nil, errors.New("no nodes available")
}

var pls []v1.Platform
if platform != nil {
pls = []v1.Platform{*platform}
}

opts := map[string]Options{"default": {Platforms: pls}}
resolved, err := resolveDrivers(ctx, nodes, opts, pw)
if err != nil {
return nil, err
}

var dialError error
for _, ls := range resolved {
for _, rn := range ls {
if platform != nil {
p := *platform
var found bool
for _, pp := range rn.platforms {
if platforms.Only(p).Match(pp) {
found = true
break
}
}
if !found {
continue
}
}

conn, err := nodes[rn.driverIndex].Driver.Dial(ctx)
if err == nil {
return conn, nil
}
dialError = stderrors.Join(err)
}
}

return nil, errors.Wrap(dialError, "no nodes available")
}
132 changes: 132 additions & 0 deletions commands/dial_stdio.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package commands

import (
"io"
"net"
"os"

"github.com/containerd/containerd/platforms"
"github.com/docker/buildx/build"
"github.com/docker/buildx/builder"
"github.com/docker/buildx/util/progress"
"github.com/docker/cli/cli/command"
"github.com/moby/buildkit/util/appcontext"
"github.com/moby/buildkit/util/progress/progressui"
v1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"golang.org/x/sync/errgroup"
)

type stdioOptions struct {
builder string
platform string
progress string
}

func runDialStdio(dockerCli command.Cli, opts stdioOptions) error {
ctx := appcontext.Context()

contextPathHash, _ := os.Getwd()
b, err := builder.New(dockerCli,
builder.WithName(opts.builder),
builder.WithContextPathHash(contextPathHash),
)
if err != nil {
return err
}

if err = updateLastActivity(dockerCli, b.NodeGroup); err != nil {
return errors.Wrapf(err, "failed to update builder last activity time")
}
nodes, err := b.LoadNodes(ctx)
if err != nil {
return err
}

printer, err := progress.NewPrinter(ctx, os.Stderr, progressui.DisplayMode(opts.progress), progress.WithPhase("dial-stdio"), progress.WithDesc("builder: "+b.Name, "builder:"+b.Name))
if err != nil {
return err
}

var p *v1.Platform
if opts.platform != "" {
pp, err := platforms.Parse(opts.platform)
if err != nil {
return errors.Wrapf(err, "invalid platform %q", opts.platform)
}
p = &pp
}

defer printer.Wait()

return progress.Wrap("Proxying to builder", printer.Write, func(sub progress.SubLogger) error {
var conn net.Conn

err := sub.Wrap("Dialing builder", func() error {
conn, err = build.Dial(ctx, nodes, printer, p)
if err != nil {
return err
}
return nil
})
if err != nil {
return err
}

defer conn.Close()

go func() {
Copy link
Collaborator

@jedevc jedevc Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we extract the logic for dial-stdio in buildkit into a reusable function so we could just call that here after getting the conn? Ideally, we wouldn't end up with diverging implementations in buildctl/buildx (e.g. that would make it a pain to fix bugs in the future, etc).

We're also having the same thing in dagger, where we need dial-stdio for the connhelper, but don't really want to ship all of buildctl, so it would be cool to have just one implementation to call instead of needing to extract the relevant code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One implementation of what exactly?
the main part of this is just wiring up copy operations.
I'm not sure its worth sharing an implementation of that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I'm just confused by the differences in handling CloseRead/CloseWrite here versus in buildctl. Given that they seem to be doing the same thing, I'm not quite sure I understand why the code is so different.

Copy link
Contributor Author

@cpuguy83 cpuguy83 Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda I'm curious on this case where its wrapping with a nop-closer.https://github.com/moby/buildkit/blame/5ae9b23c40a926615b6c3cc1da3f5457d2f61fd4/cmd/buildctl/dialstdio.go#L35

I don't think we necessarily need the stream to handle a half-close here, just that it s a little cleaner in terms of the connection semantics (hence why its implemented here).

Other than that I think the implementation is pretty much the same, just this is taking advantage of errgroup and buildctl is basically doing its own hand-rolled equivalent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my memory, tty stuff didn't work correctly without handling half-close there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to test for using tty as stdin/stdout but this broke http2.
Not sure how to check for this.

Fundamentally though, the only real change as compared to buildctl would be to not fallback to Close on when CloseRead is not implemented.

<-ctx.Done()
closeWrite(conn)
}()

var eg errgroup.Group

eg.Go(func() error {
_, err := io.Copy(conn, os.Stdin)
closeWrite(conn)
return err
})
eg.Go(func() error {
_, err := io.Copy(os.Stdout, conn)
closeRead(conn)
return err
})
return eg.Wait()
})
}

func closeRead(conn net.Conn) error {
if c, ok := conn.(interface{ CloseRead() error }); ok {
return c.CloseRead()
}
return conn.Close()
}

func closeWrite(conn net.Conn) error {
if c, ok := conn.(interface{ CloseWrite() error }); ok {
return c.CloseWrite()
}
return conn.Close()
}

func dialStdioCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
opts := stdioOptions{}

cmd := &cobra.Command{
Use: "dial-stdio",
Short: "Proxy current stdio streams to builder instance",
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
opts.builder = rootOpts.builder
return runDialStdio(dockerCli, opts)
},
}

flags := cmd.Flags()
cmd.Flags()
flags.StringVar(&opts.platform, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"), "Target platform: this is used for node selection")
flags.StringVar(&opts.progress, "progress", "quiet", "Set type of progress output (auto, plain, tty).")
return cmd
}
1 change: 1 addition & 0 deletions commands/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func addCommands(cmd *cobra.Command, dockerCli command.Cli) {
buildCmd(dockerCli, opts, nil),
bakeCmd(dockerCli, opts),
createCmd(dockerCli),
dialStdioCmd(dockerCli, opts),
rmCmd(dockerCli, opts),
lsCmd(dockerCli),
useCmd(dockerCli, opts),
Expand Down
31 changes: 16 additions & 15 deletions docs/reference/buildx.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,22 @@ Extended build capabilities with BuildKit

### Subcommands

| Name | Description |
|:-------------------------------------|:---------------------------------------|
| [`bake`](buildx_bake.md) | Build from a file |
| [`build`](buildx_build.md) | Start a build |
| [`create`](buildx_create.md) | Create a new builder instance |
| [`debug`](buildx_debug.md) | Start debugger (EXPERIMENTAL) |
| [`du`](buildx_du.md) | Disk usage |
| [`imagetools`](buildx_imagetools.md) | Commands to work on images in registry |
| [`inspect`](buildx_inspect.md) | Inspect current builder instance |
| [`ls`](buildx_ls.md) | List builder instances |
| [`prune`](buildx_prune.md) | Remove build cache |
| [`rm`](buildx_rm.md) | Remove one or more builder instances |
| [`stop`](buildx_stop.md) | Stop builder instance |
| [`use`](buildx_use.md) | Set the current builder instance |
| [`version`](buildx_version.md) | Show buildx version information |
| Name | Description |
|:-------------------------------------|:------------------------------------------------|
| [`bake`](buildx_bake.md) | Build from a file |
| [`build`](buildx_build.md) | Start a build |
| [`create`](buildx_create.md) | Create a new builder instance |
| [`debug`](buildx_debug.md) | Start debugger (EXPERIMENTAL) |
| [`dial-stdio`](buildx_dial-stdio.md) | Proxy current stdio streams to builder instance |
| [`du`](buildx_du.md) | Disk usage |
| [`imagetools`](buildx_imagetools.md) | Commands to work on images in registry |
| [`inspect`](buildx_inspect.md) | Inspect current builder instance |
| [`ls`](buildx_ls.md) | List builder instances |
| [`prune`](buildx_prune.md) | Remove build cache |
| [`rm`](buildx_rm.md) | Remove one or more builder instances |
| [`stop`](buildx_stop.md) | Stop builder instance |
| [`use`](buildx_use.md) | Set the current builder instance |
| [`version`](buildx_version.md) | Show buildx version information |


### Options
Expand Down
47 changes: 47 additions & 0 deletions docs/reference/buildx_dial-stdio.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# docker buildx dial-stdio

<!---MARKER_GEN_START-->
Proxy current stdio streams to builder instance

### Options

| Name | Type | Default | Description |
|:-------------|:---------|:--------|:-------------------------------------------------|
| `--builder` | `string` | | Override the configured builder instance |
| `--platform` | `string` | | Target platform: this is used for node selection |
| `--progress` | `string` | `quiet` | Set type of progress output (auto, plain, tty). |


<!---MARKER_GEN_END-->

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a bit more description/usage here. It isn't entirely obvious for new user what "dial stdio" means.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added extra docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what changed for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔
I wrote stuff... apparently I did not commit it somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed up again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I know what happened... I did docs the wrong way.

## Description

dial-stdio uses the stdin and stdout streams of the command to proxy to the configured builder instance.
It is not intended to be used by humans, but rather by other tools that want to interact with the builder instance via BuildKit API.

## Examples

Example go program that uses the dial-stdio command wire up a buildkit client.
This is for example use only and may not be suitable for production use.

```go
client.New(ctx, "", client.WithContextDialer(func(context.Context, string) (net.Conn, error) {
c1, c2 := net.Pipe()
cmd := exec.Command("docker", "buildx", "dial-stdio")
cmd.Stdin = c1
cmd.Stdout = c1

if err := cmd.Start(); err != nil {
c1.Close()
c2.Close()
return nil, err
}

go func() {
cmd.Wait()
c2.Close()
}()

return c2
}))
```
11 changes: 9 additions & 2 deletions driver/docker-container/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,13 +384,20 @@ func (d *Driver) Rm(ctx context.Context, force, rmVolume, rmDaemon bool) error {
return nil
}

func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
func (d *Driver) Dial(ctx context.Context) (net.Conn, error) {
_, conn, err := d.exec(ctx, []string{"buildctl", "dial-stdio"})
if err != nil {
return nil, err
}

conn = demuxConn(conn)
return conn, nil
}

func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
conn, err := d.Dial(ctx)
if err != nil {
return nil, err
}

exp, _, err := detect.Exporter()
if err != nil {
Expand Down
6 changes: 5 additions & 1 deletion driver/docker/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,14 @@ func (d *Driver) Rm(ctx context.Context, force, rmVolume, rmDaemon bool) error {
return nil
}

func (d *Driver) Dial(ctx context.Context) (net.Conn, error) {
return d.DockerAPI.DialHijack(ctx, "/grpc", "h2c", d.DialMeta)
}

func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
opts := []client.ClientOpt{
client.WithContextDialer(func(context.Context, string) (net.Conn, error) {
return d.DockerAPI.DialHijack(ctx, "/grpc", "h2c", d.DialMeta)
return d.Dial(ctx)
}), client.WithSessionDialer(func(ctx context.Context, proto string, meta map[string][]string) (net.Conn, error) {
return d.DockerAPI.DialHijack(ctx, "/session", proto, meta)
}),
Expand Down
1 change: 1 addition & 0 deletions driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type Driver interface {
Version(context.Context) (string, error)
Stop(ctx context.Context, force bool) error
Rm(ctx context.Context, force, rmVolume, rmDaemon bool) error
Dial(ctx context.Context) (net.Conn, error)
Client(ctx context.Context) (*client.Client, error)
Features(ctx context.Context) map[Feature]bool
HostGatewayIP(ctx context.Context) (net.IP, error)
Expand Down
7 changes: 5 additions & 2 deletions driver/kubernetes/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func (d *Driver) Rm(ctx context.Context, force, rmVolume, rmDaemon bool) error {
return nil
}

func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
func (d *Driver) Dial(ctx context.Context) (net.Conn, error) {
restClient := d.clientset.CoreV1().RESTClient()
restClientConfig, err := d.KubeClientConfig.ClientConfig()
if err != nil {
Expand All @@ -208,15 +208,18 @@ func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
if err != nil {
return nil, err
}
return conn, nil
}

func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
exp, _, err := detect.Exporter()
if err != nil {
return nil, err
}

var opts []client.ClientOpt
opts = append(opts, client.WithContextDialer(func(context.Context, string) (net.Conn, error) {
return conn, nil
return d.Dial(ctx)
}))
if td, ok := exp.(client.TracerDelegate); ok {
opts = append(opts, client.WithTracerDelegate(td))
Expand Down
Loading