Skip to content

Commit 1ed7f6d

Browse files
frzifusfrzifus
authored
Merge pull request #1 from jaronoff97/add/receiver_defaults
Add tests and simplify some logic
2 parents edf8554 + ff6c105 commit 1ed7f6d

8 files changed

+322
-69
lines changed

‎apis/v1beta1/collector_webhook_test.go

+33
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,39 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
177177
},
178178
},
179179
},
180+
{
181+
name: "update config defaults, leave other fields alone",
182+
otelcol: v1beta1.OpenTelemetryCollector{
183+
Spec: v1beta1.OpenTelemetryCollectorSpec{
184+
Config: func() v1beta1.Config {
185+
const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"headers":{"example":"another"}},"http":{"endpoint":"0.0.0.0:4000"}}}},"exporters":{"debug":null},"service":{"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
186+
var cfg v1beta1.Config
187+
require.NoError(t, yaml.Unmarshal([]byte(input), &cfg))
188+
return cfg
189+
}(),
190+
},
191+
},
192+
expected: v1beta1.OpenTelemetryCollector{
193+
ObjectMeta: metav1.ObjectMeta{
194+
Labels: map[string]string{},
195+
},
196+
Spec: v1beta1.OpenTelemetryCollectorSpec{
197+
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
198+
Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"},
199+
ManagementState: v1beta1.ManagementStateManaged,
200+
Replicas: &one,
201+
},
202+
Mode: v1beta1.ModeDeployment,
203+
UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic,
204+
Config: func() v1beta1.Config {
205+
const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"endpoint":"0.0.0.0:4317","headers":{"example":"another"}},"http":{"endpoint":"0.0.0.0:4000"}}}},"exporters":{"debug":null},"service":{"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
206+
var cfg v1beta1.Config
207+
require.NoError(t, yaml.Unmarshal([]byte(input), &cfg))
208+
return cfg
209+
}(),
210+
},
211+
},
212+
},
180213
{
181214
name: "all fields default",
182215
otelcol: v1beta1.OpenTelemetryCollector{},

‎apis/v1beta1/config.go

+10-2
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"strconv"
2525
"strings"
2626

27+
"dario.cat/mergo"
2728
"github.com/go-logr/logr"
2829
"gopkg.in/yaml.v3"
2930
corev1 "k8s.io/api/core/v1"
@@ -244,11 +245,18 @@ func (c *Config) applyDefaultForComponentKinds(logger logr.Logger, componentKind
244245
}
245246
for componentName := range enabledComponents[componentKind] {
246247
parser := retriever(componentName)
247-
newCfg, err := parser.GetDefaultConfig(logger, cfg.Object[componentName])
248+
componentConf := cfg.Object[componentName]
249+
newCfg, err := parser.GetDefaultConfig(logger, componentConf)
248250
if err != nil {
249251
return err
250252
}
251-
cfg.Object[componentName] = newCfg
253+
// We need to ensure we don't remove any fields in defaulting.
254+
mappedCfg := newCfg.(map[string]interface{})
255+
err = mergo.Merge(&mappedCfg, componentConf)
256+
if err != nil {
257+
return err
258+
}
259+
cfg.Object[componentName] = mappedCfg
252260
}
253261
}
254262

‎internal/components/component.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ type ProbeGenerator[ComponentConfigType any] func(logger logr.Logger, config Com
5353

5454
// Defaulter is a function that applies given defaults to the passed Config.
5555
// It's expected that type Config is the configuration used by a parser.
56-
type Defaulter[ComponentConfigType any] func(logger logr.Logger, addrProv AddressProvider, config ComponentConfigType) (map[string]interface{}, error)
56+
type Defaulter[ComponentConfigType any] func(logger logr.Logger, defaultAddr string, defaultPort int32, config ComponentConfigType) (map[string]interface{}, error)
5757

5858
// ComponentType returns the type for a given component name.
5959
// components have a name like:
@@ -94,6 +94,7 @@ type ParserRetriever func(string) Parser
9494

9595
type Parser interface {
9696
// GetDefaultConfig returns a config with set default values.
97+
// NOTE: Config merging must be done by the caller if desired.
9798
GetDefaultConfig(logger logr.Logger, config interface{}) (interface{}, error)
9899

99100
// Ports returns the service ports parsed based on the component's configuration where name is the component's name

‎internal/components/generic_parser.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func (g *GenericParser[T]) GetDefaultConfig(logger logr.Logger, config interface
5252
if err := mapstructure.Decode(config, &parsed); err != nil {
5353
return nil, err
5454
}
55-
return g.defaultsApplier(logger, func(string) (string, int32) { return g.settings.defaultRecAddr, g.settings.GetServicePort().Port }, parsed)
55+
return g.defaultsApplier(logger, g.settings.defaultRecAddr, g.settings.GetServicePort().Port, parsed)
5656
}
5757

5858
func (g *GenericParser[T]) GetLivenessProbe(logger logr.Logger, config interface{}) (*corev1.Probe, error) {

‎internal/components/generic_parser_test.go

+93
Original file line numberDiff line numberDiff line change
@@ -372,3 +372,96 @@ func TestGenericParser_GetProbe(t *testing.T) {
372372
})
373373
}
374374
}
375+
376+
func TestGenericParser_GetDefaultConfig(t *testing.T) {
377+
type args struct {
378+
logger logr.Logger
379+
config interface{}
380+
}
381+
type testCase[T any] struct {
382+
name string
383+
g *components.GenericParser[T]
384+
args args
385+
want interface{}
386+
wantErr assert.ErrorAssertionFunc
387+
}
388+
389+
tests := []testCase[*components.SingleEndpointConfig]{
390+
{
391+
name: "no settings or defaultsApplier returns config",
392+
g: &components.GenericParser[*components.SingleEndpointConfig]{},
393+
args: args{
394+
logger: logr.Discard(),
395+
config: map[string]interface{}{
396+
"endpoint": "http://localhost:8080",
397+
},
398+
},
399+
want: map[string]interface{}{
400+
"endpoint": "http://localhost:8080",
401+
},
402+
wantErr: assert.NoError,
403+
},
404+
{
405+
name: "empty defaultRecAddr returns config",
406+
g: components.NewSinglePortParserBuilder("test", 0).MustBuild(),
407+
args: args{
408+
logger: logr.Discard(),
409+
config: map[string]interface{}{
410+
"endpoint": "http://localhost:8080",
411+
},
412+
},
413+
want: map[string]interface{}{
414+
"endpoint": "http://localhost:8080",
415+
},
416+
wantErr: assert.NoError,
417+
},
418+
{
419+
name: "valid settings with defaultsApplier",
420+
g: components.NewSinglePortParserBuilder("test", 8080).WithDefaultRecAddress("127.0.0.1").WithDefaultsApplier(components.AddressDefaulter).MustBuild(),
421+
args: args{
422+
logger: logr.Discard(),
423+
config: map[string]interface{}{
424+
"endpoint": nil,
425+
},
426+
},
427+
want: map[string]interface{}{
428+
"endpoint": "127.0.0.1:8080",
429+
},
430+
wantErr: assert.NoError,
431+
},
432+
{
433+
name: "valid settings with defaultsApplier doesnt override",
434+
g: components.NewSinglePortParserBuilder("test", 8080).WithDefaultRecAddress("127.0.0.1").WithDefaultsApplier(components.AddressDefaulter).MustBuild(),
435+
args: args{
436+
logger: logr.Discard(),
437+
config: map[string]interface{}{
438+
"endpoint": "127.0.0.1:9090",
439+
},
440+
},
441+
want: map[string]interface{}{
442+
"endpoint": "127.0.0.1:9090",
443+
},
444+
wantErr: assert.NoError,
445+
},
446+
{
447+
name: "invalid config fails to decode",
448+
g: components.NewSinglePortParserBuilder("test", 8080).WithDefaultRecAddress("127.0.0.1").WithDefaultsApplier(components.AddressDefaulter).MustBuild(),
449+
args: args{
450+
logger: logr.Discard(),
451+
config: "invalid_config",
452+
},
453+
want: nil,
454+
wantErr: assert.Error,
455+
},
456+
}
457+
458+
for _, tt := range tests {
459+
t.Run(tt.name, func(t *testing.T) {
460+
got, err := tt.g.GetDefaultConfig(tt.args.logger, tt.args.config)
461+
if !tt.wantErr(t, err, fmt.Sprintf("GetDefaultConfig(%v, %v)", tt.args.logger, tt.args.config)) {
462+
return
463+
}
464+
assert.Equalf(t, tt.want, got, "GetDefaultConfig(%v, %v)", tt.args.logger, tt.args.config)
465+
})
466+
}
467+
}

‎internal/components/multi_endpoint.go

+24-62
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,8 @@ type MultiPortOption func(parser *MultiPortReceiver)
4040
type MultiPortReceiver struct {
4141
name string
4242

43-
addrMappings map[string]string
44-
portMappings map[string]*corev1.ServicePort
45-
defaultsApplier Defaulter[any]
43+
addrMappings map[string]string
44+
portMappings map[string]*corev1.ServicePort
4645
}
4746

