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

fix(client): use grpc-go default dialer for tcp and unix connections #4127

Merged
merged 1 commit into from
Mar 18, 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
23 changes: 15 additions & 8 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func New(ctx context.Context, address string, opts ...ClientOpt) (*Client, error
}

if tracerProvider != nil {
var propagators = propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{})
propagators := propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{})
unary = append(unary, filterInterceptor(otelgrpc.UnaryClientInterceptor(otelgrpc.WithTracerProvider(tracerProvider), otelgrpc.WithPropagators(propagators)))) //nolint:staticcheck // TODO(thaJeztah): ignore SA1019 for deprecated options: see https://github.com/moby/buildkit/issues/4681
stream = append(stream, otelgrpc.StreamClientInterceptor(otelgrpc.WithTracerProvider(tracerProvider), otelgrpc.WithPropagators(propagators))) //nolint:staticcheck // TODO(thaJeztah): ignore SA1019 for deprecated options: see https://github.com/moby/buildkit/issues/4681
}
Expand All @@ -111,11 +111,17 @@ func New(ctx context.Context, address string, opts ...ClientOpt) (*Client, error
if err != nil {
return nil, err
}
gopts = append(gopts, grpc.WithContextDialer(dialFn))
if dialFn != nil {
gopts = append(gopts, grpc.WithContextDialer(dialFn))
}
}
if address == "" {
address = appdefaults.Address
}
uri, err := url.Parse(address)
if err != nil {
return nil, err
}

// Setting :authority pseudo header
// - HTTP/2 (RFC7540) defines :authority pseudo header includes
Expand All @@ -130,12 +136,14 @@ func New(ctx context.Context, address string, opts ...ClientOpt) (*Client, error
}
if authority == "" {
// authority as hostname from target address
uri, err := url.Parse(address)
if err != nil {
return nil, err
}
authority = uri.Host
}
if uri.Scheme == "tcp" {
// remove tcp scheme from address, since default dialer doesn't expect that
hown3d marked this conversation as resolved.
Show resolved Hide resolved
// name resolution is done by grpc according to the following spec: https://github.com/grpc/grpc/blob/master/doc/naming.md
address = uri.Host
}

gopts = append(gopts, grpc.WithAuthority(authority))

