I'd really like to see much of this combined with the (almost) identical code in container_operations_unix.go

I looked at this, and definitely can move some of it out into a shared file, but would rather keep it as is for now, and factor it out in a future change.

This is identical to SecretMount() in container_unix.go. Can it be combined?

True, there are at least a couple of these simple functions that could be shared. It might make sense to move them to container.go but I think I'd like to keep them separate for now.

Wondering under what circumstances this condition would be hit?

IIRC File is in a oneof in the swarmkit gRPC API. If we add other possible types of secret targets, to allow exposing them in other ways than as files, File will be nil.

The comment here should probably be updated.

Updated.

setupSecretDir is doing a mount/unmount, and then that's being done in the loop at line 38 immediately after. Mount/Unmount is a very expensive (relatively) operation compared to Linux (we had strong feedback from the perf team when I was doing an accidental mount/unmounts a while back and had to refactor), so I'd really like to see it limited to at most 1 mount/unmounts pair to support secrets.

Good catch; the mount in setupSecretDir is actually not required; I will remove it.

It looks like the this would be logged if system.MkdirAllWIthACL failed.

I will skip the log message if the error is non-existence.

Is it OK to have TODO comments in the code?

Consider this one as a hint ;-)

To clarify; yes, we use TODO's in the code; usually not for "unfinished" code, but more as reminders that (e.g.) something needs to be removed after release X, or after feature Y is implemented. The TODO (name) format also works in some editors/IDE's to show a todo list for one person specific 😄 https://confluence.jetbrains.com/display/PhpStorm/Working+with+todo+comments+and+the+todo+tool+window

t.b.h., I don't think this TODO is really useful 😄 ...

Yes, perhaps not all that useful, and now in 4 separate places...

these should be fine to remove. there were some timeline and roadmap adjustments that delayed this a bit so I think it's fine to remove for now.

I know we've talked offline about this, but I would still really like a comment here to explain why not been started before

I am not sure this is the best place to explain when to mount containers on Windows, in general, but perhaps you can give me a rough idea of the comment you would like to see. Thanks for pointing me to HasBeenStartedBefore; this check fixes an issue where the code was trying to recreate the symlinks for a container that had already been started, which fails.

Something like "Ensure that we do not attempt to mount the filesystem of a Hyper-V container that has previously been started to stop an attack vector."

Again, as per offline, if we can avoid this and not rely on reference counting, that would be far better and more explicit. Note that as also mentioned, this should be co-ordinated with the configs PR

Is there a guarantee that the container file system is mounted here in the non-Hyper-V case? It seems strange to me to start modifying the container file system without first ensuring it is mounted. Is that guarantee documented anywhere, or just implicit in the design?

Yes, for Windows Server Containers, you are guaranteed that the container FS is mounted here. In start.go:

	if err := daemon.conditionalMountOnStart(container); err != nil {
		return err
	}

	if err := daemon.initializeNetworking(container); err != nil {
		return err
	}

	spec, err := daemon.createSpec(container)
	if err != nil {
		return err
	}

Where conditionalMountOnStart always mounts Windows Server Containers, but not Hyper-V containers:

	// We do not mount if a Hyper-V container
	if !daemon.runAsHyperVContainer(container.HostConfig) {
		return daemon.Mount(container)
	}

Minor nit, talked offline. When the configs PR is ready after this, change to a defer in the previous check.