Enable iptables -w in kubeadm selfhosted #46372

cmluciano · 2017-05-24T18:45:49Z

Currently containerized kube-proxy cannot support iptables -w
unless the xtables.lock is mounted.

Related: #46103

Signed-off-by: Christopher M. Luciano cmluciano@us.ibm.com

Special notes for your reviewer:

I need to figure out how to do some pre-setup to touch the file if it does not exist.
Release note:

support iptables -w in kubeadm containerized kube-proxy

cmluciano · 2017-05-24T18:46:42Z

cc @bboreham @dcbw

justinsb · 2017-05-25T14:25:51Z

I approve, but it's only a soft approve as I don't know this code very well :-)

One option to skip the file-already-exists problem is to mount /run, which I believe we can assume is always there?

cmluciano · 2017-05-25T16:28:53Z

Thanks @justinsb . I did not go with this option initially since @bboreham mentioned that it would probably introduce other bugs #46103 (comment).

luxas

Have you tested that this fixes the issue?

Basically LGTM

luxas · 2017-05-30T07:18:05Z

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

@@ -87,6 +90,9 @@ spec:
      - name: kube-proxy
        configMap:
          name: kube-proxy
+      - name: xtables-lock
+        hostpath:


I think it should be hostPath

luxas · 2017-05-30T07:19:08Z

I'm deferring to @bboreham on this one as he's the author of the linked issue

bboreham · 2017-05-30T12:47:46Z

This part is still open:

I need to figure out how to do some pre-setup to touch the file if it does not exist.

AFAIK this would be somewhere like /etc/systemd/system/kubelet.service.d/10-kubeadm.conf, which I think gets delivered by the package(s). And I have no idea where they live.

cmluciano · 2017-05-30T17:46:37Z

@luxas Any pointers on where I would do the touch for making sure that the file exists?

cmluciano · 2017-05-30T20:26:35Z

I can't filter through the log files

@k8s-bot pull-kubernetes-unit test this

0xmichalis · 2017-06-03T11:30:05Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

luxas · 2017-06-05T15:22:01Z

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

@@ -64,6 +64,16 @@ spec:
      labels:
        k8s-app: kube-proxy
    spec:
+      initContainers:
+      - name: touch-lock
+        image: busybox


Must be an image that's compatible with all architectures

May be related #46820

@luxas Do we have some examples of a generic cross ARCH image?

@ixdy ^

I think we have a gcr.io image of it somewhere. The hard question is security updates for that one @timstclair

The image needs to be templated to be cross arch. For this type of simple command, we usually just use busybox (though I'd like to switch to alpine). E.g.

kubernetes/cluster/images/etcd/Makefile

Line 37 in 25aed0a

ifeq ($(ARCH),amd64)

luxas · 2017-06-05T15:24:18Z

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

@@ -87,6 +100,9 @@ spec:
      - name: kube-proxy
        configMap:
          name: kube-proxy
+      - name: xtables-lock
+        hostPath:
+          path: /run/xtables.lock


Is this a file or directory? I suppose file. Will k8s always mount it as a file as well?

luxas · 2017-06-05T15:25:30Z

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

+      - name: touch-lock
+        image: busybox
+        command: ['/bin/touch', '/run/xtables.lock']
+      securityContext:


Should this be for pod or init container?

Good point, it should probably just be put at the pod level

luxas · 2017-06-05T15:26:20Z

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

+        command: ['/bin/touch', '/run/xtables.lock']
+      securityContext:
+        privileged: true
+      volumeMounts:


Meant for the init container, right?

yes, the run mount is only for the init container

luxas · 2017-06-05T15:26:34Z

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

+        privileged: true
+      volumeMounts:
+      - mountPath: /run
+        name: run


I don't see any run volume

cmluciano · 2017-06-05T16:46:29Z

This seems relevant certificates.go:48] Failed to start certificate controller: open /etc/kubernetes/ca/ca.pem: no such file or directory . I'm wondering if this needs to be mounted into the kube-proxy container.

luxas

LGTM for everything but the busybox constant.

cmluciano · 2017-06-09T16:45:29Z

xref: #47212

cmluciano · 2017-06-09T19:48:42Z

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

@@ -64,6 +64,14 @@ spec:
      labels:
        k8s-app: kube-proxy
    spec:
+      initContainers:
+      - name: touch-lock
+        image: {{ .ImageRepository }}/busybox-{{ .Arch }}


@timstclair @luxas does this look better?

@ixdy We'd have to push an gcr.io/google_containers/busybox-${ARCH} image then (totally possible), don't know if that's feasible from a security standpoint (we'd have to keep it up to date)

cc @timstclair @thockin Please chime in with your thoughts here...

While it would be great to get this merged, I'm inclined to await #46597 to land and then I can remove the init container.

@cmluciano How critical is this? Can we afford not having this in v1.7?
Where is the related bug/issue?

If kube-proxy is containerized, I believe that it will not properly function. cc @bboreham for confirmation

If it doesn't work at all, it's a release blocker

We should use the upstream busybox images. gcr.io/google-containers won't be kept up to date.

@cmluciano Actually you can use upstream images, but that will add more logic. In this case it seems like the only approach.
So add {{ .BusyboxImage }} or similar and set it conditionally:

GOARCH Image

amd64 busybox

arm armhf/busybox

arm64 aarch64/busybox

ppc64le ppc64le/busybox

s390x s390x/busybox

Let me test with #47212 quick and see if that will work as an interim

cmluciano · 2017-06-12T23:47:10Z

/retest

job is marked as flaky

luxas · 2017-06-13T08:16:01Z

#47212 would be the better solution yeah

Currently containerized kube-proxy cannot support iptables -w unless the xtables.lock is mounted. Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

cmluciano · 2017-06-13T15:55:33Z

Ok all tests are passing now @luxas. I am going to push up a TODO on this line to rebase on #46597 when it is merged and I opened kubernetes/kubeadm#298 to track it.

cmluciano · 2017-06-13T17:12:43Z

/retest

seems like a cleanup issue

luxas · 2017-06-13T17:36:51Z

cc @dchen1107 @yujuhong @MrHohn @freehan FYI
Implementation of #47212 for kubeadm.

This is a bug fix for v1.7

luxas · 2017-06-13T17:37:24Z

/lgtm

k8s-github-robot · 2017-06-13T17:52:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cmluciano, luxas

Associated issue: 46103

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

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

~~cmd/kubeadm/OWNERS~~ [luxas]

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

luxas · 2017-06-13T19:01:25Z

/retest

k8s-github-robot · 2017-06-13T23:53:42Z

Automatic merge from submit-queue (batch tested with PRs 47084, 46016, 46372)

@cmluciano

Automatic merge from submit-queue (batch tested with PRs 47451, 47410, 47598, 47616, 47473) kubeadm: Fix kube-proxy regression caused by #46372 **What this PR does / why we need it**: Fixes: kubernetes/kubeadm#306 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: Required for kubeadm v1.7 to work **Release note**: ```release-note NONE ``` @kubernetes/sig-cluster-lifecycle-pr-reviews @cmluciano

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

k8s-github-robot assigned krousey and justinsb May 24, 2017

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 24, 2017

luxas mentioned this pull request May 30, 2017

Running kube-proxy in a container breaks iptables locking #46103

Closed

luxas reviewed May 30, 2017

View reviewed changes

luxas self-assigned this May 30, 2017

cmluciano force-pushed the cml/updateproxykubeadm branch from 218c487 to ae8f230 Compare May 30, 2017 17:41

0xmichalis added this to the v1.7 milestone Jun 3, 2017

cmluciano force-pushed the cml/updateproxykubeadm branch from ae8f230 to 9030969 Compare June 5, 2017 15:10

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 5, 2017

luxas reviewed Jun 5, 2017

View reviewed changes

cmluciano force-pushed the cml/updateproxykubeadm branch from 9030969 to 4ffbbfe Compare June 5, 2017 15:43

cmluciano force-pushed the cml/updateproxykubeadm branch from 4ffbbfe to bc2f558 Compare June 5, 2017 19:55

luxas reviewed Jun 8, 2017

View reviewed changes

marun added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jun 8, 2017

cmluciano force-pushed the cml/updateproxykubeadm branch from bc2f558 to 4e06358 Compare June 9, 2017 19:48

cmluciano commented Jun 9, 2017

View reviewed changes

cmluciano force-pushed the cml/updateproxykubeadm branch from 4e06358 to 8bc3c8a Compare June 12, 2017 22:14

Enable iptables -w in kubeadm selfhosted

289c37a

Currently containerized kube-proxy cannot support iptables -w unless the xtables.lock is mounted. Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

cmluciano force-pushed the cml/updateproxykubeadm branch from 8bc3c8a to 289c37a Compare June 13, 2017 15:55

luxas changed the title ~~WIP: Enable iptables -w in kubeadm selfhosted~~ Enable iptables -w in kubeadm selfhosted Jun 13, 2017

luxas added the kind/bug Categorizes issue or PR as related to a bug. label Jun 13, 2017

luxas approved these changes Jun 13, 2017

View reviewed changes

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

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

k8s-github-robot merged commit 0a1b7d9 into kubernetes:master Jun 13, 2017

luxas mentioned this pull request Jun 15, 2017

kube-proxy isn't run with privileged: true at HEAD kubernetes/kubeadm#306

Closed

luxas added a commit to luxas/kubernetes that referenced this pull request Jun 15, 2017

kubeadm: Fix kube-proxy regression caused by kubernetes#46372

8628856

serbrech mentioned this pull request Nov 23, 2018

Mount /run/x-tables lockfile so that we don't interfere with other container touching iptables Azure/aad-pod-identity#124

Closed

neolit123 mentioned this pull request Nov 5, 2020

Re-configuring kube-proxy to not be a privileged container #96280

Closed

GOARCH	Image
amd64	busybox
arm	armhf/busybox
arm64	aarch64/busybox
ppc64le	ppc64le/busybox
s390x	s390x/busybox

Enable iptables -w in kubeadm selfhosted #46372

Enable iptables -w in kubeadm selfhosted #46372

Conversation

cmluciano commented May 24, 2017

cmluciano commented May 24, 2017

justinsb commented May 25, 2017

cmluciano commented May 25, 2017

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented May 30, 2017

bboreham commented May 30, 2017

cmluciano commented May 30, 2017

cmluciano commented May 30, 2017

0xmichalis commented Jun 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmluciano commented Jun 5, 2017

luxas left a comment

Choose a reason for hiding this comment

cmluciano commented Jun 9, 2017

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

cmluciano commented Jun 12, 2017

luxas commented Jun 13, 2017

cmluciano commented Jun 13, 2017 • edited Loading

cmluciano commented Jun 13, 2017

luxas commented Jun 13, 2017

luxas commented Jun 13, 2017

k8s-github-robot commented Jun 13, 2017

luxas commented Jun 13, 2017

k8s-github-robot commented Jun 13, 2017

cmluciano commented Jun 13, 2017 •

edited

Loading