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 <[email protected]>
dagger · Jan 11, 2025 · 05064d4 · 05064d4
1 parent 3cb2e55
commit 05064d4
Show file tree

Hide file tree

Showing 9 changed files with 81 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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