From 432c3f0be45780bb2938ea39fab0c8a6ef9057ba Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 19 Apr 2021 20:27:55 +0100 Subject: [PATCH] Ensure that nagios user gets created with a password The associated bug is due to a change introduced in commit d55dcde which was to ensure that the correct password update is used for different versions of mysql (pre and post 5.7.5). However, this change has broken the nagios user creation due to not setting the password. This patch creates the nagios user and passord at the same time. The updating of the password is only done if the account already exists. The change also corrects the nagios password store in leader-settings to the 'mysql-nagios.passwd' key instead of 'nagios-password'. This was an unfortuante error when the nagios change password was introduced. The charm detects if the 'nagios-password' key is used on charm-upgrade and moves it to 'mysql-nagios.passwd'. This enables the key to work with the standard MySQLHelper functions. Finally, the ALTER command (on percona) doesn't update non-InnoDB tables and thus needs to be run for each unit when the nagios password is changed via the action. The changes in percona_utils.py enable this to happen. Whilst the change looks large it ONLY affects the nagios password parts of the charm. The related bug is a tracking bug to serve as a reminder to fix this in charm-helpers and this charm (i.e. make the charm-helpers code work to change a password for any user other than root, and then enable this charm to use that code). Change-Id: Ibc751bef7b4654ebffdf843c556b193373e6e80c Related-Bug: #1925377 Closes-Bug: #1925042 --- actions/actions.py | 2 +- hooks/percona_hooks.py | 8 ++ hooks/percona_utils.py | 121 +++++++++++++----- tests/bundles/bionic-nagios-ha.yaml | 1 + tests/bundles/bionic-nagios.yaml | 28 ++++ .../bundles/overlays/bionic-nagios-ha.yaml.j2 | 1 + tests/tests.yaml | 2 + unit_tests/test_actions.py | 3 +- unit_tests/test_percona_hooks.py | 7 +- unit_tests/test_percona_utils.py | 23 +++- 10 files changed, 157 insertions(+), 39 deletions(-) create mode 120000 tests/bundles/bionic-nagios-ha.yaml create mode 100644 tests/bundles/bionic-nagios.yaml create mode 120000 tests/bundles/overlays/bionic-nagios-ha.yaml.j2 diff --git a/actions/actions.py b/actions/actions.py index 13dc6c9..73c0af4 100755 --- a/actions/actions.py +++ b/actions/actions.py @@ -227,7 +227,7 @@ def mysqldump(args): def generate_nagios_password(args): """Regenerate nagios password.""" if is_leader(): - leader_set({"nagios-password": pwgen()}) + leader_set({"mysql-nagios.passwd": pwgen()}) percona_utils.set_nagios_user() action_set({"output": "New password for nagios created successfully."}) else: diff --git a/hooks/percona_hooks.py b/hooks/percona_hooks.py index 1dac078..9f17664 100755 --- a/hooks/percona_hooks.py +++ b/hooks/percona_hooks.py @@ -530,6 +530,14 @@ def upgrade(): if not leader_get('root-password') and leader_get('mysql.passwd'): leader_set(**{'root-password': leader_get('mysql.passwd')}) + # move the nagios password out of nagios-password and into + # mysql-nagios.passwd + # BUG: #1925042 + nagios_password = leader_get('nagios-password') + if nagios_password: + leader_set(**{"mysql-nagios.passwd": nagios_password, + "nagios-password": None}) + # On upgrade-charm we assume the cluster was complete at some point kvstore = kv() initial_clustered = kvstore.get(INITIAL_CLUSTERED_KEY, False) diff --git a/hooks/percona_utils.py b/hooks/percona_utils.py index 12a6bcc..3adccba 100644 --- a/hooks/percona_utils.py +++ b/hooks/percona_utils.py @@ -830,8 +830,8 @@ def assess_status(configs): base = determine_packages()[0] yield base if '.' not in base: - for i in range(5, 7+1): - yield base+'-5.'+str(i) + for i in range(5, 7 + 1): + yield base + '-5.' + str(i) version = None for pkg in _possible_packages(): version = get_upstream_version(pkg) @@ -1054,7 +1054,11 @@ root_password = partial(_get_password, 'root-password') sst_password = partial(_get_password, 'sst-password') -nagios_password = partial(_get_password, 'nagios-password') + +def nagios_password(): + """Get the nagios password using the MySQLdb helper in charmhelpers""" + m_helper = get_db_helper() + return m_helper.get_mysql_password('nagios') def pxc_installed(): @@ -1802,38 +1806,93 @@ def create_nagios_user(): :raises: OperationalError """ - m_helper = get_db_helper() - try: - m_helper.connect(password=m_helper.get_mysql_root_password()) - except OperationalError: - log("Could not connect to db", level=ERROR) - raise - # NOTE (rgildein): This user has access to the SHOW GLOBAL STATUS - # statement, which does not require any privilege, only the ability to - # connect to the server. - nagios_exists = False - try: - # NOTE (rgildein): This method of creation nagios user should be - # replaced by the `MySQLHelper.create_user` method after - # bug #1905586 has been fixed and merged. - nagios_exists = m_helper.select( - "SELECT EXISTS(SELECT 1 FROM mysql.user WHERE user = 'nagios')" - ) - m_helper.execute("CREATE USER 'nagios'@'localhost';") - except OperationalError as error: - if not nagios_exists: - log("Creating user nagios failed due: {}".format(error), - level="ERROR") - raise error - else: - log("User 'nagios'@'localhost' already exists.", level="WARNING") + m_helper = None + if is_leader(): + # Only run the CREATE USER on the leader; it does replicate to other + # instances. + m_helper = get_db_helper() + try: + m_helper.connect(password=m_helper.get_mysql_root_password()) + except OperationalError: + log("Could not connect to db", level=ERROR) + raise + # NOTE (rgildein): This user has access to the SHOW GLOBAL STATUS + # statement, which does not require any privilege, only the ability to + # connect to the server. + nagios_exists = False + try: + # NOTE (rgildein): This method of creation nagios user should be + # replaced by the `MySQLHelper.create_user` method after + # bug #1905586 has been fixed and merged. + nagios_exists = m_helper.select( + "SELECT EXISTS(SELECT 1 FROM mysql.user WHERE user = 'nagios')" + ) + m_helper.execute( + "CREATE USER 'nagios'@'localhost' IDENTIFIED BY '{}';" + .format(nagios_password())) + m_helper.flush_priviledges() + log("Created user nagios in database.", level="INFO") + return + except OperationalError as error: + if not nagios_exists: + log("Creating user nagios failed due: {}".format(error), + level="ERROR") + raise error + else: + log("User 'nagios'@'localhost' already exists.", level="INFO") + # NOTE (rgildein): Update the user's password if it has changed. - m_helper.set_mysql_password('nagios', nagios_password()) + # Update the password for username with new_password. + + # BUG: #1925042 + # This is a strategic hotfix to fix nagios password updates. The + # charm-helpers library function set_mysql_password() tries to use the + # username that was passed, rather than root, to update the mysql.user + # table and this fails. Instead, this function uses root to update the + # password and flush privileges. Note this needs to be run on each node as + # it uses UPDATE and flush privileges. + + # See https://galeracluster.com/library/kb/user-changes.html + + # copied and adapted from MySQLHelper.set_mysql_password() + cursor = None + try: + # NOTE(freyes): Due to skip-name-resolve root@$HOSTNAME account + # fails when using SET PASSWORD so using UPDATE against the + # mysql.user table is needed, but changes to this table are not + # replicated across the cluster, so this update needs to run in + # all the nodes. More info at + # http://galeracluster.com/documentation-webpages/userchanges.html + release = CompareHostReleases(lsb_release()['DISTRIB_CODENAME']) + if release < 'bionic': + SQL_UPDATE_PASSWD = ("UPDATE mysql.user SET password = " + "PASSWORD( %s ) WHERE user = %s;") + else: + # PXC 5.7 (introduced in Bionic) uses authentication_string + SQL_UPDATE_PASSWD = ("UPDATE mysql.user SET " + "authentication_string = " + "PASSWORD( %s ) WHERE user = %s;") + + if m_helper is None: + m_helper = get_db_helper() + try: + m_helper.connect(password=m_helper.get_mysql_root_password()) + except OperationalError: + log("Could not connect to db", level=WARNING) + return + cursor = m_helper.connection.cursor() + cursor.execute(SQL_UPDATE_PASSWD, (nagios_password(), 'nagios')) + cursor.execute('FLUSH PRIVILEGES;') + m_helper.connection.commit() + except OperationalError as ex: + log("Could not connect to db: {}".format(str(ex)), level=WARNING) + finally: + if cursor: + cursor.close() def set_nagios_user(): """Create MySQL user and configuration file with user credentials.""" - if is_leader(): - create_nagios_user() + create_nagios_user() write_nagios_my_cnf() diff --git a/tests/bundles/bionic-nagios-ha.yaml b/tests/bundles/bionic-nagios-ha.yaml new file mode 120000 index 0000000..ff11cfe --- /dev/null +++ b/tests/bundles/bionic-nagios-ha.yaml @@ -0,0 +1 @@ +bionic-nagios.yaml \ No newline at end of file diff --git a/tests/bundles/bionic-nagios.yaml b/tests/bundles/bionic-nagios.yaml new file mode 100644 index 0000000..b09d826 --- /dev/null +++ b/tests/bundles/bionic-nagios.yaml @@ -0,0 +1,28 @@ +series: bionic + +applications: + percona-cluster: + series: bionic + charm: ../../../percona-cluster + num_units: 1 + keystone: + charm: cs:~openstack-charmers-next/keystone + num_units: 1 + options: + token-expiration: 60 + nagios: + charm: cs:nagios + series: bionic + num_units: 1 + nrpe: + charm: cs:nrpe + +relations: +- - keystone:shared-db + - percona-cluster:shared-db + +- - nrpe:monitors + - nagios:monitors + +- - nrpe:nrpe-external-master + - percona-cluster:nrpe-external-master diff --git a/tests/bundles/overlays/bionic-nagios-ha.yaml.j2 b/tests/bundles/overlays/bionic-nagios-ha.yaml.j2 new file mode 120000 index 0000000..8ba59d0 --- /dev/null +++ b/tests/bundles/overlays/bionic-nagios-ha.yaml.j2 @@ -0,0 +1 @@ +ha.yaml \ No newline at end of file diff --git a/tests/tests.yaml b/tests/tests.yaml index 3dbdab8..6bbf1c8 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -19,3 +19,5 @@ smoke_bundles: - bionic_model: bionic-ha dev_bundles: - xenial_series_upgrade_model: xenial-queens-ha-series-upgrade + - bionic_model: bionic-nagios + - bionic_model: bionic-nagios-ha diff --git a/unit_tests/test_actions.py b/unit_tests/test_actions.py index c25cdea..c2a0f6b 100644 --- a/unit_tests/test_actions.py +++ b/unit_tests/test_actions.py @@ -124,7 +124,8 @@ class NagiosTestCase(CharmTestCase): self.is_leader.return_value = True self.pwgen.return_value = "1234" actions.generate_nagios_password([]) - self.leader_set.assert_called_once_with({"nagios-password": "1234"}) + self.leader_set.assert_called_once_with( + {"mysql-nagios.passwd": "1234"}) mock_set_nagios_user.assert_called_once_with() self.action_set.assert_called_once_with( {"output": "New password for nagios created successfully."} diff --git a/unit_tests/test_percona_hooks.py b/unit_tests/test_percona_hooks.py index 00c7e66..1670566 100644 --- a/unit_tests/test_percona_hooks.py +++ b/unit_tests/test_percona_hooks.py @@ -816,7 +816,8 @@ class TestUpgradeCharm(CharmTestCase): self.is_leader.return_value = True self.is_unit_paused_set.return_value = False self.get_relation_ip.return_value = '10.10.10.10' - self.leader_get.side_effect = [None, 'mypasswd', 'mypasswd'] + self.leader_get.side_effect = [ + None, 'mypasswd', 'mypasswd', 'nagios-password'] def c(k): values = {'wsrep_ready': 'on', @@ -833,7 +834,9 @@ class TestUpgradeCharm(CharmTestCase): self.leader_set.assert_has_calls( [mock.call(**{'leader-ip': '10.10.10.10'}), - mock.call(**{'root-password': 'mypasswd'})]) + mock.call(**{'root-password': 'mypasswd'}), + mock.call(**{'mysql-nagios.passwd': 'nagios-password', + 'nagios-password': None})]) class TestConfigs(CharmTestCase): diff --git a/unit_tests/test_percona_utils.py b/unit_tests/test_percona_utils.py index 6b95e92..7c0971e 100644 --- a/unit_tests/test_percona_utils.py +++ b/unit_tests/test_percona_utils.py @@ -539,27 +539,32 @@ class UtilsTests(CharmTestCase): mock_apt_update.assert_not_called() + @mock.patch.object(percona_utils, "nagios_password") @mock.patch.object(percona_utils, "lsb_release") @mock.patch.object(percona_utils, "get_db_helper") @mock.patch.object(percona_utils, "write_nagios_my_cnf") def test_create_nagios_user(self, mock_create_nagios_mysql_credential, mock_get_db_helper, - mock_lsb_release): + mock_lsb_release, + mock_nagios_password): mock_lsb_release.return_value = {'DISTRIB_CODENAME': 'bionic'} my_mock = mock.Mock() - self.leader_get.return_value = "1234" + mock_nagios_password.return_value = "1234" self.is_leader.return_value = True mock_get_db_helper.return_value = my_mock + mock_cursor = mock.Mock() + my_mock.connection.cursor.return_value = mock_cursor percona_utils.create_nagios_user() my_mock.select.assert_called_once_with( "SELECT EXISTS(SELECT 1 FROM mysql.user WHERE user = 'nagios')" ) my_mock.execute.assert_has_calls([ - mock.call("CREATE USER 'nagios'@'localhost';"), + mock.call( + "CREATE USER 'nagios'@'localhost' IDENTIFIED BY '1234';"), ]) - my_mock.set_mysql_password.assert_called_once_with('nagios', '1234') + mock_cursor.execute.assert_not_called() class OperationalError(Exception): pass @@ -573,6 +578,16 @@ class UtilsTests(CharmTestCase): my_mock.execute.side_effect = mysql_create_user with self.assertRaises(OperationalError): percona_utils.create_nagios_user() + mock_cursor.execute.assert_not_called() + + my_mock.select.return_value = True + percona_utils.create_nagios_user() + mock_cursor.execute.assert_has_calls([ + mock.call('UPDATE mysql.user SET authentication_string = ' + 'PASSWORD( %s ) WHERE user = %s;', ('1234', 'nagios')), + mock.call('FLUSH PRIVILEGES;'), + ]) + my_mock.connection.commit.assert_called_once_with() def test_get_nrpe_threads_connected_thresholds(self): """Test function for getting and verifying threshold values."""