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

Validation false positive for nested interpolation in docker-compose #252

Open
AlexSkrypnyk opened this issue Nov 2, 2023 · 21 comments · May be fixed by #304
Open

Validation false positive for nested interpolation in docker-compose #252

AlexSkrypnyk opened this issue Nov 2, 2023 · 21 comments · May be fixed by #304

Comments

@AlexSkrypnyk
Copy link

AlexSkrypnyk commented Nov 2, 2023

When provisioning in Lagoon, the following variable assignment in the docker-compose.yml is reported as invalid, while Docker Compose considers this valid and can process it accordingly.

MYVAR1: ${MYVAR1:-${MYVAR2:-defaultvalue}substring}

When provisioning in Lagoon, the following error is received:

There are issues with your docker compose file that lagoon uses that should be fixed.

Please note that any modern shell supports nested variable interpolation, so supporting this in Lagoon seems a reasonable expectation.

@shreddedbacon
Copy link
Member

Interesting, because this uses the docker-compose spec library to validate. There could be something missed though.

Can you provide how you're defining this (full docker-compose file would be nice if possible)

@AlexSkrypnyk
Copy link
Author

x-environment: &default-environment
  MYVAR1: ${MYVAR1:-${MYVAR2:-defaultvalue}substring}

services:
  cli:
    image: uselagoon/php-8.1-cli-drupal:23.10.0
    environment:
      <<: *default-environment

@AlexSkrypnyk
Copy link
Author

AlexSkrypnyk commented Nov 2, 2023

@shreddedbacon

this uses the docker-compose spec library to validate

I only was able to find this

### use the `build-validate` built in validater to run over the provided docker-compose file

Not sure how to follow to the library though.

The official docker compose docs state that nested interpolation is supported
https://docs.docker.com/compose/compose-file/12-interpolation/

Looking at the tests here, https://github.com/uselagoon/build-deploy-tool/tree/6bd767a68605db8e16a553d0f61766969852d3ca/test-resources/docker-compose, there is not a single instance of nested interpolation of variables among those fixtures.

@shreddedbacon
Copy link
Member

We use the github.com/compose-spec/compose-go library to parse the docker-compose files. The error you see is from it.

We don't have a test case for nested interpolation because until today you're the first person to do it.

When I run your provided nested variable into a test case, I get the following

invalid interpolation format for x-environment.MYVAR1: "${MYVAR2:-defaultvalue". You may need to escape any $ with another $

but if I do it like the docker docs state and use ${MYVAR1:-${MYVAR2:-defaultvalue}}, it parses fine.

@shreddedbacon
Copy link
Member

For some reason, maybe there is a fix in a newer compose-go library version, it doesn't like the substring in your provided nested variable.

I'll check the compose-go library if there is a fix in a newer version, otherwise will have to log an issue with them.

@AlexSkrypnyk
Copy link
Author

What is the version of the compose-go are you using?
There is a bug that was fixed in https://github.com/compose-spec/compose-go/releases/tag/v1.13.5

https://github.com/compose-spec/compose-go/pull/405/files

@shreddedbacon
Copy link
Member

Yes, just saw that. I'll see if we can update our version. We are running a forked version at the moment due to some Lagoon-isms though that I'll need to backport.

@shreddedbacon
Copy link
Member

Will see what I can do, but it may not be today.

@tobybellwood
Copy link
Member

The provided code does not build with docker compose v2, only v1
image

It can't handle the "substring" appended inside the variable
MYVAR1: ${MYVAR1:-${MYVAR2:-defaultvalue}}substring

@AlexSkrypnyk
Copy link
Author

AlexSkrypnyk commented Nov 2, 2023

The use case is quite wide: base a value on COMPOSE_PROJECT_NAME but allow to override it from internal variables.

Example from DrevOps:

DREVOPS_LOCALDEV_URL: &default-url ${DREVOPS_LOCALDEV_URL:-${COMPOSE_PROJECT_NAME:-example-site}.docker.amazee.io}

I want the default URL to be based on COMPOSE_PROJECT_NAME by default (which defaults to the current directory) - this allows to run the same project codebase from multiple directories and the URL will be auto-prefixed with the current project dir name. But it also will allow to override that URL locally by providing DREVOPS_LOCALDEV_URL if needed without changing any code in docker-compose.yml.

