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

Move Connect build to a new Docker container #27175

Merged
merged 4 commits into from
Jun 6, 2023
Merged

Conversation

jakule
Copy link
Contributor

@jakule jakule commented May 31, 2023

This PR moves the Connect build to a new docker image. This will allow us to use the different base image for Connect and our CI, as at that point, the buildbox will be used only by our CI.

Dev build: https://drone.platform.teleport.sh/gravitational/teleport/24668/5/4

After this PR is merged, I'll create another PR upgrading the buildbox to use ubuntu:latest. I want to keep them separate for a cleaner history.

Contributes to #26856

@jakule jakule marked this pull request as ready for review June 1, 2023 07:06
@github-actions github-actions bot requested review from fspmarshall and mdwn June 1, 2023 07:07
@jakule jakule removed request for mdwn and fspmarshall June 1, 2023 07:07
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

LGTM.

Nice, this should be a lot faster to build and easier to maintain.

build.assets/Dockerfile-connect Outdated Show resolved Hide resolved
build.assets/Dockerfile-connect Outdated Show resolved Hide resolved
build.assets/Makefile Outdated Show resolved Hide resolved
Remove unused packages and unused arguments
@ravicious
Copy link
Member

I tried to run this locally. make -C build.assets teleterm fails when doing yarn install because some Electron script tries to save something to a global node_modules folder (instead of the default local location) and the global folder is not writable.

Error log
[4/4] Building fresh packages...
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
warning Cannot find a suitable global folder. Tried these: "/usr/local, /.yarn"
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
error /go/src/github.com/gravitational/teleport/node_modules/electron: Command failed.
Exit code: 1
Command: node install.js
Arguments:
Directory: /go/src/github.com/gravitational/teleport/node_modules/electron
Output:
Error: EACCES: permission denied, mkdir '/.cache'
make: *** [teleterm] Error 1

I also get this message:

warning Skipping preferred cache folder "/.cache/yarn" because it is not writable.
warning Selected the next writable cache folder in the list, will be "/tmp/.yarn-cache-501".

I think it might all be related to the -u flag that we're passing to docker run, -u $(id -u):$(id -g). All the folders set up by the node image are set up for the node user but -u causes the command to run as a different user perhaps?

@codingllama
Copy link
Contributor

If we can drop custom users shenanigans for the connect image, +1 to it.

@jakule
Copy link
Contributor Author

jakule commented Jun 6, 2023

I tried to run this locally. make -C build.assets teleterm fails when doing yarn install because some Electron script tries to save something to a global node_modules folder (instead of the default local location) and the global folder is not writable.

Error log
I also get this message:

warning Skipping preferred cache folder "/.cache/yarn" because it is not writable.
warning Selected the next writable cache folder in the list, will be "/tmp/.yarn-cache-501".

I think it might all be related to the -u flag that we're passing to docker run, -u $(id -u):$(id -g). All the folders set up by the node image are set up for the node user but -u causes the command to run as a different user perhaps?

@ravicious Thanks for checking that. I think that this image "was designed" only to work as node user with UID=1000. I hardcoded the value 😐

@jakule
Copy link
Contributor Author

jakule commented Jun 6, 2023

If we can drop custom users shenanigans for the connect image, +1 to it.

@codingllama I don't think we can. We use that image on Drone. We should not run this image as root for a few reasons:

  1. Artifacts generated as root can't be removed, copied etc. AFAIR Drone runs as a non-root user.
  2. Potential privilege escalation and other security-related issues
  3. This is something to check, but this image comes with the assumption that you will use it as a node user with UID=1000. I'm not sure what are the consequences of using root.

In this case, I vote for just hardcoding the UID. It will work on macs, as the Mac driver re-map users anyway. On Linux everyone should be fine, as of course everyone builds on Linux as the default user 🙈

@jakule
Copy link
Contributor Author

jakule commented Jun 6, 2023

