-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(tracing): Add proxy tracing configuration to control plane helm chart #13994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
IIUC, this should be followed up as well by parametrizing the deployment of the injector in linkerd-jaeger, disabling it by default? |
62c78bc to
812c616
Compare
87a4795 to
d28fd75
Compare
d28fd75 to
aa54bd9
Compare
alpeb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There still are issues with the whitespace suppression; when enabling tracing I see:
- emptyDir:
medium: Memory
name: linkerd-identity-end-entity- name: linkerd-podinfo
downwardAPI:
items:
- path: labels
fieldRef:
fieldPath: metadata.labelsaa54bd9 to
4364e08
Compare
|
should be fixed, there was some extra whitespace trimming that the templates were doing |
a7390fc to
238b25a
Compare
alpeb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting tracing.enable: true while leaving endpoint empty, produces the following error:
$ k -n linkerd logs -f linkerd-identity-6b66cd9c8b-c4bt7 linkerd-proxy
time="2025-06-05T15:11:38Z" level=info msg="Found pre-existing key: /var/run/linkerd/identity/end-entity/key.p8"
time="2025-06-05T15:11:38Z" level=info msg="Found pre-existing CSR: /var/run/linkerd/identity/end-entity/csr.der"
[ 0.001322s] INFO ThreadId(01) linkerd2_proxy: release 2.300.0 (67dc85a) by linkerd on 2025-06-02T22:14:32Z
[ 0.002305s] ERROR ThreadId(01) linkerd_app::env::types: Not a valid address:
[ 0.002311s] ERROR ThreadId(01) linkerd_app::env: LINKERD2_PROXY_TRACE_COLLECTOR_SVC_ADDR="" is not valid: AddrError(MissingPort)
Is this expected? What do you think about adding some validation at the template level, like what we have in _validate.tpl?
238b25a to
b89b429
Compare
|
That's super handy, I hadn't done much with helm template validation so that'll help prevent the biggest configuration footgun here. |
alpeb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sfleen , LGTM! 👍
…chart Previously, this would require the `linkerd-jaeger` extension to be installed. This comes with its own set of issues, namely managing yet another component of linkerd. Since the tracing config is basically just environment variables and one volume mount, hoisting them up into the main control plane helm chart for the proxy injector to handle is likely the simplest and most maintainable path going forward. The injector from the `linkerd-jaeger` extension will still work for the time being, and is interoprable to some degree as long as the injector in the extension is disabled. A follow-up to this PR should add documentation around this and how users can migrate from the extension to these configs. Signed-off-by: Scott Fleener <[email protected]>
b89b429 to
f5eca66
Compare
Previously, this would require the
linkerd-jaegerextension to be installed. This comes with its own set of issues, namely managing yet another component of linkerd.Since the tracing config is basically just environment variables and one volume mount, hoisting them up into the main control plane helm chart for the proxy injector to handle is likely the simplest and most maintainable path going forward.
The injector from the
linkerd-jaegerextension will still work for the time being, and is interoprable to some degree as long as the injector in the extension is disabled. A follow-up to this PR should add documentation around this and how users can migrate from the extension to these configs.