@AlexSkrypnyk
Copy link
Author

@tobybellwood
please see the link above - we have pin-pointed the issue - it is due to the outdated composer-go library used by Lagoon.

@tobybellwood
Copy link
Member

oh - ahahaha - I realised that i'd pinned my docker compose v2 to v2.17.3 for testing last week... And hence why it was broken on my local (and hence was hitting the exact same bug)

@AlexSkrypnyk AlexSkrypnyk changed the title Validation false positive for double interpolation in docker-compose Validation false positive for nested interpolation in docker-compose Nov 3, 2023
@AlexSkrypnyk
Copy link
Author

a gentle bump on this one. consumer projects need to use workarounds that would need to be undone later.

I'm happy to sponsor this work.

@shreddedbacon
Copy link
Member

shreddedbacon commented Jan 2, 2024

Edit: TLDR; we are addressing it, but unfortunately we have to tackle it over some time to gently nudge people.

There is a linked issue where we are looking to incorporate the changes required to do this, but there are issues that we face with changing the library to a newer version. See the linked PR #253 for the change to upgrade the library.

PR #255 was created to take care the other issue, we can't implement the changes for #253 as they would immediately fail almost every single build in AIO cloud Lagoon, and probably anyone that has used our old examples.

We use #255 to warn people now in their builds to say that they have things that they need to address before we can incorporate the changes in #253.

@AlexSkrypnyk
Copy link
Author

@shreddedbacon
do you have any ETA on this. It has been 3 months and I would like to know what to expect. thanks

@shreddedbacon
Copy link
Member

@shreddedbacon do you have any ETA on this. It has been 3 months and I would like to know what to expect. thanks

No ETA. As mentioned in the last comment there are a lot of other considerations here before we can use the newer version of the compose spec. We can't just fail every build, we have gradually been rolling out warnings in the builds for things that the upgrade of the compose spec would cause failures for.

We are working on it, but as this is the only report of the bug, it isn't as high on the list as slowly informing people that we are about to break their builds is.

@AlexSkrypnyk
Copy link
Author

AlexSkrypnyk commented Apr 25, 2024

This just took a new twist: there is now a support for defining several .env files in docker-compose.yml.

  env_file:
    - path: .env
      required: false
    - path: .env.local
      required: false

Doing so, does not pass the validator:

##############################################
STEP Docker Compose Validation: Completed at 2024-04-25 06:50:46 (UTC) Duration 00:00:01 Elapsed 00:00:02
##############################################

##############################################
Warning!
There are issues with your docker compose file that lagoon uses that should be fixed.
You can run docker compose config locally to check that your docker-compose file is valid.
##############################################

2 error(s) decoding: * 'env_file[0]' expected type 'string', got unconvertible type 'map[string]interface {}', value: 'map[path:.env required:false]' * 'env_file[1]' expected type 'string', got unconvertible type 'map[string]interface {}', value: 'map[path:.env.local required:false]'

##############################################

So this is a second case where we really need the support for a new docker-copose.yml.

Docker Compose will keep updating the format and this new format would need to be supported at some point anyway. Can this be prioritised?

@shreddedbacon
Copy link
Member

Yes, we are aware of all of these. Unfortunately as said before, the tradeoff with moving to the latest version is that people will get build failures for things that we would no longer be able to check for. We need to make a call on our end as to when we will do that.

@shreddedbacon
Copy link
Member

This PR is work on getting it done, you can see that support for env_file is there #304

@AlexSkrypnyk
Copy link
Author

Hi guys. It has been a year since the original report date and I was wondering what is the plan to move forward with this. The composer-go library would need to be updated someday, right?

@tobybellwood
Copy link
Member

The next release of Lagoon, due this week, introduces the ability to selectively fail builds on compose errors, better metrics on those errors causing failures, and build error reporting in CLI and notifications so we can work with users to get their builds working correctly.

We're working towards a library update early next year. Sorry this has taken such a long time, but we're somewhat tied by the validation/strictness differences between Docker/Docker Compose and the compose-go library (and the latters fast pace of change).

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