Fix nftables module check function doesn't understand that braces are optional#67079
Fix nftables module check function doesn't understand that braces are optional#67079nicholasmhughes wants to merge 2 commits intosaltstack:3006.xfrom
Conversation
There was a problem hiding this comment.
Should use f-strings at this point.
Can you email david-dm.murphy@broadcom.com to discuss salt extensions too, since this PR has your attention and reaching out to you has failed so far.
BTW pipelines are under development, so it will be a while before they are running correctly, hopefully by the end of the week,
| ) | ||
| search_rule = f"{rule} #" | ||
| out = __salt__["cmd.run"](cmd, python_shell=False).find(search_rule) | ||
| cmd = f"{_nftables_cmd()} --handle list chain {nft_family} {table} {chain}" |
There was a problem hiding this comment.
Why did you remove --numeric? While I don't think multiple --numeric args has any different effect, at least one does and is probably desired.
There was a problem hiding this comment.
The --numeric changes the output:
--numeric skips the lookup of IP addresses to symbolic hostnames
--numeric --numeric also shows port numbers
--numeric --numeric --numeric also shows protocols and UIDs/GUIDs
These can be substituted with -n, -nn, -nnn. So, I guess this would only affect the parsing of the results.
| search_rule = f"{rule} #" | ||
| out = __salt__["cmd.run"](cmd, python_shell=False).find(search_rule) | ||
| cmd = f"{_nftables_cmd()} --handle list chain {nft_family} {table} {chain}" | ||
| search_rule = f"{rule} #".replace("{ ", "{? ?").replace(" }", " ?}?") |
There was a problem hiding this comment.
I don't think we want to be in the business of trying to parse and maintain the syntax of nftables rules. I suggest trying to find a way to have the tools canonicalize both rules. nft also has a --json flag to give json output which may also be cleaner/safer to handle.
There was a problem hiding this comment.
There are other things that would make the rules not normalized and therefore not match like extra whitespaces. Here is a list according to copilot:
Whitespace: Variations in spaces or tabs can impact rule formatting, although it generally doesn’t affect functionality.
Comments: Inline or separate comments within rules might not be present in the canonical form.
Ordering: The sequence of rules and attributes can differ.
Default actions: When default actions aren’t explicitly stated, different interpretations may arise.
Aliases: Using aliases for set names or interfaces may introduce variations.
Optional attributes: Omitting certain attributes might create differences.
Nesting: The manner and extent of nesting in expressions might vary.
There was a problem hiding this comment.
Instead of trying to normalize and and check the rule yourself, maybe we could use the --check flag with nft.
| cmd = f"{_nftables_cmd()} --handle list chain {nft_family} {table} {chain}" | ||
| search_rule = f"{rule} #".replace("{ ", "{? ?").replace(" }", " ?}?") | ||
| out = __salt__["cmd.run"](cmd, python_shell=False) | ||
| found = re.search(search_rule, out) |
There was a problem hiding this comment.
We did not escape the search_rule before using it as a regex so regex special characters like . and [ will not be treated as literal.
dwoz
force-pushed
the
fix-nftables-brace-handling
branch
from
3654ccf to
9f888fb
Compare
|
@nicholasmhughes Could you address the comments from @bdrx312? |
twangboy
left a comment
twangboy
left a comment
There was a problem hiding this comment.
Please make this PR against the 3006.x branch as it is a bugfix there.
twangboy
added
pending-changes
twangboy
force-pushed
the
fix-nftables-brace-handling
branch
from
9f888fb to
365708c
Compare
|
@nicholasmhughes It looks like tests are passing. Could you address the comments from @bdrx312 ? |
|
@nicholasmhughes Thanks for rebasing. Could you address @bdrx312 questions? |
|
@nicholasmhughes ping. Looking to see if this can land in the next release. |
|
@nicholasmhughes Do you have some time to look at this? |
…nd that braces are optional
dwoz
force-pushed
the
fix-nftables-brace-handling
branch
from
915e29a to
ddc509e
Compare
5 participants
What does this PR do?
See issue for details
What issues does this PR fix or reference?
Fixes #67078
Previous Behavior
See issue for details
New Behavior
The braces are effectively ignored for comparison purposes
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes
Please review Salt's Contributing Guide for best practices, including the
PR Guidelines.
See GitHub's page on GPG signing for more information about signing commits with GPG.