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

Add module_utils and filters for quoting and unquoting #53

Merged
merged 18 commits into from
Oct 11, 2021

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

These allow to split RouterOS commands (basically what the api module internally does), quote argument values ("this is a comment\nin multiple lines" | community.routeros.quote_argument_value == "\"this is a comment\\nin multiple lines\""), quote arguments ("name=with spaces" | community.routeros.quote_argument == "name=\"with spaces\""), or lists of arguments.

This also adds integration tests to CI for testing the filters.

What's missing is some documentation.

CC @heuels @NikolayDachev

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

quoting filters

@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #53 (d1ba3b2) into main (f9d246c) will increase coverage by 3.31%.
The diff coverage is 98.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
+ Coverage   80.06%   83.37%   +3.31%     
==========================================
  Files          11       14       +3     
  Lines        1304     1480     +176     
  Branches      181      198      +17     
==========================================
+ Hits         1044     1234     +190     
+ Misses        193      189       -4     
+ Partials       67       57      -10     
Impacted Files Coverage Δ
plugins/modules/command.py 93.75% <ø> (ø)
plugins/modules/facts.py 82.43% <ø> (ø)
tests/unit/plugins/modules/test_api.py 98.52% <ø> (-0.09%) ⬇️
plugins/modules/api.py 69.94% <80.00%> (-1.93%) ⬇️
plugins/filter/quoting.py 100.00% <100.00%> (ø)
plugins/module_utils/quoting.py 100.00% <100.00%> (ø)
tests/unit/plugins/module_utils/test_quoting.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9d246c...d1ba3b2. Read the comment docs.

@felixfontein felixfontein changed the title [WIP] Add module_utils and filters for quoting and unquoting Add module_utils and filters for quoting and unquoting Oct 10, 2021
@felixfontein
Copy link
Collaborator Author

I think this PR is ready now. While making the parsing stricter, I noticed that using the command split function for WHERE queries wasn't a good idea, since they don't really conform to RouterOS's command syntax. I've adjusted that part and made validation of the WHERE clause more strict.

The (minimal) docs I've added for the quoting/unquoting filters can be seen here: https://ansible.fontein.de/collections/community/routeros/docsite/quoting.html

ready_for_review

@felixfontein
Copy link
Collaborator Author

felixfontein commented Oct 10, 2021

(I've added an example to the api module in 00ae229 which uses one of the filters.)

@NikolayDachev
Copy link
Collaborator

NikolayDachev commented Oct 10, 2021

@felixfontein

I do a quick test with

  - name: add script noes testing
    vars:
      ros_backup_script_noes: |
        :local host value=[/system identity get name];
        :local date value=[/system clock get date];
        :local day [ :pick $date 4 6 ];
        :local month [ :pick $date 0 3 ];
        :local year [ :pick $date 7 11 ];
        :local name value=($host."-".$day."-".$month."-".$year);
        /system backup save name=$name;
        /export file=$name;
        /tool fetch address="192.168.1.1" user=ros password="PASSWORD" mode=ftp dst-path=("/mikrotik/rsc/".$name.".rsc") src-path=($name.".rsc") upload=yes;
        /tool fetch address="192.168.1.1" user=ros password="PASSWORD" mode=ftp dst-path=("/mikrotik/backup/".$name.".backup") src-path=($name.".backup") upload=yes;

    community.routeros.api:
      hostname: "{{ inventory_hostname }}"
      ssl: "false"
      password: "{{ vault_common_pwd }}"
      username: "admin"
      path: "system script"
      add:
        name="test_noes_script"
        source={{ ros_backup_script_noes | community.routeros.quote_argument_value }}
    register: scriptout
    delegate_to: localhost

Which look ok however this add some "artefacts" to the actual script

 2   name="test_noes_script" owner="admin" policy=ftp,reboot,read,write,policy,test,password,sniff,sensitive,romon dont-require-permissions=no run-count=0
     source=:local host value=[/system identity get name];n:local date value=[/system clock get date];n:local day [ :pick $date 4 6 ];n:local month [ :pick
       $date 0 3 ];n:local year [ :pick $date 7 11 ];n:local name value=($host."-".$day."-".$month."-".$year);n/system backup save name=$name;n/export
       file=$name;n/tool fetch address="192.168.1.1" user=ros password="PASSWORD" mode=ftp dst-path=("/mikrotik/rsc/".$name.".rsc") src-path=($name.".rsc")
       upload=yes;n/tool fetch address="192.168.1.1" user=ros password="PASSWORD" mode=ftp dst-path=("/mikrotik/backup/".$name.".backup") src-
       path=($name.".backup") upload=yes;n

After every end of line we get additional 'n' I guess must be '\n'
For example

        /system backup save name=$name;

is added as

system backup save name=$name;n

Also if we add spaces after end of the line this is more clear indicator for issue with parsing '\n'

        /system backup save name=$name;***
/system backup save name=$name;***n

Same is and for not so complex strings like comments

      ros_comment: |
         Test end of space parcing
....
      add:
        name="test_noes_script"
        source={{ ros_backup_script_noes | community.routeros.quote_argument_value }}
        comment={{ ros_comment | community.routeros.quote_argument_value }}
 2   ;;; Test end of space parcingn

Probably will be more easy for you to check that!

@felixfontein
Copy link
Collaborator Author

@NikolayDachev sorry, there was a stupid bug in the parsing code (which screwed up escape sequences). I've fixed it, added a lot of tests, and also improved the quoting code so that all control characters (ASCII < 32) are always escaped.

@NikolayDachev
Copy link
Collaborator

From what I test so far (add, update remove, query, cmd ) everything work perfectly fine .
Great job @felixfontein thank you !

@NikolayDachev NikolayDachev self-requested a review October 11, 2021 08:28
Copy link
Collaborator

@NikolayDachev NikolayDachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found, work as expected

@NikolayDachev
Copy link
Collaborator

I think we can merge if no other objections ?

@NikolayDachev NikolayDachev merged commit d73eb1c into ansible-collections:main Oct 11, 2021
@NikolayDachev
Copy link
Collaborator

Thanks !

@felixfontein felixfontein deleted the quote-unquote branch October 12, 2021 05:13
@felixfontein
Copy link
Collaborator Author

@NikolayDachev thanks for reviewing and testing!

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

Successfully merging this pull request may close these issues.

2 participants