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

remote: fix connhelpers with custom dialer #2327

Merged
merged 2 commits into from
Mar 12, 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
2 changes: 1 addition & 1 deletion builder/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func (b *Builder) LoadNodes(ctx context.Context, opts ...LoadNodesOption) (_ []N
}
}

d, err := driver.GetDriver(ctx, "buildx_buildkit_"+n.Name, factory, n.Endpoint, dockerapi, imageopt.Auth, kcc, n.BuildkitdFlags, n.Files, n.DriverOpts, n.Platforms, b.opts.contextPathHash, lno.dialMeta)
d, err := driver.GetDriver(ctx, driver.BuilderName(n.Name), factory, n.Endpoint, dockerapi, imageopt.Auth, kcc, n.BuildkitdFlags, n.Files, n.DriverOpts, n.Platforms, b.opts.contextPathHash, lno.dialMeta)
if err != nil {
node.Err = err
return nil
Expand Down
14 changes: 14 additions & 0 deletions driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"io"
"net"
"strings"

"github.com/docker/buildx/store"
"github.com/docker/buildx/util/progress"
Expand Down Expand Up @@ -67,6 +68,19 @@ type Driver interface {
Config() InitConfig
}

const builderNamePrefix = "buildx_buildkit_"

func BuilderName(name string) string {
return builderNamePrefix + name
}

func ParseBuilderName(name string) (string, error) {
if !strings.HasPrefix(name, builderNamePrefix) {
return "", errors.Errorf("invalid builder name %q, must have %q prefix", name, builderNamePrefix)
}
return strings.TrimPrefix(name, builderNamePrefix), nil
}

func Boot(ctx, clientContext context.Context, d *DriverHandle, pw progress.Writer) (*client.Client, error) {
try := 0
for {
Expand Down
6 changes: 3 additions & 3 deletions driver/kubernetes/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,10 @@ func (f *factory) AllowsInstances() bool {
// eg. "buildx_buildkit_loving_mendeleev0" -> "loving-mendeleev0"
func buildxNameToDeploymentName(bx string) (string, error) {
// TODO: commands.util.go should not pass "buildx_buildkit_" prefix to drivers
if !strings.HasPrefix(bx, "buildx_buildkit_") {
return "", errors.Errorf("expected a string with \"buildx_buildkit_\", got %q", bx)
s, err := driver.ParseBuilderName(bx)
if err != nil {
return "", err
}
s := strings.TrimPrefix(bx, "buildx_buildkit_")
s = strings.ReplaceAll(s, "_", "-")
return s, nil
}
2 changes: 1 addition & 1 deletion driver/kubernetes/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func TestFactory_processDriverOpts(t *testing.T) {
}

cfg := driver.InitConfig{
Name: "buildx_buildkit_test",
Name: driver.BuilderName("test"),
KubeClientConfig: &kcc,
}
f := factory{}
Expand Down
12 changes: 11 additions & 1 deletion driver/remote/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
util "github.com/docker/buildx/driver/remote/util"
"github.com/docker/buildx/util/progress"
"github.com/moby/buildkit/client"
"github.com/moby/buildkit/client/connhelper"
"github.com/moby/buildkit/util/tracing/detect"
"github.com/pkg/errors"
)
Expand Down Expand Up @@ -95,7 +96,16 @@ func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
}

func (d *Driver) Dial(ctx context.Context) (net.Conn, error) {
network, addr, ok := strings.Cut(d.InitConfig.EndpointAddr, "://")
addr := d.InitConfig.EndpointAddr
ch, err := connhelper.GetConnectionHelper(addr)
if err != nil {
return nil, err
}
if ch != nil {
return ch.ContextDialer(ctx, addr)
}

network, addr, ok := strings.Cut(addr, "://")
if !ok {
return nil, errors.Errorf("invalid endpoint address: %s", d.InitConfig.EndpointAddr)
}
Expand Down
51 changes: 51 additions & 0 deletions tests/create.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package tests

import (
"fmt"
"os"
"strings"
"testing"

"github.com/docker/buildx/driver"
"github.com/moby/buildkit/identity"
"github.com/moby/buildkit/util/testutil/integration"
"github.com/stretchr/testify/require"
)
Expand All @@ -18,6 +22,7 @@ func createCmd(sb integration.Sandbox, opts ...cmdOpt) (string, error) {
var createTests = []func(t *testing.T, sb integration.Sandbox){
testCreateMemoryLimit,
testCreateRestartAlways,
testCreateRemoteContainer,
}

func testCreateMemoryLimit(t *testing.T, sb integration.Sandbox) {
Expand Down Expand Up @@ -57,3 +62,49 @@ func testCreateRestartAlways(t *testing.T, sb integration.Sandbox) {
require.NoError(t, err, out)
builderName = strings.TrimSpace(out)
}

func testCreateRemoteContainer(t *testing.T, sb integration.Sandbox) {
if sb.Name() != "docker" {
t.Skip("skipping test for non-docker workers")
}

ctnBuilderName := "ctn-builder-" + identity.NewID()
remoteBuilderName := "remote-builder-" + identity.NewID()
var hasCtnBuilder, hasRemoteBuilder bool
t.Cleanup(func() {
if hasCtnBuilder {
out, err := rmCmd(sb, withArgs(ctnBuilderName))
require.NoError(t, err, out)
}
if hasRemoteBuilder {
out, err := rmCmd(sb, withArgs(remoteBuilderName))
require.NoError(t, err, out)
}
})

out, err := createCmd(sb, withArgs("--driver", "docker-container", "--name", ctnBuilderName))
require.NoError(t, err, out)
hasCtnBuilder = true

out, err = inspectCmd(sb, withArgs("--bootstrap", ctnBuilderName))
require.NoError(t, err, out)

cmd := dockerCmd(sb, withArgs("container", "inspect", fmt.Sprintf("%s0", driver.BuilderName(ctnBuilderName))))
cmd.Stderr = os.Stderr
require.NoError(t, cmd.Run())

out, err = createCmd(sb, withArgs("--driver", "remote", "--name", remoteBuilderName, fmt.Sprintf("docker-container://%s0", driver.BuilderName(ctnBuilderName))))
require.NoError(t, err, out)
hasRemoteBuilder = true

out, err = inspectCmd(sb, withArgs(remoteBuilderName))
require.NoError(t, err, out)
crazy-max marked this conversation as resolved.
Show resolved Hide resolved

for _, line := range strings.Split(out, "\n") {
if v, ok := strings.CutPrefix(line, "Status:"); ok {
require.Equal(t, strings.TrimSpace(v), "running")
return
}
}
require.Fail(t, "remote builder is not running")
}
Loading