debian_ip doesn't set up_cmds

[kiemlicz]

Description
The state:

   lo:
        ----------
        __env__:
            server
        __sls__:
            os.network
        network:
            |_
              ----------
              enabled:
                  True
            |_
              ----------
              type:
                  eth
            |_
              ----------
              proto:
                  loopback
            |_
              ----------
              up_cmds:
                  - ip addr add 1.2.3.4/32 dev $IFACE label $IFACE:0
            |_
              ----------
              down_cmds:
                  - ip addr del 1.2.3.4/32 dev $IFACE label $IFACE:0
            - managed
            |_
              ----------
              order:
                  10000

Worked in v3000 for Debian OS, now it doesn't

The options:

              up_cmds:
                  - ip addr add 1.2.3.4/32 dev $IFACE label $IFACE:0
            |_
              ----------
              down_cmds:
                  - ip addr del 1.2.3.4/32 dev $IFACE label $IFACE:0

are no longer respected in debian_eth.jinja

Setup
Aforementioned state

Steps to Reproduce the behavior
salt XYZ state.apply aforementionedstate

Expected behavior
up, down entries in /etc/network/interfaces for lo adapter

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

Salt Version:
           Salt: 3001
 
Dependency Versions:
           cffi: 1.14.0
       cherrypy: unknown
       dateutil: 2.7.3
      docker-py: Not Installed
          gitdb: 2.0.5
      gitpython: 2.1.11
         Jinja2: 2.11.2
        libgit2: 0.28.4
       M2Crypto: 0.35.2
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.20
       pycrypto: 3.9.7
   pycryptodome: 3.6.1
         pygit2: 1.0.3
         Python: 3.7.3 (default, Dec 20 2019, 18:57:59)
   python-gnupg: Not Installed
         PyYAML: 5.3.1
          PyZMQ: 17.1.2
          smmap: 2.0.5
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.1
 
System Versions:
           dist: debian 10 buster
         locale: UTF-8
        machine: x86_64
        release: 4.19.0-9-amd64
         system: Linux
        version: Debian GNU/Linux 10 buster

[krionbsd]

@kiemlicz thanks for the report, could you please show the error message you've got?

[kiemlicz]

no error message

IMO the template (debian_eth.jinja) is broken (it doesn't take up_cmds, down_cmds etc. -optmap - into account while iterating over options)

The mandatory pic, friend of mine keeps showing me:
https://pbs.twimg.com/media/B20YoFiIIAAHCYf.jpg

[OrangeDog]

Perhaps related to the fact that Ubuntu stopped using ifupdown #49078.
The assumption that everything Debian-based does networking the same way hasn't held for a while.
See also #57541 (I don't see any networking epic yet).

[kiemlicz]

https://github.com/saltstack/salt/blob/master/salt/templates/debian_ip/debian_eth.jinja#L26
optmap and valid_opts are not overlapping, only the valid_opts are taken into account, leaving behind all options like:

  'up_cmds': 'up',
  'down_cmds': 'down',
  'pre_up_cmds': 'pre-up',
  'post_up_cmds': 'post-up',
  'pre_down_cmds': 'pre-down',
  'post_down_cmds': 'post-down',

IMO optmap should be merged into valid_opts and that's it

[p12n]

Can confirm, this fixes it.

@@ -12,12 +12,13 @@
   } -%}
 {% set concat_opts = ['dns_nameservers'] -%}
 {% set valid_opts = [
-  'autoconf', 'privext', 'dhcp', 'hwaddress', 'vlan_raw_device', 'address', 'addresses', 'netmask',
+  'autoconf', 'privext', 'dhcp', 'hwaddress', 'hwaddr', 'vlan_raw_device', 'address', 'addresses', 'netmask',
   'metric', 'gateway', 'pointopoint', 'scope', 'hostname', 'media', 'leasehours', 'leasetime',
   'vendor', 'client', 'bootfile', 'server', 'mode', 'endpoint', 'dstaddr', 'local', 'ttl', 'mtu',
   'provider', 'unit', 'options', 'master', 'dns_nameservers', 'wireless_mode', 'wpa_ap_scan',
   'wpa_conf', 'wpa_driver', 'wpa_group', 'wpa_key_mgmt', 'wpa_pairwise', 'wpa_proto', 'wpa_psk',
-  'wpa_roam', 'wpa_ssid', 'accept_ra'
+  'wpa_roam', 'wpa_ssid', 'accept_ra',
+  'up_cmds', 'down_cmds', 'pre_up_cmds', 'pre_down_cmds', 'post_up_cmds', 'post_down_cmds',
   ] -%}
 {% if data.enabled %}auto {{ name }}
 {% endif %}{% if data.hotplug %}allow-hotplug {{ name }}

(Adding hwaddr is not needed for this, but it's like listing addresses from optmap as an alias for address.)