update community.routeros.api query functionality #63
Conversation
Codecov Report
@@ Coverage Diff @@
## main #63 +/- ##
==========================================
+ Coverage 82.31% 82.58% +0.26%
==========================================
Files 21 21
Lines 1866 1981 +115
Branches 296 321 +25
==========================================
+ Hits 1536 1636 +100
- Misses 256 264 +8
- Partials 74 81 +7
Continue to review full report at Codecov.
|
NikolayDachev
requested review from
felixfontein,
Andersson007,
gundalow and
heuels
There was a problem hiding this comment.
Thanks for your contribution! This should have a changelog fragment.
plugins/modules/api.py
Outdated
| where = self.query[where_index + len(' WHERE '):] | ||
| # create a list of WHERE arguments | ||
| for wl in where.split(','): |
There was a problem hiding this comment.
Hmm, this is a bit complicated. A comma could show up in a value. For example if WHERE foo == "bar, baz", it would split this into two elements foo == "bar and baz, and this will result in an error.
There was a problem hiding this comment.
agree probably ',' can be changed to 'AND' ?
There was a problem hiding this comment.
It will have the same problem: WHERE foo == "bar AND baz" :) I'm afraid we have to properly parse this, instead of simply doing a split with simple string operations.
There was a problem hiding this comment.
Maybe it would be better to allow to pass the where clauses in as structural data, so we don't have to parse it? Something like:
where:
- what: foo
op: '=='
value: bar AND baz
- what: something else
op: '<'
value: 4
There was a problem hiding this comment.
Is not bad idea however we should structure it simple as possible
may be something like
query:
items:
- .id
- address
where:
address:
eq:
- "1.1.1.1"
- "2.2.2.2"
network:
not:
- "255.255.255.0"
Or:
address:
eq:
- "3.3.3.3"
- "4.4.4.4"
It will be more easy to be coded, however Or: "is in" where() and not sure how will be more proper to be as yml input (dict structure)
To be key of "where:" or key of "query:"
https://librouteros.readthedocs.io/en/latest/query.html#advanced-usage
also "In" must be considered
eq: ==
not: !=
less: <
more: >
In: vlaue1 value2 in key
or whatever are the proper words for < and > :)
Also I'm not sure if case with same key multiple values is valid query at all :|
If I not mistake librouteros.query is https://wiki.mikrotik.com/wiki/Manual:API#Queries
There was a problem hiding this comment.
query:
items:
- .id
- network
- address
where:
- network:
is: "192.168.58.0"
- address:
not: "192.168.58.254/24"
or:
- address:
not: "192.168.58.1/24"
? I think looks good (not sure if multiple where: and or: are good combination but will test it ) will fix the code
There was a problem hiding this comment.
I change the code with the new structure (not completed), however:
If we have same conditions keys with different values we should us librouteros.where Or / In
For example
items:
- .id
- network
- address
where:
- network:
is: "192.168.58.0"
- address:
is: "192.168.58.254/24"
- address:
is: "192.168.58.1/24"
This will not return result since (a == 1 and a == 2 ) 'which a ?' in this case we should use or/in (a == 1 or a == 2 ) - first match win
For reference
items:
- .id
- network
- address
where:
- network:
is: "192.168.58.0"
- address:
not: "192.168.58.254/24"
- address:
not: "192.168.58.1/24"
This will return all other ip addresses which are NOT , for example (1,2,3,4 in a) (a != 1, a !=3) will return (2,4).
Also if (a == 1, a != 3) will return only (1) etc.
What I'm not sure (specially for OR).
If we have (1,2,3,4) (a > 2 or a > 3), this must return (3,4) if OR is valid not only for '=='
This is important since we can change yml to
items:
- .id
- network
- address
where:
- network:
is: "192.168.58.0"
- address:
is:
- "192.168.58.254/24"
- "192.168.58.1/24"
if type('is') is list:
... use or in query
However if we should mix for example '<, >, ==, !=' the yml must be something like (ignore ip values this is a structure example)
items:
- .id
- network
- address
where:
- network:
is: "192.168.58.0"
- address:
or:
more: "192.168.58.254/24"
less: "192.168.58.1/24"
is: "192.168.58.2/24"
There was a problem hiding this comment.
IMO the new structure should be something that can be validated with the argument spec. (And I would really avoid putting data into dictionary keys if possible, so instead of <operation>: <value> it should be something like operation: <operation> / value: <value>. That makes it easier to process and easier to automatically generate IMO.)
How about limiting it to a or/and structure, so you can have a list of "or" conditions, each containing a list of "and" conditions?
plugins/modules/api.py
Outdated
| @@ -303,7 +303,7 @@ def __init__(self): | |||
| remove=dict(type='str'), | |||
| update=dict(type='str'), | |||
| cmd=dict(type='str'), | |||
| query=dict(type='str'), | |||
| query=dict(type='dict'), | |||
There was a problem hiding this comment.
This is a backwards-compatibility breaking change. The old form needs to continue to work as well.
How about adding a new parameter for this, making the two mutually exclusive, and have some code that transforms the old query content to the new parameter?
There was a problem hiding this comment.
I think how to solve the same, something like "extended_query" ? This will need some additional changes, will try to add the new code without change the existing one.
I'm still working on the code to add Or and In in the query, will push it when is ready for final review
If everything look ok will fix the change log, docs, unites etc.
Please give me some time to add the final code for review and we can continue the discussion
There was a problem hiding this comment.
As yml structure, code will find 'or:' and build the query , will be same for where(key.In)
Also I'm adding additional checks if the whole dict is structure correctly
items:
- .id
- network
- address
where:
- network:
is: "192.168.58.0"
- address:
not: "192.168.58.254/24"
- address:
or:
- is: "192.168.58.1/24"
- not: "192.168.58.2/24"
Bur probably will try to change it, but not sure how good look :|
'is': '=='
'not': '!='
'more': '>'
'less': '<'
'or': 'or'
'in': 'in'
items:
- .id
- network
- address
where:
- network:
'==': "192.168.58.0"
- address:
'==': "192.168.58.254/24"
- address:
or:
- '==': "192.168.58.1/24"
- '!=': "192.168.58.2/24"
There was a problem hiding this comment.
actually will do both :)
- network:
'==': "192.168.58.0"
- network:
is: "192.168.58.0"
There was a problem hiding this comment.
push new code which have query and extended_query (code is still in progress) just for info!
There was a problem hiding this comment.
build_api_extended_query() is very bulky ..but work fine, I will try to fix it when I finish the main functionalities with 'Or/In' queries
There was a problem hiding this comment.
extended_query code is ready for review !
extended_query structure is:
extended_query:
items:
- ros_attribute1
- ros_attribute2
- ros_attribute3
where:
- ros_attribute1:
operator: "value"
- ros_attribute2:
or:
- operator1: "value1"
- operator2: "value2"
- ros_attribute3:
in:
- "value1"
- "value2"
items: type(list) order is not important
'extended_query':can work only with 'items' (same as normal 'query') -> 'where' is not mandatory
where: type(list) order is important for routeros api query result
Any 'where' ros_attribute must be in 'items' !
where sub items:
ros_attribute: type(dict) single operator(key) / vlaue
operator:
'==' or 'is'
'!=' or 'not'
'>' or 'more'
'<' or 'less'
'or': type(list) (value is same as ros_attribute except 'or', 'in') order is important - depend on all other query attribute, first match win
'in': type(list) (value is a list) order is not important if any of ros_attribute is in 'in list': true
Example (ignore example values ):
extended_query:
items:
- network
- address
where:
- network:
is: "192.168.58.0"
- address:
less: "192.168.58.1/24"
- address:
or:
- more: "192.168.58.1/24"
- '==': "192.168.58.2/24"
- address:
in:
- "10.20.36.253/24"
- "192.168.58.2/24"
plugins/modules/api.py
Outdated
| where = self.query[where_index + len(' WHERE '):] | ||
| # create a list of WHERE arguments | ||
| for wl in where.split(','): |
There was a problem hiding this comment.
IMO the new structure should be something that can be validated with the argument spec. (And I would really avoid putting data into dictionary keys if possible, so instead of <operation>: <value> it should be something like operation: <operation> / value: <value>. That makes it easier to process and easier to automatically generate IMO.)
How about limiting it to a or/and structure, so you can have a list of "or" conditions, each containing a list of "and" conditions?
|
extended_query code is ready for review ! @felixfontein I prefer to have acceptable review for the code and extended_query yml structure, after that I will start to work on docs and unit test For 'where:in'
TODO:
extended_query structure is: items: type(list) order is not important 'extended_query':can work only with 'items' (same as normal 'query') -> 'where' is not mandatory where: type(list) order is important for routeros api query result Any 'where' ros_attribute must be in 'items' ! where sub items: operator: 'or': type(list) (value is same as ros_attribute except 'or', 'in') order is important - depend on all other query attribute, first match win Example (ignore example values ): |
|
How about changing:
to something like: That way you can template everything (you cannot template dictionary keys in Ansible!) and Ansible can validate the structure for you. One thing about This structure (you have a global I've also been thinking about how to do this in a more flexible way. I haven't really found a good solution yet, at least none that can get completely validated by ansible :-) How about this: A generic expression dictionary can take one of the following forms:
Then you can freely nest every possible condition together without any restrictions. What do you think? |
|
Yep I agree we allow user to make conjunctive input, from what I test routeros return 'no results' in such cases, my point is .. should we limit it in some way or we should leave it to user decision ?! I'm not sure how yml structure can be done properly as " completely validated by ansible" since is "unknown" input however if we can do that .. will be the best approach for sure, we are not in rush, so we can try to find a solution ! I do not insist for the structure which I suggest in any way! (actually 'is' can be reanmed to 'eq', no idea why I type it "is' :) ) What we know for sure
OR - I belive I make a mistake with it! Right now is "single attribute" Or(attribute1 = 3, attribute1 > 4, et. ) Something like (please focus on structure example not on values also not in spefici order of "wehre" items :) ): Aka structure in this example is tricky ! or if we use "attribute","operator", "value" key/vlaue (whcih will be the easy way to be processed no 2 opinions on that) whatever we decide, this 'or' somehow not fit !!!! From every list element we expect the same dic, but not for 'or' or :D but in this case 'or' will be treated as routeros attribute which I'm not sure if is a good idea or :D we can use 'attribute':'OR' (to mark it is as special attribute .. similar to what we do with api.py old 'query' and 'WHERE') . btw: https://librouteros.readthedocs.io/en/3.2.0/query.html note: sorry on some places I type 'eq' in other 'is' -- this is the same no idea why I continue to do that :) note: the input structure for libouteros.query I think look ok, however this "UGLINESS" which I create with those nested loops for yml input must be rewrite .. how is depend on overall yml input |
|
I think this will be a good yml structure (also for code)
3.example
will be readed as:
We should not limit users since we cannot cover all condition combination, avoidin "conjunctive expression" must depend on user input (routeros will return "no result")
However this look like is not a valid YML syntax Also But from code prospective 'attribute_or' is same as 'or' - key name, so something like Other suggestion can be:
|
For some use-cases it will make sense, for some not. It's really hard to say. If we restrict it, sooner or later someone will complain ;-)
"Completely validated by ansible" will only work for some restricted form.
The main point for the operator being a dict value (that can be templated) instead of a dict key (that cannot be templated) is that it allows templating. Like if you want to search for a value that has a certain property if BTW, how about renaming
Yes, that's true. I don't think there is a way around it. (You can still let Ansible validate it though, by using
No problem, I understand what you mean ;)
The cool thing about this is that if you let something reformat the YAML and sort every dictionary by keys, exactly this order is kept :) How about the following, which is a slightly modified version of your schema: extended_query:
attributes:
- "ros_attribute1"
- "ros_attribute2"
- "ros_attribute3"
where_all: # <-- 'where_all' instead of 'and' or the original 'where'
- attribute: "ros_attribute1"
is: "=="
value: "value"
- attribute: "ros_attribute2"
is: "in"
value:
- "value1"
- "value2"
- attribute: "ros_attribute3"
is: "=="
value: "value"
- where_any: # <-- 'where_any' instead of 'or'
- attribute: "ros_attribute2"
is: ">"
value: "value"
- attribute: "ros_attribute1"
is: "!="
value: "value"
- where_all: # a nested 'and' :)
- attribute: "ros_attribute1"
is: "=="
value: "foo"
- attribute: "ros_attribute2"
is: "!="
value: "bar"
- attribute: "ros_attribute3"
is: "!="
value: "value"
- attribute: "ros_attribute2"
is: "in"
values: # <-- I changed 'value' to 'values' here
- "value1"
- "value2"(If we want to do negation at some point (assuming RouterOS and librouteros support it), we can call it
No, but it is not that hard to implement something that rigorously checks this and converts it to a librouteros expression. I can help with that if you don't want to implement that (I've written way too many of such things in my life, so another one won't hurt :) ). |
where_any - Or() is in same group of "operators" (or, and, in) for where() whrer_all( a=1, where_any( a=b, a.IN(1,2,3) ) , a.In(1,2,3), .... ) I do not insist Or() key to be 'or' .. just I believe will be more practical where() and where(Or()) to be with not similar key names. a.In() - 'value' vs. 'values' - I don't think is a problem, except if we want to check user input (not a big deal but ) if all 3 keys are consistent we will be more easy to do user input check, if we have 'values' instance 'value' for Is then we should do additional check
Also can be the opposite, someone else to do the code I can review it, is not a problem .. just we should avoid double work :) Regards, note: https://github.com/luqasz/librouteros/blob/master/librouteros/query.py |
|
Since no objections for a week, I will start to work on the new code this days |
|
Latest code with new yml structure for extended_query is ready for review
attributes
Under where
where - attributes: { attribute: '', is: '', value: '' }
Ansible builtin check for 'attributes' and 'where' check_extended_query() - validate extended_query:attributes, extended_query:where and extended_query:where:or input api_extended_query() - main extended_query function Other api_query() - extend support for str operators (docs are updated) - C(==) or C(eq), C(!=) or C(not), C(>) or C(more), C(<) or C(less). After code review TODO
|
|
(Just wanted to say that I'm not actively ignoring you and this PR, I'm just too busy at the moment to be able to spend any cycles for this :-( ) |
|
@felixfontein Personally I don't have a free time the last several weeks on top of that I broke a leg. My point is .. when we have a time then we will continue on this PR. side note: I add just several peoples from the community for review (I just was not sure if is convenient to add all), however if any one can help .. feel free to do that. Testing, or code review whatever I still no work on docs and unity test .. no point to do that before final confirmation (aka yml structure change or any code related etc. ) Regards to all :) |
|
Any update here, I think to start working on docs/uniti if there is no majors objections for this pr |
|
@NikolayDachev I'll try to take a look at this PR during this weekend. |
|
Let me know if you need additional info ... |
plugins/modules/api.py
Outdated
| if len(self.result['message']) < 1: | ||
| msg = "no results for '%s 'query' %s" % (' '.join(self.path), | ||
| self.module.params['extended_query']) | ||
| self.result['message'].append(msg) |
There was a problem hiding this comment.
While this is what api_query() also does, I would vote to not add this if the result set is empty. That way the result is a lot easier to process, since you can better distinguish between "there was no answer" and "there was one answer".
There was a problem hiding this comment.
I removed this in 579ffcb.
|
I think this looks great! I've added some comments on how to improve some details, but I really like the general way it works! (Sorry if I screwed up some of the comments, I'm writing this with a very bad internet connection, and the GitHub UI is sooooo sluggish...) |
|
Let me know if you need additional info ... |
|
I will need a some time to check all suggestions , the other option is to commit them all and fix the rest of the code |
|
@NikolayDachev if you want I can also create a commit out of my suggestions (and the other changes needed due to them) and push them to your branch. |
|
Hi, I have zero time to work on this change the last couple of week's :( , I guess after this week I will start to work on. |
|
@NikolayDachev I've added some commits and tested the result. What do you think? |
|
I see we have 3 new pull request which are more or less related to api module, let's commit them first and merge here before we continue |
|
@NikolayDachev sounds good to me. They need reviews as well though :) I'm currently working on some more new modules for working with the API btw. |
felixfontein
force-pushed
the
extend-query
branch
from
9ea160d to
0f525d3
Compare
|
Latest changes look ok, I want to check also several other thinks, TODO:
note: sorry I was in not healthy condition this days |
|
Ouuu I miss the tests !! |
I hope you're doing better now!
Haha, no problem ;) There aren't that many yet, feel free to add more :) |
|
Extended query docs are updated (very small update) @felixfontein let me know what you thinks for examples, if is ok I believe we can push this PR ! |
|
Actually I can try to clear unit test warnings, however I have some dilemma for example codecov I guess warning is for "Or" but any idea ? |
plugins/modules/api.py
Outdated
| ip1: "1.1.1.1/32" | ||
| ip2: "2.2.2.2/32" | ||
| ip3: "3.3.3.3/32" | ||
|
|
||
| tasks: |
There was a problem hiding this comment.
Why not remove this one as well, and remove the four leading spaces from all tasks?
plugins/modules/api.py
Outdated
| - name: Dump "{{ path }} print" output | ||
| ansible.builtin.debug: | ||
| msg: '{{ print_path }}' | ||
| path: "ip address" |
There was a problem hiding this comment.
Without register and potential debug task this is not really useful. Most users don't run their playbooks with enough verbosity to see that something happened.
plugins/modules/api.py
Outdated
| is: "in" | ||
| value: | ||
| - "10.20.36.0" | ||
| - "192.168.255.0" |
plugins/modules/api.py
Outdated
| update: >- | ||
| .id=*14 | ||
| address=192.168.255.20/24 | ||
| comment={{ 'Update 192.168.255.10/24 with .id=*14 to 192.168.255.20/24 on ether2' | community.routeros.quote_argument_value }} |
There was a problem hiding this comment.
Hardcoding temporary IDs in comments is not a good idea :)
| comment={{ 'Update 192.168.255.10/24 with .id=*14 to 192.168.255.20/24 on ether2' | community.routeros.quote_argument_value }} | |
| comment={{ 'Update 192.168.255.10/24 to 192.168.255.20/24 on ether2' | community.routeros.quote_argument_value }} |
You can ignore that one, (Actually this line isn't running since |
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
|
Merged, @felixfontein bazillion number of thanks for the help with this PR |
|
@NikolayDachev thanks a lot for working on this one, and sorry again that reviewing it took so long! I'm really glad we managed to complete it, and I'm really happy about its final form :) |
|
No problems, main issue was my health conditions which was in series, nothing covid related but still .. |
SUMMARY
Right now community.routeros.api query - WHERE is limited to single criteria.
We should update it to use all librouteos functionality. Also this will help with routeros/ansible idempotent functionality.
For understandable reasons RouterOS allow to add multiple configs with same value (for some configurations), in result when we add something with API we are not able to do correct match if the required config already exist or not which will make duplicated config entries!.
For example 'add' task can be executed only if 'query' do not return .id for existing config which match multiple criteria. etc.
ISSUE TYPE
COMPONENT NAME
community.routeros.api
ADDITIONAL INFORMATION
libouteros: https://github.com/luqasz/librouteros
librouteros docs: https://librouteros.readthedocs.io/en/latest/query.html#
What we missing is AND, OR for query-where
Test playbook
RouterOS (CHR 7.2rc1) config
Testing before change:
Testing with current change (not completed read more!):
READ MORE:
I try to not breake additional functionality around api_query() and def init() (if self.query:), however in order to achive this I move self.wehre to self.where_list.
As general idea is to take ansible argument, pars evrething after 'WHERE' and make it as proper list format. After that those criteria are processed as list of lists (check api_query())
What I test so far look ok. This code add only AND functionality (aka - a == b and c == d and y == z .. etc.), however I want to add librouteros OR as well, before I add OR I want to have a review to the current changes since OR code will be added around current change.
note: several small fixes are intruduced as well more related to return messages via ansible
note: '.id network address WHERE network == 192.168.58.0, address != 192.168.58.254/24' -> ',' is used to split criteria
note: 'ip address' is used only for simplicity ! (routeros will not allow ducplicated ip addresses). Example for allowed duplicated configs can be firewall rules, ipsec policy, capsman provisioning .. etc.
Please check the code changes, I will appreciate any idea / improvement / bug fix in the current code .. etc.
If something is not clear/well understandable in the code, let me know.
TODO: