diff --git a/README.md b/README.md index ca9361ec..72704a94 100644 --- a/README.md +++ b/README.md @@ -298,7 +298,7 @@ These hosts are added to `no_proxy` for jobs, so they are always reached directl Gitea is not added. Add it to `no_proxy` yourself if it should be reached directly. -To change a value for one job, set it in a step's `env:` or in the job's `container.env`. Setting it at workflow or job level has no effect. To change it for the whole runner, set it in `runner.envs`. A `no_proxy` set there is added to the list above instead of replacing it. +For one step, set it in the step's `env:`, for the rest of a job, write it to `$GITHUB_ENV`. Setting it at workflow or job level or in `container.env` has no effect. To change it for the whole runner, set it in `runner.envs`. A `no_proxy` set there is added to the list above instead of replacing it. Images are pulled by the Docker daemon, which needs its own proxy setting. In the `dind` images the daemon runs in the same container and reads the variables above. For any other daemon, see [the Docker documentation](https://docs.docker.com/engine/daemon/proxy/). The runner logs a warning at startup if it has a proxy and the daemon does not. diff --git a/act/container/docker_run.go b/act/container/docker_run.go index 0301119f..c6c70c4b 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -593,7 +593,8 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config // If the `privileged` config has been disabled, `copts.privileged` need to be forced to false, // even if the user specifies `--privileged` in the options string. - if !hostConfig.Privileged { + if !hostConfig.Privileged && copts.privileged { + logger.Warnf("container option %q is not allowed when privileged mode is disabled and will be ignored", "--privileged") copts.privileged = false } @@ -629,6 +630,7 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config logger.Debugf("Custom container.HostConfig from options ==> %+v", containerConfig.HostConfig) + cr.sanitizeConfig(ctx, nil, containerConfig.HostConfig) // a dropped volume must not displace the runner's own overlayVolumes(hostConfig, containerConfig.HostConfig) binds := hostConfig.Binds mounts := hostConfig.Mounts @@ -1261,6 +1263,12 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai return config, hostConfig } +func SanitizeBinds(ctx context.Context, validVolumes, binds []string) []string { + cr := &containerReference{input: &NewContainerInput{ValidVolumes: validVolumes}} + _, hostConfig := cr.sanitizeConfig(ctx, nil, &container.HostConfig{Binds: binds}) + return hostConfig.Binds +} + // bindTarget returns the container path a bind mounts onto, empty if it cannot be parsed. func bindTarget(bind string) string { parsed, err := loader.ParseVolume(bind) diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index cd70d413..c7aeb751 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -682,6 +682,7 @@ func mergeOptions(t *testing.T, runnerOptions, workflowOptions string, privilege WorkflowOptions: workflowOptions, NetworkMode: "bridge", UsernsMode: "private", + ValidVolumes: []string{"**"}, }} _, hostConfig, err := cr.mergeContainerConfigs(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{ @@ -844,8 +845,10 @@ func TestMergeContainerConfigsVolumesReplaceRunnerMounts(t *testing.T) { ctx := common.WithLogger(context.Background(), logger) cr := &containerReference{ input: &NewContainerInput{ - NetworkMode: "bridge", - RunnerOptions: "--volume /host/tools:/opt/hostedtoolcache", + NetworkMode: "bridge", + RunnerOptions: "--volume /host/tools:/opt/hostedtoolcache", + WorkflowOptions: "--volume /unapproved:/var/run/docker.sock", + ValidVolumes: []string{"/host/tools"}, }, } @@ -867,6 +870,7 @@ func TestMergeContainerConfigsWarnsOnlyAboutOptionsThatWereGiven(t *testing.T) { assert.Zero(t, warnings("--volume /host/tools:/opt/hostedtoolcache", "")) assert.Zero(t, warnings("", "--shm-size 1g")) assert.Equal(t, 1, warnings("--network host", "")) + assert.Equal(t, 1, warnings("", "--privileged")) } func TestMergeContainerConfigsKeepsNetworkAliasesFromOptions(t *testing.T) { diff --git a/act/container/docker_stub.go b/act/container/docker_stub.go index 915c6740..e56b6418 100644 --- a/act/container/docker_stub.go +++ b/act/container/docker_stub.go @@ -107,6 +107,10 @@ func RemoveOrphanNetworks(_ context.Context, _, _ string, _ time.Time) error { return nil } +func SanitizeBinds(_ context.Context, _, binds []string) []string { + return binds +} + func CreateJobVolumes(ctx context.Context, runnerUUID string, volumeNames []string) error { return nil } diff --git a/act/runner/action.go b/act/runner/action.go index c31952fb..6929d18c 100644 --- a/act/runner/action.go +++ b/act/runner/action.go @@ -447,7 +447,7 @@ func newStepContainer(ctx context.Context, step step, image string, cmd, entrypo envList = append(envList, rc.runnerEnv(ctx)...) - binds, mounts := rc.GetBindsAndMounts() + binds, mounts := rc.GetBindsAndMounts(ctx) networkMode := "container:" + rc.jobContainerName() if rc.IsHostEnv() { networkMode = "default" diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 2b6e0f6c..3c7cdfed 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -64,7 +64,7 @@ type RunContext struct { ExprEval *expressionEvaluator JobContainer container.ExecutionsEnvironment serviceContainers []*serviceContainer - containerSpec model.ContainerSpec // container:, resolved once with platformImage, zero without a job container + containerSpec model.ContainerSpec // container:, resolved once with platformImage, zero without a container image JobName string ActionPath string Parent *RunContext @@ -113,6 +113,7 @@ type serviceContainer struct { name string image string container container.ExecutionsEnvironment + created bool logsDumped bool info *container.Info // last poll, the source of the `job.services` entry } @@ -334,12 +335,12 @@ func splitVolumes(specs []string) ([]string, map[string]string, map[string]bool) } // Returns the binds and mounts for the container, resolving paths as appopriate -func (rc *RunContext) GetBindsAndMounts() ([]string, map[string]string) { +func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[string]string) { name := rc.jobContainerName() ext := container.LinuxContainerEnvironmentExtensions{} // the runner's own mounts below yield to the targets the job claims - binds, mounts, claimed := splitVolumes(rc.containerSpec.Volumes) + binds, mounts, claimed := splitVolumes(container.SanitizeBinds(ctx, rc.validVolumes(), rc.containerSpec.Volumes)) if rc.containerDaemonSocket() != "-" && !claimed["/var/run/docker.sock"] { binds = append(binds, rc.jobDockerSocket()+":/var/run/docker.sock") @@ -497,10 +498,12 @@ func (rc *RunContext) startJobContainer() common.Executor { if err := model.DecodeEvaluated("job services", rc.Run.Job().RawServices, rc.ExprEval.shared(ctx).EvaluateYamlNode, &services); err != nil { return err } - for serviceID, spec := range services { + for _, serviceID := range slices.Sorted(maps0.Keys(services)) { + spec := services[serviceID] if spec == nil { return fmt.Errorf("service %s has no container definition", serviceID) } + spec.Image = strings.TrimPrefix(spec.Image, "docker://") // GitHub compatibility: skip services whose image evaluates to an // empty string, enabling conditional services via expressions if spec.Image == "" { @@ -537,6 +540,7 @@ func (rc *RunContext) startJobContainer() common.Executor { } serviceContainerName := createContainerName(rc.jobContainerName(), serviceID) + serviceLog := common.NewLineWriter(rawLogLine(ctx)) // raw, so a service's output cannot set the job's env, outputs or state c := newContainer(&container.NewContainerInput{ Name: serviceContainerName, Image: spec.Image, @@ -547,8 +551,8 @@ func (rc *RunContext) startJobContainer() common.Executor { Env: envs, Mounts: serviceMounts, Binds: serviceBinds, - Stdout: logWriter, - Stderr: logWriter, + Stdout: serviceLog, + Stderr: serviceLog, Privileged: rc.Config.Privileged, UsernsMode: rc.Config.UsernsMode, Platform: rc.Config.ContainerArchitecture, @@ -602,7 +606,7 @@ func (rc *RunContext) startJobContainer() common.Executor { return err } rc.startDockerProxy(ctx) - containerInput.Binds, containerInput.Mounts = rc.GetBindsAndMounts() + containerInput.Binds, containerInput.Mounts = rc.GetBindsAndMounts(ctx) containerInput.ValidVolumes = rc.validVolumes() rc.jobNetworkName = networkName @@ -633,16 +637,21 @@ func (rc *RunContext) startJobContainer() common.Executor { } func (rc *RunContext) commandLogWriter(ctx context.Context) io.Writer { + return common.NewLineWriter(rc.commandHandler(ctx), rawLogLine(ctx)) +} + +func rawLogLine(ctx context.Context) common.LineHandler { rawLogger := common.Logger(ctx).WithField(rawOutputField, true) - return common.NewLineWriter(rc.commandHandler(ctx), func(line string) bool { + return func(line string) bool { rawLogger.Infof("%s", line) return true - }) + } } func (rc *RunContext) cleanupJobResources(networkName string, createAndDeleteNetwork, preclean bool) common.Executor { return func(ctx context.Context) error { logger := common.Logger(ctx) + rc.printServiceLogs(ctx) errs := []error{rc.closeDockerProxy(ctx)} if rc.JobContainer != nil { errs = append(errs, rc.JobContainer.Remove()(ctx)) @@ -789,6 +798,7 @@ func (rc *RunContext) startServiceContainers() common.Executor { for _, svc := range rc.serviceContainers { execs = append(execs, common.NewPipelineExecutor( svc.container.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop), + func(context.Context) error { svc.created = true; return nil }, svc.container.Start(false), )) } @@ -927,6 +937,26 @@ func (svc *serviceContainer) dumpLogs(ctx context.Context) { } } +const serviceLogsTimeout = 15 * time.Second // cleanup gets one minute in total + +func (rc *RunContext) printServiceLogs(ctx context.Context) { + ctx, cancel := context.WithTimeout(ctx, serviceLogsTimeout) + defer cancel() + rawLogger := common.Logger(ctx).WithField(rawOutputField, true) + for _, svc := range rc.serviceContainers { + if !svc.created || svc.logsDumped { + continue + } + rawLogger.Infof("::group::Print service container logs: %s", EscapeCommandData(svc.name)) + svc.dumpLogs(ctx) + rawLogger.Infof("::endgroup::") + if ctx.Err() != nil { + common.Logger(ctx).Warnf("Timed out printing the service logs after %s", serviceLogsTimeout) + return + } + } +} + // inspect also records the state for the `job.services` context. func (svc *serviceContainer) inspect(ctx context.Context) (*container.Info, error) { info, err := svc.container.Inspect(ctx) @@ -1039,7 +1069,7 @@ func (rc *RunContext) cleanupFailedStart(ctx context.Context) { } func (rc *RunContext) IsHostEnv() bool { - return strings.EqualFold(rc.platformImage, "-self-hosted") + return rc.containerSpec.Image == "" && strings.EqualFold(rc.platformImage, "-self-hosted") } func (rc *RunContext) stopContainer() common.Executor { @@ -1162,12 +1192,13 @@ func (rc *RunContext) runsOnPlatformNames(ctx context.Context) []string { // resolvePlatformImage evaluates the job's container once for every consumer, ignoring one without an image as GitHub does. func (rc *RunContext) resolvePlatformImage(ctx context.Context) error { withoutEnv, _ := splitContainerEnv(rc.Run.Job().RawContainer) - var spec *model.ContainerSpec + var spec model.ContainerSpec if err := model.DecodeEvaluated("container", withoutEnv, rc.ExprEval.shared(ctx).EvaluateYamlNode, &spec); err != nil { return err } - if spec != nil && spec.Image != "" { - rc.containerSpec, rc.platformImage = *spec, spec.Image + spec.Image = strings.TrimPrefix(spec.Image, "docker://") + if spec.Image != "" { + rc.containerSpec, rc.platformImage = spec, spec.Image return nil } image, err := rc.runsOnImage(ctx) diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 405ad29a..a5b482db 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -32,6 +32,7 @@ import ( "github.com/docker/cli/cli/compose/loader" "github.com/moby/moby/api/types/volume" log "github.com/sirupsen/logrus" + logrustest "github.com/sirupsen/logrus/hooks/test" assert "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" require "github.com/stretchr/testify/require" @@ -342,12 +343,12 @@ jobs: require.Nil(t, redis.Cmd) } -func TestStartJobContainerEvaluatesContainersOnce(t *testing.T) { +func TestStartJobContainerEvaluatesContainersOnceAndTrimsDockerPrefix(t *testing.T) { inputs := startJobContainerInputs(t, ` jobs: job: - container: ${{ fromJSON('{"image":"node:20","options":"--label ${{ github.job }}","credentials":{"username":"user","password":"${{ vars.TITLE }}"}}') }} - services: ${{ fromJSON('{"redis":{"image":"redis:latest","volumes":["data:/data"],"env":{"TITLE":"${{ github.job }}"},"credentials":{"username":"user","password":"${{ github.job }}"},"entrypoint":"/entry.sh","command":"redis-server --port 6380"}}') }} + container: ${{ fromJSON('{"image":"docker://node:20","options":"--label ${{ github.job }}","credentials":{"username":"user","password":"${{ vars.TITLE }}"}}') }} + services: ${{ fromJSON('{"redis":{"image":"docker://redis:latest","volumes":["data:/data"],"env":{"TITLE":"${{ github.job }}"},"credentials":{"username":"user","password":"${{ github.job }}"},"entrypoint":"/entry.sh","command":"redis-server --port 6380"}}') }} `, &Config{}) redis, job := inputs[0], inputs[1] @@ -412,17 +413,14 @@ jobs: steps: [] `, &Config{ProxyEnv: map[string]string{"http_proxy": "http://proxy:3128", "no_proxy": "internal.example"}}) - env := map[string][]string{} - for _, in := range inputs { - env[in.Image] = in.Env - } + db, redis := inputs[0], inputs[1] - require.Contains(t, env["redis:latest"], "http_proxy=http://proxy:3128") - require.Contains(t, env["redis:latest"], "no_proxy=internal.example") + require.Contains(t, redis.Env, "http_proxy=http://proxy:3128") + require.Contains(t, redis.Env, "no_proxy=internal.example") // the service's own env wins over what the runner injected, without dropping the rest - require.Contains(t, env["postgres:latest"], "no_proxy=db-only.example") - require.NotContains(t, env["postgres:latest"], "no_proxy=internal.example") - require.Contains(t, env["postgres:latest"], "http_proxy=http://proxy:3128") + require.Contains(t, db.Env, "no_proxy=db-only.example") + require.NotContains(t, db.Env, "no_proxy=internal.example") + require.Contains(t, db.Env, "http_proxy=http://proxy:3128") } // act builds Dockerfile actions through the API, which does not pre-populate the proxy @@ -483,7 +481,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { config := testcase.rc.Config config.Workdir = testcase.name config.BindWorkdir = bindWorkDir - gotbind, gotmount := rctemplate.GetBindsAndMounts() + gotbind, gotmount := rctemplate.GetBindsAndMounts(t.Context()) // Name binds/mounts are either/or if config.BindWorkdir { @@ -533,7 +531,8 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { }, }, Config: &Config{ - BindWorkdir: false, + BindWorkdir: false, + ValidVolumes: []string{"**"}, Secrets: map[string]string{ "MAME": "/host/mame/roms", }, @@ -544,7 +543,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { rc.ExprEval = rc.NewExpressionEvaluator(context.Background()) require.NoError(t, rc.resolvePlatformImage(context.Background())) - gotbind, gotmount := rc.GetBindsAndMounts() + gotbind, gotmount := rc.GetBindsAndMounts(t.Context()) assert.Contains(t, gotbind, "/host/mame/roms:/root/.mame/roms:ro") assert.NotContains(t, gotbind, "${{ secrets.MAME }}") assert.NotContains(t, gotmount, "${{ secrets.MAME }}") @@ -562,12 +561,13 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { Config: &Config{ BindWorkdir: false, SharedToolCache: true, // so OverridesToolCache has a mount to displace + ValidVolumes: []string{"**"}, }, containerSpec: model.ContainerSpec{Volumes: testcase.volumes}, } rc.Run.JobID = "job1" - gotbind, gotmount := rc.GetBindsAndMounts() + gotbind, gotmount := rc.GetBindsAndMounts(t.Context()) if len(testcase.wantbind) > 0 { assert.Contains(t, gotbind, testcase.wantbind) @@ -602,11 +602,12 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { Config: &Config{}, } - _, gotmount := rc.GetBindsAndMounts() + _, gotmount := rc.GetBindsAndMounts(t.Context()) assert.NotContains(t, gotmount, sharedToolCacheVolume) rc.Config.SharedToolCache = true - _, gotmount = rc.GetBindsAndMounts() + rc.containerSpec.Volumes = []string{"/unapproved:" + container.DefaultToolCache} + _, gotmount = rc.GetBindsAndMounts(t.Context()) assert.Equal(t, container.DefaultToolCache, gotmount[sharedToolCacheVolume]) }) @@ -616,18 +617,18 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { } rc := &RunContext{ Run: &model.Run{JobID: "job1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"job1": {}}}}, - Config: &Config{BindWorkdir: true, Workdir: "/workspace/1/owner/repo", PresetGitHubContext: &model.GithubContext{}}, + Config: &Config{BindWorkdir: true, Workdir: "/workspace/1/owner/repo", PresetGitHubContext: &model.GithubContext{}, ValidVolumes: []string{"claimed"}}, } - gotbind, _ := rc.GetBindsAndMounts() + gotbind, _ := rc.GetBindsAndMounts(t.Context()) assert.True(t, slices.ContainsFunc(gotbind, func(bind string) bool { return strings.HasPrefix(bind, "/workspace/1:/workspace/1") }), gotbind) rc.Config.BindWorkdir = false - _, gotmount := rc.GetBindsAndMounts() + _, gotmount := rc.GetBindsAndMounts(t.Context()) assert.Equal(t, "/workspace/1", gotmount[rc.jobContainerName()]) rc.containerSpec.Volumes = []string{"claimed:/workspace/1"} - _, gotmount = rc.GetBindsAndMounts() + _, gotmount = rc.GetBindsAndMounts(t.Context()) assert.Equal(t, "/workspace/1/owner/repo", gotmount[rc.jobContainerName()]) }) } @@ -1272,12 +1273,13 @@ func TestRunContext_cleanupFailedStart(t *testing.T) { } for _, testcase := range []struct { - name, health string - pullError error + name, health string + pullError, startError error }{ {name: "healthy", health: container.HealthHealthy}, {name: "unhealthy", health: container.HealthUnhealthy}, {name: "pull failure", pullError: errors.New("pull failed")}, + {name: "start failure", startError: errors.New("start failed")}, } { t.Run(testcase.name, func(t *testing.T) { var operations []string @@ -1291,7 +1293,7 @@ func TestRunContext_cleanupFailedStart(t *testing.T) { for name, instance := range map[string]*containerMock{"job": job, "service": service} { instance.On("Pull", true).Return(record(name+".Pull", map[string]error{"job": testcase.pullError}[name])) instance.On("Create", mock.Anything, mock.Anything).Return(record(name+".Create", nil)) - instance.On("Start", false).Return(record(name+".Start", nil)) + instance.On("Start", false).Return(record(name+".Start", map[string]error{"service": testcase.startError}[name])) instance.On("Remove").Return(record(name+".Remove", nil)) instance.On("Close").Return(record(name+".Close", nil)) } @@ -1302,10 +1304,14 @@ func TestRunContext_cleanupFailedStart(t *testing.T) { service.On("Inspect", mock.Anything).Return(&container.Info{State: "running", Health: health}, nil). Run(func(mock.Arguments) { operations = append(operations, "service.Inspect") }).Once() } - service.On("DumpLogs", mock.Anything).Return(nil). - Run(func(mock.Arguments) { operations = append(operations, "service.DumpLogs") }) + stdouts := map[string]io.Writer{} + service.On("DumpLogs", mock.Anything).Return(nil).Run(func(mock.Arguments) { + operations = append(operations, "service.DumpLogs") + fmt.Fprintln(stdouts["postgres:latest"], "::add-mask::leak") + }) origNewContainer := newContainer newContainer = func(input *container.NewContainerInput) container.ExecutionsEnvironment { + stdouts[input.Image] = input.Stdout return map[string]*containerMock{"postgres:latest": service, "node:20": job}[input.Image] } t.Cleanup(func() { newContainer = origNewContainer }) @@ -1316,22 +1322,30 @@ func TestRunContext_cleanupFailedStart(t *testing.T) { Run: &model.Run{JobID: "job", Workflow: workflow}, platformImage: "node:20", } - ctx := common.WithDryrun(t.Context(), true) + logger, hook := logrustest.NewNullLogger() + ctx := common.WithLogger(common.WithDryrun(t.Context(), true), logger) rc.ExprEval = rc.NewExpressionEvaluator(ctx) err = rc.startContainer().Then(rc.stopContainer()).Finally(rc.closeContainer())(ctx) want := "job.Remove service.Remove service.Close service.Pull job.Pull" - if testcase.pullError != nil { + switch { + case testcase.pullError != nil: require.ErrorIs(t, err, testcase.pullError) - } else { - want += " service.Create service.Start service.Inspect job.Create job.Start job.Inspect job.Copy service.Inspect" + case testcase.startError != nil: + require.ErrorIs(t, err, testcase.startError) + want += " service.Create service.Start service.DumpLogs" + default: + want += " service.Create service.Start service.Inspect job.Create job.Start job.Inspect job.Copy service.Inspect service.DumpLogs" if testcase.health == container.HealthUnhealthy { require.ErrorContains(t, err, "the service 'postgres' is unhealthy") - want += " service.DumpLogs" } else { require.NoError(t, err) } } assert.Equal(t, strings.Fields(want+" job.Remove service.Remove service.Close job.Close"), operations) + assert.Empty(t, rc.Masks) + assert.Equal(t, testcase.pullError == nil && testcase.health != container.HealthUnhealthy, slices.ContainsFunc(hook.AllEntries(), func(entry *log.Entry) bool { + return strings.HasPrefix(entry.Message, "::group::Print service container logs") + })) }) } @@ -1593,6 +1607,12 @@ func createRunsOnRunContext(t *testing.T, runsOn string) *RunContext { }) } +func TestRunContextContainerImageCannotSelectHost(t *testing.T) { + rc := createIfTestRunContext(map[string]*model.Job{"job1": createJob(t, "container: -self-hosted", "")}) + require.NoError(t, rc.resolvePlatformImage(t.Context())) + assert.False(t, rc.IsHostEnv()) +} + func TestRunContextImageOS(t *testing.T) { ctx := context.Background()