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

Protect against spawn of CLI for wrong architecture #1317

Closed
danielkhan opened this issue Aug 31, 2022 · 22 comments
Closed

Protect against spawn of CLI for wrong architecture #1317

danielkhan opened this issue Aug 31, 2022 · 22 comments

Comments

@danielkhan
Copy link

Problem

The CLI is rarely used standalone. In most cases, it is executed via intermediates like the Sentry Webpack plugin.

When the CLI is installed via npm, it will run an install script that downloads a binary for the current architecture.

There are scenarios, when the architecture where the CLI eventually runs differs from the one npm install was executed from.

E.g.

  • A Dockerfile that will simply copy the local node_modules into the container
  • Any other deployment methods that will simply ship the bundle to the destination system including node_modules

This can lead to a rather obscure error when the binary is executed/spawned: #0 26.19 Sentry CLI Plugin: spawn Unknown system error -8.
I could reproduce this problem by running npm install @sentry/nextjs on an M1 Mac before running the application in Docker (Alpine).

The problem was reported by other users as well and we didn't come up with a solution proposal yet:

Quick fix

The problem can be solved by running

  • npm uninstall <sentry_module_that_installs_the_CLI_as_dep>
  • followed by npm install <sentry_module_that_installs_the_CLI_as_dep> as part of the deployment on the target system.

For a next.js application running in Docker, this would mean adding the following to the Dockerfile:

RUN npm uninstall -S @sentry/nextjs
RUN npm install -S @sentry/nextjs

# Further commands below, like:
# RUN npm run build

This ensures that the right binary is downloaded for the target system.

Product improvement

Sentry should detect that the binary doesn't match the current architecture and provide an actionable error message to the user.
E.g.

Wrong architecture detected while trying to execute the Sentry CLI. Please make sure to do a fresh npm uninstall/install step for @sentry modules on the target system.
@kamilogorek
Copy link
Contributor

We only need to do this for UNIX systems, as for windows .exe should be always compiled for windows.
In this case, we can most likely use file sentry-cli --brief or file sentry-cli --mime-type and match on the output.

@samueldusek
Copy link

Hi, is there any progress on this issue?

I am still getting the error: Sentry CLI Plugin: spawn Unknown system error -8 when building the nextjs app in the docker node:18-alpine image.

I tried to uninstall the @sentry/nextjs package and install back as you suggested but it did not help.

@kamilogorek
Copy link
Contributor

No, not really. As for your issue, can you run file sentry-cli and/or file node_modules/@sentry/cli/sentry-cli inside your docker image and provide logs?

@danielkhan
Copy link
Author

danielkhan commented Sep 15, 2022

@samueldusek In your Dockerfile, try adding

RUN npm uninstall --save @sentry/nextjs
RUN npm install --save @sentry/nextjs

This works for me as it will make sure that you get the right CLI for the platform your container runs on.

@samueldusek
Copy link

In your Dockerfile, try adding

RUN npm unininstall --save @sentry/nextjs
RUN npm install --save @sentry/nextjs

This works for me as it will make sure that you get the right CLI for the platform your container runs on.

@danielkhan As I mentioned in my first comment. I did try it and it did not help.

@danielkhan
Copy link
Author

@samueldusek what happens if you remove node_modules altogether before spinning up the container?

@samueldusek
Copy link

@kamilogorek This is the output from the file command you suggested.

#22 [deps 15/15] RUN file node_modules/@sentry/cli/sentry-cli
#22 sha256:4cf4b1d85d4779ee88fec1a6be0b8f9a5cb593fd6ec871aef5664aa7e3a5d389
#22 1.481 node_modules/@sentry/cli/sentry-cli: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, with debug_info, not stripped
#22 DONE 1.6s

This is how I am trying to install dependencies for my app in the docker file.

COPY package.json yarn.lock ./
RUN yarn install
RUN yarn remove @sentry/nextjs
RUN yarn add @sentry/nextjs

RUN file node_modules/@sentry/cli/sentry-cli

@samueldusek
Copy link

what happens if you remove node_modules altogether before spinning up the container?

@danielkhan The image building process fails do to the error mentioned above when running

RUN yarn next build

So I am not even able to build my app due to the sentry-cli error. 😞

Just to mention.. When I am building the app on my local machine I have no issue.

