fix: docker-entrypoint.sh file handling, closes #1456

[MasonM]

Description

The docker-entrypoint.sh script added in #1043 is intended to run the supplied command with node if it contains a - or doesn't correspond to a system command. In Alpine, this doesn't work if the supplied command corresponds to a regular, non-executable JS file.

The root issue is a bug in ash/dash: its implementation of command -v <command_name> incorrectly outputs the supplied command_name even for non-executable files. This is a violation of the POSIX standard and has been reported to the Debian team, though there's been no activity in several years.

As a workaround, this adds an additional check to docker-entrypoint.sh for regular files that aren't marked as executable.

Motivation and Context

See #1456

Testing Details

$ cd 14/alpine3.14
$ mkdir dist && echo 'console.log("success")' > dist/index.js                                                                                                              
$ docker run --rm -v "$PWD:/app" -w /app node:alpine3.14 dist/index.js                                                                                                
/usr/local/bin/docker-entrypoint.sh: exec: line 8: dist/index.js: Permission denied

$ docker build -t testimage . > /dev/null
$ docker run --rm -v "$PWD:/app" -w /app testimage dist/index.js                                                                                                           
success                                                           

Example Output(if appropriate)

N/A

Types of changes

• Documentation
• Version change (Update, remove or add more Node.js versions)
• Variant change (Update, remove or add more variants, or versions of variants)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Others (non of above)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING.md document.
• All new and existing tests passed. (I don't see any tests in this repository)

[SimenB]

We have a couple of smoke tests (just running node -v, npm -v and yarn -v to ensure the binaries are there) - should we have that for the entrypoint as well? Our current scripts run inside image when its built, so it might not be as straight forward, tho

[MasonM]

@SimenB Okay, I added a simple regression test to .github/workflows/build-test.yml, though this could lead to maintenance difficulties as that file grows. Having a test runner that can be invoked from the command line (e.g. https://github.com/docker-library/official-images/tree/master/test) seems ideal to me, but that would be quite a bit of work.

[PeterDaveHello]

I'm not sure if it's a good naming, maybe something more meaningful will be great.

[MasonM]

Okay, I renamed it to "Verify entrypoint runs regular, non-executable files with node" and slightly simplified it

[PeterDaveHello]

Nice, thanks @MasonM

[MasonM]

@SimenB I did some research to see if there was a better way of writing tests, and I found #802, which added BATS to this repo. With BATS, the test I added to .github/workflows/build-test.yml would look something like:

setup() {
  tmp_file=$(mktemp)
  echo 'console.log("success")' > "${tmp_file}"
}

@test "Verify entrypoint runs regular, non-executable files with node" {
  run docker run --rm -v "${tmp_file}:/app/index.js" node:${{ matrix.version }}-${{ matrix.variant }} app/index.js
  [ "$output" = 'success' ]
}

That's cleaner IMO and would make it easy to add additional test cases, but BATS was removed in #1339. I think it should be brought back, though it probably should be in a separate PR to keep this one focused. What do you think?

[SimenB]

Should be fine to re-add? /cc @nodejs/docker

[PeterDaveHello]

Should be fine to re-add? /cc @nodejs/docker

I'm good to add it back 👍

[SimenB]

Getting the test in would be great also beyond this specific issue, as it would have caught #1582 as well (at least I think so)

[MasonM]

Okay, I'll work on adding back BATS and integrating it with Github Actions after this is merged.

edit: Here's a draft: #1588

[SimenB]

Thanks @MasonM!

[github-actions]

Created PR on the official-images repo (docker-library/official-images#11157). See https://github.com/docker-library/faq#an-images-source-changed-in-git-now-what if you are wondering when it will be available on the Docker Hub.