You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "bhouse-nexthop (via GitHub)" <gi...@apache.org> on 2026/10/08 00:36:33 UTC
[PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
bhouse-nexthop opened a new pull request, #14350:
URL: https://github.com/apache/cloudstack/pull/14350
### Description
A VPC network ACL rule can carry several CIDRs. On the VR those rules are either applied in the wrong order or not applied at all, and nothing is reported back to the UI or API.
Fixes #12668
#### Why it breaks
**IPv4, iptables (normal VPC):** `AclDevice.process()` turns each ACL rule into one fw entry, e.g. `-A ACL_INBOUND_eth3 -s 1.2.3.4/32,2.3.4.5/32 -p tcp --dport 22 -j ACCEPT`. iptables expands a comma-separated address into one rule per address, so that line creates two rules. `CsNetfilters.compare()` inserts each ACL rule at the chain's current rule count (just ahead of the trailing `DROP`), and bumps that count by **one** per fw entry, not by the number of rules iptables actually created. After a multi-CIDR rule the count is short by `n-1`, so every later rule is inserted between the expanded rules. All but the last CIDR of each multi-CIDR rule drift down behind every rule that follows, including an explicit deny.
The effect: an allow silently stops working for every CIDR but one, and a multi-CIDR *deny* placed ahead of an allow (e.g. "block RFC1918, then allow 0.0.0.0/0") stops denying. Single-CIDR rules are unaffected. A lone multi-CIDR rule also looks fine, since there is nothing after it to land in the gap, which is why this can be hard to reproduce. Both ingress (`filter`) and egress (`mangle`) chains are affected.
**IPv6, and IPv4 in routed mode (nftables):** `__process_ip6()` and `__process_routing_ip4()` build `ip6 saddr a,b ...` / `ip saddr a,b ...`. nft rejects a bare comma list:
```
$ nft add rule ip6 t c ip6 saddr 2001:db8:1::/64,2001:db8:2::/64 tcp dport 22 accept
Error: syntax error, unexpected /, expecting end of file or newline or semicolon
```
The rules are added one `nft add rule` at a time and the failure is only logged, so each multi-CIDR rule is silently missing while the rest of the list loads.
#### How it is fixed
- **iptables:** `AclDevice.process()` emits one fw entry per CIDR, in list order. Each entry is then exactly one iptables rule, so the insert position `compare()` tracks stays correct. (This is simpler and safer than teaching `compare()` how iptables expands `-s`/`-d`.)
- **nftables:** a multi-CIDR match is rendered as a set, `ip saddr {a,b}`, via a small `nftAddrSet()` helper. This is the same syntax the VPC/IPv6 *firewall* code in the same file already uses for `source_cidr_list`. It is passed through `/bin/sh` (dash), which does not brace-expand.
#### How to reproduce the old behaviour
1. In a VPC, create a network ACL list with these ingress rules and attach it to a tier:
| # | Protocol | CIDR list | Port | Action |
|---|---|---|---|---|
| 1 | TCP | `1.2.3.4/32,2.3.4.5/32` | 22 | Allow |
| 2 | TCP | `3.4.5.6/32` | 443 | Allow |
| 3 | All | `0.0.0.0/0` | – | Deny |
2. On the VR, run `iptables -S ACL_INBOUND_ethX` for that tier's interface.
Before (the first CIDR of rule 1 sits behind the explicit deny and never matches):
```
-A ACL_INBOUND_eth3 -d 225.0.0.50/32 -j ACCEPT
-A ACL_INBOUND_eth3 -d 224.0.0.18/32 -j ACCEPT
-A ACL_INBOUND_eth3 -s 2.3.4.5/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -s 3.4.5.6/32 -p tcp -m tcp --dport 443 -j ACCEPT
-A ACL_INBOUND_eth3 -j DROP
-A ACL_INBOUND_eth3 -s 1.2.3.4/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -j DROP
```
After:
```
-A ACL_INBOUND_eth3 -d 225.0.0.50/32 -j ACCEPT
-A ACL_INBOUND_eth3 -d 224.0.0.18/32 -j ACCEPT
-A ACL_INBOUND_eth3 -s 1.2.3.4/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -s 2.3.4.5/32 -p tcp -m tcp --dport 22 -j ACCEPT
-A ACL_INBOUND_eth3 -s 3.4.5.6/32 -p tcp -m tcp --dport 443 -j ACCEPT
-A ACL_INBOUND_eth3 -j DROP
-A ACL_INBOUND_eth3 -j DROP
```
For the IPv6 side, give the tier an IPv6 CIDR and add a rule with two IPv6 CIDRs. Before the fix it is absent from `nft list chain ip6 ip6_acl ethX_ingress_policy`, and `/var/log/cloud.log` shows the `nft add rule` syntax error. After the fix it is listed as `ip6 saddr { a, b } ...`.
Until this lands, the workaround is one CIDR per ACL rule.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [x] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] Major
- [ ] Minor
- [ ] Trivial
### How Has This Been Tested?
- New `systemvm/test/TestCsAcl.py` covers the iptables, IPv6 and routed-IPv4 paths. All three tests fail on the current `4.22` code and pass with this change.
- The before/after chains above are real output, not hand-written. They come from feeding the rules `AclDevice` generates through the unmodified `CsNetfilters().compare()` against real iptables (nf_tables backend) in a network namespace. Re-applying the same ACL a second time leaves the chain unchanged.
- The nft set syntax was checked with real `nft add rule` for both `ip` and `ip6`, and the old bare-list form was confirmed to fail.
- `pycodestyle` and `pylint` (as in `systemvm/test/runtests.sh`) report nothing new.
#### How did you try to break this feature and the system with this change?
- Mixed IPv4/IPv6 CIDR lists still split the same way as before (`removeUndesiredCidrs`). Each family only gets its own CIDRs, and a single-CIDR rule produces exactly the same output as before.
- Rules with no CIDR (`all`) are unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#issuecomment-6049813246
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/14350?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 4.04%. Comparing base ([`2974af8`](https://app.codecov.io/gh/apache/cloudstack/commit/2974af84f22a8def8339380628d8653a4f016d3f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`de64fa2`](https://app.codecov.io/gh/apache/cloudstack/commit/de64fa2c05a53681516d5a42257f2c6623daca66?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (2974af8) and HEAD (de64fa2). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (2974af8) | HEAD (de64fa2) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.22 #14350 +/- ##
=============================================
- Coverage 18.02% 4.04% -13.99%
=============================================
Files 5936 449 -5487
Lines 535823 38240 -497583
Branches 65612 7082 -58530
=============================================
- Hits 96582 1546 -95036
+ Misses 428242 36482 -391760
+ Partials 10999 212 -10787
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/14350/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/14350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.04% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/14350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/14350?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#issuecomment-6057987035
We have to run our own cloudstack builds ourselves because of all the pending PRs we need incorporated. Hopefully at some point that won't be true ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#discussion_r4259283022
##########
systemvm/debian/opt/cloud/bin/configure.py:
##########
@@ -583,16 +592,21 @@ def process(self, direction, rule_list, base, is_routed):
count = base
for i in rule_list:
- ruleData = copy.copy(i)
- cidr = ruleData['cidr']
+ cidr = i['cidr']
if cidr is not None and cidr != "":
cidr = removeUndesiredCidrs(cidr, 6)
if cidr is None or cidr == "":
continue
- ruleData['cidr'] = cidr
- r = self.AclRule(direction, self, ruleData, self.config, count)
- r.create()
- count += 1
+ # iptables expands "-s a,b" into one rule per address, but CsNetfilters
+ # counts each entry as a single rule when placing the next ACL rule ahead
+ # of the DROP. Emit one entry per CIDR so the two stay in step.
+ cidrs = [c for c in cidr.split(",") if c] if cidr else [cidr]
+ for c in cidrs:
Review Comment:
did you check how long an apply takes with a few hundred cidrs in one rule? each cidr is now its own iptables call, plus one more to remove the old one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on code in PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#discussion_r4273529803
##########
systemvm/debian/opt/cloud/bin/configure.py:
##########
@@ -583,16 +592,21 @@ def process(self, direction, rule_list, base, is_routed):
count = base
for i in rule_list:
- ruleData = copy.copy(i)
- cidr = ruleData['cidr']
+ cidr = i['cidr']
if cidr is not None and cidr != "":
cidr = removeUndesiredCidrs(cidr, 6)
if cidr is None or cidr == "":
continue
- ruleData['cidr'] = cidr
- r = self.AclRule(direction, self, ruleData, self.config, count)
- r.create()
- count += 1
+ # iptables expands "-s a,b" into one rule per address, but CsNetfilters
+ # counts each entry as a single rule when placing the next ACL rule ahead
+ # of the DROP. Emit one entry per CIDR so the two stay in step.
+ cidrs = [c for c in cidr.split(",") if c] if cidr else [cidr]
+ for c in cidrs:
Review Comment:
Yes, I measured it. I fed the rules `AclDevice` generates through the unmodified `CsNetfilters.compare()`, against real iptables (1.8.10, nf_tables backend) in a throwaway network namespace. The test ACL has one TCP rule with N CIDRs, followed by a single-CIDR allow and a deny. Times are wall clock, with the number of `iptables` processes run:
| N | before, first apply | before, re-apply | after, first apply | after, re-apply |
|---|---|---|---|---|
| 100 | 0.02s / 8 | 1.5s / 106 | 0.3s / 107 | 2.0s / 205 |
| 300 | 0.03s / 8 | 5.1s / 306 | 1.0s / 307 | 6.6s / 605 |
| 1000 | rule not applied (`iptables: Message too long`) | - | 9.8s / 1007 | 43.9s / 2005 |
Generating the rules in Python takes about 1ms either way.
Most of this cost was already there before the PR. `has_rule()` never matches a rule that has a position, so on every apply each ACL rule is inserted again and its old copy is deleted as unseen. Those old copies were already one rule per CIDR, because iptables had expanded `-s a,b,c`. So a re-apply was already 1 insert + N deletes. With this PR it is N inserts + N deletes. The deletes are the slow part: about 17ms each at N=300, against about 3ms per insert, because each call reads the whole ruleset. At N=300 this PR adds roughly 1 to 1.5s to an apply that already took 5s. That is also exactly what the current workaround costs, since N single-CIDR rules produce the same iptables calls.
The real fix is to batch the changes through `iptables-restore --noflush`. That way, 300 inserts plus 300 deletes take about 0.02s. It would change how every iptables rule on the VR is applied, though:
- Failure handling changes: one bad line fails the whole batch.
- It needs chunking: a 1000-rule batch also fails with `Message too long`.
So I'd rather keep it out of this bug fix and do it as a separate PR. I'm happy to open one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#issuecomment-6342327953
@DaanHoogland this one is in the same VPC ACL area as #14355 and #14352, which you merged today. Could you or @weizhouapache take a look and add it to 4.22.2? Today a multi-CIDR rule is applied on the VR in the wrong order or not at all, with nothing reported back; this applies every CIDR, in order (fixes #12668). I understand 4.22.2 is due to go out Monday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#issuecomment-6049760568
@vladimirpetrov @sureshanaparti could you take a look at this one? We'd like this fix to make it into the upcoming 4.22.2 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "fermosan (via GitHub)" <gi...@apache.org>.
fermosan commented on PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#issuecomment-6055366097
Hello, this seems to do the work. Is there any chance for this to be applied to systemvm for 4.23 ? As far as i know there is no 4.23 systemvm. Maybe a 4.22 image with only this patch could make it to a 4.23?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "fermosan (via GitHub)" <gi...@apache.org>.
fermosan commented on PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#issuecomment-6057822590
Thanks for the clarification. We could temporarily backport the change then. When next release is up probably the patch will be already merged. I understand it can work one by one, but we have a case with a specific user that wants to create a lot of rules and doing it one by one generates around 2.000 rules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] VPC ACL: apply every CIDR of a multi-CIDR rule, in order [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#issuecomment-6057711187
@fermosan thanks for confirming. You shouldn't need a new systemvm template for this. `configure.py` and the rest of the VR scripts are shipped with the management server and pushed to the router, not baked into the template image. Once the fix is in a release your management server runs, existing routers pick it up either when they are recreated, or in place with `restartVPC id=<vpc> livepatch=true` (with `cleanup=false`), which live-patches the router's scripts before restarting it.
This PR targets 4.22. Fixes on the 4.22 branch are merged forward into main, so it should reach 4.23 the same way; the release managers decide which release. Until then, one CIDR per ACL rule avoids it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org