[3007.x] Fix/add nftables icmpv6 support

[jdelic]

What does this PR do?

Fixes #67882

The nftables state just sends its kwargs to the nftables module. This in turn is currently missing support for ipv6 icmp packet types (icmpv6). This means that currently Salt cannot configure a firewall in such a way that it allows pings, for example. This small patch remedies that.

Previous Behavior

The following was impossible:

icmp-recv-ipv4:
    nftables.append:
        - table: filter
        - family: ip4
        - chain: input
        - jump: accept
        - proto: icmp
        - icmp-type: echo-reply,destination-unreachable,source-quench,redirect,echo-request,time-exceeded,parameter-problem,timestamp-request,timestamp-reply,info-request,info-reply,address-mask-request,address-mask-reply,router-advertisement,router-solicitation
        - order: 4
        - save: True
        - require:
            - pkg: nftables


icmp-recv-ipv6:
    nftables.append:
        - table: filter
        - family: ip6
        - chain: input
        - jump: accept
        - proto: icmp
        # This wasn't supported until now --v
        - icmpv6-type: echo-reply,echo-request,nd-router-advert,nd-neighbor-solicit,nd-neighbor-advert
        - order: 4
        - save: True
        - require:
              - pkg: nftables

New Behavior

The above works now.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

I have not added any test for the above.

1. The current icmp-type functionality is untested
2. The current tests don't really test anything. They test that the module returns a state comment which fits the expectation and mock cmd.run. So nft is never executed by the tests, so you'd never know if any of this code produced an invalid rule. I do not have the time to contribute to Salt to essentially rewrite the nftables module, so this will have to live without tests.
3. The current documentation isn't really explaining the list of available parameters from the module. Neither does the module. Both need improvement, as do the tests, but this PR is about simply adding support for icmpv6.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

[twangboy]

Please write a test for this

[jdelic]

Please write a test for this

I explained in the PR description why I didn't. That argument still stands.

[dmurphy18]

@jdelic Regardless of opinions, if you change code, you write a test to check that code, unless there is already a test which covers the change. This rule applies to the core team too.
Just because some test doesn't already exist is no excuse to not write a test.

In the past, before 2019, Salt would allow code to be merged after code review without tests been written for it, and this led to a mess, hence since 2019, all code changes require tests, and tests using pytest. Using mock can test the code without having to have an actual VM up etc., noting even simple unnoticed typo's parsing a kwarg can trip code up.

[jdelic]

@dmurphy18
Alll tests in test_nftables.py boil down to "The Python parser can read the file and string concatenation and comparisons still work". These tests do nothing, add nothing, or check nothing that you can't assert by linting the code. The use of mock there breaks all potential usefulness of the tests.

For example, the first test for delete_table is this:

    mock_ret = {
        "result": False,
        "comment": "Table nat in family ipv4 does not exist",
    }
    with patch("salt.modules.nftables.check_table", MagicMock(return_value=mock_ret)):
        ret = nftables.delete_table(table="nat")
        assert ret == {
            "result": False,
            "comment": "Table nat in family ipv4 does not exist",
        }

This looks very involved, but all it does is check that this code

    res = check_table(table, family=family)
    if not res["result"]:
        return res

which thanks to mocking translates to:

    if not False:
        return mock_ret

works.

My personal opinion is that this type of testing is problematic in any project, but as a OSS project (of a 51 billion USD p.a. company no less), demanding it from the community is a bad way to deal with free labor provided by the community. I

The test I now added is exactly this sequence of code every time:

    ret = {"comment": "", "rule": "", "result": False}
    rule = ""
    rule += "icmpv6 type { echo-reply,echo-response }"
    after_jump = ["accept "]
    for item in after_jump:
        rule += item
    ret["rule"] = "{} {} rule {} {} {} {}".format(
        "/usr/sbin/nft", "add", "ip6", "filter", "input", rule
    )
   assert ret["rule"].strip() == "/usr/bin/nft add ip6 filter input icmpv6 type { echo-reply,echo-response } accept"

Like every other test in the module It will deterministically detect if Python's string assignment or comparison operators ever stop working, or if someone changes the order of format arguments in line 300. Regardless of whether that breaks actual nftables or not. As I noted in the PR description I can't contribute the time to rearchitect the nftables tests to a point where they would serve as actual tests, so this will have to do. 🚀

[dmurphy18]

Wondering if you can use a few other icmpv6-type too

[dmurphy18]

This is better written using f-strings, using Python 3.10 as python version

 rule += f"icmpv6 type {{ {kwargs["icmpv6-type"]} }} "

[jdelic]

@dmurphy18
I agree. But wouldn't it be much better to first make this small simple change first and then later change all of the .formats in the file? Instead of changing just one single instance or mixing this .format change in with this PR?

[twangboy]

I'm surprised it wasn't caught by pre-commit

[dmurphy18]

@jdelic Understand that sometimes, mock tests only test mock and not real tests, hence I favor integration tests, unless that is too complicated a test situation to instantiate. Trying to get better tests, but now very limited resources to do this compared to just over a year ago.

Mock was a suggestion, but code changes required tests, and something is better than nothing, at times ;).
Thanks for your contribution