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
Add Windows configs support #33169
Add Windows configs support #33169
Conversation
container/container_windows.go
Outdated
) | ||
|
||
const ( | ||
containerInternalConfigsDirPath = `C:\ProgramData\Docker\internal\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.
ALLUSERSPROFILE?
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.
We made a mistake choosing programdata
over program files
in the daemon due to the ACLing. I think this should be under program files
so we don't have to fix later, and have the right ACLs when running a container as a regular non-admin user.
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 don't think Program Files is the right place for this. We can adjust the security descriptor, if necessary, though file security comes from the host, and the volume is mounted read-only. My primary concern with ProgramData\Docker is overlap with the service configuration directory if we allow nested containers. This is not really an issue, so long as subdirectory names are managed carefully, but could be confusing. Any other suggestions?
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.
My intention of my comnent was os.Getenv would be preferred over hardcoding 😅
ProgramData itself looks good
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.
In the current Windows container implementation, the container system drive is always C, and we cannot easily read the container environment from the host.
ping @jhowardmsft PTAL 😅 |
daemon/oci_windows.go
Outdated
// In s.Mounts | ||
mounts, err := daemon.setupMounts(c) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if len(c.ConfigReferences) > 0 { | ||
if err := daemon.Mount(c); err != nil { |
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.
This is another case of a relatively expensive mount/unmounts which should be avoided if possible.
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.
The container file system is mounted for creating the symlinks. I believe it is already mounted, at this point, with process isolation, so this should just be reference counting. It is not mounted earlier for Hyper-V, so this is required.
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.
Right, but a much shorter code-path would be to move the isHyperV
calculation a few lines below this change up, and key off that. IOW. It'll also make the check much more obvious than indirectly relying on ref counting.
if isHyperV {
if err := daemon.Mount(c)....
}
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.
Per slack, but putting on GH too. createSpec
on Windows is called for starting a container. For Hyper-V containers, it's not safe to mount the sandbox on the host if the container has been started before as it's an attack vector on the host.
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.
Yes, good catch. I will ensure this is only done if the container has never been started. I will also update the corresponding code in #32208.
5532bfa
to
7d9be43
Compare
@jhowardmsft PR was updated PTAL |
This will be rebased, with some manual editing, once #32208 is merged. |
Updated and test locally, with and without secrets in the same service. |
Signed-off-by: John Stephens <johnstep@docker.com>
// The container file system is mounted before this function is called, | ||
// except for Hyper-V containers, so mount it here in that case. | ||
if isHyperV { | ||
if err := daemon.Mount(c); err != nil { | ||
return nil, err | ||
} | ||
defer daemon.Unmount(c) |
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.
👍
LGTM. Close enough to the secrets PR in terms of change, so most of my previous nits are addressed. |
if err := ioutil.WriteFile(fPath, config.Spec.Data, configRef.File.Mode); err != nil { | ||
return errors.Wrap(err, "error injecting config") | ||
} | ||
} |
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.
As a followup, I wonder if this code could be shared with the Linux implementation. It looks very similar.
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.
Yes, I think this is worth looking into. The Windows version is essentially a subset, since we do not yet have an equivalent to UID, GID, and mode. It is also similar to setupSecretDir
.
LGTM |
Signed-off-by: John Stephens johnstep@docker.com
This change implements configs support for Windows by writing all configs to a single container directory, mounted at
C:\ProgramData\Docker\internal\configs
, and creating symlinks from the target to these files.