interfaces_file: Regular expression characters not escaped

[jplitza]

SUMMARY

When replacing a value in /etc/network/interfaces that contains, for example, ||, the new line is garbage. This is caused by the old value being used directly in a regular expression here:

community.general/plugins/modules/system/interfaces_file.py

Line 287 in 4e56347

old_value_position = re.search(r"\s+".join(old_value.split()), old_line[prefix_start + optionLen:])

ISSUE TYPE

• Bug Report

COMPONENT NAME

interfaces_file

ANSIBLE VERSION

ansible 2.9.10                                                                                                                                                                                                                                
  config file = /home/jpl/ansible/ansible.cfg
  configured module search path = ['/home/jpl/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/jpl/.local/lib/python3.6/site-packages/ansible
  executable location = /home/jpl/.local/bin/ansible
  python version = 3.6.9 (default, Jul 17 2020, 12:50:27) [GCC 8.4.0]

CONFIGURATION

OS / ENVIRONMENT

Target Debian Buster, but probably irrelevant

STEPS TO REPRODUCE

- hosts: all
  gather_facts: no
  tasks:
    - interfaces_file:
        iface: dummy0
        option: description
        value: 'Funny || name'

    - interfaces_file:
        iface: dummy0
        option: description
        value: 'Another || funny || name'

EXPECTED RESULTS

/etc/network/interfaces:

iface dummy0 inet manual
    [...]
    description Another || funny || name

ACTUAL RESULTS

/etc/network/interfaces:

iface dummy0 inet manual
    [...]
    descriptionAnother || funny || name Funny || name

[ansibullbot]

Files identified in the description:

• plugins/modules/system/interfaces_file.py

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

[ansibullbot]

cc @MorrisA @bcoca @d-little @flynn1973 @gforster @hryamzik @kairoaraujo @marvin-sinister @mator @molekuul @obourdon @ramooncamacho @wtcross
click here for bot help

[jplitza]

I just noticed that interfaces_file expects "post-down" to actually be "down", so my example is treated as a value that should be replaced instead of added. If I use option: down, it works as intended.

However, the bug still exists in all other options. Will change the example.

[felixfontein]

Should be easy to fix (if the problem is in the line you say), by replacing that line with

old_value_position = re.search(r"\s+".join([re.escape(part) for part in old_value.split()]), old_line[prefix_start + optionLen:])

(with appropriate line breaks to avoid linting errors :) )

@jplitza do you want to create a PR?

[jplitza]

@felixfontein I have had terrible experiences with PRs for Ansible, having to re-create them several times in different projects because of restructuring, having to rebase them due to lack of response, and having to write changelogs without any guidance as to what to write where.

So with all due respect: No, I do not want to create a PR.

[mator]

i can't reproduce it on debian10 Debian GNU/Linux 10 (buster) with the development/git version of ansible:

$ ansible --version
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out
features under development. This is a rapidly changing source of code and can become unstable at any point.
ansible 2.11.0.dev0
  config file = /home/mator/ansible.dev/ansible.cfg
  configured module search path = ['/home/mator/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/mator/ansible.dev/ansible/lib/ansible
  ansible collection location = /home/mator/ansible.dev/collections
  executable location = /home/mator/ansible.dev/ansible/bin/ansible
  python version = 3.8.5 (default, Jul 27 2020, 08:42:51) [GCC 10.1.0]

just before playbook run:

root@debian10:/etc/network# tail -n 2 interfaces

iface dummy0 inet manual

running:

$ cat playbooks/debian-interfaces.yml
- hosts: debian10
  gather_facts: no
  tasks:
    - interfaces_file:
        iface: dummy0
        option: description
        value: 'Funny || name'

    - interfaces_file:
        iface: dummy0
        option: down
        value: 'Another || funny || name'

$ ansible-playbook -b playbooks/debian-interfaces.yml
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out
features under development. This is a rapidly changing source of code and can become unstable at any point.

PLAY [debian10] ****************************************************************************************************************************************************

TASK [interfaces_file] *********************************************************************************************************************************************
changed: [debian10]

TASK [interfaces_file] *********************************************************************************************************************************************
changed: [debian10]

PLAY RECAP *********************************************************************************************************************************************************
debian10                   : ok=2    changed=2    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

checking:

root@debian10:/etc/network# tail -n 3 interfaces
iface dummy0 inet manual
    description Funny || name
    down Another || funny || name

[jplitza]

@mator Sorry, I messed up the example. The second task should of course also modify "description" and not "down". Fixed.

[mator]

@jplitza
thanks, was able to reproduce...

@felixfontein ,
yep, that line

old_value_position = re.search(r"\s+".join([re.escape(part) for part in old_value.split()]), old_line[prefix_start + optionLen:])

fixes it for me... want me to submit PR ?

[felixfontein]

@felixfontein I have had terrible experiences with PRs for Ansible, having to re-create them several times in different projects because of restructuring, having to rebase them due to lack of response,

I guess you are refering to ansible/ansible#68362, #88, #89, ansible-collections/community.network#7 and ansible-collections/community.network#6. You unfortunately hit two different problems:

1. The large restructuring / move from a huge repo to a lot of smaller repos (where c.g and c.n are the two largest chunks) (TBH I was assuming that you were aware about the on-going restructuring since you pretty quickly created PRs in c.g long before it was announced to be ready);
2. The unfortunate truth that there are a lot of modules which have no maintainers, no active maintainers, or nobody who cares about community PRs. I don't know what's the exact case for edgeos, but it looks to me like that almost everything in community.network is pretty much dead and gets no reviews. (Your edgeos PR was not the first and not the last.)

I'm sorry that your experience was so terrible, especially for the edgeos PRs. I would have liked to review the PR's content, but I have no knowledge about edgeos at all, so I had no idea (and still not really have one) whether your PR is correct, needs changes, or is even plain wrong.

and having to write changelogs without any guidance as to what to write where.

You did ask and got suggestions and a link to an example.

So with all due respect: No, I do not want to create a PR.

That's too bad, because in this case, your change is a lot easier to judge without needing domain knowledge (i.e. I would dare to merge it), and there are active maintainers (@mator 👍). But if you don't want to, that's also ok. @mator if you want, feel free to create a PR.

[jplitza]

Personal experience

I guess you are refering to ansible/ansible#68362, #88, #89, ansible-collections/community.network#7 and ansible-collections/community.network#6.

Correct.

You unfortunately hit two different problems:

1. The large restructuring / move from a huge repo to a lot of smaller repos (where c.g and c.n are the two largest chunks) (TBH I was assuming that you were aware about the on-going restructuring since you pretty quickly created PRs in c.g long before it was announced to be ready);

I saw the PR ansible/ansible#65077 that partly fixed what my #89 was intended to fix being moved to and merged as ansible-collections/community.general#39. So I did the same, without having any clue about the split.

2. The unfortunate truth that there are a lot of modules which have no maintainers, no active maintainers, or nobody who cares about community PRs. I don't know what's the exact case for edgeos, but it looks to me like that almost everything in community.network is pretty much dead and gets no reviews. (Your edgeos PR was not the first and not the last.)

Good to know. Maybe I'll be able to help with that when I'm back at my job in a few weeks, as we use the modules for RouterOS and EdgeOS quite extensively.

I'm sorry that your experience was so terrible, especially for the edgeos PRs. I would have liked to review the PR's content, but I have no knowledge about edgeos at all, so I had no idea (and still not really have one) whether your PR is correct, needs changes, or is even plain wrong.

Not blaming you in particular. ;-)

and having to write changelogs without any guidance as to what to write where.

You did ask and got suggestions and a link to an example.

You are right, my memory didn't serve me well on this problem that really wasn't one. Sorry.

All in all I admire that you took the time to dig out those issues and try to explain what happened. As I said above, maybe I will have time in the future to get to know the development process and community behind Ansible and contribute "the right way", the whole experience just pushed me away from putting too much time up front in contributing.

So with all due respect: No, I do not want to create a PR.

That's too bad, because in this case, your change is a lot easier to judge without needing domain knowledge (i.e. I would dare to merge it), and there are active maintainers (@mator +1). But if you don't want to, that's also ok. @mator if you want, feel free to create a PR.

Well, in that case and since @mator doesn't seem to have found the time yet, I made #873 (which you already reviewed while I was still writing here, thank you!)

[mator]

stuck with work... and a test unit... probably going to extend it with a test unit later, sorry for the delay....

[jplitza]

stuck with work...

No problem. Aren't we all?

and a test unit... probably going to extend it with a test unit later, sorry for the delay....

Ironically, tests were one of the reasons I didn't really feel like opening a PR, because I don't have much experience with testing and am unfamiliar with the test system used by Ansible, yet had the vague feeling that some people/projects would like to have new tests with every bugfix that would have caught this particular bug.

[felixfontein]

We usually only ask for tests for modules/plugins (1) which are new, or (2) which already have tests. Since this isn't a new plugin and it currently has no tests, there's no need to start coming up with tests... (Of course a PR will not get not merged because it adds tests ;) )