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

Make custom filters available in Mistral; Disable Decryption-by-default in st2kv function #30

Merged
merged 37 commits into from Jul 31, 2017

Conversation

Projects
None yet
2 participants
@Mierdin
Member

Mierdin commented Jul 7, 2017

It's been a long-held constraint that the custom Jinja filters provided by StackStorm are only available in ActionChain workflows, and unavailable to Mistral workflows.

This PR aims to change that by duplicating the filter implementations from the st2 repo in this repo. This makes the same filters available in Mistral workflows without adding a dependency on the st2common package.

NOTE that this is a duplication of the filters in the st2 repo, so every time a filter is added or changed there, a PR must be opened here to keep things consistent. This PR only makes the existing custom filters available, not future ones.

CHANGELOG for st2 was updated in StackStorm/st2@dd72546 since st2mistral doesn't really have one and that's probably where folks will look for the change anyways.

Closes #29

BREAKING CHANGE

The st2kv function here in Mistral previously decrypted keys by default. This PR adds a parameter to the underlying function that will force users to explicitly allow decryption, instead of doing it all the time. So, workflows that use st2kv to retrieve decrypted versions of ciphertext stored in datastore must now explicitly enable this decryption in the parameter, like so:

"{{ st2kv('system.secretkey', decrypt=True) }}"

Without this parameter, st2kv will not attempt decryption, and st2kv will return whatever value is stored at that key, whether or not it is ciphertext.

This change will make st2kv behavior more consistent with equivalent usage on the CLI/API and in ActionChains, where decryption is not performed by default.

Because st2kv leverages the API, which provides decryption on its own, the decrypt_kv filter was not included in the list of filters moved over to Mistral in this PR.

Added test function to test install workflow
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

@Mierdin Mierdin self-assigned this Jul 7, 2017

@Mierdin Mierdin changed the title from {WIP] Make custom filters available in Mistral to [WIP] Make custom filters available in Mistral Jul 7, 2017

Implmented redirect functions for all filters
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
from mistral import exceptions as exc
from st2common.jinja.filters import crypto

This comment has been minimized.

@m4dcoder

m4dcoder Jul 12, 2017

Contributor

I thought we agreed to duplicate the Jinja filters in st2mistral. I'm against including st2common here as a dependency. This will complicate the deployment story for Mistral.

This comment has been minimized.

@Mierdin

Mierdin Jul 12, 2017

Member

Sorry, I must have missed that one, or assumed that "duplicate" meant implementing these redirections. I am okay with re-implementing them fully here, it just adds work to anyone wanting to create a new custom filter.

Funny enough, I didn't catch the deployment complications you allude to because of the fact I am using one virtualenv for both st2 and mistral in StackStorm/st2#3557 (which you pointed out should be done separately). I assume this is what you're talking about, since in any real deployment, mistral would not have access to st2common.

I'll adjust this PR's description and change the approach. Thanks for the catch!

This comment has been minimized.

@m4dcoder

m4dcoder Jul 12, 2017

Contributor

Yep in real deployment, st2 and mistral have different venvs and mistral currently do not have access to st2common.

This comment has been minimized.

@m4dcoder

m4dcoder Jul 12, 2017

Contributor

It is important during development to be conscious about how changes will impact CI, packaging, and deployment.

This comment has been minimized.

@Mierdin

Mierdin Jul 12, 2017

Member

Ack - keeping that in mind, will remedy my development script for Mistral to reflect that.

Mierdin added some commits Jul 12, 2017

Copied filter code into st2mistral from st2 repo
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Removed old references
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Restored original import order
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Added filter unit tests
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Removed fat from test base
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Moved use_none to its own file, added test
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
PEP8 fixes
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Undo addition of oslo.log and oslo.config
This was a lazy temporary fix to a structural issue that
has since been fixed. So not needed anymore

Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Rum unit tests in circle
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

@Mierdin Mierdin changed the title from [WIP] Make custom filters available in Mistral to [WIP] Make custom filters available in Mistral; Disable Decryption-by-default in st2kv function Jul 13, 2017

Mierdin added some commits Jul 13, 2017

Removed decrypt_kv filter; added optional parameter to disable decryp…
…t by default in st2kv

Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Removed crypto unit test
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Removed crypto references from unit test base
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Alphabetized filter dict in test
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
@Mierdin

This comment has been minimized.

Member

Mierdin commented Jul 26, 2017

So it turns out the issue I’ve been having with YAQL while writing itests for these changes in StackStorm/st2#3565 was indeed caused by a mismatch in the function signature, and the way the YAQL evaluator was trying to pass data into it, but it had nothing to do with args vs kwargs, it was only with functions that had string parameters.

Summary

