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

Update Dockerfile #7

Merged
merged 2 commits into from
Dec 5, 2021
Merged

Update Dockerfile #7

merged 2 commits into from
Dec 5, 2021

Conversation

pjv
Copy link
Contributor

@pjv pjv commented Dec 3, 2021

Change to nodejs 16 to successfully build ZeroTier v. 1.84

Change to nodejs 16 to successfully build ZeroTier v. 1.84
@kmahyyg
Copy link
Owner

kmahyyg commented Dec 3, 2021

Zerotier is a c/cpp-based program, which have no affiliate with nodejs.

Ztncui is the one who use nodejs.

However, according to https://github.com/key-networks/ztncui#prerequisites . Ztncui should use nodejs v14.

Different versions(even they are LTS, but not a same major version) might lead to unwanted compiled program and unknown execution results.

Refuse to merge due to consideration of compatibility. Please ask upstream if they support nodejs v16 or not.

@kmahyyg kmahyyg closed this Dec 3, 2021
@pjv
Copy link
Contributor Author

pjv commented Dec 3, 2021

Hi @kmahyyg,

All I know is that when I tried to build an updated image of ztncui using the Dockerfile in the HEAD of the master branch here (with ENV NODEJS_MAJOR=14), a container spun up from the resulting image would not put up the web app. When I shelled into the container and did this: ./start_ztncui.sh, the output looked like this:

ZTNCUI ENV CONFIGURATION:
NODE_ENV=production
MYADDR=[redacted]
HTTP_PORT=8000
HTTP_ALL_INTERFACES=yes
pkg/prelude/bootstrap.js:1740
      throw error;
      ^

Error: The module '/opt/key-networks/ztncui/node_modules/argon2/build/Release/argon2.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 93. This version of Node.js requires
NODE_MODULE_VERSION 72. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or `npm install`).
    at process.dlopen (pkg/prelude/bootstrap.js:2114:28)
    at Object.Module._extensions..node (internal/modules/cjs/loader.js:1057:18)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at Module.require (pkg/prelude/bootstrap.js:1719:31)
    at require (internal/modules/cjs/helpers.js:74:18)
    at bindings (/snapshot/src/node_modules/bindings/bindings.js:112:48)
    at Object.<anonymous> (/snapshot/src/node_modules/argon2/argon2.js:3:37)
    at Module._compile (pkg/prelude/bootstrap.js:1794:22)

So I changed the nodejs env variable to match the version of nodejs I saw being installed (v. 16.x) somewhere during the build. The image that resulted after that change in the Dockerfile ran fine.

Maybe you should try a build with the current Dockerfile and see if you get a working ztncui container.

@kmahyyg kmahyyg reopened this Dec 4, 2021
@kmahyyg
Copy link
Owner

kmahyyg commented Dec 4, 2021

According to your logging, it seems that this problem is not here. There must be something wrong in a deeper layer. I'll try to investigate the reason and respond ASAP. however, since I'm currently on a business trip, some delay are expected. Thanks for your understanding.

@kmahyyg kmahyyg self-assigned this Dec 4, 2021
@kmahyyg kmahyyg added the bug Something isn't working label Dec 4, 2021
@kmahyyg
Copy link
Owner

kmahyyg commented Dec 4, 2021

And also cc @key-networks to see if he could give me a hand like some explanation.

@kmahyyg
Copy link
Owner

kmahyyg commented Dec 4, 2021

@pjv could you please attach full container build log for both nodejs version number?

@pjv
Copy link
Contributor Author

pjv commented Dec 4, 2021

@kmahyyg both build logs are here.

(Line 544 in both of those logs is what made me think to change the environment variable in the Dockerfile).

For me there is no urgency about this issue; I have a working build by changing the environment variable. I just wanted to share that with anyone else who may be trying to use this repo to produce updated builds of ztncui.

Thanks again for your work here.

@kmahyyg
Copy link
Owner

kmahyyg commented Dec 4, 2021

Excellent. Things just go as what I imagined and your log confirmed this problem.

Here, even if you specified nodejs version 14, it still installed version 16. That's the root cause of this behavior.

https://gist.github.com/pjv/9a376235f5790dba63e244cb447b3964#file-nodejs_major-14-log-L561

Then all things went normal, however, here:

pkg -c ./package.json -t "node${NODEJS_MAJOR}-linux-x64" bin/www -o ztncui && \

