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.

Sign up for GitHub

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

Jump to bottom

[confmap] Restrict characters in environment variable URIs even more! #9531

Closed
mx-psi opened this issue Feb 8, 2024 · 0 comments · Fixed by #10183
Closed

[confmap] Restrict characters in environment variable URIs even more! #9531

mx-psi opened this issue Feb 8, 2024 · 0 comments · Fixed by #10183
Labels
area:confmap release:required-for-ga Must be resolved before GA release
Milestone
go.opentelemetry....

Comments

@mx-psi
Copy link
Member

mx-psi commented Feb 8, 2024 •
edited
Loading

In #5706, we discussed restricting URIs in characters for all providers. We ended up disallowing $ in #6268 for the recursive implementation done later.

I think we should restrict the character set even more for the env provider, in particular by following the restrictions defined by the OpenTelemetry Configuration Working Group (see here). While, as mentioned in #5706 (comment), there are more characters allowed on both Windows and Linux, the set of environment variable names allowed by the spec is reasonable and (pressumably!) what the vast majority of users use.

We should probably restrict it also on the expandconverter while it exists
@mx-psi mx-psi added release:required-for-ga Must be resolved before GA release area:confmap labels Feb 8, 2024
@mx-psi mx-psi mentioned this issue Mar 13, 2024
Open
@mx-psi mx-psi mentioned this issue Apr 3, 2024
Merged
@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
@mx-psi @codeboten @evan-bradley
[docs/rfc] RFC about environment variables (#9854)
fe582df 
**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>
@codeboten codeboten moved this from Blocked to Todo in Collector: v1 May 1, 2024
@ankitpatel96 ankitpatel96 mentioned this issue May 21, 2024
Merged
@TylerHelmuth TylerHelmuth moved this from Todo to In Progress in Collector: v1 May 21, 2024
mx-psi added a commit that referenced this issue May 24, 2024
@ankitpatel96 @mx-psi
Restrict characters in environment variable names (#10183)
38e1bad 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Restricts Environment Variable names according to the
[restrictions](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#environment-variable-substitution)
published by the OpenTelemetry Configuration Working Group. Changes both
the expand converter and the env provider. Defines one regex to be used
for both of these.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes
#9531

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Collector: v1 May 24, 2024
andrzej-stencel pushed a commit to andrzej-stencel/opentelemetry-collector that referenced this issue May 27, 2024
@mx-psi @codeboten
@evan-bradley @andrzej-stencel
[docs/rfc] RFC about environment variables (open-telemetry#9854)
5648678 
**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>
andrzej-stencel pushed a commit to andrzej-stencel/opentelemetry-collector that referenced this issue May 27, 2024
@ankitpatel96 @mx-psi @andrzej-stencel
Restrict characters in environment variable names (open-telemetry#10183)
24572ad 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Restricts Environment Variable names according to the
[restrictions](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#environment-variable-substitution)
published by the OpenTelemetry Configuration Working Group. Changes both
the expand converter and the env provider. Defines one regex to be used
for both of these.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes
open-telemetry#9531

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this issue Jun 14, 2024
@ankitpatel96 @mx-psi @steves-canva
Restrict characters in environment variable names (open-telemetry#10183)
fbfc110 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Restricts Environment Variable names according to the
[restrictions](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#environment-variable-substitution)
published by the OpenTelemetry Configuration Working Group. Changes both
the expand converter and the env provider. Defines one regex to be used
for both of these.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes
open-telemetry#9531

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@github-actions github-actions bot mentioned this issue Nov 11, 2024
Closed
@github-actions github-actions bot mentioned this issue Dec 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:confmap release:required-for-ga Must be resolved before GA release
Projects
Collector: v1
Archived in project
Milestone
go.opentelemetry.io/collector/confmap...
Development

Successfully merging a pull request may close this issue.

Restrict characters in environment variable names ankitpatel96/opentelemetry-collector
2 participants
@mx-psi @TylerHelmuth