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

Add node wrapper and nodejs debug helper image #34

Merged

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Apr 12, 2020

This PR:

  1. Introduces a node wrapper that has special handling to ensure for --inspect-style arguments are only used when running application scripts.
  2. Creates a new nodejs debug helper image that installs this node wrapper.

Towards fixing GoogleContainerTools/skaffold#2170. With this nodejs debug helper image in place, we can then change skaffold debug to 1) use the debug helper image, and 2) set PATH to cause this node-wrapper to be invoked.


NodeJS's node supports a set of --inspect arguments to activate debugging mode using the Chrome DevTools protocol. There are a few variants:

  • --inspect or --inspect=<port>: launches the app with devtools listening on the given port (or default 9229)
  • --inspect-brk or --inspect-brk=<port>: launches but immediately stops the app

Many NodeJS applications use NodeJS-based tools like npm and nodemon to launch the app rather than running node directly, and often use several layered together. For example, Skaffold's own nodejs example uses npm invoking nodemon, which then invokes node. Further complicating the issue is that the node images on Docker Hub use docker-entrypoint.sh as a helper script for launching apps. So this example might cause the following sequence:

  1. docker-entrypoint.sh sh -c 'npm run production'
  2. npm run production
  3. node /path/to/npm run production
  4. node src/index.js

This sequence makes it very difficult for skaffold debug to start launch the application for debugging as --inspects are usually intercepted by one of the launch tools.

This PR provides a node wrapper that adds special handling for --inspect-style arguments on the command-line and NODE_OPTIONS environment variable. When executing a node_modules script, any --inspect-like arguments are stripped and put in NODE_DEBUG. When executing an application script (i.e., does not live in node_modules), the NODE_DEBUG value is inlined.

For example, skaffold debug -p dev on nodejs example would do the following:

  1. run docker-entrypoint.sh sh -c 'npm run production' with NODE_OPTIONS=--inspect=9229 but with PATH=/path/to/this/wrapper:$PATH
  2. npm run production (with NODE_OPTIONS=--inspect=9229)
  3. /path/to/this/wrapper/node /path/to/npm run productionnpm is recognized as a non-application script and thus the --inspect=9229 is stripped from the NODE_OPTIONS and placed it in NODE_DEBUG
  4. /path/to/actual/node /path/to/npm run production
  5. /path/to/this/wrapper/node src/index.jssrc/index.js is recognized as an application script and so NODE_DEBUG is inlined to the node command-line
  6. /path/to/actual/node --inspect=9229 src/index.js

If you have npm/node on your system, you can check it out with something as simple as:

$ cd nodejs/
$ go build -o node .
$ cat package.json
{
  "name": "backend",
  "version": "1.0.0",
  "scripts": {
    "production": "node index.js",
    "development": "nodemon index.js"
  },
  "devDependencies": {
    "nodemon": "^1.18.4"
  }
}

$ cat index.js
console.log("Hello, World!");

$ NODE_OPTIONS=--inspect npm install
Debugger listening on ws://127.0.0.1:9229/16fad1a5-3de7-4813-9512-31f94caa3090
For help, see: https://nodejs.org/en/docs/inspector
...
$ NODE_OPTIONS=--inspect npm run production
Debugger listening on ws://127.0.0.1:9229/4f066a67-b682-4997-b887-8ac77fda1f3b
For help, see: https://nodejs.org/en/docs/inspector

> [email protected] production /app
> node index.js

Starting inspector on 127.0.0.1:9229 failed: address already in use
...

Whereas with the node-wrapper in place:

$ NODE_OPTIONS=--inspect PATH=.:$PATH WRAPPER_VERBOSE=debug npm install
...
$ NODE_OPTIONS=--inspect PATH=.:$PATH WRAPPER_VERBOSE=debug npm run production
...
DEBU[0000] exec: /usr/local/bin/node [/usr/local/bin/npm run production] (env: map[HOME:/root  NODE_DEBUG:--inspect NODE_OPTIONS: PATH:/n:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin  WRAPPER_VERBOSE:debug _:/usr/local/bin/npm npm_config_scripts_prepend_node_path:false]) 

> [email protected] production /app
> node index.js

...
DEBU[0000] exec: /usr/local/bin/node [--inspect index.js] (env: map[HOME:/root HOSTNAME:401afbcac003 INIT_CWD:/app NODE:/usr/local/bin/node NODE_OPTIONS: WRAPPER_VERBOSE:debug)
...
Debugger listening on ws://127.0.0.1:9229/614eb2cc-8f39-4bc2-a780-8109931cb134
For help, see: https://nodejs.org/en/docs/inspector
Hello, World!

To see it in action, we use Skaffold's simple NodeJS project with npm and the node image:

$ cd nodejs
$ GOOS=linux go build -o node .
$ cd $skaffold/examples/nodejs/backend
$ docker build -t nodejs-example .
$ docker run -it --rm -v $OLDPWD:/n \
   -v $PWD:/home/node/app \
   -w /home/node/app \
   -e 'PATH=/n:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin' \
   -e WRAPPER_VERBOSE=debug \
   -e NODE_OPTIONS=--inspect \
   nodejs-example \
   npm run production

@briandealwis briandealwis requested a review from quoctruong April 12, 2020 04:09
@loosebazooka
Copy link
Member

I think it's hard to understand what you mean in the description. Can you include like a before and after transformation (or something in the description)?

@briandealwis
Copy link
Member Author

Rewrote description and made some other fixes.


// use an absolute path in case we're being run within a node_modules directory
script := findScript(nc.args)
if abs, err := filepath.Abs(script); err == nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when there's an error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I was thinking of just treating it as a file name but maybe it's better to skip any processing and just immediately hand off to the unwrapped executable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think filepath.Abs() only errors if it can't determine the current directory

@briandealwis
Copy link
Member Author

PTAL @dgageot

@briandealwis briandealwis merged commit 4195474 into GoogleContainerTools:duct-tape Apr 28, 2020
@briandealwis briandealwis deleted the nodejs-helper branch April 28, 2020 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants