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] Should not marshal functions/channels by default #8627

Closed
jefchien opened this issue Oct 5, 2023 · 6 comments · Fixed by #10310 or #10686
Closed

[confmap] Should not marshal functions/channels by default #8627

jefchien opened this issue Oct 5, 2023 · 6 comments · Fixed by #10310 or #10686
Assignees
Labels
bug Something isn't working

Comments

@jefchien
Copy link
Contributor

jefchien commented Oct 5, 2023

Describe the bug
mapstructure will take any exported field and attempt to decode it. This includes non-tagged fields.

When decoding to a struct, mapstructure will use the field name by default to perform the mapping. For example, if a struct has a field "Username" then mapstructure will look for a key in the source value of "username" (case insensitive).

type User struct {
   Username string
}

https://pkg.go.dev/github.com/mitchellh/mapstructure

Similarly, the mapstructure encoder will also take any exported field and attempt to encode it into a form that go-yaml can use. The encoder does not handle function or channel types. Any exported function/channel which will use the default handler, which will fall back on leaving the field as is. go-yaml cannot handle the function/channel and panics.

One example of an exported function in the configuration is the confighttp.HTTPClientSettings, which exports the CustomRoundTripper function:

// Custom Round Tripper to allow for individual components to intercept HTTP requests
CustomRoundTripper func(next http.RoundTripper) (http.RoundTripper, error)

Steps to reproduce

func TestMarshal(t *testing.T) {
    settings := confighttp.NewDefaultHTTPClientSettings()
    conf := confmap.New()
    assert.NoError(t, conf.Marshal(settings))
    out, err := yaml.Marshal(conf.ToStringMap())
    assert.NoError(t, err)
    t.Log(string(out))
}

What did you expect to see?
That the function would be skipped. customroundtripper is not present in the output YAML.

        auth: null
        compression: ""
        disable_keep_alives: false
        endpoint: ""
        headers: {}
        idle_conn_timeout: 1m30s
        max_conns_per_host: null
        max_idle_conns: 100
        max_idle_conns_per_host: null
        read_buffer_size: 0
        timeout: 0s
        tls:
            ca_file: ""
            ca_pem: '[REDACTED]'
            cert_file: ""
            cert_pem: '[REDACTED]'
            insecure: false
            insecure_skip_verify: false
            key_file: ""
            key_pem: '[REDACTED]'
            max_version: ""
            min_version: ""
            reload_interval: 0s
            server_name_override: ""
        write_buffer_size: 0

What did you see instead?

panic: cannot marshal type: func(http.RoundTripper) (http.RoundTripper, error)

panic({0x1779440, 0xc0003ef630})
	/path/to/go/src/runtime/panic.go:884 +0x213
gopkg.in/yaml%2ev3.(*encoder).marshal(0x179b500?, {0x0, 0x0}, {0x17988c0?, 0x0?, 0x10?})
	/path/to/go/pkg/mod/gopkg.in/[email protected]/encode.go:182 +0xa4b
gopkg.in/yaml%2ev3.(*encoder).marshal(0x17b13e0?, {0x0, 0x0}, {0x179b500?, 0xc0003ef620?, 0x98?})
	/path/to/go/pkg/mod/gopkg.in/[email protected]/encode.go:162 +0x85b
gopkg.in/yaml%2ev3.(*encoder).mapv.func1()
	/path/to/go/pkg/mod/gopkg.in/[email protected]/encode.go:192 +0xf4
gopkg.in/yaml%2ev3.(*encoder).mappingv(0xc0003e4400, {0x0?, 0x0}, 0xc0001e1890)
	/path/to/go/pkg/mod/gopkg.in/[email protected]/encode.go:265 +0x146
gopkg.in/yaml%2ev3.(*encoder).mapv(0x17b13e0?, {0x0?, 0xc0003d6960?}, {0x17b13e0?, 0xc0003d6960?, 0x0?})
	/path/to/go/pkg/mod/gopkg.in/[email protected]/encode.go:187 +0x5f
