From 4afe23f3b0a56e1f99deb4fe9cbbd75e0a35bf7f Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 10 Mar 2025 10:28:17 -0400 Subject: [PATCH 1/6] [chore] Create RFC for Optional config types --- docs/rfcs/optional-config-type.md | 184 ++++++++++++++++++++++++++++++ 1 file changed, 184 insertions(+) create mode 100644 docs/rfcs/optional-config-type.md diff --git a/docs/rfcs/optional-config-type.md b/docs/rfcs/optional-config-type.md new file mode 100644 index 00000000000..9467d1afaac --- /dev/null +++ b/docs/rfcs/optional-config-type.md @@ -0,0 +1,184 @@ +# Optional[T] type for use in configuration structs + +## Overview + +In Go, types are by default set to a "zero-value", a supported value for the +respective type that is semantically equivalent or similar to "empty", but which +is also a valid value for the type. For many config fields, the zero value is +not a valid configuration value and can be taken to mean that the option is +disabled, but in certain cases it can indicate a default value, necessitating a +way to represent the presence of a value without using a valid value for the +type. + +Using standard Go without inventing any new types, the two most straightforward +ways to accomplish this are: + +1. Using a separate boolean field to indicate whether the field is enabled or + disabled. +2. Making the type a pointer, which makes a `nil` pointer represent that a + value is not present, and a valid pointer represent the desired value. + +Each of these approaches has deficiencies: Using a separate boolean field +requires the user to set the boolean field to `true` in addition to setting the +actual config option, leading to suboptimal UX. Using a pointer value has a few +drawbacks: + +1. It may not be immediately obvious to a new user that a pointer type indicates + a field is optional. +2. The distinction between values that are conventionally pointers (e.g. gRPC + configs) and optional values is lost. +3. Setting a default value for a pointer field when decoding will set the field + on the resulting config struct, and additional logic must be done to unset + the default if the user has not specified a value. +4. The potential for null pointer exceptions is created. + +## Optional types + +Go does not include any form of Optional type in the standard library, but other +popular languages like Rust and Java do. We could implement something similar in +our config packages that allows us to address the downsides of using pointers +for optional config fields. + +## Basic definition + +A production-grade implementation will not be discussed or shown here, but the +basic implementation for an Optional type in Go could look something like: + +```golang +type Optional[T any] struct { + hasValue bool + value T + + defaultVal T +} + +func Some[T any](value T) Optional[T] { + return Optional[T]{value: value, hasValue: true} +} + +func None[T any](value T) Optional[T] { + return Optional[T]{} +} + +func WithDefault[T any](defaultVal T) Optional[T] { + return Optional[T]{defaultVal: defaultVal} +} + +func (o Optional[T]) HasValue() bool { + return o.hasValue +} + +func (o Optional[T]) Value() T { + return o.value +} + +func (o Optional[T]) Default() T { + return o.defaultVal +} +``` + +## Use cases + +Optional types can fulfill the following use cases we have when decoding config. + +### Representing optional config fields + +To use the optional type to mark a config field as optional, the type can simply +be used as a type parameter to `Optional[T]`. The following config struct shows +how this may look, both in definition and in usage: + +```golang +type Protocols struct { + GRPC Optional[configgrpc.ServerConfig] `mapstructure:"grpc"` + HTTP Optional[HTTPConfig] `mapstructure:"http"` +} + +func (cfg *Config) Validate() error { + if !cfg.GRPC.HasValue() && !cfg.HTTP.HasValue() { + return errors.New("must specify at least one protocol when using the OTLP receiver") + } + return nil +} + +func createDefaultConfig() component.Config { + return &Config{ + Protocols: Protocols{ + GRPC: WithDefault(configgrpc.ServerConfig{ + // ... + }), + HTTP: WithDefault(HTTPConfig{ + // ... + }), + }, + } +} +``` + +### Proper unmarshaling of empty values when a default is set + +Currently, the OTLP receiver requires a workaround to make enabling each +protocol optional while providing defaults if a key for the protocol has been +set: + +```golang +type Protocols struct { + GRPC *configgrpc.ServerConfig `mapstructure:"grpc"` + HTTP *HTTPConfig `mapstructure:"http"` +} + +// Config defines configuration for OTLP receiver. +type Config struct { + // Protocols is the configuration for the supported protocols, currently gRPC and HTTP (Proto and JSON). + Protocols `mapstructure:"protocols"` +} + +func createDefaultConfig() component.Config { + return &Config{ + Protocols: Protocols{ + GRPC: configgrpc.NewDefaultServerConfig(), + HTTP: &HTTPConfig{ + // ... + }, + }, + } +} + +func (cfg *Config) Unmarshal(conf *confmap.Conf) error { + // first load the config normally + err := conf.Unmarshal(cfg) + if err != nil { + return err + } + + // gRPC will be enabled if this line is not run + if !conf.IsSet(protoGRPC) { + cfg.GRPC = nil + } + + // HTTP will be enabled if this line is not run + if !conf.IsSet(protoHTTP) { + cfg.HTTP = nil + } + + return nil +} +``` + +With an Optional type, the checks in `Unmarshal` become unnecessary, and it's +possible the entire `Unmarshal` function may no longer be needed. Instead, when +the the config is unmarshaled, no value would be put into the default Optional +values and `HasValue` would return false when using this config object. + +This situation is something of an edge case with our current unmarshaling +facilities and while not common, could be a rough edge for users looking to +implement similar config structures. + +## Disadvantages of an Optional type + +There are two noteworthy disadvantages of introducing an Optional type: + +1. Since the type isn't standard, external packages working with config may + require additional adaptations to work with our config structs. +2. It will still be possible to use pointer types for optional config fields, + and as a result there may be some fragmentation in which type is used to + represent an optional field. From 295b93be2a969be95115c3ddaf3446cc2664e7cb Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 11 Mar 2025 11:57:19 -0400 Subject: [PATCH 2/6] Update docs/rfcs/optional-config-type.md Co-authored-by: Pablo Baeyens --- docs/rfcs/optional-config-type.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/optional-config-type.md b/docs/rfcs/optional-config-type.md index 9467d1afaac..b921f8cb252 100644 --- a/docs/rfcs/optional-config-type.md +++ b/docs/rfcs/optional-config-type.md @@ -166,7 +166,7 @@ func (cfg *Config) Unmarshal(conf *confmap.Conf) error { With an Optional type, the checks in `Unmarshal` become unnecessary, and it's possible the entire `Unmarshal` function may no longer be needed. Instead, when -the the config is unmarshaled, no value would be put into the default Optional +the config is unmarshaled, no value would be put into the default Optional values and `HasValue` would return false when using this config object. This situation is something of an edge case with our current unmarshaling From 806f3ba03bae534128c8aa5916b560dcfc2c1cb2 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 11 Mar 2025 17:35:45 -0400 Subject: [PATCH 3/6] Address feedback --- docs/rfcs/optional-config-type.md | 100 ++++++++++++++++++++++++++++-- 1 file changed, 96 insertions(+), 4 deletions(-) diff --git a/docs/rfcs/optional-config-type.md b/docs/rfcs/optional-config-type.md index b921f8cb252..c34c66d831e 100644 --- a/docs/rfcs/optional-config-type.md +++ b/docs/rfcs/optional-config-type.md @@ -31,6 +31,9 @@ drawbacks: on the resulting config struct, and additional logic must be done to unset the default if the user has not specified a value. 4. The potential for null pointer exceptions is created. +5. Config structs are generally intended to be immutable and may be passed + around a lot, which makes the mutability property of pointer fields + an undesirable property. ## Optional types @@ -114,6 +117,93 @@ func createDefaultConfig() component.Config { } ``` +For something like `confighttp.ServerConfig`, using an `Optional[T]` type for +optional fields would look like this: + +```golang +type ServerConfig struct { + TLSSetting Optional[configtls.ServerConfig] `mapstructure:"tls"` + + CORS Optional[CORSConfig] `mapstructure:"cors"` + + Auth Optional[AuthConfig] `mapstructure:"auth,omitempty"` + + ResponseHeaders Optional[map[string]configopaque.String] `mapstructure:"response_headers"` +} + +func NewDefaultServerConfig() ServerConfig { + return ServerConfig{ + TLSSetting: WithDefault(configtls.NewDefaultServerConfig()), + CORS: WithDefault(NewDefaultCORSConfig()), + WriteTimeout: 30 * time.Second, + ReadHeaderTimeout: 1 * time.Minute, + IdleTimeout: 1 * time.Minute, + } +} + +func (hss *ServerConfig) ToListener(ctx context.Context) (net.Listener, error) { + listener, err := net.Listen("tcp", hss.Endpoint) + if err != nil { + return nil, err + } + + if hss.TLSSetting.HasValue() { + var tlsCfg *tls.Config + tlsCfg, err = hss.TLSSetting.Value().LoadTLSConfig(ctx) + if err != nil { + return nil, err + } + tlsCfg.NextProtos = []string{http2.NextProtoTLS, "http/1.1"} + listener = tls.NewListener(listener, tlsCfg) + } + + return listener, nil +} + +func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settings component.TelemetrySettings, handler http.Handler, opts ...ToServerOption) (*http.Server, error) { + // ... + + handler = httpContentDecompressor( + handler, + hss.MaxRequestBodySize, + serverOpts.ErrHandler, + hss.CompressionAlgorithms, + serverOpts.Decoders, + ) + + // ... + + if hss.Auth.HasValue() { + server, err := hss.Auth.Value().GetServerAuthenticator(context.Background(), host.GetExtensions()) + if err != nil { + return nil, err + } + + handler = authInterceptor(handler, server, hss.Auth.Value().RequestParameters) + } + + corsValue := hss.CORS.Value() + if hss.CORS.HasValue() && len(hss.CORS.AllowedOrigins) > 0 { + co := cors.Options{ + AllowedOrigins: corsValue.AllowedOrigins, + AllowCredentials: true, + AllowedHeaders: corsValue.AllowedHeaders, + MaxAge: corsValue.MaxAge, + } + handler = cors.New(co).Handler(handler) + } + if hss.CORS.HasValue() && len(hss.CORS.AllowedOrigins) == 0 && len(hss.CORS.AllowedHeaders) > 0 { + settings.Logger.Warn("The CORS configuration specifies allowed headers but no allowed origins, and is therefore ignored.") + } + + if hss.ResponseHeaders.HasValue() { + handler = responseHeadersHandler(handler, hss.ResponseHeaders.Value()) + } + + // ... +} +``` + ### Proper unmarshaling of empty values when a default is set Currently, the OTLP receiver requires a workaround to make enabling each @@ -178,7 +268,9 @@ implement similar config structures. There are two noteworthy disadvantages of introducing an Optional type: 1. Since the type isn't standard, external packages working with config may - require additional adaptations to work with our config structs. -2. It will still be possible to use pointer types for optional config fields, - and as a result there may be some fragmentation in which type is used to - represent an optional field. + require additional adaptations to work with our config structs. For example, + if we wanted to generate our types from a JSON schema using a package like + [github.com/atombender/go-jsonschema][go-jsonschema], we would need some way + to ensure compatibility with an Optional type. + +[go-jsonschema]: https://github.com/omissis/go-jsonschema \ No newline at end of file From 94e8a783517aeb26a788a8ff43fb6d12fc0a877d Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 12 Mar 2025 09:07:35 -0400 Subject: [PATCH 4/6] Update docs/rfcs/optional-config-type.md Co-authored-by: Pablo Baeyens --- docs/rfcs/optional-config-type.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/optional-config-type.md b/docs/rfcs/optional-config-type.md index c34c66d831e..a6b0d7e0ea7 100644 --- a/docs/rfcs/optional-config-type.md +++ b/docs/rfcs/optional-config-type.md @@ -265,7 +265,7 @@ implement similar config structures. ## Disadvantages of an Optional type -There are two noteworthy disadvantages of introducing an Optional type: +There is one noteworthy disadvantage of introducing an Optional type: 1. Since the type isn't standard, external packages working with config may require additional adaptations to work with our config structs. For example, From 9ee9aad9cc258d6e6f914ca429b81ad9f14b8667 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Fri, 11 Apr 2025 12:46:44 +0200 Subject: [PATCH 5/6] Add word to spellchecking list --- .github/workflows/utils/cspell.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/utils/cspell.json b/.github/workflows/utils/cspell.json index 84f769c5873..731560504f8 100644 --- a/.github/workflows/utils/cspell.json +++ b/.github/workflows/utils/cspell.json @@ -6,6 +6,7 @@ "Andrzej", "Anoshin", "Appy", + "atombender", "Backpressure", "Baeyens", "Bebbington", @@ -476,4 +477,4 @@ "zpagesextension", "zstd" ] - } \ No newline at end of file + } From 5d5f6527f1d4444c657d30c19f3055e781830913 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Mon, 21 Apr 2025 12:47:18 +0200 Subject: [PATCH 6/6] Clarify that there is no impact in YAML representation besides recording extra information --- docs/rfcs/optional-config-type.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/rfcs/optional-config-type.md b/docs/rfcs/optional-config-type.md index a6b0d7e0ea7..485b9daaef9 100644 --- a/docs/rfcs/optional-config-type.md +++ b/docs/rfcs/optional-config-type.md @@ -86,9 +86,10 @@ Optional types can fulfill the following use cases we have when decoding config. ### Representing optional config fields -To use the optional type to mark a config field as optional, the type can simply -be used as a type parameter to `Optional[T]`. The following config struct shows -how this may look, both in definition and in usage: +To use the optional type to mark a config field as optional, the type can simply be used as a type +parameter to `Optional[T]`. The YAML representation of `Optional[T]` is the same as that of `T`, +except that the type records whether the value is present. The following config struct shows how +this may look, both in definition and in usage: ```golang type Protocols struct { @@ -273,4 +274,4 @@ There is one noteworthy disadvantage of introducing an Optional type: [github.com/atombender/go-jsonschema][go-jsonschema], we would need some way to ensure compatibility with an Optional type. -[go-jsonschema]: https://github.com/omissis/go-jsonschema \ No newline at end of file +[go-jsonschema]: https://github.com/omissis/go-jsonschema