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

Docker Image enhancement and better CLI support. #1603

Merged
merged 3 commits into from
May 20, 2024

Conversation

jorgecuesta
Copy link
Contributor

@jorgecuesta jorgecuesta commented May 2, 2024

THIS PR INCLUDES BREAKING CHANGES

  • Dockerfile now uses app user instead of root

Why?

A few Geo-Mesh users report to POKTscan about an issue with the new image after we adopt the one on the pokt-network/pocket-core repository on the latest RC.

They report that this image is using root as the user which is recommended to be avoided. There are a lot of blogs and documentation about this, here one of them from a well-known docker image user/company.

Also, we detected a few things that could be enhanced on both, entry point and docker context.

The problem with having a public image using root right now is that pocket binary generates folders and files that now belong to the root user, so they will need to modify those permissions to belong to the proper app user and group. To this, I added another optional entry point that could be used once to fix the permission issue and then start the container as before.

Here you can see how to use it with docker-compose or docker

Changes:

  • Modifications to Dockerfile allow the container to run as app user instead of root.
  • Added a new shell script named fix_permissions.sh to fix ownership issues related to running containers.
  • Updated Dockerfile to use this new script.
  • Added a .dockerignore file to help maintain a cleaner Docker build context, excluding unnecessary files.
  • Modifications to entrypoint.sh allows the user to run all its internal commands with the proper --datadir param. Now properly handle the start command when --keybase=false is sent. Also, allow the user to pass the --datadir as an env variable to omit it on the start command.

…Entrypoint file to handle few edge cases.

Details:
*. Modifications to Dockerfile allow for the container to be run as app user instead of root.
*. Added a new shell script named fix_permissions.sh to fix ownership issues related to running containers.
*. Updated Dockerfile to use this new script.
*. Added a .dockerignore file to help maintain a cleaner Docker build context, excluding unnecessary files.
*. Modifications to entrypoint.sh allow user to run all the internal commands of it with the proper --datadir param. Now properly handle the start command when --keybase=false is sent. Also allow user to pass the --datadir as env variable to omit it on start command.
@Olshansk
Copy link
Member

Olshansk commented May 3, 2024

@okdas Please review this when you're back in office next week and see if any of the learnings / suggestions can transfer over to #495 which we should pick up then as well.

@Olshansk Olshansk self-requested a review May 3, 2024 23:41
.github/workflows/fix_permissions.sh Outdated Show resolved Hide resolved
.github/workflows/fix_permissions.sh Outdated Show resolved Hide resolved
.github/workflows/fix_permissions.sh Outdated Show resolved Hide resolved
.github/workflows/fix_permissions.sh Outdated Show resolved Hide resolved
.github/workflows/fix_permissions.sh Outdated Show resolved Hide resolved
.github/workflows/fix_permissions.sh Outdated Show resolved Hide resolved
.github/workflows/entrypoint.sh Outdated Show resolved Hide resolved
.github/workflows/entrypoint.sh Outdated Show resolved Hide resolved
.github/workflows/entrypoint.sh Outdated Show resolved Hide resolved
.github/workflows/entrypoint.sh Outdated Show resolved Hide resolved
In this commit, we have updated the warning and info messages in the 'entrypoint.sh' script for better clarity. The 'fix_permissions.sh' script has been renamed to 'change_datadir_ownership_to_app.sh' and has had its comment section and message notifications elaborated for better understanding. Changes have been reflected in the Dockerfile.
@jorgecuesta jorgecuesta requested a review from Olshansk May 9, 2024 16:17
jorgecuesta added a commit to pokt-scan/pocket-core that referenced this pull request May 9, 2024
…ns.sh" to "change_datadir_ownership_to_app.sh"
jorgecuesta added a commit to pokt-scan/pocket-core that referenced this pull request May 9, 2024
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

I think this is great!

I do want one other pair of eyes from an operator to make sure I didn't miss anything, but it has a 👍 from me.

@okdas @msmania @nodiesBlade Can one of you PTAL and review?

@nodiesBlade
Copy link
Contributor

(Reviewed this on Geomesh branch as well), looks good to me.

@Olshansk
Copy link
Member

@okdas PTAL

Copy link
Member

@okdas okdas left a comment

Choose a reason for hiding this comment

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

I haven't pulled to verify locally but code changes look good.

@Olshansk
Copy link
Member

Re-running to make sure this passes but otherwise lgtm:

Screenshot 2024-05-17 at 3 38 42 PM

@okdas
Copy link
Member

okdas commented May 17, 2024

@Olshansk it is not going to pass because the origin of the PR is a branch from pokt-scan organization which doesn't have permissions to push to our container registry.

@Olshansk
Copy link
Member

@okdas Appreciate the context. What do you recommend as the solution here?

For example:

  1. We squash & merge as is?
  2. Update permissions to allow their org to run CI on our repo?
  3. Validate in some other way?
  4. Etc?

I understand the risk is low but just looking to you to make a call here.

@okdas
Copy link
Member

okdas commented May 20, 2024

@Olshansk the workaround would be to open a new PR under pokt-network org, but I don't think we need to do that and should proceed with merging as is. The CI job seems to be failing on the very last step - and the image itself is built successfully.

@Olshansk Olshansk merged commit 33b28c1 into pokt-network:staging May 20, 2024
1 of 2 checks passed
jorgecuesta added a commit to pokt-scan/pocket-core that referenced this pull request Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants