From ba2b641bc8ccc342efdd0b7de8d080a1ac8dd36c Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Wed, 12 Jul 2023 23:08:04 -0400 Subject: [PATCH 1/2] loader: consistently resolve `build.context` path When using `extends.file` to load a service from another Compose file, if the `build.context` field wasn't explicitly set, it would get ignored by the path resolution. As a result, the build context would end up as the working directory of the _root_ Compose file instead of the working directory of the _loaded_ Compose file. This was because the relative path resolution logic ignored empty `build.context` values. Unfortunately, removing that restriction is itself not sufficient, as it then attempted to verify that the local path existed in an attempt to avoid unintentionally joining the working directory with a remote context value (e.g. `/foo/https://github.com/my/repo.git`). This is problematic because the working directory itself might be relative (rooted to an unknown location that is != `.`), so it will be resolved relative to the current process working directory, and thus fail the existence check. In practice, this happens when using `extends.file`, where we do resolve paths but intentionally pass a potentially relative value for the working dir, thus making it unsafe to be doing real filesystem operations here. In fact, even if a context was specified, it was possible to break this by running Compose from a different directory than the _root_ Compose file while specifying the path to it. As there's no formal specification for determining local path vs. remote build contexts (see discussion in #385), I'm simply eliminating the existence check. This COULD mean that Compose begins to erroneously consider remote context values as local paths if builders add new unsupported syntaxes, but I think it's fair for us to be more restrictive. Additionally, I've ensured that when path resolution is happening, it _always_ resolves the `build.context` to an absolute path for consistency. In particular, this should help make it easier to use the output of `docker compose config` from arbitrary working directories. There's a new test that covers the `extends.file` + `build.context` behavior, and everal existing test adjustments to account for the fact that Compose was emitting relative `build.context` paths from `docker compose config` despite everything else being absolute (such as volumes). Fixes docker/for-mac#6912. Signed-off-by: Milas Bowman --- loader/full-struct_test.go | 13 +++++-- loader/loader_test.go | 61 ++++++++++++++++++++++++++++++ loader/paths.go | 22 +++++------ loader/types_test.go | 4 +- loader/with-version-struct_test.go | 5 ++- 5 files changed, 87 insertions(+), 18 deletions(-) diff --git a/loader/full-struct_test.go b/loader/full-struct_test.go index 513e2450..73025c90 100644 --- a/loader/full-struct_test.go +++ b/loader/full-struct_test.go @@ -53,7 +53,7 @@ func services(workingDir, homeDir string) []types.ServiceConfig { "com.example.foo": "bar", }, Build: &types.BuildConfig{ - Context: "./dir", + Context: filepath.Join(workingDir, "dir"), Dockerfile: "Dockerfile", Args: map[string]*string{"foo": strPtr("bar")}, SSH: []types.SSHKey{{ID: "default", Path: ""}}, @@ -438,6 +438,7 @@ func services(workingDir, homeDir string) []types.ServiceConfig { { Name: "bar", Build: &types.BuildConfig{ + Context: workingDir, DockerfileInline: "FROM alpine\nRUN echo \"hello\" > /world.txt\n", }, Environment: types.MappingWithEquals{}, @@ -598,6 +599,7 @@ func fullExampleYAML(workingDir, homeDir string) string { services: bar: build: + context: %s dockerfile_inline: | FROM alpine RUN echo "hello" > /world.txt @@ -605,7 +607,7 @@ services: annotations: com.example.foo: bar build: - context: ./dir + context: %s dockerfile: Dockerfile args: foo: bar @@ -1037,6 +1039,8 @@ x-nested: bar: baz foo: bar `, + workingDir, + filepath.Join(workingDir, "dir"), filepath.Join(workingDir, "example1.env"), filepath.Join(workingDir, "example2.env"), workingDir, @@ -1148,6 +1152,7 @@ func fullExampleJSON(workingDir, homeDir string) string { "services": { "bar": { "build": { + "context": %q, "dockerfile_inline": "FROM alpine\nRUN echo \"hello\" \u003e /world.txt\n" }, "command": null, @@ -1158,7 +1163,7 @@ func fullExampleJSON(workingDir, homeDir string) string { "com.example.foo": "bar" }, "build": { - "context": "./dir", + "context": %q, "dockerfile": "Dockerfile", "args": { "foo": "bar" @@ -1683,6 +1688,8 @@ func fullExampleJSON(workingDir, homeDir string) string { toPath(workingDir, "config_data"), toPath(homeDir, "config_data"), toPath(workingDir, "secret_data"), + toPath(workingDir), + toPath(workingDir, "dir"), toPath(workingDir, "example1.env"), toPath(workingDir, "example2.env"), toPath(workingDir), diff --git a/loader/loader_test.go b/loader/loader_test.go index b8b889ab..0a35d68b 100644 --- a/loader/loader_test.go +++ b/loader/loader_test.go @@ -360,6 +360,67 @@ services: assert.DeepEqual(t, service.Command, types.ShellCommand{"/bin/ash", "-c", "rm -rf /tmp/might-not-exist"}) } +func TestLoadExtendsMultipleFiles(t *testing.T) { + if testing.Short() { + t.Skip("Test creates real files on disk") + } + + tmpdir := t.TempDir() + + aDir := filepath.Join(tmpdir, "a") + assert.NilError(t, os.Mkdir(aDir, 0o700)) + aYAML := ` +services: + a: + build: . +` + assert.NilError(t, os.WriteFile(filepath.Join(tmpdir, "a", "compose.yaml"), []byte(aYAML), 0o600)) + + bDir := filepath.Join(tmpdir, "b") + assert.NilError(t, os.Mkdir(bDir, 0o700)) + bYAML := ` +services: + b: + build: + target: fake +` + assert.NilError(t, os.WriteFile(filepath.Join(tmpdir, "b", "compose.yaml"), []byte(bYAML), 0o600)) + + rootYAML := ` +services: + a: + extends: + file: ./a/compose.yaml + service: a + b: + extends: + file: ./b/compose.yaml + service: b +` + assert.NilError(t, os.WriteFile(filepath.Join(tmpdir, "compose.yaml"), []byte(rootYAML), 0o600)) + + actual, err := Load(types.ConfigDetails{ + WorkingDir: tmpdir, + ConfigFiles: []types.ConfigFile{{ + Filename: filepath.Join(tmpdir, "compose.yaml"), + }}, + Environment: nil, + }, func(options *Options) { + options.SkipNormalization = true + options.SkipConsistencyCheck = true + }) + assert.NilError(t, err) + assert.Assert(t, is.Len(actual.Services, 2)) + + svcA, err := actual.GetService("a") + assert.NilError(t, err) + assert.Equal(t, svcA.Build.Context, aDir) + + svcB, err := actual.GetService("b") + assert.NilError(t, err) + assert.Equal(t, svcB.Build.Context, bDir) +} + func TestLoadCredentialSpec(t *testing.T) { actual, err := loadYAML(` name: load-credential-spec diff --git a/loader/paths.go b/loader/paths.go index 85b687dc..45bbc545 100644 --- a/loader/paths.go +++ b/loader/paths.go @@ -68,13 +68,9 @@ func ResolveRelativePaths(project *types.Project) error { } func ResolveServiceRelativePaths(workingDir string, s *types.ServiceConfig) { - if s.Build != nil && s.Build.Context != "" && !isRemoteContext(s.Build.Context) { - // Build context might be a remote http/git context. Unfortunately supported "remote" - // syntax is highly ambiguous in moby/moby and not defined by compose-spec, - // so let's assume runtime will check - localContext := absPath(workingDir, s.Build.Context) - if _, err := os.Stat(localContext); err == nil { - s.Build.Context = localContext + if s.Build != nil { + if !isRemoteContext(s.Build.Context) { + s.Build.Context = absPath(workingDir, s.Build.Context) } for name, path := range s.Build.AdditionalContexts { if strings.Contains(path, "://") { // `docker-image://` or any builder specific context type @@ -83,10 +79,7 @@ func ResolveServiceRelativePaths(workingDir string, s *types.ServiceConfig) { if isRemoteContext(path) { continue } - path = absPath(workingDir, path) - if _, err := os.Stat(path); err == nil { - s.Build.AdditionalContexts[name] = path - } + s.Build.AdditionalContexts[name] = absPath(workingDir, path) } } for j, f := range s.EnvFile { @@ -127,8 +120,13 @@ func absComposeFiles(composeFiles []string) ([]string, error) { return composeFiles, nil } +// isRemoteContext returns true if the value is a Git reference or HTTP(S) URL. +// +// Any other value is assumed to be a local filesystem path and returns false. +// +// See: https://github.com/moby/buildkit/blob/18fc875d9bfd6e065cd8211abc639434ba65aa56/frontend/dockerui/context.go#L76-L79 func isRemoteContext(maybeURL string) bool { - for _, prefix := range []string{"https://", "http://", "git://", "github.com/", "git@"} { + for _, prefix := range []string{"https://", "http://", "git://", "ssh://", "github.com/", "git@"} { if strings.HasPrefix(maybeURL, prefix) { return true } diff --git a/loader/types_test.go b/loader/types_test.go index 59628ef5..8917ca10 100644 --- a/loader/types_test.go +++ b/loader/types_test.go @@ -25,7 +25,7 @@ import ( is "gotest.tools/v3/assert/cmp" ) -func TestMarshallProject(t *testing.T) { +func TestMarshalProject(t *testing.T) { workingDir, err := os.Getwd() assert.NilError(t, err) homeDir, err := os.UserHomeDir() @@ -45,7 +45,7 @@ func TestMarshallProject(t *testing.T) { assert.NilError(t, err) } -func TestJSONMarshallProject(t *testing.T) { +func TestJSONMarshalProject(t *testing.T) { workingDir, err := os.Getwd() assert.NilError(t, err) homeDir, err := os.UserHomeDir() diff --git a/loader/with-version-struct_test.go b/loader/with-version-struct_test.go index 458899f5..f2d0c552 100644 --- a/loader/with-version-struct_test.go +++ b/loader/with-version-struct_test.go @@ -17,6 +17,8 @@ package loader import ( + "path/filepath" + "github.com/compose-spec/compose-go/types" ) @@ -29,12 +31,13 @@ func withVersionExampleConfig() *types.Config { } func withVersionServices() []types.ServiceConfig { + buildCtx, _ := filepath.Abs("./Dockerfile") return []types.ServiceConfig{ { Name: "web", Build: &types.BuildConfig{ - Context: "./Dockerfile", + Context: buildCtx, }, Environment: types.MappingWithEquals{}, Networks: map[string]*types.ServiceNetworkConfig{ From 2c14f1c52751f0aa0b1571ec4ed64468e053fd69 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Thu, 13 Jul 2023 12:46:04 -0400 Subject: [PATCH 2/2] load: fix windows tests Various places were using absolute Unix paths, which don't work on Windows. Signed-off-by: Milas Bowman --- loader/full-example.yml | 2 +- loader/full-struct_test.go | 12 +++++++----- loader/paths_test.go | 5 +++-- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/loader/full-example.yml b/loader/full-example.yml index 9f818f8c..e2df5092 100644 --- a/loader/full-example.yml +++ b/loader/full-example.yml @@ -24,7 +24,7 @@ services: - bar labels: [FOO=BAR] additional_contexts: - foo: /bar + foo: ./bar secrets: - secret1 - source: secret2 diff --git a/loader/full-struct_test.go b/loader/full-struct_test.go index 73025c90..ef2b55f4 100644 --- a/loader/full-struct_test.go +++ b/loader/full-struct_test.go @@ -60,7 +60,7 @@ func services(workingDir, homeDir string) []types.ServiceConfig { Target: "foo", Network: "foo", CacheFrom: []string{"foo", "bar"}, - AdditionalContexts: types.Mapping{"foo": "/bar"}, + AdditionalContexts: types.Mapping{"foo": filepath.Join(workingDir, "bar")}, Labels: map[string]string{"FOO": "BAR"}, Secrets: []types.ServiceSecretConfig{ { @@ -619,7 +619,7 @@ services: - foo - bar additional_contexts: - foo: /bar + foo: %s network: foo target: foo secrets: @@ -1041,6 +1041,7 @@ x-nested: `, workingDir, filepath.Join(workingDir, "dir"), + filepath.Join(workingDir, "bar"), filepath.Join(workingDir, "example1.env"), filepath.Join(workingDir, "example2.env"), workingDir, @@ -1152,7 +1153,7 @@ func fullExampleJSON(workingDir, homeDir string) string { "services": { "bar": { "build": { - "context": %q, + "context": "%s", "dockerfile_inline": "FROM alpine\nRUN echo \"hello\" \u003e /world.txt\n" }, "command": null, @@ -1163,7 +1164,7 @@ func fullExampleJSON(workingDir, homeDir string) string { "com.example.foo": "bar" }, "build": { - "context": %q, + "context": "%s", "dockerfile": "Dockerfile", "args": { "foo": "bar" @@ -1179,7 +1180,7 @@ func fullExampleJSON(workingDir, homeDir string) string { "bar" ], "additional_contexts": { - "foo": "/bar" + "foo": "%s" }, "network": "foo", "target": "foo", @@ -1690,6 +1691,7 @@ func fullExampleJSON(workingDir, homeDir string) string { toPath(workingDir, "secret_data"), toPath(workingDir), toPath(workingDir, "dir"), + toPath(workingDir, "bar"), toPath(workingDir, "example1.env"), toPath(workingDir, "example2.env"), toPath(workingDir), diff --git a/loader/paths_test.go b/loader/paths_test.go index 42abd89a..1a623479 100644 --- a/loader/paths_test.go +++ b/loader/paths_test.go @@ -84,6 +84,7 @@ func TestResolveBuildContextPaths(t *testing.T) { func TestResolveAdditionalContexts(t *testing.T) { wd, _ := filepath.Abs(".") + absSubdir := filepath.Join(wd, "dir") project := types.Project{ Name: "myProject", WorkingDir: wd, @@ -96,7 +97,7 @@ func TestResolveAdditionalContexts(t *testing.T) { AdditionalContexts: map[string]string{ "image": "docker-image://foo", "oci": "oci-layout://foo", - "abs_path": "/tmp", + "abs_path": absSubdir, "github": "github.com/compose-spec/compose-go", "rel_path": "./testdata", }, @@ -117,7 +118,7 @@ func TestResolveAdditionalContexts(t *testing.T) { AdditionalContexts: map[string]string{ "image": "docker-image://foo", "oci": "oci-layout://foo", - "abs_path": "/tmp", + "abs_path": absSubdir, "github": "github.com/compose-spec/compose-go", "rel_path": filepath.Join(wd, "testdata"), },