firewalld state: firewalld.prepare calls new_service, not add_service

[LukeCarrier]

Given the following state:

public:
  firewalld.present:
    - services:
      - salt-master
      - ssh

Salt will incorrectly execute the following firewall-cmd command when adding the salt-master service to the public zone:

$ /bin/firewall-cmd --new-service=salt-master --permanent

I believe it should be executing the following:

$ /bin/firewall-cmd --add-service=salt-master --permanent

See the output of salt-call -l debug below:

[DEBUG   ] LazyLoaded firewalld.present
[INFO    ] Running state [public] at time 14:59:01.064056
[INFO    ] Executing state firewalld.present for public
[INFO    ] Executing command '/bin/firewall-cmd --get-zones' in directory '/root'
[DEBUG   ] output: block dmz drop external home internal public trusted work
[INFO    ] Executing command '/bin/firewall-cmd --zone=public --list-services' in directory '/root'
[DEBUG   ] output: dhcpv6-client ssh
[INFO    ] Executing command '/bin/firewall-cmd --new-service=salt-master --permanent' in directory '/root'
[ERROR   ] Command '/bin/firewall-cmd --new-service=salt-master --permanent' failed with return code: 26
[ERROR   ] output: Error: NAME_CONFLICT: salt-master
[INFO    ] {'services': ['`salt-master` has been successfully added'], 'icmp_blocks': [], 'ports': [], 'port_fwd': []}
[INFO    ] Completed state [public] at time 14:59:01.521599

[jfindlay]

@LukeCarrier, thanks for the report. What is the output of salt-call --versions-report?

[LukeCarrier]

@jfindlay thanks for the prompt triage. Output as follows:

[vagrant@salt ~]$ salt-call --versions-report
Salt Version:
           Salt: 2015.8.0

Dependency Versions:
         Jinja2: 2.7.3
       M2Crypto: Not Installed
           Mako: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.7.0
         Python: 2.7.5 (default, Jun 24 2015, 00:41:19)
           RAET: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
        libnacl: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
   python-gnupg: Not Installed
          smmap: Not Installed
        timelib: Not Installed

System Versions:
           dist: centos 7.1.1503 Core
        machine: x86_64
        release: 3.10.0-229.11.1.el7.x86_64
         system: CentOS Linux 7.1.1503 Core

[rallytime]

Hi @LukeCarrier - What is the difference between calling firewalld.add_service and firewalld.new_service in the state? I am just trying to gauge expected behavior. Is it because you've specified public as the state id? And how will that affect the ssh service you've also listed?

I just rewrote a bunch of the internals of the firewalld state to make it more stateful in PR #28181, but I am not as familiar with the internals of firewalld, so an explanation of the expected behavior here would be appreciated. (The rewrite maintains the previous behavior of calling firewalld.new_service, so it doesn't address your report here.)

[LukeCarrier]

Hi @rallytime, thanks for the response. I hope this helps.

Looking at the arguments passed to firewall-cmd, my understanding is that firewalld.add_service adds a service to the specified zone with --add-service, and firewalld.new_service is used to load a new XML definition of a service into the firewalld daemon with --new-service. The firewall-cmd documentation doesn't clearly explain the latter switch, so the "Defining a custom service" section of this guide is the best explanation I could find.

Since we're specifying the name of a specific zone, my understanding is that the defined services should be added to the zone. Therefore, the expected behavour of the following state:

public:              # the name of the zone
  firewalld.present:
    - services:
      - salt-master  # the names of the already defined services
      - ssh

Would be that the following firewall-cmd commands are executed:

$ firewall-cmd --zone=public --add-service=salt-master --permanent
$ firewall-cmd --zone=public --add-service=ssh         --permanent
$ firewall-cmd --reload

I'd then expect the following to be true:

$ firewall-cmd --zone=public --list-services
salt-master ssh

I'm not certain why you'd need to use --new-service, as --reload appears to reload the XML service definitions anyway. Either way, I think this should be the responsibility of the user, not the Salt state module.

Thank you for investing your time into this state; the changes in PR #28181 look great. Clearly explaining the applied changes (or lack thereof) is a really nice touch. :+1:

[rallytime]

@LukeCarrier Thank you for the explanation! I think that sounds like a reasonable change and makes sense. I've fixed this in #28308. Does that look good to you?

[LukeCarrier]

@rallytime Change looks great -- thank you!

I think it would be worth a separate discussion about whether changes should be applied permanently (permanent=True option to add_service), but this behaviour is consistent with the ports option. Should I open another issue?

[rallytime]

@LukeCarrier Yeah, that is probably worth opening a new issue. That will likely entail adding a new option to set a permanent_service flag or something like that.