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

Fix 50227 malformed plists #50228

Merged
merged 10 commits into from Nov 6, 2018

Conversation

Projects
None yet
4 participants
@sheagcraig
Copy link
Contributor

commented Oct 25, 2018

What does this PR do?

This PR updates the tests for the mac_utils module to not incorrectly pass malformed binary plists and then fixes the handling of that situation in the mac_utils module itself.

What issues does this PR fix or reference?

#50227

Previous Behavior

Raised unhandled plistlib.InvalidFileException.

New Behavior

Skips the poorly formed plist file by continue-ing the for loop.

Tests written?

Yes (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.

else:
plist = plistlib.loads(
salt.utils.stringutils.to_bytes(plist_xml))
except Exception:

This comment has been minimized.

Copy link
@cachedout

cachedout Oct 25, 2018

Collaborator

We usually don't like bare excepts. Do you know the specific type(s) of exceptions that can be expected here?

This comment has been minimized.

Copy link
@sheagcraig

sheagcraig Oct 25, 2018

Author Contributor

Hah, I'm with you on that. I actually put that in there because the previous block used Exception. I thought that was a little strange. I'll push some changes to specify the actually expected exceptions tomorrow.

@sheagcraig

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

@cachedout and anybody else, this is a little more involved than I thought, and I need some guidance.

Two main questions:

  1. It appears to me like the available_services function as written is adding in a service even if the plist file is malformed. I doubt in practice that you could ever load a service if the plist file was invalid XML, so I'm not sure this is useful or expected behavior. My instinct would be to not show a service in the list if it's not possible to load it (due to invalid XML). Is this the actual intention, or should it actually not include services which cannot load?
  2. The tests seem to affirm this, and they seem to be copy & paste duplicates from the other tests, with just minor edits for each test. But with more explicit exception handling, I've got at least 3 different "malformed XML" situations to test for (py2, py3 completely invalid, py3 broken tags / mostly valid XML). Is the goal to keep the tests essentially the same, or do I have the ability to break them up into more tests?
@sheagcraig

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Hi-wondering if I can get some clarification on the following point. Currently the mac_utils services list includes services which can't actually start due to invalid plist files. The code handles this by falling back to just using the filename to populate the list of services (rather than the ID property from inside the file).

This seems wrong to me, and I'd be fine with updating the code to exclude services which can't start from the list of available services. This involves rewriting some tests as well.

OR, if the goal is still to include those services, I can revert back mac_utils and just fix the exception handling for malformed XML cases and update the tests to pass with the existing behavior.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

@Ch3LL or @weswhet can you help out with the questions above?

@weswhet

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

@sheagcraig I would say go for if the plist is malformed to NOT include it in the available services. As it technically wouldn't be available. I would probably throw a warning or error to let folks know that it's a bad file.

@rallytime could we add this into 2018 as it is a bug fix?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

@weswhet Yes, I've marked it for back-porting. Thanks!

@sheagcraig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

I'll get on finishing this up today then. Thanks for the feedback!

@sheagcraig sheagcraig force-pushed the sheagcraig:fix_50227_malformed_plists branch from 1be6942 to 38ccbb5 Nov 6, 2018

sheagcraig added some commits Oct 25, 2018

Fix trailing-slash bug in User LaunchAgent appending.
I discovered this issue while updating the tests, so there's no open
issue for it.
Update testing for mac_utils services.
This commit begins to go over the existing testing for mac_utils,
applying the updated best-practices from the unit testing section of the
Salt docs.

Wherever possible, I've tried to keep the data for each test within the
test, while factoring out repetitive functions. In this commit, for
example, the `os.walk` side_effect func has been factored into its own
helper function.

This commit begins looking at what is being tested for this module and
making it more specific, specifically to test one thing per test
(existing tests had multiple asserts and in some casaes were actually
malformed such that the tests would pass, but in actual use, code would
throw an exception and skip over the block).

@sheagcraig sheagcraig force-pushed the sheagcraig:fix_50227_malformed_plists branch from eabeebb to 45c4222 Nov 6, 2018

@sheagcraig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Okay-I think this is ready for a look again.

@weswhet

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

LGTM!!! Thanks, bud!

@rallytime rallytime requested a review from cachedout Nov 6, 2018

@cachedout
Copy link
Collaborator

left a comment

LGTM. Nice work here, @sheagcraig

@cachedout cachedout merged commit 38d6c5e into saltstack:develop Nov 6, 2018

10 of 11 checks passed

jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
WIP Ready for review
Details
codeclimate Approved by Mike Place.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details

rallytime added a commit that referenced this pull request Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.