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] Unify behavior of ${env:ENV} and ${ENV} #10161

Closed
mx-psi opened this issue May 15, 2024 · 5 comments · Fixed by #10510
Closed

[confmap] Unify behavior of ${env:ENV} and ${ENV} #10161

mx-psi opened this issue May 15, 2024 · 5 comments · Fixed by #10510
Assignees
Labels
area:confmap priority:p2 Medium release:required-for-ga Must be resolved before GA release

Comments

@mx-psi
Copy link
Member

mx-psi commented May 15, 2024

As decided on #9854, we want to unify the behavior of ${env:ENV} and ${ENV}. Currently, each has a different behavior: the ${ENV} syntax does not do any YAML parsing, while the ${env:ENV} syntax does parse the contents as YAML. We want to unify both and make them have the same behavior, described in RFC #9854.

One approach for this is to have some 'default' confmap provider used by the ${..} syntax, and control what provider is used (a expandconverter provider or the env provider) through a feature gate.

cc @TylerHelmuth

@mx-psi mx-psi added release:required-for-ga Must be resolved before GA release priority:p2 Medium area:confmap labels May 15, 2024
@TylerHelmuth TylerHelmuth changed the title [confmap] Unify behavior of ${env:ENV} and ${ENV}` [confmap] Unify behavior of ${env:ENV} and ${ENV} May 15, 2024
@TylerHelmuth
Copy link
Member

TylerHelmuth commented May 16, 2024

@mx-psi I see a couple approaches.

  1. Allow envprovider to expand ${env:} and ${}. This is conceptually simple, but I think breaks some logic in resolver.expandValue that requires providers to have an schema so it can identify which provider to use.
  2. Allow resolver.expandValue to expand ${} as an env var when it doesn't contain a uri (basically do env var expansion here). This is doable, but would mean duplicating the yaml parsing logic and logging logic (which means adding a Logger to the ResolverSettings) from the envprovide.
  3. Leave expandconverter in place, but restrict it to only allowing ${} and add yaml parsing.
  4. Create the concept of a "default" provider that is used by the Resolver when no schema is set. This would be a new field on the ResolverSettings. For our collector instances we'd set the default to be the envprovider. The envprovider would need to be updated to not error when no schema is provided.

I've experimented with 2 and think it would be workable. It would take care of this issue and #8215 at once. It did feel a little messy but there are probably ways to share code between the 2 places.

I would also be ok with option 4, as it provides a lot of flexibility.

Option 3 is pretty simple as well.

@TylerHelmuth TylerHelmuth self-assigned this May 16, 2024
@TylerHelmuth TylerHelmuth moved this from Todo to In Progress in Collector: v1 May 16, 2024
@evan-bradley
Copy link
Contributor

Option 4 feels the cleanest to me. I think we should do it without the awareness of any of the Providers by prepending the default schema to the URI when the URI is given to the Provider, like the Resolver currently does with files passed to config arguments. If we provide this option, we may also want to consider whether to make it configurable for the command line flags

@TylerHelmuth
Copy link
Member

I think we should do it without the awareness of any of the Providers by prepending the default schema to the URI when the URI is given to the Provider

Ya that would work. I don't love the idea of messing with the incoming values, but if we already have precedence I can be ok with it.

If we provide this option, we may also want to consider whether to make it configurable for the command line flags

I think this is an option, but not required for 1.0

@mx-psi
Copy link
Member Author

mx-psi commented May 17, 2024

Agreed that option 4 feels like the cleanest. If we go with (4), for now I don't think we need to make this configurable anywhere other than the Go API, but we can open the door to make this configurable in the future by end-users

@TylerHelmuth
Copy link
Member

If we go with (4), for now I don't think we need to make this configurable anywhere other than the Go API, but we can open the door to make this configurable in the future by end-users

Agreed. I'll start work on option 4. I try to do this and #7111 in one.

codeboten pushed a commit that referenced this issue May 29, 2024
#### Description
This PR adds support for expanding `${}` syntax when no schema is
provided by allowing Resolver to use a default provider.

In a followup PR I'll update otelcol with a feature gate that allow
switching between a default envProvider and the expandconverter.

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

#### Testing
Added unit tests

#### Documentation
Added godoc strings

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this issue Jun 14, 2024
#### Description
This PR adds support for expanding `${}` syntax when no schema is
provided by allowing Resolver to use a default provider.

In a followup PR I'll update otelcol with a feature gate that allow
switching between a default envProvider and the expandconverter.

#### Link to tracking issue
Related to
open-telemetry#10161
Related to
open-telemetry#7111

#### Testing
Added unit tests

#### Documentation
Added godoc strings

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
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 <pbaeyens31+github@gmail.com>
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 <11745660+evan-bradley@users.noreply.github.com>
@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:confmap 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.

3 participants