[BUG] 3000.8 MySQL module does no longer return bytes

[yiip87]

Description
After upgrading from Salt 2019.2.8 to 3000.8 the mysql module returns strings instead of bytes.
This issue is still present in master.

Steps to Reproduce the behavior
Just call salt-call -l debug mysql.db_list on 2019.2.8 and 3000.8 and observe the debug output.

2019.2.8:

[DEBUG   ] Doing query: SHOW DATABASES
[DEBUG   ] [b'information_schema', b'mysql', b'performance_schema']

3000.8:

[DEBUG   ] Doing query: SHOW DATABASES
[DEBUG   ] ['information_schema', 'mysql', 'performance_schema']

Expected behavior
The returned data should be bytes not str.

Versions Report

salt --versions-report for 2019.2.8

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

$ salt --versions-report
Salt Version:
           Salt: 2019.2.8
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.7.3
      docker-py: Not Installed
          gitdb: 2.0.5
      gitpython: 2.1.11
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 1.0.7
   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.7.3 (default, Jul 25 2020, 13:03:44)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 17.1.2
           RAET: Not Installed
          smmap: 2.0.5
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.1
 
System Versions:
           dist: debian 10.8 
         locale: UTF-8
        machine: x86_64
        release: 4.19.0-14-amd64
         system: Linux
        version: debian 10.8

salt --versions-report for 3000.8

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

$ salt --versions-report
Salt Version:
           Salt: 3000.8
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.7.3
      docker-py: Not Installed
          gitdb: 2.0.5
      gitpython: 2.1.11
         Jinja2: 2.10
        libgit2: Not Installed
       M2Crypto: Not Installed
           Mako: 1.0.7
   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.7.3 (default, Jul 25 2020, 13:03:44)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 17.1.2
          smmap: 2.0.5
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.1
 
System Versions:
           dist: debian 10.8 
         locale: UTF-8
        machine: x86_64
        release: 4.19.0-14-amd64
         system: Linux
        version: debian 10.8 

Additional context
This issue is caused by the following code from https://github.com/saltstack/salt/blob/v3000.8/salt/modules/mysql.py#L424-L426 which was added with e871a3f in #56174:

    for key in copy.deepcopy(connargs):
        if not connargs[key]:
            del connargs[key]

While this removes all connection arguments that only contain an empty string, it also removes the

connargs["use_unicode"] = False

which, according to the comment in the file, was set deliberately set to False.

Digging a little bit more, I found that the actual cause of this is a change in config.option, introduced with 16ea106 in #55637. Here, the default value of the default parameter was changed from '' to None, only to change it further down either back to '' or to {} depending on whether wildcard support is enabled or not.

This leads to unexpected behavior and also breaks all calls to this function, that previously set None explicitly, like for example in the mysql module right here.

__salt__['config.option']('foo', None)

now returns an empty string instead of None as instructed.

--

I think the "proper" fix would be to change the config module as follows:

--- /usr/lib/python3/dist-packages/salt/modules/config.py	2021-02-08 15:37:42.000000000 +0100
+++ config.py	2021-03-09 15:27:13.214106078 +0100
@@ -136,7 +136,7 @@
 
 def option(
         value,
-        default=None,
+        default='',
         omit_opts=False,
         omit_grains=False,
         omit_pillar=False,
@@ -200,8 +200,8 @@
     if omit_all:
         omit_opts = omit_grains = omit_pillar = omit_master = True
 
-    if default is None:
-        default = '' if not wildcard else {}
+    if wildcard:
+        default = {}
 
     if not wildcard:
         if not omit_opts:

and remove the deepcopy for-loop in the mysql module:

--- /usr/lib/python3/dist-packages/salt/modules/mysql.py	2021-02-08 15:37:42.000000000 +0100
+++ mysql.py	2021-03-09 15:30:08.941816096 +0100
@@ -421,10 +421,6 @@
     # Ensure MySQldb knows the format we use for queries with arguments
     MySQLdb.paramstyle = 'pyformat'
 
-    for key in copy.deepcopy(connargs):
-        if not connargs[key]:
-            del connargs[key]
-
     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')

With this change, config.option no longer silently swallows None and the mysql module is returning bytes again.

[welcome]

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

• Community Wiki
• Salt’s Contributor Guide
• Join our Community Slack
• IRC on Freenode
• SaltStack YouTube channel
• SaltStackInc Twitch channel

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

[piterpunk]

I reopened this issue as the "fix" doesn't fix the bug, and makes things worst. Previously we're returning strings instead bytes, now we are returning wrong strings instead bytes.

While the change in use_unicode parameter correctly made MySQL to return data as bytes, we are later converting the bytes objects to strings. So, instead to return b'something' the returned data is "b'something'".

Already have a patch to fix it, but will do a bit more testing before send the PR.

[piterpunk]

@yiip87

Probably you saw the problem that I mentioned in my last comment. While fixing it a question bugged me: all data returned by mysql module should be byte streams? Or you're only interested in the output of mysql.query to be a byte stream?

I am asking this because the need to encode/decode every string doesn´t seems reasonable for daily use. Let's say I need to compare the user privileges, instead 'Y' or 'N' I need to compare with b'Y' and b'N'. The same to check if a plugin is use, checking for b'ACTIVE' seems counter-intuitive.

@garethgreenaway as you did most of this module recent work, what do you think about this?

[yiip87]

@piterpunk

We use a module that wraps some of the functionality of the mysql module. This module broke with the upgrade from 2019.2.8 to 3000.8. My initial goal was to get the old 2019.2.8 behavior restored, because I was unable to find anything related to it in the changelogs.

Thinking about this again, this was also the time where the transition from Python 2 to Python 3 happened. As the comment states, the code wanted to avoid unicode() objects, because strings where just bytes in Py2. So maybe with strings being Unicode in Py3 it would make more sense to make the module return strings? In this case, the code trying to avoid Unicode could be removed entirely.

It would be nice to get some insight from the Salt devs on this.

[piterpunk]

@sagetherage sorry to be annoying with this issue, but the merge that I did to return bytes in reality returns totally broken results. It's important to not release the mysql execution module as-is now.

I have two different codes fixing the issue, but need a guide of which path to follow. Also, as @yiit87 points out, maybe the code actually doesn't need to return bytes after the Py2 to Py3 migration.

I want to know what to do, probably some guidance from Core is needed.

If we don't have enough time to discuss it, at least should be good to revert #60456 as the previous behaviour, returning strings, is better than the current one.