[K8s] Do env-variable expansion on the uninterpreted Config files by simonferquel · Pull Request #974 · docker/cli

simonferquel · 2018-03-27T16:48:26Z

This fix an issue where composefiles like

version: '3'
services:
  front:
    image: nginx:1.12.1-alpine
      ports:
      - 80

get translated into

version: "3.0"
services:
  front:
    image: nginx:1.12.1-alpine
      ports:
      - mode: ingress
        target: 80
        protocol: tcp

The problem here is that the long format for describing ports has been
introduced in 3.2 and so, the generated compose file is invalid.

I thought first about just always use latest version information to
override the output composefile version, but that would just make the
problem worse once we introduce a new compose file schema in the CLI
that is not supported on the server side.

This PR makes it possible to interpolate the compose files without
converting them into composetypes.Config (but only work on the loaded
config file format: map[string]interface{}. This way, we don't expand
short representations into long ones, and don't risk breaking original
compose files.

simonferquel · 2018-03-27T16:48:54Z

ping @mnottale @chris-crone @vdemeester

dnephin · 2018-03-27T17:13:08Z

This is an excellent example of why the Compose file YAML should not be used as part of the API. The API should have its own types and the client should translate the compose file representation into the API types (just like with swarm mode). That way updates to the Compose file on the client don't unexpected break the API.

simonferquel · 2018-03-27T17:34:03Z

@dnephin agreed. That is also why we did that for v1beta2 :)

vdemeester · 2018-03-28T08:25:59Z

cli/command/stack/kubernetes/deploy.go

 		return errors.Errorf("Please specify only one compose file (with --compose-file).")
 	}

+	// due to env var expansion logic (and the need to conserve the same compose version as original file)


the need to conserve the same compose version as original file

Do we need that ?

At the beginning, I wanted to just override the original composefile version with the latest supported by the cli (in order to align the "long-formats" generating by marshalling composetypes.Config with the version it is supported in), but after a bit of thinking I found that it was a bad idea: if in a future version of the CLI we introduce a new compose schema version, we will generate composefiles that the server-side can't understand if not updated (and we don't want to break compatibility with Docker EE for example).

@simonferquel this is only for v1beta1 api (compose controller) right ? we know which version we support on that release of the controller, so really, we could, for this case only, decide of a version and never update it (until we drop support for v1beta1 👼 ). It's a special case anyway so let's make the "fix" the simplest.

I don't think keeping an old config structure and implementing conversion from latest to this reference version is simpler than just doing the variable expansion on non-interpreted config files (this PR basically just expose exiting code to the kubernetes subpackage)

vdemeester

I don't think keeping an old config structure and implementing conversion from latest to this reference version is simpler than just doing the variable expansion on non-interpreted config files (this PR basically just expose exiting code to the kubernetes subpackage)

Not sure I get that one. We don't keep an old structure anywhere.

As is, we do interpolation twice and use a different "config" between what is sent (just the map serialized) and what's used later (to wait for it to be ready, etc..). This breaks behavior now or maybe more in the future. As an example, this mean we don't call LoadService anymore (for the composefile we send), and thus we won't parse the envfile and update the environment for the service(s) — this is something we do as of now — same for resolveVolumePaths (although it's kinda broken #966).
With that PR, what will be the behavior for v1beta2 implemented in #899 ?
The v1beta1 api version support composefile from 3.0 to 3.x in the first version of the compose controller right ?

// data/config_schema_v3.0.json
// data/config_schema_v3.1.json
// data/config_schema_v3.2.json
// data/config_schema_v3.3.json
// data/config_schema_v3.4.json

So here are my thoughs:

