LBRYCommunity/lbry-docker
1
0
Fork 0
Code Issues 2 Pull requests 2 Projects Releases 1 Packages Wiki Activity

Make config mountable #46

Closed
EnigmaCurry wants to merge 1 commit from cloud-init-new into master
pull from: cloud-init-new
merge into: LBRYCommunity:master
Conversation 16 Commits 1 Files changed 2 +22 -18
EnigmaCurry commented 2019-04-16 06:24:40 +02:00 (Migrated from github.com)
Copy link

This is an adaptation on the config style. In the case the user wants to mount his own config file, he can do so. If he does not, the environment variables are used instead to create a default config.

  • The config file is moved from /data/.lbrycrd/lbrycrd.conf to /etc/lbry/lbrycrd.conf, this way we know we are starting fresh each time, either provided config, or new config.
  • Got rid of su and use the USER directive in the Dockerfile instead.

With these changes I can fix #45

This is an adaptation on the config style. In the case the user wants to mount his own config file, he can do so. If he does not, the environment variables are used instead to create a default config. * The config file is moved from /data/.lbrycrd/lbrycrd.conf to /etc/lbry/lbrycrd.conf, this way we know we are starting fresh each time, either provided config, or new config. * Got rid of su and use the USER directive in the Dockerfile instead. With these changes I can fix #45
Leopere commented 2019-04-16 17:45:31 +02:00 (Migrated from github.com)
Copy link

I've added a few notes for the various design choices I'd made and why I think we should keep the original behavior in some aspects, but this is a handy addition.

I wasn't going to bother making the config path dynamic within the container, but it could add flexibility for people if they need it.

I've added a few notes for the various design choices I'd made and why I think we should keep the original behavior in some aspects, but this is a handy addition. I wasn't going to bother making the config path dynamic within the container, but it could add flexibility for people if they need it.
Leopere (Migrated from github.com) reviewed 2019-04-16 17:54:42 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
Leopere (Migrated from github.com) commented 2019-04-16 17:42:45 +02:00
Copy link

I opted to run this as root and then a privilege de-escalation using su -c {invocation} lbrycrd so that it wouldn't matter what the user did to the file permissions we could re-assert them properly prior to execution.

I opted to run this as root and then a privilege de-escalation using `su -c {invocation} lbrycrd` so that it wouldn't matter what the user did to the file permissions we could re-assert them properly prior to execution.
lbrycrd/stuff/start.sh
@ -9,0 +9,4 @@
# The config file does not exist in the container image. It must be mounted, or
# if not, a default config is generated using environment variables.
CONFIG_PATH=/etc/lbry/lbrycrd.conf
if [ -f "$CONFIG_PATH" ]
Leopere (Migrated from github.com) commented 2019-04-16 17:53:17 +02:00
Copy link

CONFIG_PATH=${CONFIG_PATH:-/etc/lbry/lbrycrd.conf} would let you set a default value for the config path but allow an override if there was an environment variable set.

Otherwise you're likely just going to overwrite the variable when the bash hits line 11

CONFIG_PATH=${CONFIG_PATH:-/etc/lbry/lbrycrd.conf} would let you set a default value for the config path but allow an override if there was an environment variable set. Otherwise you're likely just going to overwrite the variable when the bash hits line 11
lbrycrd/stuff/start.sh
@ -9,0 +23,4 @@
echo "rpcbind=0.0.0.0" >> $CONFIG_PATH
#echo "bind=0.0.0.0" >> $CONFIG_PATH
fi
Leopere (Migrated from github.com) commented 2019-04-16 17:39:45 +02:00
Copy link

Gosh I forgot that I'd not done this for lbrycrd my priority at the time was Chainquery. Thank you for doing this.

Gosh I forgot that I'd not done this for lbrycrd my priority at the time was Chainquery. Thank you for doing this.
lbrycrd/stuff/start.sh
Leopere (Migrated from github.com) commented 2019-04-16 17:40:49 +02:00
Copy link

So the one reason I kept this as the default behavior is when you run this script as root to launch it you gain the luxury of being able to enforce the file ownership and permissions of the config.

You then de-escalate privileges in order to run the actual application and gain the same benefits of a unpriv'd user.

So the one reason I kept this as the default behavior is when you run this script as root to launch it you gain the luxury of being able to enforce the file ownership and permissions of the config. You then de-escalate privileges in order to run the actual application and gain the same benefits of a unpriv'd user.
lbrycrd/stuff/start.sh
Leopere (Migrated from github.com) commented 2019-04-16 17:43:08 +02:00
Copy link

I like this being an option thanks for the value add!

I like this being an option thanks for the value add!
EnigmaCurry (Migrated from github.com) reviewed 2019-04-16 19:49:48 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
EnigmaCurry (Migrated from github.com) commented 2019-04-16 19:49:48 +02:00
Copy link

Ah that does make sense. I had a problem with it however:

su : must be run from a terminal

I really don't know what causes it though.

