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

diablodale commented 4 years ago

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

Setup as above

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

https://github.com/docker/cli/blob/52714e413cd1f58d5f3734191ad23a4701a9d8e3/cli/compose/loader/loader.go#L579-L595

This is related to old work that didn't implement the full set of rules for 3.4+. Issues https://github.com/docker/cli/issues/274 and https://github.com/docker/cli/issues/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 https://github.com/diablodale/cli/commit/1d4c6d8e492c0598c432314be76e5f59d4c60386

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

diablodale commented 4 years ago

branch with fix + test cases https://github.com/docker/cli/compare/v18.09.5...diablodale:fix-loadvol1

ndeloof commented 4 years ago

Fro info the reason this rule has been relaxed (https://github.com/docker/compose/commit/aaa0773b4b6ce44a3c1494d8eb2e25406f0593f9) 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 commented 4 years ago

Looking at https://github.com/docker/cli/compare/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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/aws/amazon-ecs-cli/issues/927 ? According to comments found here, external is not supported by ECS, did they maybe changed this in the meantime ?

diablodale commented 4 years ago

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 https://github.com/aws/amazon-ecs-cli/issues/607

ndeloof commented 4 years ago

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

https://github.com/aws/amazon-ecs-cli/issues/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 https://github.com/moby/moby/issues/16068 as well)

diablodale commented 4 years ago

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

schema itself
code that implements parsing the schema
data doc written to comply with schema
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 commented 4 years ago

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

docker / cli