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

MySQL state and module broken after upgrade to 3000 #56124

Closed
ymasson opened this issue Feb 11, 2020 · 26 comments
Closed

MySQL state and module broken after upgrade to 3000 #56124

ymasson opened this issue Feb 11, 2020 · 26 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P3 Priority 3 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around v3000.1 vulnerable version
Milestone

Comments

@ymasson
Copy link
Contributor

ymasson commented Feb 11, 2020

Description of Issue

MySQL module can't connect to MySQL socket.

Context

Debian 9 and MariaDB 10.4.

Module configuration:
mysql.default_file: '/etc/mysql/debian.cnf'

salt-minion 2019.2.3

~# salt-call mysql.version
local:
    10.4.12-MariaDB-1:10.4.12+maria~stretch-log

salt-minion 3000

~#  salt-call mysql.version
[ERROR   ] An un-handled exception was caught by salt's global exception handler:
AttributeError: 'unicode' object has no attribute 'items'
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    load_entry_point('salt==3000', 'console_scripts', 'salt-call')()
  File "/usr/lib/python2.7/dist-packages/salt/scripts.py", line 445, in salt_call
    client.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/call.py", line 57, in run
    caller.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 119, in run
    ret = self.call()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 218, in call
    ret['return'] = self.minion.executors[fname](self.opts, data, func, args, kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/executors/direct_call.py", line 12, in execute
    return func(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/modules/mysql.py", line 858, in version
    dbc = _connect(**connection_args)
  File "/usr/lib/python2.7/dist-packages/salt/modules/mysql.py", line 392, in _connect
    dbc = MySQLdb.connect(**connargs)
  File "/usr/lib/python2.7/dist-packages/MySQLdb/__init__.py", line 81, in Connect
    return Connection(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/MySQLdb/connections.py", line 173, in __init__
    for k, v in conv.items():
AttributeError: 'unicode' object has no attribute 'items'
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    load_entry_point('salt==3000', 'console_scripts', 'salt-call')()
  File "/usr/lib/python2.7/dist-packages/salt/scripts.py", line 445, in salt_call
    client.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/call.py", line 57, in run
    caller.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 119, in run
    ret = self.call()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 218, in call
    ret['return'] = self.minion.executors[fname](self.opts, data, func, args, kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/executors/direct_call.py", line 12, in execute
    return func(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/modules/mysql.py", line 858, in version
    dbc = _connect(**connection_args)
  File "/usr/lib/python2.7/dist-packages/salt/modules/mysql.py", line 392, in _connect
    dbc = MySQLdb.connect(**connargs)
  File "/usr/lib/python2.7/dist-packages/MySQLdb/__init__.py", line 81, in Connect
    return Connection(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/MySQLdb/connections.py", line 173, in __init__
    for k, v in conv.items():
AttributeError: 'unicode' object has no attribute 'items'

Versions Report

           Salt: 3000
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
       M2Crypto: 0.24.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: 1.3.7
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Sep 26 2018, 18:42:22)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.1
 
System Versions:
           dist: debian 9.12 
         locale: UTF-8
        machine: x86_64
        release: 4.9.0-12-amd64
         system: Linux
        version: debian 9.12
@bitskri3g
Copy link

bitskri3g commented Feb 11, 2020

I can confirm this also happens on CentOS 7. I can't get any mysql modules or states to execute successfully.

sls

test_db:
  mysql_database.present:
    - name: testing
    - connection_unix_socket: /var/lib/mysql/mysql.sock

output:

mysql-7c88a609-b9f0-5af3-9dca-5a9c5e24c282:
----------
          ID: test_db
    Function: mysql_database.present
        Name: testing
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3.6/site-packages/salt/state.py", line 1981, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3.6/site-packages/salt/loader.py", line 1977, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3.6/site-packages/salt/states/mysql_database.py", line 55, in present
                  existing = __salt__['mysql.db_get'](name, **connection_args)
                File "/usr/lib/python3.6/site-packages/salt/modules/mysql.py", line 1034, in db_get
                  dbc = _connect(**connection_args)
                File "/usr/lib/python3.6/site-packages/salt/modules/mysql.py", line 392, in _connect
                  dbc = MySQLdb.connect(**connargs)
                File "/usr/lib64/python3.6/site-packages/MySQLdb/__init__.py", line 86, in Connect
                  return Connection(*args, **kwargs)
                File "/usr/lib64/python3.6/site-packages/MySQLdb/connections.py", line 172, in __init__
                  for k, v in conv.items():
              AttributeError: 'str' object has no attribute 'items'
     Started: 11:15:00.141229
    Duration: 12.662 ms
     Changes:   

@ymasson
Copy link
Contributor Author

ymasson commented Feb 11, 2020

I don't know why, but if i print connargs in salt/modules/mysql.py line 388 (in def _connect). It is not the same.

salt-minion 2019.2.3

{u'read_default_file': u'/etc/mysql/debian.cnf', u'use_unicode': False}

salt-minion 3000

{u'use_unicode': False, u'conv': u'', u'passwd': u'', u'charset': u'', u'db': u'', u'read_default_file': u'/etc/mysql/debian.cnf', u'unix_socket': u'', u'host': u'', u'read_default_group': u'', u'user': u'', u'port': u''}

When it is past to MySQLdb, conv is define but empty.

@garethgreenaway
Copy link
Contributor

Here is a small patch that I believe will get past the above error:

diff --git a/salt/modules/mysql.py b/salt/modules/mysql.py
index 87e2361e28..37436206d4 100644
--- a/salt/modules/mysql.py
+++ b/salt/modules/mysql.py
@@ -385,6 +385,9 @@ def _connect(**kwargs):
     # Ensure MySQldb knows the format we use for queries with arguments
     MySQLdb.paramstyle = 'pyformat'
 
+    if not connargs['conv']:
+        connargs['conv'] = {}
+
     if connargs.get('passwd', True) is None:  # If present but set to None. (Extreme edge case.)
         log.warning('MySQL password of None found. Attempting passwordless login.')
         connargs.pop('passwd')

@bitskri3g
Copy link

@garethgreenaway

Not quite... different error:

mysql-0396f589-cf08-5a44-b192-fd26f04219a8:
----------
          ID: test_db
    Function: mysql_database.present
        Name: testing
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3.6/site-packages/salt/state.py", line 1981, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3.6/site-packages/salt/loader.py", line 1977, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3.6/site-packages/salt/states/mysql_database.py", line 55, in present
                  existing = __salt__['mysql.db_get'](name, **connection_args)
                File "/usr/lib/python3.6/site-packages/salt/modules/mysql.py", line 1038, in db_get
                  dbc = _connect(**connection_args)
                File "/usr/lib/python3.6/site-packages/salt/modules/mysql.py", line 396, in _connect
                  dbc = MySQLdb.connect(**connargs)
                File "/usr/lib64/python3.6/site-packages/MySQLdb/__init__.py", line 86, in Connect
                  return Connection(*args, **kwargs)
                File "/usr/lib64/python3.6/site-packages/MySQLdb/connections.py", line 204, in __init__
                  super(Connection, self).__init__(*args, **kwargs2)
              TypeError: an integer is required (got type str)
     Started: 11:47:32.769888
    Duration: 11.599 ms
     Changes:   

@ymasson
Copy link
Contributor Author

ymasson commented Feb 11, 2020

yes, it is different

~# salt-call mysql.version

Passed invalid arguments: an integer is required.

Usage:

    Return the version of a MySQL server using the output from the ``SELECT
    VERSION()`` query.

    CLI Example:

    .. code-block:: bash

        salt '*' mysql.version

@crimv42
Copy link

crimv42 commented Feb 11, 2020

Getting the same error in Ubuntu 16.04:

Function: mysql_database.present
      Name: pdns
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/state.py", line 1981, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 1977, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/states/mysql_database.py", line 55, in present
                  existing = __salt__['mysql.db_get'](name, **connection_args)
                File "/usr/lib/python3/dist-packages/salt/modules/mysql.py", line 1034, in db_get
                  dbc = _connect(**connection_args)
                File "/usr/lib/python3/dist-packages/salt/modules/mysql.py", line 392, in _connect
                  dbc = MySQLdb.connect(**connargs)
                File "/usr/lib/python3/dist-packages/MySQLdb/__init__.py", line 81, in Connect
                  return Connection(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/MySQLdb/connections.py", line 173, in __init__
                  for k, v in conv.items():
              AttributeError: 'str' object has no attribute 'items'

@garethgreenaway
Copy link
Contributor

Thanks. Will take a look at the new error.

@jeffdyke
Copy link

jeffdyke commented Feb 13, 2020

Apologies for not adding this originally, but i'm on Ubuntu 18.04, all other configurations are the same as @ymasson. Found this on a fresh install of 10.4.12

@garethgreenaway i propose this small change, sorry that its not a diff, but on line 390 of modules/mysql.py i have added

if not connargs.get('conv'):
    log.warning("setting conv to None so pymysql uses built in conversions")
    connargs['conv'] = None

Because of the following in pymysql/connections.py on line 302:

if conv is None:
    conv = converters.conversions 

There is no documentation about what to put in conv and in the 6+ years i've been using salt to handle mysql grants, this is the first time i've seen it complain about this. And no i don't feel strongly about the log.warning that was just for me.

EDIT: Change to if not connargs('conv') b/c _connarg function will set it to an empty string.

@jeffdyke
Copy link

jeffdyke commented Feb 13, 2020

And a diff in case anyone, like myself wants to patch before highstates, i spin these up quite often

diff /usr/lib/python3/dist-packages/salt/modules/mysql.py mysql.py
387,389d386
<     if not connargs.get('conv'):
<         log.warning("setting conv to None so pymysql uses built in conversions")
<         connargs['conv'] = None

@scambra
Copy link

scambra commented Feb 14, 2020

MySQLdb/connections.py (python3-mysqldb) checks conv in different way to use builtin conversions:

        if 'conv' in kwargs:
            conv = kwargs['conv']
        else:
            conv = conversions

So I would propose connargs.pop('conv'). Anyway, it's not the only error. I have checked 2019.2.3 and connargs didn't have any None or '' value. However 3000 has many '' values, because config.option changed and it changes default when is None:

   if default is None:
        default = '' if not wildcard else {}

So I would change _connarg to check '' instead of None:

-            if val is not None:
+            if val != '':
                connargs[key] = val

@scambra
Copy link

scambra commented Feb 14, 2020

Patch

patches/mysql.sls:

{% if grains.saltversion == "3000" %}
patch:
  pkg.installed

mysql_module_patch:
  file.patch:
    - name: '{{ grains.saltpath }}'
    - source: salt://patches/files/3000-mysql.diff
    - strip: 2
    - require:
        - pkg: patch

restart_salt_minion:
  cmd.run:
    - name: 'salt-call service.restart salt-minion'
    - bg: true
    - onchanges:
      - file: mysql_module_patch
{%- endif %}

patches/files/3000-mysql.diff:

diff --git a/salt/modules/mysql.py b/salt/modules/mysql.py
index 87e2361e28..37436206d4 100644
--- a/salt/modules/mysql.py
+++ b/salt/modules/mysql.py
@@ -355,7 +355,7 @@ def _connect(**kwargs):
                 except IndexError:
                     return
             val = __salt__['config.option']('mysql.{0}'.format(name), None)
-            if val is not None:
+            if val != '':
                 connargs[key] = val

     # If a default file is explicitly passed to kwargs, don't grab the

Run sudo MINION state.apply patches.mysql to apply the fix.

@jeffdyke
Copy link

I'll open another issue, but thought here was appropriate as well. With 10.4 and 3000, you need to have a default set of parameters for the minion, i've added this to our custom bootstrap to avoid restarting during a highstate

# This is required for mariadb 10.4+ as root only has socket access, no more empty passwords"
  cat <<db > /etc/salt/minion.d/database-access.conf
mysql.host: 'localhost'
mysql.user: 'root'
mysql.unix_socket: '/var/run/mysqld/mysqld.sock'
mysql.db: 'mysql'
mysql.charset: 'utf8'
db

I'm not sure if this should be solved as another part of the salt code, likely not, but I spin up shared environments quite often and found that having it run during the highstate showed its edge cases.

If this is not there before a restart salt 3K still trys an empty root password, so all clean highstates will fail.

The file.managed for database-access.conf is still part of my mariadb state, so it sees it as unchanged on the first run.

@garethgreenaway
Copy link
Contributor

Apologies for the delay on this one, I ended up addressing the initial issue with conargs and then found a few more when testing it against the various versions of MySQL, MariaDB, and Percona, so I ended up breaking out the various scenarios into their own functions to keep this more maintainable going forward. If anyone is able to test the PR at #56174 to ensure that it fixes the various issues. Thanks!

@bitskri3g
Copy link

That PR looks good - all of my test cases are now passing. Additionally, a few older issues that I've been having (e.g. mysql_user insisting that a mysql user password was incorrect and setting it again even though no change was actually needed) are now also gone. Good fix.

@garethgreenaway garethgreenaway added the fixed-pls-verify fix is linked, bug author to confirm fix label Feb 20, 2020
@sagetherage sagetherage added the v3000.1 vulnerable version label Feb 20, 2020
@Ch3LL
Copy link
Contributor

Ch3LL commented Mar 13, 2020

thanks for confirming the fix. closing with fix here #56174

@RalfJung
Copy link

RalfJung commented Apr 4, 2020

This issue was marked as fixed in "3000.1 Release" planning, but salt 3000.1 was just released and we are still seeing the issue after an upgrade. In which version will the fix be included?

----------
          ID: mariadb_stats_user
    Function: mysql_user.present
        Name: stats
      Result: False
     Comment: State 'mysql_user.present' was not found in SLS 'freifunk-mgmt.mariadb'
              Reason: 'mysql_user' __virtual__ returned False                                                                                                                                                                                
     Changes:   
----------
          ID: mariadb_stats_db
    Function: mysql_database.present
        Name: stats
      Result: False
     Comment: State 'mysql_database.present' was not found in SLS 'freifunk-mgmt.mariadb'
              Reason: 'mysql_database' __virtual__ returned False                                                                                                                                                                            
     Changes:   
----------
          ID: mariadb_stats_grant
    Function: mysql_grants.present
      Result: False
     Comment: One or more requisite failed: freifunk-mgmt.mariadb.mariadb_stats_user, freifunk-mgmt.mariadb.mariadb_stats_db
     Started: 11:17:47.620408
    Duration: 0.008 ms
     Changes:   
$ sudo salt --version
salt 3000.1

@RalfJung
Copy link

RalfJung commented Apr 4, 2020

Oh, turns out we actually had #54702... a python package was missing but the salt error message was not giving any indication of that being the source of the problem.

@timwsuqld
Copy link

@garethgreenaway Happy to open a new issue if needed, my testing of 3000.1 shows this isn't completely fixed. Of note, we are using the python3 packages, not the python2 packages, so maybe that's a factor?

          ID: grant_mysql_zabbixagent
    Function: mysql_user.present
        Name: zabbixagent
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/state.py", line 1981, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 1977, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/states/mysql_user.py", line 152, in present
                  **connection_args):
                File "/var/cache/salt/minion/extmods/modules/mysql.py", line 1266, in user_exists
                  server_version = salt.utils.data.decode(version(**connection_args))
                File "/var/cache/salt/minion/extmods/modules/mysql.py", line 858, in version
                  dbc = _connect(**connection_args)
                File "/var/cache/salt/minion/extmods/modules/mysql.py", line 392, in _connect
                  dbc = MySQLdb.connect(**connargs)
                File "/usr/lib/python3/dist-packages/MySQLdb/__init__.py", line 86, in Connect
                  return Connection(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/MySQLdb/connections.py", line 172, in __init__
                  for k, v in conv.items():
              AttributeError: 'str' object has no attribute 'items'
     Started: 12:03:49.469834
    Duration: 9.348 ms
     Changes: 
$ salt-minion --versions
Salt Version:
           Salt: 3000.1
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: 1.3.10
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.9 (default, Nov  7 2019, 10:44:02)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5
 
System Versions:
           dist: Ubuntu 18.04 bionic
         locale: UTF-8
        machine: x86_64
        release: 5.0.0-1034-gcp
         system: Linux
        version: Ubuntu 18.04 bionic

@timwsuqld
Copy link

@Ch3LL Just pinging you, this doesn't appear to be fixed in 3000.1
#56388 appears to be the same issue too

@Ch3LL Ch3LL reopened this Apr 27, 2020
@Ch3LL
Copy link
Contributor

Ch3LL commented Apr 27, 2020

ping @garethgreenaway can you take a look here ^

@garethgreenaway
Copy link
Contributor

@timwsuqld I haven't been able to reproduce this one with a clean Ubuntu 18.04 install and 3000.1 installed from the packages, using Python 3. This was using a fairly simple state to add a user into MySQL.

@genaumann
Copy link
Contributor

States in MySQL Formula are working for me.

@timwsuqld
Copy link

Sigh. @garethgreenaway please close. I got bitten by my module override from #52045, we'd removed it with 2019.2.1 but when that was pulled we reapplied it, so we were still using the old module (and no longer had our ticket open to remove it).
I wonder if overridden modules should show a warning every run? I'm sure I'm not the first person to have an override and forget about it when a fix is finally out. Salt minions could easily detect that it has modules overridden and warn in the highstate output.

@boltronics
Copy link
Contributor

I personally think an INFO message on startup would be good for anything in _states, _modules, etc. that is acting as an override.

@timwsuqld
Copy link

@boltronics As a bonus, salt-version detecting and alerting to overrides would help when opening a ticket. Maybe I should open a new feature request.

@garethgreenaway
Copy link
Contributor

@timwsuqld @boltronics That would be a very useful feature to have, would one of you mind adding a new issue with the feature request? Thanks! Since everything is looking good on this, I'm going to go ahead and close this issue out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P3 Priority 3 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around v3000.1 vulnerable version
Projects
None yet
Development

No branches or pull requests