Using the include directive in vault template

[weakcamel]

vault template command crashes with a stacktrace when you try to use it against a Jinja2 template file which contains include, for example:

file1:

foo {% include 'file2' %} bar

file2:

whatever

The problematic code is here: https://github.com/peopledoc/vault-cli/blob/master/vault_cli/client.py#L465

From https://jinja.palletsprojects.com/en/2.10.x/api/#jinja2.Template

Normally the template object is generated from an Environment but it also has a constructor that makes it possible to create a template instance directly using the constructor. It takes the same arguments as the environment constructor but it’s not possible to specify a loader.

See a discussion on similar subject here: https://stackoverflow.com/questions/39288706/jinja2-load-template-from-string-typeerror-no-loader-for-this-environment-spec

Sample stacktrace:

$ vault --token-file=~/.vault-token template config.yml.j2
Traceback (most recent call last):
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/bin/vault", line 10, in <module>
    sys.exit(main())
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/vault_cli/cli.py", line 500, in main
    return cli()
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/click/decorators.py", line 27, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/contextlib.py", line 74, in inner
    return func(*args, **kwds)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/vault_cli/cli.py", line 476, in template
    result = client_obj.render_template(template.read())
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/vault_cli/client.py", line 74, in wrapper
    return method(self, *args, **kwargs)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/vault_cli/client.py", line 465, in render_template
    return jinja2.Template(template).render(vault=vault)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/jinja2/asyncsupport.py", line 76, in render
    return original_render(self, *args, **kwargs)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/foouser/.local/share/virtualenvs/config-8gAdPw7v/lib/python3.7/site-packages/jinja2/_compat.py", line 37, in reraise
    raise value.with_traceback(tb)
  File "<template>", line 11, in top-level template code
TypeError: no loader for this environment specified

[ewjoachim]

I never imagined something like this would be done, but it's not unreasonable. I'm guessing you'd be expecting a jinja2.FileSystemLoader ?

Would you be willing to make a PR for this ? it doesn't have to be perfect, and we'll be available to help along the way. Otherwise, we can do it too.

[ewjoachim]

By the way, what should the search path be for the loader, according to you ? The folder of the templated file ? The current folder ? / (or any other way to require absolute paths) ?

[weakcamel]

Hello,
Yes, this wouldn't be a first use case to come to my mind, but turned out it could be useful.
In my case I'm trying to use vault-cli to insert credentials from Vault into a config file for GitlabForm before the config is applied.

Being able to use {% include ... %} allows to break the config into smaller, more manageable pieces; on its own, GitlabForm doesn't support (yet) multiple config files.

[weakcamel]

I'm guessing you'd be expecting a jinja2.FileSystemLoader ?

I believe so, this would be most consistent with how vault-cli template works (point to a file in filesystem).

By the way, what should the search path be for the loader, according to you ? The folder of the templated file ? The current folder ? / (or any other way to require absolute paths) ?

This is indeed tricky; Jinja2 docs don't even clarify whether include allows to use relative paths too (and if they're relative then relative from which path).

I would be inclined to assume that the parameter to include is either an absolute path, or relative from the folder of the templated file. Still not quite determined about it :-)

[ewjoachim]

Thanks for the pointers :D

I can't help but notice a question you left behind:

Would you be willing to make a PR for this ? it doesn't have to be perfect, and we'll be available to help along the way. Otherwise, we can do it too.

I'm really comfortable doing it if you don't feel like it, but wouldn't want to miss an opportunity to onboard people on the project

[weakcamel]

I can't help but notice a question you left behind:

He he, indeed. I was trying to work out if I can do as part of my day job, so as usual it takes a few hoops and loops to jump through. But yes, I just got that approved.

BTW, I started drafting something and it seems slightly more complicated than I expected, but of course no rocket science either. Seems like there'll be two slightly different cases depending if input is a file or STDIN.

[weakcamel]

@ewjoachim please see the 2 drafts above - do the approaches make sense (and if yes, which of them better fits into design of vault-cli) ?

[ewjoachim]

Hello :)

First allow me to say how sorry I am for not answering you quicker. Last week has been quite a ride as the company I work with (PeopleDoc, which has been sponsoring this project so far) held a week-long event, during which I had nearly no access to a computer :)

Now that's over, I want to thank you for taking the time to do this. Reviewing your PRs will among my priorities of today.

[weakcamel]

Hello,

And no worries at all! I'm fully aware that you're most likely donating a lot of your private time to this and other projects, so I appreciate a lot that vault-cli is available at all in the first place. Take your time and once I have your opinion, I'll see how to make the better of options into something more civilised (and tested). Of course, if you have any other approach in mind, happy to switch to that instead.

[weakcamel]

For the record: I was worried that using FileSystemLoader might make it not possible to load a template directly from stdin, but there seems to be a way. Example:

from jinja2 import Environment, FileSystemLoader

env = Environment(loader=FileSystemLoader('./'))
rtemplate = env.from_string("{% include('foo.j2') %} Baz")

templ = env.get_template('foo.j2')
print(templ.render(a='aaaa'))
print(rtemplate.render(a='bbb'))

[ewjoachim]

Ha yes, I was looking at the doc and was about to suggest using environment.from_string, I’m happy that you reached the same conclusion, this should allow implementing the issue at hand with minimal change in the architecture, so I’m definitely a huge supporter of this way !

Thank you a lot for all the work so far. I’m really glad you could make it. Let’s see what a PR with this solution would look like :) At any point, if you feel like you do not want to continue, just say it. Otherwise, GO FOR IT :D

[ewjoachim]

Hello,

I'm not sure if I was clear in what I advocated in the previous message. I think the way to go is to modify only render_template, instead of:

return jinja2.Template(template).render(vault=vault)

have something along the lines of:

env = jinja2.Environment(loader=jinja2.FileSystemLoader('./'))
return env.from_string(template).render(vault=vault)

Then, we just need an integration test, a quick mention in the README and we're good :)

[weakcamel]

Ah, indeed! Let me try that.

The one potential issue I see with that would be always using a path relative to the current working directory.

From CLI's point of view it makes sense, however for a writer of the templates this might be a bit awkward (while writing a template, you can't anticipate the working directory of someone using that template). It might get especially awkward to use if someone did e.g.

$ cd /home/homer
$ ls /tmp/my-templates/location
foo.j2
included_into_foo.j2
$ vault template /tmp/my-templates/location/foo.j2 

{% include('included_into_foo.j2') %} wouldn't work very well

P. S. For me personally. it would be sufficient - so if you're happy with that (documented) limitation, that works for me ;-)

[weakcamel]

CC: @bradphipps @jrg1381

[ewjoachim]

I'm happy to start that way. Also, we could expose an additional root_dir argument to the render_template method, and fill it in the CLI where we probably know the value (probably using template.name where template is the file handler, and pathlib to find the dirname of this path):

This way, we get the best of both worlds.

[weakcamel]

I like the idea of additional parameter; it kind of follows the "explicit rather than implicit" rule (at least in the library part), at the same time being convenient enough to use in CLI.

I suppose if there's ever demand, an extra sub-option could be added to the template command on CLI to override that.

@ewjoachim please see #116 then :-) hopefully it's getting closer?

[ewjoachim]

Yes, we're almost there :)