@danielkhan
Copy link
Author

What happens if you remove Sentry from your local install and then docker exec into the image and run the yarn install manually, @samueldusek?

@samueldusek
Copy link

What happens if you remove Sentry from your local install and then docker exec into the image and run the yarn install manually, @samueldusek?

@danielkhan Well, I tried to remove Sentry from my local install as you suggested and it started to work. But I do not think that it is a solution as I cannot build the app on my local machine anymore.

There should be a solution I can build the app both, localy and in docker as well without any issues. 😞

@danielkhan
Copy link
Author

@samueldusek this is more for the purpose of triaging the problem.
If it works in Docker then, this means that the right CLI can be installed and something just prevents this from happening.
Would you mind posting the respective Dockerfile?

@ekrokowski
Copy link

@samueldusek If you use the similar to the one of the nextjs-example (https://github.com/vercel/next.js/blob/canary/examples/with-docker/Dockerfile), you have to execute

RUN yarn remove @sentry/nextjs
RUN yarn add @sentry/nextjs

after

COPY --from=deps /app/node_modules ./node_modules
COPY . .

If you do it after the RUN yarn install --frozen-lockfile but before the COPY . ., the COPY . . will actually copy your local node_modules and overwrite the one in the container.
That could be the reason why you still have that error even after using the remove/add method.

@umuralpay
Copy link

@samueldusek If you use the similar to the one of the nextjs-example (https://github.com/vercel/next.js/blob/canary/examples/with-docker/Dockerfile), you have to execute

RUN yarn remove @sentry/nextjs
RUN yarn add @sentry/nextjs

after

COPY --from=deps /app/node_modules ./node_modules
COPY . .

If you do it after the RUN yarn install --frozen-lockfile but before the COPY . ., the COPY . . will actually copy your local node_modules and overwrite the one in the container. That could be the reason why you still have that error even after using the remove/add method.

That fixed my problem while running docker-compose build . The build process was exiting with an error Sentry CLI Plugin: spawn Unknown system error -8

@smeubank
Copy link
Member

smeubank commented Nov 9, 2022

@vladanpaunovic and @kamilogorek is this issue something we can pull back up and discuss if there is a way forward?

@kamilogorek
Copy link
Contributor

I don't believe there's an easy way out of this, as this is docker build misconfiguration, and we cannot control the user-land.
I'm more than happy to hear some suggestions though.

@danielkhan
Copy link
Author

Goals:

  1. Don't error out like that by accessing a file that is not for the given arch - at least give the user a proper error message.
  2. Automatically ensure that the right file is being downloaded if it isn't present.

Would it be possible to download and store the respective binaries in a directory structure that contains the arch /cli/<arch>/foo.xyz? With that, checking for the right binary and maybe even downloading the right one just in time should be fairly trivial.

@umuralpay
Copy link

Changing

COPY --from=deps /app/node_modules ./node_modules
COPY . .

to

COPY . .
COPY --from=deps /app/node_modules ./node_modules

fixes the issue and you don't need to remove and install sentry again

@kamilogorek
Copy link
Contributor

Would it be possible to download and store the respective binaries in a directory structure that contains the arch /cli//foo.xyz? With that, checking for the right binary and maybe even downloading the right one just in time should be fairly trivial.

That would definitely be possible, but we have lots of other tools that rely on the fact that sentry-cli{.exe} binary lives in the root of the installed package. And changing it now would definitely be not trivial. We'd have to play around with symlinking correct file, that in itself can be sometimes problematic.
It is one of possible solutions though.

@LegoKristoffer
Copy link

RUN npm unininstall --save @sentry/nextjs
RUN npm install --save @sentry/nextjs

We aware that there's a typo in the first line. It should be

RUN npm uninstall --save @sentry/nextjs
RUN npm install --save @sentry/nextjs

@kamilogorek
Copy link
Contributor

Oh, nice catch! Update the comment, thanks

@dmitrika
Copy link

Another option, without swapping copy/install is to add .dockerignore file and ignore node_modules there. This way docker won't copy that dir.

#.dockerignore
node_modules

@szokeasaurusrex
Copy link
Member

We believe #1836 should likely also resolve this issue. We are closing this issue for now; if anyone continues experiencing this problem, please reopen!

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

Successfully merging a pull request may close this issue.

9 participants