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

remote controller: Fix entrypoint interaction bugs #1970

Merged
merged 1 commit into from
Jul 27, 2023
Merged

remote controller: Fix entrypoint interaction bugs #1970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 build/result.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ func populateProcessConfigFromResult(req *gateway.StartRequest, res *gateway.Res
} else if img != nil {
args = append(args, img.Config.Entrypoint...)
}
if cfg.Cmd != nil {
if !cfg.NoCmd {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering, would it be possible to just change this to len(cfg.Cmd) != 0 for a simpler fix?

Can a user actually specify an empty cmd? I'm not sure if this has a parallel with docker run (cc @tianon)

Copy link
Contributor

Choose a reason for hiding this comment

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

This logic was hard for me to reason about, so I'm going to give some examples of docker run behavior in the hopes that it helps make things more clear. 😅

Given an image with ENTRYPOINT set to a and CMD set to b (keeping these intentionally simple and including the obvious ones too -- let's not get into shell form vs JSON form because that gets nasty and doesn't really change this basic operation very much 😂):

  • docker run img -> runs a b
  • docker run img c -> runs a c
  • docker run --entrypoint c img -> runs c (no a, but more importantly no b also!)
  • docker run --entrypoint c img d -> c d

In other words, the behavior is similar to that of ENTRYPOINT within a Dockerfile - if it's specified, it resets CMD (so we have to detect command from the CLI separate from command from the image).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the explanation! Based on the examples, len(cfg.Cmd) != 0 doesn't look like a solution here because it doesn't implement the behaviour of docker run --entrypoint c img (--invoke entrypoint=c on buildx) that specifies an empty cmd and needs to ignore the image's default.

args = append(args, cfg.Cmd...)
} else if img != nil {
args = append(args, img.Config.Cmd...)
Expand Down
4 changes: 4 additions & 0 deletions commands/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ func (cfg *invokeConfig) needsMonitor(retErr error) bool {
func parseInvokeConfig(invoke string) (cfg invokeConfig, err error) {
cfg.invokeFlag = invoke
cfg.Tty = true
cfg.NoCmd = true
switch invoke {
case "default", "debug-shell":
return cfg, nil
Expand All @@ -706,6 +707,7 @@ func parseInvokeConfig(invoke string) (cfg invokeConfig, err error) {
// TODO: make this configurable via flags or restorable from LLB.
// Discussion: https://github.com/docker/buildx/pull/1640#discussion_r1113295900
cfg.Cmd = []string{"/bin/sh"}
cfg.NoCmd = false
return cfg, nil
}

Expand All @@ -731,10 +733,12 @@ func parseInvokeConfig(invoke string) (cfg invokeConfig, err error) {
switch key {
case "args":
cfg.Cmd = append(cfg.Cmd, maybeJSONArray(value)...)
cfg.NoCmd = false
case "entrypoint":
cfg.Entrypoint = append(cfg.Entrypoint, maybeJSONArray(value)...)
if cfg.Cmd == nil {
cfg.Cmd = []string{}
cfg.NoCmd = false
}
case "env":
cfg.Env = append(cfg.Env, maybeJSONArray(value)...)
Expand Down
239 changes: 124 additions & 115 deletions controller/pb/controller.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions controller/pb/controller.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ message InitMessage {
message InvokeConfig {
repeated string Entrypoint = 1;
repeated string Cmd = 2;
bool NoCmd = 11; // Do not set cmd but use the image's default
repeated string Env = 3;
string User = 4;
bool NoUser = 5; // Do not set user but use the image's default
Expand Down
1 change: 1 addition & 0 deletions monitor/commands/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func (cm *ExecCmd) Exec(ctx context.Context, args []string) error {
cfg := controllerapi.InvokeConfig{
Entrypoint: []string{args[1]},
Cmd: args[2:],
NoCmd: false,
// TODO: support other options as well via flags
Env: cm.invokeConfig.Env,
User: cm.invokeConfig.User,
Expand Down
1 change: 1 addition & 0 deletions monitor/commands/rollback.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func (cm *RollbackCmd) Exec(ctx context.Context, args []string) error {
if len(cmds) > 0 {
cfg.Entrypoint = []string{cmds[0]}
cfg.Cmd = cmds[1:]
cfg.NoCmd = false
}
}
id := cm.m.Rollback(ctx, cfg)
Expand Down
1 change: 1 addition & 0 deletions monitor/monitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ func (m *monitor) startInvoke(ctx context.Context, pid string, cfg controllerapi
if len(cfg.Entrypoint) == 0 && len(cfg.Cmd) == 0 {
cfg.Entrypoint = []string{"sh"} // launch shell by default
cfg.Cmd = []string{}
cfg.NoCmd = false
}
go func() {
// Start a new invoke
Expand Down