From 05064d4291ad787c2ed512f77793a8506cc6da54 Mon Sep 17 00:00:00 2001 From: Marcos Nils <1578458+marcosnils@users.noreply.github.com> Date: Sat, 11 Jan 2025 11:54:25 -0300 Subject: [PATCH] engine: return error if services don't have a command set (#9247) * engine: return error if service images can't be started. this addresses the issue introduced in v0.15.0 where services will hang to start if neither CMD or ENTRYPOINT is used. fixes #9190 Signed-off-by: Marcos Lilljedahl --- core/container.go | 19 +++++++++- core/container_exec.go | 1 + core/integration/container_test.go | 50 ++++++++++++++++++++++++-- core/integration/git_test.go | 1 + core/integration/http_test.go | 1 + core/integration/module_test.go | 3 +- core/integration/services_test.go | 8 ++--- core/schema/container.go | 8 +++-- sdk/elixir/test/dagger/client_test.exs | 2 +- 9 files changed, 81 insertions(+), 12 deletions(-) diff --git a/core/container.go b/core/container.go index 38d2142c45..ed55c0a07b 100644 --- a/core/container.go +++ b/core/container.go @@ -100,6 +100,9 @@ type Container struct { // (Internal-only for now) Environment variables from the engine container, prefixed // with a special value, that will be inherited by this container if set. SystemEnvNames []string `json:"system_envs,omitempty"` + + // DefaultArgs have been explicitly set by the user + DefaultArgs bool `json:"defaultArgs,omitempty"` } func (*Container) Type() *ast.Type { @@ -1710,14 +1713,28 @@ func (container *Container) AsServiceLegacy(ctx context.Context) (*Service, erro } func (container *Container) AsService(ctx context.Context, args ContainerAsServiceArgs) (*Service, error) { + if len(args.Args) == 0 && + len(container.Config.Cmd) == 0 && + len(container.Config.Entrypoint) == 0 { + return nil, ErrNoSvcCommand + } + + useEntrypoint := args.UseEntrypoint + if len(container.Config.Entrypoint) > 0 && !container.DefaultArgs { + useEntrypoint = true + } + var cmdargs = container.Config.Cmd if len(args.Args) > 0 { cmdargs = args.Args + if !args.UseEntrypoint { + useEntrypoint = false + } } container, err := container.WithExec(ctx, ContainerExecOpts{ Args: cmdargs, - UseEntrypoint: args.UseEntrypoint, + UseEntrypoint: useEntrypoint, ExperimentalPrivilegedNesting: args.ExperimentalPrivilegedNesting, InsecureRootCapabilities: args.InsecureRootCapabilities, Expand: args.Expand, diff --git a/core/container_exec.go b/core/container_exec.go index c1cfbee459..4cf0b04b4c 100644 --- a/core/container_exec.go +++ b/core/container_exec.go @@ -19,6 +19,7 @@ import ( ) var ErrNoCommand = errors.New("no command has been set") +var ErrNoSvcCommand = errors.New("no service command has been set") type ContainerExecOpts struct { // Command to run instead of the container's default command diff --git a/core/integration/container_test.go b/core/integration/container_test.go index 8a646bf901..7d90d1b16a 100644 --- a/core/integration/container_test.go +++ b/core/integration/container_test.go @@ -4832,13 +4832,16 @@ func main() { From(alpineImage). WithExec([]string{"sh", "-c", "apk add curl"}) - t.Run("use default args by default", func(ctx context.Context, t *testctx.T) { + t.Run("use default args and entrypoint by default", func(ctx context.Context, t *testctx.T) { + // create new container with default values + defaultBin := c.Container().Import(binctr.AsTarball()) + output, err := curlctr. - WithServiceBinding("myapp", binctr.AsService()). + WithServiceBinding("myapp", defaultBin.AsService()). WithExec([]string{"sh", "-c", "curl -vXGET 'http://myapp:8080/hello'"}). Stdout(ctx) require.NoError(t, err) - require.Equal(t, "args: /bin/app,via-default-args", output) + require.Equal(t, "args: /bin/app,via-entrypoint,/bin/app,via-default-args", output) }) t.Run("can override default args", func(ctx context.Context, t *testctx.T) { @@ -4866,4 +4869,45 @@ func main() { require.NoError(t, err) require.Equal(t, "args: /bin/app,via-entrypoint,/bin/app,via-default-args", output) }) + + t.Run("use both args and entrypoint", func(ctx context.Context, t *testctx.T) { + withargsOverwritten := binctr. + AsService(dagger.ContainerAsServiceOpts{ + UseEntrypoint: true, + Args: []string{"/bin/app via-service-override"}, + }) + + output, err := curlctr. + WithServiceBinding("myapp", withargsOverwritten). + WithExec([]string{"sh", "-c", "curl -vXGET 'http://myapp:8080/hello'"}). + Stdout(ctx) + require.NoError(t, err) + require.Equal(t, "args: /bin/app,via-entrypoint,/bin/app via-service-override", output) + }) + + t.Run("error when no cmd and entrypoint is set", func(ctx context.Context, t *testctx.T) { + withargsOverwritten := binctr. + WithoutEntrypoint(). + WithoutDefaultArgs(). + AsService() + + _, err := curlctr. + WithServiceBinding("myapp", withargsOverwritten). + WithExec([]string{"sh", "-c", "curl -vXGET 'http://myapp:8080/hello'"}). + Stdout(ctx) + require.Error(t, err) + require.Contains(t, err.Error(), core.ErrNoSvcCommand.Error()) + }) + t.Run("default args no entrypoint", func(ctx context.Context, t *testctx.T) { + withargsOverwritten := binctr. + WithDefaultArgs([]string{"sh", "-c", "/bin/app via-override-args"}). + AsService() + + output, err := curlctr. + WithServiceBinding("myapp", withargsOverwritten). + WithExec([]string{"sh", "-c", "curl -vXGET 'http://myapp:8080/hello'"}). + Stdout(ctx) + require.NoError(t, err) + require.Equal(t, "args: /bin/app,via-override-args", output) + }) } diff --git a/core/integration/git_test.go b/core/integration/git_test.go index f20b5df21a..4a3eb94096 100644 --- a/core/integration/git_test.go +++ b/core/integration/git_test.go @@ -438,6 +438,7 @@ func (GitSuite) TestServiceStableDigest(ctx context.Context, t *testctx.T) { WithMountedDirectory("/repo", c.Git(url, dagger.GitOpts{ ExperimentalServiceHost: svc, }).Branch("main").Tree()). + WithDefaultArgs([]string{"sleep"}). AsService(). Hostname(ctx) require.NoError(t, err) diff --git a/core/integration/http_test.go b/core/integration/http_test.go index 059da287e6..b0d767e880 100644 --- a/core/integration/http_test.go +++ b/core/integration/http_test.go @@ -54,6 +54,7 @@ func (HTTPSuite) TestHTTPServiceStableDigest(ctx context.Context, t *testctx.T) WithMountedFile("/index.html", c.HTTP(url, dagger.HTTPOpts{ ExperimentalServiceHost: svc, })). + WithDefaultArgs([]string{"sleep"}). AsService(). Hostname(ctx) require.NoError(t, err) diff --git a/core/integration/module_test.go b/core/integration/module_test.go index 0ed6c9c4ae..3c62488453 100644 --- a/core/integration/module_test.go +++ b/core/integration/module_test.go @@ -3677,7 +3677,8 @@ func (m *Test) Fn(ctx context.Context) *dagger.Container { redis := dag.Container(). From("redis"). WithExposedPort(6379). - AsService() + AsService(dagger.ContainerAsServiceOpts{UseEntrypoint: true}) + cli := dag.Container(). From("redis"). WithoutEntrypoint(). diff --git a/core/integration/services_test.go b/core/integration/services_test.go index 532171d3c2..c2c1ebea28 100644 --- a/core/integration/services_test.go +++ b/core/integration/services_test.go @@ -226,7 +226,7 @@ func (m *Hoster) Run(ctx context.Context) error { WithDefaultArgs([]string{"httpd", "-v", "-f"}). WithExposedPort(80). AsService() - + hn, err := srv.Hostname(ctx) if err != nil { return err @@ -401,7 +401,7 @@ func (m *Hoster) Run(ctx context.Context) error { WithExposedPort(80). AsService(). WithHostname("wwwhatsup0") - + _, err := srv.Start(ctx) if err != nil { return err @@ -462,7 +462,7 @@ func (m *Caller) Run(ctx context.Context) error { WithExposedPort(80). AsService(). WithHostname("wwwhatsup1") - + _, err = srv.Start(ctx) if err != nil { return err @@ -505,7 +505,7 @@ func (m *Hoster) Run(ctx context.Context) error { WithExposedPort(80). AsService(). WithHostname("wwwhatsup1") - + _, err := srv.Start(ctx) if err != nil { return err diff --git a/core/schema/container.go b/core/schema/container.go index 8c9218b672..2440d7a112 100644 --- a/core/schema/container.go +++ b/core/schema/container.go @@ -1011,7 +1011,9 @@ type containerWithDefaultArgs struct { } func (s *containerSchema) withDefaultArgs(ctx context.Context, parent *core.Container, args containerWithDefaultArgs) (*core.Container, error) { - return parent.UpdateImageConfig(ctx, func(cfg specs.ImageConfig) specs.ImageConfig { + c := parent.Clone() + c.DefaultArgs = true + return c.UpdateImageConfig(ctx, func(cfg specs.ImageConfig) specs.ImageConfig { if args.Args == nil { cfg.Cmd = []string{} return cfg @@ -1023,7 +1025,9 @@ func (s *containerSchema) withDefaultArgs(ctx context.Context, parent *core.Cont } func (s *containerSchema) withoutDefaultArgs(ctx context.Context, parent *core.Container, _ struct{}) (*core.Container, error) { - return parent.UpdateImageConfig(ctx, func(cfg specs.ImageConfig) specs.ImageConfig { + c := parent.Clone() + c.DefaultArgs = false + return c.UpdateImageConfig(ctx, func(cfg specs.ImageConfig) specs.ImageConfig { cfg.Cmd = nil return cfg }) diff --git a/sdk/elixir/test/dagger/client_test.exs b/sdk/elixir/test/dagger/client_test.exs index b18f754d60..13cbed4180 100644 --- a/sdk/elixir/test/dagger/client_test.exs +++ b/sdk/elixir/test/dagger/client_test.exs @@ -283,7 +283,7 @@ defmodule Dagger.ClientTest do |> Client.container() |> Container.from("nginx:1.25-alpine3.18") |> Container.with_exposed_port(80) - |> Container.as_service() + |> Container.as_service(use_entrypoint: true) assert {:ok, out} = dag