Ah that does make sense. I had a problem with it however: ``` su : must be run from a terminal ``` I really don't know what causes it though.
EnigmaCurry (Migrated from github.com) reviewed 2019-04-16 19:51:36 +02:00
lbrycrd/stuff/start.sh
@ -9,0 +9,4 @@
# The config file does not exist in the container image. It must be mounted, or
# if not, a default config is generated using environment variables.
CONFIG_PATH=/etc/lbry/lbrycrd.conf
if [ -f "$CONFIG_PATH" ]
EnigmaCurry (Migrated from github.com) commented 2019-04-16 19:51:35 +02:00
Copy link

This is the path inside the container, not outside. So I don't think it needs to be configurable, does it?

This is the path inside the container, not outside. So I don't think it needs to be configurable, does it?
EnigmaCurry (Migrated from github.com) reviewed 2019-04-16 19:53:37 +02:00
lbrycrd/stuff/start.sh
EnigmaCurry (Migrated from github.com) commented 2019-04-16 19:53:37 +02:00
Copy link

happy to leave it the way it is if I can solve the su : must be run from a terminal problem I have.

happy to leave it the way it is if I can solve the `su : must be run from a terminal` problem I have.
Leopere (Migrated from github.com) reviewed 2019-04-16 19:56:19 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
Leopere (Migrated from github.com) commented 2019-04-16 19:56:19 +02:00
Copy link

I'm glancing around for any other potentially related issues to see what solutions may be for this however if you find one before I do feel free to tweak appropriately.

I'm glancing around for any other potentially related issues to see what solutions may be for this however if you find one before I do feel free to tweak appropriately.
Leopere commented 2019-04-16 19:58:24 +02:00 (Migrated from github.com)
Copy link

I'd been going through and linting the Dockerfile and this is roughly the direction I was heading in based on your last feedback.

