[confmap] Should not marshal functions/channels by default

[jefchien]

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: https://github.com/open-telemetry/opentelemetry-collector/blob/b5635a7a90d204453915db36e4e29182ee2dddb2/config/confighttp/confighttp.go#L52-L53

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.

[mx-psi]

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]

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]

@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]

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]

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.

[rlankfo]

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?