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

Nested variable interpolation in compose files is not working #4265

Open
gilbsgilbs opened this issue May 6, 2023 · 4 comments · May be fixed by #4268
Open

Nested variable interpolation in compose files is not working #4265

gilbsgilbs opened this issue May 6, 2023 · 4 comments · May be fixed by #4268

Comments

@gilbsgilbs
Copy link

gilbsgilbs commented May 6, 2023

Description

Interpolation of nested variables in compose files is not supported/bugged.

Reproduce

  1. take the following compose file:
services:
  my-service:
    image: "alpine"
    command: ["echo", "${HELLO:-${UNUSED}}"]
  1. Init swarm docker swarm init
  2. Deploy a stack with this compose file: HELLO=hello docker stack deploy --compose-file docker-compose.yml foo
  3. Show the logs of the service: docker service logs foo_my-service

The logs print hello}, meaning that the variable wasn't properly interpolated.

Note that as per compose specification, this should be allowed and is not undefined behavior.

Expected behavior

Either interpolate to hello, just like Compose does, or at least output an error such as nested variables interpolation is not supported. Currently, the interpolation is incorrect which is confusing and surprising to the user.

docker version

Client:
 Version:           23.0.5
 API version:       1.42
 Go version:        go1.20.4
 Git commit:        bc4487a59e
 Built:             Tue May  2 21:53:09 2023
 OS/Arch:           linux/amd64
 Context:           default

Server:
 Engine:
  Version:          23.0.4
  API version:      1.42 (minimum version 1.12)
  Go version:       go1.20.3
  Git commit:       cbce331930
  Built:            Fri Apr 21 22:05:37 2023
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          v1.7.0
  GitCommit:        1fbd70374134b891f97ce19c70b6e50c7b9f4e0d.m
 runc:
  Version:          1.1.7
  GitCommit:
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

docker info

Client:
 Context:    default
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc.)
    Version:  0.10.4
    Path:     /usr/lib/docker/cli-plugins/docker-buildx
  compose: Docker Compose (Docker Inc.)
    Version:  2.17.3
    Path:     /usr/lib/docker/cli-plugins/docker-compose

Server:
 Containers: 33
  Running: 3
  Paused: 0
  Stopped: 30
 Images: 276
 Server Version: 23.0.4
 Storage Driver: btrfs
  Btrfs:
 Logging Driver: json-file
 Cgroup Driver: systemd
 Cgroup Version: 2
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 1fbd70374134b891f97ce19c70b6e50c7b9f4e0d.m
 runc version:
 init version: de40ad0
 Security Options:
  seccomp
   Profile: builtin
  cgroupns
 Kernel Version: 6.2.12-arch1-1
 Operating System: Arch Linux
 OSType: linux
 Architecture: x86_64
 CPUs: 8
 Total Memory: 15.34GiB
 Name: WKS-79LR7G3
 ID: ARE2:H3PX:4QCX:J6R2:Q4CM:2BYH:EIWM:3YRO:ZISS:XTT2:4B5Y:HLIB
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Registry: https://index.docker.io/v1/
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false

Additional Info

The problem originates from the fact that the parser is regex based, which is not well suited for recursive parsing.

One solution could to write a custom tokenizer/parser, produce an AST and all that jazz. However, Compose already has a working implementation in compose-spec/compose-go. It can be used as a drop-in replacement for the custom template module used by this project. I don't know if this solution is acceptable, but if it is, I can draft a PR.

Here is a minimal patch that can make this work:

fix_interpolation.patch
diff --git a/cli/compose/interpolation/interpolation.go b/cli/compose/interpolation/interpolation.go
index e84756dba6..183ff6273b 100644
--- a/cli/compose/interpolation/interpolation.go
+++ b/cli/compose/interpolation/interpolation.go
@@ -4,7 +4,7 @@ import (
 	"os"
 	"strings"

-	"github.com/docker/cli/cli/compose/template"
+	"github.com/compose-spec/compose-go/template"
 	"github.com/pkg/errors"
 )

