Skip to content
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

Jump to bottom

loader: consistently resolve build.context path #434

Merged
merged 2 commits into from
Jul 18, 2023
Merged

loader: consistently resolve build.context path #434

merged 2 commits into from
Jul 18, 2023

Conversation

milas
Copy link
Member

@milas milas commented Jul 13, 2023

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:

@milas milas added the bug Something isn't working label Jul 13, 2023
@milas milas requested review from ndeloof, glours and laurazard July 13, 2023 03:35
@milas milas self-assigned this Jul 13, 2023
@milas
loader: consistently resolve build.context path
ba2b641 
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 compose-spec#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 <milas.bowman@docker.com>
@milas milas force-pushed the extends-file-build-context branch from b4461fa to ba2b641 Compare July 13, 2023 03:38
milas
milas commented Jul 13, 2023
View reviewed changes
loader/paths.go
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 {
Copy link
Member Author

Choose a reason for hiding this comment

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

N.B. There's actually an additional fix here for build.additional_contexts -- they were only being resolved if the full condition was met: s.Build != nil && s.Build.Context != "" && !isRemoteContext(s.Build.Context)

@milas
Copy link
Member Author

milas commented Jul 13, 2023

Ahhh I busted the tests on Windows, will look at it in the morning, at a first glance I think we need to use resolveMaybeUnixPath on the build context & additional contexts but need to think on it more 🤔

@milas
load: fix windows tests
2c14f1c 
Various places were using absolute Unix paths, which don't work
on Windows.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
milas added a commit to milas/compose-go that referenced this pull request Jul 13, 2023
@milas
load: resolve symlinks and fix various path issues
49447a0 
The loader is very inconsistent with how it deals with relative
paths, and there have been various edge cases in which it does
not behave as expected (see compose-spec#434).

At its core, this commit attempts to do the following:
 * Within the loader, do not perform real filesystem operations
   on paths (e.g. stat for existence)
 * Within the loader, use the root paths provided as-is, making
   other paths relative to them without trying to make the final
   result absolute
 * Perform real path lookup in the `ProjectOptions` helper in
   the `cli` package, including resolving symlinks and making the
   root paths absolute

Effectively, this is an attempt to make (the path-handling parts
of) the loader more "pure". It still does a lot of direct file
I/O (e.g. for `extends`, `env_file`) using `os.ReadFile` directly,
but this goes a long way towards making it practical to use the
loader for complex, multi-file projects without ever actually
touching the disk.

Pushing more of the "real system" concerns up to `ProjectOptions`
in the `cli` package means that the behavior should still be
consistent for high-level consumers.

There is a breaking behavioral change here for low-level consumers,
however, as shell-style user-home relative paths (e.g. `~/foo`)
were being resolved directly using system lookups deep within the
loader. Now, `ConfigDetails` includes a `HomeDir` field much like
`WorkingDir`, which is used to resolve these types of paths.

In the end, this should all provide greater flexibility and make
things slightly easier to test, since there's less system leakage.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas milas mentioned this pull request Jul 13, 2023
Closed
milas added a commit to milas/compose-go that referenced this pull request Jul 13, 2023
@milas
load: resolve symlinks and fix various path issues
10bf8cd 
The loader is very inconsistent with how it deals with relative
paths, and there have been various edge cases in which it does
not behave as expected (see compose-spec#434).

At its core, this commit attempts to do the following:
 * Within the loader, do not perform real filesystem operations
   on paths (e.g. stat for existence)
 * Within the loader, use the root paths provided as-is, making
   other paths relative to them without trying to make the final
   result absolute
 * Perform real path lookup in the `ProjectOptions` helper in
   the `cli` package, including resolving symlinks and making the
   root paths absolute

Effectively, this is an attempt to make (the path-handling parts
of) the loader more "pure". It still does a lot of direct file
I/O (e.g. for `extends`, `env_file`) using `os.ReadFile` directly,
but this goes a long way towards making it practical to use the
loader for complex, multi-file projects without ever actually
touching the disk.

Pushing more of the "real system" concerns up to `ProjectOptions`
in the `cli` package means that the behavior should still be
consistent for high-level consumers.

There is a breaking behavioral change here for low-level consumers,
however, as shell-style user-home relative paths (e.g. `~/foo`)
were being resolved directly using system lookups deep within the
loader. Now, `ConfigDetails` includes a `HomeDir` field much like
`WorkingDir`, which is used to resolve these types of paths.

In the end, this should all provide greater flexibility and make
things slightly easier to test, since there's less system leakage.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas
Copy link
Member Author

milas commented Jul 14, 2023

See also:

@milas milas mentioned this pull request Jul 14, 2023
Closed
milas added a commit to milas/compose-go that referenced this pull request Jul 17, 2023
@milas
load: resolve symlinks and fix various path issues
87077b4 
The loader is very inconsistent with how it deals with relative
paths, and there have been various edge cases in which it does
not behave as expected (see compose-spec#434).

At its core, this commit attempts to do the following:
 * Within the loader, do not perform real filesystem operations
   on paths (e.g. stat for existence)
 * Within the loader, use the root paths provided as-is, making
   other paths relative to them without trying to make the final
   result absolute
 * Perform real path lookup in the `ProjectOptions` helper in
   the `cli` package, including resolving symlinks and making the
   root paths absolute

Effectively, this is an attempt to make (the path-handling parts
of) the loader more "pure". It still does a lot of direct file
I/O (e.g. for `extends`, `env_file`) using `os.ReadFile` directly,
but this goes a long way towards making it practical to use the
loader for complex, multi-file projects without ever actually
touching the disk.

Pushing more of the "real system" concerns up to `ProjectOptions`
in the `cli` package means that the behavior should still be
consistent for high-level consumers.

There is a breaking behavioral change here for low-level consumers,
however, as shell-style user-home relative paths (e.g. `~/foo`)
were being resolved directly using system lookups deep within the
loader. Now, `ConfigDetails` includes a `HomeDir` field much like
`WorkingDir`, which is used to resolve these types of paths.

In the end, this should all provide greater flexibility and make
things slightly easier to test, since there's less system leakage.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
milas added a commit to milas/compose-go that referenced this pull request Jul 17, 2023
@milas
load: resolve symlinks and fix various path issues
c15dc4b 
The loader is very inconsistent with how it deals with relative
paths, and there have been various edge cases in which it does
not behave as expected (see compose-spec#434).

At its core, this commit attempts to do the following:
 * Within the loader, do not perform real filesystem operations
   on paths (e.g. stat for existence)
 * Within the loader, use the root paths provided as-is, making
   other paths relative to them without trying to make the final
   result absolute
 * Perform real path lookup in the `ProjectOptions` helper in
   the `cli` package, including resolving symlinks and making the
   root paths absolute

Effectively, this is an attempt to make (the path-handling parts
of) the loader more "pure". It still does a lot of direct file
I/O (e.g. for `extends`, `env_file`) using `os.ReadFile` directly,
but this goes a long way towards making it practical to use the
loader for complex, multi-file projects without ever actually
touching the disk.

Pushing more of the "real system" concerns up to `ProjectOptions`
in the `cli` package means that the behavior should still be
consistent for high-level consumers.

There is a breaking behavioral change here for low-level consumers,
however, as shell-style user-home relative paths (e.g. `~/foo`)
were being resolved directly using system lookups deep within the
loader. Now, `ConfigDetails` includes a `HomeDir` field much like
`WorkingDir`, which is used to resolve these types of paths.

In the end, this should all provide greater flexibility and make
things slightly easier to test, since there's less system leakage.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
milas added a commit to milas/compose-go that referenced this pull request Jul 17, 2023
@milas
load: resolve symlinks and fix various path issues
30d840f 
The loader is very inconsistent with how it deals with relative
paths, and there have been various edge cases in which it does
not behave as expected (see compose-spec#434).

At its core, this commit attempts to do the following:
 * Within the loader, do not perform real filesystem operations
   on paths (e.g. stat for existence)
 * Within the loader, use the root paths provided as-is, making
   other paths relative to them without trying to make the final
   result absolute
 * Perform real path lookup in the `ProjectOptions` helper in
   the `cli` package, including resolving symlinks and making the
   root paths absolute

Effectively, this is an attempt to make (the path-handling parts
of) the loader more "pure". It still does a lot of direct file
I/O (e.g. for `extends`, `env_file`) using `os.ReadFile` directly,
but this goes a long way towards making it practical to use the
loader for complex, multi-file projects without ever actually
touching the disk.

Pushing more of the "real system" concerns up to `ProjectOptions`
in the `cli` package means that the behavior should still be
consistent for high-level consumers.

There is a breaking behavioral change here for low-level consumers,
however, as shell-style user-home relative paths (e.g. `~/foo`)
were being resolved directly using system lookups deep within the
loader. Now, `ConfigDetails` includes a `HomeDir` field much like
`WorkingDir`, which is used to resolve these types of paths.

In the end, this should all provide greater flexibility and make
things slightly easier to test, since there's less system leakage.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@glours glours merged commit 95ac1be into compose-spec:master Jul 18, 2023
milas added a commit to milas/compose-go that referenced this pull request Jul 18, 2023
@milas
load: resolve symlinks and fix various path issues
40ba5f0 
The loader is very inconsistent with how it deals with relative
paths, and there have been various edge cases in which it does
not behave as expected (see compose-spec#434).

At its core, this commit attempts to do the following:
 * Within the loader, do not perform real filesystem operations
   on paths (e.g. stat for existence)
 * Within the loader, use the root paths provided as-is, making
   other paths relative to them without trying to make the final
   result absolute
 * Perform real path lookup in the `ProjectOptions` helper in
   the `cli` package, including resolving symlinks and making the
   root paths absolute

Effectively, this is an attempt to make (the path-handling parts
of) the loader more "pure". It still does a lot of direct file
I/O (e.g. for `extends`, `env_file`) using `os.ReadFile` directly,
but this goes a long way towards making it practical to use the
loader for complex, multi-file projects without ever actually
touching the disk.

Pushing more of the "real system" concerns up to `ProjectOptions`
in the `cli` package means that the behavior should still be
consistent for high-level consumers.

There is a breaking behavioral change here for low-level consumers,
however, as shell-style user-home relative paths (e.g. `~/foo`)
were being resolved directly using system lookups deep within the
loader. Now, `ConfigDetails` includes a `HomeDir` field much like
`WorkingDir`, which is used to resolve these types of paths.

In the end, this should all provide greater flexibility and make
things slightly easier to test, since there's less system leakage.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndeloof ndeloof ndeloof approved these changes

@glours glours glours approved these changes

@laurazard laurazard Awaiting requested review from laurazard

Assignees

@milas milas

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@milas @ndeloof @glours