Add volume and volumeMount for buster-based proxy-init by cpretzer · Pull Request #4692 · linkerd/linkerd2

cpretzer · 2020-07-01T06:16:44Z

Subject

Add volume and volumeMount resources for the proxy-init container which uses buster as described in proxy-init PR 3

Problem

The buster image uses iptables 1.8.x which attempts to lock a file named /run/xtables.lock in order to safely manage iptables updates to the system. This also requires using iptables-legacy to support backwards compatibility.

The securityPolicy of the proxy-init container sets readOnlyRootFileSytem to true, and the buster-based proxy-init container fails to start because it can't create /run/xtables.lock.

Solution

This PR adds a volumeMount to the proxy-init container with using subPath to ensure that /run/xtables.lock is writeable by the container. In addition, an emptyDir volume resource is added to the pod spec to make the volumeMount available to the container.

Many of the template files have been updated to ensure that the injected/patched pod spec output correctly reflects the changes required by these additions.

This change touches how the service account volume mount is handled in values.go. The SAMountPath struct is renamed to a more abstract VolumeMountPath and a field named SubPath has been added. The ProxyInit struct has a new field named XTMountPath which is of type VolumeMountPath and instances of this field are instantiated in install.go and inject.go.

uninject.go includes a check for the volume named InitXtablesLockVolumeMountName, as defined in labels.go.

Validation

These changes were validated by running bin/tests with kind on a laptop. All test ran successfully.

A second validation was done by manually installing linkerd from the binary generated from this branch and adding the --init-image-version and --init-image flags to make sure that the changes are compatible with the changes in the proxy init PR

During this test:

the emojivoto app was injected and confirmed that it works as expected
verified that linkerd dashboard works
verified that the grafana dashboard is available
confirmed that metrics were logged to prometheus
confirmed that the proxy-init container started without any errors and the iptables were written properly
verified that linkerd stat and linked tap work for the emojivoto app and linkerd control plane components
verified that the changes are backwards compatible with proxy-init:v1.3.3 (the current release version)

Signed-off-by: Charles Pretzer charles@buoyant.io

alpeb

Thanks for the very comprehensive changeset 👍
Just one question: why a volume mount using subPath? My understanding is that that's used only when doing multiple mounts drawn from the same volume...

alpeb · 2020-07-01T19:37:17Z

charts/partials/templates/_proxy-init.tpl

+- mountPath: {{.Values.global.proxyInit.xtMountPath.mountPath}}
+  name: {{.Values.global.proxyInit.xtMountPath.name}}
+  subPath: {{.Values.global.proxyInit.xtMountPath.subPath}}


TIOLI: To be consistent with the changes for the volume declarations in the other files, we could wrap this with {{ if not .Values.global.cniEnabled -}}, but that might turn things a bit complicated as we also have to check for .Values.global.proxyInit.saMountPath to decide whether the volumeMounts section is rendered at all...

Good catch @alpeb I added those statements.

The reason I used subPath is to limit the volume to just /run/xtables.lock rather than the entire /run directory. I may be misusing subPath and will change it if that's not the right way to limit the scope of the volume

Right, in that case I believe the right format would be

mountPath: /run/xtables.lock name: linkerd-proxy-init-xtables-lock subPath: xtables.lock

but even so, it doesn't feel very kosher to me given xtables.lock doesn't even exist when this gets mounted. IMO we should leave things simpler and omit subPath.

alpeb · 2020-07-02T11:31:03Z

charts/add-ons/tracing/templates/tracing.yaml

          name: {{ printf "%s-config-val" .Values.collector.name}}
      - {{- include "partials.proxy" . | indent 8 | trimPrefix (repeat 7 " ") }}
-      {{ if not .Values.global.noInitContainer -}}
+      {{ if not .Values.global.cniEnabled -}}


alpeb · 2020-07-02T11:39:08Z

charts/partials/templates/_proxy-init.tpl

+- mountPath: {{.Values.global.proxyInit.xtMountPath.mountPath}}
+  name: {{.Values.global.proxyInit.xtMountPath.name}}
+  subPath: {{.Values.global.proxyInit.xtMountPath.subPath}}


Right, in that case I believe the right format would be

mountPath: /run/xtables.lock name: linkerd-proxy-init-xtables-lock subPath: xtables.lock

but even so, it doesn't feel very kosher to me given xtables.lock doesn't even exist when this gets mounted. IMO we should leave things simpler and omit subPath.

cpretzer · 2020-07-02T17:17:55Z

TIL @alpeb 😄 I'll remove the subPath

cpretzer · 2020-07-07T18:23:17Z

@alpeb I've removed the subPath references and rebased from main after the last edge release

alpeb

@cpretzer It seems you missed a change for charts/linkerd2/templates/smi-metrics.yaml

Signed-off-by: Charles Pretzer <charles@buoyant.io>

Pothulapati

LGTM and tried the same with the init image from linkerd/linkerd2-proxy-init#3 and it works without failing.

Pothulapati · 2020-07-09T07:41:26Z

charts/partials/templates/_volumes.tpl

+
+{{ define "partials.proxyInit.volumes.xtables" -}}
+emptyDir: {}
+name: linkerd-proxy-init-xtables-lock


How about using .Values.global.proxyInit,xMountPath.name directly here too? so that we don't have to maintain multiple copies of this?

Good catch 👍

* Add volume and volumeMount for buster-based proxy-init Signed-off-by: Charles Pretzer <charles@buoyant.io> Signed-off-by: Eric Solomon <errcsool@engineer.com>

cpretzer requested a review from a team as a code owner July 1, 2020 06:16

cpretzer requested review from Pothulapati and alpeb July 1, 2020 06:19

alpeb approved these changes Jul 1, 2020

View reviewed changes

alpeb reviewed Jul 2, 2020

View reviewed changes

cpretzer force-pushed the cpretzer/add-xtables-emptydir branch 2 times, most recently from 6eb85d2 to e5951f0 Compare July 7, 2020 18:22

alpeb reviewed Jul 7, 2020

View reviewed changes

cpretzer added 4 commits July 7, 2020 15:34

Add volume and volumeMount for buster-based proxy-init

9d1558c

Signed-off-by: Charles Pretzer <charles@buoyant.io>

Add conditional statements and cleanup references to noInitContainer

7a3e9aa

Signed-off-by: Charles Pretzer <charles@buoyant.io>

Rebase from main

3ca2fca

Signed-off-by: Charles Pretzer <charles@buoyant.io>

Add volume to smi-metrics.yaml

b84b867

Signed-off-by: Charles Pretzer <charles@buoyant.io>

cpretzer force-pushed the cpretzer/add-xtables-emptydir branch from 25cf600 to b84b867 Compare July 7, 2020 22:34

alpeb approved these changes Jul 8, 2020

View reviewed changes

alpeb mentioned this pull request Jul 8, 2020

Update Dockerfile to use buster linkerd/linkerd2-proxy-init#3

Merged

Pothulapati approved these changes Jul 9, 2020

View reviewed changes

cpretzer merged commit d3553c5 into main Jul 9, 2020

cpretzer deleted the cpretzer/add-xtables-emptydir branch July 9, 2020 16:55

cpretzer mentioned this pull request Jul 9, 2020

set the xtables mountPath name from a variable #4745

Merged

alpeb added a commit that referenced this pull request Jul 9, 2020

Add /run mount introduced in #4692

ec454c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add volume and volumeMount for buster-based proxy-init#4692

Add volume and volumeMount for buster-based proxy-init#4692
cpretzer merged 4 commits intomainfrom
cpretzer/add-xtables-emptydir

cpretzer commented Jul 1, 2020 •

edited

Loading

Uh oh!

alpeb left a comment

Uh oh!

alpeb Jul 1, 2020

Uh oh!

cpretzer Jul 2, 2020

Uh oh!

alpeb Jul 2, 2020

Uh oh!

alpeb Jul 2, 2020

Uh oh!

alpeb Jul 2, 2020

Uh oh!

cpretzer commented Jul 2, 2020

Uh oh!

cpretzer commented Jul 7, 2020

Uh oh!

alpeb left a comment

Uh oh!

Pothulapati left a comment

Uh oh!

Pothulapati Jul 9, 2020

Uh oh!

alpeb Jul 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cpretzer commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Subject

Problem

Solution

Validation

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

alpeb Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

cpretzer Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

alpeb Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

alpeb Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

alpeb Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

cpretzer commented Jul 2, 2020

Uh oh!

cpretzer commented Jul 7, 2020

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

Pothulapati left a comment

Choose a reason for hiding this comment

Uh oh!

Pothulapati Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

alpeb Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cpretzer commented Jul 1, 2020 •

edited

Loading