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

[Merged by Bors] - Creates a new lighthouse user and makes it the default user to be use… #1502

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Creates a new lighthouse user and makes it the default user to be use… #1502

wants to merge 2 commits into from

Conversation

b-m-f
Copy link
Contributor

@b-m-f b-m-f commented Aug 10, 2020

…d in the Docker image

Issue Addressed

#1459

Proposed Changes

  • Create new lighthouse user and group in Docker container
  • Set user as the default user

@acequicksilver
Copy link

Do the startup scripts in lighthouse-docker need to change to reflect the new lighthouse user?

@b-m-f
Copy link
Contributor Author

b-m-f commented Aug 11, 2020

@acequicksilver they should not need any changes as far as I can see.

I ran docker exec -ti lighthouse /bin/bash and was logged in as the lighthouse user, confirmed with running id.
This should be the case for the scripts as well.

@acequicksilver
Copy link

start-beacon-node.sh references the root home directory --network-dir /root/network and there will be some references in the docker compose-compose.yml

@b-m-f
Copy link
Contributor Author

b-m-f commented Aug 11, 2020 •
edited
Loading

@acequicksilver Good catch. Could simply change it to the environment variabale $HOME. Will update later today.

For the docker-compose I would open another PR if this one gets approved

@b-m-f
Copy link
Contributor Author

b-m-f commented Aug 12, 2020

@acequicksilver I cant find the reference you mention.

Updated the relevant part in the book though.

If this PR lands I will be happy to update lighthouse-docker

@acequicksilver
Copy link

I should have included a link. See below

https://github.com/sigp/lighthouse-docker/blob/master/docker-compose.yml

https://github.com/sigp/lighthouse-docker/blob/master/scripts/start-beacon-node.sh

Thanks

@b-m-f
Copy link
Contributor Author

b-m-f commented Aug 12, 2020

@acequicksilver Okay, but this is unrelated to this repository. That shoudnt be touched before this change is actually included here I believe.

AgeManning
AgeManning approved these changes Aug 31, 2020
View reviewed changes
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

This seems good to me.

Better practice to use non-root users in docker imo.

@AgeManning
Copy link
Member

AgeManning commented Aug 31, 2020 •
edited
Loading

I'll update the docker-compose scripts once this gets merged in also and the image gets uploaded (if you don't see this time and want to do it yourself)

Thanks guys!

@AgeManning
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 31, 2020
@b-m-f
Creates a new lighthouse user and makes it the default user to be use… (
0df0687 
#1502)

…d in the Docker image

## Issue Addressed
#1459

## Proposed Changes

- Create new `lighthouse` user and group in Docker container
- Set user as the default user
@bors
Copy link

bors bot commented Aug 31, 2020

Build failed:

@b-m-f
Copy link
Contributor Author

b-m-f commented Aug 31, 2020 •
edited
Loading

@AgeManning is the CI test failure related to changes in here?

---- eth1_cache::simple_scenario stdout ----
thread 'eth1_cache::simple_scenario' panicked at 'should start eth1 environment: "Timed out waiting for ganache to start. Is ganache-cli installed?"', beacon_node/eth1/tests/test.rs:111:18


failures:
    deposit_tree::cache_consistency
    deposit_tree::double_update
    deposit_tree::updating
    eth1_cache::big_skip
    eth1_cache::double_update
    eth1_cache::pruning
    eth1_cache::simple_scenario
    fast::deposit_cache_query

@michaelsproul
Copy link
Member

@b-m-f Nah, unfortunately the macOS VMs are a bit flaky and occasionally seem to timeout

bors retry

bors bot pushed a commit that referenced this pull request Aug 31, 2020
@b-m-f
Creates a new lighthouse user and makes it the default user to be use… (
7d71d98 
#1502)

…d in the Docker image

## Issue Addressed
#1459

## Proposed Changes

- Create new `lighthouse` user and group in Docker container
- Set user as the default user
@bors
Copy link

bors bot commented Aug 31, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title Creates a new lighthouse user and makes it the default user to be use… [Merged by Bors] - Creates a new lighthouse user and makes it the default user to be use… Aug 31, 2020
@bors bors bot closed this Aug 31, 2020
AgeManning added a commit that referenced this pull request Sep 1, 2020
@AgeManning
Revert "Creates a new lighthouse user and makes it the default user t…
f404ad4 
…o be use… (#1502)"

This reverts commit 7d71d98.
@AgeManning AgeManning mentioned this pull request Sep 1, 2020
Closed
bors bot pushed a commit that referenced this pull request Sep 1, 2020
@AgeManning
Revert 1502 - Switching docker user to lighthouse (#1578)
8301a98 
## Issue Addressed

The lighthouse user has recently changed to `lighthouse` from root. 

This requires uses to change ownership of their current docker mounted volumes and the upgrade path is non-trivial. 
This reverts #1502 and we will include it in a major release in the future.

## Proposed Changes

N/A

## Additional Info

N/A
darcys22 pushed a commit to darcys22/lighthouse that referenced this pull request Sep 5, 2020
@b-m-f @darcys22
Creates a new lighthouse user and makes it the default user to be use… (
363f297 
sigp#1502)

…d in the Docker image

## Issue Addressed
sigp#1459

## Proposed Changes

- Create new `lighthouse` user and group in Docker container
- Set user as the default user
darcys22 pushed a commit to darcys22/lighthouse that referenced this pull request Sep 5, 2020
@AgeManning @darcys22
Revert 1502 - Switching docker user to lighthouse (sigp#1578)
7c87b87 
## Issue Addressed

The lighthouse user has recently changed to `lighthouse` from root. 

This requires uses to change ownership of their current docker mounted volumes and the upgrade path is non-trivial. 
This reverts sigp#1502 and we will include it in a major release in the future.

## Proposed Changes

N/A

## Additional Info

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgeManning AgeManning AgeManning approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@b-m-f @acequicksilver @AgeManning @michaelsproul