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

file.serialize state: fix corruption when using msgpack serializer #57858

Merged
merged 2 commits into from Jul 30, 2020

Conversation

terminalmage
Copy link
Collaborator

@terminalmage terminalmage commented Jul 2, 2020

The file.serialize state adds a newline to the end of the serialized contents before writing them to disk. This works fine with some serializers, but not so well with others. MacOS plist files are invalid with a trailing newline, so a recent release added a check for that. But that still leaves msgpack, which is also corrupted by the addition of a trailing newline (resulting in a "received extra bytes" error when one attempts to unpack the file).

This changeset keeps the state from adding the newline to the end of the file when using the msgpack serializer. In doing so, it simplifies the logic that determines whether or not to add the newline. It makes the assumption that if the serialized contents are a bytestring (as in the plist and msgpack serializers), a newline should not be added, as the results are likely in a binary format that will not work well with unexpected bytes added.

Additionally, it introduces a new argument to the state: serializer. When the file.serialize state was added to Salt nearly 7 years ago, it for some reason was written to use formatter to specify which serializer to use. Which is fine, in its own quirky way, but later on the serializer_opts and deserializer_opts arguments were added to allow the user to pass through additional options to the serializer. And this leaves you with serializer_opts/deserializer_opts, and formatter instead of serializer. It's weird and asymmetric. So this changeset also adds serializer (updating docs to prefer this usage), while maintaining backwards compatibility with formatter.

@terminalmage terminalmage requested a review from a team as a code owner Jul 2, 2020
@ghost ghost requested review from DmitryKuzmenko and removed request for a team Jul 2, 2020
rallytime
rallytime previously approved these changes Jul 2, 2020
terminalmage added 2 commits Jul 3, 2020
The `file.serialize` state adds a newline to the end of the serialized
contents before writing them to disk. This works fine with some
serializers, but not so well with others. MacOS plist files are invalid
with a trailing newline, so a recent release added a check for that. But
that still leaves msgpack, which is also corrupted by the addition of a
trailing newline (resulting in a "received extra bytes" error when one
attempts to unpack the file).

This changeset keeps the state from adding the newline to the end of the
file when using the msgpack serializer. In doing so, it simplifies the
logic that determines whether or not to add the newline. It makes the
assumption that if the serialized contents are a bytestring (as in the
plist and msgpack serializers), a newline should not be added, as the
results are likely in a binary format that will not work well with
unexpected bytes added.

Additionally, it introduces a new argument to the state: `serializer`.
When the `file.serialize` state was added to Salt nearly 7 years ago, it
for some reason was written to use `formatter` to specify which
serializer to use. Which is fine, in its own quirky way, but later on
the `serializer_opts` and `deserializer_opts` arguments were added to
allow the user to pass through additional options to the serializer. And
this leaves you with `serializer_opts`/`deserializer_opts`, and
`formatter` instead of `serializer`. It's weird and asymmetric. So this
changeset also adds `serializer` (updating docs to prefer this usage),
while maintaining backwards compatibility with `formatter`.
@dwoz dwoz merged commit 0b3b783 into saltstack:master Jul 30, 2020
26 checks passed
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants