Sync iptables rules with an event-driven loop - #8215
Open
hongliangl wants to merge 3 commits into
Open
Conversation
hongliangl
force-pushed
the
unified-event-driven-iptables-sync
branch
3 times, most recently
from
July 28, 2026 03:29
3814b46 to
8e33873
Compare
hongliangl
changed the title
hongliangl
changed the title
hongliangl
force-pushed
the
unified-event-driven-iptables-sync
branch
from
July 28, 2026 06:36
8e33873 to
be8e561
Compare
hongliangl
changed the title
hongliangl
marked this pull request as ready for review
July 28, 2026 06:37
hongliangl
force-pushed
the
unified-event-driven-iptables-sync
branch
from
August 18, 2026 02:34
be8e561 to
15c37bc
Compare
iptables rules were managed with two different mechanisms: most features only updated their in-memory caches and waited for the 60s periodic sync to apply them with iptables-restore, while Egress installed and removed its SNAT rules directly with iptables -I / -D. The periodic sync rewrites the whole state from the caches, so it could overwrite a SNAT rule inserted after its snapshot of the caches was taken, leaving the traffic unSNATed until the next sync, up to 60s later. The iptables rules are now synced by a dedicated loop, fed by a queue which a feature notifies when it has updated the caches, and which syncNetworkConfig notifies on its period. Egress only updates the caches and notifies the queue instead of writing to the datapath itself, which removes the race without having to serialize its updates with the sync. AddOrUpdateNodeNetworkPolicyIPTables keeps writing the rules itself, as its callers rely on them being enforced when it returns, but it now updates the caches before writing and notifies the queue after, so that a sync holding an older snapshot cannot restore the previous rules over them. The notification is delayed by 100ms and the queue only holds one item, so that updates occurring in quick succession are coalesced into a single sync. Any feature updating the caches can now have its rules applied about 100ms later, instead of waiting for the next periodic sync. A failed sync is requeued with an exponential backoff, as the notification which caused it was consumed. syncNetworkConfig notifies the queue after syncing the ipsets, keeping the existing ordering: iptables rules reference ipsets, and iptables-restore fails as a whole if one of them is missing. AddSNATRule and DeleteSNATRule become asynchronous: the rules are applied about 100ms after the calls return. snatRuleSpec is removed, as the SNAT rules are now only built by restoreIptablesData. Signed-off-by: Hongliang Liu <hongliang.liu@broadcom.com>
hongliangl
force-pushed
the
unified-event-driven-iptables-sync
branch
from
August 18, 2026 05:22
15c37bc to
b6cbb2e
Compare
syncIPTables only rewrites the chains the caches know about, so a chain deleted while a sync was holding an older snapshot is recreated by that sync, and nothing removes it afterwards: the rules of a deleted policy stay in the datapath until the Agent restarts. The periodic sync now lists the chains of the filter table and deletes the ones which are not in the caches any more. The datapath is listed before the caches are read, which is what makes it safe: the rules of a chain are stored in the caches before the chain is written to the datapath, so a chain which appears in the listing was already in the caches when the listing was taken, and is not mistaken for a leftover. Signed-off-by: Hongliang Liu <hongliang.liu@broadcom.com>
hongliangl
force-pushed
the
unified-event-driven-iptables-sync
branch
from
August 18, 2026 06:31
b6cbb2e to
a869b8d
Compare
cleanupOrphanNodeNetworkPolicyChains compares the chains in the datapath
with the chains in the caches, and deletes the ones which are not cached
any more. The caches and the datapath are written by several goroutines,
and never in the same operation, so at any instant a chain can
legitimately be in one and not in the other. Deciding on a single
observation therefore risks deleting a chain which is about to be
cached.
A chain is now only deleted when it was already a candidate in the
previous run, which means it has been missing from the caches for a
whole period rather than at one instant. The candidates of the previous
run are kept in a new field, which is only accessed by syncNetworkConfig
and therefore needs no lock.
The scenarios below use ING for ANTREA-POL-INGRESS-RULES, which is the
chain holding the jump rules, and X for the chain of one policy. What
matters about the datapath is whether X exists and whether anything
jumps to it.
Agent restart, with the NodeNetworkPolicy controller realizing the
policies within one period:
1. The previous process left X in the datapath, and ING jumps to it.
2. initNodeNetworkPolicy seeds ING with an empty rule list, so the
initial sync flushes ING and nothing jumps to X any more.
3. First run. X is in the datapath and not in the caches, so it
becomes a candidate, but it is not deleted.
4. The controller realizes the policy, which caches X and makes ING
jump to it again.
5. Second run. X is cached, so it is skipped.
The same restart, but the controller takes longer than one period:
3. First run. X becomes a candidate.
4. Second run. X is still not cached, so it is deleted. Nothing jumps
to it at that point, which means the policy was not enforced
anyway, and the controller installs it again when it gets there.
The only cost is the work done twice.
A run which reads the caches while a policy is being added. The rules of
a policy are cached one chain at a time and written to the datapath in a
single restore, so the caches are incomplete for a while:
1. X1 is cached, X2 is not, and neither is in the datapath yet.
2. A run lists the chains of the datapath and sees neither of them, so
it does not look them up in the caches at all. The incomplete state
of the caches is invisible to it.
3. X2 is cached, and the restore writes both chains to the datapath.
4. The next run finds both of them cached.
A writer which updates the datapath before the caches, which is what the
callers did before this series. The grace period is what covers this
case:
1. The restore creates X, which is not cached yet. The window lasts
microseconds.
2. A run lands in that window and makes X a candidate, without
deleting it.
3. X is cached, and the next run skips it.
The case the cleanup exists for, which is a sync writing back a snapshot
taken before a policy was deleted:
1. A sync reads a snapshot holding X and the jump rule of ING.
2. The policy is deleted, so X is removed from the caches and from the
datapath, and ING is emptied in the caches.
3. The sync writes its snapshot, which brings X and the jump rule
back. Nothing will touch X after this.
4. First run. X becomes a candidate. Deleting it here would fail
anyway, because ING still jumps to it.
5. The sync triggered by the deletion empties ING in the datapath.
6. Second run. X is deleted.
A run which lands in the middle of a deletion. The chains are deleted
from the datapath before they are removed from the caches:
1. If the chain is already gone from the datapath, the run does not
list it and ignores it, whatever the caches hold.
2. If deleting it from the datapath failed, the caller returns without
touching the caches, so the run finds it cached and skips it.
A run which fails to list the chains returns before it replaces the
candidates of the previous run, so a failure lengthens the grace period
instead of shortening it.
Deleting a chain which is still needed would require it to be missing
from the caches in two runs a period apart. On top of that, iptables
refuses to delete a chain which is still referenced, so a chain is only
ever deleted once nothing jumps to it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: #8285
iptables rules were managed with two different mechanisms: most features
only updated their in-memory caches and waited for the 60s periodic sync
to apply them with iptables-restore, while Egress installed and removed
its SNAT rules directly with iptables -I / -D. The periodic sync rewrites
the whole state from the caches, so it could overwrite a SNAT rule
inserted after its snapshot of the caches was taken, leaving the traffic
unSNATed until the next sync, up to 60s later.
The iptables rules are now synced by a dedicated loop, which syncs when a
feature notifies it that the caches have been updated, and when
syncNetworkConfig notifies it on its period. Egress only updates the
caches and notifies the loop instead of writing to the datapath itself,
which removes the race without having to serialize its updates with the
sync.
The notification channel has a buffer of 1 and the loop waits for 100ms
before syncing, so that updates occurring in quick succession are batched
into a single call. Any feature updating the caches can now have its
rules applied about 100ms later, instead of waiting for the next periodic
sync.
syncNetworkConfig notifies the loop after syncing the ipsets, keeping the
existing ordering: iptables rules reference ipsets, and iptables-restore
fails as a whole if one of them is missing.
AddSNATRule and DeleteSNATRule become asynchronous: the rules are applied
about 100ms after the calls return. snatRuleSpec is removed, as the SNAT
rules are now only built by restoreIptablesData.
Signed-off-by: Hongliang Liu hongliang.liu@broadcom.com