packed all the binaries stuff or nodejs dependencies using nodejs 14, which made all dependencies inconsistent.


There're two ways to fix this problem, I personally prefer the first one, since it wouldn't break anything in theory.

  1. Debugging here:

curl -sL -o node_lts.sh https://deb.nodesource.com/setup_lts.x && \

Why this scirpt does not take corresponding environment variable? This is an unintended behavior. But it is working previously.

  1. Change the whole things into nodejs version 16. That's what you have done.

Further comments are welcome.

@kmahyyg kmahyyg mentioned this pull request Dec 4, 2021
@kmahyyg kmahyyg linked an issue Dec 4, 2021 that may be closed by this pull request
@kmahyyg
Copy link
Owner

kmahyyg commented Dec 4, 2021

I found the solution, either you or me fix it should be okay.

The following changes should be done:

  • Add detailed explanation about environment variable NODEJS_MAJOR in Docker file that this variable is only used in pkg tool, not for node.js runtime environment.
  • Replace nodejs installation script to: https://deb.nodesource.com/setup_14.x , others are the same. Also, local filename might need a change. To prevent further issues, we might need to add a local copy of this script to this git repo. Make sure to check open source license issue and attach corresponding license.
  • Rebuild and check using NodeJS 14

Copy link
Owner

@kmahyyg kmahyyg left a comment

Choose a reason for hiding this comment

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

Check latest comments in discussion of PR.

Dockerfile Outdated Show resolved Hide resolved
@pjv
Copy link
Contributor Author

pjv commented Dec 4, 2021

@kmahyyg I reverted the NODEJS_MAJOR environment variable back to 14 and updated the curl URL for the nodejs installation on line 13.

I tested this commit in my environment and it is producing a working image / container.

I did not add the detailed explanation about the environment variable because I do not think I understand well enough where it is and isn't being used, so I will leave that documentation to you whenever you have the chance to add it.

Thank you again for making this code available. It is good to be able to keep ztncui running with an up to date zerotier one node.

Copy link
Owner

@kmahyyg kmahyyg left a comment

Choose a reason for hiding this comment

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

Excellent work.

@kmahyyg kmahyyg merged commit e301f53 into kmahyyg:master Dec 5, 2021
kmahyyg added a commit that referenced this pull request Dec 5, 2021
kmahyyg added a commit that referenced this pull request Dec 5, 2021
@kmahyyg
Copy link
Owner

kmahyyg commented Dec 5, 2021

@kmahyyg I reverted the NODEJS_MAJOR environment variable back to 14 and updated the curl URL for the nodejs installation on line 13.

I tested this commit in my environment and it is producing a working image / container.

I did not add the detailed explanation about the environment variable because I do not think I understand well enough where it is and isn't being used, so I will leave that documentation to you whenever you have the chance to add it.

Thank you again for making this code available. It is good to be able to keep ztncui running with an up to date zerotier one node.

It is pretty important for us to keep zerotier up to date. Thanks again for your inquiry. And also, I here personally recommend @key-networks update the repo they have. Here's a relate ZT official post which respond to a recent vulnerability: https://www.zerotier.com/2021/09/21/incident-response-to-september-20th-2021/ .

@kmahyyg
Copy link
Owner

kmahyyg commented Dec 5, 2021

By the way, there seems some bad news here: key-networks/ztncui#79

I might need to upgrade to nodejs version 16 later :(

@pjv pjv deleted the patch-1 branch December 5, 2021 10:56
key-networks added a commit to key-networks/ztncui-aio that referenced this pull request Dec 20, 2021
* fix constant seed issue before generate password

* fix #6: remove gosu due to strict perm check and use fixed debian version

* fix #6: add custom docker image hosted by github

* Update Dockerfile (kmahyyg#7)

* Change pkg to nodejs 16.x to successfully build packed ZTNCUI

* Switch back to correct nodejs 14.x installation script

* docs(README.md): update nodejs environment note

Related to PR kmahyyg#7

* chore(node_lts.sh): backup nodejs v14 env install script

Related to kmahyyg#7

* update nodejs installation script

* update to node 16

* update golang builder to bullseye

* update readme

* add explanation about public IP detection

* readme.md: fix typo

Co-authored-by: Patrick Young <16604643+kmahyyg@users.noreply.github.com>
Co-authored-by: pjv <pjv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trying to fix PR #7
2 participants