builder: fix bridge networking when using buildkit

Signed-off-by: Tibor Vass <tibor@docker.com>

builder/builder-next/builder.go

@@ -37,6 +37,7 @@ func init() {
 type Opt struct {
 	SessionManager    *session.Manager
 	Root              string
+	NetnsRoot         string
 	Dist              images.DistributionServices
 	NetworkController libnetwork.NetworkController
 }

builder/builder-next/controller.go

@@ -90,7 +90,7 @@ func newController(rt http.RoundTripper, opt Opt) (*control.Controller, error) {
 		return nil, err
 	}
 
-	exec, err := newExecutor(root, opt.NetworkController)
+	exec, err := newExecutor(root, opt.NetnsRoot, opt.NetworkController)
 	if err != nil {
 		return nil, err
 	}

builder/builder-next/executor_unix.go

@@ -3,41 +3,46 @@
 package buildkit
 
 import (
-	"fmt"
+	"os"
 	"path/filepath"
+	"strconv"
 	"sync"
 
 	"github.com/docker/libnetwork"
 	"github.com/moby/buildkit/executor"
 	"github.com/moby/buildkit/executor/runcexecutor"
 	"github.com/moby/buildkit/identity"
+	"github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/network"
-	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 const networkName = "bridge"
 
-func newExecutor(root string, net libnetwork.NetworkController) (executor.Executor, error) {
-	// FIXME: fix bridge networking
-	_ = bridgeProvider{}
+func newExecutor(root, netnsRoot string, net libnetwork.NetworkController) (executor.Executor, error) {
+	networkProviders := map[pb.NetMode]network.Provider{
+		pb.NetMode_UNSET: &bridgeProvider{NetworkController: net, netnsRoot: netnsRoot},
+		pb.NetMode_HOST:  network.NewHostProvider(),
+		pb.NetMode_NONE:  network.NewNoneProvider(),
+	}
 	return runcexecutor.New(runcexecutor.Opt{
 		Root:              filepath.Join(root, "executor"),
 		CommandCandidates: []string{"docker-runc", "runc"},
-	}, nil)
+	}, networkProviders)
 }
 
 type bridgeProvider struct {
 	libnetwork.NetworkController
+	netnsRoot string
 }
 
-func (p *bridgeProvider) NewInterface() (network.Interface, error) {
+func (p *bridgeProvider) New() (network.Namespace, error) {
 	n, err := p.NetworkByName(networkName)
 	if err != nil {
 		return nil, err
 	}
 
-	iface := &lnInterface{ready: make(chan struct{})}
+	iface := &lnInterface{ready: make(chan struct{}), provider: p}
 	iface.Once.Do(func() {
 		go iface.init(p.NetworkController, n)
 	})
@@ -45,33 +50,13 @@ func (p *bridgeProvider) NewInterface() (network.Interface, error) {
 	return iface, nil
 }
 
-func (p *bridgeProvider) Release(iface network.Interface) error {
-	go func() {
-		if err := p.release(iface); err != nil {
-			logrus.Errorf("%s", err)
-		}
-	}()
-	return nil
-}
-
-func (p *bridgeProvider) release(iface network.Interface) error {
-	li, ok := iface.(*lnInterface)
-	if !ok {
-		return errors.Errorf("invalid interface %T", iface)
-	}
-	err := li.sbx.Delete()
-	if err1 := li.ep.Delete(true); err1 != nil && err == nil {
-		err = err1
-	}
-	return err
-}
-
 type lnInterface struct {
 	ep  libnetwork.Endpoint
 	sbx libnetwork.Sandbox
 	sync.Once
-	err   error
-	ready chan struct{}
+	err      error
+	ready    chan struct{}
+	provider *bridgeProvider
 }
 
 func (iface *lnInterface) init(c libnetwork.NetworkController, n libnetwork.Network) {
@@ -99,14 +84,26 @@ func (iface *lnInterface) init(c libnetwork.NetworkController, n libnetwork.Netw
 	iface.ep = ep
 }
 
-func (iface *lnInterface) Set(pid int) error {
+func (iface *lnInterface) Set(s *specs.Spec) {
 	<-iface.ready
 	if iface.err != nil {
-		return iface.err
+		return
+	}
+	// attach netns to bridge within the container namespace, using reexec in a prestart hook
+	s.Hooks = &specs.Hooks{
+		Prestart: []specs.Hook{{
+			Path: filepath.Join("/proc", strconv.Itoa(os.Getpid()), "exe"),
+			Args: []string{"libnetwork-setkey", iface.sbx.ContainerID(), iface.provider.NetworkController.ID()},
+		}},
 	}
-	return iface.sbx.SetKey(fmt.Sprintf("/proc/%d/ns/net", pid))
 }
 
-func (iface *lnInterface) Remove(pid int) error {
-	return nil
+func (iface *lnInterface) Close() error {
+	<-iface.ready
+	err := iface.sbx.Delete()
+	if iface.err != nil {
+		// iface.err takes precedence over cleanup errors
+		return iface.err
+	}
+	return err
 }

builder/builder-next/executor_windows.go

@@ -10,7 +10,7 @@ import (
 	"github.com/moby/buildkit/executor"
 )
 
-func newExecutor(_ string, _ libnetwork.NetworkController) (executor.Executor, error) {
+func newExecutor(_, _ string, _ libnetwork.NetworkController) (executor.Executor, error) {
 	return &winExecutor{}, nil
 }
 

cmd/dockerd/daemon.go

@@ -288,6 +288,7 @@ func newRouterOptions(config *config.Config, daemon *daemon.Daemon) (routerOptio
 	bk, err := buildkit.New(buildkit.Opt{
 		SessionManager:    sm,
 		Root:              filepath.Join(config.Root, "buildkit"),
+		NetnsRoot:         filepath.Join(config.ExecRoot, "netns"),
 		Dist:              daemon.DistributionServices(),
 		NetworkController: daemon.NetworkController(),
 	})

vendor.conf

@@ -26,7 +26,7 @@ github.com/imdario/mergo v0.3.6
 golang.org/x/sync 1d60e4601c6fd243af51cc01ddf169918a5407ca
 
 # buildkit
-github.com/moby/buildkit 49906c62925ed429ec9174a0b6869982967f1a39
+github.com/moby/buildkit e1cd06ad6b74e4b747306c4408c451b3b6d87a89
 github.com/tonistiigi/fsutil b19464cd1b6a00773b4f2eb7acf9c30426f9df42
 github.com/grpc-ecosystem/grpc-opentracing 8e809c8a86450a29b90dcc9efbf062d0fe6d9746
 github.com/opentracing/opentracing-go 1361b9cd60be79c4c3a7fa9841b3c132e40066a7

vendor/github.com/moby/buildkit/executor/oci/spec_unix.go

@@ -14,6 +14,7 @@ import (
 	"github.com/mitchellh/hashstructure"
 	"github.com/moby/buildkit/executor"
 	"github.com/moby/buildkit/snapshot"
+	"github.com/moby/buildkit/util/network"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 )
@@ -21,7 +22,7 @@ import (
 // Ideally we don't have to import whole containerd just for the default spec
 
 // GenerateSpec generates spec using containerd functionality.
-func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mount, id, resolvConf, hostsFile string, hostNetwork bool, opts ...oci.SpecOpts) (*specs.Spec, func(), error) {
+func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mount, id, resolvConf, hostsFile string, namespace network.Namespace, opts ...oci.SpecOpts) (*specs.Spec, func(), error) {
 	c := &containers.Container{
 		ID: id,
 	}
@@ -30,16 +31,15 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou
 		ctx = namespaces.WithNamespace(ctx, "buildkit")
 	}
 
-	if hostNetwork {
-		opts = append(opts, oci.WithHostNamespace(specs.NetworkNamespace))
-	}
-
 	// Note that containerd.GenerateSpec is namespaced so as to make
 	// specs.Linux.CgroupsPath namespaced
 	s, err := oci.GenerateSpec(ctx, nil, c, opts...)
 	if err != nil {
 		return nil, nil, err
 	}
+	// set the networking information on the spec
+	namespace.Set(s)
+
 	s.Process.Args = meta.Args
 	s.Process.Env = meta.Env
 	s.Process.Cwd = meta.Cwd

vendor/github.com/moby/buildkit/executor/runcexecutor/executor.go

@@ -10,7 +10,6 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
-	"sync"
 	"syscall"
 
 	"github.com/containerd/containerd/contrib/seccomp"
@@ -26,7 +25,6 @@ import (
 	"github.com/moby/buildkit/util/network"
 	rootlessspecconv "github.com/moby/buildkit/util/rootless/specconv"
 	"github.com/moby/buildkit/util/system"
-	runcsystem "github.com/opencontainers/runc/libcontainer/system"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -43,14 +41,14 @@ type Opt struct {
 var defaultCommandCandidates = []string{"buildkit-runc", "runc"}
 
 type runcExecutor struct {
-	runc            *runc.Runc
-	root            string
-	cmd             string
-	rootless        bool
-	networkProvider network.Provider
+	runc             *runc.Runc
+	root             string
+	cmd              string
+	rootless         bool
+	networkProviders map[pb.NetMode]network.Provider
 }
 
-func New(opt Opt, networkProvider network.Provider) (executor.Executor, error) {
+func New(opt Opt, networkProviders map[pb.NetMode]network.Provider) (executor.Executor, error) {
 	cmds := opt.CommandCandidates
 	if cmds == nil {
 		cmds = defaultCommandCandidates
@@ -70,10 +68,6 @@ func New(opt Opt, networkProvider network.Provider) (executor.Executor, error) {
 
 	root := opt.Root
 
-	if err := setSubReaper(); err != nil {
-		return nil, err
-	}
-
 	if err := os.MkdirAll(root, 0700); err != nil {
 		return nil, errors.Wrapf(err, "failed to create %s", root)
 	}
@@ -98,36 +92,28 @@ func New(opt Opt, networkProvider network.Provider) (executor.Executor, error) {
 	}
 
 	w := &runcExecutor{
-		runc:            runtime,
-		root:            root,
-		rootless:        opt.Rootless,
-		networkProvider: networkProvider,
+		runc:             runtime,
+		root:             root,
+		rootless:         opt.Rootless,
+		networkProviders: networkProviders,
 	}
 	return w, nil
 }
 
 func (w *runcExecutor) Exec(ctx context.Context, meta executor.Meta, root cache.Mountable, mounts []executor.Mount, stdin io.ReadCloser, stdout, stderr io.WriteCloser) error {
-	var iface network.Interface
-	// FIXME: still uses host if no provider configured
-	if meta.NetMode == pb.NetMode_UNSET {
-		if w.networkProvider != nil {
-			var err error
-			iface, err = w.networkProvider.NewInterface()
-			if err != nil || iface == nil {
-				meta.NetMode = pb.NetMode_HOST
-			}
-		} else {
-			meta.NetMode = pb.NetMode_HOST
-		}
+	provider, ok := w.networkProviders[meta.NetMode]
+	if !ok {
+		return errors.Errorf("unknown network mode %s", meta.NetMode)
+	}
+	namespace, err := provider.New()
+	if err != nil {
+		return err
 	}
+	defer namespace.Close()
+
 	if meta.NetMode == pb.NetMode_HOST {
 		logrus.Info("enabling HostNetworking")
 	}
-	defer func() {
-		if iface != nil {
-			w.networkProvider.Release(iface)
-		}
-	}()
 
 	resolvConf, err := oci.GetResolvConf(ctx, w.root)
 	if err != nil {
@@ -187,7 +173,7 @@ func (w *runcExecutor) Exec(ctx context.Context, meta executor.Meta, root cache.
 	if meta.ReadonlyRootFS {
 		opts = append(opts, containerdoci.WithRootFSReadonly())
 	}
-	spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, resolvConf, hostsFile, meta.NetMode == pb.NetMode_HOST, opts...)
+	spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, resolvConf, hostsFile, namespace, opts...)
 	if err != nil {
 		return err
 	}
@@ -225,75 +211,14 @@ func (w *runcExecutor) Exec(ctx context.Context, meta executor.Meta, root cache.
 	}
 	defer forwardIO.Close()
 
-	pidFilePath := filepath.Join(w.root, "runc_pid_"+identity.NewID())
-	defer os.RemoveAll(pidFilePath)
-
 	logrus.Debugf("> creating %s %v", id, meta.Args)
-	err = w.runc.Create(ctx, id, bundle, &runc.CreateOpts{
-		PidFile: pidFilePath,
-		IO:      forwardIO,
+	status, err := w.runc.Run(ctx, id, bundle, &runc.CreateOpts{
+		IO: forwardIO,
 	})
 	if err != nil {
 		return err
 	}
-	forwardIO.release()
 
-	defer func() {
-		go func() {
-			if err := w.runc.Delete(context.TODO(), id, &runc.DeleteOpts{}); err != nil {
-				logrus.Errorf("failed to delete %s: %+v", id, err)
-			}
-		}()
-	}()
-
-	dt, err := ioutil.ReadFile(pidFilePath)
-	if err != nil {
-		return err
-	}
-	pid, err := strconv.Atoi(string(dt))
-	if err != nil {
-		return err
-	}
-
-	done := make(chan struct{})
-	defer close(done)
-
-	go func() {
-		select {
-		case <-done:
-		case <-ctx.Done():
-			syscall.Kill(-pid, syscall.SIGKILL)
-		}
-	}()
-
-	if iface != nil {
-		if err := iface.Set(pid); err != nil {
-			return errors.Wrap(err, "could not set the network")
-		}
-		defer func() {
-			iface.Remove(pid)
-		}()
-	}
-
-	err = w.runc.Start(ctx, id)
-	if err != nil {
-		return err
-	}
-
-	p, err := os.FindProcess(pid)
-	if err != nil {
-		return err
-	}
-
-	status := 0
-	ps, err := p.Wait()
-	if err != nil {
-		status = 255
-	}
-
-	if ws, ok := ps.Sys().(syscall.WaitStatus); ok {
-		status = ws.ExitStatus()
-	}
 	if status != 0 {
 		return errors.Errorf("exit code: %d", status)
 	}
@@ -336,7 +261,7 @@ func newForwardIO(stdin io.ReadCloser, stdout, stderr io.WriteCloser) (f *forwar
 }
 
 func (s *forwardIO) Close() error {
-	s.release()
+	s.CloseAfterStart()
 	var err error
 	for _, cl := range s.toClose {
 		if err1 := cl.Close(); err == nil {
@@ -348,11 +273,12 @@ func (s *forwardIO) Close() error {
 }
 
 // release releases active FDs if the process doesn't need them any more
-func (s *forwardIO) release() {
+func (s *forwardIO) CloseAfterStart() error {
 	for _, cl := range s.toRelease {
 		cl.Close()
 	}
 	s.toRelease = nil
+	return nil
 }
 
 func (s *forwardIO) Set(cmd *exec.Cmd) {
@@ -401,16 +327,6 @@ func (s *forwardIO) writeCloserToFile(wc io.WriteCloser) (*os.File, error) {
 	return pw, nil
 }
 
-var subReaperOnce sync.Once
-var subReaperError error
-
-func setSubReaper() error {
-	subReaperOnce.Do(func() {
-		subReaperError = runcsystem.SetSubreaper(1)
-	})
-	return subReaperError
-}
-
 func (s *forwardIO) Stdin() io.WriteCloser {
 	return nil
 }

vendor/github.com/moby/buildkit/util/network/host.go

@@ -0,0 +1,28 @@
+package network
+
+import (
+	"github.com/containerd/containerd/oci"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
+)
+
+func NewHostProvider() Provider {
+	return &host{}
+}
+
+type host struct {
+}
+
+func (h *host) New() (Namespace, error) {
+	return &hostNS{}, nil
+}
+
+type hostNS struct {
+}
+
+func (h *hostNS) Set(s *specs.Spec) {
+	oci.WithHostNamespace(specs.NetworkNamespace)(nil, nil, nil, s)
+}
+
+func (h *hostNS) Close() error {
+	return nil
+}

vendor/github.com/moby/buildkit/util/network/network.go

@@ -1,17 +1,32 @@
 package network
 
+import (
+	"io"
+
+	"github.com/moby/buildkit/solver/pb"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
+)
+
+// Default returns the default network provider set
+func Default() map[pb.NetMode]Provider {
+	return map[pb.NetMode]Provider{
+		// FIXME: still uses host if no provider configured
+		pb.NetMode_UNSET: NewHostProvider(),
+		pb.NetMode_HOST:  NewHostProvider(),
+		pb.NetMode_NONE:  NewNoneProvider(),
+	}
+}
+
 // Provider interface for Network
 type Provider interface {
-	NewInterface() (Interface, error)
-	Release(Interface) error
+	New() (Namespace, error)
 }
 
-// Interface of network for workers
-type Interface interface {
-	// Set the pid with network interace namespace
-	Set(int) error
-	// Removes the network interface
-	Remove(int) error
+// Namespace of network for workers
+type Namespace interface {
+	io.Closer
+	// Set the namespace on the spec
+	Set(*specs.Spec)
 }
 
 // NetworkOpts hold network options

vendor/github.com/moby/buildkit/util/network/none.go

@@ -0,0 +1,26 @@
+package network
+
+import (
+	specs "github.com/opencontainers/runtime-spec/specs-go"
+)
+
+func NewNoneProvider() Provider {
+	return &none{}
+}
+
+type none struct {
+}
+
+func (h *none) New() (Namespace, error) {
+	return &noneNS{}, nil
+}
+
+type noneNS struct {
+}
+
+func (h *noneNS) Set(s *specs.Spec) {
+}
+
+func (h *noneNS) Close() error {
+	return nil
+}