Fix Secrets mount source path while using Dockertoolbox

[jpcmadeira]

Resolves #6585

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "6585-Secrets-Mount-Path-Windows" git@github.com:Fiercely/compose.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842358451992
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[ijc]

Thanks for your PR. I'm a bit wary of importing this function like this, from the existing use I would take it as being somewhat internal to compose/config/types.py rather than of general utility.

I think we should instead go via the MountSpec.parse() method, which has an explicit normalize flag that, one hopes, does the right thing. e.g.

return MountSpec({'type': 'bind', 'source': secret['file'], 'target': target, 'read_only': True}, normalize=True)

However I think there's one more wrinkle, in this entry from CHANGELOG.md:

1.9.0 (2016-11-16)
-----------------

**Breaking changes**

- When using Compose with Docker Toolbox/Machine on Windows, volume paths are
  no longer converted from `C:\Users` to `/c/Users`-style by default. To
  re-enable this conversion so that your volumes keep working, set the
  environment variable `COMPOSE_CONVERT_WINDOWS_PATHS=1`. Users of
  Docker for Windows are not affected and do not need to set the variable.

Which suggests to me that we need to obey that here too, and need to handle 'COMPOSE_FORCE_WINDOWS_HOST' too. I think you can crib the logic from compose/config/config.py:finalize_service_volumes() to end up with something like

normalize = environment.get_boolean('COMPOSE_CONVERT_WINDOWS_PATHS')
win_host = environment.get_boolean('COMPOSE_FORCE_WINDOWS_HOST')
return MountSpec({'type': 'bind', 'source': secret['file'], 'target': target, 'read_only': True}, normalize, win_host)

WDYT?

[ijc]

Thanks for the updates, I think the is definitely moving in the right direction.

I left one comment, but at a higher level I think this:

        service_dict['secrets'] = [
            types.ServiceSecret.parse(s) for s in service_dict['secrets']
        ]

should be folded into your new finalize_service_secrets function rather then processing them twice.

I think that will amount to turning the

for v in service_dict['secrets']:
    finalized_secrets.append(MountSpec.parse(v, normalize, win_host))

into

for c in service_dict['secrets']:
    finalized_secrets.append(MountSpec.parse(types.ServiceConfig.parse(c)), normalize, win_host))

or perhaps better for readability:

for c in service_dict['secrets']:
    v = types.ServiceConfig.parse(c)
    finalized_secrets.append(MountSpec.parse(v), normalize, win_host))

[ijc]

I'm unsure about this check, at this point I think what we must have in our hand is the result of an earlier call to types.ServiceSecret.parse(s) which is a subclass of collections.namedtuple with a repr that returns a dict.

So I think this if can be removed, i.e .making the finalized_secrets.append(...) unconditional.

The variant in finalize_service_volumes is different because in that context the thing in hand could be a dict (MountSpec) or a tuple (VolumeSpec) hence the check.

[jpcmadeira]

So i don't think i can make a similar function for secrets like the one for volumes (finalize_service_volumes).
From what i have seen, secrets are divided into 2-3 parts which are only later converged into a final object (MountSpec).
The one part containing the source path is loaded through this function:

def load_mapping(config_files, get_func, entity_type, working_dir=None): 
    mapping = {}

    for config_file in config_files:
        for name, config in getattr(config_file, get_func)().items():
            mapping[name] = config or {}
            if not config:
                continue

            external = config.get('external')
            if external:
                validate_external(entity_type, name, config, config_file.version)
                if isinstance(external, dict):
                    config['name'] = external.get('name')
                elif not config.get('name'):
                    config['name'] = name

            if 'driver_opts' in config:
                config['driver_opts'] = build_string_dict(
                    config['driver_opts']
                )

            if 'labels' in config:
                config['labels'] = parse_labels(config['labels'])

            if 'file' in config:
                config['file'] = expand_path(working_dir, config['file'])

    return mapping

which is used to obtain a dict with the path of the secret file.

secrets = load_mapping(
        config_details.config_files, 'get_secrets', 'Secret', config_details.working_dir
    )

which i after apply the path conversion according to:

normalize = environment.get_boolean('COMPOSE_CONVERT_WINDOWS_PATHS')
win_host = environment.get_boolean('COMPOSE_FORCE_WINDOWS_HOST')

[howcroft]

Hi there @ijc
Thanks for supporting @jmadeira95 on getting this PR going in the right direction.
Other than rebasing and signing-off on all commits, what else would you say would be needed to get this "Mergeable"?