Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate compose file on supported API, not version #7201

Closed
ndeloof opened this issue Jan 31, 2020 · 8 comments
Closed

Validate compose file on supported API, not version #7201

ndeloof opened this issue Jan 31, 2020 · 8 comments

Comments

@ndeloof
Copy link
Contributor

ndeloof commented Jan 31, 2020

Is your feature request related to a problem? Please describe.
docker-compose validates a docker-compose.yml file based on version top-level element, and use matching json schema for this purpose.

Doing so:

  • if a newcomer copy/paste some sample configuration without using the adequate version, an error will occur
  • docker-compose will reject fields introduced in a recent version until user updates version accordingly, whenever this field is fully supported and mapping to docker engine implemented
  • docker-compose will select Docker API according to version, not based on actual needs. User to select a recent version will get his docker-compose.yaml file rejected when using an old version of the engine (some distro still only support 1.13), whenever new API might not be actually needed.
  • multiple compose files to be merged MUST use the exact same version.

note: version is required to distinguish with 1.x format

2.x and 3.x are mostly always adding new fields or just replacing them with alternative equivalent fields. 3.x major version removed some fields that are still implemented by docker-compose and artificially rejected withing the code by version range checks.

Describe the solution you'd like
Better than locking features based on declared version field, docker-compose should check API exposed by the engine (docker info ApiVersion) and warn user if some of the fields set by his docker-compose.yml file can't be implemented with this version of the API.

So I propose docker-compose to evolve so that:

  • version of the compose schema used for validation supports all 2.x and 3.x fields
  • all configuration fields are implemented regardless version declared in compose file(s)
  • docker-compose check engine API and warn user for feature that can't be implemented
  • version field is made optional. As long as docker-compose.yaml file is valid according to schema is is considered valid. If it doesn't docker-compose will attempt to parse as a 1.x file for backward compatibility, and warn user for deprecation

Describe alternatives you've considered

Additional context

@justincormack
Copy link
Member

I am not even convinced we need to deprecate v1, just try parsing that schema as well.

@sudo-bmitch
Copy link

sudo-bmitch commented Feb 3, 2020

(Speaking as a user so take this as an uninformed opinion.)

I am a little concerned of how this might impact error handling. If a yml file is not parseable with any of the versions, it may be difficult to identify what the user was trying to do and give a good error message. This concern comes from seeing users mixing version 1 and version 2/3 syntax in a single file, so it's not clear if you should error that there are invalid parameters to the service named "services" or if you should flag the invalid top level keyword from the v1 service name.

I do like the idea of merging the 2.0 - 3.x versions into a single parsing since they are all relatively compatible and I've run into plenty of errors for minor version number mismatches that could have been ignored.

A few thoughts for how to improve things going forward:

  • Deprecate the implicit version 1 syntax, instead requiring an explicit version line. Generate a warning that the version line is missing. Then make a best effort at parsing the file. If all parsing attempts fail then the error can be a missing version line.
  • Make the version: 2 and version: 3 lines automatically match the highest version rather than 2.0 and 3.0 respectively. This would better follow expectations from users accustomed to semver.
  • Consider a requires top level clause to allow dependencies on functionality added in a specific version. Docker could then error out when parsing with an older version. This would solve the version mismatch issue when merging multiple files since each file could require something different, while all files are loaded into the most recent available version syntax.
  • As an alternative to the requires, automatically up-convert versions when merging files rather than requiring versions to match.

I'm also hesitant to remove the version completely since future releases may need major syntax redesigns the way v1 to v2 did. For instance, adding a "pod" concept to docker services may require a v4 syntax.

@thaJeztah
Copy link
Member

some distro still only support 1.13

Those 1.13 versions are a fork of Docker, and have many, many issues anyway (not something we should take into account).

Better than locking features based on declared version field, docker-compose should check API exposed by the engine (docker info ApiVersion) and warn user if some of the fields set by his docker-compose.yml file can't be implemented with this version of the API.

This is actually what the docker CLI does; it performs API version negotiation (using HEAD /_ping or GET /_ping), and based on that information downgrades the API version to use (if needed) to a lower version, and disables features if those are not supported by the negotiated version of the API; https://github.com/moby/moby/blob/ecdb0b22393bb669325099320d26d18687425e5f/client/client.go#L200-L246

If it doesn't docker-compose will attempt to parse as a 1.x file for backward compatibility, and warn user for deprecation

Wondering if we should just error out. My concern here is a bit that compose v1.x may be parseable as a v2.x file, but has a different behavior for the same options (e.g. no network is created for the project, and all containers are started using the default "bridge" network, use legacy --link to communicate, no DNS based resolution of services).

This is also my biggest concern with this approach; same fields/options may have different behavior defined between versions; we cannot heuristically determine which of those to pick.

I'm slightly tempted to;

  • still have a v2 or v3 version in there
  • ignore "minor" version; minor version should not be needed:
    • if a field is found that's not supported(*), we can produce an error

(*) note that this means we cannot have "allow additional properties" (so always have "additionalProperties": false) in the schema, otherwise properties will be silently ignored, and running the compose file could cause settings to not be set without the user knowing.

@thaJeztah
Copy link
Member

As an alternative to the requires, automatically up-convert versions when merging files rather than requiring versions to match.

At first glance, this makes sense to do. But.. it's also assuming that v2 and v3 are fully compatible (v2 a superset of v3). Following SemVer though, v3 is a major update (breaking changes) so there could be properties in use that are not in the v3 syntax, so after merging the files, the result should be parsed as v<max of merged files> and validated against that schema (correct?).

@ndeloof
Copy link
Contributor Author

ndeloof commented Feb 3, 2020

itentifying a v1 file is easy as its top-level structure is distinct from v2/v3. So for those if we want to keep backward compatibility we can keep existing code in place, and still rely on --links etc. We don't need a version attribute just for this purpose.

on v2/v3 none of the fields AFAICT have significantly changed their meaning. Some where deperecated and replaced, but none switched to a distinct implementation model. v3 is a major update due to field removal, but is mostly compatible if we re-introduce them

about a possible v4, this would introduce much more confusion that we already have, if we want to introduce concepts like pod, we better should look for an extended way to define a service, with not just one single container.

@sudo-bmitch
Copy link

itentifying a v1 file is easy as its top-level structure is distinct from v2/v3

Identify v1 vs v2/v3 in well formatted files should be easy enough, I'm thinking more of the error scenarios when users try to parse:

redis:
  image: redis
services:
  app:
    image: my-app

Do you error that app is not a valid field, or that redis is not a valid top level. Here's an example of this from a few days ago. My suggestion is to error "parsing failed, version field missing" rather than assuming one or the other. But that unfortunately goes against the premise of this RfC.

@ndeloof
Copy link
Contributor Author

ndeloof commented Feb 3, 2020

I would expect this one to fail as "redis" is not a valid field. Whenever we can support v1 syntax it would be saner to just deprecate this format, at least so that users stop posting sample config using it on stackoverflow ;)

@ndeloof
Copy link
Contributor Author

ndeloof commented Jun 5, 2020

This has been approved by the Compose Spec community meeting.
PR to update the spec compose-spec/compose-spec#82
Should be pretty easy to implement in docker-compose as this is mostly about removing all the version checks to enable attributes support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants