dockershim hostport manager should use HostIP #94382

aojea · 2020-08-31T23:07:22Z

/kind bug

What this PR does / why we need it:

the hostport manager was not taking into consideration the hostIP
when binding the socket of the hostPort, causing that the same
HostPort can not be used with different IP addresses.

fix a bug where the hostport manager used by dockershim didn't allow to bind the same HostPort on different HostIPs

aojea · 2020-08-31T23:13:40Z

/assign @thockin @danwinship @dcbw

I need to check if there is some job exercising this code with e2e tests, lot of jobs were move to containerd and (I think) using the CNI portmap plugin (at least the KIND ones)

thockin · 2020-08-31T23:55:52Z

I am frankly surprised this works at all in the scheduler - does it? Do we really want it to? What's the use case? This seems verrrrry subtle.

aojea · 2020-09-01T07:33:28Z

I am frankly surprised this works at all in the scheduler - does it? Do we really want it to? What's the use case? This seems verrrrry subtle.

There is a conformance test covering exactly this use case

kubernetes/test/e2e/scheduling/predicates.go

Lines 654 to 660 in 62b8fae

    
           	/* 
        
           		Release: v1.16 
        
           		Testname: Scheduling, HostPort matching and HostIP and Protocol not-matching 
        
           		Description: Pods with the same HostPort value MUST be able to be scheduled to the same node 
        
           		if the HostIP or Protocol is different. 
        
           	*/ 
        
           	framework.ConformanceIt("validates that there is no conflict between pods with same hostPort but different hostIP and protocol", func() {

It seems that works (at least in CRIO cri-o/cri-o#4130 (comment)) because, after the first attempt to allocate the HostPort, it removes ALL sockets listening in that port, so the second attempt works

aojea · 2020-09-01T10:31:01Z

I can't find any job in the CI running this code :/

I was reading the issue tracking the hostport implementation #35457, and it seems that this was temporary until the CNI plugins implements the portmapping,

@thockin do you think that does not make sense to pursue this and let it as is, fact is nobody noticed this based on a quick github search.

If we don fix it here., @danwinship I can make it a package in crio instead of vendoring it

danwinship · 2020-09-01T20:53:22Z

do you think that does not make sense to pursue this and let it as is, fact is nobody noticed this based on a quick github search.

Well, besides the fact that we know some people are vendoring this code, there's also the fact that it means that people using kubernetes with dockershim get non-conforming behavior. The fact that we don't currently run any CI jobs in that configuration doesn't change the fact that it's broken.

The fact that the conformance test fails to accurately test the behavior it's supposed to be testing is... interesting. We should probably fix the conformance test in 1.20 (by making it check that the first hostport still connects to the first pod after the second pod has been created) but not backport the fix, so as to not retroactively de-conform systems that previously passed conformance in earlier releases. At least, I think that's the right approach?

danwinship · 2020-09-01T20:58:18Z

pkg/kubelet/dockershim/network/hostport/hostport_manager_test.go

@@ -35,13 +35,15 @@ func TestOpenCloseHostports(t *testing.T) {
 		podPortMapping *PodPortMapping
 		expectError    bool
 	}{
+		// no portmaps


if you're going to identify all the test cases you might as well identify them as strings in the test cases so you can then print the description with the error when an error occurs?

Naming is not useful here because the test cases are not independent. Personally, I don't like using this test structure but didn't want to change the whole test,so I carried over with current status. At least the comments explain better what is supposed to happen

danwinship · 2020-09-01T21:00:38Z

pkg/kubelet/dockershim/network/hostport/hostport_manager_test.go

 	for _, tc := range openPortCases {
 		mapping, err := manager.openHostports(tc.podPortMapping)
+		for hostport, socket := range mapping {
+			manager.hostPortMap[hostport] = socket


Why is this needed here? This seems like something that should either happen in the actual manager code, or else not be needed.

If it's just for the counting below, then that's really only testing the behavior of the test program, not the behavior of the manager...

this was actually a bug in the test.
The same loop to add the open sockets to the hostPortMap map does happen in the manager.Add() method, https://github.com/kubernetes/kubernetes/blob/12fc5e50b9394cc3fa498b24a96c894d96b3bff2/pkg/kubelet/dockershim/network/hostport/hostport_manager.go#L107-L114

We need this because later we test the closeHostports() method that iterates over that map https://github.com/kubernetes/kubernetes/blob/12fc5e50b9394cc3fa498b24a96c894d96b3bff2/pkg/kubelet/dockershim/network/hostport/hostport_manager.go#L310-L327
in

for _, tc := range closePortCases { err := manager.closeHostports(tc.portMappings)

if we don't populate the map the closeHostports will always succeed because it's an empty map.
I've also added the counting below as a safety check and to make it easier to debug

pkg/kubelet/dockershim/network/hostport/hostport_manager_test.go

thockin · 2020-09-01T23:36:22Z

I did some quick tests on my own cluster and, shock, it does seem to work. I dislike this very much. It seems like a great opportunity to make a mistake (honestly, hostIP at all seems poorly thought out but that dates to 2014, sigh). So the choice is really a) fix it in dockershim to match containerd or b) don't allow same-port-different-IP between pods. I want to say B, but I suspect the answer is A. Can we document this behavior on the API types.go, please?

andrewsykim · 2020-09-01T23:47:03Z

pkg/kubelet/dockershim/network/hostport/hostport_manager.go

-			"-m", "comment", "--comment", fmt.Sprintf(`"%s hostport %d"`, podFullName, pm.HostPort),
-			"-m", protocol, "-p", protocol,
-			"-j", "DNAT", fmt.Sprintf("--to-destination=%s", hostPortBinding))
+		if pm.HostIP == "" || pm.HostIP == "0.0.0.0" || pm.HostIP == "::" {


Does kubelet know to use iptables for IPv6 if HostIP is :: here?

kubelet is ALWAYS single stack as today, and its IP family depends on the nodeIP,

kubernetes/pkg/kubelet/kubelet.go

Lines 468 to 473 in c236285

parsedNodeIP := net.ParseIP(nodeIP)

protocol := utilipt.ProtocolIPv4

if utilnet.IsIPv6(parsedNodeIP) {

klog.V(0).Infof("IPv6 node IP (%s), assume IPv6 operation", nodeIP)

protocol = utilipt.ProtocolIPv6

}

The hostport_manager has its own iptables handler, but depends on the kubelet for some rules, however, due to this #82125 , those rules are added by kube-proxy

Now that you mention it, without kube-proxy this won't work for dual-stack @danwinship

aojea · 2020-09-03T08:41:57Z

I want to say B, but I suspect the answer is A. Can we document this behavior on the API types.go, please?

I guess it is

kubernetes/pkg/apis/core/types.go

Lines 1762 to 1764 in adac1bf

    
           // Optional: What host IP to bind the external port to. 
        
           // +optional 
        
           HostIP string

aojea · 2020-09-03T08:57:45Z

pkg/kubelet/dockershim/network/hostport/hostport_manager_test.go

+		"-A KUBE-HOSTPORTS -m comment --comment \"pod5_ns5 hostport 8888\" -m tcp -p tcp --dport 8888 -j KUBE-HP-TUKTZ736U5JD5UTK":                        true,
+		"-A KUBE-HOSTPORTS -m comment --comment \"pod5_ns5 hostport 8888\" -m tcp -p tcp --dport 8888 -j KUBE-HP-CAAJ45HDITK7ARGM":                        true,


~~actually this does not seem to work as expected, it should load balance between the different backend HostIPs~~
it does work, it has the HostIP in the iptables rule

aojea · 2020-09-03T09:42:43Z

I just realized that the hostport_ syncer code was removed recently
#93581

In the kubernetes repo only the kubenet seems to use this code 🤔

the hostport manager was not taking into consideration the hostIP when binding the socket of the hostPort, causing that the same HostPort can not be used with different IP addresses.

The e2e test, included as part of Conformance, "validates that there is no conflict between pods with same hostPort but different hostIP and protocol" was only testing that the pods were scheduled without conflict but was never testing the functionality. The test should check that pods with containers forwarding the same hostPort can be scheduled without conflict, and that those exposed HostPort are forwarding the ports to the corresponding pods.

k8s-ci-robot · 2020-09-03T14:36:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aojea
To complete the pull request process, please assign thockin
You can assign the PR to them by writing /assign @thockin in a comment when ready.

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

Needs approval from an approver in each of these files:

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

SergeyKanzhelev · 2020-09-03T22:01:59Z

I just realized that the hostport_ syncer code was removed recently
#93581

In the kubernetes repo only the kubenet seems to use this code 🤔

My PR was just a follow up clean up after this one: #62212 I wasn't sure about hostport functionality and whether it is actually used.

One more discussion point here - there is a leftover of duplicates suppression logic for nodeport mappings. Is it needed in kubelet if CNI should be responsible for setting it all up?

aojea · 2020-09-04T08:19:29Z

/retest

aojea · 2020-09-04T08:24:29Z

One more discussion point here - there is a leftover of duplicates suppression logic for nodeport mappings. Is it needed in kubelet if CNI should be responsible for setting it all up?

@thockin I think that this is only being used by kubenet in k8s, the rest of the imports are just for the type []*hostport.PortMapping

is kubenet going to move out of the kubelet?

k8s-ci-robot requested review from danwinship and thockin August 31, 2020 23:08

k8s-ci-robot added area/kubelet sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 31, 2020

aojea changed the title ~~dockershim hostport manager use HostIP~~ dockershim hostport manager should use HostIP Aug 31, 2020

k8s-ci-robot assigned danwinship, dcbw and thockin Aug 31, 2020

danwinship reviewed Sep 1, 2020

View reviewed changes

andrewsykim reviewed Sep 1, 2020

View reviewed changes

aojea commented Sep 3, 2020

View reviewed changes

danwinship mentioned this pull request Sep 3, 2020

DualStack: kubelet: do dual-stack iptables rules #94474

Merged

Antonio Ojea added 2 commits September 3, 2020 16:22

dockershim hostport manager use HostIP

aa680c5

the hostport manager was not taking into consideration the hostIP when binding the socket of the hostPort, causing that the same HostPort can not be used with different IP addresses.

aojea force-pushed the hosport branch from 12fc5e5 to 8689507 Compare September 3, 2020 14:35

k8s-ci-robot added area/test sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 3, 2020

SergeyKanzhelev mentioned this pull request Sep 3, 2020

Allow to map the same container port to different host ports #94494

Merged

aojea mentioned this pull request Sep 4, 2020

Refactor hostport manager into own package cri-o/cri-o#4120

Closed

aramase mentioned this pull request Oct 13, 2020

dual stack services #91824

Merged

27 tasks

aojea mentioned this pull request Oct 28, 2020

Deprecate HostPort Manager in favor of Portmap CNI plugin cri-o/cri-o#4312

Closed

aojea mentioned this pull request Nov 10, 2020

Portmap support for dual-stack cri-o/cri-o#4361

Merged

aojea closed this Nov 14, 2020

This was referenced Feb 1, 2021

skip hostport tests on jobs with Docker runtime kubernetes/test-infra#20688

Closed

[Failing Test] [sig-network] HostPort validates that there is no conflict between pods with same hostPort but different hostIP and protocol #98648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dockershim hostport manager should use HostIP #94382

dockershim hostport manager should use HostIP #94382

aojea commented Aug 31, 2020

aojea commented Aug 31, 2020

thockin commented Aug 31, 2020

aojea commented Sep 1, 2020

aojea commented Sep 1, 2020

danwinship commented Sep 1, 2020

danwinship Sep 1, 2020

aojea Sep 1, 2020

danwinship Sep 1, 2020

aojea Sep 1, 2020 •

edited

Loading

thockin commented Sep 1, 2020 via email

andrewsykim Sep 1, 2020

aojea Sep 2, 2020

aojea commented Sep 3, 2020

aojea Sep 3, 2020 •

edited

Loading

aojea commented Sep 3, 2020

k8s-ci-robot commented Sep 3, 2020

SergeyKanzhelev commented Sep 3, 2020

aojea commented Sep 4, 2020

aojea commented Sep 4, 2020

	parsedNodeIP := net.ParseIP(nodeIP)
	protocol := utilipt.ProtocolIPv4
	if utilnet.IsIPv6(parsedNodeIP) {
	klog.V(0).Infof("IPv6 node IP (%s), assume IPv6 operation", nodeIP)
	protocol = utilipt.ProtocolIPv6
	}

		"-A KUBE-HOSTPORTS -m comment --comment \"pod5_ns5 hostport 8888\" -m tcp -p tcp --dport 8888 -j KUBE-HP-TUKTZ736U5JD5UTK": true,
		"-A KUBE-HOSTPORTS -m comment --comment \"pod5_ns5 hostport 8888\" -m tcp -p tcp --dport 8888 -j KUBE-HP-CAAJ45HDITK7ARGM": true,

dockershim hostport manager should use HostIP #94382

dockershim hostport manager should use HostIP #94382

Conversation

aojea commented Aug 31, 2020

aojea commented Aug 31, 2020

thockin commented Aug 31, 2020

aojea commented Sep 1, 2020

aojea commented Sep 1, 2020

danwinship commented Sep 1, 2020

danwinship Sep 1, 2020

Choose a reason for hiding this comment

aojea Sep 1, 2020

Choose a reason for hiding this comment

danwinship Sep 1, 2020

Choose a reason for hiding this comment

aojea Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

thockin commented Sep 1, 2020 via email

andrewsykim Sep 1, 2020

Choose a reason for hiding this comment

aojea Sep 2, 2020

Choose a reason for hiding this comment

aojea commented Sep 3, 2020

aojea Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

aojea commented Sep 3, 2020

k8s-ci-robot commented Sep 3, 2020

SergeyKanzhelev commented Sep 3, 2020

aojea commented Sep 4, 2020

aojea commented Sep 4, 2020

aojea Sep 1, 2020 •

edited

Loading

aojea Sep 3, 2020 •

edited

Loading