diff --git a/cli/compose/interpolation/interpolation_test.go b/cli/compose/interpolation/interpolation_test.go
index 069d8d2549..f8adfe419b 100644
--- a/cli/compose/interpolation/interpolation_test.go
+++ b/cli/compose/interpolation/interpolation_test.go
@@ -11,6 +11,7 @@ import (
 var defaults = map[string]string{
 	"USER":  "jenny",
 	"FOO":   "bar",
+	"CMD":   "my-cmd",
 	"count": "5",
 }

@@ -22,7 +23,11 @@ func defaultMapping(name string) (string, bool) {
 func TestInterpolate(t *testing.T) {
 	services := map[string]interface{}{
 		"servicea": map[string]interface{}{
-			"image":   "example:${USER}",
+			"image": "example:${USER}",
+			"command": []interface{}{
+				"${CMD:-${UNDEF_CMD:-}}",
+				"${UNDEF:-${CMD}}",
+			},
 			"volumes": []interface{}{"$FOO:/target"},
 			"logging": map[string]interface{}{
 				"driver": "${FOO}",
@@ -36,6 +41,7 @@ func TestInterpolate(t *testing.T) {
 		"servicea": map[string]interface{}{
 			"image":   "example:jenny",
 			"volumes": []interface{}{"bar:/target"},
+			"command": []interface{}{"my-cmd", "my-cmd"},
 			"logging": map[string]interface{}{
 				"driver": "bar",
 				"options": map[string]interface{}{
diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go
index a9b8f1f1f9..bde9efbf99 100644
--- a/cli/compose/loader/loader.go
+++ b/cli/compose/loader/loader.go
@@ -9,9 +9,10 @@ import (
 	"strings"
 	"time"

 	interp "github.com/docker/cli/cli/compose/interpolation"
 	"github.com/docker/cli/cli/compose/schema"
-	"github.com/docker/cli/cli/compose/template"
+	"github.com/compose-spec/compose-go/template"
 	"github.com/docker/cli/cli/compose/types"
 	"github.com/docker/cli/opts"
 	"github.com/docker/docker/api/types/versions"

As you can see, there's not a lot to it.

@gilbsgilbs gilbsgilbs changed the title Nested compose variable interpolation not working Nested variable interpolation in compose files is not working May 6, 2023
@bsousaa
Copy link
Contributor

bsousaa commented May 8, 2023

Looks like you have some code about this issue. Feel free to open a PR.

gilbsgilbs added a commit to gilbsgilbs/docker-cli that referenced this issue May 8, 2023
This allows for nested variables interpolation.

Fixes docker#4265

Signed-off-by: N. Le Roux <[email protected]>
@gilbsgilbs gilbsgilbs linked a pull request May 8, 2023 that will close this issue
gilbsgilbs added a commit to gilbsgilbs/docker-cli that referenced this issue May 8, 2023
This allows for nested variables interpolation.

Fixes docker#4265

Signed-off-by: N. Le Roux <[email protected]>
@gilbsgilbs
Copy link
Author

@bsousaa Thanks, done. Struggled a bit figuring out how to deal with the vendor.mod thing. Maybe this should be explained somewhere in the contributing guide.

deltasquare4 added a commit to deltasquare4/frappe_docker that referenced this issue Oct 24, 2024
Nested interpolation is not supported by docker (docker/cli#4265). Changed the file so that it uses `CUSTOM_TAG` or `ERPNEXT_VERSION` if that's not available.
revant pushed a commit to frappe/frappe_docker that referenced this issue Oct 25, 2024
Nested interpolation is not supported by docker (docker/cli#4265). Changed the file so that it uses `CUSTOM_TAG` or `ERPNEXT_VERSION` if that's not available.
@JacquesOfAllTrades
Copy link

Hi all,

This is still an issue in 27.4.0.

Technically I think the PR (#4268) just needs another approval, though I'm sure it's far out of date by now.

If it makes any difference in terms of priority, the area/swarm tag is inaccurate, i.e. the issue occurs with basic compose usage. Here's an example that I'm running in WSL on Win10:

$ ls -a
./  ../  docker-compose.yml

$ cat docker-compose.yml
services:
    myservice:
        image: alpine:3.18
        command: ["/bin/echo","The value of $$FOO is '$FOO'."]
        environment:
            # Propagate $FOO if set, else use $BAR
            - FOO=${FOO:-${BAR}}

$ FOO="baz"           docker compose run --rm myservice
time="2025-01-08T23:11:09-05:00" level=warning msg="The \"BAR\" variable is not set. Defaulting to a blank string."
The value of $FOO is 'baz'.

$ FOO="baz" BAR="qux" docker compose run --rm myservice
The value of $FOO is 'baz'.

$           BAR="qux" docker compose run --rm myservice
time="2025-01-08T23:11:27-05:00" level=warning msg="The \"FOO\" variable is not set. Defaulting to a blank string."
The value of $FOO is ''.

Of note, changing my compose file to match Frappe's workaround referenced above (i.e., changing FOO=${FOO:-${BAR}} to FOO=${FOO:-$BAR}) doesn't affect the result at all for me. (I'd paste the output, but it's fully identical to what's above, barring the timestamps.)

And if this makes any difference in terms of priority, here's a cute animal that happens to be the world's most effective feline predator. (Seriously.)
Image

Thanks for any attention this could get.

gilbsgilbs added a commit to gilbsgilbs/docker-cli that referenced this issue Jan 9, 2025
This allows for nested variables interpolation.

Fixes docker#4265

Signed-off-by: N. Le Roux <[email protected]>
gilbsgilbs added a commit to gilbsgilbs/docker-cli that referenced this issue Jan 9, 2025
This allows for nested variables interpolation.

Fixes docker#4265

Signed-off-by: N. Le Roux <[email protected]>
gilbsgilbs added a commit to gilbsgilbs/docker-cli that referenced this issue Jan 9, 2025
This allows for nested variables interpolation.

Fixes docker#4265

Signed-off-by: Gilbert Gilb's <[email protected]>
gilbsgilbs added a commit to gilbsgilbs/docker-cli that referenced this issue Jan 9, 2025
This allows for nested variables interpolation.

Fixes docker#4265

Signed-off-by: N. Le Roux <[email protected]>
@gilbsgilbs
Copy link
Author

Technically I think the PR (#4268) just needs another approval, though I'm sure it's far out of date by now.

It was out of date indeed. But I've just rebased it.

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

Successfully merging a pull request may close this issue.

3 participants