And use the fake interface in the unit tests, removing the dependency
on setting up FakeExec stuff when conntrack cleanup will be invoked.
Also, remove the isIPv6 argument to CleanStaleEntries, because it can
be inferred from the other args.
The iptables and nftables proxy backends had 2 unit tests
(TestDeleteEndpointConnections and TestProxierDeleteNodePortStaleUDP)
that were effectively testing that:
- If the proxy saw various Service/EndpointSlice events this would
result in specific changes to the service/endpoints trackers, AND
- If the service/endpoints trackers changed in those specific ways
this would result in specific UpdateServiceMapResult and
UpdateEndpointsMapResult values being generated, AND
- If you passed those specific UpdateServiceMapResult and
UpdateEndpointsMapResult values to conntrack.CleanStaleEntries it
would make specific calls to the lower-level conntrack methods,
AND
- If you called the lower-level conntrack methods with those
specific arguments, it would result in specific executions of the
conntrack binary, mixed with a specific number of klog
invocations.
This... is not a good unit test. We already test the change tracker
behavior in other unit tests, and we already tested the
Update{Service,Endpoints}MapResult behavior in the pkg/proxy unit
tests, and we already tested the conntrack exec behavior in
pkg/proxy/conntrack/conntrack_test.go, and we now test the
CleanStaleEntries behavior in pkg/proxy/conntrack/cleanup_test.go. So
there is no need to try to test the top-to-bottom behavior as a "unit
test".
Add an interface between CleanStaleEntries and the lower-level
conntrack helpers (ClearEntriesForIP, etc), and a fake implementation
of that interface, so that we can explicitly test CleanStaleEntries's
logic.
Remove some comments from conntrack.go that were explaining the
functions' callers rather than explaining the functions themselves
(and which were redundant with other comments in the callers anyway).
Fix the test names to match the functions they are testing.
Abstract out the repetitive FakeExec handling.
Explicitly specify the "expectCommand" in each one, to make it clearer
that that's really the part that we're testing.
For everything except TestExec(), test each case with both a "success"
result and a "nothing to delete" result from the conntrack binary.
Previously, the firewall-check chain was run in input, forward, and
output hook but not prerouting hook. When the LoadBalancer traffic
arrived at input or forward hook, it had been DNATed to endpoint IP and
port, so the firewall-check chain didn't take effect, traffic from out
of LoadBalancerSourceRanges was not dropped.
It was not detected by unit test because the chains were sorted by
priority only, while hook should be taken into consideration.
The commit links the firewall-check chain to prerouting hook and unlinks
it from input and forward hook to ensure the traffic is filtered before
DNAT. The priorities of filter chains are updated from "DNATPriority-1"
to "DNATPriority-10" to allow third parties to insert something else
between them.
Signed-off-by: Quan Tian <qtian@vmware.com>
The nftables implementation made use of concatenation of ranges when
creating the set "firewall-allow", but the support was not available
before kernel 5.6. Therefore, nftables mode couldn't run on earlier
kernels, while 5.4 is still widely used.
An alternative of concatenation of ranges is to create a separate
firewall chain for every service port that needs firewalling, and jump
to the service's firewall chain from the common firewall chain via a
rule with vmap.
Renaming from "firewall" to "firewall-ips" is required when changing the
set to the map to support existing clusters to upgrade, otherwise it
would fail to create the map. Besides, "firewall-ips" corresponds to the
"service-ips" map, later we can add use "firewall-nodeports" if it's
determined that NodePort traffic should be subject to
LoadBalancerSourceRanges.
Signed-off-by: Quan Tian <qtian@vmware.com>
In some cases a chain could change from stale to active, but once it's
added to staleChains it would always be deleted once. When the proxier
tries to delete a previously stale but currently active chain, it would
fail and lead to errors, though it won't cause real problem thanks to
kernel's validation.
The commit removes a chain from staleChains if it becomes active.
Signed-off-by: Quan Tian <qtian@vmware.com>
