Wrap msgpack

[waynew]

What does this PR do?

Note: We definitely want to run the full test suite on this

The msgpack API changed, and is in the process of changing - it will probably stabilize with version 1.0.

We don't specify a minimum msgpack version, and newer versions warn about future API changes.

This PR replaces all of the existing msgpack references with references to salt.payload, which has existed for a while as a msgpack wrapper, but hasn't been globally used. It also extends the wrapper to fully support the warning-free API.

What issues does this PR fix or reference?

#50994
#51398
#51398
#51741
#51666

There may be more...

Previous Behavior

On more modern versions of msgpack (>=0.5.2), a warning was raised when the depreciated encoding parameter was sent.

New Behavior

According to https://github.com/msgpack/msgpack-python:

When msgpack 1.0, I planning these breaking changes:

• packer and unpacker: Remove encoding and unicode_errors option.
• packer: Change default of use_bin_type option from False to True.
• unpacker: Change default of raw option from True to False.
• unpacker: Reduce all max_xxx_len options for typical usage.
• unpacker: Remove write_bytes option from all methods.

Now we use raw=True if an encoding is not specified, otherwise we use raw=False.

Tests written?

Yes - added a couple, and the other changes should be covered by existing tests.

Commits signed with GPG?

Yes

[waynew]

re-run full all

[waynew]

re-run full all

[s0undt3ch]

Looks good, though I'd prefer msgpack specific handling to be on its own msgpack utils module or similar, and not mix it with salt's payload module.

Having that would allows us to transparently "fix" the lack of python set's support(see #53350) by converting it to a list for example.

[waynew]

@s0undt3ch I was actually going to do that and then I came across the payload module that had comments explicitly stating that it was wrapping msgpack... it just wasn't completely wrapping it, nor was it used everywhere. 🤷‍♂

We could make that change here within this module. Though it also sounds like maybe there's a different approach for that? msgpack/msgpack-python#241

[mattp-]

this PR confuses me. a few things:

• why is this going into 2018?
• doesn't roughly similar code exist in develop?
• how does this interact with threadlocalproxy?

[waynew]

@mattp- historically the approach has been to push bug fixes to the oldest affected branches, where they would be continually merged forward.

There's some discussion around changing the practice, as we've seen very real problems due to that approach.

"similar" code, sort of. I've seen probably at least 4-5 PRs patching one instance or another where we patched a particular call to msgpack. We have also had the start of this particular wrapping code in question for quite some time, we just haven't been strict in its use. The irony is that if that we had used this payload code in the first place this would have been a 10 second fix to just change the adapter like each of those (incomplete) PRs has done.

[waynew]

As far as interaction with threadlocalproxy goes - I'm not sure? That's probably another piece of interaction that needs to be properly tested.

[mattp-]

I would encourage you to look at the changes in the develop/neon branch as its doing the same thing, and I think your changes conflict with it

[waynew]

@mattp every branch has some subset of these changes.

From the existing code it's clear that at one point we intended to wrap msgpack, it's also clear that was never completely enforced, so now we have a situation where we have several different places wrapping msgpack, but I don't believe that any of them have collected all the places.

Given our history of wanting to support as wide a range of dependencies as possible, and that (afaict) the underlying msgpack hasn't changed, just the API, it makes a lot of sense to me to go ahead and finish the original concept, wrap msgpack like we started, and replace all existing uses with the wrapped version.

I didn't find any more history, or it escapes my memory, but if you have any more light to shed on the subject, I very much welcome it! (All this to say conflicts are expected 🙃)

[austinpapp]

to @s0undt3ch's point, the util exists in develop. https://github.com/saltstack/salt/blob/develop/salt/utils/msgpack.py

a few spot checks of the mods you updated use the msgpack util...

[waynew]

Ah! That is new. And ironically just stuck a wrapped msgpack inside payload instead of unifying them 😂

[waynew]

@Akm0d pretty sure you had a PR that changed things a bit - I don't remember if I have anything in this that wasn't present there.