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

Fix the "permisison denied" bug properly #260

Merged
merged 1 commit into from
May 17, 2021
Merged

Fix the "permisison denied" bug properly #260

merged 1 commit into from
May 17, 2021

Conversation

namnx228
Copy link
Member

@namnx228 namnx228 commented May 4, 2021

Issue: For some reason the script mysqld_safe_helper is used to run mysqld. This script runs with --user=mysql and --log-error=/var/log/mariadb/mariadb.log. This causes the permisison denied error because user mysql tries to write to /var/log/mariadb/mariadb.log which, in this repo, is a symlink of root's stdout.
Error message:
/usr/bin/mysqld_safe_helper: Can't create/write to file '/var/log/mariadb/mariadb.log' (Errcode: 13 "Permission denied")
This PR solves the problem by removing the symlink hack and make the error log go to stdout in a proper way.

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 4, 2021
@namnx228
Copy link
Member Author

namnx228 commented May 4, 2021

/test-integration
/assign @dtantsur
/cc @elfosardo

@namnx228
Copy link
Member Author

namnx228 commented May 4, 2021

/test-integration

dtantsur
dtantsur reviewed May 4, 2021
View reviewed changes
scripts/runmariadb Outdated
if [ ! -d "${DATADIR}/mysql" ]; then
crudini --set "$MARIADB_CONF_FILE" mysqld max_connections 64
crudini --set "$MARIADB_CONF_FILE" mysqld max_heap_table_size 1M
crudini --set "$MARIADB_CONF_FILE" mysqld innodb_buffer_pool_size 5M
crudini --set "$MARIADB_CONF_FILE" mysqld innodb_log_buffer_size 512K
crudini --set "$MARIADB_CONF_FILE" mariadb-10.3 skip_log_error # Error log will be redirected to stderr
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure the group name is actually "mariadb-10.3", not mysqld as above?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should work in both group, I guess. I choose "mariadb-10.3" since in the example they use a similar group. https://mariadb.com/kb/en/error-log/#writing-the-error-log-to-stderr-on-unix.

@namnx228 namnx228 changed the title Fix the "permisison denied" bug properly WIP: Fix the "permisison denied" bug properly May 4, 2021
@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2021
@namnx228
Copy link
Member Author

namnx228 commented May 4, 2021

/test-integration

@namnx228 namnx228 changed the title WIP: Fix the "permisison denied" bug properly Fix the "permisison denied" bug properly May 4, 2021
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2021
@namnx228
Copy link
Member Author

namnx228 commented May 4, 2021

/test-integration

dtantsur
dtantsur reviewed May 4, 2021
View reviewed changes
scripts/runmariadb Outdated
if [ ! -d "${DATADIR}/mysql" ]; then
crudini --set "$MARIADB_CONF_FILE" mysqld max_connections 64
crudini --set "$MARIADB_CONF_FILE" mysqld max_heap_table_size 1M
crudini --set "$MARIADB_CONF_FILE" mysqld innodb_buffer_pool_size 5M
crudini --set "$MARIADB_CONF_FILE" mysqld innodb_log_buffer_size 512K

# Error log will be redirected to stderr
crudini --set "$MARIADB_CONF_FILE" mariadb-10.3 skip_log_error
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe use mysqld so that we're not broken if/when centos updates mariadb?

@namnx228 namnx228 changed the title Fix the "permisison denied" bug properly WIP: Fix the "permisison denied" bug properly May 4, 2021
@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2021
@namnx228 namnx228 force-pushed the fix-bug-nam branch 2 times, most recently from d75afe5 to 78186ed Compare May 4, 2021 15:51
@namnx228
Copy link
Member Author

namnx228 commented May 4, 2021

@dtantsur I tried to use skip-log-error, but then mysqld_safe wrote the error log to /var/log/mysql/<hostname>.err. I go through the mysqld_safe script and see that it will write the log to a file rather than to stderr.
Look like we have to use the symlink hack. So the possible solution now is to run mysqld by root rather than user mysql

@namnx228
Copy link
Member Author

namnx228 commented May 4, 2021

/test-integration

@namnx228
Copy link
Member Author

namnx228 commented May 4, 2021

/test-integration

