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.

Sign up for GitHub

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

Jump to bottom

Provide Node.js for running VS Code #471

Closed
wants to merge 1 commit into from
Closed

Provide Node.js for running VS Code #471

wants to merge 1 commit into from

Conversation

RomanNikitenko
Copy link
Contributor

Provide Node.js for running VS Code.
We need this change for eclipse-che/che#21778

@RomanNikitenko
Provide Node.js for running VS Code
4e93b51 
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
RomanNikitenko
RomanNikitenko commented Jul 6, 2023
View reviewed changes
devspaces-code/build/dockerfiles/libc-content-provider.Dockerfile
if [ -n "$NODE_ARCH" ]; then \
NODE_URL="${NODE_LOCATION}/dist/v${NODE_VERSION}/node-v${NODE_VERSION}-${PLATFORM}-${NODE_ARCH}.tar.gz"; \
echo "Downloading Node.js from ${NODE_URL}"; \
wget -q "${NODE_URL}"; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickboldt
I would like to know your opinion about current PR.
We need it for eclipse-che/che#21778.

The main difference is: Node.js is going to be downloaded at the building libc-content-provider.Dockerfile step.

Copy link
Member

@nickboldt nickboldt Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Node.js is going to be downloaded at the building

Sounds like the opposite of what was done for https://issues.redhat.com/browse/CRW-3160 ... you can't download random stuff from the internet anymore, you can only use rpms :(

Do you require a specific version of nodejs that's not available from UBI 8 or UBI 9 rpm content sets? If so the workaround is to use fetch-artifacts-url.yaml with content we have pre-uploaded to download.devel server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the quick answer!

Do you require a specific version of nodejs that's not available from UBI 8 or UBI 9 rpm content sets?

ubi-8 brings nodejs that incompatible with ubi-9.
I tested in the che-code that starting VS Code works well for both (ubi-8 and ubi-9) when Node.js is downloaded...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more question:
in the upstream we can use From docker.io/node:16.17.1-alpine3.15

If I'm not mistaken we can not use From docker.io/node:16.17.1-alpine3.15 for the Devspaces-Code.
Do you know a workaround for such use case?

Copy link
Member

@nickboldt nickboldt Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Correct.

You can use any of UBI8 based images in https://catalog.redhat.com/software/containers/search?q=ubi8/nodejs-16

If you need a RHEL9-based image for downstream we'll probably have to migrate to UBI9 as the deployment layer.

I'm not sure if you can BUILD with RHEL9/UBI9 and then set the runtime/deployment later to be FROM UBI8. That might break the build pipeline in Brew/OSBS.

Side question: why are we using Node 16 when 18 is available?

https://catalog.redhat.com/software/containers/search?q=nodejs-18&p=1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side question: why are we using Node 16 when 18 is available?

It's VS Code requirement: https://github.com/microsoft/vscode/wiki/How-to-Contribute#prerequisites

Copy link
Member

@nickboldt nickboldt Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ok, well ubi8/nodejs-16 contains nodejs-16.19.1-1.module+el8.7.0+18373+704f5cef.x86_64 ... which should be compatible with required Node.JS, x64, version >=16.17.x and <17

Meanwhile the ubi9/nodejs-16 container has nodejs-16.19.1-2.el9_2.x86_64

Are you saying these versions don't work for your build?

https://catalog.redhat.com/software/containers/ubi8/nodejs-16/615aee9fc739c0a4123a87e1?tag=1-72.1669834625&push_date=1669927389000 contains 16.17.1 but it's 6mo old and is rated D because of security issues. NOT recommended to use this older version of nodejs 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying these versions don't work for your build?

Let's say I've built libc docker image from the dockerfile based on the ubi-8 - it contains Node.js 16 that requires OpenSSL 1.

  • we run VS Code in the ubi-8 based container - it works - no problem!
  • we run VS Code in the ubi-9 based container - it doesn't work - ubi-9 image uses OpenSSL 3

The same problem when I build libc docker image from the dockerfile based on the ubi-9, but user tries to run VS Code in the ubi-8 based container!

I don't have such problems when I just download required version of Node.js - the workspace can be started in both: ubi-8 and ubi-9 based containers...

@nickboldt
Copy link
Member

I wonder if the libc build can be set up to NOT directly depend on a specific openssl implementation, or to include its own embedded one?

I don't have such problems when I just download required version of Node.js

Are you downloading the 66MB node-v16.20.1.tar.gz or the 33MB node-v16.20.1-linux-x64.tar.gz ?

Looks like the larger tarball includes:

  • node-v16.20.1/deps/openssl/
  • node-v16.20.1/deps/openssl/openssl/util/opensslwrap.sh
  • arch-specific config, header, and node-gyp files

But the smaller one only includes:

  • node-v16.20.1-linux-x64/include/node/openssl/ *.h headers (for multiple arches, which is odd for an arch-specific tarball (?))

So... maybe it's the openssl wrapper that's solving your problem for RHEL 8 vs. 9? Both tarballs include these:

  • node-v16.20.1/deps/openssl/config/archs/linux-x86_64/no-asm/include/openssl/opensslconf.h

  • node-v16.20.1/deps/openssl/config/archs/linux-x86_64/asm_avx2/include/openssl/opensslconf.h

  • node-v16.20.1/deps/openssl/config/archs/linux-x86_64/asm/include/openssl/opensslconf.h

  • node-v16.20.1-linux-x64/include/node/openssl/archs/linux-x86_64/no-asm/include/openssl/opensslconf.h

  • node-v16.20.1-linux-x64/include/node/openssl/archs/linux-x86_64/asm_avx2/include/openssl/opensslconf.h

  • node-v16.20.1-linux-x64/include/node/openssl/archs/linux-x86_64/asm/include/openssl/opensslconf.h

Alternatively, perhaps it's time to migrate Dev Spaces to RHEL 9 so we don't have to worry about RHEL 8 anymore. (This will take a few releases to achieve, however, but is ultimately where DS will end up.)

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Jul 7, 2023 •
edited
Loading

Are you downloading the 66MB node-v16.20.1.tar.gz or the 33MB node-v16.20.1-linux-x64.tar.gz ?

https://nodejs.org/dist/v16.17.1/node-v16.17.1-linux-x64.tar.gz

The process of running VS Code is:

  • we build an image that contains VS Code assembly and Node.js to run VS Code
  • we copy the assembly + Node.js to another tools container when a workspace is starting
  • we use that cached Node.js to run VS Code in the tools container

As far as I understand the problem:

  • the cached Node.js depends on some libraries
  • there is no problem when the libs are present in the tools container
  • there is a problem when something is absent

Currently we copy already installed node binary file from the /usr/bin/node, you can see it here, the Node.js dynamically links to the OpenSSL shared libraries installed on the system:

openssl_dynamic

If the libs are absent in the tools container - Node.js fails with the error, like:

./node: error while loading shared libraries: libcrypto.so.1.1: cannot open shared object file: No such file or directory

My idea was to download Node.js (instead of copying already installed one in the libc image).
For such case Node.js statically links against OpenSSL, it means that Node.js includes a built-in version of the OpenSSL library as part of its binary distribution:

openssl_static

You can see that:

  • I use ubi-9 based tools container
  • the system uses OpenSSL 3.0.7
  • Node.js statically links against OpenSSL 1.1.1q+quic
  • there are no libs related to OpenSSL when I do ldd node (please compare with the previous use case)

@nickboldt
Copy link
Member

nickboldt commented Jul 17, 2023 •
edited
Loading

Note that a dependency on libssl or libcrypto indicates that there are FIPS implications here. We need to be careful that the node we're using is FIPS-friendly. Static links are generally not preferred over dynamic links, if I understand correcty based on the recent discussions about golang + FIPS.

See https://issues.redhat.com/browse/CRW-4582?focusedId=22617302&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-22617302

@RomanNikitenko RomanNikitenko deleted the provide_nodejs_for_vscode branch January 25, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickboldt nickboldt nickboldt left review comments

@svor svor Awaiting requested review from svor svor will be requested when the pull request is marked ready for review svor is a code owner

@ibuziuk ibuziuk Awaiting requested review from ibuziuk ibuziuk will be requested when the pull request is marked ready for review ibuziuk is a code owner

@SDawley SDawley Awaiting requested review from SDawley SDawley will be requested when the pull request is marked ready for review SDawley is a code owner

Assignees
No one assigned
Labels
do-not-merge/work-in-progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RomanNikitenko @nickboldt