From 6c60af76779b739b7487e3fbe83ea15cd2ab8ad5 Mon Sep 17 00:00:00 2001
From: Ryan <me@hackerc.at>
Date: Mon, 27 Sep 2021 19:01:14 +0000
Subject: [PATCH] fix: rewrite how image env is merged (#828)
* fix: rewrite how image env is merged
* test: add test for extractFromImageEnv
---
pkg/container/docker_run.go | 78 +++++++++++++++----------------
pkg/container/docker_run_test.go | 57 +++++++++++++---------
pkg/container/testdata/Dockerfile | 5 ++
pkg/runner/run_context.go | 1 +
pkg/runner/step_context.go | 6 ++-
5 files changed, 84 insertions(+), 63 deletions(-)
create mode 100644 pkg/container/testdata/Dockerfile
diff --git a/pkg/container/docker_run.go b/pkg/container/docker_run.go
index f2402cb..3757d84 100644
--- a/pkg/container/docker_run.go
+++ b/pkg/container/docker_run.go
@@ -17,6 +17,7 @@ import (
"github.com/go-git/go-billy/v5/helper/polyfill"
"github.com/go-git/go-billy/v5/osfs"
"github.com/go-git/go-git/v5/plumbing/format/gitignore"
+ "github.com/joho/godotenv"
"github.com/docker/cli/cli/connhelper"
"github.com/docker/docker/api/types"
@@ -72,6 +73,7 @@ type Container interface {
Start(attach bool) common.Executor
Exec(command []string, env map[string]string, user, workdir string) common.Executor
UpdateFromEnv(srcPath string, env *map[string]string) common.Executor
+ UpdateFromImageEnv(env *map[string]string) common.Executor
UpdateFromPath(env *map[string]string) common.Executor
Remove() common.Executor
}
@@ -167,6 +169,10 @@ func (cr *containerReference) UpdateFromEnv(srcPath string, env *map[string]stri
return cr.extractEnv(srcPath, env).IfNot(common.Dryrun)
}
+func (cr *containerReference) UpdateFromImageEnv(env *map[string]string) common.Executor {
+ return cr.extractFromImageEnv(env).IfNot(common.Dryrun)
+}
+
func (cr *containerReference) UpdateFromPath(env *map[string]string) common.Executor {
return cr.extractPath(env).IfNot(common.Dryrun)
}
@@ -296,13 +302,6 @@ func (cr *containerReference) create(capAdd []string, capDrop []string) common.E
isTerminal := term.IsTerminal(int(os.Stdout.Fd()))
input := cr.input
- insp, _, err := cr.cli.ImageInspectWithRaw(ctx, input.Image)
- if err != nil {
- logger.Error(err)
- }
-
- input.Env = mergeEnvFromImage(input.Env, insp.Config.Env)
-
config := &container.Config{
Image: input.Image,
Cmd: input.Cmd,
@@ -355,39 +354,6 @@ func (cr *containerReference) create(capAdd []string, capDrop []string) common.E
}
}
-func mergeEnvFromImage(inputEnv, imageEnv []string) []string {
- envMap := make(map[string]string)
- for _, v := range inputEnv {
- e := strings.Split(v, `=`)
- if e[1] == "" {
- envMap[e[0]] = ""
- } else {
- envMap[e[0]] = e[1]
- }
- }
-
- for _, v := range imageEnv {
- e := strings.SplitN(v, `=`, 2)
- if env, ok := envMap[e[0]]; !ok {
- if e[1] != "" {
- envMap[e[0]] = e[1]
- }
- } else {
- if e[0] == "PATH" {
- if e[1] != "" {
- envMap[e[0]] = strings.Join([]string{env, e[1]}, `:`)
- }
- }
- }
- }
-
- out := make([]string, 0)
- for k, v := range envMap {
- out = append(out, strings.Join([]string{k, v}, `=`))
- }
- return out
-}
-
var singleLineEnvPattern, mulitiLineEnvPattern *regexp.Regexp
func (cr *containerReference) extractEnv(srcPath string, env *map[string]string) common.Executor {
@@ -437,6 +403,38 @@ func (cr *containerReference) extractEnv(srcPath string, env *map[string]string)
}
}
+func (cr *containerReference) extractFromImageEnv(env *map[string]string) common.Executor {
+ envMap := *env
+ return func(ctx context.Context) error {
+ logger := common.Logger(ctx)
+
+ inspect, _, err := cr.cli.ImageInspectWithRaw(ctx, cr.input.Image)
+ if err != nil {
+ logger.Error(err)
+ }
+
+ imageEnv, err := godotenv.Unmarshal(strings.Join(inspect.Config.Env, "\n"))
+ if err != nil {
+ logger.Error(err)
+ }
+
+ for k, v := range imageEnv {
+ if k == "PATH" {
+ if envMap[k] == "" {
+ envMap[k] = v
+ } else {
+ envMap[k] += `:` + v
+ }
+ } else if envMap[k] == "" {
+ envMap[k] = v
+ }
+ }
+
+ env = &envMap
+ return nil
+ }
+}
+
func (cr *containerReference) extractPath(env *map[string]string) common.Executor {
localEnv := *env
return func(ctx context.Context) error {
diff --git a/pkg/container/docker_run_test.go b/pkg/container/docker_run_test.go
index b5bdb66..89a0568 100644
--- a/pkg/container/docker_run_test.go
+++ b/pkg/container/docker_run_test.go
@@ -1,34 +1,47 @@
package container
import (
- "sort"
+ "context"
"testing"
"github.com/stretchr/testify/assert"
)
-func TestMergeEnvFromImage(t *testing.T) {
- inputEnv := []string{
- "PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/sbin",
- "GOPATH=/root/go",
- "GOOS=linux",
+func TestDocker(t *testing.T) {
+ ctx := context.Background()
+ client, err := GetDockerClient(ctx)
+ assert.NoError(t, err)
+
+ dockerBuild := NewDockerBuildExecutor(NewDockerBuildExecutorInput{
+ ContextDir: "testdata",
+ ImageTag: "envmergetest",
+ })
+
+ err = dockerBuild(ctx)
+ assert.NoError(t, err)
+
+ cr := &containerReference{
+ cli: client,
+ input: &NewContainerInput{
+ Image: "envmergetest",
+ },
}
- imageEnv := []string{
- "PATH=/root/go/bin",
- "GOPATH=/tmp",
- "GOARCH=amd64",
+ env := map[string]string{
+ "PATH": "/usr/local/bin:/usr/bin:/usr/sbin:/bin:/sbin",
+ "RANDOM_VAR": "WITH_VALUE",
+ "ANOTHER_VAR": "",
+ "CONFLICT_VAR": "I_EXIST_IN_MULTIPLE_PLACES",
}
- merged := mergeEnvFromImage(inputEnv, imageEnv)
- sort.Strings(merged)
-
- expected := []string{
- "PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/sbin:/root/go/bin",
- "GOPATH=/root/go",
- "GOOS=linux",
- "GOARCH=amd64",
- }
- sort.Strings(expected)
-
- assert.Equal(t, expected, merged)
+ envExecutor := cr.extractFromImageEnv(&env)
+ err = envExecutor(ctx)
+ assert.NoError(t, err)
+ assert.Equal(t, map[string]string{
+ "PATH": "/usr/local/bin:/usr/bin:/usr/sbin:/bin:/sbin:/this/path/does/not/exists/anywhere:/this/either",
+ "RANDOM_VAR": "WITH_VALUE",
+ "ANOTHER_VAR": "",
+ "SOME_RANDOM_VAR": "",
+ "ANOTHER_ONE": "BUT_I_HAVE_VALUE",
+ "CONFLICT_VAR": "I_EXIST_IN_MULTIPLE_PLACES",
+ }, env)
}
diff --git a/pkg/container/testdata/Dockerfile b/pkg/container/testdata/Dockerfile
new file mode 100644
index 0000000..3653a63
--- /dev/null
+++ b/pkg/container/testdata/Dockerfile
@@ -0,0 +1,5 @@
+FROM scratch
+ENV PATH="/this/path/does/not/exists/anywhere:/this/either"
+ENV SOME_RANDOM_VAR=""
+ENV ANOTHER_ONE="BUT_I_HAVE_VALUE"
+ENV CONFLICT_VAR="I_EXIST_ONLY_HERE"
diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go
index 791c1d4..cb4c5c9 100755
--- a/pkg/runner/run_context.go
+++ b/pkg/runner/run_context.go
@@ -156,6 +156,7 @@ func (rc *RunContext) startJobContainer() common.Executor {
rc.stopJobContainer(),
rc.JobContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop),
rc.JobContainer.Start(false),
+ rc.JobContainer.UpdateFromImageEnv(&rc.Env),
rc.JobContainer.UpdateFromEnv("/etc/environment", &rc.Env),
rc.JobContainer.Exec([]string{"mkdir", "-m", "0777", "-p", ActPath}, rc.Env, "root", ""),
rc.JobContainer.CopyDir(copyToPath, rc.Config.Workdir+string(filepath.Separator)+".", rc.Config.UseGitIgnore).IfBool(copyWorkspace),
diff --git a/pkg/runner/step_context.go b/pkg/runner/step_context.go
index a33db58..0db3ff7 100644
--- a/pkg/runner/step_context.go
+++ b/pkg/runner/step_context.go
@@ -163,7 +163,11 @@ func (sc *StepContext) setupEnv(ctx context.Context) (ExpressionEvaluator, error
rc := sc.RunContext
sc.Env = sc.mergeEnv()
if sc.Env != nil {
- err := rc.JobContainer.UpdateFromEnv(sc.Env["GITHUB_ENV"], &sc.Env)(ctx)
+ err := rc.JobContainer.UpdateFromImageEnv(&sc.Env)(ctx)
+ if err != nil {
+ return nil, err
+ }
+ err = rc.JobContainer.UpdateFromEnv(sc.Env["GITHUB_ENV"], &sc.Env)(ctx)
if err != nil {
return nil, err
}