fix(discovery.relabel): Use the underlying string of secret target values#6605
fix(discovery.relabel): Use the underlying string of secret target values#6605Churi12 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes discovery.relabel target decoding so secret-capsule values (notably local.file.<name>.content) are converted to their underlying string instead of Go’s default struct formatting (e.g., {true value}), aligning runtime behavior with user expectations and the referenced bug report.
Changes:
- Special-case
alloytypes.Secretandalloytypes.OptionalSecretinTarget.ConvertFromto unwrap to the underlying string before the%vfallback. - Add a regression test that covers non-secret
OptionalSecret, secretOptionalSecret, andSecretdecoding into aTarget.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/component/discovery/target.go | Unwraps secret capsule types to their wrapped string when converting map[string]syntax.Value into discovery target label values. |
| internal/component/discovery/target_test.go | Adds regression coverage ensuring secret capsule target values decode to the expected string form. |
kalleep
left a comment
There was a problem hiding this comment.
Hey it think it's and oversight that we even convert secrets here in the first place, even though the value is not usable.
IMO we should not convert Secrets at all and only convert optional secrets if IsSecretm == false.
…g them
When a value such as local.file.<name>.content was used as a target value in
a discovery.relabel targets block, the capsule was rendered via the Go structs
default formatting (for example "{false value}") instead of the string it wraps.
Target.ConvertFrom only special-cased plain strings and fell back to
fmt.Sprintf("%v", ...) for everything else, so the OptionalSecret and Secret
capsule types exported by components like local.file printed as their struct
representation.
A secret is never usable as a target value, so this now unwraps a non-secret
OptionalSecret to its underlying string and rejects a Secret or a secret
OptionalSecret with a clear error rather than exposing its contents in a label.
Fixes grafana#6163
52a9b2b to
7da2b4f
Compare
|
Good call, you are right. Converting a secret here only produced a usable label by stripping the protection the type is meant to carry, and the capsule contract already forbids secret to string conversion everywhere else. I pushed a new version: a non-secret OptionalSecret (the common local.file case with is_secret = false) is unwrapped to its underlying string, and a Secret or a secret OptionalSecret is now rejected with a clear error instead of being unwrapped. That keeps the struct formatting bug from #6163 fixed for the legitimate case while never leaking a secret into a target label. Updated the test accordingly: the non-secret case still asserts the string, and the two secret cases now assert an error. |
Brief description of Pull Request
A secret-typed value such as
local.file.<name>.contentused as a target value in adiscovery.relabeltargets block was rendered as the Go struct's default formatting (for example{true value}) instead of the string it wraps.Pull Request Details
Target.ConvertFrombuilds the label set from amap[string]syntax.Value. It only special-cased plain strings and fell back tofmt.Sprintf("%v", ...)for every other value:local.fileexports itscontentas analloytypes.OptionalSecret{IsSecret bool, Value string}, which is a capsule, not a string, so it hit the%vbranch and printed as its struct representation:{false value}, or{true value}whenis_secret = true. The reported workaround was to wrap it inconvert.nonsensitive().This unwraps the two secret capsule types to their underlying string before the
%vfallback. The value returned is the same string the type already held, so nothing that was previously hidden becomes newly exposed: the old code already printed that string inside the{...}struct form.Issue(s) fixed by this Pull Request
Fixes #6163
Notes to the Reviewer
I considered fixing this generically (attempting a capsule to string conversion for any capsule), but
OptionalSecret.ConvertInto(*string)deliberately errors for actual secrets, so a generic path would either drop secret values or reintroduce the struct formatting. Special-casing the two secret types keeps the behavior explicit and matches how these values are already used as target values in practice.Added
TestDecodeMapWithSecretValuescovering a non-secretOptionalSecret, a secretOptionalSecret, and aSecret. Confirmed the test fails onmain(produces{true value}) and passes with this change.PR Checklist