-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[confmap] Stricter types on configuration resolution #9532
Comments
How do we assess the impact of this? Doesn't this effectively mean a very large number of breaking changes for users?
Can you give an example? Is the problem that custom unmarshalers may preempt these input type conversions? |
@djaglowski Here is an example configuration that is accepted by the OpenTelemetry Collector today and has all the edge cases listed above for receivers:
otlp:
protocols:
grpc:
max_concurrent_streams: -1 # Wraps around to math.MaxUint32 - 1 (8)
exporters:
otlp:
endpoint: endpoint.local
headers:
- truthy: true # Sets header to truthy: 1 because of implicit casting to string (1)
number: 0123 # Sets header to number: 83 because of int (base 8) -> string -> int (base 10) (2)
- this-is-actually-a-slice: yikes # The two maps get merged into one with three entries (9)
read_buffer_size: true # Sets read_buffer_size to 1 (3)
write_buffer_size: "1234" # sets write_buffer_size to 1234 (4)
keepalive:
permit_without_stream: 23 # sets permit_without_stream to true (5)
tls:
insecure: t # sets insecure to true (6)
service:
pipelines:
metrics:
receivers: otlp # <-- Implicit casting of value to slice (10)
processors: {} # <-- Implicit casting of empty map to empty slice (7)
exporters: [otlp] 7-10 are specially confusing and franly seem nonsensical to me, and 1-6 deviate us from the YAML spec which I think leads to confusion. |
As an (incomplete) start we can take a look at open-telemetry/opentelemetry-collector-contrib/pull/31188. There are four tests that need changes here for rule (2), all of them are of the form To be honest (and again, this is my personal opinion and we should think about how to back with data) I believe the changes related to #8510 are going to be much more impactful in terms of people having to change their configurations than a change like this. |
From today's SIG: @songy23 to research on the difference in the type conversion rules between YAML library and mapstructure. We probably want to preserve the conversion rules overlapping with YAML library. |
Looking at all the implicit conversions listed by Pablo, I'm of the opinion that they should be removed, meaning the I suppose it's virtually impossible to assess the impact this change would have. We don't know if users have configurations that rely on any of these implicit conversions. Speaking from my experience, I think the only place I might have seen one of those conversions used is in the Filelog receiver's receivers:
filelog:
include: logs.log To make transition as painless for users as possible, we could prepare a migration plan like:
Anything else we can do? |
Warnings could be done by doing unmarshaling twice, once with |
Note: when we address this we need to remove the hook added in #9169 |
**Description:** Adds an RFC about how environment variable resolution should work **Link to tracking Issue:** Fixes #9515, relates to: - #8215 - #8565 - #9162 - #9531 - #9532 --------- Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com> Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
**Description:** Adds an RFC about how environment variable resolution should work **Link to tracking Issue:** Fixes open-telemetry#9515, relates to: - open-telemetry#8215 - open-telemetry#8565 - open-telemetry#9162 - open-telemetry#9531 - open-telemetry#9532 --------- Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com> Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Adds tests of current type casting behavior when using env and file provider. #### Link to tracking issue Relates to #9854, #8565, #9532
#### Description <!-- Issue number if applicable --> - Add `confmap.strictlyTypedInput` feature gate that introduces stricter type checks when resolving configuration - Make `confmap.NewRetrievedFromYAML` function public so that external providers have consistent behavior when resolving YAML - Adds `confmap.Retrieved.AsString` method to retrieve string representation for retrieved values #### Link to tracking issue Relates to #9854, updates #8565, #9532
…10553) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Adds coda to errors related to `confmap.strictlyTypedInput` that direct users to #10552 and explain the feature gate. #### Link to tracking issue Updates #9532 <!--Describe what testing was performed and which tests were added.--> #### Testing Tests that the coda is present when there are weakly typed errors and not present with non weakly-typed errors. <!--Describe the documentation added.-->
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Makes type resolution strict and conforming to the behavior described in [the env vars RFC](https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/rfcs/env-vars.md) Depends on: - #10553 - #10555 - #10559 - open-telemetry/opentelemetry-collector-contrib/pull/33950 <!-- Issue number if applicable --> #### Link to tracking issue Fixes #9532, Fixes #8565 --------- Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
We enable the
WeaklyTypedInput
option from mapstructure. This option implies the following implicit conversions from the internalconfmap
'smap[string]any
to ourConfig
structs:strconv.ParseInt
)element is weakly decoded. For example: "4" can become []int{4}
if the target type is an int slice.
We also do implicit casting to string when expanding a
${provider:URI}
variable:opentelemetry-collector/confmap/expand.go
Lines 124 to 139 in cf51a35
I think this is too lax and commits us to supporting too many weird edge cases (in particular all but 2 and 4 seem like clearly things we should remove, and I would argue even 2 and 4 should be removed eventually). This conflicts with the changes I proposed on #9515, so I think we should resolve this one first.
The text was updated successfully, but these errors were encountered: