Skip to content

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

Merged
alpeb merged 50 commits intolinkerd:masterfrom
Pothulapati:downward-labels
Apr 22, 2020
Merged

charts: Using downwardAPI to mount labels to the proxy container#4199
alpeb merged 50 commits intolinkerd:masterfrom
Pothulapati:downward-labels

Conversation

@Pothulapati
Copy link
Contributor

@Pothulapati Pothulapati commented Mar 25, 2020 •
edited
Loading

Fixes #4008

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 [email protected]

@Pothulapati Pothulapati mentioned this pull request Mar 25, 2020
Merged
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Mar 25, 2020

Traces would have the pod labels as additional attributes like
Screenshot_2020-03-25 Jaeger UI
image

@Pothulapati Pothulapati requested a review from adleong as a code owner March 25, 2020 09:43
@Pothulapati Pothulapati force-pushed the downward-labels branch 3 times, most recently from de2e4c6 to e81a534 Compare March 25, 2020 12:08
@Pothulapati
Formatting fix
62c4cce 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Fix formatting of templates
bbbe9dc 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update uninject code to remove the podinfo volume
57887b5 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update golden test files
fbaaf8c 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update controller inject files
f85bbd3 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Apr 2, 2020 •
edited
Loading

@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 -

@Pothulapati
Copy link
Contributor Author

Pothulapati commented Apr 14, 2020

@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. :)

@Pothulapati
update add-ons to have workload-ns
5594216 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Apr 15, 2020

@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
Copy link
Member

alpeb commented Apr 15, 2020

@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
Copy link
Contributor Author

Pothulapati commented Apr 15, 2020

@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.

@alpeb
Copy link
Member

alpeb commented Apr 15, 2020

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

@Pothulapati
add podinfo volume to smi-metrics
91c790d 
Signed-off-by: Tarun Pothulapati <[email protected]>
alpeb
alpeb approved these changes Apr 15, 2020
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@adleong adleong mentioned this pull request Apr 15, 2020
Open
adleong
adleong reviewed Apr 16, 2020
View reviewed changes
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Pothulapati
Copy link
Contributor Author

Pothulapati commented Apr 17, 2020

@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
adleong reviewed Apr 17, 2020
View reviewed changes
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Pothulapati
Copy link
Contributor Author

Pothulapati commented Apr 17, 2020

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

adleong
adleong approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Pothulapati
Merge branch 'master' into downward-labels
f56e3da 
Signed-off-by: Tarun Pothulapati <[email protected]>
@Pothulapati
update new golden files
ea6651e 
Signed-off-by: Tarun Pothulapati <[email protected]>
@alpeb alpeb merged commit 2b1cbc6 into linkerd:master Apr 22, 2020
@Pothulapati Pothulapati deleted the downward-labels branch April 22, 2020 15:50
alpeb added a commit that referenced this pull request Apr 23, 2020
@alpeb
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 alpeb mentioned this pull request Apr 23, 2020
Merged
alpeb added a commit that referenced this pull request Apr 23, 2020
@alpeb
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
@alpeb
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alpeb alpeb alpeb approved these changes

@adleong adleong adleong approved these changes

@zaharidichev zaharidichev Awaiting requested review from zaharidichev

@kleimkuhler kleimkuhler Awaiting requested review from kleimkuhler

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Owner Metadata for Spans.

3 participants

@Pothulapati @alpeb @adleong