fix returners.postgres inability to handle byte string responses

It would probably make sense for something like this to be in a util for other returners to access. I have updated for the three mentioned returners in https://github.com/saltstack/salt/issues/55226

What does this PR do?

What issues does this PR fix or reference?

Fixes: https://github.com/saltstack/salt/issues/55226

Previous Behavior

Error when supplied with binary data. for example nacl.enc_file would always cause this

[CRITICAL] The specified 'postgres' returner threw a stack trace
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/utils/job.py", line 140, in store_job
    mminion.returners[fstr](load)
  File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 149, in __call__
    return self.loader.run(run_func, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 1201, in run
    return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 1216, in _run_as
    return _func_or_method(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/returners/postgres.py", line 242, in returner
    salt.utils.json.dumps(ret["return"]),
  File "/usr/lib/python3/dist-packages/salt/utils/json.py", line 137, in dumps
    return json_module.dumps(obj, **kwargs)
  File "/usr/lib/python3.8/json/__init__.py", line 234, in dumps
    return cls(
  File "/usr/lib/python3.8/json/encoder.py", line 199, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python3.8/json/encoder.py", line 257, in iterencode
    return _iterencode(o, 0)
  File "/usr/lib/python3.8/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type bytes is not JSON serializable

New Behavior

convert the ['return']['return'] element to an actual string instead of bytes

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• [ ] Docs
• [ ] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• [ ] Tests written/updated

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

I'm just looking at this on my phone so I have no idea if this comment is valid, but maybe there's an encoding/raw flag we should be passing to the postgres constructor/connection?

Howdy! Im not sure that comment is valid (the issue happens before the db insert). I understand where that view is coming from though and it would be related to the schema, however the scheme for this field is (by many examples) is just a varchar field that stores a json dump.

The issue is that the salt python code is blowing up before the db is involved as its the in-python conversion of dict to json thats failing to happen as the returned dict contains a bytes object (for example a salt-cli nacl encode file response).

Gotcha, that makes sense, and on a computer where I can actually see and grok this code a bit... yeah, I think this makes sense to be delegated to each of the returners that are storing data in a non-binary-able format.

But... it may actually make sense to extract that functionality into a utility, if we don't already have that within Salt :thinking:

I agree it should probably be moved to a util. I actually brought that up in the pr message.

@waynew I have de-duped the code and created a method for it in salt.returners. it is now a much more minimal change-set to the returners. I have not however read through the rest of the returners to see if they also need the fix implemented. If you believe we already have this functionality elsewhere, it should be quick to update to an alternative method

I'm just heading out for some PTO, but I did a cursory glance and there does exist salt.utils.data.decode -- does that do what we need here?

I haven't done a full audit of the other returners (mostly operating under the assumption that as I work through the testing I'd expose any issues here), but at least at a glance my thought is that it makes sense for each individual returner to make that distinction themselves.

For example, if I'm storing data in a JSON format, I can't actually store it as binary data without some kind of conversion, like base64 encoding.

OTOH, if I'm storing data as msgpack or pickle, well I can store all kinds of binary data just fine.

So it makes sense to me that each of the returners should be deciding if they need to encode the payload or not -- and I'm totally fine with making an incremental change like these particular ones here and then converting other ones later, if necessary :+1:

Enjoy the pto!

So i tried using salt.utils.data.decode eg: json_data = salt.utils.json.dumps(salt.utils.data.decode(load)) but it did not work. Ive also tried with to_str enabled but it also did not work

root@saltdirector:~# salt-run nacl.enc data='test'
[CRITICAL] The specified 'postgres' returner threw a stack trace
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/utils/job.py", line 140, in store_job
    mminion.returners[fstr](load)
  File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 149, in __call__
    return self.loader.run(run_func, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 1201, in run
    return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 1216, in _run_as
    return _func_or_method(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/returners/postgres.py", line 243, in returner
    salt.utils.json.dumps(ret["return"]),
  File "/usr/lib/python3/dist-packages/salt/utils/json.py", line 137, in dumps
    return json_module.dumps(obj, **kwargs)
  File "/usr/lib/python3.8/json/__init__.py", line 234, in dumps
    return cls(
  File "/usr/lib/python3.8/json/encoder.py", line 199, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python3.8/json/encoder.py", line 257, in iterencode
    return _iterencode(o, 0)
  File "/usr/lib/python3.8/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type bytes is not JSON serializable
<removed nacl encoded output>

def save_load(jid, load, minions=None):  # pylint: disable=unused-argument
    """
    Save the load to the specified jid id
    """
    with _get_serv(commit=True) as cur:

        sql = """INSERT INTO jids
               (jid, load)
                VALUES (%s, %s)"""
        json_data = salt.utils.json.dumps(salt.utils.data.decode(load, to_str=True))
        try:
            cur.execute(sql, (jid, json_data))
        except psycopg2.IntegrityError:
            # https://github.com/saltstack/salt/issues/22171
            # Without this try/except we get tons of duplicate entry errors
            # which result in job returns not being stored properly
            pass

@waynew seems to have looked at this and agrees with this approach. It needs a changelog and test (something simple, unit is fine), and then it'll be good to go!

@ITJamie Any possibility you could add a test and changelog here? Thanks!

Ive moved the function into salt/utils/jid's

this seems like a sanish place for it, its already being imported into the required returners + already had unit tests i could extend.

There were no tests for (these) returners to extend. I believe @waynew will be added those in future and can deeper check per returner type

I just tried to reproduce it locally, and it worked with salt.utils.data.decode. It seems your error msg and the function you pasted are unrelated because save_load is not called inside returner. Can you try using salt.utils.data.decode for your cleaned_return in returner?

To test this, I created a /tmp/foo with some text in it and did salt \* file.read /tmp/foo binary=True.

Also, I forgot to mention this, but your solution seems to be breaking on something with a simple non-dict return, for instance with a test.ping. Using salt.utils.data.decode should handle all of those cases.

@ITJamie Any updates here?