KUBE-NODE-PORTS might make more sense here actually because healthCheckNodePort is technically still a "node port"

Yeah originally there's no KUBE-NODE-PORT chain in the filter table for both iptables and ipvs versions of the kube-proxy. Just FYI that this change will add the node port chain to the filter table.

You shouldn't need to add this; inbound connections already eventually pass through kubeNodePortsChain via the "tail-call" created at the end of syncProxyRules.

Is that still true when the default INPUT chain policy is DROP?

I thought it would work without this rule in my early attempts but turned out it didn't. The fact is for NodePort services, the traffic was DNATted in nat table, basically in this order:

-A PREROUTING -m comment --comment "kubernetes service portals" -j KUBE-SERVICES
-A KUBE-SERVICES -d 10.102.149.66/32 -p tcp -m comment --comment "default/nginx cluster IP" -m tcp --dport 8080 -j KUBE-SVC-2CMXP7HKUVJN7L6M
-A KUBE-SVC-2CMXP7HKUVJN7L6M -m comment --comment "default/nginx" -j KUBE-SEP-V6JOQ423IM4BM4HR
-A KUBE-SEP-V6JOQ423IM4BM4HR -s 100.96.0.11/32 -m comment --comment "default/nginx" -j KUBE-MARK-MASQ
-A KUBE-SEP-V6JOQ423IM4BM4HR -p tcp -m comment --comment "default/nginx" -m tcp -j DNAT --to-destination 100.96.0.11:80

In the rules above, 100.96.0.11 is the nginx pod IP.
In filter table, the traffic is accepted by the KUBE-FORWARD chain, see the third rule:

➜  ~ iptables -L KUBE-FORWARD --line-n -v
Chain KUBE-FORWARD (1 references)
num   pkts bytes target     prot opt in     out     source               destination
1        0     0 DROP       all  --  any    any     anywhere             anywhere             ctstate INVALID
2        0     0 ACCEPT     all  --  any    any     anywhere             anywhere             /* kubernetes forwarding rules */ mark match 0x4000/0x4000
3      235 42692 ACCEPT     all  --  any    any     anywhere             anywhere             /* kubernetes forwarding conntrack pod source rule */ ctstate RELATED,ESTABLISHED
4        0     0 ACCEPT     all  --  any    any     anywhere             anywhere             /* kubernetes forwarding conntrack pod destination rule */ ctstate RELATED,ESTABLISHED

So, different from healthcheck nodeports, the traffic towards node ports got DNATted and instead of hitting INPUT chain, it will hit FORWARD chain. However health check node ports are listening on local addresses so the incoming packets will hit INPUT chain.

Before this change, there's no KUBE-NODEPORTS chain in the filter table at all, so whether the traffic is accepted or not is purely depending on the policy of INPUT chain. That's why we need to add a rule from INPUT chain to the newly added KUBE-NODEPORTS chain.

Speaking of the difference of node port traffic that is DNATted and the traffic towards health check endpoints that listens on local ports, I realized that if we have pods on hostNetwork, then node ports towards them won't work just like for health check node ports. I just validated that in my environment and the issue is really there, and I'm confident that I could fix that using the same approach just like for health check node port. I don't want to make more changes in the PR so maybe open an issue and fix it in another one, what do you guys think? @andrewsykim @danwinship ?

I think that better to do that in a follow up PR,

Ah... there are two completely separate KUBE-NODEPORTS chains now... one in -t nat and one in -t filter

ok, I think this is right, but don't use the word "portals" in the description. That's archaic pre-kube-1.0 terminology. Actually, just changing it to "ports" is probably what you meant anyway.

Sure, changed here.

should go inside the if svcInfo.NodePort() != 0 section

Node ports are now optional for load balancers (see spec.allocateLoadBalancerNodePorts), so a separate check still makes sense since healthCheckNodePorts is always set regardless of whether node ports is enabled.

Second to @andrewsykim, I think these are effectively the same, for now, HealthCheckNodePort will be non-zero only if the svc is of LoadBalancer type. And if the service is of LoadBalancer type, then it should have a non-zero NodePort. To me it's api-server's job to validate the spec and ensure all the svc proxy sees are valid (see my previous comment). If couple the logic here then if someday the validation logic changed, then we also need to fix it here.

My concern is if I just modify the INPUT chain policy to DROP then we might lose ssh connection to that VM instantly

You don't need to change the default INPUT chain policy, you can just do

iptables -A INPUT -p tcp --dport <PORT> -j DROP

or whatever

I'm less familiar with the IPVS rules but I believe the comment I made about iptables should apply here too

yeah same reason as my previous comment why we need to keep this rule.

I'm not sure if you also need to add something to ipsetWithIptablesChain...

Aha that's a tricky one, at first I thought I should add it here, but then I realized that ipsetWithIptablesChain is for nat table where we need to add it in filter table. I've already added the rule below:

       // Add rule to accept traffic towards health check node port
       utilproxy.WriteLine(proxier.filterRules,
               "-A", string(KubeNodePortChain),
               "-m", "comment", "--comment", proxier.ipsetList[kubeHealthCheckNodePortSet].getComment(),
               "-m", "set", "--match-set", proxier.ipsetList[kubeHealthCheckNodePortSet].Name, "dst",
               "-j", "ACCEPT",
       )