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] Add strict type validation under a feature gate #10400

Merged
merged 12 commits into from
Jun 17, 2024
25 changes: 25 additions & 0 deletions .chloggen/mx-psi_asstring.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: enhancement

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

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Adds `confmap.Retrieved.AsString` method that returns the configuration value as a string"

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

# (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/mx-psi_newretrievedfromyaml.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: enhancement

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

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Adds `confmap.NewRetrievedFromYAML` helper to create `confmap.Retrieved` values from YAML bytes"

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

# (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]
28 changes: 28 additions & 0 deletions .chloggen/mx-psi_weaklytypedinput.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Use this changelog template to create an entry for release notes.

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

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

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Adds alpha `confmap.strictlyTypedInput` feature gate that enables strict type checks during configuration resolution"

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

# (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: |
When enabled, the configuration resolution system will:
- Stop doing most kinds of implicit type casting when resolving configuration values
- Use the original string representation of configuration values if the ${} syntax is used in inline position

# 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: []
2 changes: 2 additions & 0 deletions cmd/mdatagen/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ require (
github.com/go-viper/mapstructure/v2 v2.0.0-alpha.1 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/hashicorp/go-version v1.7.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/knadh/koanf/maps v0.1.1 // indirect
github.com/knadh/koanf/providers/confmap v0.1.0 // indirect
Expand All @@ -45,6 +46,7 @@ require (
github.com/prometheus/client_model v0.6.1 // indirect
github.com/prometheus/common v0.54.0 // indirect
github.com/prometheus/procfs v0.15.0 // indirect
go.opentelemetry.io/collector/featuregate v1.9.0 // indirect
go.opentelemetry.io/otel/exporters/prometheus v0.49.0 // indirect
go.opentelemetry.io/otel/sdk v1.27.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions cmd/mdatagen/go.sum

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

4 changes: 4 additions & 0 deletions config/configauth/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-viper/mapstructure/v2 v2.0.0-alpha.1 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/hashicorp/go-version v1.7.0 // indirect
github.com/knadh/koanf/maps v0.1.1 // indirect
github.com/knadh/koanf/providers/confmap v0.1.0 // indirect
github.com/knadh/koanf/v2 v2.1.1 // indirect
Expand All @@ -22,6 +23,7 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
go.opentelemetry.io/collector/config/configtelemetry v0.102.1 // indirect
go.opentelemetry.io/collector/confmap v0.102.1 // indirect
go.opentelemetry.io/collector/featuregate v1.9.0 // indirect
go.opentelemetry.io/collector/pdata v1.9.0 // indirect
go.opentelemetry.io/otel v1.27.0 // indirect
go.opentelemetry.io/otel/metric v1.27.0 // indirect
Expand All @@ -48,3 +50,5 @@ replace go.opentelemetry.io/collector/config/configtelemetry => ../configtelemet
replace go.opentelemetry.io/collector/extension => ../../extension

replace go.opentelemetry.io/collector/extension/auth => ../../extension/auth

replace go.opentelemetry.io/collector/featuregate => ../../featuregate
2 changes: 2 additions & 0 deletions config/configauth/go.sum

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

3 changes: 2 additions & 1 deletion confmap/confmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/knadh/koanf/providers/confmap"
"github.com/knadh/koanf/v2"

"go.opentelemetry.io/collector/confmap/internal"
encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure"
)

Expand Down Expand Up @@ -156,7 +157,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool, skipTopLevelUnmarshaler
ErrorUnused: errorUnused,
Result: result,
TagName: "mapstructure",
WeaklyTypedInput: true,
WeaklyTypedInput: !internal.StrictlyTypedInputGate.IsEnabled(),
MatchName: caseSensitiveMatchName,
DecodeHook: mapstructure.ComposeDecodeHookFunc(
expandNilStructPointersHookFunc(),
Expand Down
28 changes: 23 additions & 5 deletions confmap/expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
"regexp"
"strconv"
"strings"

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

// schemePattern defines the regexp pattern for scheme names.
Expand Down Expand Up @@ -111,7 +113,12 @@
if uri == input {
// If the value is a single URI, then the return value can be anything.
// This is the case `foo: ${file:some_extra_config.yml}`.
expanded, err := mr.expandURI(ctx, input)
ret, err := mr.expandURI(ctx, input)
if err != nil {
return input, false, err
}

expanded, err := ret.AsRaw()
if err != nil {
return input, false, err
}
Expand All @@ -121,16 +128,27 @@
if err != nil {
return input, false, err
}
repl, err := toString(expanded)

var repl string
if internal.StrictlyTypedInputGate.IsEnabled() {
repl, err = expanded.AsString()

Check warning on line 134 in confmap/expand.go

View check run for this annotation

Codecov / codecov/patch

confmap/expand.go#L134

Added line #L134 was not covered by tests
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
} else {
repl, err = toString(expanded)
}
if err != nil {
return input, false, fmt.Errorf("expanding %v: %w", uri, err)
}
return strings.ReplaceAll(input, uri, repl), true, err
}

// toString attempts to convert input to a string.
func toString(input any) (string, error) {
func toString(ret *Retrieved) (string, error) {
// This list must be kept in sync with checkRawConfType.
input, err := ret.AsRaw()
if err != nil {
return "", err

Check warning on line 149 in confmap/expand.go

View check run for this annotation

Codecov / codecov/patch

confmap/expand.go#L149

Added line #L149 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this covered by e2e tests as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to hit this path today

return r.rawConf, nil

}

val := reflect.ValueOf(input)
switch val.Kind() {
case reflect.String:
Expand All @@ -146,7 +164,7 @@
}
}

func (mr *Resolver) expandURI(ctx context.Context, input string) (any, error) {
func (mr *Resolver) expandURI(ctx context.Context, input string) (*Retrieved, error) {
// strip ${ and }
uri := input[2 : len(input)-1]

Expand All @@ -167,7 +185,7 @@
return nil, err
}
mr.closers = append(mr.closers, ret.Close)
return ret.AsRaw()
return ret, nil
}

type location struct {
Expand Down
7 changes: 4 additions & 3 deletions confmap/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ require (
github.com/knadh/koanf/providers/confmap v0.1.0
github.com/knadh/koanf/v2 v2.1.1
github.com/stretchr/testify v1.9.0
go.opentelemetry.io/collector/featuregate v1.9.0
go.uber.org/goleak v1.3.0
go.uber.org/multierr v1.11.0
go.uber.org/zap v1.27.0
Expand All @@ -16,15 +17,15 @@ require (

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/kr/pretty v0.3.1 // indirect
github.com/hashicorp/go-version v1.7.0 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rogpeppe/go-internal v1.10.0 // indirect
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
)

retract (
v0.76.0 // Depends on retracted pdata v1.0.0-rc10 module, use v0.76.1
v0.69.0 // Release failed, use v0.69.1
)

replace go.opentelemetry.io/collector/featuregate => ../featuregate
8 changes: 2 additions & 6 deletions confmap/go.sum

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

4 changes: 4 additions & 0 deletions confmap/internal/e2e/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ require (
go.opentelemetry.io/collector/confmap v0.102.1
go.opentelemetry.io/collector/confmap/provider/envprovider v0.102.1
go.opentelemetry.io/collector/confmap/provider/fileprovider v0.102.1
go.opentelemetry.io/collector/featuregate v1.9.0
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-viper/mapstructure/v2 v2.0.0 // indirect
github.com/hashicorp/go-version v1.7.0 // indirect
github.com/knadh/koanf/maps v0.1.1 // indirect
github.com/knadh/koanf/providers/confmap v0.1.0 // indirect
github.com/knadh/koanf/v2 v2.1.1 // indirect
Expand All @@ -28,3 +30,5 @@ replace go.opentelemetry.io/collector/confmap => ../../
replace go.opentelemetry.io/collector/confmap/provider/fileprovider => ../../provider/fileprovider

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

replace go.opentelemetry.io/collector/featuregate => ../../../featuregate
2 changes: 2 additions & 0 deletions confmap/internal/e2e/go.sum

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

Loading
Loading