gopkg.in/yaml%2ev3.(*encoder).marshal(0xc0003e4400?, {0x0, 0x0}, {0x17b13e0?, 0xc0003d6960?, 0xc0001e1a60?})
	/path/to/go/pkg/mod/gopkg.in/[email protected]/encode.go:164 +0x88b
gopkg.in/yaml%2ev3.(*encoder).marshalDoc(0xc0003e4400, {0x0, 0x0}, {0x17b13e0?, 0xc0003d6960?, 0x1021445?})
	/path/to/go/pkg/mod/gopkg.in/[email protected]/encode.go:105 +0x12b
gopkg.in/yaml%2ev3.Marshal({0x17b13e0?, 0xc0003d6960})
	/path/to/go/pkg/mod/gopkg.in/[email protected]/yaml.go:222 +0x389
go.opentelemetry.io/collector/config/confighttp.TestMarshal(0xc00019d520)

What version did you use?
Version: v0.86.0

What config did you use?
Config: N/A

Environment
OS: (e.g., "Ubuntu 20.04") macOS
Compiler(if manually compiled): Go 1.20.8

Additional context
Add any other context about the problem here.

@jefchien jefchien added the bug Something isn't working label Oct 5, 2023
@mx-psi
Copy link
Member

mx-psi commented Oct 5, 2023

My vote is on moving the CustomRoundTripper field out of the HTTPClientSettings and instead have it as an argument for ToClient. Configuration structs should only have public fields that are set by end-users in YAML

@jefchien
Copy link
Contributor Author

I would still rather have the marshaler skip the fields. Ran into this issue with another configuration in the contrib (https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/stanza/operator/input/tcp/tcp.go#L79) and, while that should be fixed as well, doing it in the marshaler will prevent these changes from breaking the functionality.

@mx-psi
Copy link
Member

mx-psi commented Dec 12, 2023

@djaglowski any thoughts on this? Does pkg/stanza have any others functions/channels on the configuration? Is it possible/desirable to move them to a different place?

@djaglowski
Copy link
Member

Does pkg/stanza have any others functions/channels on the configuration? Is it possible/desirable to move them to a different place?

I don't believe it's a common pattern, at least not to have them exported. In this case, I don't see any reason why the we couldn't just unexport the field.

@evan-bradley
Copy link
Contributor

evan-bradley commented Jun 4, 2024

In this case, I don't see any reason why the we couldn't just unexport the field.

It had to be exported because that was the only way to set it. However, it's unused in any non-test code inside upstream components, so I agree we can essentially hide it.

Considering it can easily be set on the client returned from ToClient, I'm going to propose that we remove it and anyone who wants to set this can do so themselves.

mx-psi pushed a commit that referenced this issue Jun 5, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Deprecates the `CustomRoundTripper` field on `confighttp.ClientConfig`,
which is unused outside tests in Contrib and causes errors because it
cannot be unmarshaled or marshaled. Additionally, having a
non-configurable field on a Config struct seems non-ideal.

Soft depends on
open-telemetry/opentelemetry-collector-contrib#33371
so we're not using deprecated APIs.

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

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

Adapted tests to how the new way of doing this will look. It's slightly
less ergonomic (you can't load up all the settings then just run
`ToClient`), but we have no examples of this being used by any
components, so I'm reluctant to add it to the API.
mx-psi pushed a commit that referenced this issue Jul 26, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Delete deprecated `ClientConfig.CustomRoundTripper`

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes #8627
@rlankfo
Copy link

rlankfo commented Sep 27, 2024

Considering it can easily be set on the client returned from ToClient, I'm going to propose that we remove it and anyone who wants to set this can do so themselves.

@evan-bradley looks like ToClient returns a copy of the client, so we can't actually set a custom transport to be used by a component. Am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment