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

RFC: Allow python code in set-file and suite variables #3169

Closed

Conversation

TomekTrzeciak
Copy link
Contributor

Setting suite variables in Rose's [jinja2:suite.rc] allow Jinja2 (essentially Python) objects and expressions like list, dictionaries, ranges, etc, not just simple strings. This is a quick stab at allowing similar capability with Cylc --set and --set-file options. The intention is to gather opinions.

Note: this doesn't work with current serialisation of suite variables to suite.db, which I presume cannot handle generic Python objects, but that should be easily fixable (serialise set options and file content, rather than execed result).

@hjoliver
Copy link
Member

My opinion: this seems like a good capability to have.

@matthewrmshin
Copy link
Contributor

Just need to be careful to make sure we avoid opening up opportunities for code injection via the web UI.

@TomekTrzeciak
Copy link
Contributor Author

Just need to be careful to make sure we avoid opening up opportunities for code injection via the web UI.

This is in the same bucket as running shell commands to start the suite. As long as it's not possible to do that via web, it should be fine.

@matthewrmshin
Copy link
Contributor

Sure, but I'd imagine that users will want to start a suite via the web UI? In which case, not exposing this will make the suite a bit awkward to start up?

@TomekTrzeciak
Copy link
Contributor Author

Sure, but I'd imagine that users will want to start a suite via the web UI? In which case, not exposing this will make the suite a bit awkward to start up?

In that case I don't know. I guess to start the suite you would require the user to have enough permissions to access system command prompt, right? Note, that you already have to think about this in the context of current [jinja2:suite.rc]. Happy to discuss this further offline, as some security sensitive info might be involved.

@matthewrmshin
Copy link
Contributor

In the context of the web UI, running cylc run [some-fixed-options] SUITE is not the same as cylc run --set=key=[random-python] SUITE.

It is OK as long as we are careful - and perhaps we'll have to add an authorisation action to enable/disable this.

(On the other note, if we are going to support users editing their suite.rc (or whatever its name will become) via the web UI, then there will be a similar security implication any way.)

@matthewrmshin
Copy link
Contributor

See also cylc/cylc-ui#53.

@oliver-sanders
Copy link
Member

From current discussions, in Cylc8 we will likely have a new file for setting suite variables which will be a Python file (allowing variables to be directly imported) which would provide a much nicer solution for this.

We also have plans to change the way suite parameters are preserved, modified and presented. So support for this approach would be very short lived. Is there a pressing requirement for this feature in Cylc 7?

@TomekTrzeciak
Copy link
Contributor Author

From current discussions, in Cylc8 we will likely have a new file for setting suite variables which will be a Python file (allowing variables to be directly imported) which would provide a much nicer solution for this.

Happy with that. BTW, we already do something along this lines in IMPROVER (rose config -f log/rose-suite-run.conf jinja2:suite.rc >suitevars.py and then we can import or exec suitevars.py).

Is there a pressing requirement for this feature in Cylc 7?

No, I actually planned to repoint this PR at master, but in that case I'll just close it instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants