dockershim hostport manager use HostIP #98755

aojea · 2021-02-04T07:47:49Z

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.

What type of PR is this?

/kind bug
/kind failing-test

kubelet: fixes a bug in the HostPort dockershim implementation that caused the conformance test "HostPort validates that there is no conflict between pods with same hostPort but different hostIP and protocol" to fail.

Fixes: #98648

aojea · 2021-02-04T07:52:52Z

/hold
this is an alternative to just skipping the test for dockershim
kubernetes/test-infra#20688

/test

k8s-ci-robot · 2021-02-04T07:52:53Z

@aojea: The /test command needs one or more targets.
The following commands are available to trigger jobs:

/test pull-kubernetes-bazel-build
/test pull-kubernetes-bazel-test
/test pull-kubernetes-conformance-image-test
/test pull-kubernetes-conformance-kind-ipv6-parallel
/test pull-kubernetes-dependencies
/test pull-kubernetes-dependencies-canary
/test pull-kubernetes-e2e-ipvs-azure-dualstack
/test pull-kubernetes-e2e-iptables-azure-dualstack
/test pull-kubernetes-e2e-aws-eks-1-13-correctness
/test pull-kubernetes-files-remake
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-gce-no-stage
/test pull-kubernetes-e2e-gce-kubetest2
/test pull-kubernetes-e2e-gce-canary
/test pull-kubernetes-e2e-gce-ubuntu
/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-e2e-gce-ubuntu-containerd-canary
/test pull-kubernetes-e2e-gce-rbe
/test pull-kubernetes-e2e-gce-alpha-features
/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-integration
/test pull-kubernetes-cross
/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-kind-canary
/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-e2e-kind-ipv6-canary
/test pull-kubernetes-conformance-kind-ga-only
/test pull-kubernetes-conformance-kind-ga-only-parallel
/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-bazel-build-canary
/test pull-kubernetes-bazel-test-canary
/test pull-kubernetes-bazel-test-integration-canary
/test pull-kubernetes-local-e2e
/test pull-publishing-bot-validate
/test pull-kubernetes-e2e-gce-network-proxy-http-connect
/test pull-kubernetes-e2e-gce-network-proxy-grpc
/test pull-kubernetes-e2e-gci-gce-autoscaling
/test pull-kubernetes-e2e-aks-engine-azure
/test pull-kubernetes-e2e-azure-disk
/test pull-kubernetes-e2e-azure-disk-vmss
/test pull-kubernetes-e2e-azure-file
/test pull-kubernetes-e2e-kind-dual-canary
/test pull-kubernetes-e2e-kind-ipvs-dual-canary
/test pull-kubernetes-e2e-ubuntu-gce-network-policies
/test pull-kubernetes-e2e-gci-gce-ipvs
/test pull-kubernetes-node-e2e
/test pull-kubernetes-node-e2e-podutil
/test pull-kubernetes-e2e-containerd-gce
/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-node-e2e-alpha
/test pull-kubernetes-node-kubelet-serial-cpu-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager
/test pull-kubernetes-node-kubelet-serial-hugepages
/test pull-kubernetes-node-crio-cgrpv2-e2e
/test pull-kubernetes-node-crio-e2e
/test pull-kubernetes-node-kubelet-serial-memory-manager
/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-gce-big-performance
/test pull-kubernetes-e2e-gce-correctness
/test pull-kubernetes-e2e-gce-large-performance
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-kubemark-e2e-gce-scale
/test pull-kubernetes-e2e-gce-storage-slow
/test pull-kubernetes-e2e-gce-storage-snapshot
/test pull-kubernetes-e2e-gce-storage-slow-rbe
/test pull-kubernetes-e2e-gce-csi-serial
/test pull-kubernetes-e2e-gce-iscsi
/test pull-kubernetes-e2e-gce-iscsi-serial
/test pull-kubernetes-e2e-gce-storage-disruptive
/test pull-kubernetes-e2e-aks-engine-azure-windows
/test pull-kubernetes-e2e-aks-engine-windows-contianerd
/test pull-kubernetes-e2e-azure-disk-windows
/test pull-kubernetes-e2e-azure-file-windows
/test pull-kubernetes-e2e-aks-engine-windows-gpu
/test pull-kubernetes-e2e-azure-disk-windows-containerd
/test pull-kubernetes-e2e-azure-file-windows-containerd
/test pull-kubernetes-typecheck
/test pull-kubernetes-verify-govet-levee
/test pull-kubernetes-verify
/test pull-kubernetes-e2e-windows-gce

Use /test all to run the following jobs:

pull-kubernetes-bazel-build
pull-kubernetes-bazel-test
pull-kubernetes-dependencies
pull-kubernetes-e2e-gce-ubuntu-containerd
pull-kubernetes-integration
pull-kubernetes-e2e-kind
pull-kubernetes-e2e-kind-ipv6
pull-kubernetes-conformance-kind-ga-only-parallel
pull-kubernetes-node-e2e
pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-typecheck
pull-kubernetes-verify-govet-levee
pull-kubernetes-verify

In response to this:

/hold
this is an alternative to just skipping the test for dockershim
kubernetes/test-infra#20688

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aojea · 2021-02-04T09:14:30Z

the kubelet seems is using kubenet (that is the one used for the hostport thing with kubernetes)

I0204 08:39:35.290281 9514 flags.go:59] FLAG: --minimum-container-ttl-duration="0s"
I0204 08:39:35.290287 9514 flags.go:59] FLAG: --minimum-image-ttl-duration="2m0s"
I0204 08:39:35.290294 9514 flags.go:59] FLAG: --network-plugin="kubenet"
I0204 08:39:35.290300 9514 flags.go:59] FLAG: --network-plugin-mtu="0"

and the test passes now:

Kubernetes e2e suite: [sig-network] HostPort validates that there is no conflict between pods with same hostPort but different hostIP and protocol [LinuxOnly] [Conformance]

aojea · 2021-02-04T10:48:51Z

The hostport tests passes now, these tests are failing though, I think that is due to the commit to force the runtime, will recheck

[sig-node] RuntimeClass should run a Pod requesting a RuntimeClass with scheduling without

aojea · 2021-02-04T10:54:32Z

/assign @thockin @BenTheElder @liggitt
This is the fix for kubenet/dockershim/hostport, this is the same code we have working in CRI-O, that used to vendor this code until we realised that it had a bug because is not able to bind HostPorts to IPs (and that the test that was supposed to verified that was not doing it)
I think that the reason to not detecting this before is that in cloud environments doesn´t make much sense to use HostPorts with HostIP, but people do a lot of "complicated" things in bare metal with multiple interfaces, ips, ... and the API is there and clear about that 🤷

aojea · 2021-02-04T11:51:12Z

/retest
unrelated failures, everything is passing right now

dims · 2021-02-04T12:07:00Z

/milestone v1.21
/area conformance
/priority critical-urgent
/triage accepted

this has conformance implications, without this dockershim nodes cannot pass conformance.

liggitt · 2021-02-04T13:54:28Z

I don't have the networking expertise to review this, but I'm +💯 on making sure dockershim can pass conformance tests.

/unassign

dims · 2021-02-04T16:26:54Z

cc @SergeyKanzhelev @ehashman @adisky

johnbelamaric

Can we cherry pick to 1.20? Ref: kubernetes/test-infra#20688 (comment)

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

johnbelamaric · 2021-02-04T19:16:30Z

this looks good to me but I would rather someone with some experience with this code give the lgtm and approval

thockin

Despite the size, this seems OK to me.

Do you want to clean up the commits? "TEMP" sets off alarm bells.

/approve

thockin · 2021-02-05T00:46:08Z

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

@@ -148,14 +148,14 @@ func (f *fakeIPTables) ensureRule(position utiliptables.RulePosition, tableName
 		return true, nil
 	}

