Support camelcase matchLabels and matchExpressions in TA config #3418
Conversation
|
@davidhaja did you try to use mapstructure? I wonder if that would work more effectively than trying to implement some custom support here. Also, because LabelSelector has json tags, we may be able to use this unmarshaller which does a conversion from yaml -> json and then uses json decoders |
@jaronoff97 I've tried this package. Copying it here: The way I see is either we try to support the flexible/case insensitive unmarshalling in TA, or we change both the marshalling and unmarshalling to follow (and require) the k8s camel case convention. The latter one is a breaking change, which I believe requires more effort. (Although probably results in cleaner logic, code as well) In the submitted PR I've made some changes that implement the former option. |
|
Ah thank you, and did mapstructure not work at all? |
I haven't used mapstructure before your comment. :) |
cmd/otel-allocator/config/config.go
Outdated
| func StringToModelDurationHookFunc() mapstructure.DecodeHookFunc { | ||
| return func( | ||
| f reflect.Type, | ||
| t reflect.Type, | ||
| data interface{}, | ||
| ) (interface{}, error) { | ||
| if f.Kind() != reflect.String { | ||
| return data, nil | ||
| } | ||
| if t != reflect.TypeOf(model.Duration(5)) { | ||
| return data, nil | ||
| } | ||
|
|
||
| // Convert it by parsing | ||
| return time.ParseDuration(data.(string)) | ||
| } | ||
| } | ||
|
|
||
| func decodeSubConfig(t interface{}, dc mapstructure.DecoderConfig) error { | ||
| dec, decError := mapstructure.NewDecoder(&dc) | ||
| if decError != nil { | ||
| return decError | ||
| } | ||
| if err := dec.Decode(t); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func flexibleUnmarshal(yamlFile []byte, cfg *Config) error { |
There was a problem hiding this comment.
We should add docstrings to these functions, as well as a longer comment explaining why they exist.
cmd/otel-allocator/config/config.go
Outdated
| if err = yaml.Unmarshal(yamlFile, cfg); err != nil { | ||
| return fmt.Errorf("error unmarshaling YAML: %w", err) | ||
| } | ||
|
|
||
| if err := flexibleUnmarshal(yamlFile, cfg); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
We should add a comment here explaining why both these statements are necessary. Incidentally, what we're doing here also means that if someone were to use both versions, the camel case version wins.
|
Hey @davidhaja wdyt? |
|
@davidhaja check how we're doing that on the prometheus receiver: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/770befbc7a5e4f436b67e7bb9ecf42bf3c5e7053/receiver/prometheusreceiver/targetallocator/config.go#L31C6-L31C22 I think we can have a similar approach, wdyt? |
I will check this out. |
…ta config unmarshalling
…y-operator into 3350-ta-matchlabels
|
@nicolastakashi In my latest commit, I have implemented a solution where the camel case The regular expression is employed to identify only those Please let me know if you consider this approach to be a viable solution. |
|
@davidhaja I'm not comfortable with a solution based on regular expressions. There's a bunch of places in the configuration where arbitrary strings can appear, so any substitutions we make should take the file structure into consideration. If you want to go this way, I'd recommend unmarshalling the raw config to a map, making the changes, marshalling it back, and then unmarshalling into the final config struct. More wasteful, but safer. |
|
@swiatekm Please let me know what you think about this change. |
Description:
Currently TA cannot parse camel case matchLabels and matchExpressions fields due to the limitation of
gopkg.in/yaml.v2package and due to the lack of the yaml tag inmetav1.LabelSelectorstruct definition.The
github.com/goccy/go-yamlparses the camel case labelselector fields by default and provides the possibility to parse a specific route in a yaml string. Relying on this feature the TA can unmarshal the only lowercase version of these fields, and then merge together with the originally unmarshalled config.Therefore both the camel case and the only lowercase fields of the LabelSelector will be handled.
Link to tracking Issue(s): 3350
Testing:
Provided test configs with camelcase matchLabels and matchExpressions
Documentation:
N/A