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

Support Jinja imports with relative paths. Issue #13889 #47490

Merged
merged 16 commits into from
Jun 8, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 44 additions & 12 deletions salt/utils/jinja.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,28 +98,60 @@ def check_cache(self, template):
self.cached.append(template)

def get_source(self, environment, template):
# checks for relative '..' paths
if '..' in template:
log.warning(
'Discarded template path \'%s\', relative paths are '
'prohibited', template
)
raise TemplateNotFound(template)
'''
Salt-specific loader to find imported jinja files.

Jinja imports will be interpreted as originating from the top
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import behavior needs to be in the documentation. Where is the best location for it?

of each of the directories in the searchpath when the template
name does not begin with './' or '../'. When a template name
begins with './' or '../' then the import will be relative to
the importing file.

'''
# FIXME: somewhere do seprataor replacement: '\\' => '/'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Translation of \ => / might be broken. Help me, Obi Juan, you're my only hope!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwoz @twangboy When using jinja imports on Windows, are Windows-specific path-separators supported? If so, are they the only ones supported, or are forward slashes also supported? I would assume both are supported, since they are in Python proper, but I don't want to assume without knowing for sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use Windows separators in my Jinja. I don't even have to escape them. Jinja apparently does some escaping for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tpldir = os.path.dirname(template).replace('\\', '/') construct was in the original code and appears to do some path translation on Windows. I'm not sure how to test it to ensure that I haven't broken the functionality. Is there a unit test?

_template = template
if template.split('/', 1)[0] in ('..', '.'):
is_relative = True
else:
is_relative = False
# checks for relative '..' paths that step-out of file_roots
if is_relative:
# Starts with a relative path indicator

if not environment or 'tpldir' not in environment.globals:
log.warning(
'Relative path "%s" cannot be resolved without an environment',
template
)
print('Relative path "{0}" cannot be resolved without an environment'.format(template))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be printing to the CLI here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's embarrassing - that should have been removed.

raise TemplateNotFound
base_path = environment.globals['tpldir']
_template = os.path.normpath('/'.join((base_path, _template)))
if _template.split('/', 1)[0] == '..':
log.warning(
'Discarded template path "%s": attempts to'
' ascend outside of file roots', template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have multiple backends, we can't assume people will be using file_roots, so this log message should reference salt:// instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

)
raise TemplateNotFound(template)

self.check_cache(template)
self.check_cache(_template)

if environment and template:
tpldir = os.path.dirname(template).replace('\\', '/')
tpldir = os.path.dirname(_template).replace('\\', '/')
tplfile = _template
if is_relative:
tpldir = environment.globals.get('tpldir', tpldir)
tplfile = template
tpldata = {
'tplfile': template,
'tplfile': tplfile,
'tpldir': '.' if tpldir == '' else tpldir,
'tpldot': tpldir.replace('/', '.'),
}
environment.globals.update(tpldata)

# pylint: disable=cell-var-from-loop
for spath in self.searchpath:
filepath = os.path.join(spath, template)
filepath = os.path.join(spath, _template)
try:
with salt.utils.files.fopen(filepath, 'rb') as ifile:
contents = ifile.read().decode(self.encoding)
Expand Down Expand Up @@ -205,7 +237,7 @@ def skip_filter(data):
'''
Suppress data output

.. code-balock:: yaml
.. code-block:: yaml

{% my_string = "foo" %}

Expand Down
2 changes: 1 addition & 1 deletion tests/support/parser/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ def parse_args(self, args=None, values=None):
os.path.basename(fpath).startswith('test_'):
self.options.name.append(fpath)
continue
self.exit(status=1, msg='\'{}\' is not a valid test module'.format(fpath))
self.exit(status=1, msg='\'{}\' is not a valid test module\n'.format(fpath))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand this change? I don't know what adding a newline here fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the newline then the error message is not on a line by itself and the command prompt is left on the same line as the error message.


print_header(u'', inline=True, width=self.options.output_columns)
self.pre_execution_cleanup()
Expand Down
1 change: 1 addition & 0 deletions tests/unit/templates/files/rescape
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
FAILURE
1 change: 1 addition & 0 deletions tests/unit/templates/files/test/relative/rescape
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% import '../../rescape' as xfail -%}
2 changes: 2 additions & 0 deletions tests/unit/templates/files/test/relative/rhello
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{% from './rmacro' import rmacro with context -%}
{{ rmacro('Hey') ~ rmacro(a|default('a'), b|default('b')) }}
4 changes: 4 additions & 0 deletions tests/unit/templates/files/test/relative/rmacro
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{% from '../macro' import mymacro with context %}
{% macro rmacro(greeting, greetee='world') -%}
{{ mymacro(greeting, greetee) }}
{%- endmacro %}
17 changes: 17 additions & 0 deletions tests/unit/templates/test_jinja.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,23 @@ def test_import(self):
self.assertEqual(fc.requests[0]['path'], 'salt://hello_import')
self.assertEqual(fc.requests[1]['path'], 'salt://macro')

def test_relative_import(self):
'''
You can import using relative paths
issue-13889
'''
fc, jinja = self.get_test_saltenv()
tmpl = jinja.get_template('relative/rhello')
result = tmpl.render()
self.assertEqual(result, 'Hey world !a b !')
assert len(fc.requests) == 3
self.assertEqual(fc.requests[0]['path'], 'salt://relative/rhello')
self.assertEqual(fc.requests[1]['path'], 'salt://relative/rmacro')
self.assertEqual(fc.requests[2]['path'], 'salt://macro')
# This must fail when rendered: attempts to import from outside file root
template = jinja.get_template('relative/rescape')
self.assertRaises(exceptions.TemplateNotFound, template.render)

def test_include(self):
'''
You can also include a template that imports and uses macros
Expand Down