Skip to content

fix: MarshalYAML receivers on TLSVersion and Curve #288

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.

Sign up for GitHub

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

Open
mahendrapaipuri wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: MarshalYAML receivers on TLSVersion and Curve #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d3e61fc
fix: MarshalYAML receivers on `TLSVersion` and `Curve`
mahendrapaipuri Jan 28, 2025
d658754
Use values inside non-pointer MarshalYAML receivers
mahendrapaipuri Feb 1, 2025
a0ec107
tests: Add unit tests to verify config generation from structs
mahendrapaipuri Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions web/tls_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,26 +42,26 @@ var (
)

type Config struct {
TLSConfig TLSConfig `yaml:"tls_server_config"`
HTTPConfig HTTPConfig `yaml:"http_server_config"`
RateLimiterConfig RateLimiterConfig `yaml:"rate_limit"`
Users map[string]config_util.Secret `yaml:"basic_auth_users"`
TLSConfig TLSConfig `yaml:"tls_server_config,omitempty"`
Copy link
Copy Markdown
Member

@roidelapluie roidelapluie Apr 9, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@mahendrapaipuri mahendrapaipuri Apr 10, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@roidelapluie roidelapluie Apr 10, 2026

Choose a reason for hiding this comment

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

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

HTTPConfig HTTPConfig `yaml:"http_server_config,omitempty"`
RateLimiterConfig RateLimiterConfig `yaml:"rate_limit,omitempty"`
Users map[string]config_util.Secret `yaml:"basic_auth_users,omitempty"`
}

type TLSConfig struct {
TLSCert string `yaml:"cert"`
TLSKey config_util.Secret `yaml:"key"`
ClientCAsText string `yaml:"client_ca"`
TLSCertPath string `yaml:"cert_file"`
TLSKeyPath string `yaml:"key_file"`
ClientAuth string `yaml:"client_auth_type"`
ClientCAs string `yaml:"client_ca_file"`
CipherSuites []Cipher `yaml:"cipher_suites"`
CurvePreferences []Curve `yaml:"curve_preferences"`
MinVersion TLSVersion `yaml:"min_version"`
MaxVersion TLSVersion `yaml:"max_version"`
PreferServerCipherSuites bool `yaml:"prefer_server_cipher_suites"`
ClientAllowedSans []string `yaml:"client_allowed_sans"`
TLSCert string `yaml:"cert,omitempty"`
TLSKey config_util.Secret `yaml:"key,omitempty"`
ClientCAsText string `yaml:"client_ca,omitempty"`
TLSCertPath string `yaml:"cert_file,omitempty"`
TLSKeyPath string `yaml:"key_file,omitempty"`
ClientAuth string `yaml:"client_auth_type,omitempty"`
ClientCAs string `yaml:"client_ca_file,omitempty"`
CipherSuites []Cipher `yaml:"cipher_suites,omitempty"`
CurvePreferences []Curve `yaml:"curve_preferences,omitempty"`
MinVersion TLSVersion `yaml:"min_version,omitempty"`
MaxVersion TLSVersion `yaml:"max_version,omitempty"`
PreferServerCipherSuites bool `yaml:"prefer_server_cipher_suites,omitempty"`
ClientAllowedSans []string `yaml:"client_allowed_sans,omitempty"`
}

