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

expand additional context relative paths #385

Merged
merged 1 commit into from
Apr 17, 2023
Merged

expand additional context relative paths #385

merged 1 commit into from
Apr 17, 2023

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Apr 7, 2023

With introduction for additional_contexts a user can set context to a local path, and as we support relative paths in many places it also make sense to expand relative paths for such additional contexts. Challenge here is that such a context can be a path or URL (just like context) but also builder specific string. I'm assuming here such a custom string will ALWAYS use a <type>:// prefix as buildx does

see docker/compose#10369

@ndeloof ndeloof requested review from laurazard, glours and milas and removed request for laurazard April 7, 2023 07:07
@ndeloof
Copy link
Collaborator Author

ndeloof commented Apr 7, 2023 •
edited
Loading

@crazy-max does it sound reasonable to assume a custom context type will always include some xxx:// prefix ?

@crazy-max
Copy link

@crazy-max does it sound reasonable to assume a custom context type will always include some xxx:// prefix ?

Named context target can also be a path. Such as with buildx:

docker buildx build --build-context project=path/to/project/source .

Otherwise yes it's either docker-image://, oci-layout://, http:// https://, or Git URL.

More info: https://docs.docker.com/engine/reference/commandline/buildx_build/#build-context

@ndeloof
Copy link
Collaborator Author

ndeloof commented Apr 7, 2023

yes, that's my point: when not prefix is given, we can check this is a path or git url (just like for context attribute)
My point here is to ensure we don't introduce in the future some new context types without a prefix, which might confuse compose file parsing. Typically, being able to distinguish path vs git url is already somehow acrobatic

milas
milas requested changes Apr 7, 2023
View reviewed changes
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

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

Change to Mapping from MappingWithEquals violates the spec

types/types.go Show resolved Hide resolved
loader/normalize.go Show resolved Hide resolved
@EronWright
Copy link

EronWright commented Apr 7, 2023 •
edited
Loading

Thanks for fixing this issue!
docker/compose#10448

Please consider updating the compose spec to clarify that relative paths should be expanded.

@milas milas mentioned this pull request Apr 11, 2023
Closed
@ndeloof
expand additional context relative paths
1592cda 
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof force-pushed the additional_context_path branch from bc97dc4 to 1592cda Compare April 12, 2023 13:48
@ndeloof ndeloof requested a review from milas April 12, 2023 13:50
@ndeloof ndeloof mentioned this pull request Apr 17, 2023
Merged
@ndeloof ndeloof merged commit e4d5895 into compose-spec:master Apr 17, 2023
@ndeloof ndeloof deleted the additional_context_path branch April 17, 2023 09:00
milas added a commit to milas/compose-go that referenced this pull request Jul 13, 2023
@milas
loader: consistently resolve build.context path
b4461fa 
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 mentioned this pull request Jul 13, 2023
Merged
milas added a commit to milas/compose-go that referenced this pull request 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milas milas milas approved these changes

@glours glours Awaiting requested review from glours

@laurazard laurazard Awaiting requested review from laurazard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ndeloof @crazy-max @EronWright @milas