Support TCP type runtime endpoint for kubelet #46089

karataliu · 2017-05-19T09:30:08Z

What this PR does / why we need it:
Currently the grpc server for kubelet and dockershim has a hardcoded endpoint: unix socket '/var/run/dockershim.sock', which is not applicable on non-unix OS.

This PR is to support TCP endpoint type besides unix socket.

Which issue this PR fixes
This is a first attempt to address issue #45927

Special notes for your reviewer:
Before this change, running on Windows node results in:

Container Manager is unsupported in this build

After adding the cm stub, error becomes:

listen unix /var/run/dockershim.sock: socket: An address incompatible with the requested protocol was used.

This PR is to fix those two issues.

After this change, still meets 'seccomp' related issue when running on Windows node, needs more updates later.

Release note:

k8s-ci-robot · 2017-05-19T09:30:16Z

Hi @karataliu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

yujuhong · 2017-05-19T21:08:05Z

/cc @Random-Liu @feiskyer

@karataliu, thanks for the PR! I have a few questions.

What's the status of windows support w/ this PR? We don't have any testing and it's hard for me to gauge how complete this PR is. I remember seeing more changes in docker_manager.go about host config and network setting changes.
Supporting TCP may cause security concerns since the connection could be non-local. I don't think we are ready to do that. /cc @timstclair

yujuhong · 2017-05-19T22:49:53Z

Supporting TCP may cause security concerns since the connection could be non-local. I don't think we are ready to do that. /cc @timstclair

@karataliu I'd be ok with adding an --experimental-windows flag, and only allow TCP connections when the flag is set. I think it also aligns with the fact that windows support is in its early Alpha stage.

karataliu · 2017-05-22T07:53:18Z

@yujuhong Thanks for the comments.

I can see the concern. If the communication is limited to local only, one option is to switch to named pipe on windows, which supports more security settings.
I've some attempts but seems grpc on windows does not work well with named pipe now. So here it is going to fallback to TCP.
I've pushed a new commit by extracting out two os dependent methods, so that both linux and windows build would only support specific protocol:
- linux will support 'unix socket' (which is the existing behaviour)
- windows would support 'tcp' for now, and may change to named pipe or others in future
Please comment if you think a new flag or other approach is preferred.
Since there's no testing as you mentioned, I also do not get a full map on how complete the support will be. For now I suppose the first step it's to catch up with docker manager implementation in v1.6 .

We're still dealing with docker running on windows, thus code snippets in docker_manager would remain useful. This PR is to fix the issue that blocks all remaining changes.

p.s. With this PR included, we'll get the following error during pod creating:

