Repository: saltstack/salt
Pull request: [master] Fix ipset new_set bug
### What does this PR do?
Fixes a bug in ipset introduced in [0e46c149](https://github.com/saltstack/salt/commit/0e46c14907f1c63c9bd7206ed3a7dd59e59e0800#diff-f0cc809208d5a7ae3c8d9190536575ef7a8f5141d6785056ba2703991f971346L332-R332)

### What issues does this PR fix or reference?
Fixes: #61620, #62255, #63224

### Previous Behavior
Creating an ipset would fail if the set type used the `family` option.

### New Behavior
The ipset is created as expected.

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltproject.io/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [ ] Docs
- [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
No

Please review [Salt's Contributing Guide](https://docs.saltproject.io/en/master/topics/development/contributing.html) for best practices.

See GitHub's [page on GPG signing](https://help.github.com/articles/signing-commits-using-gpg/) for more information about signing commits with GPG.


Problem statement:
[BUG] modules/ipset.py new_set Syntax error
**Description**
Running
`salt-call -l debug ipset.new_set name=whitelist set_type="hash:net"  comment=True counters=True`
Gives error:
`[ERROR   ] salt.loaded.int.module.cmdmod:1305 output: ipset v7.5: Syntax error: unknown inet family ['/usr/sbin/ipset', 'create', 'whitelist', 'hash:net', 'counters', 'family', [...], 'inet', 'comment']`

**Setup**
Default setup, no special configuration.


Please be as specific as possible and give set-up details.

- [ ] on-prem machine
- [ ] VM (Virtualbox, KVM, etc. please specify)
- [ ] VM running on a cloud service, please be explicit and add details
- [ ] container (Kubernetes, Docker, containerd, etc. please specify)
- [ ] or a combination, please be explicit
- [ ] jails if it is FreeBSD

  
**Steps to Reproduce the behavior**

```
# salt-call -l debug ipset.new_set name=whitelist set_type="hash:net"  comment=True counters=True family=ipv4
[DEBUG   ] Reading configuration from /etc/salt/minion
[DEBUG   ] Including configuration from '/etc/salt/minion.d/_schedule.conf'
[DEBUG   ] Reading configuration from /etc/salt/minion.d/_schedule.conf
[DEBUG   ] Including configuration from '/etc/salt/minion.d/odoopbx.conf'
[DEBUG   ] Reading configuration from /etc/salt/minion.d/odoopbx.conf
[DEBUG   ] Including configuration from '/etc/salt/minion_local.conf'
[DEBUG   ] Reading configuration from /etc/salt/minion_local.conf
[DEBUG   ] Using cached minion ID from /etc/salt/minion_id: baba4741-4bcd-44a9-a4af-3682d8224631
[DEBUG   ] Using importlib_metadata to load entry points
[DEBUG   ] Override  __grains__: <module 'salt.loaded.int.log_handlers.sentry_mod' from '/usr/local/lib/python3.8/dist-packages/salt/log/handlers/sentry_mod.py'>
14:16:19 [DEBUG   ] salt.utils.parsers:220 Configuration file path: /etc/salt/minion
14:16:19 [WARNING ] salt.utils.verify:590 Insecure logging configuration detected! Sensitive data may be logged.
14:16:19 [DEBUG   ] salt.loader:868 Grains refresh requested. Refreshing grains.
14:16:19 [DEBUG   ] salt.config:1917 Reading configuration from /etc/salt/minion
14:16:19 [DEBUG   ] salt.config:2080 Including configuration from '/etc/salt/minion.d/_schedule.conf'
14:16:19 [DEBUG   ] salt.config:1917 Reading configuration from /etc/salt/minion.d/_schedule.conf
14:16:19 [DEBUG   ] salt.config:2080 Including configuration from '/etc/salt/minion.d/odoopbx.conf'
14:16:19 [DEBUG   ] salt.config:1917 Reading configuration from /etc/salt/minion.d/odoopbx.conf
14:16:19 [DEBUG   ] salt.config:2080 Including configuration from '/etc/salt/minion_local.conf'
14:16:19 [DEBUG   ] salt.config:1917 Reading configuration from /etc/salt/minion_local.conf
14:16:19 [DEBUG   ] salt.loader.lazy:857 Override  __utils__: <module 'salt.loaded.int.grains.zfs' from '/usr/local/lib/python3.8/dist-packages/salt/grains/zfs.py'>
14:16:19 [DEBUG   ] salt.modules.network:2132 Elapsed time getting FQDNs: 0.009395122528076172 seconds
14:16:19 [DEBUG   ] salt.loaded.int.grains.extra:56 Loading static grains from /etc/salt/grains
14:16:20 [DEBUG   ] salt.utils.lazy:99 LazyLoaded zfs.is_supported
14:16:20 [DEBUG   ] salt.minion:798 Connecting to master. Attempt 1 of 1
14:16:20 [DEBUG   ] salt.utils.network:2268 "localhost" Not an IP address? Assuming it is a hostname.
14:16:20 [DEBUG   ] salt.minion:238 Master URI: tcp://127.0.0.1:4506
14:16:20 [DEBUG   ] salt.crypt:505 Initializing new AsyncAuth for ('/etc/salt/pki/minion', 'baba4741-4bcd-44a9-a4af-3682d8224631', 'tcp://127.0.0.1:4506')
14:16:20 [DEBUG   ] salt.transport.zeromq:394 Generated random reconnect delay between '1000ms' and '11000ms' (10505)
14:16:20 [DEBUG   ] salt.transport.zeromq:401 Setting zmq_reconnect_ivl to '10505ms'
14:16:20 [DEBUG   ] salt.transport.zeromq:405 Setting zmq_reconnect_ivl_max to '11000ms'
14:16:20 [DEBUG   ] salt.transport.zeromq:156 Connecting the Minion to the Master URI (for the return server): tcp://127.0.0.1:4506
14:16:20 [DEBUG   ] salt.transport.zeromq:1180 Trying to connect to: tcp://127.0.0.1:4506
14:16:20 [DEBUG   ] salt.crypt:219 salt.crypt.get_rsa_pub_key: Loading public key
14:16:20 [DEBUG   ] salt.crypt:942 Decrypting the current master AES key
14:16:20 [DEBUG   ] salt.crypt:211 salt.crypt.get_rsa_key: Loading private key
14:16:20 [DEBUG   ] salt.crypt:191 salt.crypt._get_key_with_evict: Loading private key
14:16:20 [DEBUG   ] salt.crypt:869 Loaded minion key: /etc/salt/pki/minion/minion.pem
14:16:20 [DEBUG   ] salt.crypt:219 salt.crypt.get_rsa_pub_key: Loading public key
14:16:20 [DEBUG   ] salt.transport.zeromq:177 Closing AsyncZeroMQReqChannel instance
14:16:20 [DEBUG   ] salt.transport.zeromq:462 Connecting the Minion to the Master publish port, using the URI: tcp://127.0.0.1:4505
14:16:20 [DEBUG   ] salt.crypt:211 salt.crypt.get_rsa_key: Loading private key
14:16:20 [DEBUG   ] salt.crypt:869 Loaded minion key: /etc/salt/pki/minion/minion.pem
14:16:20 [DEBUG   ] salt.pillar:69 Determining pillar cache
14:16:20 [DEBUG   ] salt.crypt:505 Initializing new AsyncAuth for ('/etc/salt/pki/minion', 'baba4741-4bcd-44a9-a4af-3682d8224631', 'tcp://127.0.0.1:4506')
14:16:20 [DEBUG   ] salt.transport.zeromq:156 Connecting the Minion to the Master URI (for the return server): tcp://127.0.0.1:4506
14:16:20 [DEBUG   ] salt.transport.zeromq:1180 Trying to connect to: tcp://127.0.0.1:4506
14:16:20 [DEBUG   ] salt.crypt:211 salt.crypt.get_rsa_key: Loading private key
14:16:20 [DEBUG   ] salt.crypt:869 Loaded minion key: /etc/salt/pki/minion/minion.pem
14:16:20 [DEBUG   ] salt.transport.zeromq:177 Closing AsyncZeroMQReqChannel instance
14:16:20 [DEBUG   ] salt.utils.lazy:99 LazyLoaded jinja.render
14:16:20 [DEBUG   ] salt.utils.lazy:99 LazyLoaded yaml.render
14:16:20 [DEBUG   ] salt.utils.lazy:99 LazyLoaded ipset.new_set
14:16:20 [DEBUG   ] salt.utils.lazy:99 LazyLoaded direct_call.execute
14:16:20 [DEBUG   ] salt.utils.lazy:99 LazyLoaded cmd.run
14:16:20 [INFO    ] salt.loaded.int.module.cmdmod:417 Executing command /usr/sbin/ipset in directory '/root'
14:16:20 [ERROR   ] salt.loaded.int.module.cmdmod:877 Command '/usr/sbin/ipset' failed with return code: 1
14:16:20 [ERROR   ] salt.loaded.int.module.cmdmod:879 stdout: ipset v7.5: Syntax error: unknown inet family ['/usr/sbin/ipset', 'create', 'whitelist', 'hash:net', 'counters', 'family', [...], 'inet', 'comment']
14:16:20 [ERROR   ] salt.loaded.int.module.cmdmod:883 retcode: 1
14:16:20 [ERROR   ] salt.loaded.int.module.cmdmod:1300 Command '/usr/sbin/ipset' failed with return code: 1
14:16:20 [ERROR   ] salt.loaded.int.module.cmdmod:1305 output: ipset v7.5: Syntax error: unknown inet family ['/usr/sbin/ipset', 'create', 'whitelist', 'hash:net', 'counters', 'family', [...], 'inet', 'comment']
14:16:20 [DEBUG   ] salt.crypt:505 Initializing new AsyncAuth for ('/etc/salt/pki/minion', 'baba4741-4bcd-44a9-a4af-3682d8224631', 'tcp://127.0.0.1:4506')
14:16:20 [DEBUG   ] salt.transport.zeromq:156 Connecting the Minion to the Master URI (for the return server): tcp://127.0.0.1:4506
14:16:20 [DEBUG   ] salt.transport.zeromq:1180 Trying to connect to: tcp://127.0.0.1:4506
14:16:20 [DEBUG   ] salt.transport.zeromq:177 Closing AsyncZeroMQReqChannel instance
14:16:20 [DEBUG   ] salt.utils.lazy:99 LazyLoaded nested.output
local:
    ipset v7.5: Syntax error: unknown inet family ['/usr/sbin/ipset', 'create', 'whitelist', 'hash:net', 'counters', 'family', [...], 'inet', 'comment']
```

**Versions Report**
```
# salt-call  --versions-report
Salt Version:
          Salt: 3004
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: 18.6.1
      dateutil: 2.7.3
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 2.11.2
       libgit2: Not Installed
      M2Crypto: 0.31.0
          Mako: 1.0.7
       msgpack: 1.0.3
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: Not Installed
  pycryptodome: 3.14.1
        pygit2: Not Installed
        Python: 3.8.10 (default, Nov 26 2021, 20:14:08)
  python-gnupg: Not Installed
        PyYAML: 5.3.1
         PyZMQ: 21.0.2
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.3
 
System Versions:
          dist: ubuntu 20.04 focal
        locale: utf-8
       machine: x86_64
       release: 5.4.0-99-generic
        system: Linux
       version: Ubuntu 20.04 focal
 ```


</details>

Hints from issue discussion:
+
The above (closed) PR resolved this issue for me. https://github.com/saltstack/salt/pull/61621

Commit message: Fix ipset new_set bug

Patch to review:
diff --git a/changelog/61620.fixed.md b/changelog/61620.fixed.md
new file mode 100644
index 000000000000..cd0818244ce9
--- /dev/null
+++ b/changelog/61620.fixed.md
@@ -0,0 +1 @@
+`ipset.new_set` no longer fails when creating a set type that uses the `family` create option
diff --git a/salt/modules/ipset.py b/salt/modules/ipset.py
index 25717e7676a6..e47880d1b6e7 100644
--- a/salt/modules/ipset.py
+++ b/salt/modules/ipset.py
@@ -328,7 +328,7 @@ def new_set(name=None, set_type=None, family="ipv4", comment=False, **kwargs):
 
     # Family only valid for certain set types
     if "family" in _CREATE_OPTIONS[set_type]:
-        cmd.extend(["family", cmd, ipset_family])
+        cmd.extend(["family", ipset_family])
 
     if comment:
         cmd.append("comment")
diff --git a/tests/pytests/functional/modules/test_ipset.py b/tests/pytests/functional/modules/test_ipset.py
index 634365662bb8..5b77fa3dd0bf 100644
--- a/tests/pytests/functional/modules/test_ipset.py
+++ b/tests/pytests/functional/modules/test_ipset.py
@@ -47,3 +47,19 @@ def test_ipset_add_comment_kwarg(ipset, setup_set):
     assert ret == "Success"
     check_set = ipset.list_sets()
     assert any([x for x in check_set if x["Name"] == setup_set])
+
+
+def test_ipset_new_set_with_family(ipset):
+    """
+    test ipset.new_set with set_type that uses family (eg. hash:ip)
+    """
+    set_name = "test_name_haship"
+    ret = ipset.new_set(name=set_name, set_type="hash:ip")
+    assert ret == True
+    check_set = ipset.list_sets()
+    try:
+        assert any([x for x in check_set if x["Name"] == set_name])
+    except Exception as e:
+        raise e
+    finally:
+        ipset.delete_set(set_name)


Code review comments:
diff_hunk: @@ -47,3 +47,19 @@ def test_ipset_add_comment_kwarg(ipset, setup_set):
     assert ret == "Success"
     check_set = ipset.list_sets()
     assert any([x for x in check_set if x["Name"] == setup_set])
+
+
+def test_ipset_new_set_with_family(ipset):
+    """
+    test ipset.new_set with set_type that uses family (eg. hash:ip)
+    """
+    set_name = "test_name_haship"
+    ret = ipset.new_set(name=set_name, set_type="hash:ip")
+    assert ret == True
+    check_set = ipset.list_sets()
+    try:
+        assert any([x for x in check_set if x["Name"] == set_name])
+    except Exception as e:
path: tests/pytests/functional/modules/test_ipset.py
text: @user1:
Can you remove catching the exception here. The test will properly raise any assertion errors and call the finally.

@author:
You are right. Done.