-	if position == utiliptables.Prepend {


Ideally this would be in a sub-pkg, not linked into prod code. Not going to hold this up for that.

cluster/gce/config-default.sh

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.

k8s-ci-robot · 2021-02-05T07:52:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, thockin

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubelet/dockershim/network/OWNERS~~ [thockin]

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

aojea · 2021-02-05T07:53:01Z

/hold cancel

BenTheElder · 2021-02-05T20:34:46Z

/lgtm

…upstream-release-1.20 Automated cherry pick of #98755 upstream release 1.20

k8s-ci-robot requested review from bowei and rramkumar1 February 4, 2021 07:49

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2021

aojea force-pushed the hostportdockershim branch from 41c9c3e to d7ab35b Compare February 4, 2021 08:24

k8s-ci-robot added area/provider/gcp Issues or PRs related to gcp provider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Feb 4, 2021

aojea force-pushed the hostportdockershim branch from d7ab35b to f2430e1 Compare February 4, 2021 09:15

aojea force-pushed the hostportdockershim branch from f2430e1 to 845de65 Compare February 4, 2021 10:49

k8s-ci-robot assigned BenTheElder, liggitt and thockin Feb 4, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 4, 2021

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

dims mentioned this pull request Feb 4, 2021

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

Closed

k8s-ci-robot unassigned liggitt Feb 4, 2021

johnbelamaric reviewed Feb 4, 2021

View reviewed changes

pkg/kubelet/dockershim/network/hostport/hostport_manager.go Outdated Show resolved Hide resolved

aojea mentioned this pull request Feb 4, 2021

hostport manager clean up host ports cri-o/cri-o#4542

Merged

ehashman added this to Needs Reviewer in SIG Node PR Triage Feb 5, 2021

thockin reviewed Feb 5, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2021

BenTheElder reviewed Feb 5, 2021

View reviewed changes

cluster/gce/config-default.sh Outdated Show resolved Hide resolved

Antonio Ojea added 2 commits February 5, 2021 08:51

dockershim hostport manager use HostIP

f7d86e8

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.

dockershim hostport respect IPFamily

ad4776b

aojea force-pushed the hostportdockershim branch from f1785e4 to ad4776b Compare February 5, 2021 07:51

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2021

k8s-ci-robot merged commit 0faf096 into kubernetes:master Feb 5, 2021

SIG Node PR Triage automation moved this from Needs Reviewer to Done Feb 5, 2021

aojea mentioned this pull request Feb 6, 2021

move e2e hostport conflict test to sig-network #98299

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 6, 2021

aojea mentioned this pull request Feb 6, 2021

Automated cherry pick of #98755 upstream release 1.20 #98838

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 10, 2021

Merge pull request #98838 from aojea/automated-cherry-pick-of-#98755-…

a3d4ea9

…upstream-release-1.20 Automated cherry pick of #98755 upstream release 1.20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dockershim hostport manager use HostIP #98755

dockershim hostport manager use HostIP #98755

aojea commented Feb 4, 2021 •

edited by liggitt

aojea commented Feb 4, 2021

k8s-ci-robot commented Feb 4, 2021

aojea commented Feb 4, 2021

aojea commented Feb 4, 2021

aojea commented Feb 4, 2021 •

edited

aojea commented Feb 4, 2021

dims commented Feb 4, 2021

liggitt commented Feb 4, 2021

dims commented Feb 4, 2021

johnbelamaric left a comment

johnbelamaric commented Feb 4, 2021

thockin left a comment

thockin Feb 5, 2021

k8s-ci-robot commented Feb 5, 2021

aojea commented Feb 5, 2021

BenTheElder commented Feb 5, 2021

dockershim hostport manager use HostIP #98755

dockershim hostport manager use HostIP #98755

Conversation

aojea commented Feb 4, 2021 • edited by liggitt

aojea commented Feb 4, 2021

k8s-ci-robot commented Feb 4, 2021

aojea commented Feb 4, 2021

aojea commented Feb 4, 2021

aojea commented Feb 4, 2021 • edited

aojea commented Feb 4, 2021

dims commented Feb 4, 2021

liggitt commented Feb 4, 2021

dims commented Feb 4, 2021

johnbelamaric left a comment

Choose a reason for hiding this comment

johnbelamaric commented Feb 4, 2021

thockin left a comment

Choose a reason for hiding this comment

thockin Feb 5, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 5, 2021

aojea commented Feb 5, 2021

BenTheElder commented Feb 5, 2021

aojea commented Feb 4, 2021 •

edited by liggitt

aojea commented Feb 4, 2021 •

edited