fix: MarshalYAML receivers on TLSVersion and Curve

[mahendrapaipuri]

Currently the MarshalYAML receivers are defined on pointers of TLSVersion and Curve but TLSConfig is defined with values of TLSVersion and Curve in its fields. So, MarshalYAML receiver is never called upon. This commit fixes it by changing receivers to non-pointer values which should work on both values and pointers.

Reproducer that shows the behaviour:

package main

import (
	"crypto/tls"
	"errors"
	"fmt"

	"github.com/prometheus/exporter-toolkit/web"
	"gopkg.in/yaml.v3"
)

type TLSVersion uint16

var tlsVersions = map[string]TLSVersion{
	"TLS13": (TLSVersion)(tls.VersionTLS13),
	"TLS12": (TLSVersion)(tls.VersionTLS12),
	"TLS11": (TLSVersion)(tls.VersionTLS11),
	"TLS10": (TLSVersion)(tls.VersionTLS10),
}

func (tv *TLSVersion) UnmarshalYAML(unmarshal func(interface{}) error) error {
	var s string
	err := unmarshal((*string)(&s))
	if err != nil {
		return err
	}
	if v, ok := tlsVersions[s]; ok {
		*tv = v
		return nil
	}
	return errors.New("unknown TLS version: " + s)
}

// NOTE THAT WE HAVE CHANGED RECEIVER TO NON-POINTER VALUE
func (tv TLSVersion) MarshalYAML() (interface{}, error) {
	for s, v := range tlsVersions {
		if tv == v {
			return s, nil
		}
	}
	return fmt.Sprintf("%v", tv), nil
}

type Curve tls.CurveID

var curves = map[string]Curve{
	"CurveP256": (Curve)(tls.CurveP256),
	"CurveP384": (Curve)(tls.CurveP384),
	"CurveP521": (Curve)(tls.CurveP521),
	"X25519":    (Curve)(tls.X25519),
}

func (c *Curve) UnmarshalYAML(unmarshal func(interface{}) error) error {
	var s string
	err := unmarshal((*string)(&s))
	if err != nil {
		return err
	}
	if curveid, ok := curves[s]; ok {
		*c = curveid
		return nil
	}
	return errors.New("unknown curve: " + s)
}

// NOTE THAT WE HAVE CHANGED RECEIVER TO NON-POINTER VALUE
func (c Curve) MarshalYAML() (interface{}, error) {
	for s, curveid := range curves {
		if c == curveid {
			return s, nil
		}
	}
	return fmt.Sprintf("%v", c), nil
}

type NewConfig struct {
	TLSConfig TLSConfig `yaml:"tls_server_config"`
}

type TLSConfig struct {
	MinVersion       TLSVersion `yaml:"min_version"`
	MaxVersion       TLSVersion `yaml:"max_version"`
	CurvePreferences []Curve    `yaml:"curve_preferences"`
}

func main() {
	actualConfig := web.Config{
		TLSConfig: web.TLSConfig{
			MinVersion:       web.TLSVersion(tls.VersionTLS12),
			MaxVersion:       web.TLSVersion(tls.VersionTLS13),
			CurvePreferences: []web.Curve{web.Curve(tls.CurveP256)},
			CipherSuites:     []web.Cipher{web.Cipher(tls.TLS_AES_128_GCM_SHA256)},
		},
	}

	newConfig := NewConfig{
		TLSConfig: TLSConfig{
			MinVersion:       TLSVersion(tls.VersionTLS12),
			MaxVersion:       TLSVersion(tls.VersionTLS13),
			CurvePreferences: []Curve{Curve(tls.CurveP256)},
		},
	}

	actualConfigYAML, _ := yaml.Marshal(&actualConfig)
	fmt.Println("Actual Config struct: \n", string(actualConfigYAML))

	newConfigYAML, _ := yaml.Marshal(&newConfig)
	fmt.Println("Modified Config struct: \n", string(newConfigYAML))
}

This should produce an output as follows:

Actual Config struct 
tls_server_config:
    cert: ""
    key: null
    client_ca: ""
    cert_file: ""
    key_file: ""
    client_auth_type: ""
    client_ca_file: ""
    cipher_suites:
        - TLS_AES_128_GCM_SHA256
    curve_preferences:
        - 23
    min_version: 771
    max_version: 772
    prefer_server_cipher_suites: false
    client_allowed_sans: []
http_server_config:
    http2: false
basic_auth_users: {}

Modified Config struct: 
tls_server_config:
    min_version: TLS12
    max_version: TLS13
    curve_preferences:
        - CurveP256

With current package's implementation, yaml.Marshal produces uint for min_version, max_version and curve_preferneces. When changed to receivers of MarshalYAML to non-pointer values, marshalled config produces correct output.

This PR fixes the marshall receivers and add unit tests to verify config file generation from structs. omitempty has been added to the config fields to avoid producing "zero" values when config file is being generated from Go structs. When fields are not set, the library use "sane" defaults which can be leveraged by client libraries.

[SuperQ]

Seems like we should have tests for this behavior.

[mahendrapaipuri]

Cheers @SuperQ !! Currently there are no unit tests that generates YAML files from Go structs. Happy to add few unit tests for this use case.

[SuperQ]

Yes, if you could add some tests, that would be very useful.

[mahendrapaipuri]

@SuperQ Added few unit tests and updated PR description

[ataleksandrov]

Do you have any plans to merge this? We are also looking for this fix.

[roidelapluie]

Can you rebase this? Thanks!

[mahendrapaipuri]

@roidelapluie Should be good now!!

[roidelapluie]

Please remove all the omitempty and let's have this in another PR where we can discuss which ones are valuable and which one are not wanted.

[mahendrapaipuri]

Well, without omitempty, the marshalling config structs can produce "invalid" config files which is not desired I assume. For instance, if we do not set min_version or max_version in config struct, withot omitempty, the config file will contain min_version: "0" which is not valid.

[roidelapluie]

Yes it depends on the fields, but some of them are really off. I do think we would want to discuss this separately

[roidelapluie]

Please use t.Fatalf and subtests

[mahendrapaipuri]

Ok, will do that! Cheers!

[roidelapluie]

Maybe we can have narrower tests as wel.

Additionally, this test could test the round trip as well.

[mahendrapaipuri]

Could you please elaborate on narrower tests? What exactly do you see? Cheers!

[roidelapluie]

testing marshal/unmarshal on curve directly as well

[roidelapluie]

(removing approval mark for now)