From 414edfaa7d20ed5d22876fdaa82eff212944a233 Mon Sep 17 00:00:00 2001 From: Anthony Nandaa Date: Tue, 21 Nov 2023 06:25:00 -0800 Subject: [PATCH 1/2] windows integration tests: plumbing work to be able to run on windows Co-authored-by: Gabriel Samfira This PR does most of the plumbing work requred for us to start running integration tests on Windows. We will need this to land before we can do the follow-up PRs to handle the specific tests. Signed-off-by: Anthony Nandaa --- cmd/buildkitd/main_containerd_worker.go | 2 +- cmd/buildkitd/main_unix.go | 2 + cmd/buildkitd/main_windows.go | 2 + util/testutil/integration/sandbox.go | 2 +- util/testutil/integration/util.go | 32 +----- util/testutil/integration/util_unix.go | 40 +++++++ util/testutil/integration/util_windows.go | 33 ++++++ util/testutil/workers/containerd.go | 32 +++--- util/testutil/workers/dockerd.go | 2 +- util/testutil/workers/sysprocattr_unix.go | 18 +++ util/testutil/workers/sysprocattr_windows.go | 24 +++- util/testutil/workers/util.go | 87 --------------- util/testutil/workers/util_unix.go | 111 +++++++++++++++++++ util/testutil/workers/util_windows.go | 79 +++++++++++++ worker/containerd/containerd.go | 38 ++++++- worker/containerd/containerd_test.go | 11 +- worker/containerd/containerd_test_unix.go | 15 +++ worker/containerd/containerd_test_windows.go | 8 ++ 18 files changed, 395 insertions(+), 143 deletions(-) create mode 100644 util/testutil/integration/util_unix.go create mode 100644 util/testutil/integration/util_windows.go create mode 100644 util/testutil/workers/util_unix.go create mode 100644 util/testutil/workers/util_windows.go create mode 100644 worker/containerd/containerd_test_unix.go create mode 100644 worker/containerd/containerd_test_windows.go diff --git a/cmd/buildkitd/main_containerd_worker.go b/cmd/buildkitd/main_containerd_worker.go index 161fb2bad151..f77ee95d226b 100644 --- a/cmd/buildkitd/main_containerd_worker.go +++ b/cmd/buildkitd/main_containerd_worker.go @@ -354,7 +354,7 @@ func validContainerdSocket(cfg config.ContainerdConfig) bool { // FIXME(AkihiroSuda): prohibit tcp? return true } - socketPath := strings.TrimPrefix(socket, "unix://") + socketPath := strings.TrimPrefix(socket, socketScheme) if _, err := os.Stat(socketPath); errors.Is(err, os.ErrNotExist) { // FIXME(AkihiroSuda): add more conditions bklog.L.Warnf("skipping containerd worker, as %q does not exist", socketPath) diff --git a/cmd/buildkitd/main_unix.go b/cmd/buildkitd/main_unix.go index f9373356dbc8..d819d1187f59 100644 --- a/cmd/buildkitd/main_unix.go +++ b/cmd/buildkitd/main_unix.go @@ -14,6 +14,8 @@ import ( "github.com/pkg/errors" ) +const socketScheme = "unix://" + func init() { syscall.Umask(0) } diff --git a/cmd/buildkitd/main_windows.go b/cmd/buildkitd/main_windows.go index e273760b53f8..5ca20a689534 100644 --- a/cmd/buildkitd/main_windows.go +++ b/cmd/buildkitd/main_windows.go @@ -13,6 +13,8 @@ import ( "github.com/pkg/errors" ) +const socketScheme = "npipe://" + func listenFD(addr string, tlsConfig *tls.Config) (net.Listener, error) { return nil, errors.New("listening server on fd not supported on windows") } diff --git a/util/testutil/integration/sandbox.go b/util/testutil/integration/sandbox.go index d7f1dfff2734..585a8baa189b 100644 --- a/util/testutil/integration/sandbox.go +++ b/util/testutil/integration/sandbox.go @@ -99,7 +99,7 @@ func newSandbox(ctx context.Context, w Worker, mirror string, mv matrixValue) (s b, closer, err := w.New(ctx, cfg) if err != nil { - return nil, nil, err + return nil, nil, errors.Wrap(err, "creating worker") } deferF.Append(closer) diff --git a/util/testutil/integration/util.go b/util/testutil/integration/util.go index 6a7979e4ae16..5f865dab966f 100644 --- a/util/testutil/integration/util.go +++ b/util/testutil/integration/util.go @@ -5,10 +5,8 @@ import ( "context" "fmt" "io" - "net" "os" "os/exec" - "strings" "sync" "syscall" "testing" @@ -100,31 +98,11 @@ func StartCmd(cmd *exec.Cmd, logs map[string]*bytes.Buffer) (func() error, error }, nil } -func WaitUnix(address string, d time.Duration, cmd *exec.Cmd) error { - address = strings.TrimPrefix(address, "unix://") - addr, err := net.ResolveUnixAddr("unix", address) - if err != nil { - return errors.Wrapf(err, "failed resolving unix addr: %s", address) - } - - step := 50 * time.Millisecond - i := 0 - for { - if cmd != nil && cmd.ProcessState != nil { - return errors.Errorf("process exited: %s", cmd.String()) - } - - if conn, err := net.DialUnix("unix", nil, addr); err == nil { - conn.Close() - break - } - i++ - if time.Duration(i)*step > d { - return errors.Errorf("failed dialing: %s", address) - } - time.Sleep(step) - } - return nil +// WaitSocket will dial a socket opened by a command passed in as cmd. +// On Linux this socket is typically a Unix socket, +// while on Windows this will be a named pipe. +func WaitSocket(address string, d time.Duration, cmd *exec.Cmd) error { + return waitSocket(address, d, cmd) } func LookupBinary(name string) error { diff --git a/util/testutil/integration/util_unix.go b/util/testutil/integration/util_unix.go new file mode 100644 index 000000000000..7cbc6d1e0efa --- /dev/null +++ b/util/testutil/integration/util_unix.go @@ -0,0 +1,40 @@ +//go:build !windows +// +build !windows + +package integration + +import ( + "net" + "os/exec" + "strings" + "time" + + "github.com/pkg/errors" +) + +func waitSocket(address string, d time.Duration, cmd *exec.Cmd) error { + address = strings.TrimPrefix(address, "unix://") + addr, err := net.ResolveUnixAddr("unix", address) + if err != nil { + return errors.Wrapf(err, "failed resolving unix addr: %s", address) + } + + step := 50 * time.Millisecond + i := 0 + for { + if cmd != nil && cmd.ProcessState != nil { + return errors.Errorf("process exited: %s", cmd.String()) + } + + if conn, err := net.DialUnix("unix", nil, addr); err == nil { + conn.Close() + break + } + i++ + if time.Duration(i)*step > d { + return errors.Errorf("failed dialing: %s", address) + } + time.Sleep(step) + } + return nil +} diff --git a/util/testutil/integration/util_windows.go b/util/testutil/integration/util_windows.go new file mode 100644 index 000000000000..32f235772f69 --- /dev/null +++ b/util/testutil/integration/util_windows.go @@ -0,0 +1,33 @@ +package integration + +import ( + "os/exec" + "strings" + "time" + + "github.com/Microsoft/go-winio" + "github.com/pkg/errors" +) + +func waitSocket(address string, d time.Duration, cmd *exec.Cmd) error { + address = strings.TrimPrefix(address, "npipe://") + step := 50 * time.Millisecond + i := 0 + + for { + if cmd != nil && cmd.ProcessState != nil { + return errors.Errorf("process exited: %s", cmd.String()) + } + + if conn, err := winio.DialPipe(address, nil); err == nil { + conn.Close() + break + } + i++ + if time.Duration(i)*step > d { + return errors.Errorf("failed dialing: %s", address) + } + time.Sleep(step) + } + return nil +} diff --git a/util/testutil/workers/containerd.go b/util/testutil/workers/containerd.go index 6f590e604600..d670645215e7 100644 --- a/util/testutil/workers/containerd.go +++ b/util/testutil/workers/containerd.go @@ -88,9 +88,11 @@ func (c *Containerd) New(ctx context.Context, cfg *integration.BackendConfig) (b if err := integration.LookupBinary(c.Containerd); err != nil { return nil, nil, err } + if err := integration.LookupBinary("buildkitd"); err != nil { return nil, nil, err } + if err := requireRoot(); err != nil { return nil, nil, err } @@ -106,7 +108,7 @@ func (c *Containerd) New(ctx context.Context, cfg *integration.BackendConfig) (b }() rootless := false - if c.UID != 0 { + if runtime.GOOS != "windows" && c.UID != 0 { if c.GID == 0 { return nil, nil, errors.Errorf("unsupported id pair: uid=%d, gid=%d", c.UID, c.GID) } @@ -117,6 +119,7 @@ func (c *Containerd) New(ctx context.Context, cfg *integration.BackendConfig) (b if err != nil { return nil, nil, err } + if rootless { if err := os.Chown(tmpdir, c.UID, c.GID); err != nil { return nil, nil, err @@ -125,7 +128,7 @@ func (c *Containerd) New(ctx context.Context, cfg *integration.BackendConfig) (b deferF.Append(func() error { return os.RemoveAll(tmpdir) }) - address := filepath.Join(tmpdir, "containerd.sock") + address := getContainerdSock(tmpdir) config := fmt.Sprintf(`root = %q state = %q # CRI plugins listens on 10010/tcp for stream server. @@ -137,8 +140,11 @@ disabled_plugins = ["cri"] [debug] level = "debug" - address = %q -`, filepath.Join(tmpdir, "root"), filepath.Join(tmpdir, "state"), address, filepath.Join(tmpdir, "debug.sock")) + address = %q`, + filepath.Join(tmpdir, "root"), + filepath.Join(tmpdir, "state"), + address, getContainerdDebugSock(tmpdir), + ) var snBuildkitdArgs []string if c.Snapshotter != "" { @@ -185,19 +191,13 @@ disabled_plugins = ["cri"] if err != nil { return nil, nil, err } - if err := integration.WaitUnix(address, 10*time.Second, cmd); err != nil { + if err := integration.WaitSocket(address, 10*time.Second, cmd); err != nil { ctdStop() return nil, nil, errors.Wrapf(err, "containerd did not start up: %s", integration.FormatLogs(cfg.Logs)) } deferF.Append(ctdStop) - buildkitdArgs := append([]string{"buildkitd", - "--oci-worker=false", - "--containerd-worker-gc=false", - "--containerd-worker=true", - "--containerd-worker-addr", address, - "--containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true", // Include use of --containerd-worker-labels to trigger https://github.com/moby/buildkit/pull/603 - }, snBuildkitdArgs...) + buildkitdArgs := append(getBuildkitdArgs(address), snBuildkitdArgs...) if runtime.GOOS != "windows" && c.Snapshotter != "native" { c.ExtraEnv = append(c.ExtraEnv, "BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF=true") @@ -266,9 +266,13 @@ func runStargzSnapshotter(cfg *integration.BackendConfig) (address string, cl fu if err != nil { return "", nil, err } - if err = integration.WaitUnix(address, 10*time.Second, cmd); err != nil { + if err = integration.WaitSocket(address, 10*time.Second, cmd); err != nil { snStop() - return "", nil, errors.Wrapf(err, "containerd-stargz-grpc did not start up: %s", integration.FormatLogs(cfg.Logs)) + errMsg := fmt.Sprintf( + "containerd-stargz-grpc did not start up: %s", + integration.FormatLogs(cfg.Logs), + ) + return "", nil, errors.Wrapf(err, errMsg) } deferF.Append(snStop) diff --git a/util/testutil/workers/dockerd.go b/util/testutil/workers/dockerd.go index 41622d84d8f6..8443ca9041af 100644 --- a/util/testutil/workers/dockerd.go +++ b/util/testutil/workers/dockerd.go @@ -159,7 +159,7 @@ func (c Moby) New(ctx context.Context, cfg *integration.BackendConfig) (b integr } deferF.Append(d.StopWithError) - if err := integration.WaitUnix(d.Sock(), 5*time.Second, nil); err != nil { + if err := integration.WaitSocket(d.Sock(), 5*time.Second, nil); err != nil { return nil, nil, errors.Errorf("dockerd did not start up: %q, %s", err, integration.FormatLogs(cfg.Logs)) } diff --git a/util/testutil/workers/sysprocattr_unix.go b/util/testutil/workers/sysprocattr_unix.go index 7f4db76ea5c7..406982cd02e6 100644 --- a/util/testutil/workers/sysprocattr_unix.go +++ b/util/testutil/workers/sysprocattr_unix.go @@ -21,3 +21,21 @@ func getBuildkitdAddr(tmpdir string) string { func getTraceSocketPath(tmpdir string) string { return filepath.Join(tmpdir, "otel-grpc.sock") } + +func getContainerdSock(tmpdir string) string { + return filepath.Join(tmpdir, "containerd.sock") +} + +func getContainerdDebugSock(tmpdir string) string { + return filepath.Join(tmpdir, "debug.sock") +} + +func getBuildkitdArgs(address string) []string { + return []string{"buildkitd", + "--oci-worker=false", + "--containerd-worker-gc=false", + "--containerd-worker=true", + "--containerd-worker-addr", address, + "--containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true", // Include use of --containerd-worker-labels to trigger https://github.com/moby/buildkit/pull/603 + } +} diff --git a/util/testutil/workers/sysprocattr_windows.go b/util/testutil/workers/sysprocattr_windows.go index 9d67037fbf8a..1654c48a2ddb 100644 --- a/util/testutil/workers/sysprocattr_windows.go +++ b/util/testutil/workers/sysprocattr_windows.go @@ -5,6 +5,7 @@ package workers import ( "path/filepath" + "strings" "syscall" ) @@ -13,9 +14,30 @@ func getSysProcAttr() *syscall.SysProcAttr { } func getBuildkitdAddr(tmpdir string) string { - return "//./pipe/buildkitd-" + filepath.Base(tmpdir) + return "npipe:////./pipe/buildkitd-" + filepath.Base(tmpdir) } func getTraceSocketPath(tmpdir string) string { return `\\.\pipe\buildkit-otel-grpc-` + filepath.Base(tmpdir) } + +func getContainerdSock(tmpdir string) string { + return `\\.\pipe\containerd-` + filepath.Base(tmpdir) +} + +func getContainerdDebugSock(tmpdir string) string { + return `\\.\pipe\containerd-` + filepath.Base(tmpdir) + `debug` +} + +func getBuildkitdArgs(address string) []string { + address = filepath.ToSlash(address) + if !strings.HasPrefix(address, "npipe://") { + address = "npipe://" + address + } + return []string{"buildkitd", + "--containerd-worker-gc=false", + "--containerd-worker=true", + "--containerd-worker-addr", address, + "--containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true", // Include use of --containerd-worker-labels to trigger https://github.com/moby/buildkit/pull/603 + } +} diff --git a/util/testutil/workers/util.go b/util/testutil/workers/util.go index f6861bbf32dc..c9505c1ba8e3 100644 --- a/util/testutil/workers/util.go +++ b/util/testutil/workers/util.go @@ -1,98 +1,11 @@ package workers import ( - "bufio" - "bytes" - "context" "fmt" - "os" - "os/exec" - "path/filepath" - "strings" - "time" "github.com/moby/buildkit/util/testutil/integration" - "github.com/pkg/errors" ) -func requireRoot() error { - if os.Getuid() != 0 { - return errors.Wrap(integration.ErrRequirements, "requires root") - } - return nil -} - -func runBuildkitd(ctx context.Context, conf *integration.BackendConfig, args []string, logs map[string]*bytes.Buffer, uid, gid int, extraEnv []string) (address string, cl func() error, err error) { - deferF := &integration.MultiCloser{} - cl = deferF.F() - - defer func() { - if err != nil { - deferF.F()() - cl = nil - } - }() - - tmpdir, err := os.MkdirTemp("", "bktest_buildkitd") - if err != nil { - return "", nil, err - } - if err := os.Chown(tmpdir, uid, gid); err != nil { - return "", nil, err - } - if err := os.MkdirAll(filepath.Join(tmpdir, "tmp"), 0711); err != nil { - return "", nil, err - } - if err := os.Chown(filepath.Join(tmpdir, "tmp"), uid, gid); err != nil { - return "", nil, err - } - deferF.Append(func() error { return os.RemoveAll(tmpdir) }) - - cfgfile, err := integration.WriteConfig(append(conf.DaemonConfig, withOTELSocketPath(getTraceSocketPath(tmpdir)))) - if err != nil { - return "", nil, err - } - deferF.Append(func() error { - return os.RemoveAll(filepath.Dir(cfgfile)) - }) - args = append(args, "--config="+cfgfile) - - address = getBuildkitdAddr(tmpdir) - - args = append(args, "--root", tmpdir, "--addr", address, "--debug") - cmd := exec.Command(args[0], args[1:]...) //nolint:gosec // test utility - cmd.Env = append(os.Environ(), "BUILDKIT_DEBUG_EXEC_OUTPUT=1", "BUILDKIT_DEBUG_PANIC_ON_ERROR=1", "TMPDIR="+filepath.Join(tmpdir, "tmp")) - cmd.Env = append(cmd.Env, extraEnv...) - cmd.SysProcAttr = getSysProcAttr() - - stop, err := integration.StartCmd(cmd, logs) - if err != nil { - return "", nil, err - } - deferF.Append(stop) - - if err := integration.WaitUnix(address, 15*time.Second, cmd); err != nil { - return "", nil, err - } - - deferF.Append(func() error { - f, err := os.Open("/proc/self/mountinfo") - if err != nil { - return errors.Wrap(err, "failed to open mountinfo") - } - defer f.Close() - s := bufio.NewScanner(f) - for s.Scan() { - if strings.Contains(s.Text(), tmpdir) { - return errors.Errorf("leaked mountpoint for %s", tmpdir) - } - } - return s.Err() - }) - - return address, cl, err -} - func withOTELSocketPath(socketPath string) integration.ConfigUpdater { return otelSocketPath(socketPath) } diff --git a/util/testutil/workers/util_unix.go b/util/testutil/workers/util_unix.go new file mode 100644 index 000000000000..40d6fbd0031e --- /dev/null +++ b/util/testutil/workers/util_unix.go @@ -0,0 +1,111 @@ +//go:build !windows +// +build !windows + +package workers + +import ( + "bufio" + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/moby/buildkit/util/testutil/integration" + "github.com/pkg/errors" +) + +func requireRoot() error { + if os.Getuid() != 0 { + return errors.Wrap(integration.ErrRequirements, "requires root") + } + return nil +} + +func runBuildkitd( + ctx context.Context, + conf *integration.BackendConfig, + args []string, + logs map[string]*bytes.Buffer, + uid, gid int, + extraEnv []string, +) (address string, cl func() error, err error) { + deferF := &integration.MultiCloser{} + cl = deferF.F() + + defer func() { + if err != nil { + deferF.F()() + cl = nil + } + }() + + tmpdir, err := os.MkdirTemp("", "bktest_buildkitd") + if err != nil { + return "", nil, err + } + + if err := os.Chown(tmpdir, uid, gid); err != nil { + return "", nil, err + } + + if err := os.MkdirAll(filepath.Join(tmpdir, "tmp"), 0711); err != nil { + return "", nil, err + } + + if err := os.Chown(filepath.Join(tmpdir, "tmp"), uid, gid); err != nil { + return "", nil, err + } + deferF.Append(func() error { return os.RemoveAll(tmpdir) }) + + cfgfile, err := integration.WriteConfig( + append(conf.DaemonConfig, withOTELSocketPath(getTraceSocketPath(tmpdir)))) + if err != nil { + return "", nil, err + } + deferF.Append(func() error { + return os.RemoveAll(filepath.Dir(cfgfile)) + }) + + args = append(args, "--config="+cfgfile) + address = getBuildkitdAddr(tmpdir) + + args = append(args, "--root", tmpdir, "--addr", address, "--debug") + cmd := exec.Command(args[0], args[1:]...) //nolint:gosec // test utility + cmd.Env = append( + os.Environ(), + "BUILDKIT_DEBUG_EXEC_OUTPUT=1", + "BUILDKIT_DEBUG_PANIC_ON_ERROR=1", + "TMPDIR="+filepath.Join(tmpdir, "tmp")) + cmd.Env = append(cmd.Env, extraEnv...) + cmd.SysProcAttr = getSysProcAttr() + + stop, err := integration.StartCmd(cmd, logs) + if err != nil { + return "", nil, err + } + deferF.Append(stop) + + if err := integration.WaitSocket(address, 15*time.Second, cmd); err != nil { + return "", nil, err + } + + deferF.Append(func() error { + f, err := os.Open("/proc/self/mountinfo") + if err != nil { + return errors.Wrap(err, "failed to open mountinfo") + } + defer f.Close() + s := bufio.NewScanner(f) + for s.Scan() { + if strings.Contains(s.Text(), tmpdir) { + return errors.Errorf("leaked mountpoint for %s", tmpdir) + } + } + return s.Err() + }) + + return address, cl, err +} diff --git a/util/testutil/workers/util_windows.go b/util/testutil/workers/util_windows.go new file mode 100644 index 000000000000..452f0a78bde4 --- /dev/null +++ b/util/testutil/workers/util_windows.go @@ -0,0 +1,79 @@ +package workers + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "time" + + "github.com/moby/buildkit/util/testutil/integration" +) + +func requireRoot() error { + return nil +} + +func runBuildkitd( + ctx context.Context, + conf *integration.BackendConfig, + args []string, + logs map[string]*bytes.Buffer, + uid, gid int, + extraEnv []string, +) (address string, cl func() error, err error) { + deferF := &integration.MultiCloser{} + cl = deferF.F() + + defer func() { + if err != nil { + deferF.F()() + cl = nil + } + }() + + tmpdir, err := os.MkdirTemp("", "bktest_buildkitd") + if err != nil { + return "", nil, err + } + + if err := os.MkdirAll(filepath.Join(tmpdir, "tmp"), 0711); err != nil { + return "", nil, err + } + deferF.Append(func() error { return os.RemoveAll(tmpdir) }) + + cfgfile, err := integration.WriteConfig( + append(conf.DaemonConfig, withOTELSocketPath(getTraceSocketPath(tmpdir)))) + if err != nil { + return "", nil, err + } + deferF.Append(func() error { + return os.RemoveAll(filepath.Dir(cfgfile)) + }) + + args = append(args, "--config="+cfgfile) + address = getBuildkitdAddr(tmpdir) + + args = append(args, "--root", tmpdir, "--addr", address, "--debug") + cmd := exec.Command(args[0], args[1:]...) //nolint:gosec // test utility + cmd.Env = append( + os.Environ(), + "BUILDKIT_DEBUG_EXEC_OUTPUT=1", + "BUILDKIT_DEBUG_PANIC_ON_ERROR=1", + "TMPDIR="+filepath.Join(tmpdir, "tmp")) + cmd.Env = append(cmd.Env, extraEnv...) + cmd.SysProcAttr = getSysProcAttr() + + stop, err := integration.StartCmd(cmd, logs) + if err != nil { + return "", nil, err + } + deferF.Append(stop) + + if err := integration.WaitSocket(address, 15*time.Second, cmd); err != nil { + return "", nil, err + } + + return address, cl, err +} diff --git a/worker/containerd/containerd.go b/worker/containerd/containerd.go index 7d3d28fe5728..42987b727b21 100644 --- a/worker/containerd/containerd.go +++ b/worker/containerd/containerd.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + goRuntime "runtime" "strconv" "strings" @@ -30,14 +31,47 @@ import ( type RuntimeInfo = containerdexecutor.RuntimeInfo // NewWorkerOpt creates a WorkerOpt. -func NewWorkerOpt(root string, address, snapshotterName, ns string, rootless bool, labels map[string]string, dns *oci.DNSConfig, nopt netproviders.Opt, apparmorProfile string, selinux bool, parallelismSem *semaphore.Weighted, traceSocket string, runtime *RuntimeInfo, opts ...containerd.ClientOpt) (base.WorkerOpt, error) { +func NewWorkerOpt( + root string, + address, snapshotterName, ns string, + rootless bool, + labels map[string]string, + dns *oci.DNSConfig, + nopt netproviders.Opt, + apparmorProfile string, + selinux bool, + parallelismSem *semaphore.Weighted, + traceSocket string, + runtime *RuntimeInfo, + opts ...containerd.ClientOpt, +) (base.WorkerOpt, error) { opts = append(opts, containerd.WithDefaultNamespace(ns)) + if goRuntime.GOOS == "windows" { + // TODO(profnandaa): once the upstream PR[1] is merged and + // vendored in buildkit, we will remove this block. + // [1] https://github.com/containerd/containerd/pull/9412 + address = strings.TrimPrefix(address, "npipe://") + } client, err := containerd.New(address, opts...) if err != nil { return base.WorkerOpt{}, errors.Wrapf(err, "failed to connect client to %q . make sure containerd is running", address) } - return newContainerd(root, client, snapshotterName, ns, rootless, labels, dns, nopt, apparmorProfile, selinux, parallelismSem, traceSocket, runtime) + return newContainerd( + root, + client, + snapshotterName, + ns, + rootless, + labels, + dns, + nopt, + apparmorProfile, + selinux, + parallelismSem, + traceSocket, + runtime, + ) } func newContainerd(root string, client *containerd.Client, snapshotterName, ns string, rootless bool, labels map[string]string, dns *oci.DNSConfig, nopt netproviders.Opt, apparmorProfile string, selinux bool, parallelismSem *semaphore.Weighted, traceSocket string, runtime *RuntimeInfo) (base.WorkerOpt, error) { diff --git a/worker/containerd/containerd_test.go b/worker/containerd/containerd_test.go index ceebd12d0392..82e804a4fcbc 100644 --- a/worker/containerd/containerd_test.go +++ b/worker/containerd/containerd_test.go @@ -1,11 +1,10 @@ -//go:build linux -// +build linux +//go:build !windows +// +build !windows package containerd import ( "context" - "os" "testing" "github.com/moby/buildkit/util/network/netproviders" @@ -37,12 +36,6 @@ func newWorkerOpt(t *testing.T, addr string) base.WorkerOpt { return workerOpt } -func checkRequirement(t *testing.T) { - if os.Getuid() != 0 { - t.Skip("requires root") - } -} - func testContainerdWorkerExec(t *testing.T, sb integration.Sandbox) { if sb.Rootless() { t.Skip("requires root") diff --git a/worker/containerd/containerd_test_unix.go b/worker/containerd/containerd_test_unix.go new file mode 100644 index 000000000000..0e08990c2017 --- /dev/null +++ b/worker/containerd/containerd_test_unix.go @@ -0,0 +1,15 @@ +//go:build !windows +// +build !windows + +package containerd + +import ( + "os" + "testing" +) + +func checkRequirement(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("requires root") + } +} diff --git a/worker/containerd/containerd_test_windows.go b/worker/containerd/containerd_test_windows.go new file mode 100644 index 000000000000..b3b412684e71 --- /dev/null +++ b/worker/containerd/containerd_test_windows.go @@ -0,0 +1,8 @@ +package containerd + +import "testing" + +//lint:ignore U1000 for parity with unix +func checkRequirement(t *testing.T) { + // no special requirements needed for Windows +} From 68afa16683d88e95ea6d2e1531827f258957824f Mon Sep 17 00:00:00 2001 From: Anthony Nandaa Date: Thu, 30 Nov 2023 02:11:38 -0800 Subject: [PATCH 2/2] fix/pr: fixes from pr review NOTE: all these follow-up commits will be squashed after the review is done. What changed in this commit: - refactor the code to make the dial retry logic be the same across platforms. - a number of code duplication fixes and refactors Signed-off-by: Anthony Nandaa --- util/testutil/integration/util.go | 21 +++- util/testutil/integration/util_unix.go | 33 ++---- util/testutil/integration/util_windows.go | 30 +---- util/testutil/workers/containerd.go | 20 ++-- util/testutil/workers/sysprocattr_unix.go | 41 ------- util/testutil/workers/sysprocattr_windows.go | 43 ------- util/testutil/workers/util.go | 82 +++++++++++++ util/testutil/workers/util_unix.go | 117 +++++++------------ util/testutil/workers/util_windows.go | 94 ++++++--------- 9 files changed, 203 insertions(+), 278 deletions(-) delete mode 100644 util/testutil/workers/sysprocattr_unix.go delete mode 100644 util/testutil/workers/sysprocattr_windows.go diff --git a/util/testutil/integration/util.go b/util/testutil/integration/util.go index 5f865dab966f..85b52b572ce8 100644 --- a/util/testutil/integration/util.go +++ b/util/testutil/integration/util.go @@ -7,6 +7,7 @@ import ( "io" "os" "os/exec" + "strings" "sync" "syscall" "testing" @@ -102,7 +103,25 @@ func StartCmd(cmd *exec.Cmd, logs map[string]*bytes.Buffer) (func() error, error // On Linux this socket is typically a Unix socket, // while on Windows this will be a named pipe. func WaitSocket(address string, d time.Duration, cmd *exec.Cmd) error { - return waitSocket(address, d, cmd) + address = strings.TrimPrefix(address, socketScheme) + step := 50 * time.Millisecond + i := 0 + for { + if cmd != nil && cmd.ProcessState != nil { + return errors.Errorf("process exited: %s", cmd.String()) + } + + if conn, err := dialPipe(address); err == nil { + conn.Close() + break + } + i++ + if time.Duration(i)*step > d { + return errors.Errorf("failed dialing: %s", address) + } + time.Sleep(step) + } + return nil } func LookupBinary(name string) error { diff --git a/util/testutil/integration/util_unix.go b/util/testutil/integration/util_unix.go index 7cbc6d1e0efa..f096f954779e 100644 --- a/util/testutil/integration/util_unix.go +++ b/util/testutil/integration/util_unix.go @@ -5,36 +5,19 @@ package integration import ( "net" - "os/exec" - "strings" - "time" "github.com/pkg/errors" ) -func waitSocket(address string, d time.Duration, cmd *exec.Cmd) error { - address = strings.TrimPrefix(address, "unix://") +var socketScheme = "unix://" + +// abstracted function to handle pipe dialing on unix. +// some simplification has been made to discard +// laddr for unix -- left as nil. +func dialPipe(address string) (net.Conn, error) { addr, err := net.ResolveUnixAddr("unix", address) if err != nil { - return errors.Wrapf(err, "failed resolving unix addr: %s", address) - } - - step := 50 * time.Millisecond - i := 0 - for { - if cmd != nil && cmd.ProcessState != nil { - return errors.Errorf("process exited: %s", cmd.String()) - } - - if conn, err := net.DialUnix("unix", nil, addr); err == nil { - conn.Close() - break - } - i++ - if time.Duration(i)*step > d { - return errors.Errorf("failed dialing: %s", address) - } - time.Sleep(step) + return nil, errors.Wrapf(err, "failed resolving unix addr: %s", address) } - return nil + return net.DialUnix("unix", nil, addr) } diff --git a/util/testutil/integration/util_windows.go b/util/testutil/integration/util_windows.go index 32f235772f69..eef0e37f85e8 100644 --- a/util/testutil/integration/util_windows.go +++ b/util/testutil/integration/util_windows.go @@ -1,33 +1,15 @@ package integration import ( - "os/exec" - "strings" - "time" + "net" "github.com/Microsoft/go-winio" - "github.com/pkg/errors" ) -func waitSocket(address string, d time.Duration, cmd *exec.Cmd) error { - address = strings.TrimPrefix(address, "npipe://") - step := 50 * time.Millisecond - i := 0 +var socketScheme = "npipe://" - for { - if cmd != nil && cmd.ProcessState != nil { - return errors.Errorf("process exited: %s", cmd.String()) - } - - if conn, err := winio.DialPipe(address, nil); err == nil { - conn.Close() - break - } - i++ - if time.Duration(i)*step > d { - return errors.Errorf("failed dialing: %s", address) - } - time.Sleep(step) - } - return nil +// abstracted function to handle pipe dialing on windows. +// some simplification has been made to discard timeout param. +func dialPipe(address string) (net.Conn, error) { + return winio.DialPipe(address, nil) } diff --git a/util/testutil/workers/containerd.go b/util/testutil/workers/containerd.go index d670645215e7..37501c676e6c 100644 --- a/util/testutil/workers/containerd.go +++ b/util/testutil/workers/containerd.go @@ -108,7 +108,7 @@ func (c *Containerd) New(ctx context.Context, cfg *integration.BackendConfig) (b }() rootless := false - if runtime.GOOS != "windows" && c.UID != 0 { + if c.UID != 0 { if c.GID == 0 { return nil, nil, errors.Errorf("unsupported id pair: uid=%d, gid=%d", c.UID, c.GID) } @@ -197,7 +197,17 @@ disabled_plugins = ["cri"] } deferF.Append(ctdStop) - buildkitdArgs := append(getBuildkitdArgs(address), snBuildkitdArgs...) + // handles only windows case, no effect on unix + address = normalizeAddress(address) + buildkitdArgs := []string{ + "buildkitd", + "--containerd-worker-gc=false", + "--containerd-worker=true", + "--containerd-worker-addr", address, + "--containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true", // Include use of --containerd-worker-labels to trigger https://github.com/moby/buildkit/pull/603 + } + buildkitdArgs = applyBuildkitdPlatformFlags(buildkitdArgs) + buildkitdArgs = append(buildkitdArgs, snBuildkitdArgs...) if runtime.GOOS != "windows" && c.Snapshotter != "native" { c.ExtraEnv = append(c.ExtraEnv, "BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF=true") @@ -268,11 +278,7 @@ func runStargzSnapshotter(cfg *integration.BackendConfig) (address string, cl fu } if err = integration.WaitSocket(address, 10*time.Second, cmd); err != nil { snStop() - errMsg := fmt.Sprintf( - "containerd-stargz-grpc did not start up: %s", - integration.FormatLogs(cfg.Logs), - ) - return "", nil, errors.Wrapf(err, errMsg) + return "", nil, errors.Wrapf(err, "containerd-stargz-grpc did not start up: %s", integration.FormatLogs(cfg.Logs)) } deferF.Append(snStop) diff --git a/util/testutil/workers/sysprocattr_unix.go b/util/testutil/workers/sysprocattr_unix.go deleted file mode 100644 index 406982cd02e6..000000000000 --- a/util/testutil/workers/sysprocattr_unix.go +++ /dev/null @@ -1,41 +0,0 @@ -//go:build !windows -// +build !windows - -package workers - -import ( - "path/filepath" - "syscall" -) - -func getSysProcAttr() *syscall.SysProcAttr { - return &syscall.SysProcAttr{ - Setsid: true, // stretch sudo needs this for sigterm - } -} - -func getBuildkitdAddr(tmpdir string) string { - return "unix://" + filepath.Join(tmpdir, "buildkitd.sock") -} - -func getTraceSocketPath(tmpdir string) string { - return filepath.Join(tmpdir, "otel-grpc.sock") -} - -func getContainerdSock(tmpdir string) string { - return filepath.Join(tmpdir, "containerd.sock") -} - -func getContainerdDebugSock(tmpdir string) string { - return filepath.Join(tmpdir, "debug.sock") -} - -func getBuildkitdArgs(address string) []string { - return []string{"buildkitd", - "--oci-worker=false", - "--containerd-worker-gc=false", - "--containerd-worker=true", - "--containerd-worker-addr", address, - "--containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true", // Include use of --containerd-worker-labels to trigger https://github.com/moby/buildkit/pull/603 - } -} diff --git a/util/testutil/workers/sysprocattr_windows.go b/util/testutil/workers/sysprocattr_windows.go deleted file mode 100644 index 1654c48a2ddb..000000000000 --- a/util/testutil/workers/sysprocattr_windows.go +++ /dev/null @@ -1,43 +0,0 @@ -//go:build windows -// +build windows - -package workers - -import ( - "path/filepath" - "strings" - "syscall" -) - -func getSysProcAttr() *syscall.SysProcAttr { - return &syscall.SysProcAttr{} -} - -func getBuildkitdAddr(tmpdir string) string { - return "npipe:////./pipe/buildkitd-" + filepath.Base(tmpdir) -} - -func getTraceSocketPath(tmpdir string) string { - return `\\.\pipe\buildkit-otel-grpc-` + filepath.Base(tmpdir) -} - -func getContainerdSock(tmpdir string) string { - return `\\.\pipe\containerd-` + filepath.Base(tmpdir) -} - -func getContainerdDebugSock(tmpdir string) string { - return `\\.\pipe\containerd-` + filepath.Base(tmpdir) + `debug` -} - -func getBuildkitdArgs(address string) []string { - address = filepath.ToSlash(address) - if !strings.HasPrefix(address, "npipe://") { - address = "npipe://" + address - } - return []string{"buildkitd", - "--containerd-worker-gc=false", - "--containerd-worker=true", - "--containerd-worker-addr", address, - "--containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true", // Include use of --containerd-worker-labels to trigger https://github.com/moby/buildkit/pull/603 - } -} diff --git a/util/testutil/workers/util.go b/util/testutil/workers/util.go index c9505c1ba8e3..4d9d7dccc045 100644 --- a/util/testutil/workers/util.go +++ b/util/testutil/workers/util.go @@ -1,7 +1,13 @@ package workers import ( + "bytes" + "context" "fmt" + "os" + "os/exec" + "path/filepath" + "time" "github.com/moby/buildkit/util/testutil/integration" ) @@ -19,3 +25,79 @@ func (osp otelSocketPath) UpdateConfigFile(in string) string { socketPath = %q `, in, osp) } + +func runBuildkitd( + ctx context.Context, + conf *integration.BackendConfig, + args []string, + logs map[string]*bytes.Buffer, + uid, gid int, + extraEnv []string, +) (address string, cl func() error, err error) { + deferF := &integration.MultiCloser{} + cl = deferF.F() + + defer func() { + if err != nil { + deferF.F()() + cl = nil + } + }() + + tmpdir, err := os.MkdirTemp("", "bktest_buildkitd") + if err != nil { + return "", nil, err + } + + if err := chown(tmpdir, uid, gid); err != nil { + return "", nil, err + } + + if err := os.MkdirAll(filepath.Join(tmpdir, "tmp"), 0711); err != nil { + return "", nil, err + } + + if err := chown(filepath.Join(tmpdir, "tmp"), uid, gid); err != nil { + return "", nil, err + } + deferF.Append(func() error { return os.RemoveAll(tmpdir) }) + + cfgfile, err := integration.WriteConfig( + append(conf.DaemonConfig, withOTELSocketPath(getTraceSocketPath(tmpdir)))) + if err != nil { + return "", nil, err + } + deferF.Append(func() error { + return os.RemoveAll(filepath.Dir(cfgfile)) + }) + + args = append(args, "--config="+cfgfile) + address = getBuildkitdAddr(tmpdir) + + args = append(args, "--root", tmpdir, "--addr", address, "--debug") + cmd := exec.Command(args[0], args[1:]...) //nolint:gosec // test utility + cmd.Env = append( + os.Environ(), + "BUILDKIT_DEBUG_EXEC_OUTPUT=1", + "BUILDKIT_DEBUG_PANIC_ON_ERROR=1", + "TMPDIR="+filepath.Join(tmpdir, "tmp")) + cmd.Env = append(cmd.Env, extraEnv...) + cmd.SysProcAttr = getSysProcAttr() + + stop, err := integration.StartCmd(cmd, logs) + if err != nil { + return "", nil, err + } + deferF.Append(stop) + + if err := integration.WaitSocket(address, 15*time.Second, cmd); err != nil { + return "", nil, err + } + + // separated out since it's not required in windows + deferF.Append(func() error { + return mountInfo(tmpdir) + }) + + return address, cl, err +} diff --git a/util/testutil/workers/util_unix.go b/util/testutil/workers/util_unix.go index 40d6fbd0031e..76d5c8b516b5 100644 --- a/util/testutil/workers/util_unix.go +++ b/util/testutil/workers/util_unix.go @@ -5,18 +5,19 @@ package workers import ( "bufio" - "bytes" - "context" "os" - "os/exec" "path/filepath" "strings" - "time" + "syscall" "github.com/moby/buildkit/util/testutil/integration" "github.com/pkg/errors" ) +func applyBuildkitdPlatformFlags(args []string) []string { + return append(args, "--oci-worker=false") +} + func requireRoot() error { if os.Getuid() != 0 { return errors.Wrap(integration.ErrRequirements, "requires root") @@ -24,88 +25,50 @@ func requireRoot() error { return nil } -func runBuildkitd( - ctx context.Context, - conf *integration.BackendConfig, - args []string, - logs map[string]*bytes.Buffer, - uid, gid int, - extraEnv []string, -) (address string, cl func() error, err error) { - deferF := &integration.MultiCloser{} - cl = deferF.F() - - defer func() { - if err != nil { - deferF.F()() - cl = nil - } - }() - - tmpdir, err := os.MkdirTemp("", "bktest_buildkitd") - if err != nil { - return "", nil, err - } - - if err := os.Chown(tmpdir, uid, gid); err != nil { - return "", nil, err - } - - if err := os.MkdirAll(filepath.Join(tmpdir, "tmp"), 0711); err != nil { - return "", nil, err +func getSysProcAttr() *syscall.SysProcAttr { + return &syscall.SysProcAttr{ + Setsid: true, // stretch sudo needs this for sigterm } +} - if err := os.Chown(filepath.Join(tmpdir, "tmp"), uid, gid); err != nil { - return "", nil, err - } - deferF.Append(func() error { return os.RemoveAll(tmpdir) }) +func getBuildkitdAddr(tmpdir string) string { + return "unix://" + filepath.Join(tmpdir, "buildkitd.sock") +} - cfgfile, err := integration.WriteConfig( - append(conf.DaemonConfig, withOTELSocketPath(getTraceSocketPath(tmpdir)))) - if err != nil { - return "", nil, err - } - deferF.Append(func() error { - return os.RemoveAll(filepath.Dir(cfgfile)) - }) +func getTraceSocketPath(tmpdir string) string { + return filepath.Join(tmpdir, "otel-grpc.sock") +} - args = append(args, "--config="+cfgfile) - address = getBuildkitdAddr(tmpdir) +func getContainerdSock(tmpdir string) string { + return filepath.Join(tmpdir, "containerd.sock") +} - args = append(args, "--root", tmpdir, "--addr", address, "--debug") - cmd := exec.Command(args[0], args[1:]...) //nolint:gosec // test utility - cmd.Env = append( - os.Environ(), - "BUILDKIT_DEBUG_EXEC_OUTPUT=1", - "BUILDKIT_DEBUG_PANIC_ON_ERROR=1", - "TMPDIR="+filepath.Join(tmpdir, "tmp")) - cmd.Env = append(cmd.Env, extraEnv...) - cmd.SysProcAttr = getSysProcAttr() +func getContainerdDebugSock(tmpdir string) string { + return filepath.Join(tmpdir, "debug.sock") +} - stop, err := integration.StartCmd(cmd, logs) +func mountInfo(tmpdir string) error { + f, err := os.Open("/proc/self/mountinfo") if err != nil { - return "", nil, err + return errors.Wrap(err, "failed to open mountinfo") } - deferF.Append(stop) - - if err := integration.WaitSocket(address, 15*time.Second, cmd); err != nil { - return "", nil, err + defer f.Close() + s := bufio.NewScanner(f) + for s.Scan() { + if strings.Contains(s.Text(), tmpdir) { + return errors.Errorf("leaked mountpoint for %s", tmpdir) + } } + return s.Err() +} - deferF.Append(func() error { - f, err := os.Open("/proc/self/mountinfo") - if err != nil { - return errors.Wrap(err, "failed to open mountinfo") - } - defer f.Close() - s := bufio.NewScanner(f) - for s.Scan() { - if strings.Contains(s.Text(), tmpdir) { - return errors.Errorf("leaked mountpoint for %s", tmpdir) - } - } - return s.Err() - }) +// moved here since os.Chown is not supported on Windows. +// see no-op counterpart in util_windows.go +func chown(name string, uid, gid int) error { + return os.Chown(name, uid, gid) +} - return address, cl, err +func normalizeAddress(address string) string { + // for parity with windows, no effect for unix + return address } diff --git a/util/testutil/workers/util_windows.go b/util/testutil/workers/util_windows.go index 452f0a78bde4..8d95f04ef5fa 100644 --- a/util/testutil/workers/util_windows.go +++ b/util/testutil/workers/util_windows.go @@ -1,79 +1,53 @@ package workers import ( - "bytes" - "context" - "os" - "os/exec" "path/filepath" - "time" - - "github.com/moby/buildkit/util/testutil/integration" + "strings" + "syscall" ) +func applyBuildkitdPlatformFlags(args []string) []string { + return args +} + func requireRoot() error { return nil } -func runBuildkitd( - ctx context.Context, - conf *integration.BackendConfig, - args []string, - logs map[string]*bytes.Buffer, - uid, gid int, - extraEnv []string, -) (address string, cl func() error, err error) { - deferF := &integration.MultiCloser{} - cl = deferF.F() - - defer func() { - if err != nil { - deferF.F()() - cl = nil - } - }() +func getSysProcAttr() *syscall.SysProcAttr { + return &syscall.SysProcAttr{} +} - tmpdir, err := os.MkdirTemp("", "bktest_buildkitd") - if err != nil { - return "", nil, err - } +func getBuildkitdAddr(tmpdir string) string { + return "npipe:////./pipe/buildkitd-" + filepath.Base(tmpdir) +} - if err := os.MkdirAll(filepath.Join(tmpdir, "tmp"), 0711); err != nil { - return "", nil, err - } - deferF.Append(func() error { return os.RemoveAll(tmpdir) }) +func getTraceSocketPath(tmpdir string) string { + return `\\.\pipe\buildkit-otel-grpc-` + filepath.Base(tmpdir) +} - cfgfile, err := integration.WriteConfig( - append(conf.DaemonConfig, withOTELSocketPath(getTraceSocketPath(tmpdir)))) - if err != nil { - return "", nil, err - } - deferF.Append(func() error { - return os.RemoveAll(filepath.Dir(cfgfile)) - }) +func getContainerdSock(tmpdir string) string { + return `\\.\pipe\containerd-` + filepath.Base(tmpdir) +} - args = append(args, "--config="+cfgfile) - address = getBuildkitdAddr(tmpdir) +func getContainerdDebugSock(tmpdir string) string { + return `\\.\pipe\containerd-` + filepath.Base(tmpdir) + `debug` +} - args = append(args, "--root", tmpdir, "--addr", address, "--debug") - cmd := exec.Command(args[0], args[1:]...) //nolint:gosec // test utility - cmd.Env = append( - os.Environ(), - "BUILDKIT_DEBUG_EXEC_OUTPUT=1", - "BUILDKIT_DEBUG_PANIC_ON_ERROR=1", - "TMPDIR="+filepath.Join(tmpdir, "tmp")) - cmd.Env = append(cmd.Env, extraEnv...) - cmd.SysProcAttr = getSysProcAttr() +// no-op for parity with unix +func mountInfo(tmpdir string) error { + return nil +} - stop, err := integration.StartCmd(cmd, logs) - if err != nil { - return "", nil, err - } - deferF.Append(stop) +func chown(name string, uid, gid int) error { + // Chown not supported on Windows + return nil +} - if err := integration.WaitSocket(address, 15*time.Second, cmd); err != nil { - return "", nil, err +func normalizeAddress(address string) string { + address = filepath.ToSlash(address) + if !strings.HasPrefix(address, "npipe://") { + address = "npipe://" + address } - - return address, cl, err + return address }