opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[confmap] Strings containing numbers in nonstandard notation are normalized to decimal notation

Open rassmate opened this issue 1 year ago • 6 comments

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 https://github.com/rassmate/opentelemetry-collector/commit/855679d1b20f46b214f2389d590ee79573617337

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 avatar Oct 03 '23 07:10 rassmate

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.

mx-psi avatar Oct 03 '23 11:10 mx-psi

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.

rassmate avatar Oct 03 '23 11:10 rassmate

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.

mx-psi avatar Oct 03 '23 11:10 mx-psi

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.

rassmate avatar Oct 03 '23 12:10 rassmate

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

andrzej-stencel avatar Feb 01 '24 13:02 andrzej-stencel

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 avatar Feb 06 '24 16:02 mx-psi

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.

TylerHelmuth avatar Jun 10 '24 20:06 TylerHelmuth

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!

rassmate avatar Jun 10 '24 22:06 rassmate

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

mx-psi avatar Jun 11 '24 11:06 mx-psi