Even with newer version of the controller, we should make the assumption that the v1beta1 api endpoint does not support composefile version higher (to make sure we don't have some weird behavior, like having different behavior between too v1beta1 endpoint served by different controller version).
That means we can/should know for sure in the CLI, which version of the composefile format we can and cannot send to the v1beta1 endpoint. As long format for describing ports has been introduced in 3.2, we know for a fact that v1beta1 supports that too. That means, we can/should send the compose file to the v1beta1 as version: "3.4" all the time if necessary (or at least 3.2 if it's lower than that).
If we need to "customize" the way the structure is marshalled back to a yaml for a specific version (from the current one to "3.x"), then it should be on the struct itself.

vdemeester · 2018-03-28T10:04:12Z

cc @silvin-lubecki @thaJeztah

silvin-lubecki · 2018-03-28T10:30:28Z

That means we can/should know for sure in the CLI, which version of the composefile format we can and cannot send to the v1beta1 endpoint. As long format for describing ports has been introduced in 3.2, we know for a fact that v1beta1 supports that too. That means, we can/should send the compose file to the v1beta1 as version: "3.4" all the time if necessary (or at least 3.2 if it's lower than that).

👍 on this.

I think the CLI can error out if a Compose file with greater version than v1beta1 supports is used.

simonferquel · 2018-03-28T12:40:37Z

Ok, so we would have to be very careful anytime the composetypes.Config (or any of its nested struct) is updated, that we don't replace constructs from v3.5 with a different representation (e.g. that we only add fields). (v3.5 is the latest format supported in ee)

simonferquel · 2018-03-28T12:52:38Z

Done that. Also added a v3.5 schema validation of the resulting composefile after marshalling the composetypes.Config

simonferquel · 2018-03-29T07:47:20Z

@vdemeester does it fit you ?

vdemeester

LGTM 🐯
(honestly I would have prefered "3.4" because it's what 0.1.x version of controller supports)

simonferquel · 2018-03-29T09:37:52Z

0.1.x only shipped with a private beta of Docker for Mac, we don't want to support it any way (and docker for desktop ships its own version of the docker CLI aligned with the engine/kubernetes integration anyway)

vdemeester · 2018-03-29T09:43:15Z

0.1.x only shipped with a private beta of Docker for Mac, we don't want to support it any way (and docker for desktop ships its own version of the docker CLI aligned with the engine/kubernetes integration anyway)

Right, but the image is there and public (I think ?) so it "could" still be used. I know we don't support it (as of Docker inc.), but if the there is no need to use 3.5 over 3.4 for v1beta1, it's the safest choice 👼

silvin-lubecki · 2018-03-30T08:10:25Z

cli/command/stack/kubernetes/loader.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+const v1beta1MaxComposeVersion = "3.5"


Can we move this constant to the apiv1beta1 package? It should be the package responsibility to know which compose version it can handle.

And maybe we should add one also for v1beta2, and generalize the process?

@silvin-lubecki we don't need one for v1beta2 as we send a struct that is not the composefile, this it has nothing to do with the composefile version

Good point 😅

But I think my first comment is still valid.

silvin-lubecki · 2018-03-30T08:13:07Z

cli/command/stack/kubernetes/loader.go

+		return nil, err
+	}
+	if err = schema.Validate(resparsedConfig, v1beta1MaxComposeVersion); err != nil {
+		return nil, errors.Wrapf(err, "the generated compose yaml file is invalid with v%s", v1beta1MaxComposeVersion)


Remove "generated" in the message? I don't know if it will be a useful information for the user?

silvin-lubecki · 2018-03-30T08:14:04Z

cli/command/stack/kubernetes/loader_test.go

 		},
 		Spec: apiv1beta1.StackSpec{
-			ComposeFile: `version: "3.1"
+			ComposeFile: `version: "3.5"


Add a test with a compose file which fails with a 3.6 version?

simonferquel · 2018-04-25T13:45:06Z

@silvin-lubecki I've moved the const declaration and added a test for v3.6 files

thaJeztah · 2018-05-09T19:26:32Z

ping @simonferquel this needs a rebase

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

thaJeztah

LGTM, thanks!

thaJeztah · 2018-05-16T18:18:38Z

I think all comments were addressed, so merging

simonferquel requested review from dnephin and vdemeester as code owners March 27, 2018 16:48

GordonTheTurtle added the status/0-triage label Mar 27, 2018

simonferquel force-pushed the compose-kube-env-expansion branch 2 times, most recently from 16442b4 to 270b9eb Compare March 27, 2018 17:42

vdemeester added status/2-code-review and removed status/0-triage labels Mar 28, 2018

vdemeester reviewed Mar 28, 2018

View reviewed changes

vdemeester requested changes Mar 28, 2018

View reviewed changes

simonferquel force-pushed the compose-kube-env-expansion branch from 270b9eb to 5aff96f Compare March 28, 2018 12:51

vdemeester approved these changes Mar 29, 2018

View reviewed changes

silvin-lubecki requested changes Mar 30, 2018

View reviewed changes

thaJeztah added area/stack area/kubernetes labels Apr 30, 2018

chris-crone mentioned this pull request Apr 30, 2018

Using ports only works with Compose v3.2 and above docker-archive-public/docker.app#65

Closed

Make sure every composefiles sent to a v1beta1 endpoint are parsable

f766aff

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel force-pushed the compose-kube-env-expansion branch from 8ae6e4d to f766aff Compare May 9, 2018 20:18

thaJeztah approved these changes May 16, 2018

View reviewed changes

thaJeztah merged commit bac0d8f into docker:master May 16, 2018

GordonTheTurtle added this to the 18.06.0 milestone May 16, 2018

Conversation

simonferquel commented Mar 27, 2018

Uh oh!

simonferquel commented Mar 27, 2018

Uh oh!

dnephin commented Mar 27, 2018

Uh oh!

simonferquel commented Mar 27, 2018

Uh oh!

vdemeester Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Mar 28, 2018

Uh oh!

silvin-lubecki commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonferquel commented Mar 28, 2018

Uh oh!

simonferquel commented Mar 28, 2018

Uh oh!

simonferquel commented Mar 29, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

simonferquel commented Mar 29, 2018

Uh oh!

vdemeester commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonferquel commented Apr 25, 2018

Uh oh!

thaJeztah commented May 9, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vdemeester Mar 28, 2018 •

edited

Loading

vdemeester left a comment •

edited

Loading

silvin-lubecki commented Mar 28, 2018 •

edited

Loading