Strings as input to functions are recognized as <type 'unicode'> in YAQL, and all of the custom filters that have default string values (i.e. ‘’) are <type 'str'> (https://github.com/StackStorm/st2mistral/pull/30/files#diff-bda8d34192458bf7865ba4eb5d602996R30).
The YAQL engine checks that the input type, and the type it perceives is required from the function signature (https://github.com/openstack/yaql/blob/master/yaql/language/yaqltypes.py#L110) and if they are not the same, that result filters all the way back up several layers and eventually raises NoMatchingFunctionException (https://github.com/openstack/yaql/blob/master/yaql/language/runner.py#L70), which is what I was seeing.

From the look of it, the only integration tests that use any custom filters within a YAQL statement are testing st2kv, which doesn’t have any parameters that default to a string,.
My theory is that if anyone were to try to use any custom filters using YAQL (possibly even in ActionChains, haven’t tested that yet) that do have string-default parameters, they’d run into the same thing.
Of course, in Jinja this is not a problem because it doesn’t have the same explicit check.

Solution

The easiest way of addressing this is to add the unicode marker before each default string parameter in each custom filter function. Certainly, since this PR is not merged yet, I can take care of this. Jinja should continue to work, and this will make the equivalent YAQL work as well:

def regex_match(context, value, pattern=u'', ignorecase=False):
    if not isinstance(value, six.string_types):
        value = str(value)
    flags = _get_regex_flags(ignorecase)
    return bool(re.match(pattern, value, flags))

I already have a bit on my plate so I can’t do this now, but someone should try to use one of the custom filters that have one of these parameters, inside a YAQL statement in an ActionChain. My theory is that it won’t work, but I could be wrong. Just something to follow up on.

In addition, I couldn’t get both YAQL and Jinja to work with the additional context parameter (like in st2kv function). I dug into this issue in particular for an hour or two, and it seems like it should work because of the use of partial in mistral’s expression_utils (https://github.com/StackStorm/mistral/blob/master/mistral/utils/expression_utils.py#L92) but at runtime, context is always set to what value should be, and value to what pattern should be, so it doesn’t work. Seems like YAQL is more flexible wrt passing context (probably referring to it via keyword and suppressing exception when it doesn’t exist is my guess).

Anyways, I’m ignoring this for now so I can move on but it may be worth backtracking to figure this out because it doesn’t seem to make sense given the use of partial (even my single-script testing didn’t match up with the behavior I was seeing from mistral). For now, I'm going to submit some commits to this PR that add the unicode specifier to all string parameters for the functions here.

Mierdin added some commits Jul 27, 2017

Removed extra underscore in reference to use_none
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Added unicode specifiers for params with default values in regex filters
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Added to_complex unit test
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Renamed test base to be more generic
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Added infra for YAQL tests
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

Mierdin added some commits Jul 27, 2017

Added unit test for to_complex in yaql
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Condensing YAQL unit tests into a single file for now
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Added tests for data, json escape, and regex filters
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
@m4dcoder

LGTM overall. Please use the term function instead of filter in Mistral. Also, there are coding guidelines for Mistral if you can address.

setup.cfg Outdated
@@ -12,6 +12,26 @@ mistral.actions =
mistral.expression.functions =
st2kv = st2mistral.functions.stackstorm:st2kv_

to_json_string = st2mistral.filters.data:to_json_string

This comment has been minimized.

@m4dcoder

m4dcoder Jul 27, 2017

Contributor

Please use function instead of filter In Mistral. The term filter is specific to Jinja. In Mistral, these are called function. Please move all the implementation into the functions folder.

This comment has been minimized.

@Mierdin

This comment has been minimized.

@Mierdin

Mierdin Jul 29, 2017

Member

Oh, and in 5291379 (updated tests accordingly)

setup.cfg Outdated
version_bump_patch = st2mistral.filters.version:version_bump_patch
version_strip_patch = st2mistral.filters.version:version_strip_patch
json_escape = st2mistral.filters.json_escape:json_escape
use_none = st2mistral.filters.use_none:use_none

This comment has been minimized.

@m4dcoder

m4dcoder Jul 27, 2017

Contributor

Please list them by alphabetical order.

This comment has been minimized.

@Mierdin
def to_complex(value):
result = json.dumps(value)

return result

This comment has been minimized.

@m4dcoder

m4dcoder Jul 27, 2017

Contributor

How about just return json.dumps(value)?

This comment has been minimized.

@m4dcoder

m4dcoder Jul 27, 2017

Contributor

Why not move this to data.py?

This comment has been minimized.

@Mierdin

Mierdin Jul 29, 2017

Member

All good ideas - this was just some copypasta from the original implementation. Fixed in a164d6c

return {
'to_json_string': data.to_json_string,
'to_yaml_string': data.to_yaml_string,

This comment has been minimized.

@m4dcoder

m4dcoder Jul 27, 2017

Contributor

Why the blank lines?

This comment has been minimized.

@Mierdin

Mierdin Jul 29, 2017

Member

No reason, just copied from st2. Fixed in f49a9ff

env = self.get_jinja_environment()
obj = {'a': 'b', 'c': {'d': 'e', 'f': 1, 'g': True}}

template = '{{k1 | to_complex}}'

This comment has been minimized.

@m4dcoder

m4dcoder Jul 27, 2017

Contributor

Why use piping to pass k1 to to_complex? I mean we can test it if we want to make sure piping is supported. But how about also testing it as to_complex(k1)?

This comment has been minimized.

@Mierdin

Mierdin Jul 29, 2017

Member

As discussed in StackStorm/st2#3565, we're deciding (for now) to not support the filter format due to the upstream mistral issue, and requiring that users use the function format. I added context as the first positional arg (and modified testing accordingly) in 739fa88

tox.ini Outdated
@@ -37,6 +37,7 @@ commands = {posargs}
#Skip PEP257 violation.
[flake8]
ignore = D100,D101,D102,D103,D104,D105,D200,D203,D202,D204,D205,D208,D400,D401
max-line-length = 100

This comment has been minimized.

@m4dcoder

m4dcoder Jul 27, 2017

Contributor

Please leave default to max line length to 79 to match w/ Mistral. As I explained in Slack, this is an extension project for Mistral and I have been following the coding guidelines for Mistral. Sorry if this doesn't make any sense.

This comment has been minimized.

@Mierdin

Mierdin Jul 30, 2017

Member

No problem, I reverted this in 5dd1696 (and made sure everything passed tox -epep8 again). Following the rules of the upstream project this is a "part" of makes sense to me (though it wreaks havoc on my regex patterns)!

PoC for handling filter vs function usage in Jinja
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
@Mierdin

This comment has been minimized.

Member

Mierdin commented Jul 28, 2017

Style fixes for me to make:

  1. The length for each line is 79.
  2. Blank line is required after each conditional/logical blocks (i.e. for, if, while, etc.)
  3. Blank line is required before return statement.
  4. For parentheses, if the block involves multiple lines, the closing parentheses ) needs to be in a separate line.
  5. For functional arguments, if too long and require separate lines, put individual argument in a separate line.

@Mierdin Mierdin referenced this pull request Jul 29, 2017

Open

Allowing "pipe" notation in Jinja snippets #31

0 of 2 tasks complete

Mierdin added some commits Jul 29, 2017

Rollback previous commit (going to fix upstream instead)
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Moved functions out of 'filters' dir into 'functions'
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Alphabetizing some things
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
More filters -> functions renaming
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Fixed reference to get_functions
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Removed old filters dir
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
to_complex improvements
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Added context to functions; revamped testing accordingly
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Style-related corrections
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
@Mierdin

This comment has been minimized.

Member

Mierdin commented Jul 30, 2017

@m4dcoder So I made style corrections that Flake8 caught (after changing the max line length back to pep8 standard) but I had some questions about the other style rules you proposed (and I reposted above).

I know only of https://docs.openstack.org/hacking/latest/user/hacking.html in terms of OpenStack style standards, and I only found references to the last rule: "For functional arguments, if too long and require separate lines, put individual argument in a separate line." Is there another doc I'm missing?

Also what about automated checks? The mistral repo doesn't seem to use anything beyond what's already used in this repo, and it doesn't currently seem to be checking for any of that (except line length of course).

@m4dcoder

Please fix the config register opts. Otherwise, changes LGTM. @lakshmi-kannan asks me not to go easy on you so I'm going to enforce the coding style loosely (except the line length of 79). Some of the Mistral coding guidelines are additions to OpenStack so there's no doc around that. These guidelines you'll just have to get used to as you contribute more to Mistral.

from mistral import exceptions as exc
from st2mistral.utils import http

from st2mistral import config
config.register_opts()

This comment has been minimized.

@m4dcoder

m4dcoder Jul 31, 2017

Contributor

Why did you move this config section? There's a reason for the config section to be up top. It'll load config which will be used in the http module.

This comment has been minimized.

@Mierdin

Mierdin Jul 31, 2017

Member

This was to address the "module level import not at top of file" PEP8 violation, though I can see now that flake8 doesn't care for some reason (even though E402 is not being ignored).

Anyways, I can revert this, just part of the overall style fixes.

This comment has been minimized.

@Mierdin

Mierdin Jul 31, 2017

Member

Okay, fixed in c12ff85

Mierdin added some commits Jul 31, 2017

Reverted import order for config
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Finish YAQL unit tests
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Reorganized unit tests to follow the OpenStack layout
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

@Mierdin Mierdin changed the title from [WIP] Make custom filters available in Mistral; Disable Decryption-by-default in st2kv function to Make custom filters available in Mistral; Disable Decryption-by-default in st2kv function Jul 31, 2017

@Mierdin Mierdin merged commit 22e2469 into master Jul 31, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Mierdin Mierdin deleted the add-custom-filters branch Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment