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

Unable to add firewall rules to IP Tables: Nomad enforces first rule #10180

Closed
microadam opened this issue Mar 13, 2021 · 7 comments · Fixed by #10181
Closed

Unable to add firewall rules to IP Tables: Nomad enforces first rule #10180

microadam opened this issue Mar 13, 2021 · 7 comments · Fixed by #10181

Comments

@microadam
Copy link
Contributor

microadam commented Mar 13, 2021

Nomad version

Nomad v1.0.4 (9294f35)

Issue

I am unable to add any additional firewall rules to the NOMAD-ADMIN IPTables chain, as nomad enforces that its ACCEPT rule is the first rule. So any rules that I append before nomad runs, are skipped.

This seems to be the offending line:

if err := ensureFirstChainRule(ipt, cniAdminChainName, b.generateAdminChainRule()); err != nil {

Is there any reason to ensure it's always the first rule?

According to https://www.cni.dev/plugins/meta/firewall/

The admin chain is intended as an user-controlled chain for custom rules that run prior to rules managed by the firewall plugin

Reproduction steps

Add an IP Tables rule before nomad runs to the NOMAD-ADMIN chain. Run nomad with a container that exposes a port.

Expected Result

Nomad to append its rule, so any existing rules are executed first.

-A NOMAD-ADMIN -j DOCKER-USER >>>>>>> CUSTOM RULE
-A NOMAD-ADMIN -d 172.26.64.0/20 -o nomad -j ACCEPT >>>>>> NOMAD RULE

Actual Result

Rule is ignored as nomad enforces its rule to be first

-A NOMAD-ADMIN -d 172.26.64.0/20 -o nomad -j ACCEPT >>>>>> NOMAD RULE
-A NOMAD-ADMIN -j DOCKER-USER >>>>>>> CUSTOM RULE
@tgross
Copy link
Member

tgross commented Mar 24, 2021

Looks like that link needs to be https://www.cni.dev/plugins/current/meta/firewall/#forward:

The CNI-FORWARD chain first sends all traffic to CNI-ADMIN chain, which is intended as an user-controlled chain for custom rules that run prior to rules managed by the firewall plugin. The firewall plugin does not add, delete or modify rules in the CNI-ADMIN chain.

This seems reasonable to me. The existing behavior was added in 99742f2 and the doc comment here from @nickethier is awfully suggestive that I might be missing something as to why that could be a problem. So tagging him and @shoenig for a consult here.

@microadam
Copy link
Contributor Author

Thanks for taking a look! I believe the linked PR that I made is completely backwards compatible, as the NOMAD-ADMIN chain is purely controlled by Nomad, it only adds one rule to it. So if others don't care about adding firewall rules, it would still be the first and only rule in the chain if its appended (as opposed to being forced to be first). Would very much love to know if there is a good reason to have it as always first, as it seems like quite a conscious design decision. Any input from @nickethier would def be most appreciated :)

@microadam
Copy link
Contributor Author

@tgross @nickethier Not sure what the usual time frame for responding is, so apologies if I am prematurely following up and there is a process in place / queue that I am not aware of! Would just love to know the if the associated PR is something that could be merged, or if I need to find an alternative solution / workaround

@tgross
Copy link
Member

tgross commented Apr 13, 2021

No worries about pinging us, especially after I totally said we'd look at it. I wanted to do some testing to make sure we understood what would happen around Nomad client restarts and it looks ok. Going to take one more pass over it in the next day or so and then merge if there's nothing we run into.

@microadam
Copy link
Contributor Author

Thank you very much for the quick response! Sounds excellent to me :)

Nomad - Community Issues Triage automation moved this from In Progress to Done Apr 15, 2021
@tgross
Copy link
Member

tgross commented Apr 15, 2021

#10181 has been merged and will ship in the upcoming Nomad 1.1.0

@tgross tgross added this to the 1.1.0 milestone Apr 15, 2021
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants