charts: Using downwardAPI to mount labels to the proxy container #4199

Pothulapati · 2020-03-25T09:20:04Z

This PR updates yaml manifests to include pod's metadata.labels as a volume, for the linkerd-proxy container to use.

Changes Include

Adds env, volume and volumeMounts to the manifests using downwardAPI, when tracing is enabled through .Values.global.proxy.trace
Add a Inject unit test for the same
Include linkerd.io/workload-ns as a label that is added during proxy injection

@grampelberg

Signed-off-by: Tarun Pothulapati tarunpothulapati@outlook.com

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati · 2020-03-25T09:29:38Z

Traces would have the pod labels as additional attributes like

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update test files Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati · 2020-04-02T16:49:26Z

@alpeb This PR adds a new ENV variable for labels file along with a volumeMount of the pod labels. Use tarunpothulapati/l5d-proxy as the proxy image (along with the tracing component, and control-plane-tracing enabled) to see it working, as it includes the changes from linkerd/linkerd2-proxy#463

Complete installation command would be ./target/cli/linux/linkerd install --ignore-cluster --addon-config config.yaml --control-plane-tracing --proxy-image tarunpothulapati/l5d-proxy --proxy-version latest | k apply -f -

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati · 2020-04-14T20:12:13Z

@alpeb My Initial thinking was to leave the control-plane components, as they already had linkerd-io/controlplane-ns and it would be redundant. I was planning to do some js magic to handle those cases in the dashboard. But I guess its better to be consistent, So added those labels. :)

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati · 2020-04-15T06:19:37Z

@alpeb Looks like updating the label selectors for control plane components is going to hurt us for upgrades kubernetes/kubernetes#26202 (comment) (Upgrade tests fail too)

But, This should not effect us for injection, as we add labels to the pod but not the workload.

alpeb · 2020-04-15T15:02:56Z

@alpeb Looks like updating the label selectors for control plane components is going to hurt us for upgrades kubernetes/kubernetes#26202 (comment) (Upgrade tests fail too)

But, This should not effect us for injection, as we add labels to the pod but not the workload.

Ok you're right. I thought replacing the deployment with changes in the label selectors would swap the entire deployment and that wouldn't be considered a mutation, but I do see the error when I attempt doing that:

The Deployment "linkerd-controller" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"foo":"bar", "linkerd.io/control-plane-component":"controller", "linkerd.io/control-plane-ns":"linkerd", "linkerd
.io/proxy-deployment":"linkerd-controller"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

So this means we have to rollback doing that to the control plane components and rely on the javascript magic you mentioned before?

Pothulapati · 2020-04-15T15:07:02Z

@alpeb Yep, Doing that.

P.S: It feels really bad to have this blocker to update labels, as it could be a common use-case/change.

This reverts commit 6023ac4. Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

alpeb · 2020-04-15T19:56:51Z

@Pothulapati seems like you missed including partials.proxy.volumes.labels in smi-metrics.yaml?

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

alpeb

Thanks @Pothulapati , this looks and tests fine to me! 👍 🎖️

adleong

It took me a while to review this just because it took me a while to wrap my head around what was happening here. In the end, it's pretty simple, but it seems more complex than it is. I think we should definitely leave some comments behind here because it can be pretty confusing, especially if you don't have all the right context.

In particular there were a few things that are not obvious that we should spend extra effort documenting:

It's not obvious why we want a workload-ns label. After all, the namespace is already a field in the metadata. We should explain that adding the namespace as a label is the only way to send this value to the container through the downward api
It's surprising that the workload-ns label is not part of the proxy labels partial. At first I thought this was a mistake. We should explain that we omit this to avoid changing the control plane deployment label selectors, since doing so would cause problems for upgrades. This is super non-obvious.
In general, the labels volume could use some comments describing what it is and what it's for.

Other than adding some documentation, this looks correct to me.

charts/partials/templates/_volumes.tpl

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati · 2020-04-17T14:15:04Z

@adleong Thanks, That's a really good suggestion!

As the changes are spread across multiple files, I added a comment in volumes.tpl with the full context. Feel free to comment if you have any changes in your mind! :)

adleong

Thanks for adding that comment! Maybe move the explanation about workload-ns to the proxy labels partial to explain why the label should not be put there? If I was reading this code my instinct would be to "fix" it by moving the workload-ns label into the proxy labels partial, so it would be good to have a comment there to stop me.

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati · 2020-04-17T19:09:57Z

@adleong I added a caution comment in partials/metadata.tpl :)

adleong

Thanks!

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Followup to #4271 Add missing annotation `linkerd.io/workload-ns: linkerd` in in the addons test fixtures, introduced the downward work from #4199

Followup to #4271 Add missing annotation `linkerd.io/workload-ns: linkerd` in in the addons test fixtures, introduced by the downward work from #4199

use downward API to mount labels to the proxy container as a volume

87f377c

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati requested review from alpeb and zaharidichev as code owners March 25, 2020 09:20

Pothulapati mentioned this pull request Mar 25, 2020

Add Labels from a path as oc-collector attributes linkerd/linkerd2-proxy#463

Merged

Pothulapati requested a review from adleong as a code owner March 25, 2020 09:43

Pothulapati force-pushed the downward-labels branch 3 times, most recently from de2e4c6 to e81a534 Compare March 25, 2020 12:08

Formatting fix

62c4cce

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati force-pushed the downward-labels branch from e81a534 to 62c4cce Compare March 25, 2020 12:31

