[collector] Specify the collector's configuration with structure yaml instead of a string

[TylerHelmuth]

Currently the only way to define the collector's config in the OpentlemetryCollector resource is via a string in the Spec.Config field. Although it works, it can be frustrating to write, especially for complex collector configurations, since indentation is important. For operator contributors and maintainers working with the string internally is also challenging.

I believe it would be beneficial to allow the config to be expressed as explicit yaml, mirroring the collector's internal config structure. This will allow users a better configuration experience when managing OpentlemetryCollector resources and will allow operator contributors and maintainers and typed struct to manipulate internally.

Since changing Spec.Config would be a significant breaking change, I propose the following process:

1. Update the operator's internals to convert the string to a structured representation of the collector config and use the struct for all operations. This is a non-breaking change to users and prepares us for the next steps.
2. Introduce a new field such as ConfigSpec that is a structured representation of the collector config. If this field is set, use it it for all the operations. To avoid the need to coalesce, require exactly one of Config and ConfigSpec to be set.
3. Following the deprecation process, deprecate Config.
4. (Optional) Follow a rename process to rename ConfigSpec to Config.

[jaronoff97]

I am a big fan of this idea! I think the scope of it is great as well.

As we discussed during our last SIG meeting, I would like to keep the config spec such that it cannot be layered or separated from the CRD at this time. A few ideas were proposed to create an OpenTelemetryConfig CRD that a collector could pull in, but this brings up a myriad of concerns around discoverability and understanding that worry me. The main question I imagine is someone asking "what configuration is my collector actually running" at which point we would need to shrug and pull together the various layers in just the right order. I believe the onus of reusable collector configuration blocks is on the cluster manager and not on the operator.

[swiatekm]

Sounds like a good plan to me as well.

For the prospective OpenTelemetryConfig CRD, I don't think we need to add it now, but we should think about it while we're here. The way I imagined it, is that we'd keep the ability to specify the config directly in the Collector CR, but create a Config CR in the background to represent it, and also have an "effective config" computed field on the Collector. This would mostly be transparent for users just wanting the simple case, and would free us to add more complex config resolution logic in the Config CR without disturbing those simpler use cases.

[jaronoff97]

@swiatekm-sumo i'd prefer we don't add complex config resolution logic at all – there are already too many ways to configure the collector in kubernetes and I'm not sure i see the value in adding another right now. I'd be happy to be proven / shown wrong though, open to feedback in this area.

[frzifus]

I'm not quite sure I understand the outcome of this discussion. But here is my take 🐌

We can overwrite the collector's internal config structure Unmarshal method and support a native yaml configuration as well as parsing a single string into the same structure. This should be done during the validation process in the webhook.
To avoid rewriting logic on everyplace the current string field is used, we might add a String() method that can be used.

The final result should be not noticeable for an end user. But both configs should be valid:

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: simplest
spec:
-  config: |
+  config:
    receivers:
      otlp:
        protocols:
          http:
    exporters:
      logging:
    service:
      pipelines:
        traces:
          receivers: [otlp]
          exporters: [logging]

wdyt @TylerHelmuth @swiatekm-sumo @jaronoff97 ?

@iblancasa are you still working on this task? Otherwise, I could submit a PR for it.

[iblancasa]

@frzifus I'm working on it.

To avoid rewriting logic on everyplace the current string field is used, we might add a String() method that can be used.

I'm not sure about this. Why do we want to convert the struct all the time to string? It sounds a little bit like a workaround for me to not fix the unmarshalling/marshalling problem.

[frzifus]

I'm not sure about this. Why do we want to convert the struct all the time to string?

Simply to avoid having to adapt all the functions that are currently based on string representation.
I would recommend doing this in a further step.

It sounds a little bit like a workaround for me to not fix the unmarshalling/marshalling problem.

What problem do you have in mind?

[iblancasa]

What problem do you have in mind?

Basically what @TylerHelmuth suggested:

Update the operator's internals to convert the string to a structured representation of the collector config and use the struct for all operations. This is a non-breaking change to users and prepares us for the next steps.

[frzifus]

It breaks this step down into two smaller changes.

1. Providing an internal structure that could be used to operate on, but still provides you a string representation if needed. (That way you do not need to touch all the functionality that is build on top. Like the config validation or the mods done for the targetAllocator).
2. Changing it on all the places we benefit from a native yaml representation. (This can result in multiple tasks and way smaller PRs then having all in one)

I have not tested the following lines, but they roughly represent what I was thinking of:

func MustParseConfigSpec(in string) *ConfigSpec {
	cfg := &ConfigSpec{}
	if err := cfg.UnmarshalJSON([]byte(in)); err != nil {
		panic(fmt.Sprintf("could not parse collector config: %s", err))
	}
	return cfg
}

func ParseConfigSpec(in string) (*ConfigSpec, error) {
	cfg := &ConfigSpec{}
	if err := cfg.UnmarshalJSON([]byte(in)); err != nil {
		return nil, fmt.Errorf("could not parse collector config: %s", err)
	}
	return cfg, nil
}

type ConfigSpec struct {
	otelcol.Config `json:"internal"`
}

func (c *ConfigSpec) String() string {
	v, err := json.Marshal(c)
	if err != nil {
		return ""
	}
	return string(v)
}

func (c *ConfigSpec) UnmarshalJSON(data []byte) error {
	var rawConf map[string]any
	if err := yaml.Unmarshal(data, &rawConf); err != nil {
		return err
	}
	cm := confmap.NewFromStringMap(rawConf)
	cfg := otelcol.Config{}
	if err := component.UnmarshalConfig(cm, &cfg); err != nil {
		return err
	}
	c.Config = cfg
	return nil
}

[iblancasa]

I'm not a big fan of using something from a package that is not a library. But, anyway, I spent some time trying your approach and we need to use a lot of objects from the collector in order to unmarshal properly the structure.

I think we need a much simpler approach like the one proposed by @TylerHelmuth. I'll continue with his approach since it is easier to implement and maintain.

[andreasgerstmayr]

I'm not a big fan of using something from a package that is not a library. But, anyway, I spent some time trying your approach and we need to use a lot of objects from the collector in order to unmarshal properly the structure.

I think it makes sense to re-use the https://pkg.go.dev/go.opentelemetry.io/[email protected]/otelcol#Config data type, otherwise this struct and all nested structs need to be duplicated (and kept in sync) in the operator?

And this way you could run Validate() (https://pkg.go.dev/go.opentelemetry.io/[email protected]/otelcol#Config.Validate) in the webhook.

[pavolloffay]

On the last SIG meeting there was agreement to bump CRD API version and in the new version drop the string config and expose only the typed config.

[frzifus]

Linking this here: #2182 (review)

[jaronoff97]

This will be a breaking change and will be the primary driver of our milestone