Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need tests of KUBE-MARK-DROP #85572

Closed
danwinship opened this issue Nov 23, 2019 · 33 comments
Closed

Need tests of KUBE-MARK-DROP #85572

danwinship opened this issue Nov 23, 2019 · 33 comments
Assignees
Labels
area/kube-proxy kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/network Categorizes an issue or PR as relevant to SIG Network. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@danwinship
Copy link
Contributor

We have apparently been accidentally deleting the

-t nat -A KUBE-MARK-DROP -j MARK --set-xmark 0xXXXX

rule for a few weeks (#85527), and no one noticed.

I suspect this is because KUBE-MARK-DROP is really only needed if the host accepts all incoming packets by default, and if you have any sort of plausible firewall, then KUBE-MARK-DROP is redundant, and so therefore the e2e tests that might otherwise catch KUBE-MARK-DROP failures don't actually catch them.

The iptables proxier uses KUBE-MARK-DROP in two cases, on cloud platforms where we create iptables rules for LoadBalancer IPs (eg, GCE but not AWS), when a service has a load balancer IP, and it has endpoints, and a packet arrives on the node whose destination is the load-balancer IP:

  1. If the service specifies spec.loadBalancerSourceRanges, and the packet's source IP is not in the source ranges, then we call KUBE-MARK-DROP on the packet to drop it later.
    • This is theoretically tested by "It should only allow access from service loadbalancer source ranges". However, if the KUBE-MARK-DROP rule becomes a no-op, then the pod-to-LoadBalancer-IP connection will fall through the firewall chain, never hit the XLB chain, and eventually just get masqueraded and delivered to the LoadBalancer IP like it would for any other cluster-external IP. Since the cloud loadbalancer is also programmed with the source ranges, and the source range in this test is a single pod IP, the load balancer will then reject the packet (since it has the node's IP as its source at this point).
    • I think we can fix this test to fail in the absence of the drop rule by adding the node's IP to the source range. Then the expected-to-fail connection would (erroneously) not get dropped by the node, get passed to the cloud load balancer, which would accept it, and then get passed back to the service, causing the test case to fail.
  2. If the service has ServiceExternalTrafficPolicyTypeLocal and no local endpoints then we call KUBE-MARK-DROP on the packet to drop it later.
    • This does not get tested by "It should only target nodes with endpoints", because if the load balancers are working correctly then they won't send any traffic to the nodes that are creating the drop rules anyway.
    • It also does not get tested by "It should work from pods" because a pod-to-LoadBalancer-IP connection will be rewritten to be pod-to-ClusterIP before the only-local check and bypass the drop rule.
    • I think it should be possible to test this by trying to connect to an only-local LoadBalancer service from a hostNetwork pod on a node that has no endpoints for the service. The drop rule ought to cause that connection to fail, but if the drop rule was missing then it would make a connection directly to the LoadBalancer and then succeed.

The ipvs proxier refers to the KUBE-MARK-DROP chain, but I think it doesn't actually use it...

/sig network
/priority important-soon

@danwinship danwinship added the kind/bug Categorizes issue or PR as related to a bug. label Nov 23, 2019
@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 23, 2019
@athenabot
Copy link

/triage unresolved

Comment /remove-triage unresolved when the issue is assessed and confirmed.

🤖 I am a bot run by vllry. 👩‍🔬

@k8s-ci-robot k8s-ci-robot added the triage/unresolved Indicates an issue that can not or will not be resolved. label Nov 23, 2019
@danwinship
Copy link
Contributor Author

/remove-triage unresolved

@k8s-ci-robot k8s-ci-robot removed the triage/unresolved Indicates an issue that can not or will not be resolved. label Jan 9, 2020
@danwinship
Copy link
Contributor Author

/assign @robscott

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 8, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 8, 2020
@robscott
Copy link
Member

robscott commented May 9, 2020

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 9, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2020
@robscott
Copy link
Member

robscott commented Aug 7, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 5, 2020
@robscott
Copy link
Member

robscott commented Nov 5, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 5, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 3, 2021
@aojea
Copy link
Member

aojea commented Feb 3, 2021

/remove-lifecycle stale
/help wanted

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 3, 2021
@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 12, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 11, 2022
@danwinship
Copy link
Contributor Author

danwinship commented Feb 7, 2022

The iptables proxier uses KUBE-MARK-DROP in two cases, on cloud platforms where we create iptables rules for LoadBalancer IPs (eg, GCE but not AWS), when a service has a load balancer IP, and it has endpoints, and a packet arrives on the node whose destination is the load-balancer IP:

  1. If the service specifies spec.loadBalancerSourceRanges, and the packet's source IP is not in the source ranges, then we call KUBE-MARK-DROP on the packet to drop it later.
    • This is theoretically tested by "It should only allow access from service loadbalancer source ranges". However, if the KUBE-MARK-DROP rule becomes a no-op, then the pod-to-LoadBalancer-IP connection will fall through the firewall chain, never hit the XLB chain, and eventually just get masqueraded and delivered to the LoadBalancer IP like it would for any other cluster-external IP. Since the cloud loadbalancer is also programmed with the source ranges, and the source range in this test is a single pod IP, the load balancer will then reject the packet (since it has the node's IP as its source at this point).
    • I think we can fix this test to fail in the absence of the drop rule by adding the node's IP to the source range. Then the expected-to-fail connection would (erroneously) not get dropped by the node, get passed to the cloud load balancer, which would accept it, and then get passed back to the service, causing the test case to fail.

That was wrong; kube-proxy adds rules to redirect pod-to-LB and node-to-LB traffic directly to the service IP, so traffic from within the cluster will never hit the actual LB regardless of whether there is a DROP rule. (And traffic from outside the cluster is never expected to hit a node with no endpoints.)

  1. If the service has ServiceExternalTrafficPolicyTypeLocal and no local endpoints then we call KUBE-MARK-DROP on the packet to drop it later.
    • This does not get tested by "It should only target nodes with endpoints", because if the load balancers are working correctly then they won't send any traffic to the nodes that are creating the drop rules anyway.
    • It also does not get tested by "It should work from pods" because a pod-to-LoadBalancer-IP connection will be rewritten to be pod-to-ClusterIP before the only-local check and bypass the drop rule.
    • I think it should be possible to test this by trying to connect to an only-local LoadBalancer service from a hostNetwork pod on a node that has no endpoints for the service. The drop rule ought to cause that connection to fail, but if the drop rule was missing then it would make a connection directly to the LoadBalancer and then succeed.

This was also wrong, because, as above, we also rewrite node-to-LB traffic (a few lines below the pod-to-LB rewrite linked above).

For my next trick, I tried connecting from Node A to the service's NodePort on Node B, where Node B has no endpoints; this then gets forwarded to the XLB chain, but doesn't hit the "node-to-LB" rewrite rule because it's not from the local node. But the traffic still seems to get dropped, even when there is no DROP rule, presumably because the node isn't set up to route traffic that comes from off-node to another off-node IP.

So there doesn't seem to be any easy way to test that the KUBE-MARK-DROP rules are working.

However, once we implement KEP-3178, then all of the drop-related rules would be in kube-proxy itself rather than some being in kube-proxy and some being in kubelet. And then, either the existing proxier_test.go tests or the new ones in #107471 would noticed if something happened that caused us to stop hitting -j DROP.

@danwinship
Copy link
Contributor Author

/remove-lifecycle rotten
/priority backlog
/assign

Kube-proxy cleanup automation moved this from Untriaged to Done/Closed Feb 7, 2022
@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Feb 7, 2022
@danwinship danwinship reopened this Feb 7, 2022
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 7, 2022
@danwinship
Copy link
Contributor Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 7, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 14, 2022
@k8s-triage-robot
Copy link

The issue has been marked as an important bug and triaged.
Such issues are automatically marked as frozen when hitting the rotten state
to avoid missing important bugs.

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Jun 14, 2022
@danwinship
Copy link
Contributor Author

Fixed by getting rid of KUBE-MARK-DROP. (Well, the chain still exists in 1.25, but kube-proxy no longer uses it.)

@aojea
Copy link
Member

aojea commented Aug 3, 2022

Fixed by getting rid of KUBE-MARK-DROP. (Well, the chain still exists in 1.25, but kube-proxy no longer uses it.)

I've found that the spanish translation of the saying "muerto el perro se acabó la rabia" is "Dead dogs don't bite" 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kube-proxy kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/network Categorizes an issue or PR as relevant to SIG Network. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging a pull request may close this issue.