Skip to content

fix: search $DOCKER_CONFIG if no base64 config is provided #398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Oct 30, 2024
Merged

fix: search $DOCKER_CONFIG if no base64 config is provided #398

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8c0198f
fix: always set DOCKER_CONFIG unless manually set and no base64 confi…
mafredri Oct 28, 2024
5fbcf2d
simplify if statement
mafredri Oct 28, 2024
d559c1d
simplify return
mafredri Oct 28, 2024
5da2aaa
copy config to protect against remount, fix docker perms
mafredri Oct 28, 2024
6459b39
rewrite config detection and env override
mafredri Oct 29, 2024
3f6830d
update tests
mafredri Oct 29, 2024
d463de4
remote shadow
mafredri Oct 29, 2024
86db5a5
add a bit of extra validation
mafredri Oct 29, 2024
296ab91
remove log
mafredri Oct 29, 2024
167af18
update docs, add make gen
mafredri Oct 30, 2024
8ad40eb
Merge branch 'main' into mafredri/fix-docker-config-path
mafredri Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 117 additions & 44 deletions envbuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,13 @@ func run(ctx context.Context, opts options.Options, execArgs *execArgsInfo) erro

opts.Logger(log.LevelInfo, "%s %s - Build development environments from repositories in a container", newColor(color.Bold).Sprintf("envbuilder"), buildinfo.Version())

cleanupDockerConfigJSON, err := initDockerConfigJSON(opts.Logger, workingDir, opts.DockerConfigBase64)
cleanupDockerConfigOverride, err := initDockerConfigOverride(opts.Filesystem, opts.Logger, workingDir, opts.DockerConfigBase64)
if err != nil {
return err
}
defer func() {
if err := cleanupDockerConfigJSON(); err != nil {
opts.Logger(log.LevelError, "failed to cleanup docker config JSON: %w", err)
if err := cleanupDockerConfigOverride(); err != nil {
opts.Logger(log.LevelError, "failed to cleanup docker config override: %w", err)
}
}() // best effort

Expand Down Expand Up @@ -711,6 +711,11 @@ func run(ctx context.Context, opts options.Options, execArgs *execArgsInfo) erro
// Sanitize the environment of any opts!
options.UnsetEnv()

// Remove the Docker config secret file!
if err := cleanupDockerConfigOverride(); err != nil {
return err
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Moved this higher up so that when we reset DOCKER_CONFIG, we don't interfere with envbuilder trying to set envs from the build/devcontainer.


// Set the environment from /etc/environment first, so it can be
// overridden by the image and devcontainer settings.
err = setEnvFromEtcEnvironment(opts.Logger)
Expand Down Expand Up @@ -770,11 +775,6 @@ func run(ctx context.Context, opts options.Options, execArgs *execArgsInfo) erro
exportEnvFile.Close()
}

// Remove the Docker config secret file!
if err := cleanupDockerConfigJSON(); err != nil {
return err
}

if runtimeData.ContainerUser == "" {
opts.Logger(log.LevelWarn, "#%d: no user specified, using root", stageNumber)
}
Expand Down Expand Up @@ -978,13 +978,13 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error)

opts.Logger(log.LevelInfo, "%s %s - Build development environments from repositories in a container", newColor(color.Bold).Sprintf("envbuilder"), buildinfo.Version())

