Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Syntax Highlighting for nftables-firewall config file #3325

Merged
merged 1 commit into from
Oct 6, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions runtime/syntax/nftables.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
filetype: nftables

detect:
filename: "nftables.conf$"
header: "^(#!.*/(env +)?nft( |$)|flush +ruleset)"

rules:
- type: "\\b(chain|counter|map|rule|ruleset|set|table)\\b"
- type: "\\b(ether|icmp|icmpv6|icmpx|inet|ip|ip6|ipv4|ipv6|tcp|udp)\\b"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be reduced to i(cm)?p(x|(v?(4|6))?), but I'm unsure if it improves readability or regex compile time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think not that this can be reduced, because of then would be things like chain, counter, map etc and tcp udp not highlighed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, sure...forget to fill up: ether|inet|i(cm)?p(x|(v?(4|6))?)|tcp|udp 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I`ve added your suggestions, you can try if you want to. If i should build it for you, please answer me, then i will build it and create a little release for you.

But you can clone the Repo, and do the described manual build. (Don`t forget to install golang before)
https://github.com/theredcmdcraft/micro/tree/nftables-improvements

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt @JoeKar needs help with that. He is a maintainer of micro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt @JoeKar needs help with that. He is a maintainer of micro.

Oops. I didn`t see the Collaborator Symbol on the right side.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, it was my fault not testing it earlier at work were we've some good examples of nftable rules. It was still on my todo before it was merged.
Ok, anyway...since this merge request is already merged we should create a new one to properly add the fixes into the nftables.yaml.
I suggest to do the following (after the "upstream" is available in your private fork too):

git check nftables-improvements
git fetch --all
git reset HEAD~1
git stash
git reset --hard upstream/master
git stash apply
git stash add runtime/syntax/nftables.yaml
git commit -m "Added Suggested changes"
git push --force-with-lease

Create from this resulting branch (nftables-improvements) in your fork a new pull request to to our upstream repository. Sure you can follow your own approach to create a new clean PR.
Currently I've some doubts that the changed symbol and constant.string will work as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, it was my fault not testing it earlier at work were we've some good examples of nftable rules. It was still on my todo before it was merged. Ok, anyway...since this merge request is already merged we should create a new one to properly add the fixes into the nftables.yaml. I suggest to do the following (after the "upstream" is available in your private fork too):

git check nftables-improvements
git fetch --all
git reset HEAD~1
git stash
git reset --hard upstream/master
git stash apply
git stash add runtime/syntax/nftables.yaml
git commit -m "Added Suggested changes"
git push --force-with-lease

Create from this resulting branch (nftables-improvements) in your fork a new pull request to to our upstream repository. Sure you can follow your own approach to create a new clean PR. Currently I've some doubts that the changed symbol and constant.string will work as expected.

Did i have to do these steps, when i synced my repo with this repo? I Synced the Fork, and then i created a new branch from the master branch, where i did the changes yesterday. Or can i simply create a new PR with my newly created branch as source. The Target is the micro master branch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did i have to do these steps, when i synced my repo with this repo? I Synced the Fork, and then i created a new branch from the master branch, where i did the changes yesterday.

Sure, you can use a fully new/clean branch derived from the synced master to publish your changes within a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new PR is #3517

- special: "\\b(elements|hook|policy|priority|type)\\b"
- identifier: "\\b(ct|iif|iifname|meta|oif|oifname|th)\\b"
- statement: "\\b(accept|drop|goto|jump|log|masquerade|reject)\\b"
- preproc: "\\b(add|define|flush|include|delete)\\b"
- symbol: "[-=/:;,@]"
- symbol.operator: "[<>.&|^!]|\\b(and|ge|gt|le|lt|or|xor)\\b"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The != isn't properly handled as unequal-operator. I'd assume something similar to:

- symbol.operator: "[-+*/%=<>.:;,~&|^!?]|\\b(offsetof|sizeof)\\b"

Currently the = is tracked in line 14.

- constant.string: '([\"]{1})(.*)([\"]{1})'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string handling is broken and I recommend to use a region for this purpose, similar to:

- constant.string:
start: "'"
end: "'"

- identifier.var: "[$@][a-zA-Z_.][a-zA-Z0-9_/.-]*"
- comment: "(^|[[:space:]])#([^{].*)?$"
- indent-char.whitespace: "[[:space:]]+$"
- indent-char: " + +| + +"