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
56 changes: 44 additions & 12 deletions salt/utils/jinja.py
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
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
@@ -0,0 +1 @@
FAILURE
1 change: 1 addition & 0 deletions tests/unit/templates/files/test/relative/rescape
@@ -0,0 +1 @@
{% import '../../rescape' as xfail -%}
2 changes: 2 additions & 0 deletions tests/unit/templates/files/test/relative/rhello
@@ -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
@@ -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
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