-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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.
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
Enable configs.file's on remote docker hosts #11871
Enable configs.file's on remote docker hosts #11871
Conversation
Copy configs.file's and secrets.file's instead of bind-mounting them to make it possible to use file configs when working with remote docker hosts (like setting DOCKER_HOST to a ssh address or setting docker context) Includes support for config.files and secrets.files as directories. Note that file.Content as source of secrets is denied elsewhere with the error "validating docker-compose.yml: secrets.content_secret Additional property content is not allowed" implements: #11867
6638c2f
to
d9dd161
Compare
file := project.Secrets[config.Source] | ||
var tarArchive bytes.Buffer | ||
var err error | ||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: not handling file.Content as this seems to be rejected some other place, causing the printout "validating docker-compose.yml: secrets.content_secret Additional property content is not allowed"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it comes from the compose-spec.
There is no content
property in secrets
, only in configs
.
This correspond to the doc where secrets can only use file
or environment
.
I assume the reason is to prevent writing secrets value directly in the compose file.
} | ||
|
||
func makeTarFileEntryParams(config types.FileReferenceConfig) (mode int64, uid, gid int, err error) { | ||
mode = 0o444 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ should the default mode be different for secrets compared to configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to remove at least public read (0o440
) for secrets
err = fs.WalkDir(subdir, ".", func(filePath string, d fs.DirEntry, err error) error { | ||
header := &tar.Header{ | ||
Name: filepath.Join(config.Target, filePath), | ||
Mode: mode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Should directories have the same mode as files? Or should they perhaps have the exec bit set for the owner/group/other to access them?
@ndeloof left you a few questions in the review |
any ETA? |
See discussion in #11867 - it might take some time to decide how to proceed. |
Yesterday I made a PR which I then realized it was a duplicate of this one. I think this PR could benefit from the changes I made in the file pkg/compose/create.go. It basically keeps some preexisting checks and adds a preflight check to ensure the file used as the source for config/secret can be accessed so that if it's not the case it will fail before creating the container. Please also see the following comment I made here:
|
What I did
Copy configs.file's instead of bind-mounting them to make it possible to use file configs when working with remote docker hosts (like setting DOCKER_HOST to a ssh address or setting docker context)
Related issue
implements: #11867
(not mandatory) A picture of a cute animal, if possible in relation to what you did