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

Password containing $ doesn't work when passed via ${env:DB_PASSWORD} #8215

Closed
wand2016 opened this issue Aug 9, 2023 · 7 comments · Fixed by #10510
Closed

Password containing $ doesn't work when passed via ${env:DB_PASSWORD} #8215

wand2016 opened this issue Aug 9, 2023 · 7 comments · Fixed by #10510
Assignees
Labels
area:config bug Something isn't working priority:p2 Medium release:required-for-ga Must be resolved before GA release

Comments

@wand2016
Copy link

wand2016 commented Aug 9, 2023

Component(s)

receiver/postgresql

What happened?

Password containing $ doesn't work when passed via ${env:DB_PASSWORD}.
When passed directly, it works.

Description

Steps to Reproduce

  1. Prepare PostgreSQL DB:
    • username: user
    • password: p@s$w0rd
  2. Configure postgresql receiver:
  postgresql:
    ...
    username: "${env:DB_USERNAME}"
    password: "${env:DB_PASSWORD}"
  1. Pass environment variables to OpenTelemetry CR:
apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
...
spec:
  config: <configuration mentioned above>
  env:
    - name: DB_USERNAME
      value: user
    - name: DB_PASSWORD
      value: p@s$w0rd

Expected Result

Authentication passes.

Actual Result

Authentication fails.

Given password directly, authentication passes.

  postgresql:
    ...
    username: "${env:DB_USERNAME}"
    password: "p@s$$w0rd"

Collector version

otel/opentelemetry-collector-contrib:0.81.0

Environment information

Environment

contrib Docker Image

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

@wand2016 wand2016 added the bug Something isn't working label Aug 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@wand2016 wand2016 changed the title [receiver/postgresqlreceiver] password with $ doesn't work when passed via ${env:DB_PASSWORD} [receiver/postgresqlreceiver] password containing $ doesn't work when passed via ${env:DB_PASSWORD} Aug 9, 2023
@wand2016 wand2016 changed the title [receiver/postgresqlreceiver] password containing $ doesn't work when passed via ${env:DB_PASSWORD} [receiver/postgresql] password containing $ doesn't work when passed via ${env:DB_PASSWORD} Aug 9, 2023
@mx-psi
Copy link
Member

mx-psi commented Aug 9, 2023

The Collector supports recursively expanding ${env:...} and the legacy $ENV syntax in the contents of an environment variable, so you need to escape the $ as documented on the official docs:

Use $$ to indicate a literal $.

In your case this would be p@s$$w0rd

@wand2016
Copy link
Author

wand2016 commented Aug 10, 2023

Thanks for replying!

Actually, the password is stored in a Secret resource:

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
...
spec:
  config: <configuration mentioned above>
  env:
    - name: DB_PASSWORD   
      valueFrom:
        secretKeyRef:
          name: db-secrets
          key: password

(By the way, the Secret resource is synchronized by External Secrets Operator in my project.)

Must it be stored in the Secret resource (or its secret backend) as escaped p@s$$w0rd?
This couples the secrets and opentelemetry collector, which makes reuse hard.
Is there any way to disable recursive expanding?

@mx-psi mx-psi transferred this issue from open-telemetry/opentelemetry-collector-contrib Aug 10, 2023
@mx-psi mx-psi changed the title [receiver/postgresql] password containing $ doesn't work when passed via ${env:DB_PASSWORD} Password containing $ doesn't work when passed via ${env:DB_PASSWORD} Aug 10, 2023
@mx-psi
Copy link
Member

mx-psi commented Aug 10, 2023

I moved this to opentelemetry-collector since this is not specific to the postgresql receiver, but rather applies to configuration more generally.

I think this would not happen if the expandconverter was not enabled. Since this is a legacy component that we want to remove in favor of ${env:...} syntax, we could add a feature gate to disable the expand converter and keep it at alpha for now.

@open-telemetry/collector-maintainers what do you think? Would you be open to such feature gate?

@atoulme atoulme added the release:required-for-ga Must be resolved before GA release label Dec 19, 2023
@atoulme
Copy link
Contributor

atoulme commented Dec 20, 2023

Discussed in SIG 12/20, we will work towards deprecating the naked $ notation and support ${...} instead, and therefore avoid this type of bug moving forward.

See open-telemetry/opentelemetry-specification#3744 for the spec change.

As part of this, we will probably deprecate the $ expand, as noted by Pablo, we can't really log a warning right now when this happens because the expansion happens before logging is set up. We can look into that.

@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
@TylerHelmuth
Copy link
Member

We've made progress toward this issue. The collector now:

  1. supports logging during confmap resolution
  2. prints a log if $ENV syntax is used.

To officially work towards deprecating the $ENV syntax I believe we do need a feature gate that can move use through:

  1. allowing the syntax, with the option to disallow (alpha)
  2. disallow the syntax, with an option to allow (beta)
  3. disallow the syntax entirely (stable)

@TylerHelmuth
Copy link
Member

Actually this issue is still blocked by #7111

@TylerHelmuth TylerHelmuth moved this from In Progress to Blocked in Collector: v1 May 14, 2024
@TylerHelmuth TylerHelmuth moved this from Blocked to In Progress in Collector: v1 May 20, 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]>
mx-psi pushed a commit that referenced this issue Jun 14, 2024
#### Description
This PR adds a feature gate that will handle transitioning users away
from expandconverter, specifically expanding `$VAR` syntax.

The wholistic strategy is:
1. create a new feature gate, `confmap.unifyEnvVarExpansion`, that will
be the single feature gate to manage unifying collector configuraiton
resolution.
2. Update expandconverter to return an error if the feature gate is
enabled and it is about to expand `$VAR` syntax.
3. Update `otelcol.NewCommand` to set a `DefaultScheme="env"` when the
feature gate is enabled and no `DefaultScheme` is set, this handles
`${VAR}` syntax.
  4. Separately, deprecate `expandconverter`.
  5. Follow a normal feature gate cycle.
6. Removed the `confmap.unifyEnvVarExpansion` feature gates and
`expandconverter` at the same time

Supersedes
#10259

#### Link to tracking issue
Related to
#10161
Related to
#8215
Related to
#7111

#### Testing
Unit tests
mx-psi added a commit that referenced this issue Jun 28, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Moves confmap.unifyEnvVarExpansion to beta. This means the collector
will, by default, use the env var provider to expand `${FOO}` synatx and
will error if the expandconverter is used to expand `$FOO` syntax.

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
#10161
Related to
#8215
Related to
#7111

---------

Co-authored-by: Pablo Baeyens <[email protected]>
dmitryax pushed a commit that referenced this issue Aug 7, 2024
…0508)

#### Description

This PR promotes the `confmap.unifyEnvVarExpansion` feature gate to
stable and sets a `ToVersion` of `v0.106.0`, anticipating that the gate
be completely removed in that version.

We should weigh if switching the Stable should be done in `v0.105.0` or
if it needs more time in `Beta` to give users more time to switch.
Delaying promotion to `Stable` delays confmap 1.0.

If we merge this we need to commit to merging
#10510 in
the same release.

#### Link to tracking issue
Related to
#10161
Related to
#7111
Related to
#8215

---------

Co-authored-by: Evan Bradley <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Collector: v1 Aug 7, 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 priority:p2 Medium release:required-for-ga Must be resolved before GA release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants