Add Windows configs support#33169
Conversation
johnstep
requested review from
aaronlehmann,
cpuguy83,
ehazlett,
lowenna and
thaJeztah
There was a problem hiding this comment.
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.
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.
My intention of my comnent was os.Getenv would be preferred over hardcoding 😅
ProgramData itself looks good
There was a problem hiding this comment.
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 😅 |
There was a problem hiding this comment.
This is another case of a relatively expensive mount/unmounts which should be avoided if possible.
There was a problem hiding this comment.
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.
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.
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.
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.
johnstep
force-pushed
the
windows-configs
branch
2 times, most recently
from
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 <[email protected]>
| if err := daemon.Mount(c); err != nil { | ||
| return nil, err | ||
| } | ||
| defer daemon.Unmount(c) |
|
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.
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.
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 |
7 participants
Signed-off-by: John Stephens [email protected]
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.