fix: stop service containers from clobbering the job container's credentials (#1083)

Fixes #835
Fixes #643

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1083
Reviewed-by: Zettat123 <39446+zettat123@noreply.gitea.com>
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>

act/container/docker_pull.go

@@ -72,7 +72,9 @@ func NewDockerPullExecutor(input NewDockerPullExecutorInput) common.Executor {
 
 				_ = logDockerResponse(logger, reader, err != nil)
 			}
-			return err
+			if err != nil {
+				return fmt.Errorf("failed to pull image '%s' (%s): %w", imageRef, input.Platform, err)
+			}
 		}
 		return nil
 	}

act/runner/expression.go

@@ -229,7 +229,8 @@ func (ee expressionEvaluator) evaluate(ctx context.Context, in string, defaultSt
 	logger.Debugf("evaluating expression '%s'", in)
 	evaluated, err := ee.interpreter.Evaluate(in, defaultStatusCheck)
 
-	printable := regexp.MustCompile(`::add-mask::.*`).ReplaceAllString(fmt.Sprintf("%t", evaluated), "::add-mask::***)")
+	// evaluated is an any: %t renders everything but a bool as "%!t(string=...)"
+	printable := regexp.MustCompile(`::add-mask::.*`).ReplaceAllString(fmt.Sprintf("%v", evaluated), "::add-mask::***)")
 	logger.Debugf("expression '%s' evaluated to '%s'", in, printable)
 
 	return evaluated, err

act/runner/run_context.go

@@ -339,6 +339,9 @@ func printStartJobContainerGroup(ctx context.Context, image, name, network strin
 	}
 }
 
+// newContainer is a variable so tests can substitute a container that needs no Docker daemon.
+var newContainer = container.NewContainer
+
 func (rc *RunContext) startJobContainer() common.Executor {
 	return func(ctx context.Context) error {
 		logger := common.Logger(ctx)
@@ -402,7 +405,9 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			for _, v := range spec.Cmd {
 				interpolatedCmd = append(interpolatedCmd, rc.ExprEval.Interpolate(ctx, v))
 			}
-			username, password, err = rc.handleServiceCredentials(ctx, spec.Credentials)
+			// keep these local: reusing username/password would overwrite the
+			// credentials the job container is pulled with further down
+			serviceUsername, servicePassword, err := rc.handleServiceCredentials(ctx, spec.Credentials)
 			if err != nil {
 				return fmt.Errorf("failed to handle service %s credentials: %w", serviceID, err)
 			}
@@ -423,12 +428,12 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			}
 
 			serviceContainerName := createContainerName(rc.jobContainerName(), serviceID)
-			c := container.NewContainer(&container.NewContainerInput{
+			c := newContainer(&container.NewContainerInput{
 				Name:           serviceContainerName,
 				WorkingDir:     ext.ToContainerPath(rc.Config.Workdir),
 				Image:          serviceImage,
-				Username:       username,
+				Username:       serviceUsername,
-				Password:       password,
+				Password:       servicePassword,
 				Cmd:            interpolatedCmd,
 				Env:            envs,
 				Mounts:         serviceMounts,
@@ -484,7 +489,7 @@ func (rc *RunContext) startJobContainer() common.Executor {
 		// For Gitea, `jobContainerNetwork` should be the same as `networkName`
 		jobContainerNetwork := networkName
 
-		rc.JobContainer = container.NewContainer(&container.NewContainerInput{
+		rc.JobContainer = newContainer(&container.NewContainerInput{
 			Cmd:            nil,
 			Entrypoint:     []string{"/bin/sleep", fmt.Sprint(rc.Config.ContainerMaxLifetime.Round(time.Second).Seconds())},
 			WorkingDir:     ext.ToContainerPath(rc.Config.Workdir),

act/runner/run_context_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"gitea.com/gitea/runner/act/common"
+	"gitea.com/gitea/runner/act/container"
 	"gitea.com/gitea/runner/act/exprparser"
 	"gitea.com/gitea/runner/act/model"
 
@@ -202,6 +203,93 @@ jobs:
 	assert.Empty(t, password)
 }
 
+// fakeContainer turns every container operation into a no-op, so startJobContainer
+// runs without a Docker daemon. The embedded interface is nil, so any method the
+// test does not exercise panics rather than silently doing the wrong thing.
+type fakeContainer struct {
+	container.ExecutionsEnvironment
+}
+
+func (fakeContainer) Pull(bool) common.Executor  { return func(context.Context) error { return nil } }
+func (fakeContainer) Start(bool) common.Executor { return func(context.Context) error { return nil } }
+func (fakeContainer) Remove() common.Executor    { return func(context.Context) error { return nil } }
+func (fakeContainer) Close() common.Executor     { return func(context.Context) error { return nil } }
+func (fakeContainer) GetActPath() string         { return "/var/run/act" }
+func (fakeContainer) Create([]string, []string) common.Executor {
+	return func(context.Context) error { return nil }
+}
+
+func (fakeContainer) Copy(string, ...*container.FileEntry) common.Executor {
+	return func(context.Context) error { return nil }
+}
+
+// Regression test: a service without a `credentials:` block resolves to empty
+// credentials, which used to overwrite the job container's own credentials.
+func TestStartJobContainerKeepsJobCredentialsWithServices(t *testing.T) {
+	workflow, err := model.ReadWorkflow(strings.NewReader(`
+name: test
+on: push
+jobs:
+  job:
+    runs-on: ubuntu-latest
+    container:
+      image: registry.example/private:latest
+      credentials:
+        username: job-user
+        password: job-password
+    services:
+      redis:
+        image: redis:latest
+      db:
+        image: postgres:latest
+        credentials:
+          username: db-user
+          password: db-password
+    steps: []
+`))
+	require.NoError(t, err)
+
+	var inputs []*container.NewContainerInput
+	origNewContainer := newContainer
+	newContainer = func(input *container.NewContainerInput) container.ExecutionsEnvironment {
+		inputs = append(inputs, input)
+		return fakeContainer{}
+	}
+	t.Cleanup(func() { newContainer = origNewContainer })
+
+	rc := &RunContext{
+		Name: "test",
+		Config: &Config{
+			Workdir: "/tmp",
+			// no daemon: an explicit network mode creates no network, and
+			// reusing containers short-circuits the volume cleanup executors
+			ContainerNetworkMode: "host",
+			ReuseContainers:      true,
+			Env:                  map[string]string{},
+			Secrets:              map[string]string{},
+		},
+		Env: map[string]string{},
+		Run: &model.Run{
+			JobID:    "job",
+			Workflow: workflow,
+		},
+	}
+	rc.ExprEval = rc.NewExpressionEvaluator(t.Context())
+
+	require.NoError(t, rc.startJobContainer()(t.Context()))
+
+	credentials := map[string][2]string{}
+	for _, in := range inputs {
+		credentials[in.Image] = [2]string{in.Username, in.Password}
+	}
+
+	// the job container keeps its own credentials, whichever services exist
+	require.Equal(t, [2]string{"job-user", "job-password"}, credentials["registry.example/private:latest"])
+	// each service keeps its own, and a service without credentials gets none
+	require.Equal(t, [2]string{"db-user", "db-password"}, credentials["postgres:latest"])
+	require.Equal(t, [2]string{"", ""}, credentials["redis:latest"])
+}
+
 func TestRunContext_GetBindsAndMounts(t *testing.T) {
 	rctemplate := &RunContext{
 		Name: "TestRCName",