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

kube-proxy: Drop packets in INVALID state #74840

Merged
merged 1 commit into from Apr 26, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+11 −1
Diff settings

Always

Just for now

@@ -34,7 +34,7 @@ import (

"k8s.io/klog"

"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
utilversion "k8s.io/apimachinery/pkg/util/version"
"k8s.io/apimachinery/pkg/util/wait"
@@ -1298,6 +1298,16 @@ func (proxier *Proxier) syncProxyRules() {
}
}

// Drop the packets in INVALID state, which would potentially cause
// unexpected connection reset.
// https://github.com/kubernetes/kubernetes/issues/74839
writeLine(proxier.filterRules,
"-A", string(kubeForwardChain),

This comment has been minimized.

Copy link
@danwinship

danwinship Mar 28, 2019

Contributor

Wouldn't this need to be near the start of syncProxyRules rather than near the end? Many packets have already been matched and redirected by this point...

This comment has been minimized.

Copy link
@anfernee

anfernee Mar 28, 2019

Author Member

Yes, it's the first rule in KUBE-FORWARD chain.

Chain KUBE-FORWARD (1 references)
target     prot opt source               destination 
*** NEW RULE INSERTED HERE ***
ACCEPT     all  --  anywhere             anywhere             /* kubernetes forwarding rules */ mark match 0x4000/0x4000
ACCEPT     all  --  10.36.0.0/14         anywhere             /* kubernetes forwarding conntrack pod source rule */ ctstate RELATED,ESTABLISHED
ACCEPT     all  --  anywhere             10.36.0.0/14         /* kubernetes forwarding conntrack pod destination rule */ ctstate RELATED,ESTABLISHED

This comment has been minimized.

Copy link
@danwinship

danwinship Mar 29, 2019

Contributor

oh, all the (relevant) stuff before that is in natRules, not filterRules. ok

This comment has been minimized.

Copy link
@cmluciano

cmluciano Apr 4, 2019

Member

FYI Calico is doing similar things for disabling ConntrackInvalid projectcalico/felix#1424 .

The Calico implementation also works for a cluster that runs IPVS as a load-balancer with iptables-mode of k8s-proxy .

This comment has been minimized.

Copy link
@anfernee

anfernee Apr 5, 2019

Author Member

Ah, good to know. Thanks to bring it up @cmluciano . not sure if it should be an option here.. any reason not to enable that option?

"-m", "conntrack",
"--ctstate", "INVALID",
"-j", "DROP",
)

// If the masqueradeMark has been added then we want to forward that same
// traffic, this allows NodePort traffic to be forwarded even if the default
// FORWARD policy is not accept.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.