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

Drop support for Python 2 unicode string literals in YAML renderer #48309

Merged
merged 27 commits into from Jun 30, 2018

Conversation

Projects
None yet
5 participants
@terminalmage
Member

terminalmage commented Jun 26, 2018

In earlier releases, the below was considered valid usage in Python 2, assuming that data was a dictionary containing keys/values which are unicode types:

/etc/foo.conf:
  file.managed:
    - source: salt://foo.conf.jinja
    - template: jinja
    - context:
        data: {{ data }}

Jinja will render the unicode string types in Python 2 with the "u" prefix (e.g. {u'foo': u'bar'}. While not valid YAML, earlier releases would successfully load these values.

This pull request drops this support, effective in the upcoming Fluorine release. Instead, it will now be necessary to use Jinja's "tojson" filter.

/etc/foo.conf:
  file.managed:
    - source: salt://foo.conf.jinja
    - template: jinja
    - context:
        data: {{ data|tojson }}

The "tojson" filter was added to Jinja in version 2.9, but several LTS releases (RHEL 7, Ubuntu 14.04, etc.) ship with earlier versions in their repositories. Therefore, this PR also adds an implementation of this filter, which will be used if Jinja doesn't already provide it (when it does, the upstream version of the filter will be used).

These changes allow Salt's custom YAML SafeLoader to use the libyaml versions of PyYAML's SafeLoader and SafeDumper (i.e. yaml.CSafeLoader and yaml.CSafeDumper) if PyYAML was built with libyaml support.

Thanks to @jacksontj for working with me on this.

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 26, 2018

My new integration test doesn't take into account dict iteration order differences. Derp. Fixing this as well as a lint violation, and there appear to be some other related failures.

@terminalmage terminalmage force-pushed the terminalmage:fix-yaml-hacks branch from e72f90b to 74e4dbb Jun 26, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 26, 2018

OK, the necessary tests have been updated. I also have deprecated the json_encode_list and json_encode_dict filters, as they don't emit actual JSON, and are superseded by the tojson filter.

@rallytime rallytime requested review from saltstack/team-core Jun 26, 2018

@@ -309,6 +309,26 @@ def to_bool(val):
return False
@jinja_filter('tojson')
def tojson(val, indent=None):

This comment has been minimized.

@isbm

isbm Jun 27, 2018

Contributor

I would suggest to have a few of Unit Tests for this.

This comment has been minimized.

@terminalmage

terminalmage Jun 27, 2018

Member

I decided not to because A) Jinja 2.9+ has this filter built in, and B) the integration test I added already confirms that the content is dumped to JSON (before loading as YAML). I suppose I could add some unit tests just to be thorough.

This comment has been minimized.

@terminalmage

terminalmage Jun 27, 2018

Member

OK, unit test added.

@terminalmage terminalmage force-pushed the terminalmage:fix-yaml-hacks branch from 0786332 to 5a46e3c Jun 27, 2018

terminalmage added some commits Jun 26, 2018

Remove scanner unicode hack
The tojson filter makes this obsolete
Remove unicode literal hack
The tojson filter makes this obsolete
Remove obsolete tests
The hacks that these tested are no longer in place
Deprecate json_encode_dict and json_encode_list jinja filters
These should be replaced with "tojson".
Use eval instead of yaml loading the pprint output
Since the YAML renderer no longer supports this, we can't use it to load
structures with unicode literal strings within them.

terminalmage added some commits Jun 27, 2018

@terminalmage terminalmage requested a review from isbm Jun 27, 2018

@isbm

isbm approved these changes Jun 27, 2018

@terminalmage thanks 😉

terminalmage added some commits Jun 27, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 27, 2018

I backported just the tojson filter to 2018.3 in #48339. I've updated the docs I wrote for this filter to reflect that.

This PR should be good to merge pending test results.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 28, 2018

@terminalmage

This comment has been minimized.

Member

terminalmage commented Jun 29, 2018

@rallytime that should be fixed now. Apparently the jinja sort filter doesn't, you know, sort anything.

@rallytime rallytime merged commit fdd1f24 into saltstack:develop Jun 30, 2018

8 of 14 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
default Pull Requests » Salt PR - Main Build - PY2/PY3 #9867
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #11110 — ABORTED
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #6140 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #20193 — FAILURE
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
WIP ready for review
Details
codeclimate 2 fixed issues
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #26344 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #18390 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #24068 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #23025 — SUCCESS
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
@cristinapauna

This comment has been minimized.

cristinapauna commented on cd051f0 Jul 2, 2018

Kind request to port this fix to 2016.11.9 version.

This comment has been minimized.

Contributor

rallytime replied Jul 2, 2018

Hi @cristinapauna the 2016.11 branch is no longer receiving non-CVE fixes. This fix has been back-ported to the 2018.3 branch, however. The fix will be available in the 2018.3.3 release of Salt.

EDIT:

The commit in question is actually a large change and we will not be backporting that. It needs to stay in the develop branch in order to work through any bugs or unforeseen issues that we don't want to occur for people in a dot release.

Some of this change was backported to 2018.3 in #48339, but not the yaml loader portion.

rallytime added a commit that referenced this pull request Jul 2, 2018

Merge pull request #48339 from terminalmage/backport-tojson-filter
Backport tojson filter from #48309 to 2018.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment