[extension/opamp] Add opt-in raw configuration reporting - #49692
Conversation
… config
Implement the extensioncapabilities.ConfigSnapshotWatcher interface and add an
opt-in `reports_raw_config` capability (default false, requires
reports_effective_config). When enabled, the raw, unexpanded configuration is
reported under a new "raw" key of the OpAMP effective config map, while the
fully expanded effective configuration is left unchanged under the "" (empty)
key for backward compatibility.
Disabled by default because raw config files may contain hardcoded secrets;
provider-sourced secrets such as ${env:TOKEN} are redacted by core, but literal
secrets written into the config are not.
Migrates from the deprecated ConfigWatcher.NotifyConfig to
ConfigSnapshotWatcher.NotifyConfigSnapshot, which exposes both the effective and
unexpanded configuration (core support added in
open-telemetry/opentelemetry-collector#15399).
Related to open-telemetry#44341
Assisted-by: Claude Opus 4.8
Signed-off-by: hitkall <kalluruhitesh3@gmail.com>
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
|
|
/workflow-approve |
| // configuration are not. Only enable this if you trust the OpAMP server with | ||
| // the full contents of your configuration files. It is disabled by default | ||
| // for this reason. | ||
| ReportsRawConfig bool `mapstructure:"reports_raw_config"` |
There was a problem hiding this comment.
@hitkall can you please move this outside of the capabilities struct? This struct should match the capabilities as defined in the OpAMP specification and this new flag is not one of them. I think this should be in the Config struct.
There was a problem hiding this comment.
Done in 8e5a144. Moved ReportsRawConfig to the top-level Config struct, so it's now a top-level reports_raw_config setting instead of living under capabilities. Updated the schema, README, and tests to match. @douglascamata
| This is disabled by default because raw configuration files may contain | ||
| hardcoded secrets; secrets sourced from providers such as `${env:TOKEN}` are | ||
| redacted, but literal secrets written into the configuration are not. |
There was a problem hiding this comment.
I think this sentence is slightly incorrect. From what I see, anything (not only secrets) provided from env var references like ${env:TOKEN} will retain their unexpanded form.
There was a problem hiding this comment.
You're right, my wording was off. It's not redaction. Anything sourced from a provider reference like ${env:TOKEN} just stays in its unexpanded form in the raw config, secrets or not, so it's never exposed in the first place. Fixed the wording in the chloggen entry, README, config comment, and schema in 8e5a144.
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | |
There was a problem hiding this comment.
I would also mention here that configuration fields that are using types meant for opaque information (like sqlreceiver.password and many others) are redacted. Users should consult the documentation/source of their components to verify what's automatically redacted or not.
There was a problem hiding this comment.
@douglascamata Added in 8e5a144, the chloggen subtext and README now mention that fields using opaque types (like password fields) are redacted, and that users should check their components' docs/source to see what's automatically redacted.
Pull request dashboard statusWaiting on the author · refreshed 2026-07-31 21:30 UTC Respond to 5 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
…ction docs Assisted-by: Claude Fable 5
hitkall
changed the title
|
/workflow-approve |
|
/workflow-approve |
|
@hitkall I'm about to approve this, can you please clean up the PR description? There's a mix of duplication and bad formatting that makes it very difficult to read. Also it seems like you got hit by an intermittent data race failure in the build that I fixed in another PR that was recently merged (like yday), so please rebase and push again for us to get a clean build here. I know the problem is not introduced by this PR. |
…/hitkall/opentelemetry-collector-contrib into opampextension-report-raw-config
Sorry for the delay, I was able to update the description. Please go ahead and approve |
|
/workflow-approve |
|
Hey @hitkall @douglascamata, I wonder if the proposed implementation violates the OpAMP spec configuration part: Does it mean that in case there is more than a single config, empty can't be used? |
|
I think we might need to update the spec to accommodate this, because it's not a new config file per-se in the Collector, but another special/reserved key (besides the If other maintainers disagree with this idea we might have to consider a different way to implement this. The OpAMP spec says this about what the contents of the files in the config map should have:
But the I'll bring this point for conversation in the |
|
Not an OpAMP maintainer, but as someone who's implemented an OpAMP server, I've always found it confusing how the "raw file name → raw file contents" data model described in the spec is quite different from what's actually implemented, so I would support a spec change clarifying "reserved" keys and their semantics. |
We can remove the word "raw" from OpAMP spec. I don't think it is necessary there. The config recorded in AgentConfigMap, including the key names and config content is expected to be entirely agent-type specific and the spec is not expected any opinion. The spec also says it is OK to have an empty key, which is essentially another way of saying the spec doesn't have an opinion about keys. We could consider evolving the spec such that it does have an opinion about keys and there is an equivalent of "semconv" but for config key names. I don't know if we want to do that. I think it is fine if the Collector's spec defines what keys and values the Collector uses in OpAMP payloads. |
|
@tigrannajaryan I think the issue with the spec is that, given how it's written today, each key in the configmap should be a file (with the extra that if there's only one file it can be unnamed, through the empty string key). @assafad1's concern is still valid an unanswered though. Given what the spec says (see below) I kind of understand it as: if, and only if, there's one entry in the configmap, it can be unnamed (in the
|
|
To summarize my previous comment, I'm in favor of changing two specific points in the OpAMP spec:
Please let me know if you agree. @jade-guiton-dd I really like your feedback here because, even though you contributed a few PRs in the area, I think you have the least "contributor" bias out of all of us. So your perspective is very valuable. :) |
|
I agree on both points. If the intent is for the keys and values to both be arbitrary, maybe it would be good to use more general wording like configuration "objects" instead of "files", and clarify that the keys have implementation-defined semantics and may be empty under any circumstance |
|
I agree with both points. We can remove "raw" from spec wording and rename "file" to "object". Please feel free to submit a PR in opamp-spec.
That is not intentional. Can be reworded to "empty key is allowed", regardless of whether it is the only key or there is others. |
5 participants
Description
Adds an opt-in top-level reports_raw_config setting to the opamp extension. When enabled, the extension reports the raw config as the user wrote it (before env vars and other provider references get expanded) under a new raw key in the effective config map. The expanded effective config stays unchanged under the "" key, so nothing changes for existing users.
The point is that a server can now tell what was actually authored vs what's a default filled in during expansion, which the effective config alone can't answer.
To get the unexpanded config I moved the extension off the deprecated ConfigWatcher.NotifyConfig onto ConfigSnapshotWatcher.NotifyConfigSnapshot, which core added in open-telemetry/opentelemetry-collector#15399 and exposes both versions of the config.
It defaults to false because raw files can have secrets hardcoded in them. Values from provider references like ${env:TOKEN} keep their unexpanded form in the raw config, so they're not exposed — but a literal secret typed into the yaml would go out as-is, which is why this is a deliberate opt-in. Fields using opaque types (like passwords) are redacted; users should check their components' docs for what's covered. It also requires reports_effective_config — validation errors out otherwise. If marshaling the raw config ever fails, the effective config under "" still gets reported.
Link to tracking issue
Fixes #44341
Testing
Added validation tests for the new setting (with and without
reports_effective_config— the latter should fail). ExtendedTestComposeEffectiveConfigto checkrawdoesn't show up when disabled. AddedTestComposeEffectiveConfigWithRawConfigcovering: enabled but nothing captured yet (only""reported), and both keys once a raw config is captured — including checking the${env:OTLP_ENDPOINT}reference comes through unexpanded inrawwhile""has the expanded value. Also a table-driven test forNotifyConfigSnapshotwith the flag on/off.Documentation
Documented the new setting in the README with a warning about the secrets risk, added it to
config.schema.yaml, and included a changelog entry.