[extension/opamp] Redact values in effective config

[evan-bradley]

Description:

Redacts primitive values in the effective config reported by the extension.

Necessary for #31641 until open-telemetry/opentelemetry-collector#10139 is resolved.

Relates to #32983

[srikanthccv]

Can processors be excluded from the redaction? The ability to see the processor's original config and the sequence of processors under a pipeline would be great because when we (me and the internal team I work with) look at the effective config, it's usually the processors and their config to troubleshoot the output data-related issues. I am probably wrong about this but there aren't any processors with secrets in the config?

[evan-bradley]

Can processors be excluded from the redaction?

Unfortunately one of the resource detection processor detectors has a secret in the config.

We could loosen the requirements to pass through values in arrays, which aren't used as secrets anywhere. Would this help your case, or do you need full processor configs?

If absolutely necessary, I could put this functionality behind an on-by-default feature gate, though I would like to avoid that if possible.

[srikanthccv]

We could loosen the requirements to pass through values in arrays, which aren't used as secrets anywhere. Would this help your case, or do you need full processor configs?

is it going to be arrays with primitive data types only or arrays of structs (for example, sampling policies) would also be excluded?

[tigrannajaryan]

It would be great to add some sort of annotation mechanism in the future to allow components to declare which parts of their config needs to be redacted and which part can stay as is.

[evan-bradley]

is it going to be arrays with primitive data types only or arrays of structs (for example, sampling policies) would also be excluded?

@srikanthccv I think we would probably just terminate recursing into the map once we hit an array, so any array values would be unredacted. I will need to double check that no configopaque.String values may be present in these fields.

It would be great to add some sort of annotation mechanism in the future to allow components to declare which parts of their config needs to be redacted and which part can stay as is.

@tigrannajaryan We have this today with configopaque.String, but the work to properly marshal the config will require possibly extensive changes to the Collector APIs. We agreed in yesterday's WG meeting that this is an acceptable step to get us through until those changes are available.

[evan-bradley]

@srikanthccv One example I found of secrets inside arrays is that the Prometheus receiver has secrets in its scrape configs. I would prefer not to make guesses for which values may be secret or not, is there another way we can make this work for you?

[evan-bradley]

@jaronoff97 I'd also appreciate your feedback here, I know the Operator uses the OpAMP extension. Will this change cause major issues for users?

[jaronoff97]

@evan-bradley right now the operator only uses the opamp-go repo for this, so the bridge shouldn't be effected by this change. The bridge gets around all of this because we're not loading any potential secrets/env vars and just taking exactly what the user gave prior to any replacements. Taking a look at this, I agree with Tigran that having some mechanism to annotate what should and shouldn't be redacted would be valuable. I understand that doing so is probably not the simplest and is fine to not include in this initial version.

Is there a reason we can't just use the confmap's Marshal function which should do the redaction correctly? I saw this test in core that made me think this was possible. This would be different than today where we just do a blind convert to a map[string]any.

[evan-bradley]

The issue is that the marshaling logic in confmap only reads in mapstructure tags, which means that third-party config structs (namely those provided by Prometheus) that use yaml tags get improperly marshaled. When marshaling from the confmap representation into YAML, this then causes errors.

I investigated this and ended up at a dead end in this PR. We will need to either update the marshaling/unmarshaling library the Collector uses (moving away from mapstructure and to a YAML library) or update our use of mapstructure.

[srikanthccv]

In addition to the default service::pipelines, it would be great to have an option to add additional paths to skip redaction. I have found a temporary workaround, so this change isn't blocking and any improvements can be done later.

[BinaryFissionGames]

This LGTM.

This is actively blocking work on the supervisor, so I would like for us to merge this. This provides a very restrictive starting point that we can relax further in separate PRs.

[evan-bradley]

Thanks everyone for your input. I think we are all in agreement, so I will merge this. Since it is so restrictive, any changes going forward will be non-breaking and should be more straightforward.