4847
func (m *MultiPortReceiver) Ports(logger logr.Logger, name string, config interface{}) ([]corev1.ServicePort, error) {
@@ -79,24 +78,29 @@ func (m *MultiPortReceiver) GetDefaultConfig(logger logr.Logger, config interfac
7978
if err := mapstructure.Decode(config, multiProtoEndpointCfg); err != nil {
8079
return nil, err
8180
}
82-
83-
defaulter := func(protocol string) (string, int32) {
84-
var port int32
81+
defaultedConfig := map[string]interface{}{}
82+
for protocol, ec := range multiProtoEndpointCfg.Protocols {
8583
if defaultSvc, ok := m.portMappings[protocol]; ok {
86-
port = defaultSvc.Port
87-
ec := multiProtoEndpointCfg.Protocols[protocol]
84+
port := defaultSvc.Port
8885
if ec != nil {
8986
port = ec.GetPortNumOrDefault(logger, port)
9087
}
88+
var addr string
89+
if defaultAddr, ok := m.addrMappings[protocol]; ok {
90+
addr = defaultAddr
91+
}
92+
conf, err := AddressDefaulter(logger, addr, port, ec)
93+
if err != nil {
94+
return nil, err
95+
}
96+
defaultedConfig[protocol] = conf
97+
} else {
98+
return nil, fmt.Errorf("unknown protocol set: %s", protocol)
9199
}
92-
var addr string
93-
if defaultAddr, ok := m.addrMappings[protocol]; ok {
94-
addr = defaultAddr
95-
}
96-
return addr, port
97100
}
98-
99-
return m.defaultsApplier(logger, defaulter, multiProtoEndpointCfg)
101+
return map[string]interface{}{
102+
"protocols": defaultedConfig,
103+
}, nil
100104
}
101105

102106
func (m *MultiPortReceiver) GetLivenessProbe(logger logr.Logger, config interface{}) (*corev1.Probe, error) {
@@ -118,7 +122,7 @@ func NewMultiPortReceiverBuilder(name string) MultiPortBuilder[*MultiProtocolEnd
118122
}
119123

120124
func NewProtocolBuilder(name string, port int32) Builder[*MultiProtocolEndpointConfig] {
121-
return NewBuilder[*MultiProtocolEndpointConfig]().WithName(name).WithPort(port).WithDefaultsApplier(MultiAddressDefaulter)
125+
return NewBuilder[*MultiProtocolEndpointConfig]().WithName(name).WithPort(port)
122126
}
123127

124128
func (mp MultiPortBuilder[ComponentConfigType]) AddPortMapping(builder Builder[ComponentConfigType]) MultiPortBuilder[ComponentConfigType] {
@@ -130,21 +134,18 @@ func (mp MultiPortBuilder[ComponentConfigType]) Build() (*MultiPortReceiver, err
130134
return nil, fmt.Errorf("must provide at least one port mapping")
131135
}
132136

133-
mp0Defaulter := mp[0].MustBuild().defaultsApplier
134137
multiReceiver := &MultiPortReceiver{
135-
name: mp[0].MustBuild().name,
136-
defaultsApplier: createMultiAddressDefaulter(mp0Defaulter),
137-
addrMappings: map[string]string{},
138-
portMappings: map[string]*corev1.ServicePort{},
138+
name: mp[0].MustBuild().name,
139+
addrMappings: map[string]string{},
140+
portMappings: map[string]*corev1.ServicePort{},
139141
}
140142
for _, bu := range mp[1:] {
141143
built, err := bu.Build()
142144
if err != nil {
143145
return nil, err
144146
}
145-
multiReceiver.defaultsApplier = createMultiAddressDefaulter(built.defaultsApplier)
146-
multiReceiver.portMappings[built.name] = built.settings.GetServicePort()
147147
if built.settings != nil {
148+
multiReceiver.portMappings[built.name] = built.settings.GetServicePort()
148149
multiReceiver.addrMappings[built.name] = built.settings.defaultRecAddr
149150
}
150151
}
@@ -158,42 +159,3 @@ func (mp MultiPortBuilder[ComponentConfigType]) MustBuild() *MultiPortReceiver {
158159
return p
159160
}
160161
}
161-
162-
func createMultiAddressDefaulter[ComponentConfigType any](defaultsApplier Defaulter[ComponentConfigType]) Defaulter[any] {
163-
return func(logger logr.Logger, addrProv AddressProvider, config any) (map[string]interface{}, error) {
164-
tc, ok := config.(ComponentConfigType)
165-
if !ok {
166-
return nil, fmt.Errorf("invalid config type, expected ComponentConfigType")
167-
}
168-
169-
result, err := defaultsApplier(logger, addrProv, tc)
170-
if err != nil {
171-
return nil, err
172-
}
173-
174-
return result, nil
175-
}
176-
}
177-
178-
func MultiAddressDefaulter(logger logr.Logger, addrProv AddressProvider, config *MultiProtocolEndpointConfig) (map[string]interface{}, error) {
179-
root := make(map[string]interface{})
180-
if err := mapstructure.Decode(config, &root); err != nil {
181-
return nil, err
182-
}
183-
184-
proto, ok := root["protocols"].(map[string]interface{})
185-
if !ok {
186-
proto = make(map[string]interface{})
187-
root["protocols"] = proto
188-
}
189-
190-
for protocol, ec := range config.Protocols {
191-
res, err := AddressDefaulter(logger, func(string) (string, int32) { return addrProv(protocol) }, ec)
192-
if err != nil {
193-
return nil, err
194-
}
195-
proto[protocol] = res
196-
}
197-
198-
return root, nil
199-
}

0 commit comments

Comments
 (0)