Explicitly add iptables rule to allow healthcheck nodeport #97824
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -454,6 +454,14 @@ func TestDeleteEndpointConnectionsIPv6(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // fakeCloseable implements utilproxy.Closeable | ||
| type fakeCloseable struct{} | ||
|
|
||
| // Close fakes out the close() used by syncProxyRules to release a local port. | ||
| func (f *fakeCloseable) Close() error { | ||
| return nil | ||
| } | ||
|
|
||
| // fakePortOpener implements portOpener. | ||
| type fakePortOpener struct { | ||
| openPorts []*utilproxy.LocalPort | ||
|
|
@@ -463,7 +471,7 @@ type fakePortOpener struct { | |
| // to lock a local port. | ||
| func (f *fakePortOpener) OpenLocalPort(lp *utilproxy.LocalPort, isIPv6 bool) (utilproxy.Closeable, error) { | ||
| f.openPorts = append(f.openPorts, lp) | ||
| return &fakeCloseable{}, nil | ||
| } | ||
|
|
||
| const testHostname = "test-hostname" | ||
|
|
@@ -913,6 +921,50 @@ func TestNodePort(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestHealthCheckNodePort(t *testing.T) { | ||
|
There was a problem hiding this comment. hiya @hanlins , looking at this unit test, do we also need a if not, what E2Es break if the default There was a problem hiding this comment. Oh true. I was thinking about it early today but don't know if there's a way to change the e2e VM settings? My concern is if I just modify the There was a problem hiding this comment. no need to make an e2e for it, i guess all that matters is... knowing which of the i bet theres something that requires nodeport healthchecks (i.e. maybe in test/e2e/network/services.go) which demonstrates this bug ? There was a problem hiding this comment. We have quite a few e2es already on health check node port because it's required for any Service LB w/ externalTrafficPolicy=Local. The tricky part is testing this on a node using default There was a problem hiding this comment.
You don't need to change the default or whatever There was a problem hiding this comment.
This rule would conflict with the rules we add from kube-proxy though. There was a problem hiding this comment.
That's the goal! Add a rule (to the very end of the There was a problem hiding this comment. That depends on if we prepend or append though right? Do we always prepend filter rules? There was a problem hiding this comment. The actual per-service rules get added to kubernetes-specific chains, not to the top-level |
||
| ipt := iptablestest.NewFake() | ||
| fp := NewFakeProxier(ipt, false) | ||
| svcIP := "10.20.30.42" | ||
| svcPort := 80 | ||
| svcNodePort := 3001 | ||
| svcHealthCheckNodePort := 30000 | ||
| svcPortName := proxy.ServicePortName{ | ||
| NamespacedName: makeNSN("ns1", "svc1"), | ||
| Port: "p80", | ||
| Protocol: v1.ProtocolTCP, | ||
| } | ||
|
|
||
| makeServiceMap(fp, | ||
| makeTestService(svcPortName.Namespace, svcPortName.Name, func(svc *v1.Service) { | ||
| svc.Spec.Type = "LoadBalancer" | ||
| svc.Spec.ClusterIP = svcIP | ||
| svc.Spec.Ports = []v1.ServicePort{{ | ||
| Name: svcPortName.Port, | ||
| Port: int32(svcPort), | ||
| Protocol: v1.ProtocolTCP, | ||
| NodePort: int32(svcNodePort), | ||
| }} | ||
| svc.Spec.HealthCheckNodePort = int32(svcHealthCheckNodePort) | ||
| svc.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeLocal | ||
| }), | ||
| ) | ||
|
|
||
| itf := net.Interface{Index: 0, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0} | ||
| addrs := []net.Addr{utilproxytest.AddrStruct{Val: "127.0.0.1/16"}} | ||
| itf1 := net.Interface{Index: 1, MTU: 0, Name: "eth1", HardwareAddr: nil, Flags: 0} | ||
| addrs1 := []net.Addr{utilproxytest.AddrStruct{Val: "::1/128"}} | ||
| fp.networkInterfacer.(*utilproxytest.FakeNetwork).AddInterfaceAddr(&itf, addrs) | ||
| fp.networkInterfacer.(*utilproxytest.FakeNetwork).AddInterfaceAddr(&itf1, addrs1) | ||
| fp.nodePortAddresses = []string{"127.0.0.1/16"} | ||
|
|
||
| fp.syncProxyRules() | ||
|
|
||
| kubeNodePortsRules := ipt.GetRules(string(kubeNodePortsChain)) | ||
| if !hasJump(kubeNodePortsRules, iptablestest.Accept, "", svcHealthCheckNodePort) { | ||
| errorf(fmt.Sprintf("Failed to find Accept rule"), kubeNodePortsRules, t) | ||
| } | ||
| } | ||
|
|
||
| func TestMasqueradeRule(t *testing.T) { | ||
| for _, testcase := range []bool{false, true} { | ||
| ipt := iptablestest.NewFake().SetHasRandomFully(testcase) | ||
|
|
@@ -2594,6 +2646,7 @@ func TestEndpointSliceE2E(t *testing.T) { | |
| :KUBE-SERVICES - [0:0] | ||
| :KUBE-EXTERNAL-SERVICES - [0:0] | ||
| :KUBE-FORWARD - [0:0] | ||
| :KUBE-NODEPORTS - [0:0] | ||
|
There was a problem hiding this comment. can we add a test like There was a problem hiding this comment. Good call, will do that. |
||
| -A KUBE-FORWARD -m conntrack --ctstate INVALID -j DROP | ||
| -A KUBE-FORWARD -m comment --comment "kubernetes forwarding rules" -m mark --mark 0x4000/0x4000 -j ACCEPT | ||
| -A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT | ||
|
|
@@ -2686,4 +2739,113 @@ COMMIT | |
| assert.NotEqual(t, expectedIPTablesWithSlice, fp.iptablesData.String()) | ||
| } | ||
|
|
||
| func TestHealthCheckNodePortE2E(t *testing.T) { | ||
| expectedIPTables := `*filter | ||
| :KUBE-SERVICES - [0:0] | ||
| :KUBE-EXTERNAL-SERVICES - [0:0] | ||
| :KUBE-FORWARD - [0:0] | ||
| :KUBE-NODEPORTS - [0:0] | ||
| -A KUBE-NODEPORTS -m comment --comment "ns1/svc1 health check node port" -m tcp -p tcp --dport 30000 -j ACCEPT | ||
| -A KUBE-FORWARD -m conntrack --ctstate INVALID -j DROP | ||
| -A KUBE-FORWARD -m comment --comment "kubernetes forwarding rules" -m mark --mark 0x4000/0x4000 -j ACCEPT | ||
| -A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod source rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT | ||
| -A KUBE-FORWARD -m comment --comment "kubernetes forwarding conntrack pod destination rule" -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT | ||
| COMMIT | ||
| *nat | ||
| :KUBE-SERVICES - [0:0] | ||
| :KUBE-NODEPORTS - [0:0] | ||
| :KUBE-POSTROUTING - [0:0] | ||
| :KUBE-MARK-MASQ - [0:0] | ||
| :KUBE-SVC-AQI2S6QIMU7PVVRP - [0:0] | ||
| :KUBE-XLB-AQI2S6QIMU7PVVRP - [0:0] | ||
| :KUBE-SEP-3JOIVZTXZZRGORX4 - [0:0] | ||
| :KUBE-SEP-IO5XOSKPAXIFQXAJ - [0:0] | ||
| :KUBE-SEP-XGJFVO3L2O5SRFNT - [0:0] | ||
| -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN | ||
| -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 | ||
| -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE | ||
| -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 | ||
| -A KUBE-SERVICES -m comment --comment "ns1/svc1 cluster IP" -m tcp -p tcp -d 172.20.1.1/32 --dport 0 ! -s 10.0.0.0/24 -j KUBE-MARK-MASQ | ||
| -A KUBE-SERVICES -m comment --comment "ns1/svc1 cluster IP" -m tcp -p tcp -d 172.20.1.1/32 --dport 0 -j KUBE-SVC-AQI2S6QIMU7PVVRP | ||
| -A KUBE-NODEPORTS -m comment --comment ns1/svc1 -m tcp -p tcp --dport 30010 -s 127.0.0.0/8 -j KUBE-MARK-MASQ | ||
| -A KUBE-NODEPORTS -m comment --comment ns1/svc1 -m tcp -p tcp --dport 30010 -j KUBE-XLB-AQI2S6QIMU7PVVRP | ||
| -A KUBE-SVC-AQI2S6QIMU7PVVRP -m comment --comment ns1/svc1 -m statistic --mode random --probability 0.3333333333 -j KUBE-SEP-3JOIVZTXZZRGORX4 | ||
| -A KUBE-SEP-3JOIVZTXZZRGORX4 -m comment --comment ns1/svc1 -s 10.0.1.1/32 -j KUBE-MARK-MASQ | ||
| -A KUBE-SEP-3JOIVZTXZZRGORX4 -m comment --comment ns1/svc1 -m tcp -p tcp -j DNAT --to-destination 10.0.1.1:80 | ||
| -A KUBE-SVC-AQI2S6QIMU7PVVRP -m comment --comment ns1/svc1 -m statistic --mode random --probability 0.5000000000 -j KUBE-SEP-IO5XOSKPAXIFQXAJ | ||
| -A KUBE-SEP-IO5XOSKPAXIFQXAJ -m comment --comment ns1/svc1 -s 10.0.1.2/32 -j KUBE-MARK-MASQ | ||
| -A KUBE-SEP-IO5XOSKPAXIFQXAJ -m comment --comment ns1/svc1 -m tcp -p tcp -j DNAT --to-destination 10.0.1.2:80 | ||
| -A KUBE-SVC-AQI2S6QIMU7PVVRP -m comment --comment ns1/svc1 -j KUBE-SEP-XGJFVO3L2O5SRFNT | ||
| -A KUBE-SEP-XGJFVO3L2O5SRFNT -m comment --comment ns1/svc1 -s 10.0.1.3/32 -j KUBE-MARK-MASQ | ||
| -A KUBE-SEP-XGJFVO3L2O5SRFNT -m comment --comment ns1/svc1 -m tcp -p tcp -j DNAT --to-destination 10.0.1.3:80 | ||
| -A KUBE-XLB-AQI2S6QIMU7PVVRP -m comment --comment "Redirect pods trying to reach external loadbalancer VIP to clusterIP" -s 10.0.0.0/24 -j KUBE-SVC-AQI2S6QIMU7PVVRP | ||
| -A KUBE-XLB-AQI2S6QIMU7PVVRP -m comment --comment "masquerade LOCAL traffic for ns1/svc1 LB IP" -m addrtype --src-type LOCAL -j KUBE-MARK-MASQ | ||
| -A KUBE-XLB-AQI2S6QIMU7PVVRP -m comment --comment "route LOCAL traffic for ns1/svc1 LB IP to service chain" -m addrtype --src-type LOCAL -j KUBE-SVC-AQI2S6QIMU7PVVRP | ||
| -A KUBE-XLB-AQI2S6QIMU7PVVRP -m comment --comment "Balancing rule 0 for ns1/svc1" -j KUBE-SEP-3JOIVZTXZZRGORX4 | ||
| -A KUBE-SERVICES -m comment --comment "kubernetes service nodeports; NOTE: this must be the last rule in this chain" -m addrtype --dst-type LOCAL -j KUBE-NODEPORTS | ||
| COMMIT | ||
| ` | ||
|
|
||
| ipt := iptablestest.NewFake() | ||
| fp := NewFakeProxier(ipt, true) | ||
| fp.OnServiceSynced() | ||
| fp.OnEndpointsSynced() | ||
| fp.OnEndpointSlicesSynced() | ||
|
|
||
| serviceName := "svc1" | ||
| namespaceName := "ns1" | ||
|
|
||
| svc := &v1.Service{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: namespaceName}, | ||
| Spec: v1.ServiceSpec{ | ||
| ClusterIP: "172.20.1.1", | ||
| Selector: map[string]string{"foo": "bar"}, | ||
| Ports: []v1.ServicePort{{Name: "", TargetPort: intstr.FromInt(80), NodePort: 30010, Protocol: v1.ProtocolTCP}}, | ||
| Type: "LoadBalancer", | ||
| HealthCheckNodePort: 30000, | ||
| ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyTypeLocal, | ||
| }, | ||
| } | ||
| fp.OnServiceAdd(svc) | ||
|
|
||
| tcpProtocol := v1.ProtocolTCP | ||
| endpointSlice := &discovery.EndpointSlice{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("%s-1", serviceName), | ||
| Namespace: namespaceName, | ||
| Labels: map[string]string{discovery.LabelServiceName: serviceName}, | ||
| }, | ||
| Ports: []discovery.EndpointPort{{ | ||
| Name: utilpointer.StringPtr(""), | ||
| Port: utilpointer.Int32Ptr(80), | ||
| Protocol: &tcpProtocol, | ||
| }}, | ||
| AddressType: discovery.AddressTypeIPv4, | ||
| Endpoints: []discovery.Endpoint{{ | ||
| Addresses: []string{"10.0.1.1"}, | ||
| Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, | ||
| Topology: map[string]string{"kubernetes.io/hostname": testHostname}, | ||
| }, { | ||
| Addresses: []string{"10.0.1.2"}, | ||
| Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, | ||
| Topology: map[string]string{"kubernetes.io/hostname": "node2"}, | ||
| }, { | ||
| Addresses: []string{"10.0.1.3"}, | ||
| Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, | ||
| Topology: map[string]string{"kubernetes.io/hostname": "node3"}, | ||
| }, { // not ready endpoints should be ignored | ||
| Addresses: []string{"10.0.1.4"}, | ||
| Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(false)}, | ||
| Topology: map[string]string{"kubernetes.io/hostname": "node4"}, | ||
| }}, | ||
| } | ||
| fp.OnEndpointSliceAdd(endpointSlice) | ||
| fp.syncProxyRules() | ||
| assert.Equal(t, expectedIPTables, fp.iptablesData.String()) | ||
|
|
||
| fp.OnServiceDelete(svc) | ||
| fp.syncProxyRules() | ||
| assert.NotEqual(t, expectedIPTables, fp.iptablesData.String()) | ||
| } | ||
|
|
||
| // TODO(thockin): add *more* tests for syncProxyRules() or break it down further and test the pieces. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,7 @@ var iptablesJumpChain = []struct { | |
| {utiliptables.TableNAT, utiliptables.ChainPrerouting, kubeServicesChain, "kubernetes service portals"}, | ||
| {utiliptables.TableNAT, utiliptables.ChainPostrouting, kubePostroutingChain, "kubernetes postrouting rules"}, | ||
| {utiliptables.TableFilter, utiliptables.ChainForward, KubeForwardChain, "kubernetes forwarding rules"}, | ||
| {utiliptables.TableFilter, utiliptables.ChainInput, KubeNodePortChain, "kubernetes health check rules"}, | ||
|
There was a problem hiding this comment. I'm less familiar with the IPVS rules but I believe the comment I made about iptables should apply here too There was a problem hiding this comment. yeah same reason as my previous comment why we need to keep this rule. |
||
| } | ||
|
|
||
| var iptablesChains = []struct { | ||
|
|
@@ -119,6 +120,7 @@ var iptablesChains = []struct { | |
| {utiliptables.TableNAT, KubeLoadBalancerChain}, | ||
| {utiliptables.TableNAT, KubeMarkMasqChain}, | ||
| {utiliptables.TableFilter, KubeForwardChain}, | ||
| {utiliptables.TableFilter, KubeNodePortChain}, | ||
| } | ||
|
|
||
| var iptablesEnsureChains = []struct { | ||
|
|
@@ -161,6 +163,7 @@ var ipsetInfo = []struct { | |
| {kubeNodePortLocalSetUDP, utilipset.BitmapPort, kubeNodePortLocalSetUDPComment}, | ||
| {kubeNodePortSetSCTP, utilipset.HashIPPort, kubeNodePortSetSCTPComment}, | ||
| {kubeNodePortLocalSetSCTP, utilipset.HashIPPort, kubeNodePortLocalSetSCTPComment}, | ||
| {kubeHealthCheckNodePortSet, utilipset.BitmapPort, kubeHealthCheckNodePortSetComment}, | ||
|
There was a problem hiding this comment. I'm not sure if you also need to add something to There was a problem hiding this comment. Aha that's a tricky one, at first I thought I should add it here, but then I realized that // 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",
) |
||
| } | ||
|
|
||
| // ipsetWithIptablesChain is the ipsets list with iptables source chain and the chain jump to | ||
|
|
@@ -1581,6 +1584,22 @@ func (proxier *Proxier) syncProxyRules() { | |
| } | ||
| } | ||
| } | ||
|
|
||
| if svcInfo.HealthCheckNodePort() != 0 { | ||
| nodePortSet := proxier.ipsetList[kubeHealthCheckNodePortSet] | ||
| entry := &utilipset.Entry{ | ||
| // No need to provide ip info | ||
| Port: svcInfo.HealthCheckNodePort(), | ||
| Protocol: "tcp", | ||
| SetType: utilipset.BitmapPort, | ||
| } | ||
|
|
||
| if valid := nodePortSet.validateEntry(entry); !valid { | ||
| klog.Errorf("%s", fmt.Sprintf(EntryInvalidErr, entry, nodePortSet.Name)) | ||
| continue | ||
| } | ||
| nodePortSet.activeEntries.Insert(entry.String()) | ||
| } | ||
| } | ||
|
|
||
| // sync ipset entries | ||
|
|
@@ -1817,6 +1836,14 @@ func (proxier *Proxier) writeIptablesRules() { | |
| "-j", "ACCEPT", | ||
| ) | ||
|
|
||
| // 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", | ||
| ) | ||
|
|
||
| // Install the kubernetes-specific postrouting rules. We use a whole chain for | ||
| // this so that it is easier to flush and change, for example if the mark | ||
| // value should ever change. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should go inside the
if svcInfo.NodePort() != 0sectionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second to @andrewsykim, I think these are effectively the same, for now,
HealthCheckNodePortwill be non-zero only if the svc is ofLoadBalancertype. And if the service is ofLoadBalancertype, then it should have a non-zeroNodePort. 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.