"CreatePodSandbox for pod \"s1_default(11312b63-3ebc-11e7-809f-000d3a80d419)\" failed: rpc error: code = 2 desc = failed to start sandbox container for pod \"s1\": 
Error response from daemon: {\"message\":\"security option not supported: seccomp\"}"

And related code of docker_manager in v1.6:
https://github.com/kubernetes/kubernetes/blob/release-1.6.3/pkg/kubelet/dockertools/docker_manager_windows.go#L78

feiskyer · 2017-05-22T09:43:50Z

@k8s-bot ok to test

yujuhong · 2017-05-22T16:26:18Z

I can see the concern. If the communication is limited to local only, one option is to switch to named pipe on windows, which supports more security settings.
I've some attempts but seems grpc on windows does not work well with named pipe now. So here it is going to fallback to TCP.

I've pushed a new commit by extracting out two os dependent methods, so that both linux and windows build would only support specific protocol:

linux will support 'unix socket' (which is the existing behaviour)
windows would support 'tcp' for now, and may change to named pipe or others in future
Please comment if you think a new flag or other approach is preferred.

Yes, named pipe would be a better choice if grpc can support it. I am okay with Windows using TCP for now since it's an experimental/Alpha feature. The idea sounds good to me. Will review the PR later.

yujuhong · 2017-05-22T16:32:39Z

cmd/kubelet/app/options/options.go

@@ -302,8 +302,8 @@ func (c *kubeletConfiguration) addFlags(fs *pflag.FlagSet) {
 	// CRI flags.
 	fs.BoolVar(&c.ExperimentalDockershim, "experimental-dockershim", c.ExperimentalDockershim, "Enable dockershim only mode. In this mode, kubelet will only start dockershim without any other functionalities. This flag only serves test purpose, please do not use it unless you are conscious of what you are doing. [default=false]")
 	fs.MarkHidden("experimental-dockershim")
-	fs.StringVar(&c.RemoteRuntimeEndpoint, "container-runtime-endpoint", c.RemoteRuntimeEndpoint, "[Experimental] The unix socket endpoint of remote runtime service.")
-	fs.StringVar(&c.RemoteImageEndpoint, "image-service-endpoint", c.RemoteImageEndpoint, "[Experimental] The unix socket endpoint of remote image service. If not specified, it will be the same with container-runtime-endpoint by default.")
+	fs.StringVar(&c.RemoteRuntimeEndpoint, "container-runtime-endpoint", c.RemoteRuntimeEndpoint, "[Experimental] The endpoint of remote runtime service. Currently unix socket is supported on Linux, and tcp is supported on windows.  Examples:'unix:///var/run/dockershim.sock', 'tcp://localhost:3735'")


You changed the format of the endpoint to include the protocol. I don't want to break existing users. Could you make sure that endpoints without protocols can still be accepted by assuming the default protocol for each OS?

It's also a bit strange that the new schemes are not registered with the IANA. They seem to be Go-isms. Is it worth mentioning (here or in some other docs?) that unix is a SOCK_STREAM (vs. unixgram's SOCK_DGRAM and unixpacket's SOCK_SEQPACKET)?

yujuhong · 2017-05-22T16:40:06Z

pkg/kubelet/dockershim/cm/container_manager_windows.go

+// +build windows
+
+/*
+Copyright 2016 The Kubernetes Authors.


s/2016/2017

yujuhong · 2017-05-22T16:43:26Z

pkg/kubelet/kubelet.go

@@ -577,6 +570,9 @@ func NewMainKubelet(kubeCfg *componentconfig.KubeletConfiguration, kubeDeps *Kub
 			return nil, fmt.Errorf("unsupported CRI runtime: %q", kubeCfg.ContainerRuntime)
 		}
 		runtimeService, imageService, err := getRuntimeAndImageServices(kubeCfg)
+		if err != nil {


Good catch! :-)

karataliu · 2017-05-23T05:53:51Z

Thanks for the comments.

I thinks it's good to keep compatible with existing "container-runtime-endpoint" and "image-service-endpoint" flag formats, though they're still marked as '[Experimental]'.

But for windows, since the CRI support is new, suppose we could enforce the full url format as there being no 'legacy' behaviour. Otherwise it is likely to become chaos if we're to change default protocol type on windows in future.
For example, when switching from TCP to named PIPE, we might need to keep support for both 'localhost:6666' (exisiting behaviour), and also '\.\pipe\PipeName' (new default).

I've pushed a new commit: add fallback support for using '/var/run/a.sock' as unix socket on linux, and also include a hint message for users to update endpoint format. Please comment.

yujuhong · 2017-05-23T15:57:05Z

The new commit looks good.

Could you add some basic unit tests for the parsing functions? I think the PR will be good to go after that.

karataliu · 2017-05-24T05:46:59Z

unit test commit pushed.

feiskyer

LGTM

feiskyer · 2017-05-24T06:31:09Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

yujuhong · 2017-05-24T16:41:48Z

LGTM. Will apply the label after you squash the commits. Thanks!

yujuhong · 2017-05-24T18:49:02Z

LGTM. Will apply the label after you squash the commits. Thanks!

@karataliu, to speed up the review turnaround time, feel free to send another PR for the rest of changes (e.g., fixing the security options) whenever you're ready. Thanks

karataliu · 2017-05-25T02:24:46Z

@yujuhong Squashed commit pushed. Looks fine, I'll try that. Thanks.

feiskyer · 2017-05-25T03:40:58Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

k8s-ci-robot · 2017-05-25T04:08:33Z

@karataliu: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`fb26c91`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

yujuhong · 2017-05-25T16:14:39Z

@karataliu thanks for working on this!

/lgtm

yujuhong · 2017-05-25T16:22:50Z

cc @mtaufen for approval.

mtaufen · 2017-05-25T17:50:42Z

componentconfig changes look fine to me
/approve

k8s-github-robot · 2017-05-25T17:50:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karataliu, mtaufen, yujuhong

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubelet/OWNERS~~ [yujuhong]
~~pkg/apis/componentconfig/OWNERS~~ [mtaufen]
~~pkg/kubelet/OWNERS~~ [yujuhong]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-26T04:40:00Z

Automatic merge from submit-queue (batch tested with PRs 46124, 46434, 46089, 45589, 46045)

Automatic merge from submit-queue Support windows in dockershim **What this PR does / why we need it**: This is the 2nd part for #45927 . The non-cri implementation dockertools was removed from kubelet v1.7 . Part of previous work for supporting windows container lies in v1.6 dockertools, this PR is to port them to dockershim. Main reference file in v1.6 dockertools windows support: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go **Which issue this PR fixes** 45927, for now catching up the implementation of v1.6 **Special notes for your reviewer**: The code change includes 4 parts, put them together as we discussed in #46089 1. Update go-winio package to a newer version 'go-winio' package is used by docker client. This change is to bring the support for Go v1.8, specifically included in the PR: microsoft/go-winio#48 Otherwise it will produce a lot of error like in: fsouza/go-dockerclient#648 2. Add os dependent getSecurityOpts helper method. seccomp not supported on windows Corresponding code in v1.6: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go#L78 3. Add updateCreateConfig. Allow user specified network mode setting. This is to be compatible with what kube-proxy package does on Windows. Also, there is a Linux section in both sandbox config and container config： LinuxPodSandboxConfig， LinuxContainerConfig. And that section later goes to Config and HostConfig section under docker container createConfig. Ideally hostconfig section should be dependent on host os, while config should depend on container image os. To simplify the case, here it assumes that windows host supports windows type container image only. It needs to be updated when kubernetes is to support windows host running linux container image or the like. Corresponding code in v1.6: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go#L57 4. Add podIpCache in dockershim. For v1.6 windows implementation, it still does not use sandbox, thus only allow single container to be exposed. Here added a cache for saving container IP, to get adapted to the new CRI api. Corresponding code in v1.6: No sandbox: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go#L66 Use container id as pod ip: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager.go#L2727 **Release note**:

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 19, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 19, 2017

k8s-github-robot assigned yujuhong and tmrts May 19, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 19, 2017

yujuhong unassigned tmrts May 19, 2017

k8s-ci-robot requested review from feiskyer and Random-Liu May 19, 2017 21:08

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 22, 2017

yujuhong added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 22, 2017

yujuhong reviewed May 22, 2017

View reviewed changes

feiskyer approved these changes May 24, 2017

View reviewed changes

Support TCP type runtime endpoint for kubelet.

fb26c91

karataliu force-pushed the wincri1 branch from c27c691 to fb26c91 Compare May 25, 2017 02:21

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2017

yujuhong assigned mtaufen May 25, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2017

yujuhong added this to the v1.7 milestone May 25, 2017

k8s-github-robot merged commit 5e85370 into kubernetes:master May 26, 2017

yujuhong mentioned this pull request May 27, 2017

Add windows support in the docker-CRI implementation (pkg/kubelet/dockershim) #45927

Closed

karataliu mentioned this pull request Jun 1, 2017

Support windows in dockershim #46744

Merged

karataliu deleted the wincri1 branch June 8, 2017 03:18

danwinship mentioned this pull request Apr 5, 2018

Prefix with unix protocol for docker shim socket openshift/origin#19224

Merged

pacoxu mentioned this pull request Mar 27, 2021

kubeadm set --container-runtime-endpoint with prefix 'unix://' #100578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TCP type runtime endpoint for kubelet #46089

Support TCP type runtime endpoint for kubelet #46089

karataliu commented May 19, 2017

k8s-ci-robot commented May 19, 2017

yujuhong commented May 19, 2017

yujuhong commented May 19, 2017

karataliu commented May 22, 2017

feiskyer commented May 22, 2017

yujuhong commented May 22, 2017

yujuhong May 22, 2017

wking Feb 20, 2018

yujuhong May 22, 2017

yujuhong May 22, 2017

karataliu commented May 23, 2017

yujuhong commented May 23, 2017

karataliu commented May 24, 2017

feiskyer left a comment

feiskyer commented May 24, 2017

yujuhong commented May 24, 2017

yujuhong commented May 24, 2017

karataliu commented May 25, 2017

feiskyer commented May 25, 2017

k8s-ci-robot commented May 25, 2017 •

edited

yujuhong commented May 25, 2017

yujuhong commented May 25, 2017 •

edited

mtaufen commented May 25, 2017

k8s-github-robot commented May 25, 2017

k8s-github-robot commented May 26, 2017

Support TCP type runtime endpoint for kubelet #46089

Support TCP type runtime endpoint for kubelet #46089

Conversation

karataliu commented May 19, 2017

k8s-ci-robot commented May 19, 2017

yujuhong commented May 19, 2017

yujuhong commented May 19, 2017

karataliu commented May 22, 2017

feiskyer commented May 22, 2017

yujuhong commented May 22, 2017

yujuhong May 22, 2017

Choose a reason for hiding this comment

wking Feb 20, 2018

Choose a reason for hiding this comment

yujuhong May 22, 2017

Choose a reason for hiding this comment

yujuhong May 22, 2017

Choose a reason for hiding this comment

karataliu commented May 23, 2017

yujuhong commented May 23, 2017

karataliu commented May 24, 2017

feiskyer left a comment

Choose a reason for hiding this comment

feiskyer commented May 24, 2017

yujuhong commented May 24, 2017

yujuhong commented May 24, 2017

karataliu commented May 25, 2017

feiskyer commented May 25, 2017

k8s-ci-robot commented May 25, 2017 • edited

yujuhong commented May 25, 2017

yujuhong commented May 25, 2017 • edited

mtaufen commented May 25, 2017

k8s-github-robot commented May 25, 2017

k8s-github-robot commented May 26, 2017

k8s-ci-robot commented May 25, 2017 •

edited

yujuhong commented May 25, 2017 •

edited