file.replace fails to append if repl string partially available

[variia]

Hi,

I just managed to trigger a funny bug I need some input from you guys.

$ salt --versions
           Salt: 2014.7.5
         Python: 2.7.5 (default, Jun 17 2014, 18:11:42)
         Jinja2: 2.7.2
       M2Crypto: 0.21.1
 msgpack-python: 0.4.6
   msgpack-pure: Not Installed
       pycrypto: 2.6.1
        libnacl: Not Installed
         PyYAML: 3.10
          ioflo: Not Installed
          PyZMQ: 14.3.1
           RAET: Not Installed
            ZMQ: 3.2.5
           Mako: Not Installed

Target file:

$ cat /etc/sysconfig/network-scripts/ifcfg-eth1
# Generated by parse-kickstart
UUID=c8f0fafb-ea51-40f4-83fa-a4a7e709c5eb
GATEWAY=
IPV6_AUTOCONF=yes
BOOTPROTO=none
DEVICE=eth1
ONBOOT=yes
IPV6INIT=yes
TYPE=Ethernet
IPADDR=10.2.6.3
PREFIX=24
DEFROUTE=yes
IPV4_FAILURE_FATAL=no
IPV6_DEFROUTE=yes
IPV6_PEERDNS=no
IPV6_PEERROUTES=yes
IPV6_FAILURE_FATAL=no
NAME="System eth1"

The issue is that I want to manage my own /etc/resolv.conf file with salt hence I have no DNS specification in my interface config. Redhat manages /etc/resolv.conf by default with PEERDNS=yes resulting an empty resolv.conf file after reboot so I have to append PEERDNS=no to my interface config for BOTH IPV6 and IPV4 to avoid this. As you can see, the installer only added IPV6_PEERDNS so my search string is partially available in the file content.

$ vim test.sls
test:
  file.replace:
    - name: /etc/sysconfig/network-scripts/ifcfg-eth1
    - pattern: funnybug
    - repl: PEERDNS=no
    - append_if_not_found: True

$ salt -v test.centos state.sls test
Executing job with jid 20150502115657247951
-------------------------------------------

test.centos:
----------
          ID: test
    Function: file.replace
        Name: /etc/sysconfig/network-scripts/ifcfg-eth1
      Result: True
     Comment: No changes needed to be made
     Started: 11:57:00.147777
    Duration: 2.554 ms
     Changes:

Summary
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1

funnybug is not part of the target file and the reason why the repl string is not appended is caused by the logic which is trying to check if it was already appended:
https://github.com/saltstack/salt/blob/2014.7/salt/modules/file.py#L1125
https://github.com/saltstack/salt/blob/2014.7/salt/modules/file.py#L1155
Even if I specified ^string to specifically indicate line start, it is not taken into account.

I would not know right now how I would tackle this sort of issue so if you guys think it is not something worth to fix, I just fall back to cmd.run solution.

Thanks,
Ivan

[lorengordon]

That logic at L1155 shouldn't actually result in a match, so found shouldn't be set to True. Maybe the '=' is confusing things? Have you tried wrapping the repl value in single quotes?

test:
  file.replace:
    - name: /etc/sysconfig/network-scripts/ifcfg-eth1
    - pattern: funnybug
    - repl: 'PEERDNS=no'
    - append_if_not_found: True

[variia]

I certainly have tried everything under the sun, quoting, you name it, no luck.
I tried the logic in python shell and in my opinion, it does what it is coded for.
The only way it would work for me is if the L1155 searched for ^ + content.

>>> import re
>>> line = 'IPV6_PEERDNS=no'
>>> content = 'PEERDNS=no'
>>> re.search(content, line)
<_sre.SRE_Match object at 0x107534098>
>>> content = '^PEERDNS=no'
>>> re.search(content, line)
>>>

[lorengordon]

Oh I see, I misunderstood. Yes, that's behaving exactly as intended. 2015.2 introduces file.line, which may do what you need, as its intended to work on entire lines, where file.replace needs to be able to work on partial lines as well.

That said, considering L1155 is determining whether to append/prepend the content as a new line, it may make sense to assume the search is for the entire line...

[variia]

Agreed. If prepend or append flag set, '^' should be added to the search content. I don't mind using file.line in the next release but this functionality either needs to be sorted or removed in favour of file.line, otherwise it just remains confusing.

[lorengordon]

I went ahead and created a branch with a possible fix, but I can't test it effectively while at work (though I did make sure the code worked in a python shell). If you can test the file.py module in my branch to make sure it works the way you expect, I'll open the PR. Otherwise, I'll test it later when I get home.

https://github.com/lorengordon/salt/tree/file.replace_assume_line

[jfindlay]

@variia, @lorengordon, thanks for working on this.

[variia]

@lorengordon sure, can test it in couple of hours. Thanks

[lorengordon]

Ok, the patch appears to work as intended. PR is #23350.

[variia]

Thanks a lot for the prompt action @lorengordon, I can also confirm that the fix is working on my production cluster.