Skip to content

Commit

Permalink
[confmap] Deprecate expandconverter (#10510)
Browse files Browse the repository at this point in the history
#### Description
This PR deprecates `expandconverter` and removes its use from
`otelcoltest.LoadConfig` and OCB.

This cannot be merged until the `confmap.unifyEnvVarExpansion` [feature
gate is made
stable](#10508).

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

---------

Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
  • Loading branch information
3 people authored Aug 7, 2024
1 parent 981657b commit 2b9697f
Show file tree
Hide file tree
Showing 16 changed files with 376 additions and 38 deletions.
25 changes: 25 additions & 0 deletions .chloggen/deprecate-expandconverter-2.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: otelcoltest

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: The `otelcol.LoadConfig` method no longer sets the `expandconverter`.

# One or more tracking issues or pull requests related to the change
issues: [10510]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
25 changes: 25 additions & 0 deletions .chloggen/deprecate-expandconverter-3.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: ocb

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Collectors built with OCB will no longer include the `expandconverter`

# One or more tracking issues or pull requests related to the change
issues: [10510]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
25 changes: 25 additions & 0 deletions .chloggen/deprecate-expandconverter.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: deprecation

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: expandconverter

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Deprecate `expandconverter`.

# One or more tracking issues or pull requests related to the change
issues: [10510]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
2 changes: 0 additions & 2 deletions cmd/builder/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ processors:
providers:
- gomod: go.opentelemetry.io/collector/confmap/provider/fileprovider v0.99.0

converters:
- gomod: go.opentelemetry.io/collector/confmap/converter/expandconverter v0.99.0
EOF
$ builder --config=otelcol-builder.yaml
$ cat > /tmp/otelcol.yaml <<EOF
Expand Down
1 change: 0 additions & 1 deletion cmd/builder/internal/builder/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ var (
"/config/configtls",
"/config/internal",
"/confmap",
"/confmap/converter/expandconverter",
"/confmap/provider/envprovider",
"/confmap/provider/fileprovider",
"/confmap/provider/httpprovider",
Expand Down
1 change: 0 additions & 1 deletion cmd/builder/internal/builder/templates/go.mod.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ go 1.21

require (
{{if .Distribution.SupportsConfmapFactories -}}
go.opentelemetry.io/collector/confmap/converter/expandconverter v{{.Distribution.OtelColVersion}}
{{- range .Providers}}
{{if .GoMod}}{{.GoMod}}{{end}}
{{- end}}
Expand Down
4 changes: 0 additions & 4 deletions cmd/builder/internal/builder/templates/main.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"go.opentelemetry.io/collector/component"
{{- if .Distribution.SupportsConfmapFactories}}
"go.opentelemetry.io/collector/confmap"
"go.opentelemetry.io/collector/confmap/converter/expandconverter"
{{- range .Providers}}
{{.Name}} "{{.Import}}"
{{- end}}
Expand Down Expand Up @@ -38,9 +37,6 @@ func main() {
{{- if .ConfResolver.DefaultURIScheme }}
DefaultScheme: "{{ .ConfResolver.DefaultURIScheme }}",
{{- end }}
ConverterFactories: []confmap.ConverterFactory{
expandconverter.NewFactory(),
},
},
},
{{- end}}
Expand Down
1 change: 0 additions & 1 deletion cmd/builder/test/core.builder.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ replaces:
- go.opentelemetry.io/collector/config/configtls => ${WORKSPACE_DIR}/config/configtls
- go.opentelemetry.io/collector/config/internal => ${WORKSPACE_DIR}/config/internal
- go.opentelemetry.io/collector/confmap => ${WORKSPACE_DIR}/confmap
- go.opentelemetry.io/collector/confmap/converter/expandconverter => ${WORKSPACE_DIR}/confmap/converter/expandconverter
- go.opentelemetry.io/collector/confmap/provider/envprovider => ${WORKSPACE_DIR}/confmap/provider/envprovider
- go.opentelemetry.io/collector/confmap/provider/fileprovider => ${WORKSPACE_DIR}/confmap/provider/fileprovider
- go.opentelemetry.io/collector/confmap/provider/httpprovider => ${WORKSPACE_DIR}/confmap/provider/httpprovider
Expand Down
1 change: 0 additions & 1 deletion cmd/otelcorecol/builder-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ replaces:
- go.opentelemetry.io/collector/config/configtls => ../../config/configtls
- go.opentelemetry.io/collector/config/internal => ../../config/internal
- go.opentelemetry.io/collector/confmap => ../../confmap
- go.opentelemetry.io/collector/confmap/converter/expandconverter => ../../confmap/converter/expandconverter
- go.opentelemetry.io/collector/confmap/provider/envprovider => ../../confmap/provider/envprovider
- go.opentelemetry.io/collector/confmap/provider/fileprovider => ../../confmap/provider/fileprovider
- go.opentelemetry.io/collector/confmap/provider/httpprovider => ../../confmap/provider/httpprovider
Expand Down
3 changes: 0 additions & 3 deletions cmd/otelcorecol/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ toolchain go1.21.12
require (
go.opentelemetry.io/collector/component v0.106.1
go.opentelemetry.io/collector/confmap v0.106.1
go.opentelemetry.io/collector/confmap/converter/expandconverter v0.106.1
go.opentelemetry.io/collector/confmap/provider/envprovider v0.106.1
go.opentelemetry.io/collector/confmap/provider/fileprovider v0.106.1
go.opentelemetry.io/collector/confmap/provider/httpprovider v0.106.1
Expand Down Expand Up @@ -170,8 +169,6 @@ replace go.opentelemetry.io/collector/config/internal => ../../config/internal

replace go.opentelemetry.io/collector/confmap => ../../confmap

replace go.opentelemetry.io/collector/confmap/converter/expandconverter => ../../confmap/converter/expandconverter

replace go.opentelemetry.io/collector/confmap/provider/envprovider => ../../confmap/provider/envprovider

replace go.opentelemetry.io/collector/confmap/provider/fileprovider => ../../confmap/provider/fileprovider
Expand Down
4 changes: 0 additions & 4 deletions cmd/otelcorecol/main.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 5 additions & 15 deletions confmap/converter/expandconverter/expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,11 @@ import (
"context"
"fmt"
"os"
"regexp"

"go.uber.org/zap"

"go.opentelemetry.io/collector/confmap"
"go.opentelemetry.io/collector/confmap/internal/envvar"
"go.opentelemetry.io/collector/internal/globalgates"
)

type converter struct {
Expand All @@ -25,6 +23,9 @@ type converter struct {

// NewFactory returns a factory for a confmap.Converter,
// which expands all environment variables for a given confmap.Conf.
//
// Deprecated: [v0.107.0] BASH-style env var expansion is deprecated. Use the `envprovider` instead to expand `${FOO}` and `${env:FOO}`.
// Using the expandconverter with `confmap.Resolver` will cause double escaping, so `$$$$` -> `$` instead of `$$`.
func NewFactory() confmap.ConverterFactory {
return confmap.NewConverterFactory(newConverter)
}
Expand All @@ -36,6 +37,8 @@ func newConverter(set confmap.ConverterSettings) confmap.Converter {
}
}

// Deprecated: [v0.107.0] BASH-style env var expansion is deprecated. Use the `envprovider` instead to expand `${FOO}` and `${env:FOO}`.
// Using the expandconverter with `confmap.Resolver` will cause double escaping, so `$$$$` -> `$` instead of `$$`.
func (c converter) Convert(_ context.Context, conf *confmap.Conf) error {
var err error
out := make(map[string]any)
Expand Down Expand Up @@ -90,19 +93,6 @@ func (c converter) expandEnv(s string) (string, error) {
return "$"
}

// Matches on $VAR style environment variables
// in order to make sure we don't log a warning for ${VAR}
var regex = regexp.MustCompile(fmt.Sprintf(`\$%s`, regexp.QuoteMeta(str)))
if _, exists := c.loggedDeprecations[str]; !exists && regex.MatchString(s) {
if globalgates.UseUnifiedEnvVarExpansionRules.IsEnabled() {
err = fmt.Errorf("variable substitution using $VAR has been deprecated in favor of ${VAR} and ${env:VAR} - please update $%s or temporarily disable the confmap.unifyEnvVarExpansion feature gate", str)
return ""
}
msg := fmt.Sprintf("Variable substitution using $VAR will be deprecated in favor of ${VAR} and ${env:VAR}, please update $%s", str)
c.logger.Warn(msg, zap.String("variable", str))
c.loggedDeprecations[str] = struct{}{}
}

// For $ENV style environment variables os.Expand returns once it hits a character that isn't an underscore or
// an alphanumeric character - so we cannot detect those malformed environment variables.
// For ${ENV} style variables we can detect those kinds of env var names!
Expand Down
Loading

0 comments on commit 2b9697f

Please sign in to comment.