Skip to content

Commit

Permalink
engine: return error if service images can't be started.
Browse files Browse the repository at this point in the history
this addresses the issue introduced in v0.15.0 where services will hang
to start if neither CMD or ENTRYPOINT is used.

fixes dagger#9190

Signed-off-by: Marcos Lilljedahl <[email protected]>
  • Loading branch information
marcosnils committed Jan 7, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 8c0cefb commit 7559e77
Showing 8 changed files with 47 additions and 10 deletions.
20 changes: 17 additions & 3 deletions core/container.go
Original file line number Diff line number Diff line change
@@ -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"`

// default args set via withDefaultArgs.
DefaultArgs []string `json:"defaultArgs,omitempty"`
}

func (*Container) Type() *ast.Type {
@@ -1710,13 +1713,24 @@ func (container *Container) AsServiceLegacy(ctx context.Context) (*Service, erro
}

func (container *Container) AsService(ctx context.Context, args ContainerAsServiceArgs) (*Service, error) {
var cmdargs = container.Config.Cmd
if len(args.Args) == 0 && !args.UseEntrypoint && len(container.DefaultArgs) == 0 {
return nil, ErrNoSvcCommand
}

cmdArgs := []string{}
if args.UseEntrypoint {
cmdArgs = container.Config.Cmd
}

if len(container.DefaultArgs) > 0 {
cmdArgs = container.DefaultArgs
}
if len(args.Args) > 0 {
cmdargs = args.Args
cmdArgs = args.Args
}

container, err := container.WithExec(ctx, ContainerExecOpts{
Args: cmdargs,
Args: cmdArgs,
UseEntrypoint: args.UseEntrypoint,
ExperimentalPrivilegedNesting: args.ExperimentalPrivilegedNesting,
InsecureRootCapabilities: args.InsecureRootCapabilities,
1 change: 1 addition & 0 deletions core/container_exec.go
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions core/integration/git_test.go
Original file line number Diff line number Diff line change
@@ -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{"/bin/sh"}).
AsService().
Hostname(ctx)
require.NoError(t, err)
1 change: 1 addition & 0 deletions core/integration/http_test.go
Original file line number Diff line number Diff line change
@@ -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{"/bin/sh"}).
AsService().
Hostname(ctx)
require.NoError(t, err)
3 changes: 2 additions & 1 deletion core/integration/module_test.go
Original file line number Diff line number Diff line change
@@ -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().
25 changes: 21 additions & 4 deletions core/integration/services_test.go
Original file line number Diff line number Diff line change
@@ -35,6 +35,7 @@ import (
"golang.org/x/sync/errgroup"

"dagger.io/dagger"
"github.com/dagger/dagger/core"
"github.com/dagger/dagger/internal/testutil"
"github.com/dagger/dagger/network"
"github.com/dagger/dagger/testctx"
@@ -226,7 +227,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 +402,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 +463,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 +506,7 @@ func (m *Hoster) Run(ctx context.Context) error {
WithExposedPort(80).
AsService().
WithHostname("wwwhatsup1")
_, err := srv.Start(ctx)
if err != nil {
return err
@@ -2124,6 +2125,22 @@ func (ServiceSuite) TestSearchDomainAlwaysSet(ctx context.Context, t *testctx.T)
require.True(t, found)
}

func (ServiceSuite) TestEntrypointCMDCheck(ctx context.Context, t *testctx.T) {
t.Run("return error when missing", func(ctx context.Context, t *testctx.T) {
c, err := dagger.Connect(ctx, dagger.WithLogOutput(testutil.NewTWriter(t.T)))
require.NoError(t, err)
t.Cleanup(func() { c.Close() })

s1 := c.Container().From("nginx").
AsService()

_, err = s1.Start(ctx)

require.Error(t, err)
require.Contains(t, err.Error(), core.ErrNoSvcCommand.Error())
})
}

func httpService(ctx context.Context, t *testctx.T, c *dagger.Client, content string) (*dagger.Service, string) {
t.Helper()

4 changes: 3 additions & 1 deletion core/schema/container.go
Original file line number Diff line number Diff line change
@@ -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 = args.Args
return c.UpdateImageConfig(ctx, func(cfg specs.ImageConfig) specs.ImageConfig {
if args.Args == nil {
cfg.Cmd = []string{}
return cfg
2 changes: 1 addition & 1 deletion sdk/elixir/test/dagger/client_test.exs
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 7559e77

Please sign in to comment.