unary = append(unary, grpcerrors.UnaryClientInterceptor)
Expand Down Expand Up @@ -375,8 +383,7 @@ func resolveDialer(address string) (func(context.Context, string) (net.Conn, err
if ch != nil {
return ch.ContextDialer, nil
}
// basic dialer
return dialer, nil
return nil, nil
}

func filterInterceptor(intercept grpc.UnaryClientInterceptor) grpc.UnaryClientInterceptor {
Expand Down
31 changes: 20 additions & 11 deletions client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3912,7 +3912,7 @@ func testBuildExportWithUncompressed(t *testing.T, sb integration.Sandbox) {
dt, err := content.ReadBlob(ctx, img.ContentStore(), img.Target())
require.NoError(t, err)

var mfst = struct {
mfst := struct {
MediaType string `json:"mediaType,omitempty"`
ocispecs.Manifest
}{}
Expand Down Expand Up @@ -4181,6 +4181,7 @@ func testPullZstdImage(t *testing.T, sb integration.Sandbox) {
})
}
}

func testBuildPushAndValidate(t *testing.T, sb integration.Sandbox) {
workers.CheckFeatureCompat(t, sb, workers.FeatureDirectPush)
requiresLinux(t)
Expand Down Expand Up @@ -4319,7 +4320,7 @@ func testBuildPushAndValidate(t *testing.T, sb integration.Sandbox) {
dt, err = content.ReadBlob(ctx, img.ContentStore(), img.Target())
require.NoError(t, err)

var mfst = struct {
mfst := struct {
MediaType string `json:"mediaType,omitempty"`
ocispecs.Manifest
}{}
Expand Down Expand Up @@ -5397,7 +5398,8 @@ func testBasicCacheImportExport(t *testing.T, sb integration.Sandbox, cacheOptio
{
Type: ExporterLocal,
OutputDir: destDir,
}},
},
},
CacheImports: cacheOptionsEntryImport,
}, nil)
require.NoError(t, err)
Expand Down Expand Up @@ -5860,10 +5862,12 @@ func testMultipleRecordsWithSameLayersCacheImportExport(t *testing.T, sb integra

base := llb.Image("busybox:latest")
// layerA and layerB create identical layers with different LLB
layerA := base.Run(llb.Args([]string{"sh", "-c",
layerA := base.Run(llb.Args([]string{
"sh", "-c",
`echo $(( 1 + 2 )) > /result && touch -d "1970-01-01 00:00:00" /result`,
})).Root()
layerB := base.Run(llb.Args([]string{"sh", "-c",
layerB := base.Run(llb.Args([]string{
"sh", "-c",
`echo $(( 2 + 1 )) > /result && touch -d "1970-01-01 00:00:00" /result`,
})).Root()

Expand Down Expand Up @@ -9757,7 +9761,8 @@ func testMountStubsTimestamp(t *testing.T, sb integration.Sandbox) {

const sourceDateEpoch = int64(1234567890) // Fri Feb 13 11:31:30 PM UTC 2009
st := llb.Image("busybox:latest").Run(
llb.Args([]string{"/bin/touch", fmt.Sprintf("--date=@%d", sourceDateEpoch),
llb.Args([]string{
"/bin/touch", fmt.Sprintf("--date=@%d", sourceDateEpoch),
"/bin",
"/etc",
"/var",
Expand Down Expand Up @@ -9928,8 +9933,10 @@ func (*secModeInsecure) UpdateConfigFile(in string) string {
return in + "\n\ninsecure-entitlements = [\"security.insecure\"]\n"
}

var securitySandbox integration.ConfigUpdater = &secModeSandbox{}
var securityInsecure integration.ConfigUpdater = &secModeInsecure{}
var (
securitySandbox integration.ConfigUpdater = &secModeSandbox{}
securityInsecure integration.ConfigUpdater = &secModeInsecure{}
)

type netModeHost struct{}

Expand Down Expand Up @@ -9961,9 +9968,11 @@ nameservers = ["10.11.0.1"]
`
}

var hostNetwork integration.ConfigUpdater = &netModeHost{}
var defaultNetwork integration.ConfigUpdater = &netModeDefault{}
var bridgeDNSNetwork integration.ConfigUpdater = &netModeBridgeDNS{}
var (
hostNetwork integration.ConfigUpdater = &netModeHost{}
defaultNetwork integration.ConfigUpdater = &netModeDefault{}
bridgeDNSNetwork integration.ConfigUpdater = &netModeBridgeDNS{}
)

func fixedWriteCloser(wc io.WriteCloser) filesync.FileOutputFunc {
return func(map[string]string) (io.WriteCloser, error) {
Expand Down
21 changes: 0 additions & 21 deletions client/client_unix.go

This file was deleted.

25 changes: 0 additions & 25 deletions client/client_windows.go

This file was deleted.

8 changes: 8 additions & 0 deletions client/connhelper/npipe/npipe.go
Copy link
Member

@jedevc jedevc Jan 31, 2024

Choose a reason for hiding this comment

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

I think this package should now be included in cmd/buildctl/main.go since npipe is refactored to a connhelper. Otherwise, this connhelper can't actually get used.

I think this is what is causing the windows tests to fail.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I needed to refactor to support windows and non windows builds, because importing the package in main assumes that it's always build.

When building non-windows and trying to use npipe connections, it will default to an error:
https://github.com/moby/buildkit/pull/4127/files#diff-0010b4ef47621e028d85218a669df091152ba11f4bafee2566797d759dc28214R12-R14

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Package npipe provides connhelper for npipe://<address>
package npipe

import "github.com/moby/buildkit/client/connhelper"

func init() {
connhelper.Register("npipe", Helper)
}
14 changes: 14 additions & 0 deletions client/connhelper/npipe/npipe_other.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//go:build !windows

package npipe
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think the idea was that we try as much as possible to abstract the naming so that it's not platform specific (especially for Windows); that's why we'd gone with just a generic package client. This npipe package is very lean and it's Windows-specific; might be good to go this route only after we've a number of things we are doing with npipe other than just one helper.


import (
"errors"
"net/url"

"github.com/moby/buildkit/client/connhelper"
)

func Helper(u *url.URL) (*connhelper.ConnectionHelper, error) {
return nil, errors.New("npipe connections are only supported on windows")
}
28 changes: 28 additions & 0 deletions client/connhelper/npipe/npipe_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//go:build windows

package npipe

import (
"context"
"net"
"net/url"
"strings"

"github.com/Microsoft/go-winio"
"github.com/moby/buildkit/client/connhelper"
"github.com/pkg/errors"
)

// Helper returns helper for connecting to a url via npipes.
func Helper(u *url.URL) (*connhelper.ConnectionHelper, error) {
addrParts := strings.SplitN(u.String(), "://", 2)
if len(addrParts) != 2 {
return nil, errors.Errorf("invalid address %s", u)
}
address := strings.Replace(addrParts[1], "/", "\\", -1)
return &connhelper.ConnectionHelper{
ContextDialer: func(ctx context.Context, addr string) (net.Conn, error) {
return winio.DialPipeContext(ctx, address)
},
}, nil
}
1 change: 1 addition & 0 deletions cmd/buildctl/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
_ "github.com/moby/buildkit/client/connhelper/dockercontainer"
_ "github.com/moby/buildkit/client/connhelper/kubepod"
_ "github.com/moby/buildkit/client/connhelper/nerdctlcontainer"
_ "github.com/moby/buildkit/client/connhelper/npipe"
_ "github.com/moby/buildkit/client/connhelper/podmancontainer"
_ "github.com/moby/buildkit/client/connhelper/ssh"
bccommon "github.com/moby/buildkit/cmd/buildctl/common"
Expand Down
3 changes: 3 additions & 0 deletions util/testutil/integration/util_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import (
"net"

"github.com/Microsoft/go-winio"

// include npipe connhelper for windows tests
_ "github.com/moby/buildkit/client/connhelper/npipe"
)

var socketScheme = "npipe://"
Expand Down