Skip to content

Commit 41d4e94

Browse files
committed
Update 3178-iptables-cleanup for alpha
1 parent 949bd7d commit 41d4e94

File tree

2 files changed

+58
-13
lines changed

2 files changed

+58
-13
lines changed

keps/sig-network/3178-iptables-cleanup/README.md

Lines changed: 57 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@
2222
- [GA + 2](#ga--2)
2323
- [Indeterminate Future](#indeterminate-future)
2424
- [Test Plan](#test-plan)
25+
- [Prerequisite testing updates](#prerequisite-testing-updates)
26+
- [Unit tests](#unit-tests)
27+
- [Integration tests](#integration-tests)
28+
- [e2e tests](#e2e-tests)
2529
- [Graduation Criteria](#graduation-criteria)
2630
- [Alpha](#alpha)
2731
- [Beta](#beta)
@@ -399,23 +403,64 @@ Kubelet, but there is no specific plan for this at this time.
399403

400404
### Test Plan
401405

406+
[X] I/we understand the owners of the involved components may require updates to
407+
existing tests to make this code solid enough prior to committing the changes necessary
408+
to implement this enhancement.
409+
410+
##### Prerequisite testing updates
411+
402412
We discovered a while back that our existing e2e tests do not properly
403-
test the cases that are expected to result in dropped packets
404-
([kubernetes #85572]). Attempting to fix this resulted in the
405-
discovery that [there is not any easy way to test these rules]; in any
406-
of the scenarios we can easily create in our e2e environment, it is
407-
either impossible to hit kube-proxy's "drop" rules, or else the
408-
connection would end up getting dropped for other reasons even if
409-
kube-proxy failed to drop it.
410-
411-
Thus, the new code (like the existing code), will primarily be tested
412-
by the unit tests in `pkg/proxy/iptables/proxier_test.go`, not by e2e
413-
tests. We will need to extend those tests to test the functionality
414-
both with the feature gate disabled and enabled.
413+
test the cases that are expected to result in dropped packets. (The
414+
tests still pass even when we don't drop the packets: [kubernetes
415+
#85572]). However, attempting to fix this resulted in the discovery
416+
that [there is not any easy way to test these rules]. In the
417+
`LoadBalancerSourceRanges` case, the drop rule will never get hit on
418+
GCP (unless there is a bug in the GCP CloudProvider or the cloud load
419+
balancers). (The drop rule _can_ get hit in a bare-metal environment
420+
when using a third-party load balancer like MetalLB, but we have no
421+
way of testing this in Kubernetes CI). In the traffic policy case, the
422+
drop rule is needed during periods when kube-proxy and the cloud load
423+
balancers are out of sync, but there is no good way to reliably
424+
trigger this race condition for e2e testing purposes.
425+
426+
However, we can manually test the new rules (eg, by killing kube-proxy
427+
before updating a service to ensure that kube-proxy and the cloud load
428+
balancer will remain out of sync), and then once we are satisfied that
429+
the rules do what we expect them to do, we can use the unit tests to
430+
ensure that we continue to generate the same (or functionally
431+
equivalent) rules in the future.
415432

416433
[kubernetes #85572]: https://github.com/kubernetes/kubernetes/issues/85572
417434
[there is not any easy way to test these rules]: https://github.com/kubernetes/kubernetes/issues/85572#issuecomment-1031733890
418435

436+
##### Unit tests
437+
438+
The unit tests in `pkg/proxy/iptables/proxier_test.go` ensure that we
439+
are generating the iptables rules that we expect to, and the [new
440+
tests already added in 1.25] allow us to assert specifically that
441+
particular packets would behave in particular ways.
442+
443+
Thus, for example, although we can't reproduce the race conditions
444+
mentioned above in an e2e environment, we can at least confirm that if
445+
a packet arrived on a node which it shouldn't have because of this
446+
race condition, that the iptables rules we generate would [route it to
447+
a `DROP` rule], rather than delivering or rejecting it.
448+
449+
- `pkg/proxy/iptables`: `06-21` - `65.1%`
450+
451+
[new tests already added in 1.25]: https://github.com/kubernetes/kubernetes/pull/107471
452+
[route it to a `DROP` rule]: https://github.com/kubernetes/kubernetes/blob/v1.25.0-alpha.1/pkg/proxy/iptables/proxier_test.go#L5974
453+
454+
##### Integration tests
455+
456+
There are no existing integration tests of the proxy code and no plans
457+
to add any.
458+
459+
##### e2e tests
460+
461+
As discussed above, it is not possible to test this functionality via
462+
e2e tests in our CI environment.
463+
419464
### Graduation Criteria
420465

421466
#### Alpha

keps/sig-network/3178-iptables-cleanup/kep.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ authors:
55
owning-sig: sig-network
66
participating-sigs:
77
- sig-node
8-
status: provisional
8+
status: implementable
99
creation-date: 2022-01-23
1010
reviewers:
1111
- "@thockin"

0 commit comments

Comments
 (0)