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

[#4292] Fix mounted data path directory permissions for besu user #7575

Merged
merged 12 commits into from
Sep 18, 2024

Conversation

pullurib
Copy link
Contributor

@pullurib pullurib commented Sep 4, 2024

PR description

  • Fix data path directory permissions for besu user by adding an entry script to update the permissions depending on the data path parameter passed while running the docker image
  • Script is run as root user and switches to besu user at the end before launching besu

Looking to get approval for this approach at this point . Has more work todo

  • Manually copied the entry script to install path for now - this should be part of build.
  • These checks and updates in entry script can be moved to main besu launch script
  • More checks should be added to get and validate data path as described in the entry script comments

Fixed Issue(s)

fixes #4292

@pullurib pullurib marked this pull request as draft September 4, 2024 16:21
@fab-10
Copy link
Contributor

fab-10 commented Sep 5, 2024

Your approach looks fine, and I would like to review a more elegant and future proof way to detect which are the paths that need to be owned by Besu, I was first thinking of a kind of Docker introspection where from inside the container it is possible to detect which are the mounted volumes, but then I thought could be better to delegate this directly to Besu, I mean having a dedicated subcommand, or option, that given the passed options, or configuration, just prints the data dir path and exit. WDYT?

Also will be nice to have a test that reproduce the use case, so we can add it to our CI and detect possible regressions.

@pullurib
Copy link
Contributor Author

pullurib commented Sep 6, 2024

Thanks for taking a look. Besu subcommand approach sounds good. Since the options can be set through CLI, env vars or config file, we can let the subcommand consume options passed , check read/write access for required paths (data path and few more ) and output the paths which need adjusting the permissions .
The entry script would first run besu with this subcommand as a child, as besu user, get the list of paths and adjust permissions for besu users and finally run besu

list of options to include in the check now (which can be extended in future)

  • --data-path
  • --genesis-file
  • --rpc-ws-authentication-credentials-file
  • --tx-pool-save-file
  • --config-file

Does this look ok?

I'll add the test case

@fab-10
Copy link
Contributor

fab-10 commented Sep 9, 2024

@pullurib yes it looks ok, not sure at the moment if it is better to do this via a subcommand or via a special option like --print-paths-and-exit but probably you will get a better idea working on it.

This option doesn't have a corresponding config file entry as it's a
standalone option to be used with docker containers

Signed-off-by: Bhanu Pulluri <[email protected]>
@pullurib pullurib marked this pull request as ready for review September 14, 2024 16:53
@pullurib
Copy link
Contributor Author

@fab-10 went with --print-paths-and-exit option as the config has to be resolved from options passed , env vars and config files which is already done in besu main command . Added a test case in docker tests

Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

Looks good, just some comments to make the options more generic for other possible use cases, and other little things.
Please add a CHANGELOG entry as well.

besu/src/main/scripts/besu-entry.sh Outdated Show resolved Hide resolved
@fab-10 fab-10 added the doc-change-required Indicates an issue or PR that requires doc to be updated label Sep 16, 2024
@pullurib
Copy link
Contributor Author

@fab-10 , thanks for the valuable suggestions. I made the review changes along with CHANGELOG addition. I see that doc changes would go into a different repo. Once this is merged, I can update the docs and link that PR here . Or should we wait to merge both at about same time ?

Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

just some final touches before approval, check the suggested change for the CHANGELOG, and since this options seems to be platform dependent, it works on Linux, not sure on MacOs, but for sure will not work on Linux, then I suggest to check the OS if the option is passed and report an error message on not supported OSes.
You can use the already provided dependency for system info https://www.oshi.ooo/oshi-core-java11/apidocs/com.github.oshi/oshi/SystemInfo.html#getCurrentPlatform()

CHANGELOG.md Outdated Show resolved Hide resolved
@fab-10
Copy link
Contributor

fab-10 commented Sep 17, 2024

I see that doc changes would go into a different repo. Once this is merged, I can update the docs and link that PR here . Or should we wait to merge both at about same time ?

You can do the update on your own, or you can wait that someone else will pick up the doc task.

Bhanu Pulluri and others added 2 commits September 17, 2024 23:37
Co-authored-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Bhanu Pulluri <[email protected]>
@pullurib
Copy link
Contributor Author

Added platform checks. The option works fine on MacOS too, so checking for Linux or Mac for now to be able to use the option. Thanks for the CHANELOG correction, included it

@fab-10 fab-10 enabled auto-merge (squash) September 18, 2024 07:41
@fab-10 fab-10 merged commit 0d9fa16 into hyperledger:main Sep 18, 2024
41 checks passed
@fab-10
Copy link
Contributor

fab-10 commented Sep 18, 2024

@pullurib I have merged the PR, but on main the CI report this error https://github.com/hyperledger/besu/actions/runs/10919521049/job/30307346592, could you check?

@pullurib
Copy link
Contributor Author

Raised a small PR #7637 for hadolint failure fix, please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-change-required Indicates an issue or PR that requires doc to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read file permission denied when starting besu from docker image
2 participants