Skip to content

Update config/secret handling: Copy files into containers instead of bind mounting - #12448

Closed
schaubl wants to merge 5 commits into
docker:mainfrom
SixSq:fix-configs-secrets-2
Closed

schaubl wants to merge 5 commits into
docker:mainfrom
SixSq:fix-configs-secrets-2

Conversation

@schaubl

@schaubl schaubl commented Jan 9, 2025

Copy link
Copy Markdown

Currently, Docker Compose copies a config or secret into the container if the source is an environment variable or inlined content. However, if the source is a file, the file is bind mounted into the container. This approach fails when Docker Compose is invoked on a different filesystem than the Docker daemon host filesystem (e.g., when called from within a container or remotely using the -H option).

This PR changes the behavior for file-based config/secret sources to match the handling of environment variables and inlined content. Specifically, it reads the file from the filesystem where Docker Compose is executed (like Docker Stack) and copies it into the container.

This update enhances the functionality of Docker Compose when run "remotely," providing the ability to push configuration files, a feature previously available only with Docker Stack/Swarm.

What I did:

  • Updated the behavior for handling configs and secrets sourced from files.
  • Ensured consistency with the documentation and the behavior of Docker Stack.
  • Enabled Docker Compose to push configuration files when run remotely, aligning with Docker Stack/Swarm capabilities.

Notes:

  1. It fixes Support remote DOCKER_HOST by copying configs and secrets files instead of bind mounting them #11867
  2. It is a reopening of Update config/secret handling: Copy files into containers instead of bind mounting #11984 based on the latest code base
  3. It is an alternative to Add support for configs.file's and secrets.file's on remote docker hosts #12251
    Differences it that this PR checks the validity of the config/secret parameters in getCreateConfigs before the container is created where the other one does it after.
    Also this PR doesn't support directories which correspond to the doc and the behaviour of Docker Configs&Secrets used by Swarm/Stack.

@polarathene

Copy link
Copy Markdown

This PR changes the behavior for file-based config/secret sources to match the handling of environment variables and inlined content.

Question, will this allow file sources to leverage uid, gid, and mode long options for config like already supported for content and environment? That's been a long standing caveat with file for secrets in compose when not using swarm IIRC.

…it into the container

Signed-off-by: schaubl <schaubl@users.noreply.github.com>
@schaubl
schaubl force-pushed the fix-configs-secrets-2 branch from c34fb16 to d9e531d Compare February 14, 2025 09:52
@schaubl
schaubl requested a review from a team as a code owner February 14, 2025 09:52
@schaubl
schaubl requested review from glours and ndeloof February 14, 2025 09:52
@ndeloof

ndeloof commented Feb 14, 2025

Copy link
Copy Markdown
Contributor

This PR doesn't address comments on #11867
tl;dr:
config/secret file MAY be mounted from (remote) docker host. Many users rely on this, some do even require changes to the original file to be propagated inside container. So we can't just switch to a distinct behavior which would introduce a breaking change. This would at least require an explicit opt-in flag in compose.yaml

@ndeloof

ndeloof commented Feb 14, 2025

Copy link
Copy Markdown
Contributor

@polarathene yes indeed, this is the main reason for this approach to be experimented

@schaubl

schaubl commented Feb 18, 2025 •

Copy link
Copy Markdown
Author

This PR changes the behavior for file-based config/secret sources to match the handling of environment variables and inlined content.

Question, will this allow file sources to leverage uid, gid, and mode long options for config like already supported for content and environment? That's been a long standing caveat with file for secrets in compose when not using swarm IIRC.

@polarathene @ndeloof Yes, uid, gid, and mode are all working with this PR.

However currently it still show the following warning which I will remove later today: (done)
"uid", "gid" and "mode" are not supported, they will be ignored

…and secrets

Signed-off-by: schaubl <schaubl@users.noreply.github.com>
@schaubl
schaubl force-pushed the fix-configs-secrets-2 branch from f9611db to b530e6c Compare February 18, 2025 12:58
Signed-off-by: schaubl <schaubl@users.noreply.github.com>
@schaubl
schaubl force-pushed the fix-configs-secrets-2 branch from b8884c2 to 66579df Compare February 18, 2025 16:01
…iner creation

Signed-off-by: schaubl <schaubl@users.noreply.github.com>
@schaubl

schaubl commented Feb 18, 2025 •

Copy link
Copy Markdown
Author

This PR doesn't address comments on #11867 tl;dr: config/secret file MAY be mounted from (remote) docker host. Many users rely on this, some do even require changes to the original file to be propagated inside container. So we can't just switch to a distinct behavior which would introduce a breaking change. This would at least require an explicit opt-in flag in compose.yaml

@ndeloof I think if the goal of a user is to mount files from the (remote) docker host, he should use bind mounts.
I explained my rationale more in detail in the following comment on the original GH issue (#11867 (comment)) and on my first PR (#11984 (comment)) and the first comment on the current PR.
As mentioned in my comment on #11867, I think this is a breaking change that "fix" the initial breaking change.
I share your view on your first comment on the GH issue, which says that this change of behaviour should be mentioned in the release notes.

@andoks @LaXiS96 @domsew @jgraichen @terev @polarathene @0xbase12 I'm mentioning you here because you showed interest into this and you might want to test this branch and then maybe provide feedback.

Signed-off-by: schaubl <schaubl@users.noreply.github.com>
@schaubl
schaubl force-pushed the fix-configs-secrets-2 branch from 926b0df to 0762b94 Compare February 18, 2025 23:46
Comment thread pkg/compose/create.go
values = append(values, v)
}
return values, nil
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

just curious - why not keeping the convention of value, error instead of value or error

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hello,
It's not value or error but just error if any, nil otherwise.
This function now only do checks, it doesn't return any value (its name&definition has been changed accordingly).
See point "9. Return nil for Success" of Best Practices For Error Handling in Go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support remote DOCKER_HOST by copying configs and secrets files instead of bind mounting them

4 participants