Currently, I'm getting some x86/arm issues on MacOS

  ⨯ cannot execute  cause=exit status 1
                    errorOut=/home/node/.cache/electron-builder/fpm/fpm-1.9.3-2.3.1-linux-x86/lib/ruby/bin/ruby: line 6: /home/node/.cache/electron-builder/fpm/fpm-1.9.3-2.3.1-linux-x86/lib/ruby/bin.real/ruby: cannot execute binary file: Exec format error
    /home/node/.cache/electron-builder/fpm/fpm-1.9.3-2.3.1-linux-x86/lib/ruby/bin/ruby: line 6: /home/node/.cache/electron-builder/fpm/fpm-1.9.3-2.3.1-linux-x86/lib/ruby/bin.real/ruby: Success

I'll take a look at this tomorrow, but looks like this is a common problem: jordansissel/fpm#1801

@codingllama
Copy link
Contributor

@jakule hardcoding the UID seems alright to me, as at least we have a deterministic user. It's the "binding the local user" shenanigans that I tend to dislike. (Of course, not having to bother with users at all would be the ideal.)

@ravicious
Copy link
Member

Re fpm issues, I just checked and it looks like make -C build.assets teleterm has never actually worked on arm64 Macs.

I completely forgot about it when I was writing my comment, but whenever we build Connect in Linux VMs to test stuff, we have to remember about setting USE_SYSTEM_FPM=1. I'm fine with not addressing this in this PR.

Copy link
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Tested the dev build on Ubuntu 20.04, no problems.

I really like the idea of using node image as a base.

@jakule jakule added this pull request to the merge queue Jun 6, 2023
Merged via the queue into master with commit eb4acdd Jun 6, 2023
@ravicious
Copy link
Member

I really like the idea of using node image as a base.

Oh yeah, I forgot to give a thumbs up to this. I feel like 80% of build failures of Connect is caused by the Node.js CDN returning an error.

jakule added a commit that referenced this pull request Jun 6, 2023
After moving the Connect to a separate Docker image #27175 we're able to use the latest ubuntu LTS on our build image. We're not using this image to produce any releases (only CI runs), so updating the image will have no effect on our releases.
jakule added a commit that referenced this pull request Jun 6, 2023
* Move Connect build to a new Docker container

* Update comments

* Update comments
Remove unused packages and unused arguments

* Always use UID=1000 for building teleterm.
jakule added a commit that referenced this pull request Jun 6, 2023
* Move Connect build to a new Docker container

* Update comments

* Update comments
Remove unused packages and unused arguments

* Always use UID=1000 for building teleterm.
jakule added a commit that referenced this pull request Jun 7, 2023
* Move Connect build to a new Docker container

* Update comments

* Update comments
Remove unused packages and unused arguments

* Always use UID=1000 for building teleterm.
jakule added a commit that referenced this pull request Jun 7, 2023
After moving the Connect to a separate Docker image #27175 we're able to use the latest ubuntu LTS on our build image. We're not using this image to produce any releases (only CI runs), so updating the image will have no effect on our releases.
jakule added a commit that referenced this pull request Jun 8, 2023
* Update base Ubuntu image to 20.04 (#26905)

* Update base Ubuntu image to 22.04

* Revert the ubuntu image to 20.04

* Update the Dockerfile comment

* Add CentOS 7 note

* Add Connect note

* Move Connect build to a new Docker container (#27175)

* Move Connect build to a new Docker container

* Update comments

* Update comments
Remove unused packages and unused arguments

* Always use UID=1000 for building teleterm.
github-merge-queue bot pushed a commit that referenced this pull request Jun 22, 2023
* Update base Ubuntu image to 20.04 (#26905)

* Update base Ubuntu image to 22.04

* Revert the ubuntu image to 20.04

* Update the Dockerfile comment

* Add CentOS 7 note

* Add Connect note

* Move Connect build to a new Docker container (#27175)

* Move Connect build to a new Docker container

* Update comments

* Update comments
Remove unused packages and unused arguments

* Always use UID=1000 for building teleterm.
github-merge-queue bot pushed a commit that referenced this pull request Jun 22, 2023
* Update base Ubuntu image to 20.04 (#26905)

* Update base Ubuntu image to 22.04

* Revert the ubuntu image to 20.04

* Update the Dockerfile comment

* Add CentOS 7 note

* Add Connect note

* Move Connect build to a new Docker container (#27175)

* Move Connect build to a new Docker container

* Update comments

* Update comments
Remove unused packages and unused arguments

* Always use UID=1000 for building teleterm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants