compose parser fails 3.4+ schema for volume with external, driver, driver_opts, labels

[diablodale]

Description

The compose parser in /cli/compose/loader/ does not follow the compose 3.4+ schema rules as they apply to volumes that are external and also declare driver, driver_opts, labels. Instead, the code only follows the rules that apply for schemas 3.0-3.3.

Errant code has been isolated and trivial fix is ready.

Setup

• https://github.com/docker/cli 18.06, 18.09, or current master in 19.03
• go code that loads/parses a compose file using high-level functions like loader.Load() or the low-level errant function loader.LoadVolumes().
• parse a docker-compose.yml that is version=3.4, has a top-level volumes: that declares a volume with both external:true and driver, driver_opts, or labels

Repro

1. Setup as above
2. Parse a docker-compose similar to the following

   version: '3.4'
   volumes:
     myVol:
       external: true
       driver: 'rexray/ebs:0.11.4'
   services:
     web:
       image: ubuntu
       volumes:
         - myVol:/mnt/one

Result

Error. conflicting parameters "external" and driver specified for volume myVol

Expected

No error. And to have a fully parsed Volume section.

Notes

The errant code is below. You can see the old 3.0-3.3 rules of mutual exclusion are being applied to all schema versions. The fix is to only apply them to versions 3.0-3.3 as per Docker documentation https://docs.docker.com/compose/compose-file/#external

cli/cli/compose/loader/loader.go

Lines 579 to 595 in 52714e4

	func LoadVolumes(source map[string]interface{}, version string) (map[string]types.VolumeConfig, error) {
	volumes := make(map[string]types.VolumeConfig)
	if err := Transform(source, &volumes); err != nil {
	return volumes, err
	}
	for name, volume := range volumes {
	if !volume.External.External {
	continue
	}
	switch {
	case volume.Driver != "":
	return nil, externalVolumeError(name, "driver")
	case len(volume.DriverOpts) > 0:
	return nil, externalVolumeError(name, "driver_opts")
	case len(volume.Labels) > 0:
	return nil, externalVolumeError(name, "labels")

This is related to old work that didn't implement the full set of rules for 3.4+. Issues #274 and #608

Trivial fix. I have a commit that I'm using which was based on the 18.09.5 release. Also trivial to apply to the 18.06 and 19.03 releases. Here is the fix diablodale@1d4c6d8

Happy to make three separate PRs with fixes and test cases for the three branches 18.06, 18.09, and 19.03.

[diablodale]

branch with fix + test cases v18.09.5...diablodale:fix-loadvol1

[ndeloof]

Fro info the reason this rule has been relaxed (docker/compose@aaa0773) is to allow external.name to be replaced by two sibling fields eternal and name.

For my information can you please explain in which scenario you need to declare both external and driver?
Sounds a valid request by the way, we need to fix this.

[ndeloof]

Looking at v18.09.5...diablodale:fix-loadvol1 your fix sounds good. I personnaly would just remove this whole check, as it only check for a legacy fields combination that is now considered valid because additional attributes (but name) are ignored, so worst case we would just accepts a pre-3.4 docker-config file using those and ignore attributes that should have triggered a parsing error.

[diablodale]

Personally, I want to use external and driver on AWS ECS with a Rexray driver to mount existing EBS and S3 volumes. Currently it is not possible due to the errant code.

I'm neutral regarding the fix as I've written and your preference to remove the denial on 3.0-3.3. I wrote the fix to match the schema. However, the core team for this project might have a different philosophy regarding such topics. It's easy for me to adjust the fix and test cases for either approach.

Is there a second opinion from the core team that would add value here?

[ndeloof]

external means that docker-compose will not manage the volume and just ensure it exists on platform, so driver wouldn't make any sense in this context (should at most be ignored).

Does this relate to aws/amazon-ecs-cli#927 ? According to comments found here, external is not supported by ECS, did they maybe changed this in the meantime ?

[diablodale]

A few comments....

External means something slightly different then you wrote according to docker reference documentation. "external: If set to true, specifies that this volume has been created outside of Compose. docker-compose up does not attempt to create it, and raises an error if it doesn’t exist."
It is the creation/provisioning, not the management. Declaring external means to expect a precreated volume with the given name, driver, etc

In the general, the PR I suggest brings these compose functions in line with the schema 3.4+ spec.

In my specific case, I'm working through a series of dependencies to enable AWS ecs-cli to use a docker-compose file to mount volumes (EBS, S3FS,etc.) using the Rexray drivers.

The issue you link is somewhat related. The direct issue is aws/amazon-ecs-cli#607

[ndeloof]

Declaring external means to expect a precreated volume with the given name, driver

aws/amazon-ecs-cli#607 is interesting. IIUC the use of external: true allows to make it a share resource, but you still want ECS to create a volume (in terms of a docker volume API reference) for underlying shared AWS resource (EBS). Sounds to me the mapping for external in this scenario is incorect by ECS implementation, as you WANT the docker volume to be created by docker engine (with the adequate driver) BUT reference a shared resource. I'll think twice about this scenario, thanks for sharing.

Otherwise, I repeat I'm +1 with the proposed change

[diablodale]

Glad to hear your +1.
For your thinking on the general case... I would think both of the following docker-compose are valid wrt 3.4 schema. The first one creates/destroys driver volumes everytime. The second references pre-created driver volumes. In both cases, the driver-as-a-namespace is needed to disambiguate driver volume names that are the same, but not unique across all docker volumes.

Create/destroy driver volumes every up

version: '3.4'
volumes:
  myVol1:
    driver: 'vanhalen/driverfoo:0.11.4'
    name: themaster
  myVol2:
    driver: 'acdc/driverbar:0.11.4'
    name: themaster

services:
  web:
    image: ubuntu
    volumes:
      - myVol1:/mnt/one
      - myVol2:/mnt/two

Reference existing driver volumes on up

version: '3.4'
volumes:
  myVol1:
    external: true
    driver: 'vanhalen/driverfoo:0.11.4'
    name: themaster
  myVol2:
    external: true
    driver: 'acdc/driverbar:0.11.4'
    name: themaster

services:
  web:
    image: ubuntu
    volumes:
      - myVol1:/mnt/one
      - myVol2:/mnt/two

A change I suggested will allow parsing both the above schema. Without it, the second will fail to parse.

Separate from parsing...is code to act on the values parsed. ecs-cli already mounts/creates volumes. I have no work there to do. However, it uses a proprietary separate yaml file to declare such volumes that that file lacks the wider compatibility of docker-compose, working across platforms, merging, etc. The work I'm doing in ecs-cli will take the parsed docker-compose yaml (available from this proposed change), and use the code/variables it already has. I'm mostly just shuffling values from the docker-compose.yaml into structs that are usually filled from their separate yaml file. If curious, I've got that work in progress in branch https://github.com/diablodale/amazon-ecs-cli/tree/fix607-volumes. Given the progress I've made, I expect to have this working with my private branches this week.

[ndeloof]

Volume name must be unique in docker daemon whenever driver isn't the same. So your first sample is not valid (you can't have twice name: themaster to declare volumes) (see https://docs.docker.com/compose/compose-file/#name)

[thaJeztah]

Declaring external means to expect a precreated volume with the given name, driver, etc

Only name is used here, as it doesn't look for combination of "name+driver" (some discussion about that in moby/moby#16068 as well)

[diablodale]

Hi. I think you are collapsing some things together. There is distinction between the following:

1. schema itself
2. code that implements parsing the schema
3. data doc written to comply with schema
4. code that implements features from data parsed from data doc

bullet 1 is defined by https://docs.docker.com/compose/compose-file/#external

For version 3.3 and below of the format, external cannot be used in conjunction with other volume configuration keys (driver, driver_opts, labels). This limitation no longer exists for version 3.4 and above.

bullet 2 is this PR. It corrects the errant implementation of the parser for compose 3.4+

You have jumped to bullet 4. How dockerd and docker.exe take values parsed out of a data doc. This is not the topic of this PR and I have no inquiry into this area.

[diablodale]

Hi @thaJeztah , checking back on your inquiry - or perhaps it is being triaged?