-
Notifications
You must be signed in to change notification settings - Fork 858
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
Conversation
Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
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. |
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 . list of options to include in the check now (which can be extended in future)
Does this look ok? I'll add the test case |
@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 |
Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
a077985
to
793dbee
Compare
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 <bhanu.pulluri@kaleido.io>
@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 |
There was a problem hiding this 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.
…y permissions Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
…spaces Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
@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 ? |
Signed-off-by: Bhanu Pulluri <59369753+pullurib@users.noreply.github.com>
Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
There was a problem hiding this 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()
You can do the update on your own, or you can wait that someone else will pick up the doc task. |
… Linux and Mac Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Bhanu Pulluri <59369753+pullurib@users.noreply.github.com>
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 |
@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? |
Raised a small PR #7637 for hadolint failure fix, please take a look |
@pullurib Corresponding documentation issue raised: hyperledger/besu-docs#1701 |
PR description
Looking to get approval for this approach at this point . Has more work todo
Fixed Issue(s)
fixes #4292