KEP-3178: Cleaning up IPTables Chain Ownership #3179

danwinship · 2022-01-23T20:39:14Z

One-line PR description: Initial draft of KEP-3178, to clean up kubelet-vs-kube-proxy IPTables chain ownership
Issue link: Cleaning up IPTables Chain Ownership #3178
Other comments: (I'm filing this now but targeting it at 1.25 for alpha given that it requires changes to the same parts of the iptables proxy that there are already multiple other pending KEP PRs for.)

aojea · 2022-02-04T07:05:39Z

/cc

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

thockin · 2022-02-17T00:28:39Z

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

+post.)
+
+Kube-proxy will be updated to not use `KUBE-MARK-DROP`, as described
+above. (This change is unconditional; it is _not_ feature-gated.) We


why no gate? What happens if we've done something disastrously dumb - how can a cluster back out of this change?

Since this just changes kube-proxy's internals without actually changing the visible functionality, I was considering it a cleanup/bugfix, not a feature. (So if we do something disastrously dumb, and somehow don't catch it, then affected users would have to downgrade, just like with any other major bug.)

If we feature-gate the kube-proxy change, then we'd have to push out the rest of the schedule out by two releases, because you can't have the kubelet feature gate go to beta until 2 releases after kube-proxy starts not depending on KUBE-MARK-DROP. (So we'd have two separate feature gates, a lot like the "Move KUBE-MARK-DROP to Kube-Proxy Rather Than Removing It" example under "Alternatives".)

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

thockin

LGTM except ACCEPT questions,

Thanks for this, long overdue

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

thockin · 2022-02-23T22:48:25Z

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

+ever seemed to care. Given the current state, in which only
+LoadBalancer traffic gets firewalled, the simplification from reusing
+the existing matching logic in the `filter` table seems to be
+outweighed by the complication of having to deal with


I'm 50/50 - having MARK-DROP in the nat chain puts all the logic in one place, which is nice. That said, I buy the REJECT consistency below

aojea · 2022-02-24T09:21:03Z

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

+of the chain, it calls `KUBE-MARK-DROP` on any unmatched packets.
+
+But there is no reason why the `FW` chain could not simply be moved to
+the `filter` table (with calls to `-j ACCEPT` for allowed source


currently, KUBE-SERVICES is called from PREROUTING and OUTPUT ... these new filtering rules has to go to the filter tables on INPUT, FORWARDING and OUTPUT , right?

meh, nat PREROUTING happens before anything in filter. One of these days I'll remember how iptables ordering works without having to google it...

@thockin didn't you have some kube-proxy iptables-processing-order infographic somewhere?

I have this printed

🤔 we miss the service IP on the filter tables, DNAT was already performed

Anyway, this worked right in the original proposal (keep the FW chain in NAT, fall through rather than KUBE-MARK-DROP at the end, have the KUBE-FIREWALL chain in filter drop any not-yet-DNAT-ed packets addressed to the LB IP). I changed it in part because of Tim's comment here but (a) as long as we keep doing the pod-to-LB-IP short-circuiting, the original proposal works, and (b) if we stop doing the short-circuiting in the future, we'd have to rewrite all the rules anyway, and we can just address this then.

aojea · 2022-02-24T09:26:03Z

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

+  - `KUBE-MARK-DROP` and `KUBE-FIREWALL`
+
+    `KUBE-MARK-DROP` marks packets as needing to be dropped by setting
+    a _different_ configurable bit on the packet mark. `KUBE-FIREWALL`


KUBE-FIREWALL was also extended to deal with the route_localnet CVE
kubernetes/kubernetes#91569

Yeah, forgot about that.

So this is its own mess: 91569 only talks about NodePort, but the problem theoretically applies to HostPorts too. Except, it only applies if you set route_localnet, which kube-proxy does and kubelet doesn't... so for our own purposes we can move 91569's rule from kubelet to kube-proxy. (In fact, it should have been there originally, but KUBE-FIREWALL sounds like a good place to put it, and that's created from kubelet, so it got put there.)

But given that it was previously in kubelet, there are probably alternate proxies and hostport implementations that would be vulnerable if we didn't have this rule... so we may have to keep creating it...

We should probably have a KEP about eventually getting rid of localhost NodePort/HostPort behavior...

aojea · 2022-02-24T10:11:14Z

+1 but I suggest to postpone one release waiting for the dockershim feedback, it seems is causing more drama than expected and adding more changes on top can complicate it more

danwinship · 2022-02-24T13:44:01Z

+1 but I suggest to postpone one release waiting for the dockershim feedback, it seems is causing more drama than expected and adding more changes on top can complicate it more

Current proposal in kep.yaml is alpha in 1.25, and we can decide in the 1.25 timeframe if that still makes sense. But we should merge soon so external components know what's coming.

aojea · 2022-02-24T13:53:55Z

+1 but I suggest to postpone one release waiting for the dockershim feedback, it seems is causing more drama than expected and adding more changes on top can complicate it more

Current proposal in kep.yaml is alpha in 1.25, and we can decide in the 1.25 timeframe if that still makes sense. But we should merge soon so external components know what's coming.

I wanted to mean 1.25, I missed that detail
lgm

danwinship · 2022-02-24T14:18:14Z

Updated:

squashed, removed UNRESOLVED section
reverted back to original LoadBalancerServices proposal, which fixes the iptables-ordering problem and resolves Tim's objection to splitting up the service logic between nat and filter.
explained the martian packet problem and clarified that we should probably have kubelet keep creating the rule protecting against that, even though it's not kubelet's problem 🙁

I wanted to mean 1.25, I missed that detail

yeah, I didn't want to try to stack another iptables proxier change on top of what's already in 1.24...

danwinship · 2022-03-27T15:30:52Z

@thockin can we merge this and then merge kubernetes/kubernetes#109059 to add the KUBE-IPTABLES-HINT chain for 1.24?

You had previously "mostly LGTM"ed it, and I added some "UNRESOLVED" sections making it explicit that the low-level details are still subject to change. But I'd like to get this as the plan of record so that we can (1) add KUBE-IPTABLES-HINT in 1.24, and (2) do a blog post about the upcoming iptables rules reorg and what third-party components need to know (which I've already started writing).

squeed · 2022-04-12T15:20:25Z

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

+kube-proxy. But because they are generally doing much less IPTables
+processing than kube-proxy does, they could fairly easily be rewritten
+to not use the packet mark, and just have slightly-redundant
+`PREROUTING` and `POSTROUTING` rules instead.


Yeah, we can fix CNI to not use a mark, though it will have a lot of duplicated logic.

More relevantly, once this goes out, I can reference the deprecation in whatever CNI release notes make sense. We only added that option so that users could save mark bits.

squeed · 2022-04-12T15:39:22Z

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

+be the one creating the fix for it as well, and we should make
+kube-proxy create this filtering rule itself.
+
+However, it is possible that users of other proxy implementations (or


The CNI portmap plugin is also in this situation -- it enables route_localnet as well.

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

thockin

Thanks!

/lgtm
/approve

danwinship · 2022-05-28T16:53:18Z

/assign @johnbelamaric
for PRR approval
(and can you /lgtm too because I just had to re-push with the prod-readiness file filled in, but it's otherwise unchanged from the version Tim lgtm'ed)

aojea · 2022-06-02T11:23:15Z

/lgtm

johnbelamaric

/approve

It's OK as is but a few more hints for users to know if this is causing issues would be nice.

johnbelamaric · 2022-06-09T20:55:59Z

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

+
+###### What specific metrics should inform a rollback?
+
+Any failures would be the result of third-party components being


I understand that we can't predict what some other tool will do. But perhaps we could give some hints as to where folks could look. For example, what components (CNI, netpol implementation, etc.) are likely to have potential errors or failures?

johnbelamaric · 2022-06-09T20:57:11Z

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

+There is no simple way to do this because if the feature is working
+correctly there will be no difference in externally-visible behavior.
+(The generated iptables rules will be different, but the _effect_ of
+the generated iptables rules will be the same.)


This feature isn't really "used by workloads" so much as it is "configured by the administrator", so you can say something that effect.

k8s-ci-robot · 2022-06-09T20:59:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, johnbelamaric, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/prod-readiness/OWNERS~~ [johnbelamaric]
~~keps/sig-network/OWNERS~~ [johnbelamaric,thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

johnbelamaric · 2022-06-09T21:02:23Z

by the way, I don't see this on the enhancements spreadsheet for 1.25, please add it there.

danwinship · 2022-06-10T21:40:45Z

/retest

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 23, 2022

k8s-ci-robot requested review from caseydavenport and thockin January 23, 2022 20:39

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. labels Jan 23, 2022

danwinship mentioned this pull request Jan 27, 2022

Cleaning up IPTables Chain Ownership #3178

Closed

15 tasks

k8s-ci-robot requested a review from aojea February 4, 2022 07:05

thockin reviewed Feb 17, 2022

View reviewed changes

thockin reviewed Feb 23, 2022

View reviewed changes

aojea reviewed Feb 24, 2022

View reviewed changes

danwinship force-pushed the iptables-cleanup branch from a8a130e to 14c5ba3 Compare February 24, 2022 14:15

danwinship force-pushed the iptables-cleanup branch from 14c5ba3 to 18edf22 Compare February 24, 2022 14:19

danwinship force-pushed the iptables-cleanup branch from 18edf22 to 8add266 Compare March 27, 2022 15:22

danwinship mentioned this pull request Mar 27, 2022

Create a KUBE-IPTABLES-HINT chain kubernetes/kubernetes#109059

Merged

github-actions bot mentioned this pull request Mar 29, 2022

Week Ending March 27, 2022 dev-obs/actus#407

Open

danwinship mentioned this pull request Apr 11, 2022

update for KUBE-IPTABLES-HINT (and other 2022-ness) kubernetes-sigs/iptables-wrappers#3

Merged

squeed reviewed Apr 12, 2022

View reviewed changes

danwinship force-pushed the iptables-cleanup branch from 8add266 to 7e7e93e Compare April 29, 2022 13:55

danwinship force-pushed the iptables-cleanup branch from 7e7e93e to cb1e576 Compare May 6, 2022 14:24

thockin reviewed May 6, 2022

View reviewed changes

keps/sig-network/3178-iptables-cleanup/README.md Outdated Show resolved Hide resolved

keps/sig-network/3178-iptables-cleanup/README.md Outdated Show resolved Hide resolved

keps/sig-network/3178-iptables-cleanup/README.md Outdated Show resolved Hide resolved

danwinship force-pushed the iptables-cleanup branch from cb1e576 to f8b4f47 Compare May 9, 2022 16:40

thockin reviewed May 28, 2022

View reviewed changes

k8s-ci-robot assigned thockin May 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2022

Initial draft of iptables cleanup KEP

610f9b5

danwinship force-pushed the iptables-cleanup branch from f8b4f47 to 610f9b5 Compare May 28, 2022 16:52

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2022

k8s-ci-robot assigned johnbelamaric May 28, 2022

k8s-ci-robot assigned aojea Jun 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2022

johnbelamaric approved these changes Jun 9, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2022

k8s-ci-robot merged commit 27ddd8f into kubernetes:master Jun 10, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 10, 2022

danwinship deleted the iptables-cleanup branch June 13, 2022 14:36

danwinship mentioned this pull request Dec 13, 2022

KEP 3178: update for beta #3690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3178: Cleaning up IPTables Chain Ownership #3179

KEP-3178: Cleaning up IPTables Chain Ownership #3179

danwinship commented Jan 23, 2022

aojea commented Feb 4, 2022

thockin Feb 17, 2022

danwinship Feb 17, 2022 •

edited

Loading

thockin left a comment

thockin Feb 23, 2022

aojea Feb 24, 2022

danwinship Feb 24, 2022

danwinship Feb 24, 2022

aojea Feb 24, 2022 •

edited

Loading

aojea Feb 24, 2022

danwinship Feb 24, 2022

aojea Feb 24, 2022

danwinship Feb 24, 2022

aojea commented Feb 24, 2022

danwinship commented Feb 24, 2022

aojea commented Feb 24, 2022

danwinship commented Feb 24, 2022

danwinship commented Mar 27, 2022

squeed Apr 12, 2022

squeed Apr 12, 2022

squeed Apr 12, 2022

thockin left a comment

danwinship commented May 28, 2022

aojea commented Jun 2, 2022

johnbelamaric left a comment

johnbelamaric Jun 9, 2022

johnbelamaric Jun 9, 2022

k8s-ci-robot commented Jun 9, 2022

johnbelamaric commented Jun 9, 2022

danwinship commented Jun 10, 2022


		###### What specific metrics should inform a rollback?

		Any failures would be the result of third-party components being

KEP-3178: Cleaning up IPTables Chain Ownership #3179

KEP-3178: Cleaning up IPTables Chain Ownership #3179

Conversation

danwinship commented Jan 23, 2022

aojea commented Feb 4, 2022

Choose a reason for hiding this comment

danwinship Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 24, 2022

danwinship commented Feb 24, 2022

aojea commented Feb 24, 2022

danwinship commented Feb 24, 2022

danwinship commented Mar 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

danwinship commented May 28, 2022

aojea commented Jun 2, 2022

johnbelamaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 9, 2022

johnbelamaric commented Jun 9, 2022

danwinship commented Jun 10, 2022

danwinship Feb 17, 2022 •

edited

Loading

aojea Feb 24, 2022 •

edited

Loading