Skip to content
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] Strings containing numbers in nonstandard notation are normalized to decimal notation #8565

Closed
rassmate opened this issue Oct 3, 2023 · 9 comments · Fixed by #10554
Labels
area:config bug Something isn't working

Comments

@rassmate
Copy link

rassmate commented Oct 3, 2023

Describe the bug
When using the $env:ENVIRONMENT_VARIABLE_NAME syntax in the collector configuration a string with numbers only starting with zero is interpreted as a number.

Steps to reproduce
rassmate@855679d

What did you expect to see?
When working with environment variables I expected to get the exact value that the environment variable holds.

What did you see instead?
A string got interpreted into a number very different from the one in the environment variable when the value had a leading zero.

What version did you use?
v0.84

What config did you use?

env: 
  name: TEST
  value: "0123"
...
processors:
  transform:
    error_mode: ignore
    trace_statements:
      - context: span
        statements:
          - set(resource.attributes["TEST"], "${env:TEST}") 
... 

Environment
OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")
Macbook air m2. Same error in openshift cluster

Additional context
Add any other context about the problem here.

@rassmate rassmate added the bug Something isn't working label Oct 3, 2023
@rassmate rassmate changed the title Strings containing numbers only and starting with zero Strings containing numbers only and starting with zero is interpreted as a number Oct 3, 2023
@mx-psi
Copy link
Member

mx-psi commented Oct 3, 2023

Thanks for reporting this.

This comes from the YAML library we use, a minimal example can be seen here: https://go.dev/play/p/eO1O_q2ITAU. It is documented on the README:

Octals encode and decode as 0777 per YAML 1.1, rather than 0o777 as specified in YAML 1.2, because most parsers still use the old format

You don't need to be using the ${env:..} feature for this: directly setting an integer-type option to 0123 will cause it to be interpreted as the equivalent octal number (83).

In your particular case, setting TEST to "!!str 0123" (using YAML explicit tags) or with explicit quotes as in "\"0123\"" would be a valid workaround (this workaround is valid so long as the variable is not being used to set a configuration value of int type).

I asked upstream for a toggle on the library to avoid this quirk on go-yaml/yaml/issues/996.

@rassmate
Copy link
Author

rassmate commented Oct 3, 2023

Ok, thanks for the clarification.

I think it is a bit strange though that in the envprovider.go even use the yaml-parser to get environment variable values.
I would have guessed on just fetching the clean value from the environment without any parsing.

@mx-psi
Copy link
Member

mx-psi commented Oct 3, 2023

Right, the ${env:...} feature actually supports YAML syntax, e.g. you can do

processors:
   transform: ${env:TRANSFORM_CONFIG}

and pull in your full transform processor configuration from the TRANSFORM_CONFIG environment variable.

Your issue though points to a general problem where our configuration loading system does not know it is mapping to a string and it loses the original representation of string values that can be interpreted as integers/floats.

I wrote a small example here: https://go.dev/play/p/3gXGDllytzd, in summary with a config struct like:

type Config struct {
	Octal      string
	Hex        string
	Scientific string
	Positive   string
	Infinite   string
}

a configuration like:

Octal: 0123
Hex: 0xdeadbeef
Scientific: 1.23e5
Positive: +123
Infinite: .inf

when loaded through ${env:...} or any other provider, will get the following loadedConfig struct:

loadedConfig := Config{ 
 Octal: "83",
 Hex: "3735928559",
 Scientific: "123000",
 Positive: "123",
 Infinite: "+Inf",
}

where even if the fields are string, their original values are not kept and instead a normalized representation of the strings as integers/floats is applied.

@rassmate
Copy link
Author

rassmate commented Oct 3, 2023

Thanks for a great explanation!

Since env is such a well established term I as a new user does not expect the support for yaml-parsing on my environment variables although i like the feature.

Perhaps a solution could be suggested where we explicitly can call for the raw value of the environment variable?

${env:YAML_VALUE_OR_OTHER}

${env_raw:RAW_ENV_VALUE_NOT_RUN_THROUGH_YAML_PARSER}

I've managed to work around it by adding the double quotes in the value so it is not critical for me.
Just trying to share a newbies thoughts.

@mx-psi mx-psi changed the title Strings containing numbers only and starting with zero is interpreted as a number [confmap] Strings containing numbers in nonstandard notation are normalized to decimal notation Oct 3, 2023
@andrzej-stencel
Copy link
Member

@rassmate If you use ${ENV_VAR} instead of ${env:ENV_VAR}, then you get the verbatim string value, don't you?

@mx-psi
Copy link
Member

mx-psi commented Feb 6, 2024

I think we can use something like this https://go.dev/play/p/g8hciqbI921 to fix this, but I want to first agree on how ${env:..}, ${..} and how confmap resolution more generally should work, for which I will open a separate document/issue/something else.

@mx-psi mx-psi moved this to Blocked in Collector: v1 Apr 18, 2024
codeboten added a commit that referenced this issue Apr 30, 2024
**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 <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
@codeboten codeboten moved this from Blocked to Todo in Collector: v1 May 1, 2024
andrzej-stencel pushed a commit to andrzej-stencel/opentelemetry-collector that referenced this issue May 27, 2024
**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 <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
@TylerHelmuth
Copy link
Member

As part of the RFC we decided that ${FOO} and ${env:FOO} would be resolved the same way (at least for our distros) which means supporting YAML syntax for both styles.

@mx-psi what needs worked on for this issue? If there are reasonable work arounds, such as export TEST=\"0123\", I propose we remove this as a 1.0 requirement.

@rassmate
Copy link
Author

Sorry for ghosting you on this issue for a while but I like what I read and some thought is put in to this matter.

Keep up the good work!

@mx-psi
Copy link
Member

mx-psi commented Jun 11, 2024

@TylerHelmuth This will be solved by setting WeaklyTypedInput to false, there is nothing else needed here

mx-psi added a commit that referenced this issue Jun 13, 2024
<!--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
mx-psi added a commit that referenced this issue Jun 17, 2024
#### 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
@mx-psi mx-psi closed this as completed in 6227646 Jul 12, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Collector: v1 Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants