Skip to content

systemvm: ipv6 fw_input — accept return traffic from established,rela…#13173

Merged
weizhouapache merged 1 commit into
apache:4.22from
agronaught:fix-13171-v6-fw-input-established-related
May 19, 2026
Merged

systemvm: ipv6 fw_input — accept return traffic from established,rela…#13173
weizhouapache merged 1 commit into
apache:4.22from
agronaught:fix-13171-v6-fw-input-established-related

Conversation

@agronaught
Copy link
Copy Markdown

@agronaught agronaught commented May 17, 2026

This PR adds the IPv6 equivalent of fw_router_routing() to the systemvm Virtual Router's network configuration, so that return traffic for VR-initiated IPv6 connections (BGP to upstream PE peers, NTP, DNS lookups, etc.) is allowed back through the ip6_firewall fw_input chain.

Problem

The systemvm VR's nftables ip6 ip6_firewall fw_input chain is created with policy=drop and only ICMPv6 accept rules. The IPv4 INPUT chain has the equivalent iifname "eth2" ct state established,related accept rule (added by fw_router_routing() in CsAddress.py); the IPv6 path has no such rule.

Effect: any v6 connection the VR itself initiates outbound has its return traffic silently dropped at the v6 INPUT hook before TCP processes it. For Isolated IPv6 ROUTED networks this is fatal — BGP IPv6 sessions cannot reach Established, tenant /64 prefixes are never advertised upstream, and VMs in the network are unreachable from the IPv6 internet.

#10970 added the equivalent rule to the FORWARD chain (covering tenant VM return traffic) but explicitly removed it from the INPUT chain in its second commit. This PR completes that fix for VR-originated traffic.

Behavioural change

Before this PR, IPv6 BGP sessions from VRs in IsolatedV6RoutedFiltered (and similar Routed v6) network offerings stay in Connect state indefinitely. After this PR, sessions reach Established within seconds of VR start and prefix advertisements work normally.

The change is additive and behind the existing is_routed() / is_vpc() gating — only routed, non-VPC networks see new INPUT rules. No change for existing v4 paths, v4 NATted networks, or VPC networks.

Fixes: #13171

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • 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
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Justifying Major: any operator wanting to ship the IsolatedV6RoutedFiltered offering (or any v6 Routed isolated network with Firewall service) for production tenant workloads is blocked. Workaround requires per-VR nft injection that wipes on every tenant FW rule change, making the offering unusable as a customer product without a downstream patch like this one.

Screenshots (if appropriate)

N/A — kernel-level firewall change.

How Has This Been Tested?

Verified end-to-end on Apache CloudStack 4.22.0.0, KVM hypervisor (Ubuntu 24.04 hosts), with:

  • Zone configured for BGP Routed networks (ASN range, BGP peers, IPv6 guest prefix /48)
  • Tenant network using IsolatedV6RoutedFiltered offering
  • Two independent fresh VRs in two different tenant networks

Before the patch:

vtysh -c "show bgp ipv6 unicast summary"
Neighbor State/PfxRcd
2400:88e0:ffff:258::2 Connect 0
2400:88e0:ffff:258::3 Connect 0

Hypervisor-side packet capture on the underlay confirms PE responds with SYN-ACK, but the VR's TCP stack never delivers it to FRR. Kernel TCPMD5* counters stay at zero — drop happens at netfilter before TCP processes the segment. Inside the VR:

$ nft list table ip6 ip6_firewall
table ip6 ip6_firewall {
chain fw_input {
type filter hook input priority filter; policy drop;
icmpv6 type { ... } accept
}
...
}

No ct state established,related accept rule.

After the patch:

vtysh -c "show bgp ipv6 unicast summary"
Neighbor State/PfxRcd
2400:88e0:ffff:258::2 Established 1
2400:88e0:ffff:258::3 Established 1

fw_input now includes the new rule with active counters:

iifname "eth2" ct state established,related counter packets ... bytes ... accept

Verified end-to-end: SSH from public IPv6 internet to a VM inside the v6-routed network succeeds. Reachability survives subsequent tenant firewall rule updates (the rule is rebuilt from nft_ipv6_fw on every IpTablesExecutor.process() cycle).

How did you try to break this feature and the system with this change?

  • Tenant firewall rule churn: added/removed tenant ingress rules via cmk createIpv6FirewallRule / deleteIpv6FirewallRule repeatedly after the patch. IpTablesExecutor.process() flushes and rebuilds the v6 table each time; the new INPUT rule is re-emitted on every cycle because it's now in nft_ipv6_fw. Counters resume; BGP stays Established.
  • VR reboot: rebooted the VR (cmk rebootRouter). After the reboot pulls fresh cloud-scripts.tgz, the patched CsAddress.py runs in the rebuilt VR and the rule is in place from boot. BGP establishes within ~30s of VR ready.
  • Non-routed networks: confirmed is_routed() gating means standard Isolated v4 networks and VPC networks see no new rules in either chain — no behaviour change for them.
  • Cross-account / cross-domain: verified the rule fires per-VR (each tenant network's VR gets its own rule with its own eth2 reference and per-VR counter), with no cross-tenant traffic leakage.

Tested with both single-tenant and multi-tenant network deployments. Validated the substrate change on ACS 4.22.0.0; same code path exists in 4.20 branch HEAD per inspection.

@boring-cyborg
Copy link
Copy Markdown

boring-cyborg Bot commented May 17, 2026

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@agronaught agronaught force-pushed the fix-13171-v6-fw-input-established-related branch from 0a11f6e to 992fbf5 Compare May 17, 2026 23:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@weizhouapache weizhouapache added this to the 4.23.0 milestone May 18, 2026
@weizhouapache
Copy link
Copy Markdown
Member

@agronaught
thanks for creating the PR
I will verify it

@agronaught
Copy link
Copy Markdown
Author

any chance of also getting this backported into the next 4.22 release ?
another pull request ?

@weizhouapache
Copy link
Copy Markdown
Member

any chance of also getting this backported into the next 4.22 release ? another pull request ?

@agronaught
you can rebase your branch with 4.22 branch.
all 4.22 commits will be merged into main branch

@agronaught
Copy link
Copy Markdown
Author

agronaught commented May 18, 2026 via email

@agronaught agronaught force-pushed the fix-13171-v6-fw-input-established-related branch from 992fbf5 to 8dcc070 Compare May 18, 2026 10:23
@agronaught agronaught changed the base branch from main to 4.22 May 18, 2026 10:28
@agronaught
Copy link
Copy Markdown
Author

rebased to 4.22
thank you.

@weizhouapache
Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.67%. Comparing base (4a49ffa) to head (dd7e103).

Additional details and impacted files
@@             Coverage Diff              @@
##               4.22   #13173      +/-   ##
============================================
- Coverage     17.67%   17.67%   -0.01%     
+ Complexity    15788    15785       -3     
============================================
  Files          5922     5922              
  Lines        533123   533123              
  Branches      65201    65201              
============================================
- Hits          94242    94230      -12     
- Misses       428237   428248      +11     
- Partials      10644    10645       +1     
Flag Coverage Δ
uitests 3.69% <ø> (ø)
unittests 18.75% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@blueorangutan
Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 17892

Copy link
Copy Markdown
Member

@winterhazel winterhazel left a comment

Choose a reason for hiding this comment

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

@agronaught thanks for the pull request. The code looks good.

I think that we can restore the changes removed in the 2nd commit of #10970 to accept IPv6 packets from related/established connections going to the virtual router even for non-routed networks. I only removed the rule from the INPUT chain because it was not required to address the issue I was facing at the time, but can't think of a reason for not having it.

@agronaught
Copy link
Copy Markdown
Author

@agronaught thanks for the pull request. The code looks good.

I think that we can restore the changes removed in the 2nd commit of #10970 to accept IPv6 packets from related/established connections going to the virtual router even for non-routed networks. I only removed the rule from the INPUT chain because it was not required to address the issue I was facing at the time, but can't think of a reason for not having it.

Sounds fair.
should i incorporate the additional changes for the non-routed isolated networks into this patch ?

@winterhazel
Copy link
Copy Markdown
Member

@agronaught thanks for the pull request. The code looks good.
I think that we can restore the changes removed in the 2nd commit of #10970 to accept IPv6 packets from related/established connections going to the virtual router even for non-routed networks. I only removed the rule from the INPUT chain because it was not required to address the issue I was facing at the time, but can't think of a reason for not having it.

Sounds fair. should i incorporate the additional changes for the non-routed isolated networks into this patch ?

@agronaught yes, please do.

@weizhouapache
Copy link
Copy Markdown
Member

@agronaught thanks for the pull request. The code looks good.

I think that we can restore the changes removed in the 2nd commit of #10970 to accept IPv6 packets from related/established connections going to the virtual router even for non-routed networks. I only removed the rule from the INPUT chain because it was not required to address the issue I was facing at the time, but can't think of a reason for not having it.

@winterhazel @agronaught

In most cases, CloudStack VRs simply forward traffic to guest VMs and do not expose public services themselves. That is why we originally did not add the rule ct state established,related accept to the input chain.

It was my suggestion to apply the changes introduced in the 2nd commit of PR #10970. At that time, I did not consider BGP sessions as a use case.

I agree that we could revert the changes introduced here:
145f4fe#diff-105798cff64ba9131433182777feb6bfc8084e93808f736d61da95387eabc1fe

That might resolve the issue without requiring the changes proposed in this PR.

The IPv4 rules are correct. When I developed the Dynamic Routing feature, I explicitly added the rules for routed networks and routed VPCs:

        self.nft_ipv4_fw.append({'type': "", 'chain': 'INPUT',
                                 'rule': "iifname eth2 ct state related,established counter accept"})
...
        self.nft_ipv4_acl.append({'type': "", 'chain': 'INPUT',
                                  'rule': "iifname eth1 ct state related,established counter accept"})

So the issue appears to be specific to the IPv6 rules.

agronaught pushed a commit to agronaught/cloudstack that referenced this pull request May 19, 2026
…olated

Per upstream review feedback on apache#13173: drop the is_routed() guard on
fw_router_routing_v6() so non-routed Isolated v6 networks also accept
established/related return traffic to the VR.

Keep the is_vpc() guard (VPC has its own firewall path via
fw_vpcrouter_routing).

Scope stays narrow: only the established/related rules. v4's service-port
rules (tcp/3922, tcp/8080) are not mirrored into the v6 INPUT chain.

Tested on staging (4.22.0.0):

- Routed Isolated v6 (Filtered offering): BGP v6 sessions reach
  Established, eth2 established/related rule counter active
  (81 packets / 9893 bytes).
- Non-routed Isolated v6 (DualStack offering with VirtualRouter +
  SourceNat): fw_input contains lo/eth2/eth0 established/related rules
  identical to the routed case; counter activity on eth2
  (66 packets / 8369 bytes) confirms the rule is reached.

Signed-off-by: Jason Ball <jball@resetdata.com>
@agronaught
Copy link
Copy Markdown
Author

Done — pushed b89599b.

Tested on ACS 4.22.0.0 staging:

Routed Isolated v6 (IsolatedV6RoutedFiltered offering):
BGP v6 sessions reach Established with both upstream peers, tenant /64
advertised, eth2 established/related counter active (81 packets / 9893
bytes at first observation).

Non-routed Isolated v6 (DualStack offering, VirtualRouter +
SourceNat)
: fw_input contains lo/eth2/eth0 established/related rules
identical to the routed case. Counter activity on eth2 (66 packets /
8369 bytes) confirms the rule is reached by real traffic. Without this
expansion the chain would only contain the icmpv6 accept rules.

Scope kept narrow per your suggestion: only the established/related
rules, no mirror of v4's service-port rules (tcp/3922, tcp/8080).

Note: fw_input ends up with the rules duplicated (lo + eth2 appear
twice in the running chain). Same observed in v4 INPUT (3 copies of
established,related there). Looks like a pre-existing post_config_change
multi-call pattern, not introduced by this patch — happy to address
separately if you'd like.

@weizhouapache
Copy link
Copy Markdown
Member

@agronaught
to be clear, can you test the changes below ?

diff --git a/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py b/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py
index 93d0d0388ef..63d7724dd20 100755
--- a/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py
+++ b/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py
@@ -232,7 +232,7 @@ class CsNetfilters(object):
         if hook == "input" or hook == "output":
             CsHelper.execute("nft add rule %s %s %s icmpv6 type { echo-request, echo-reply, \
                 nd-neighbor-solicit, nd-router-advert, nd-neighbor-advert } accept" % (address_family, table, chain))
-        elif hook == "forward":
+        if hook == "input" or hook == "forward":
             CsHelper.execute("nft add rule %s %s %s ct state established,related accept" % (address_family, table, chain))
 
     def add_ip4_chain(self, address_family, table, chain, hook, action):

do not make other changes

@agronaught
Copy link
Copy Markdown
Author

@agronaught to be clear, can you test the changes below ?

diff --git a/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py b/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py
index 93d0d0388ef..63d7724dd20 100755
--- a/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py
+++ b/systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py
@@ -232,7 +232,7 @@ class CsNetfilters(object):
         if hook == "input" or hook == "output":
             CsHelper.execute("nft add rule %s %s %s icmpv6 type { echo-request, echo-reply, \
                 nd-neighbor-solicit, nd-router-advert, nd-neighbor-advert } accept" % (address_family, table, chain))
-        elif hook == "forward":
+        if hook == "input" or hook == "forward":
             CsHelper.execute("nft add rule %s %s %s ct state established,related accept" % (address_family, table, chain))
 
     def add_ip4_chain(self, address_family, table, chain, hook, action):

do not make other changes

missed this.
on it.

The IPv6 fw_input chain currently accepts only ICMPv6 control traffic
and policy=drop everything else. Return traffic for VR-initiated v6
connections (e.g. BGP SYN-ACKs from upstream peers) is silently
dropped.

PR apache#10970 added the equivalent rule to the v6 FORWARD chain but
removed it from INPUT in its second commit. This restores it at chain
creation, so it applies uniformly to all v6 input-hook chains
(routed Isolated, non-routed Isolated, etc.).

Tested on ACS 4.22.0.0 staging:

- Routed Isolated v6 (IsolatedV6RoutedFiltered offering): BGP v6
  sessions reach Established, tenant /64 advertised upstream.
- Non-routed Isolated v6 (DualStack with VirtualRouter + SourceNat):
  fw_input contains the established/related accept rule and accepts
  return traffic.

Resulting fw_input on both shapes:

    chain fw_input {
        type filter hook input priority filter; policy drop;
        icmpv6 type { ... } accept
        ct state established,related accept
    }

Fixes: apache#13171
Refs: apache#10970

Co-authored-by: Bryan Lima <[email protected]>

Signed-off-by: Jason Ball <jball@resetdata.com>
@agronaught agronaught force-pushed the fix-13171-v6-fw-input-established-related branch from b89599b to dd7e103 Compare May 19, 2026 07:26
@agronaught
Copy link
Copy Markdown
Author

Tested and confirmed working — replaced my two earlier commits with your diff (dd7e103).

Routed Isolated v6: BGP v6 reaches Established with both peers; PfxRcd:1, PfxSnt:2.

Non-routed Isolated v6 (DualStack offering w/ VirtualRouter + SourceNat): fw_input has the rule from chain creation; tested return traffic flow.

Resulting chain on both shapes:

chain fw_input {
    type filter hook input priority filter; policy drop;
    icmpv6 type { ... } accept
    ct state established,related accept
}

Much cleaner than my CsAddress.py approach — single rule, no duplicates from post_config_change re-runs. Thanks for the pointer to #10970's 2nd commit; my fault for not finding that during the original investigation.

@weizhouapache
Copy link
Copy Markdown
Member

Tested and confirmed working — replaced my two earlier commits with your diff (dd7e103).

Routed Isolated v6: BGP v6 reaches Established with both peers; PfxRcd:1, PfxSnt:2.

Non-routed Isolated v6 (DualStack offering w/ VirtualRouter + SourceNat): fw_input has the rule from chain creation; tested return traffic flow.

Resulting chain on both shapes:

chain fw_input {
    type filter hook input priority filter; policy drop;
    icmpv6 type { ... } accept
    ct state established,related accept
}

Much cleaner than my CsAddress.py approach — single rule, no duplicates from post_config_change re-runs. Thanks for the pointer to #10970's 2nd commit; my fault for not finding that during the original investigation.

@agronaught
thanks for the update. great to know it fixes your issue.

@winterhazel thanks for the good point.

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

code lgtm

@weizhouapache
Copy link
Copy Markdown
Member

tested ok

routed network VR

table ip6 ip6_firewall {
	chain fw_input {
		type filter hook input priority filter; policy drop;
		icmpv6 type { echo-request, echo-reply, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } accept
		ct state established,related accept
	}

routed VPC VR

table ip6 ip6_acl {
	chain acl_input {
		type filter hook input priority filter; policy drop;
		icmpv6 type { echo-request, echo-reply, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } accept
		ct state established,related accept
	}

@weizhouapache weizhouapache merged commit 3285e2f into apache:4.22 May 19, 2026
25 of 26 checks passed
@boring-cyborg
Copy link
Copy Markdown

boring-cyborg Bot commented May 19, 2026

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IPv6 ROUTED Filtered networks: VR's ip6_firewall fw_input chain missing 'ct state established,related accept' rule — IPv6 BGP cannot establish

5 participants