@namnx228 namnx228 changed the title WIP: Fix the "permisison denied" bug properly Fix the "permisison denied" bug properly May 5, 2021
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 5, 2021
@namnx228 namnx228 requested a review from dtantsur May 5, 2021 06:47
@namnx228 namnx228 mentioned this pull request May 5, 2021
Closed
@elfosardo
Copy link
Member

@dtantsur I tried to use skip-log-error, but then mysqld_safe wrote the error log to /var/log/mysql/<hostname>.err. I go through the mysqld_safe script and see that it will write the log to a file rather than to stderr.
Look like we have to use the symlink hack. So the possible solution now is to run mysqld by root rather than user mysql

@namnx228 have you tried adding the tty group to the mysqld user ?

@namnx228
Copy link
Member Author

namnx228 commented May 6, 2021

@dtantsur I tried to use skip-log-error, but then mysqld_safe wrote the error log to /var/log/mysql/<hostname>.err. I go through the mysqld_safe script and see that it will write the log to a file rather than to stderr.
Look like we have to use the symlink hack. So the possible solution now is to run mysqld by root rather than user mysql

@namnx228 have you tried adding the tty group to the mysqld user ?

I haven't tried it. However, do we have a specific reason to avoid using root user here? I know that in general, root should not be used, but in this case, I have a feeling that using root doesn't cause any troubles.

@elfosardo
Copy link
Member

@dtantsur I tried to use skip-log-error, but then mysqld_safe wrote the error log to /var/log/mysql/<hostname>.err. I go through the mysqld_safe script and see that it will write the log to a file rather than to stderr.
Look like we have to use the symlink hack. So the possible solution now is to run mysqld by root rather than user mysql

@namnx228 have you tried adding the tty group to the mysqld user ?

I haven't tried it. However, do we have a specific reason to avoid using root user here? I know that in general, root should not be used, but in this case, I have a feeling that using root doesn't cause any troubles.

in general we should always adhere to the principle of least privilege, or do our best to
this applies on services running on servers as on containers
considering that there are workarounds to avoid running a service as root also in this case, IMHO we should give them a try before resorting to the run-as-root solution

@namnx228 namnx228 force-pushed the fix-bug-nam branch 3 times, most recently from 3f3d76b to de44d7d Compare May 6, 2021 13:33
@namnx228
Copy link
Member Author

namnx228 commented May 6, 2021

@elfosardo I have tried to add mysql user to the tty group, but the permission denied still occurred, unfortunately.

@namnx228
Copy link
Member Author

namnx228 commented May 6, 2021

/test-integration

@elfosardo
Copy link
Member

@namnx228 could you please provide the steps you used to do the test?
I did test that myself and it worked for me

@namnx228
Copy link
Member Author

namnx228 commented May 7, 2021 •
edited
Loading

@namnx228 could you please provide the steps you used to do the test?
I did test that myself and it worked for me

This is the commit that I push to test the tty group: 3f3d76b
I did the test locally in the same way as we do in the CI integration test. Maybe I could do something wrong.
I can cherry pick that commit onto the branch of this PR, then run the CI test. Let's see how it goes.

@namnx228
Copy link
Member Author

namnx228 commented May 7, 2021

/test-integration

@namnx228
Copy link
Member Author

namnx228 commented May 7, 2021

@elfosardo The CI test fails because of the the same error.

@namnx228
Copy link
Member Author

/test-integration

@dtantsur
Copy link
Member

/approve

Root inside a container doesn't give a potential attacker much, so I think we're fine with that.

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, namnx228

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2021
@fmuyassarov
Copy link
Member

/retest

@namnx228
Copy link
Member Author

/retest

@namnx228
Copy link
Member Author

/test-integration

@namnx228
Copy link
Member Author

/test-integration

@maelk
Copy link
Member

maelk commented May 17, 2021

/test-integration
/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2021
@metal3-io-bot metal3-io-bot merged commit 5984f1e into metal3-io:master May 17, 2021
elfosardo pushed a commit to elfosardo/ironic-image that referenced this pull request Mar 31, 2022
@openshift-merge-robot
Merge pull request metal3-io#260 from multi-arch/ipxe-snponly
86107da 
Drop unused ipxe.efi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elfosardo elfosardo Awaiting requested review from elfosardo

@dtantsur dtantsur Awaiting requested review from dtantsur

Assignees

@dtantsur dtantsur

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@namnx228 @elfosardo @dtantsur @metal3-io-bot @fmuyassarov @maelk