Use typed config in v1alpha2

[pavolloffay]

Description:

I have as well tried suggestions from kubernetes-sigs/controller-tools#636 (comment)

runtime.RawExtension

	// +kubebuilder:pruning:PreserveUnknownFields
        // +kubebuilder:validation:Schemaless
	Spec interface{} `json:"spec"`

But nothing worked better than byte array

Link to tracking Issue:

Resolves #1707

Testing:

Documentation:

[pavolloffay]

The struct will have methods to simplify parsing - e.g. extract enabled component, extract pipeline name etc.

[jaronoff97]

i was wondering if we could match what the collector's config looks like right prior to the naming of various components. This would mean having explicit fields set for telemetry, pipelines, extensions, etc. (see here).

Something like this:

type Config struct {
	Receivers []receiverConfig `mapstructure:"receivers"`
	Processors []processorConfig `mapstructure:"processors"`
	Exporters []exporterConfig `mapstructure:"exporters"`
	Connectors []connectorConfig `mapstructure:"connectors"`
	Telemetry telemetryConfig `mapstructure:"telemetry"`
	Extensions extensionsConfig `mapstructure:"extensions"`
	Pipelines pipelinesConfig `mapstructure:"pipelines"`
}

which would let us know the type of each component when doing logic like the port parsing

[swiatekm]

On one hand, the more structured we can make this config, the more confident we can be about rejecting invalid ones, which is a major win for users.

The potential negative is that we'll be heavily coupled to the collector at the schema level. A breaking change in the collector config schema would require us to do a corresponding breaking change in the CRD. Maybe that's ok though? Realistically, we'd need to do something like that regardless.

It would also prevent us from easily supporting alternative syntax which the collector may add without breaking changes, like open-telemetry/opentelemetry-collector#9077.

[pavolloffay]

I like @jaronoff97's proposal. I was thinking about the same, but I wanted to do this gradually. I will play with it and add it to the current PR.

[pavolloffay]

A breaking change in the collector config schema would require us to do a corresponding breaking change in the CRD

In the operator, we are trying to resolve breaking changes of the collector anyways. I believe that a strongly typed config will enable us to do.

[jaronoff97]

agreed with pavol. I think if / when the collector does those types of syntax changes we should have first-party support for the version that includes said changes :)

[jaronoff97]

a few small suggestions, the only one I think may be useful prior to merge is using the AnyConfig type

[jaronoff97]

this is great – in the future in the defaulting webhook we could set the empty map using this as well :)

[jaronoff97]

nit: should we just have this work on the AnyConfig type?

[pavolloffay]

here is a recursive call on line 131 if the value is map, so it would not work.

[jaronoff97]

Given that AnyConfig is just map[string]interface{}, wouldn't we just change 131 to assert on v.(AnyConfig)

[jaronoff97]

regardless, i don't think this is blocking so it's fine as is ✅

[jaronoff97]

this is awesome :D

[jaronoff97]

regardless, i don't think this is blocking so it's fine as is ✅