use init container load kernel modules in kube-proxy #56732

Lion-Wei · 2017-12-02T01:55:59Z

What this PR does / why we need it:

kube-proxy ipvs mode need load kernel modules ip_vs/ip_vs_* . Now we do this in kube-proxy container, but have some safety concern. Had some discussion in this pr: #52003 . Mostly think use init container load kernel modules is a good idea.

This pr to use init container load kernel modules instead of load those modules inside kube-proxy container, so that we can lower the kube-proxy container permission.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #54497

Special notes for your reviewer:

Release note:

NONE

Lion-Wei · 2017-12-02T01:57:03Z

/cc @MrHohn @cmluciano @m1093782566 @vfreex

k8s-ci-robot · 2017-12-02T01:57:05Z

@Lion-Wei: GitHub didn't allow me to request PR reviews from the following users: vfreex.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @MrHohn @cmluciano @m1093782566 @vfreex

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.

dims · 2017-12-02T19:59:49Z

/ok-to-test

bowei · 2017-12-03T00:40:15Z

Shouldn't this be loaded only if the IPVS proxier is enabled?

Lion-Wei · 2017-12-04T01:10:53Z

Thanks for replaying, @bowei
Yes, I use --proxy-mode=ipvs and mode: ipvs to identify kube-proxy ipvs mode in init container, with command:

echo "{{params}}" | grep "\-\-proxy-mode=ipvs" && modprobe ...
cat /var/lib/kube-proxy/config.conf | grep "mode: ipvs" && modprobe ...

cmluciano

few comments

cmluciano · 2017-12-04T03:40:13Z

cmd/kubeadm/app/phases/addons/proxy/manifests.go

@@ -175,6 +175,20 @@ spec:
        - mountPath: /run/xtables.lock
          name: xtables-lock
          readOnly: false
+	  initContainers:


Does this spacing align with the containers line?

No....
Thanks for pointing out.

cmluciano · 2017-12-04T03:43:34Z

cmd/kubeadm/app/phases/addons/proxy/manifests.go

+        command:
+        - /bin/sh
+        - -c
+        - cat /var/lib/kube-proxy/config.conf | grep "mode: ipvs" &&


Instead of parsing the file on disk, can we pull in the component configmap ?

using configmap to config conf will be better

Thanks for reviewing. I tried to use configmap. Kubeadm created kube-proxy configmap with a struct:

apiVersion: v1 data: config.conf: |- apiVersion: kubeproxy.config.k8s.io/v1alpha1 ... mode: ipvs ...

In which config.conf is a key with a bug value. I can use this key get all config.conf value and see it include ipvs: mode or not. But can't get mode field. Is there any other way of using configmap?

cmluciano · 2017-12-04T03:45:39Z

cmd/kubeadm/app/phases/addons/proxy/manifests.go

+        - -c
+        - cat /var/lib/kube-proxy/config.conf | grep "mode: ipvs" &&
+          modprobe -a ip_vs ip_vs_rr ip_vs_wrr ip_vs_sh nf_conntrack_ipv4 ||
+          echo "didn't load ipvs related kernel modules"


Can we use an if statement and use an explicit success/failure return code?

I thought this can be simpler. And I worried if the command cat /var/lib/kube-proxy/config.conf | grep "mode: ipvs" return non zero, the init container will exist with error.

But if we specified that we want IPVS mode, wouldn't we want that to happen? Otherwise we will log the error but continue on to the regular container and fallback to iptables/userspace.

cc @MrHohn @bowei

Yeah, that's the original logic. If can't use ipvs mode, give a warning and slightly fallback to iptables to make sure kube-proxy still can work.
We can talk about it, to chose a more reasonable logic.
@m1093782566

If failed to load kernel modules, IPVS proxier can't work. In order to make kube-proxy work, we let kube-proxy fall back to iptables or userspace mode... That's NOT a silent action, kube-proxy will log the reason.

Any problems?

Cite a case, if user specify iptables mode but kernel is not compatible or iptables version too low, kube-proxy will fall back to userspace mode.

MrHohn · 2017-12-04T22:26:51Z

cluster/addons/kube-proxy/kube-proxy-ds.yaml

+          readOnly: true
+      initContainers:
+      - name: init-kube-proxy
+        image: gcr.io/google_containers/busybox


Please use a versioned image. Probably gcr.io/google_containers/busybox:1.27.2?

Thanks, fixed.

MrHohn · 2017-12-04T22:32:59Z

cluster/addons/kube-proxy/kube-proxy-ds.yaml

@@ -57,6 +57,19 @@ spec:
        - mountPath: /run/xtables.lock
          name: xtables-lock
          readOnly: false
+          readOnly: true


Duplicate readOnly field?

m1093782566 · 2017-12-05T05:37:03Z

pkg/proxy/ipvs/proxier.go

@@ -693,15 +693,6 @@ func (em proxyEndpointsMap) unmerge(other proxyEndpointsMap) {
 // return an error if it fails to get the kernel modules information without error, in which
 // case it will also return false.
 func CanUseIPVSProxier(ipsetver IPSetVersioner) (bool, error) {
-	// Try to load IPVS required kernel modules using modprobe


Why delete these lines? kube-proxy is not always running inside container. If kube-proxy running on host, it can modprobe kernel modules. Besides, if kube-proxy is running inside container and failed to load kernel modules, it just log warning messages - please double check it.

I figured out if kube-proxy run inside the container, it will certainly fail.
But maybe we can accept an warning message. I will take it back.

Yeah, that's why we just log warning message instead of existing with error.

k8s-ci-robot · 2017-12-07T01:44:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Lion-Wei
We suggest the following additional approvers: bowei, matchstick, timothysc

Assign the PR to them by writing /assign @bowei @matchstick @timothysc in a comment when ready.

Associated issue: #52003

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

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

cluster/addons/kube-proxy/OWNERS
cluster/saltbase/salt/kube-proxy/OWNERS
cmd/kubeadm/OWNERS
pkg/proxy/OWNERS

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

gyliu513 · 2017-12-29T03:27:55Z

pkg/proxy/ipvs/proxier.go

@@ -698,7 +698,7 @@ func CanUseIPVSProxier(ipsetver IPSetVersioner) (bool, error) {
 		err := utilexec.New().Command("modprobe", "--", kmod).Run()
 		if err != nil {
 			glog.Warningf("Failed to load kernel module %v with modprobe. "+
-				"You can ignore this message when kube-proxy is running inside container without mounting /lib/modules", kmod)
+				"You can ignore this message when kube-proxy is running inside container", kmod)


What does the warning message mean here? Does it mean that I do not need to load those kernel modules? But this PR is using initContainer to load those modules, seems a bit weird to me, can you help elaborate?

What does the warning message mean here? Does it mean that I do not need to load those kernel modules?

ip_vs_* kernel modules is needed. But, it's unnecessary that we should load it via modprobe by kube-proxy. For example, kube-proxy run inside container and ip_vs_* kernel modules are already installed in host.

But this PR is using initContainer to load those modules, seems a bit weird to me, can you help elaborate?

If kube-proxy is always installed by the scripts/manifests which this PR has covered, we can change the warning to error, but actually it's NOT.

Thanks for replaying, we have this warning when modprobe command return err, if we use init container then we don't need load kernel modules inside kube-proxy container, so can ignore this message. The follow code will check if kernel loaded.

Thanks @Lion-Wei and @m1093782566 , still have some question: If the modprobe return error, then it should means that some modules are not loaded, I think that we should return error for such case, as the ipvs may not work well if some modules are not loaded even running in container?

@gyliu513

If the modprobe return error, then it should means that some modules are not loaded

NO.

Suppose you run kube-proxy in a container. When you execute modprobe ip_vs command, you will get an error since there is no kernel module in container by default. However, you will find some kernel modules in /proc/modules since there are some kernel modules in host.

That's why we do 2 steps:

1 modprobe - log warning if error occur

2 "cut", "-f1", "-d", " ", "/proc/modules" - return error

We should consider both container and non-container cases.

thanks @m1093782566

So if the host has kernel modules in /proc/modules, then we do not need to load those modules in container any more if the container is using security mode?

Got it, as the initContainer already done the work, how about add more info claim that the initContainer already helped to do the modprobe work?

no? kube-proxy process should not be aware of init container? In other words, process should not care how it's deployed - deployed with init container or anything else.

so, why ignore this message when proxy is running in container? How to handle the case if there is no init container and kube proxy do not mount /lib/modules either?

so, why ignore this message when proxy is running in container?

Because users can deploy kube-proxy in anyway they like, they may don't use init container.

How to handle the case if there is no init container and kube proxy do not mount /lib/modules either?

If kube-proxy is running in container and didn't mount /lib/modules from host, it still has a chance to run IPVS mode.

gyliu513 · 2018-01-02T14:16:19Z

cmd/kubeadm/app/phases/addons/proxy/manifests.go

+        - /bin/sh
+        - -c
+        - 'echo $(PROXY_CONFIG) | grep "mode: ipvs" && modprobe -a
+          ip_vs ip_vs_rr ip_vs_wrr ip_vs_sh nf_conntrack_ipv4 || echo "did not load


More comments, do we really need this init container? As the kube-proxy already modprobe the required modules at here https://github.com/kubernetes/kubernetes/blob/release-1.9/pkg/proxy/ipvs/proxier.go#L697-L703 for 1.9?

As we discussed in #52003, Folks believed a better solution is to load kernel modules in init container instead as that has less security concerns, but use this as an alternative solution in first to unblock the IPVS beta work in rebase-1.9.

Thanks @Lion-Wei

Can you please clarify more for the security risk? and how does initContainer can help this?

Why modprobe is needed in initContainer? I see that the kube-proxy can load those modules dynamically.

Lion-Wei · 2018-01-03T06:36:21Z

@MrHohn @luxas . I think it's ready for approve, since this pr need approval from each of these OWNERS Files. : )

Lion-Wei · 2018-01-03T06:40:21Z

/assign @thockin . : )

errordeveloper · 2018-01-10T15:02:22Z

This adds many lines of configuration and a duplicate shell script, all of this code can diverge too easily and very few users would notice. If we want to improve security, I'd say we could ask IPVS users to ensure these modules are present on the nodes (which is easy with kube-up.sh), and we could add a pre-flight check in kubeadm too. This doesn't sound like the kind of thing that needs to be added upstream.

k8s-github-robot · 2018-01-17T08:26:12Z

@Lion-Wei PR needs rebase

freehan · 2018-01-17T18:53:27Z

Wouldn't it make more sense if we split the manifest per mode? I would imagine different kube-proxy implementations will diverge. If every mode uses the same manifest, it will become more and more clunky and harder to maintain.

Probably structure it like this ??

cluster/addons/kube-proxy/kube-proxy-ds.yaml (default, if no specific manifest)
cluster/addons/kube-proxy/iptables/...
cluster/addons/kube-proxy/ipvs/...
cluster/addons/kube-proxy/windows/...

Another long term goal would be to split the kube-proxy binary and transform the common components into a library or interface.

Lion-Wei · 2018-01-18T11:27:09Z

@errordeveloper Sorry for the delay. But I don't think tell user to load those kernel modules is a good idea. We can do this work in script like kube-up.sh but can't cover all solution of build cluster, In kubeadm, user have to load kernel modules in each node, which sounds not very friendly.

Lion-Wei · 2018-01-18T12:07:15Z

@freehan Definitely make sense. IPVS and iptables may have lots different features and different option in the future.
And also, I understand each kube-proxy mode have common component can be extract, but not sure about split the kube-proxy binary.

errordeveloper · 2018-01-22T13:16:03Z

If we really are looking for best user experience, I'd be looking forward to any ideas where we would introduce an API change/extensions that allows pods to somehow specify modules it depends on.
If that sounds like a big feature, perhaps we could at least consider using a dedicated Go program (with a container image of its own) that can load a list of modules, instead of this ad-hoc shell script and busybox image.

Lion-Wei · 2018-01-24T12:46:00Z

@errordeveloper Yeah, I understand your concern, indeed, use busybox and shell script to load kernel modules is very primaeval and unsightly, I'll try to make it more graceful. Thanks for your recommendation.
: )

errordeveloper · 2018-02-01T22:19:24Z

Thanks!

…

On Wed, 24 Jan 2018, 12:46 pm Lion-Wei, ***@***.***> wrote: @errordeveloper <https://github.com/errordeveloper> Yeah, I understand your concern, indeed, use busybox and shell script to load kernel modules is very primaeval and unsightly, I'll try to make it more graceful. Thanks for your recommendation. : ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56732 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPWS9bS-tENBt7btTZpDBQPznVnkHIUks5tNyYMgaJpZM4QzH55> .

rramkumar1 · 2018-02-02T21:09:16Z

@Lion-Wei This needs to be resolved before ipvs goes GA. I think the general sentiment is that putting the modprobe logic in the init container is not ideal. I think the approach that will require the least work from the user's perspective is to bake the logic of loading the relevant modules into the startup scripts. This looks like it would involve plumbing the KUBEPROXY_MODE environment variable around everywhere.

I can work with you on this to make sure that all cases are covered.

m1093782566 · 2018-02-03T01:36:01Z

@rramkumar1 Cool!!

m1093782566 · 2018-02-03T01:57:17Z

Another long term goal would be to split the kube-proxy binary and transform the common components into a library or interface.

I suspect if it's doable because ipvs will fall back to iptables and iptables will fall back to userspace.

Wouldn't it make more sense if we split the manifest per mode? I would imagine different kube-proxy implementations will diverge. If every mode uses the same manifest, it will become more and more clunky and harder to maintain.

Interesting, but I also worry about the proxy mode fall back path.

m1093782566 · 2018-02-03T02:55:02Z

@rramkumar1

Do you mean we don't need to load kernel modules in init container, instead, we should load kernel modules in deployment tools/scripts. @Lion-Wei has already done it for local-up.sh. Should we continue do this for kubeadm, kubernetes-anywhere, gce scripts...?

m1093782566 · 2018-02-04T04:03:40Z

Please check context around #52003 for more information, @gyliu513

Following are some thoughts off the top of my head:

Currently we mount /lib/modules to kube-proxy container. It is going to cause problems if we try to reduce the capability of kube-proxy down to CAP_NET_ADMIN? - I am not pretty sure, there is an interesting article: xref: https://lwn.net/Articles/430462/. That's why we are trying to load kernel modules by init container.
Apart from init container, we can also load kernel modules in start up scripts. Since IPVS is not the default mode for kube-proxy, possibly we can test an environment variable and then load ip_vs_* kernel modules in start-up scripts for now. When IPVS mode graduates to default mode, we can ALWAYS load ip_vs_* kernel modules in kube-proxy start-up scripts.

rramkumar1 · 2018-02-04T15:36:12Z

@m1093782566 Yes, startup scripts. I think it would be great if you guys could open an issue to track this with a checkbox for each piece you need to cover (kubeadm, gce, kubernetes-anywhere). That way, we can easily track which pieces are complete.

m1093782566 · 2018-02-06T03:47:53Z

@Lion-Wei

What's your opinion?

Lion-Wei · 2018-02-06T06:57:43Z

Summarize what we discussed before, to be concise, I think there are three concerns about this pr:

We need a better way to do this walk, like a dedicated Go program with it's own image. @errordeveloper

consider using a dedicated Go program (with a container image of its own) that can load a list of modules, instead of this ad-hoc shell script and busybox image.

We shold load kernel modules in setup cluster phase, not kube-proxy init container. @rramkumar1

I think the approach that will require the least work from the user's perspective is to bake the logic of loading the relevant modules into the startup scripts

We should split the manifest per mode. @freehan

I would imagine different kube-proxy implementations will diverge. If every mode uses the same manifest, it will become more and more clunky and harder to maintain.

I understand use busybox and shell script to load kernel modules is very primaeval and unsightly, but I do think use init container load kernel modules is a better idea then bake the logic of loading the relevant modules into the startup scripts.

But if we decide that's what we should do, I can accept.

@cmluciano @MrHohn @bowei

m1093782566 · 2018-02-06T07:03:07Z

@Lion-Wei

but I do think use init container load kernel modules is a better idea then bake the logic of loading the relevant modules into the startup scripts.

Do not forget we are NOT always running kube-proxy in container. init container may don't work in such case.

rramkumar1 · 2018-02-06T17:08:16Z

@Lion-Wei can we close this?

Lion-Wei · 2018-02-07T03:47:31Z

@rramkumar1 . I'd like leave this open until our work finished, for reference or something else. : )

Lion-Wei · 2018-03-05T02:07:13Z

/close

Lion-Wei · 2018-03-05T02:08:19Z

I think this can be closed.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 2, 2017

k8s-github-robot assigned bowei and freehan Dec 2, 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 Dec 2, 2017

k8s-ci-robot requested review from MrHohn, cmluciano and m1093782566 December 2, 2017 01:57

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. area/ipvs labels Dec 2, 2017

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 Dec 2, 2017

cmluciano reviewed Dec 4, 2017

View reviewed changes

MrHohn reviewed Dec 5, 2017

View reviewed changes

m1093782566 reviewed Dec 5, 2017

View reviewed changes

Use init container load kernel modules for kube-proxy

f82b6e4

kubernetes deleted a comment from k8s-github-robot Dec 7, 2017

gyliu513 reviewed Dec 29, 2017

View reviewed changes

gyliu513 reviewed Jan 2, 2018

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2018

rramkumar1 mentioned this pull request Feb 2, 2018

Declare IPVS-based kube-proxy GA #58442

Merged

m1093782566 mentioned this pull request Feb 6, 2018

Automatically load ipvs required kernel modules in startup scripts #59402

Closed

3 tasks

k8s-ci-robot closed this Mar 5, 2018

use init container load kernel modules in kube-proxy #56732

use init container load kernel modules in kube-proxy #56732

Conversation

Lion-Wei commented Dec 2, 2017

Lion-Wei commented Dec 2, 2017

k8s-ci-robot commented Dec 2, 2017

dims commented Dec 2, 2017

bowei commented Dec 3, 2017

Lion-Wei commented Dec 4, 2017

cmluciano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lion-Wei Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 7, 2017

Choose a reason for hiding this comment

m1093782566 Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lion-Wei commented Jan 3, 2018

Lion-Wei commented Jan 3, 2018

errordeveloper commented Jan 10, 2018 • edited Loading

k8s-github-robot commented Jan 17, 2018

freehan commented Jan 17, 2018 • edited Loading

Lion-Wei commented Jan 18, 2018

Lion-Wei commented Jan 18, 2018

errordeveloper commented Jan 22, 2018

Lion-Wei commented Jan 24, 2018 • edited Loading

errordeveloper commented Feb 1, 2018 via email

rramkumar1 commented Feb 2, 2018 • edited Loading

m1093782566 commented Feb 3, 2018

m1093782566 commented Feb 3, 2018 • edited Loading

m1093782566 commented Feb 3, 2018

m1093782566 commented Feb 4, 2018 • edited Loading

rramkumar1 commented Feb 4, 2018

m1093782566 commented Feb 6, 2018

Lion-Wei commented Feb 6, 2018

m1093782566 commented Feb 6, 2018 • edited Loading

rramkumar1 commented Feb 6, 2018

Lion-Wei commented Feb 7, 2018

Lion-Wei commented Mar 5, 2018

Lion-Wei commented Mar 5, 2018

m1093782566 Dec 8, 2017 •

edited

Loading

Lion-Wei Dec 7, 2017 •

edited

Loading

m1093782566 Dec 29, 2017 •

edited

Loading

m1093782566 Dec 29, 2017 •

edited

Loading

m1093782566 Feb 4, 2018 •

edited

Loading

m1093782566 Feb 4, 2018 •

edited

Loading

errordeveloper commented Jan 10, 2018 •

edited

Loading

freehan commented Jan 17, 2018 •

edited

Loading

Lion-Wei commented Jan 24, 2018 •

edited

Loading

rramkumar1 commented Feb 2, 2018 •

edited

Loading

m1093782566 commented Feb 3, 2018 •

edited

Loading

m1093782566 commented Feb 4, 2018 •

edited

Loading

m1093782566 commented Feb 6, 2018 •

edited

Loading