type FlagConfig struct {
Expand Down Expand Up @@ -481,9 +481,9 @@ func (c *Curve) UnmarshalYAML(unmarshal func(interface{}) error) error {
return errors.New("unknown curve: " + s)
}

func (c *Curve) MarshalYAML() (interface{}, error) {
func (c Curve) MarshalYAML() (interface{}, error) {
for s, curveid := range curves {
if *c == curveid {
if c == curveid {
return s, nil
}
}
Expand Down Expand Up @@ -512,9 +512,9 @@ func (tv *TLSVersion) UnmarshalYAML(unmarshal func(interface{}) error) error {
return errors.New("unknown TLS version: " + s)
}

func (tv *TLSVersion) MarshalYAML() (interface{}, error) {
func (tv TLSVersion) MarshalYAML() (interface{}, error) {
for s, v := range tlsVersions {
if *tv == v {
if tv == v {
return s, nil
}
}
Expand Down
127 changes: 127 additions & 0 deletions web/tls_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,13 @@ import (
"net/http"
"os"
"regexp"
"strings"
"sync"
"testing"
"time"

"github.com/prometheus/common/config"
"go.yaml.in/yaml/v2"
)

// Helpers for literal FlagConfig
Expand Down Expand Up @@ -715,3 +719,126 @@ func TestUsers(t *testing.T) {
t.Run(testInputs.Name, testInputs.Test)
}
}

func TestConfigGeneration(t *testing.T) {
Copy link
Copy Markdown
Member

@roidelapluie roidelapluie Apr 9, 2026

Choose a reason for hiding this comment

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

Maybe we can have narrower tests as wel.

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

Copy link
Copy Markdown
Author

@mahendrapaipuri mahendrapaipuri Apr 10, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@roidelapluie roidelapluie Apr 10, 2026

Choose a reason for hiding this comment

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

testing marshal/unmarshal on curve directly as well

// Secrets to be rendered without any masking
config.MarshalSecretValue = true

testTables := []struct {
Name string
Config Config
Expected string
}{
{
Name: "Only basic auth",
Config: Config{
Users: map[string]config.Secret{
"admin": config.Secret("$2y$10$X0h1gDsPszWURQaxFh.zoubFi6DXncSjhoQNJgRrnGs7EsimhC7zG"),
},
},
Expected: `
basic_auth_users:
admin: $2y$10$X0h1gDsPszWURQaxFh.zoubFi6DXncSjhoQNJgRrnGs7EsimhC7zG`,
},
{
Name: "Only TLS",
Config: Config{
TLSConfig: TLSConfig{
TLSCertPath: "cert.pem",
TLSKeyPath: "key.pem",
MinVersion: TLSVersion(tls.VersionTLS12),
CurvePreferences: []Curve{
Curve(tls.CurveP256),
Curve(tls.CurveP521),
},
CipherSuites: []Cipher{
Cipher(tls.TLS_AES_128_GCM_SHA256),
},
ClientAllowedSans: []string{
"example.com",
"example.org",
},
},
},
Expected: `
tls_server_config:
cert_file: cert.pem
key_file: key.pem
cipher_suites:
- TLS_AES_128_GCM_SHA256
curve_preferences:
- CurveP256
- CurveP521
min_version: TLS12
client_allowed_sans:
- example.com
- example.org`,
},
{
Name: "Only HTTP config",
Config: Config{
HTTPConfig: HTTPConfig{
HTTP2: true,
Header: map[string]string{
"X-Custom-Header": "value",
},
},
},
Expected: `
http_server_config:
http2: true
headers:
X-Custom-Header: value`,
},
{
Name: "Basic auth and TLS",
Config: Config{
Users: map[string]config.Secret{
"admin": config.Secret("$2y$10$X0h1gDsPszWURQaxFh.zoubFi6DXncSjhoQNJgRrnGs7EsimhC7zG"),
},
TLSConfig: TLSConfig{
TLSCertPath: "cert.pem",
TLSKeyPath: "key.pem",
MinVersion: TLSVersion(tls.VersionTLS12),
CurvePreferences: []Curve{
Curve(tls.CurveP256),
Curve(tls.CurveP521),
},
CipherSuites: []Cipher{
Cipher(tls.TLS_AES_128_GCM_SHA256),
},
ClientAllowedSans: []string{
"example.com",
"example.org",
},
},
},
Expected: `
tls_server_config:
cert_file: cert.pem
key_file: key.pem
cipher_suites:
- TLS_AES_128_GCM_SHA256
curve_preferences:
- CurveP256
- CurveP521
min_version: TLS12
client_allowed_sans:
- example.com
- example.org
basic_auth_users:
admin: $2y$10$X0h1gDsPszWURQaxFh.zoubFi6DXncSjhoQNJgRrnGs7EsimhC7zG`,
},
}

for _, test := range testTables {
Copy link
Copy Markdown
Member

@roidelapluie roidelapluie Apr 9, 2026

Choose a reason for hiding this comment

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

Please use t.Fatalf and subtests

Copy link
Copy Markdown
Author

@mahendrapaipuri mahendrapaipuri Apr 10, 2026

Choose a reason for hiding this comment

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

Ok, will do that! Cheers!

yamlConfig, err := yaml.Marshal(&test.Config)
if err != nil {
t.Error(err)
}

if strings.TrimSpace(test.Expected) != strings.TrimSpace(string(yamlConfig)) {
t.Fatalf("Expected config: %s, got config: %s", test.Expected, string(yamlConfig))
}
}
}
Loading