Skip to content

Commit

Permalink
[confmap] Use values as string if YAML is invalid (#10794)
Browse files Browse the repository at this point in the history
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

If YAML parsing fails, assume the user wanted to pass the value as a
string.

This has the downside that the error messages are less informative: it
will tell you it expected something other than a string instead of the
YAML parser error.

A future improvement could be to pass these errors down as extra
metadata up until the unmarshaling stage.

#### Link to tracking issue

Fixes #10759

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

<!--Describe the documentation added.-->

Added test case for this.
  • Loading branch information
mx-psi authored Aug 5, 2024
1 parent 685625a commit 78df5c7
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 12 deletions.
26 changes: 26 additions & 0 deletions .chloggen/mx-psi_fix-invalid-yaml-string.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Use this changelog template to create an entry for release notes.

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

# 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: If loading an invalid YAML string through a provider, use it verbatim instead of erroring out.

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

# (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: |
This makes the ${env:ENV} syntax closer to how ${ENV} worked before unifying syntaxes.
# 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: []
11 changes: 11 additions & 0 deletions confmap/internal/e2e/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,17 @@ func TestStrictTypeCasting(t *testing.T) {
targetField: TargetFieldInlineString,
expected: "inline field with 2006-01-02T15:04:05Z07:00 expansion",
},
// issue 10759
{
value: `["a",`,
targetField: TargetFieldString,
expected: `["a",`,
},
{
value: `["a",`,
targetField: TargetFieldInlineString,
expected: `inline field with ["a", expansion`,
},
}

previousValue := globalgates.StrictlyTypedInputGate.IsEnabled()
Expand Down
5 changes: 5 additions & 0 deletions confmap/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ func withStringRepresentation(stringRepresentation string) RetrievedOption {
func NewRetrievedFromYAML(yamlBytes []byte, opts ...RetrievedOption) (*Retrieved, error) {
var rawConf any
if err := yaml.Unmarshal(yamlBytes, &rawConf); err != nil {
if globalgates.StrictlyTypedInputGate.IsEnabled() {
// If the string is not valid YAML, we try to use it verbatim as a string.
strRep := string(yamlBytes)
return NewRetrieved(strRep, append(opts, withStringRepresentation(strRep))...)
}
return nil, err
}

Expand Down
9 changes: 6 additions & 3 deletions confmap/provider/envprovider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,14 @@ func TestUnsupportedScheme(t *testing.T) {
}

func TestInvalidYAML(t *testing.T) {
const envName = "invalid-yaml"
const envName = "invalid_yaml"
t.Setenv(envName, "[invalid,")
env := createProvider()
_, err := env.Retrieve(context.Background(), envSchemePrefix+envName, nil)
assert.Error(t, err)
ret, err := env.Retrieve(context.Background(), envSchemePrefix+envName, nil)
require.NoError(t, err)
raw, err := ret.AsRaw()
require.NoError(t, err)
assert.IsType(t, "", raw)
assert.NoError(t, env.Shutdown(context.Background()))
}

Expand Down
16 changes: 12 additions & 4 deletions confmap/provider/fileprovider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,18 @@ func TestNonExistent(t *testing.T) {

func TestInvalidYAML(t *testing.T) {
fp := createProvider()
_, err := fp.Retrieve(context.Background(), fileSchemePrefix+filepath.Join("testdata", "invalid-yaml.yaml"), nil)
assert.Error(t, err)
_, err = fp.Retrieve(context.Background(), fileSchemePrefix+absolutePath(t, filepath.Join("testdata", "invalid-yaml.yaml")), nil)
assert.Error(t, err)
ret, err := fp.Retrieve(context.Background(), fileSchemePrefix+filepath.Join("testdata", "invalid-yaml.yaml"), nil)
require.NoError(t, err)
raw, err := ret.AsRaw()
require.NoError(t, err)
assert.IsType(t, "", raw)

ret, err = fp.Retrieve(context.Background(), fileSchemePrefix+absolutePath(t, filepath.Join("testdata", "invalid-yaml.yaml")), nil)
require.NoError(t, err)
raw, err = ret.AsRaw()
require.NoError(t, err)
assert.IsType(t, "", raw)

require.NoError(t, fp.Shutdown(context.Background()))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,11 @@ func TestInvalidYAML(t *testing.T) {
}
}))
defer ts.Close()
_, err := fp.Retrieve(context.Background(), ts.URL, nil)
assert.Error(t, err)
ret, err := fp.Retrieve(context.Background(), ts.URL, nil)
require.NoError(t, err)
raw, err := ret.AsRaw()
require.NoError(t, err)
assert.Equal(t, "wrong : [", raw)
require.NoError(t, fp.Shutdown(context.Background()))
}

Expand Down
9 changes: 7 additions & 2 deletions confmap/provider/yamlprovider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"go.opentelemetry.io/collector/confmap"
"go.opentelemetry.io/collector/confmap/confmaptest"
Expand All @@ -26,8 +27,12 @@ func TestEmpty(t *testing.T) {

func TestInvalidYAML(t *testing.T) {
sp := createProvider()
_, err := sp.Retrieve(context.Background(), "yaml:[invalid,", nil)
assert.Error(t, err)
ret, err := sp.Retrieve(context.Background(), "yaml:[invalid,", nil)
require.NoError(t, err)
raw, err := ret.AsRaw()
require.NoError(t, err)
assert.IsType(t, "", raw)

assert.NoError(t, sp.Shutdown(context.Background()))
}

Expand Down
13 changes: 12 additions & 1 deletion confmap/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,19 @@ func TestNewRetrievedFromYAMLWithOptions(t *testing.T) {
}

func TestNewRetrievedFromYAMLInvalidYAMLBytes(t *testing.T) {
_, err := NewRetrievedFromYAML([]byte("[invalid:,"))
ret, err := NewRetrievedFromYAML([]byte("[invalid:,"))
require.NoError(t, err)

_, err = ret.AsConf()
assert.Error(t, err)

str, err := ret.AsString()
require.NoError(t, err)
assert.Equal(t, "[invalid:,", str)

raw, err := ret.AsRaw()
require.NoError(t, err)
assert.Equal(t, "[invalid:,", raw)
}

func TestNewRetrievedFromYAMLInvalidAsMap(t *testing.T) {
Expand Down

0 comments on commit 78df5c7

Please sign in to comment.