Should ConfigValidator be in confmap?

[bogdandrutu]

Is your feature request related to a problem? Please describe.

Currently the ConfigValidator is part of the component package, and for example in mdatagen where we use confmap it is a bit strange to depend on the component.ConfigValidator for config validation.

Describe the solution you'd like

Provide the configuration Validator as part of the confmap repository, and probably make it part of Conf.Unmarshal (configurable) to also do validation.

Describe alternatives you've considered

Leave where it is right now.

[TylerHelmuth]

Is it correct to say from a component developer perspective it doesn't matter where it lives?

[sfc-gh-bdrutu]

Is it correct to say from a component developer perspective it doesn't matter where it lives?

Yes, but if we want to use confmap in other places like I said in mdatagen it is confusing.

[dmitryax]

I agree confmap seems like a better place for it

[mx-psi]

I don't have a strong opinion on this (which makes me lean slightly into "leave it as it is")

[TylerHelmuth]

probably make it part of Conf.Unmarshal (configurable) to also do validation.

Today I believe that otelcol is our collector's use of ConfigValidator. What problem would we be solving by adding something configurable to Conf.Unmarshal?

[mx-psi]

Thinking about this again, I feel like it makes sense to move this to confmap since this should evolve with confmap and if we do a confmap/v2 we would want to be able to change this interface alongside it

[mx-psi]

@bogdandrutu If we were to move this, what would we do with component.ValidateConfig?

[dmitryax]

Do we even need component.ValidateConfig? I can't see any value in it...

[mx-psi]

@dmitryax Here are the places where this is used: https://github.com/search?q=org%3Aopen-telemetry+language%3AGo+component.ValidateConfig&type=code

There are a lot of usages that could be avoided/converted, but I am not confident that we would be able to remove all of them.

[atoulme]

We discussed this issue during the stability SIG meeting of 1/6 and came up with the following suggestions:

1. Move the validation code out of component to an experimental package (prefixed with x)
2. Look at whether this validation code should be moved to stable, or use a new approach, using for example https://github.com/go-playground/validator so we don't implement our own validation framework. A new issue for this will be cut to continue the work.

[TylerHelmuth]

I'll submit a PR to move validation stuff to xcomponent.

[TylerHelmuth]

@bogdandrutu @atoulme @dmitryax would adding support for https://github.com/go-playground/validator be a breaking change? Config validation is an important part of collector 1.0 and the Validator interface is a sufficient solution. I think we could move forward with our Validator interface for 1.0 and add support for https://github.com/go-playground/validator as a feature add after.

[TylerHelmuth]

Thinking about ConfigValidator and ValidateConfig some more I would prefer not to move ConfigValidator to an xcomponent as I don't believe it should be considered experimental. ConfigValidator is implemented widely by end-user's custom components and it is a sufficient solution for component developers to enforce config validation.

If we must move ConfigValidator, I'd prefer to move it directly to confmap instead of xcomponent.

ValidateConfig is a helper function that allows checking if a Config (any) implements ConfigValidator, and executes Validate() it if it can. This is used in otelcol and a lot of tests. While otelcol and all those test functions could implement this feature themselves, I believe it is beneficial to provide this capability to our users.

Component also defines the public type Config any that ValidateConfig config uses, but is also used throughout otelcol modules to denote component config, such as in otelcol.Config:

// Config defines the configuration for the various elements of collector or agent.
type Config struct {
	// Receivers is a map of ComponentID to Receivers.
	Receivers map[component.ID]component.Config `mapstructure:"receivers"`

	// Exporters is a map of ComponentID to Exporters.
	Exporters map[component.ID]component.Config `mapstructure:"exporters"`

	// Processors is a map of ComponentID to Processors.
	Processors map[component.ID]component.Config `mapstructure:"processors"`

	// Connectors is a map of ComponentID to connectors.
	Connectors map[component.ID]component.Config `mapstructure:"connectors"`

	// Extensions is a map of ComponentID to extensions.
	Extensions map[component.ID]component.Config `mapstructure:"extensions"`

	Service service.Config `mapstructure:"service"`
}

I agree that if confmap could do what ValidateConfig does as part of its unmarshal then we may not need it anymore.

Here is what I propose we do to move forward without any experimental packages.

1. Continue with the PoC of https://github.com/go-playground/validator. Validation with tags would be cool and could help with doc generation. I believe this is a feature-add that should be worked independently of this issue - there are too many custom components in the world today for us to abandon the ConfigValidator interface entirely.
2. Duplicate ConfigValidator in confmap and add the functionality that ValidateConfig provides directly to Conf.Unmarshal.
3. If we like the solution from step 2, deprecate and eventually remove component.ConfigValidator and component.ValidateConfig.

This plan leaves component.Config in kind of an awkward place as there wont be anything in component that uses it. The ergonomics it provides to otelcol, connectors, and other public structs is really nice tho, so I say leave it.

[atoulme]

@bogdandrutu @atoulme @dmitryax would adding support for https://github.com/go-playground/validator be a breaking change? Config validation is an important part of collector 1.0 and the Validator interface is a sufficient solution. I think we could move forward with our Validator interface for 1.0 and add support for https://github.com/go-playground/validator as a feature add after.

I think we can build a shim with a ConfigValidator impl that calls out to Validator as part of a POC to see what the end user experience is.

[atoulme]

#12027 is open for the validator POC.

[TylerHelmuth]

I ran into an issue today while implementing validation in the Unmarshal. While it does work for invoking the Validate function of components and Service, we would need to make some changes to the way otelcol handles config unmarshalling in order to get otelcol.Config.Validate to be invoked.

This is because otelcol doesn't ask confmap to unmarshal into a otelcol.Config but into a otelcol.configSettings, which otelcol then uses to create the otelcol.Config. otelcol then manually calls otelcol.Config.Validate. There is validation logic in otelcol.Config.Validate, such has checking that pipelines list only configured components, that we need to invoke, so we'd either need to keep that particular check in place or modify otelcol's unmarshaling logic to use a otelcol.Config.

[TylerHelmuth]

There is also an issues when components implement custom Unmarshal functions.

For example, the otlpreceiver implements a custom unmarshaller than requires that the user have set grpc or http in their config. If the user supplies

receivers:
  otlp:

the collector should fail validation.

The custom unmarshaller first calls confmap.Unmarshal, which, assuming we've updated confmap to handle invoking Validate(), will then calls the receiver's Validate function. When this happens the current cfg value is the receiver's default config, so validation passes. Then the custom unmarshal logic removes the default http/grpc values since the user did not provide their keys, resulting in an invalid config that passed validation.

[TylerHelmuth]

I was able to work around the custom unmarshal issue by updating the cases in the reflective validate logic. The confmap hook handling the custom unmarshalling logic does resolve as part of decodeConfig, which means the resulting unmarshalled object can get passed correctly into confmap's validate logic after the custom unmarshal has been complete.

This means the components validate function may get executed multiple times. I believe that is safe.

[TylerHelmuth]

Created #12031 to add ConfigValidator to confmap and hook it up. After working on it and seeing some complications because of how otelcol does unmarshalling I'm not convinced this is enough of an improvement over the existing solution to warrant the effort.