Pothulapati added 4 commits March 25, 2020 18:05

Fix formatting of templates

bbbe9dc

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update uninject code to remove the podinfo volume

57887b5

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update golden test files

fbaaf8c

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update controller inject files

f85bbd3

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati requested a review from kleimkuhler as a code owner March 25, 2020 13:45

Pothulapati added 2 commits March 25, 2020 19:17

Merge branch 'master' into downward-labels

e310587

update proxy injector code to add volumes by default

4a2691a

update test files Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati force-pushed the downward-labels branch from de69e33 to 4a2691a Compare March 25, 2020 16:24

Pothulapati added 4 commits March 26, 2020 22:53

add a new labels env path

588e0a9

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update go test files

89e329e

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Merge branch 'master' into downward-labels

f42ee8f

keep addrootvolume flag in helm chart

681b66a

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati force-pushed the downward-labels branch from 4bbd2ba to 681b66a Compare April 1, 2020 05:57

Merge branch 'master' into downward-labels

1c6325d

Pothulapati force-pushed the downward-labels branch from 79d126f to 1c6325d Compare April 1, 2020 18:33

Pothulapati added 3 commits April 2, 2020 20:42

allow patch value setting even when identity is nil

9f3446a

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update golden files for proxy injector

838c3b9

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update integration test golden files

a6abfe6

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati added 2 commits April 15, 2020 01:26

add controlPlaneTracing condition to volumeMounts

dbf5317

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

add workload-ns label for control-plane components

6023ac4

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update add-ons to have workload-ns

5594216

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati added 2 commits April 15, 2020 21:29

Revert "add workload-ns label for control-plane components"

f09b66e

This reverts commit 6023ac4. Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

add workload-ns label to control-plane components

6d36844

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

add podinfo volume to smi-metrics

91c790d

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

alpeb approved these changes Apr 15, 2020

View reviewed changes

adleong mentioned this pull request Apr 15, 2020

Add source metadata RFC linkerd/rfc#15

Open

adleong reviewed Apr 16, 2020

View reviewed changes

charts/partials/templates/_volumes.tpl Show resolved Hide resolved

Pothulapati added 2 commits April 17, 2020 18:20

Add more documentation on the approach

345e098

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

lower case comments and formatting

3f5c252

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

adleong reviewed Apr 17, 2020

View reviewed changes

add a caution comment at paritals/metadata.tpl

a96d441

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

adleong approved these changes Apr 17, 2020

View reviewed changes

Pothulapati added 2 commits April 21, 2020 00:02

Merge branch 'master' into downward-labels

f56e3da

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

update new golden files

ea6651e

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

Pothulapati force-pushed the downward-labels branch from 89b69f5 to ea6651e Compare April 20, 2020 19:05

alpeb merged commit 2b1cbc6 into linkerd:master Apr 22, 2020

Pothulapati deleted the downward-labels branch April 22, 2020 15:50

alpeb added a commit that referenced this pull request Apr 23, 2020

Added missing annotations to addon test fixtures

9218415

Followup to #4271 Add missing annotation `linkerd.io/workload-ns: linkerd` in in the addons test fixtures, introduced the downward work from #4199

alpeb mentioned this pull request Apr 23, 2020

Added missing annotations to addon test fixtures #4286

Merged

alpeb added a commit that referenced this pull request Apr 23, 2020

Added missing annotations to addon test fixtures

dd4863b

Followup to #4271 Add missing annotation `linkerd.io/workload-ns: linkerd` in in the addons test fixtures, introduced the downward work from #4199

alpeb added a commit that referenced this pull request Apr 23, 2020

Added missing annotations to addon test fixtures (#4286)

dacf87e

Followup to #4271 Add missing annotation `linkerd.io/workload-ns: linkerd` in in the addons test fixtures, introduced by the downward work from #4199

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

charts: Using downwardAPI to mount labels to the proxy container #4199

charts: Using downwardAPI to mount labels to the proxy container #4199

Pothulapati commented Mar 25, 2020 •

edited

Loading

Pothulapati commented Mar 25, 2020

Pothulapati commented Apr 2, 2020 •

edited

Loading

Pothulapati commented Apr 14, 2020

Pothulapati commented Apr 15, 2020

alpeb commented Apr 15, 2020

Pothulapati commented Apr 15, 2020

alpeb commented Apr 15, 2020

alpeb left a comment

adleong left a comment

Pothulapati commented Apr 17, 2020

adleong left a comment

Pothulapati commented Apr 17, 2020

adleong left a comment

charts: Using downwardAPI to mount labels to the proxy container #4199

charts: Using downwardAPI to mount labels to the proxy container #4199

Conversation

Pothulapati commented Mar 25, 2020 • edited Loading

Pothulapati commented Mar 25, 2020

Pothulapati commented Apr 2, 2020 • edited Loading

Pothulapati commented Apr 14, 2020

Pothulapati commented Apr 15, 2020

alpeb commented Apr 15, 2020

Pothulapati commented Apr 15, 2020

alpeb commented Apr 15, 2020

alpeb left a comment

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

Pothulapati commented Apr 17, 2020

adleong left a comment

Choose a reason for hiding this comment

Pothulapati commented Apr 17, 2020

adleong left a comment

Choose a reason for hiding this comment

Pothulapati commented Mar 25, 2020 •

edited

Loading

Pothulapati commented Apr 2, 2020 •

edited

Loading