FROM ubuntu:18.04 as prep
LABEL MAINTAINER="leopere [at] nixc [dot] us"
## TODO: Implement version pinning. `apt-get install curl=<version>`
RUN apt-get update && \
    apt-get -y install unzip curl && \
    apt-get autoclean -y && \
    rm -rf /var/lib/apt/lists/*
WORKDIR /
SHELL ["/bin/bash", "-o", "pipefail", "-c"]
RUN curl -L -o ./lbrycrd-linux.zip $(curl -s https://api.github.com/repos/lbryio/lbrycrd/releases | grep -F 'lbrycrd-linux.zip' | grep download | head -n 1 | cut -d'"' -f4) && \
    unzip ./lbrycrd-linux.zip && \
    chmod +x ./lbrycrdd ./lbrycrd-cli ./lbrycrd-tx

FROM ubuntu:18.04 as app
RUN addgroup --gid 1000 lbrycrd && \
    adduser lbrycrd --uid 1000 --gid 1000 --gecos GECOS --shell /bin/bash --disabled-password --home /data
COPY --from=prep /lbrycrdd /usr/bin
COPY --from=prep /lbrycrd-cli /usr/bin
COPY --from=prep /lbrycrd-tx /usr/bin
COPY stuff/start.sh /usr/local/bin/start
COPY stuff/healthcheck.sh /usr/local/bin/healthcheck
VOLUME ["/data"]
WORKDIR /data
## TODO: Implement healthcheck.
# HEALTHCHECK ["healthcheck"]
EXPOSE 9245 9246

CMD ["start"]

You'd mentioned in your last PR the preference to potentially shift to a pinned Lbrycrd version however there are fairly few excuses to have an old version of Lbry blockchain tools in operation so I added curl that pulls the latest path for the latest .zip file.

I'd been going through and linting the Dockerfile and this is roughly the direction I was heading in based on your last feedback. ``` FROM ubuntu:18.04 as prep LABEL MAINTAINER="leopere [at] nixc [dot] us" ## TODO: Implement version pinning. `apt-get install curl=<version>` RUN apt-get update && \ apt-get -y install unzip curl && \ apt-get autoclean -y && \ rm -rf /var/lib/apt/lists/* WORKDIR / SHELL ["/bin/bash", "-o", "pipefail", "-c"] RUN curl -L -o ./lbrycrd-linux.zip $(curl -s https://api.github.com/repos/lbryio/lbrycrd/releases | grep -F 'lbrycrd-linux.zip' | grep download | head -n 1 | cut -d'"' -f4) && \ unzip ./lbrycrd-linux.zip && \ chmod +x ./lbrycrdd ./lbrycrd-cli ./lbrycrd-tx FROM ubuntu:18.04 as app RUN addgroup --gid 1000 lbrycrd && \ adduser lbrycrd --uid 1000 --gid 1000 --gecos GECOS --shell /bin/bash --disabled-password --home /data COPY --from=prep /lbrycrdd /usr/bin COPY --from=prep /lbrycrd-cli /usr/bin COPY --from=prep /lbrycrd-tx /usr/bin COPY stuff/start.sh /usr/local/bin/start COPY stuff/healthcheck.sh /usr/local/bin/healthcheck VOLUME ["/data"] WORKDIR /data ## TODO: Implement healthcheck. # HEALTHCHECK ["healthcheck"] EXPOSE 9245 9246 CMD ["start"] ``` You'd mentioned in your last PR the preference to potentially shift to a pinned Lbrycrd version however there are fairly few excuses to have an old version of Lbry blockchain tools in operation so I added curl that pulls the latest path for the latest .zip file.
EnigmaCurry commented 2019-04-16 20:05:40 +02:00 (Migrated from github.com)
Copy link

cool, multi-stage builds looks cleaner.

cool, multi-stage builds looks cleaner.
Leopere commented 2019-04-16 20:09:08 +02:00 (Migrated from github.com)
Copy link

@EnigmaCurry I'll wait on this PR to reach stable before I merge that in unless you want to streamline mine into yours I don't require commit history so I can sleep at night.

Also the SHELL directive might solve the issue with su -c

@EnigmaCurry I'll wait on this PR to reach stable before I merge that in unless you want to streamline mine into yours I don't require commit history so I can sleep at night. Also the SHELL directive might solve the issue with `su -c`
EnigmaCurry (Migrated from github.com) reviewed 2019-04-16 20:14:12 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
EnigmaCurry (Migrated from github.com) commented 2019-04-16 20:14:12 +02:00
Copy link

So without "USER lbrycrd" every command is run as root, which means that if they don't use start.sh, things will run as root. This is probably wrong, so it would be fixed with an ENTRYPOINT (then nothing could run except for start.sh, which does the user descalation) But, if were to use an ENRTRYPOINT, I would want to make sure that we are not hiding any of parameters from the lbrycrd binary itself. That's one of the problems with wrapper scripts..

So without "USER lbrycrd" every command is run as root, which means that if they don't use start.sh, things will run as root. This is probably wrong, so it would be fixed with an ENTRYPOINT (then nothing could run except for start.sh, which does the user descalation) But, if were to use an ENRTRYPOINT, I would want to make sure that we are not hiding any of parameters from the lbrycrd binary itself. That's one of the problems with wrapper scripts..
EnigmaCurry (Migrated from github.com) reviewed 2019-04-16 20:16:57 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
EnigmaCurry (Migrated from github.com) commented 2019-04-16 20:16:57 +02:00
Copy link

This is just an idea, I haven't tested it, but can we have "USER lbrycrd" and then set the start.sh script as setuid root? That way nothing could run as root except for start.sh, which can fix permissions, then deescalate.

This is just an idea, I haven't tested it, but can we have "USER lbrycrd" and then set the start.sh script as setuid root? That way nothing could run as root except for start.sh, which can fix permissions, then deescalate.
EnigmaCurry (Migrated from github.com) reviewed 2019-04-16 20:23:31 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
EnigmaCurry (Migrated from github.com) commented 2019-04-16 20:23:31 +02:00
Copy link

or maybe add another script to stuff called fix_permissions.sh and set that script to setuid root.

or maybe add another script to stuff called fix_permissions.sh and set *that* script to setuid root.
Leopere (Migrated from github.com) reviewed 2019-04-16 20:28:40 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
Leopere (Migrated from github.com) commented 2019-04-16 20:28:39 +02:00
Copy link

So the only problem is if the config's ownership is already set to Root and it needs to be executable by the user lbrycrd it would not have access. You could give the lbrycrd user the ability to use sudo however that would be self-defeating, and at that point you might as well operate the application at the root level, to begin with within the container.

So the only problem is if the config's ownership is already set to Root and it needs to be executable by the user `lbrycrd` it would not have access. You could give the `lbrycrd` user the ability to use `sudo` however that would be self-defeating, and at that point you might as well operate the application at the root level, to begin with within the container.
Leopere (Migrated from github.com) reviewed 2019-04-16 20:30:07 +02:00
lbrycrd/Dockerfile
@ -27,3 +27,3 @@
# USER lbrycrd
USER lbrycrd
# RUN mkdir /data
Leopere (Migrated from github.com) commented 2019-04-16 20:30:07 +02:00
Copy link

I believe that the solution will have to likely reside within aiming for an alternative to su -c which operates the same and doesn't necessarily require more things to be installed if at all possible. I've seen things like su-exec and whatnot but I've not tested them.

I believe that the solution will have to likely reside within aiming for an alternative to `su -c` which operates the same and doesn't necessarily require more things to be installed if at all possible. I've seen things like `su-exec` and whatnot but I've not tested them.
EnigmaCurry commented 2019-04-16 20:40:27 +02:00 (Migrated from github.com)
Copy link

yes I will work on it some more, but I'll open a different PR.

yes I will work on it some more, but I'll open a different PR.
Leopere commented 2019-04-17 16:57:02 +02:00 (Migrated from github.com)
Copy link

This PR closes #18

This PR closes #18

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Accepted

Just for marking accepted PR's

  
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
Epic

   
good first issue

Good for newcomers

  
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

Extra attention is needed

  
icebox

Long-term storage

  
In Review

Pull Requests that are currently in review and potentially waiting tweaks or feedback for clarity.

  
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
resilience

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels
Accepted
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
Epic
good first issue
hacktoberfest
help wanted
icebox
In Review
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-docker#46
No description provided.
Delete branch "cloud-init-new"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?