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.

Sign up for GitHub

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 hostname specifying for building #1339

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Add hostname specifying for building #1339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e7ae23
Add hostname specifying for building
jingxiaolu Jun 16, 2020
059dac1
tests: Adding tests for hostname specifying for building
jingxiaolu Oct 10, 2020
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
25 changes: 25 additions & 0 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ func TestIntegration(t *testing.T) {
testSSHMount,
testStdinClosed,
testHostnameLookup,
testHostnameSpecifying,
testPushByDigest,
testBasicInlineCacheImportExport,
testExportBusyboxLocal,
Expand Down Expand Up @@ -296,6 +297,30 @@ func testHostnameLookup(t *testing.T, sb integration.Sandbox) {
require.NoError(t, err)
}

// moby/buildkit#1301
func testHostnameSpecifying(t *testing.T, sb integration.Sandbox) {
if sb.Rootless() {
t.SkipNow()
}

c, err := New(context.TODO(), sb.Address())
require.NoError(t, err)
defer c.Close()

hostname := "testtest"
st := llb.Image("busybox:latest").With(llb.Hostname(hostname)).
Run(llb.Shlexf("sh -c 'echo $HOSTNAME | grep %s'", hostname)).
Run(llb.Shlexf("sh -c 'echo $(hostname) | grep %s'", hostname))

def, err := st.Marshal(context.TODO())
require.NoError(t, err)

_, err = c.Solve(context.TODO(), def, SolveOpt{
FrontendAttrs: map[string]string{"hostname": hostname},
}, nil)
require.NoError(t, err)
}

// moby/buildkit#614
func testStdinClosed(t *testing.T, sb integration.Sandbox) {
c, err := New(context.TODO(), sb.Address())
Expand Down
14 changes: 10 additions & 4 deletions client/llb/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,17 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []
return "", nil, nil, nil, err
}

hostname, err := getHostname(e.base)(ctx)
if err != nil {
return "", nil, nil, nil, err
}

meta := &pb.Meta{
Args: args,
Env: env.ToArray(),
Cwd: cwd,
User: user,
Args: args,
Env: env.ToArray(),
Cwd: cwd,
User: user,
Hostname: hostname,
}
extraHosts, err := getExtraHosts(e.base)(ctx)
if err != nil {
Expand Down
20 changes: 20 additions & 0 deletions client/llb/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var (
keyDir = contextKeyT("llb.exec.dir")
keyEnv = contextKeyT("llb.exec.env")
keyUser = contextKeyT("llb.exec.user")
keyHostname = contextKeyT("llb.exec.hostname")
keyExtraHost = contextKeyT("llb.exec.extrahost")
keyPlatform = contextKeyT("llb.platform")
keyNetwork = contextKeyT("llb.network")
Expand Down Expand Up @@ -143,6 +144,25 @@ func getUser(s State) func(context.Context) (string, error) {
}
}

func Hostname(str string) StateOption {
return func(s State) State {
return s.WithValue(keyHostname, str)
}
}

func getHostname(s State) func(context.Context) (string, error) {
return func(ctx context.Context) (string, error) {
v, err := s.getValue(keyHostname)(ctx)
if err != nil {
return "", err
}
if v != nil {
return v.(string), nil
}
return "", nil
}
}

func args(args ...string) StateOption {
return func(s State) State {
return s.WithValue(keyArgs, args)
Expand Down
8 changes: 8 additions & 0 deletions client/llb/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,14 @@ func (s State) User(v string) State {
return User(v)(s)
}

func (s State) Hostname(v string) State {
tonistiigi marked this conversation as resolved.
Show resolved Hide resolved
return Hostname(v)(s)
}

func (s State) GetHostname(ctx context.Context) (string, error) {
return getHostname(s)(ctx)
}

func (s State) Platform(p specs.Platform) State {
return platform(p)(s)
}
Expand Down
2 changes: 1 addition & 1 deletion executor/containerdexecutor/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root cache.Moun
return err
}

hostsFile, clean, err := oci.GetHostsFile(ctx, w.root, meta.ExtraHosts, nil)
hostsFile, clean, err := oci.GetHostsFile(ctx, w.root, meta.ExtraHosts, nil, meta.Hostname)
if err != nil {
return err
}
Expand Down
1 change: 1 addition & 0 deletions executor/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ type Meta struct {
Env []string
User string
Cwd string
Hostname string
Tty bool
ReadonlyRootFS bool
ExtraHosts []HostIP
Expand Down
46 changes: 27 additions & 19 deletions executor/oci/hosts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,28 +14,26 @@ import (
"github.com/pkg/errors"
)

const hostsContent = `
127.0.0.1 localhost buildkitsandbox
::1 localhost ip6-localhost ip6-loopback
`
const defaultHostname = "buildkitsandbox"

func GetHostsFile(ctx context.Context, stateDir string, extraHosts []executor.HostIP, idmap *idtools.IdentityMapping) (string, func(), error) {
if len(extraHosts) == 0 {
_, err := g.Do(ctx, stateDir, func(ctx context.Context) (interface{}, error) {
_, _, err := makeHostsFile(stateDir, nil, idmap)
return nil, err
})
if err != nil {
return "", nil, err
}
return filepath.Join(stateDir, "hosts"), func() {}, nil
func GetHostsFile(ctx context.Context, stateDir string, extraHosts []executor.HostIP, idmap *idtools.IdentityMapping, hostname string) (string, func(), error) {
if len(extraHosts) != 0 || hostname != defaultHostname {
return makeHostsFile(stateDir, extraHosts, idmap, hostname)
}

_, err := g.Do(ctx, stateDir, func(ctx context.Context) (interface{}, error) {
_, _, err := makeHostsFile(stateDir, nil, idmap, hostname)
return nil, err
})
if err != nil {
return "", nil, err
}
return makeHostsFile(stateDir, extraHosts, idmap)
return filepath.Join(stateDir, "hosts"), func() {}, nil
}

func makeHostsFile(stateDir string, extraHosts []executor.HostIP, idmap *idtools.IdentityMapping) (string, func(), error) {
func makeHostsFile(stateDir string, extraHosts []executor.HostIP, idmap *idtools.IdentityMapping, hostname string) (string, func(), error) {
p := filepath.Join(stateDir, "hosts")
if len(extraHosts) != 0 {
if len(extraHosts) != 0 || hostname != defaultHostname {
p += "." + identity.NewID()
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to handle the case where hostname changes for different executions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry @tonistiigi what do you mean by "different executions", could you share more info? Thanks~

Copy link
Member

Choose a reason for hiding this comment

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

There is some weird caching in this function with the Stat call. The changes here seem to conflict with that when different hostname values are passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonistiigi This is hard to imagine... From cmdline input's POV, there accepts only one "hostname".
Do you mean that if there're several cmdline executions with different hostnames, here makeHostsFile() may get different hostsnames at the same time? At this case, what should we do to elimite this case? Put the whole meta to GetHostsFile()?

Copy link

Choose a reason for hiding this comment

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

@tonistiigi
Are you describing case when in Dockerfile hostname can overwritten by setting
ARG HOSTNAME in several layers?

Copy link

Choose a reason for hiding this comment

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

@AkihiroSuda @thaJeztah maybe you can help us to find the correct solution? 5 months PR stuck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonistiigi now this hostfile will be created with new one when the special hostname is specified, could you please check whether this is what you expected? Many thanks.

Copy link

Choose a reason for hiding this comment

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

@tonistiigi ping

Copy link

Choose a reason for hiding this comment

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

HI @tonistiigi
Could you check the new changes please?

Copy link

Choose a reason for hiding this comment

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

Hi @tonistiigi
I saw that you added this issue to the 0.8.0 milestone. Many thanks!
Could you please review the fix above if it's looking good?

}
_, err := os.Stat(p)
Expand All @@ -47,8 +45,7 @@ func makeHostsFile(stateDir string, extraHosts []executor.HostIP, idmap *idtools
}

b := &bytes.Buffer{}

if _, err := b.Write([]byte(hostsContent)); err != nil {
if _, err := b.Write([]byte(initHostsFile(hostname))); err != nil {
return "", nil, err
}

Expand Down Expand Up @@ -77,3 +74,14 @@ func makeHostsFile(stateDir string, extraHosts []executor.HostIP, idmap *idtools
os.RemoveAll(p)
}, nil
}

func initHostsFile(hostname string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering; is validation needed on the value? (i.e., would google.com or any other FQDN be allowed?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thaJeztah I checked the code in docker builder and buildah, but not found similar code logic, chould you please share me some hints on how to handle it? Or just leave it here when we meet with that scarios?

var hosts string
if hostname != "" {
hosts = fmt.Sprintf("127.0.0.1 localhost %s", hostname)
} else {
hosts = fmt.Sprintf("127.0.0.1 localhost %s", defaultHostname)
}
hosts = fmt.Sprintf("%s\n::1 localhost ip6-localhost ip6-loopback\n", hosts)
return hosts
}
7 changes: 6 additions & 1 deletion executor/oci/spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,17 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou
return nil, nil, err
}

hostname := defaultHostname
if meta.Hostname != "" {
hostname = meta.Hostname
}

opts = append(opts,
oci.WithProcessArgs(meta.Args...),
oci.WithEnv(meta.Env),
oci.WithProcessCwd(meta.Cwd),
oci.WithNewPrivileges,
oci.WithHostname("buildkitsandbox"),
oci.WithHostname(hostname),
)

s, err := oci.GenerateSpec(ctx, nil, c, opts...)
Expand Down
2 changes: 1 addition & 1 deletion executor/runcexecutor/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func (w *runcExecutor) Run(ctx context.Context, id string, root cache.Mountable,
return err
}

hostsFile, clean, err := oci.GetHostsFile(ctx, w.root, meta.ExtraHosts, w.idmap)
hostsFile, clean, err := oci.GetHostsFile(ctx, w.root, meta.ExtraHosts, w.idmap, meta.Hostname)
if err != nil {
return err
}
Expand Down
2 changes: 2 additions & 0 deletions frontend/dockerfile/builder/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const (
keyContextSubDir = "contextsubdir"
keyContextKeepGitDir = "build-arg:BUILDKIT_CONTEXT_KEEP_GIT_DIR"
keySyntax = "build-arg:BUILDKIT_SYNTAX"
keyHostname = "hostname"
)

var httpPrefix = regexp.MustCompile(`^https?://`)
Expand Down Expand Up @@ -395,6 +396,7 @@ func Build(ctx context.Context, c client.Client) (*client.Result, error) {
OverrideCopyImage: opts[keyOverrideCopyImage],
LLBCaps: &caps,
SourceMap: sourceMap,
Hostname: opts[keyHostname],
})

if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type ConvertOpt struct {
LLBCaps *apicaps.CapSet
ContextLocalName string
SourceMap *llb.SourceMap
Hostname string
}

func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State, *Image, error) {
Expand Down Expand Up @@ -324,6 +325,9 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State,
k, v := parseKeyValue(env)
d.state = d.state.AddEnv(k, v)
}
if opt.Hostname != "" {
d.state = d.state.Hostname(opt.Hostname)
}
if d.image.Config.WorkingDir != "" {
if err = dispatchWorkdir(d, &instructions.WorkdirCommand{Path: d.image.Config.WorkingDir}, false, nil); err != nil {
return nil, nil, parser.WithLocation(err, d.stage.Location)
Expand Down
33 changes: 33 additions & 0 deletions frontend/dockerfile/dockerfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ var allTests = []integration.Test{
testErrorsSourceMap,
testMultiArgs,
testFrontendSubrequests,
testDockefileCheckHostname,
}

var fileOpTests = []integration.Test{
Expand Down Expand Up @@ -4767,6 +4768,38 @@ COPY Dockerfile Dockerfile
require.True(t, called)
}

// moby/buildkit#1301
func testDockefileCheckHostname(t *testing.T, sb integration.Sandbox) {
f := getFrontend(t, sb)
dockerfile := []byte(`
FROM busybox
RUN cat /etc/hosts | grep testtest
Copy link
Member

Choose a reason for hiding this comment

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

also check hostname(1) and $HOSTNAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, PTAL~

RUN echo $HOSTNAME | grep testtest
RUN echo $(hostname) | grep testtest
`)

dir, err := tmpdir(
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)
require.NoError(t, err)
defer os.RemoveAll(dir)

c, err := client.New(context.TODO(), sb.Address())
require.NoError(t, err)
defer c.Close()

_, err = f.Solve(context.TODO(), c, client.SolveOpt{
FrontendAttrs: map[string]string{
"hostname": "testtest",
},
LocalDirs: map[string]string{
builder.DefaultLocalNameDockerfile: dir,
builder.DefaultLocalNameContext: dir,
},
}, nil)
require.NoError(t, err)
}

func tmpdir(appliers ...fstest.Applier) (string, error) {
tmpdir, err := ioutil.TempDir("", "buildkit-dockerfile")
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions solver/llbsolver/ops/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ func (e *execOp) Exec(ctx context.Context, g session.Group, inputs []solver.Resu
Env: e.op.Meta.Env,
Cwd: e.op.Meta.Cwd,
User: e.op.Meta.User,
Hostname: e.op.Meta.Hostname,
ReadonlyRootFS: readonlyRootFS,
ExtraHosts: extraHosts,
NetMode: e.op.Network,
Expand Down
Loading