cleanupDockerConfigJSON, err := initDockerConfigJSON(opts.Logger, workingDir, opts.DockerConfigBase64)
cleanupDockerConfigOverride, err := initDockerConfigOverride(opts.Filesystem, opts.Logger, workingDir, opts.DockerConfigBase64)
if err != nil {
return nil, err
}
defer func() {
if err := cleanupDockerConfigJSON(); err != nil {
opts.Logger(log.LevelError, "failed to cleanup docker config JSON: %w", err)
if err := cleanupDockerConfigOverride(); err != nil {
opts.Logger(log.LevelError, "failed to cleanup docker config override: %w", err)
}
}() // best effort

Expand Down Expand Up @@ -1315,7 +1315,7 @@ func RunCacheProbe(ctx context.Context, opts options.Options) (v1.Image, error)
options.UnsetEnv()

// Remove the Docker config secret file!
if err := cleanupDockerConfigJSON(); err != nil {
if err := cleanupDockerConfigOverride(); err != nil {
return nil, err
}

Expand Down Expand Up @@ -1571,6 +1571,20 @@ func fileExists(fs billy.Filesystem, path string) bool {
return err == nil
}

func readFile(fs billy.Filesystem, name string) ([]byte, error) {
f, err := fs.Open(name)
if err != nil {
return nil, fmt.Errorf("open file: %w", err)
}
defer f.Close()

b, err := io.ReadAll(f)
if err != nil {
return nil, fmt.Errorf("read file: %w", err)
}
return b, nil
}

func copyFile(fs billy.Filesystem, src, dst string, mode fs.FileMode) error {
srcF, err := fs.Open(src)
if err != nil {
Expand All @@ -1595,6 +1609,21 @@ func copyFile(fs billy.Filesystem, src, dst string, mode fs.FileMode) error {
return nil
}

func writeFile(fs billy.Filesystem, name string, data []byte, perm fs.FileMode) error {
f, err := fs.OpenFile(name, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, perm)
if err != nil {
return fmt.Errorf("create file: %w", err)
}
_, err = f.Write(data)
if err != nil {
err = fmt.Errorf("write file: %w", err)
}
if err2 := f.Close(); err2 != nil && err == nil {
err = fmt.Errorf("close file: %w", err2)
}
return err
}

func writeMagicImageFile(fs billy.Filesystem, path string, v any) error {
file, err := fs.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o644)
if err != nil {
Expand Down Expand Up @@ -1627,55 +1656,99 @@ func parseMagicImageFile(fs billy.Filesystem, path string, v any) error {
return nil
}

func initDockerConfigJSON(logf log.Func, workingDir workingdir.WorkingDir, dockerConfigBase64 string) (func() error, error) {
var cleanupOnce sync.Once
noop := func() error { return nil }
func initDockerConfigOverride(bfs billy.Filesystem, logf log.Func, workingDir workingdir.WorkingDir, dockerConfigBase64 string) (func() error, error) {
configFile := "config.json"
oldDockerConfig := os.Getenv("DOCKER_CONFIG")
newDockerConfig := workingDir.Path()

err := os.Setenv("DOCKER_CONFIG", newDockerConfig)
if err != nil {
logf(log.LevelError, "Failed to set DOCKER_CONFIG: %s", err)
return nil, fmt.Errorf("set DOCKER_CONFIG: %w", err)
}
logf(log.LevelInfo, "Set DOCKER_CONFIG to %s", newDockerConfig)
restoreEnv := onceErrFunc(func() error {
// Restore the old DOCKER_CONFIG value.
if err := func() error {
if oldDockerConfig == "" {
return os.Unsetenv("DOCKER_CONFIG")
}
return os.Setenv("DOCKER_CONFIG", oldDockerConfig)
}(); err != nil {
return fmt.Errorf("restore DOCKER_CONFIG: %w", err)
}
logf(log.LevelInfo, "Restored DOCKER_CONFIG to %s", oldDockerConfig)
return nil
})

// If the user hasn't set the BASE64 encoded Docker config, we
// should respect the DOCKER_CONFIG environment variable.
oldDockerConfigFile := filepath.Join(oldDockerConfig, configFile)
if dockerConfigBase64 == "" && fileExists(bfs, oldDockerConfigFile) {
// It's possible that the target file is mounted and needs to
// be remounted later, so we should copy the file instead of
// hoping that the file is still there when we build.
logf(log.LevelInfo, "Using DOCKER_CONFIG at %s", oldDockerConfig)
b, err := readFile(bfs, oldDockerConfigFile)
if err != nil {
return nil, fmt.Errorf("read existing DOCKER_CONFIG: %w", err)
}
// Read into dockerConfigBase64 so we can keep the same logic.
dockerConfigBase64 = base64.StdEncoding.EncodeToString(b)
}

if dockerConfigBase64 == "" {
return noop, nil
return restoreEnv, nil
}
cfgPath := workingDir.Join("config.json")

decoded, err := base64.StdEncoding.DecodeString(dockerConfigBase64)
if err != nil {
return noop, fmt.Errorf("decode docker config: %w", err)
return restoreEnv, fmt.Errorf("decode docker config: %w", err)
}
var configFile DockerConfig
decoded, err = hujson.Standardize(decoded)
if err != nil {
return noop, fmt.Errorf("humanize json for docker config: %w", err)
return restoreEnv, fmt.Errorf("humanize json for docker config: %w", err)
}
err = json.Unmarshal(decoded, &configFile)
var dockerConfig DockerConfig
err = json.Unmarshal(decoded, &dockerConfig)
if err != nil {
return noop, fmt.Errorf("parse docker config: %w", err)
return restoreEnv, fmt.Errorf("parse docker config: %w", err)
}
for k := range configFile.AuthConfigs {
for k := range dockerConfig.AuthConfigs {
logf(log.LevelInfo, "Docker config contains auth for registry %q", k)
}
err = os.WriteFile(cfgPath, decoded, 0o644)

newDockerConfigFile := filepath.Join(newDockerConfig, configFile)
err = writeFile(bfs, newDockerConfigFile, decoded, 0o644)
if err != nil {
return noop, fmt.Errorf("write docker config: %w", err)
return restoreEnv, fmt.Errorf("write docker config: %w", err)
}
logf(log.LevelInfo, "Wrote Docker config JSON to %s", cfgPath)
oldDockerConfig := os.Getenv("DOCKER_CONFIG")
_ = os.Setenv("DOCKER_CONFIG", workingDir.Path())
newDockerConfig := os.Getenv("DOCKER_CONFIG")
logf(log.LevelInfo, "Set DOCKER_CONFIG to %s", newDockerConfig)
cleanup := func() error {
var cleanupErr error
cleanupOnce.Do(func() {
// Restore the old DOCKER_CONFIG value.
os.Setenv("DOCKER_CONFIG", oldDockerConfig)
logf(log.LevelInfo, "Restored DOCKER_CONFIG to %s", oldDockerConfig)
// Remove the Docker config secret file!
if cleanupErr = os.Remove(cfgPath); err != nil {
if !errors.Is(err, fs.ErrNotExist) {
cleanupErr = fmt.Errorf("remove docker config: %w", cleanupErr)
}
logf(log.LevelError, "Failed to remove the Docker config secret file: %s", cleanupErr)
logf(log.LevelInfo, "Wrote Docker config JSON to %s", newDockerConfigFile)

cleanup := onceErrFunc(func() error {
err := restoreEnv()
// Remove the Docker config secret file!
if err2 := os.Remove(newDockerConfigFile); err2 != nil {
if !errors.Is(err2, fs.ErrNotExist) {
err2 = fmt.Errorf("remove docker config: %w", err2)
}
logf(log.LevelError, "Failed to remove the Docker config secret file: %s", err2)
err = errors.Join(err, err2)
}
return err
})
return cleanup, nil
}

func onceErrFunc(f func() error) func() error {
var once sync.Once
return func() error {
var err error
once.Do(func() {
err = f()
})
return cleanupErr
return err
}
return cleanup, err
}

// Allows quick testing of layer caching using a local directory!
Expand Down
106 changes: 92 additions & 14 deletions integration/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,13 +552,21 @@ func TestBuildFromDockerfile(t *testing.T) {
"Dockerfile": "FROM " + testImageAlpine,
},
})
ctr, err := runEnvbuilder(t, runOpts{env: []string{
envbuilderEnv("GIT_URL", srv.URL),
envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
envbuilderEnv("DOCKER_CONFIG_BASE64", base64.StdEncoding.EncodeToString([]byte(`{"experimental": "enabled"}`))),
}})
logbuf := new(bytes.Buffer)
ctr, err := runEnvbuilder(t, runOpts{
env: []string{
envbuilderEnv("GIT_URL", srv.URL),
envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
envbuilderEnv("DOCKER_CONFIG_BASE64", base64.StdEncoding.EncodeToString([]byte(`{"experimental": "enabled"}`))),
"DOCKER_CONFIG=/config", // Ignored, because we're setting DOCKER_CONFIG_BASE64.
},
logbuf: logbuf,
})
require.NoError(t, err)

require.Contains(t, logbuf.String(), "Set DOCKER_CONFIG to /.envbuilder")
require.NotContains(t, logbuf.String(), "Using DOCKER_CONFIG at ")
Copy link
Contributor

@SasSwart SasSwart Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we generally trust log output for test assertions if we aren't testing logs?

If this log line ever drifts, this test will become a silent false positive. Is there a practical way to assert against the actual behaviour of the system? Could we copy the written docker config secret file so that it makes its way into the container and then assert against its contents?

Would that approach be worth it?

Copy link
Member

@johnstcn johnstcn Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could modify ENVBUILDER_INIT_SCRIPT to write the content of $DOCKER_CONFIG and check the value from inside the container

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SasSwart good points, I agree, in general it’s best to test behavior and not output. @johnstcn I wanted to do that here but we can’t do it in the init script since it runs after cleanup.

Not sure if it’ll work but we could try to copy it as part of Dockerfile build instructions, will look into that.

Copy link
Member Author

@mafredri mafredri Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went back to the drawing board with the implementation, changed the behavior for user-provided config files and converted the integration tests to primarily testing behavior.


output := execContainer(t, ctr, "echo hello")
require.Equal(t, "hello", strings.TrimSpace(output))

Expand All @@ -568,6 +576,66 @@ func TestBuildFromDockerfile(t *testing.T) {
require.Contains(t, output, "No such file or directory")
}

func TestBuildDockerConfigPathFromEnv(t *testing.T) {
// Ensures that a Git repository with a Dockerfile is cloned and built.
srv := gittest.CreateGitServer(t, gittest.Options{
Files: map[string]string{
"Dockerfile": "FROM " + testImageAlpine,
},
})
config, err := json.Marshal(envbuilder.DockerConfig{
AuthConfigs: map[string]clitypes.AuthConfig{
"mytestimage": {
Username: "user",
Password: "test",
},
},
})
require.NoError(t, err)
dir := t.TempDir()
err = os.WriteFile(filepath.Join(dir, "config.json"), config, 0o644)
require.NoError(t, err)

logbuf := new(bytes.Buffer)
_, err = runEnvbuilder(t, runOpts{
env: []string{
envbuilderEnv("GIT_URL", srv.URL),
envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
"DOCKER_CONFIG=/config",
},
privileged: true,
binds: []string{fmt.Sprintf("%s:/config:ro", dir)},
logbuf: logbuf,
})
require.NoError(t, err)

// Logs that the DOCKER_CONFIG is used.
require.Contains(t, logbuf.String(), "Using DOCKER_CONFIG at /config")
// Logs registry auth info from existing file.
require.Contains(t, logbuf.String(), "mytestimage")
}

func TestBuildDockerConfigDefaultPath(t *testing.T) {
// Ensures that a Git repository with a Dockerfile is cloned and built.
srv := gittest.CreateGitServer(t, gittest.Options{
Files: map[string]string{
"Dockerfile": "FROM " + testImageAlpine,
},
})
logbuf := new(bytes.Buffer)
_, err := runEnvbuilder(t, runOpts{
env: []string{
envbuilderEnv("GIT_URL", srv.URL),
envbuilderEnv("DOCKERFILE_PATH", "Dockerfile"),
},
logbuf: logbuf,
})
require.NoError(t, err)

require.Contains(t, logbuf.String(), "Set DOCKER_CONFIG to /.envbuilder")
require.NotContains(t, logbuf.String(), "Using DOCKER_CONFIG at ")
}

func TestBuildPrintBuildOutput(t *testing.T) {
// Ensures that a Git repository with a Dockerfile is cloned and built.
srv := gittest.CreateGitServer(t, gittest.Options{
Expand Down Expand Up @@ -2292,10 +2360,12 @@ func startContainerFromRef(ctx context.Context, t *testing.T, cli *client.Client
}

type runOpts struct {
image string
binds []string
env []string
volumes map[string]string
image string
privileged bool // Required for remounting.
binds []string
env []string
volumes map[string]string
logbuf *bytes.Buffer
}

// runEnvbuilder starts the envbuilder container with the given environment
Expand Down Expand Up @@ -2333,17 +2403,22 @@ func runEnvbuilder(t *testing.T, opts runOpts) (string, error) {
require.NoError(t, err, "failed to read image pull response")
img = opts.image
}
hostConfig := &container.HostConfig{
NetworkMode: container.NetworkMode("host"),
Binds: opts.binds,
Mounts: mounts,
}
if opts.privileged {
hostConfig.CapAdd = append(hostConfig.CapAdd, "SYS_ADMIN")
hostConfig.Privileged = true
}
ctr, err := cli.ContainerCreate(ctx, &container.Config{
Image: img,
Env: opts.env,
Labels: map[string]string{
testContainerLabel: "true",
},
}, &container.HostConfig{
NetworkMode: container.NetworkMode("host"),
Binds: opts.binds,
Mounts: mounts,
}, nil, nil, "")
}, hostConfig, nil, nil, "")
require.NoError(t, err)
t.Cleanup(func() {
_ = cli.ContainerRemove(ctx, ctr.ID, container.RemoveOptions{
Expand All @@ -2357,6 +2432,9 @@ func runEnvbuilder(t *testing.T, opts runOpts) (string, error) {
logChan, errChan := streamContainerLogs(t, cli, ctr.ID)
go func() {
for log := range logChan {
if opts.logbuf != nil {
opts.logbuf.WriteString(log + "\n")
}
if strings.HasPrefix(log, "=== Running init command") {
errChan <- nil
return
Expand Down