-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
mysql_replication: add connection_name param for MariaDB multi source replication support #63229
mysql_replication: add connection_name param for MariaDB multi source replication support #63229
Conversation
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.
LGTM
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.
Two more words: changelog fragment :)
cursor.execute("SHOW SLAVE STATUS") | ||
def get_slave_status(cursor, connection_name=''): | ||
if connection_name: | ||
cursor.execute("SHOW SLAVE '%s' STATUS" % connection_name) |
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.
Does this need any proper escaping?
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.
@felixfontein i didn't get an idea, could you please describe this? (it's covered by ci:)
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.
If connection_name
is '; DROP TABLE ORDERS; --
this will have unintended side-effects. (https://www.xkcd.com/327/)
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.
@felixfontein shouldn't we trust the argument ?
I mean, these features aren't ment to used with arguments coming from untrusted sources I guess, or should be escaped / protected earlier in the code.
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.
And, as far as I remember, cursor.execute
won't execute a multi statement query, so the injection should fail.
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.
if a user wants to drop his production databases, i believe, it is unavoidable
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.
Most security holes come from such assumptions, which get forgotten over time :) If you want to merge it this way, fine for me. Just don't complain nobody told you ;)
… support, add changelog
it is a blocker for me and others, i can't implement #29311 before this PR is merged |
@bmalynovytch would be cool to get your suggestion about this in general |
shipit |
@resmo , @felixfontein , @bmalynovytch thank you for reviewing ! |
In general, maybe should we add some injection CI tests. (dunno if it is what you were talking about 🙂 ) |
@bmalynovytch , bright idea, we should certainly try ! :) |
SUMMARY
mysql_replication: add connection_name param for MariaDB multi source replication support
fixes #46243
ISSUE TYPE
COMPONENT NAME
lib/ansible/modules/database/mysql/mysql_replication.py