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

wire up all prometheus proxy config settings for flow and the converters #6306

Merged
merged 14 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ Main (unreleased)

- Mutex and block pprofs are now available via the pprof endpoint. (@mattdurham)

- Added additional http client proxy configurations to components for
`no_proxy`, `proxy_from_environment`, and `proxy_connect_header`. (@erikbaranowski)

### Bugfixes

- Fix an issue in `remote.s3` where the exported content of an object would be an empty string if `remote.s3` failed to fully retrieve
Expand Down
220 changes: 160 additions & 60 deletions component/common/config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type HTTPClientConfig struct {
OAuth2 *OAuth2Config `river:"oauth2,block,optional"`
BearerToken rivertypes.Secret `river:"bearer_token,attr,optional"`
BearerTokenFile string `river:"bearer_token_file,attr,optional"`
ProxyURL URL `river:"proxy_url,attr,optional"`
ProxyConfig *ProxyConfig `river:",squash"`
TLSConfig TLSConfig `river:"tls_config,block,optional"`
FollowRedirects bool `river:"follow_redirects,attr,optional"`
EnableHTTP2 bool `river:"enable_http2,attr,optional"`
Expand All @@ -33,63 +33,46 @@ func (h *HTTPClientConfig) SetToDefault() {

// Validate returns an error if h is invalid.
func (h *HTTPClientConfig) Validate() error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This refactor pushes the responsibility for validating other types to their own Validate funcs except for ProxyConfig (squashed). It also tries to more sanely handle the scenarios where only 1 auth method can be set. I think this code got more and more convoluted over time.

// Backwards compatibility with the bearer_token field.
if len(h.BearerToken) > 0 && len(h.BearerTokenFile) > 0 {
return fmt.Errorf("at most one of bearer_token & bearer_token_file must be configured")
}
if (h.BasicAuth != nil || h.OAuth2 != nil) && (len(h.BearerToken) > 0 || len(h.BearerTokenFile) > 0) {
return fmt.Errorf("at most one of basic_auth, oauth2, bearer_token & bearer_token_file must be configured")
if h == nil {
return nil
}
if h.BasicAuth != nil && (string(h.BasicAuth.Password) != "" && h.BasicAuth.PasswordFile != "") {
return fmt.Errorf("at most one of basic_auth password & password_file must be configured")

authCount := 0
if h.BasicAuth != nil {
authCount++
}
if h.Authorization != nil {
if len(h.BearerToken) > 0 || len(h.BearerTokenFile) > 0 {
return fmt.Errorf("authorization is not compatible with bearer_token & bearer_token_file")
}
if string(h.Authorization.Credentials) != "" && h.Authorization.CredentialsFile != "" {
return fmt.Errorf("at most one of authorization credentials & credentials_file must be configured")
}
h.Authorization.Type = strings.TrimSpace(h.Authorization.Type)
if len(h.Authorization.Type) == 0 {
h.Authorization.Type = bearerAuth
}
if strings.ToLower(h.Authorization.Type) == "basic" {
return fmt.Errorf(`authorization type cannot be set to "basic", use "basic_auth" instead`)
}
if h.BasicAuth != nil || h.OAuth2 != nil {
return fmt.Errorf("at most one of basic_auth, oauth2 & authorization must be configured")
}
} else {
if len(h.BearerToken) > 0 {
h.Authorization = &Authorization{Credentials: h.BearerToken}
h.Authorization.Type = bearerAuth
h.BearerToken = ""
}
if len(h.BearerTokenFile) > 0 {
h.Authorization = &Authorization{CredentialsFile: h.BearerTokenFile}
h.Authorization.Type = bearerAuth
h.BearerTokenFile = ""
}
authCount++
}
if h.OAuth2 != nil {
if h.BasicAuth != nil {
return fmt.Errorf("at most one of basic_auth, oauth2 & authorization must be configured")
}
if len(h.OAuth2.ClientID) == 0 {
return fmt.Errorf("oauth2 client_id must be configured")
}
if len(h.OAuth2.ClientSecret) == 0 && len(h.OAuth2.ClientSecretFile) == 0 {
return fmt.Errorf("either oauth2 client_secret or client_secret_file must be configured")
}
if len(h.OAuth2.TokenURL) == 0 {
return fmt.Errorf("oauth2 token_url must be configured")
}
if len(h.OAuth2.ClientSecret) > 0 && len(h.OAuth2.ClientSecretFile) > 0 {
return fmt.Errorf("at most one of oauth2 client_secret & client_secret_file must be configured")
}
authCount++
}
return nil
if len(h.BearerToken) > 0 {
authCount++
}
if len(h.BearerTokenFile) > 0 {
authCount++
}

if authCount > 1 {
return fmt.Errorf("at most one of basic_auth, authorization, oauth2, bearer_token & bearer_token_file must be configured")
}

// TODO: Validate should not be modifying the object
if len(h.BearerToken) > 0 {
erikbaranowski marked this conversation as resolved.
Show resolved Hide resolved
h.Authorization = &Authorization{Credentials: h.BearerToken}
h.Authorization.Type = bearerAuth
h.BearerToken = ""
}

// TODO: Validate should not be modifying the object
if len(h.BearerTokenFile) > 0 {
h.Authorization = &Authorization{CredentialsFile: h.BearerTokenFile}
h.Authorization.Type = bearerAuth
h.BearerTokenFile = ""
}

return h.ProxyConfig.Validate()
}

// Convert converts HTTPClientConfig to the native Prometheus type. If h is
Expand All @@ -108,9 +91,7 @@ func (h *HTTPClientConfig) Convert() *config.HTTPClientConfig {
TLSConfig: *h.TLSConfig.Convert(),
FollowRedirects: h.FollowRedirects,
EnableHTTP2: h.EnableHTTP2,
ProxyConfig: config.ProxyConfig{
ProxyURL: h.ProxyURL.Convert(),
},
ProxyConfig: h.ProxyConfig.Convert(),
}
}

Expand Down Expand Up @@ -145,6 +126,59 @@ func (b *BasicAuth) Convert() *config.BasicAuth {
}
}

func (b *BasicAuth) Validate() error {
if b == nil {
return nil
}

if string(b.Password) != "" && b.PasswordFile != "" {
return fmt.Errorf("at most one of basic_auth password & password_file must be configured")
}

return nil
}

type ProxyConfig struct {
ProxyURL URL `river:"proxy_url,attr,optional"`
NoProxy string `river:"no_proxy,attr,optional"`
ProxyFromEnvironment bool `river:"proxy_from_environment,attr,optional"`
ProxyConnectHeader Header `river:",squash"`
}

func (p *ProxyConfig) Convert() config.ProxyConfig {
if p == nil {
return config.ProxyConfig{}
}

return config.ProxyConfig{
ProxyURL: p.ProxyURL.Convert(),
NoProxy: p.NoProxy,
ProxyFromEnvironment: p.ProxyFromEnvironment,
ProxyConnectHeader: p.ProxyConnectHeader.Convert(),
}
}

func (p *ProxyConfig) Validate() error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if p == nil {
return nil
}

if len(p.ProxyConnectHeader.Header) > 0 && (!p.ProxyFromEnvironment && (p.ProxyURL.URL == nil || p.ProxyURL.String() == "")) {
return fmt.Errorf("if proxy_connect_header is configured, proxy_url or proxy_from_environment must also be configured")
}
if p.ProxyFromEnvironment && p.ProxyURL.URL != nil && p.ProxyURL.String() != "" {
return fmt.Errorf("if proxy_from_environment is configured, proxy_url must not be configured")
}
if p.ProxyFromEnvironment && p.NoProxy != "" {
return fmt.Errorf("if proxy_from_environment is configured, no_proxy must not be configured")
}
if p.ProxyURL.URL == nil && p.NoProxy != "" {
return fmt.Errorf("if no_proxy is configured, proxy_url must also be configured")
}

return nil
}

// URL mirrors config.URL
type URL struct {
*url.URL
Expand Down Expand Up @@ -173,10 +207,35 @@ func (u *URL) UnmarshalText(text []byte) error {
}

// Convert converts our type to the native prometheus type
func (u URL) Convert() config.URL {
func (u *URL) Convert() config.URL {
if u == nil {
return config.URL{URL: nil}
}
return config.URL{URL: u.URL}
}

type Header struct {
Header map[string][]rivertypes.Secret `river:"proxy_connect_header,attr,optional"`
}

func (h *Header) Convert() config.Header {
if h == nil {
return nil
}
header := make(config.Header)
for name, values := range h.Header {
var s []config.Secret
if values != nil {
s = make([]config.Secret, 0, len(values))
for _, value := range values {
s = append(s, config.Secret(value))
}
}
header[name] = s
}
return header
}

// Authorization sets up HTTP authorization credentials.
type Authorization struct {
Type string `river:"type,attr,optional"`
Expand All @@ -196,6 +255,28 @@ func (a *Authorization) Convert() *config.Authorization {
}
}

func (a *Authorization) Validate() error {
if a == nil {
return nil
}

if string(a.Credentials) != "" && a.CredentialsFile != "" {
return fmt.Errorf("at most one of authorization credentials & credentials_file must be configured")
}

// TODO: Validate should not be modifying the object
a.Type = strings.TrimSpace(a.Type)
if len(a.Type) == 0 {
a.Type = bearerAuth
erikbaranowski marked this conversation as resolved.
Show resolved Hide resolved
}

if strings.ToLower(a.Type) == "basic" {
return fmt.Errorf(`authorization type cannot be set to "basic", use "basic_auth" block instead`)
}

return nil
}

// TLSVersion mirrors config.TLSVersion
type TLSVersion uint16

Expand Down Expand Up @@ -283,7 +364,7 @@ type OAuth2Config struct {
Scopes []string `river:"scopes,attr,optional"`
TokenURL string `river:"token_url,attr,optional"`
EndpointParams map[string]string `river:"endpoint_params,attr,optional"`
ProxyURL URL `river:"proxy_url,attr,optional"`
ProxyConfig *ProxyConfig `river:",squash"`
TLSConfig *TLSConfig `river:"tls_config,block,optional"`
}

Expand All @@ -299,12 +380,31 @@ func (o *OAuth2Config) Convert() *config.OAuth2 {
Scopes: o.Scopes,
TokenURL: o.TokenURL,
EndpointParams: o.EndpointParams,
ProxyConfig: config.ProxyConfig{
ProxyURL: o.ProxyURL.Convert(),
},
ProxyConfig: o.ProxyConfig.Convert(),
}
if o.TLSConfig != nil {
oa.TLSConfig = *o.TLSConfig.Convert()
}
return oa
}

func (o *OAuth2Config) Validate() error {
if o == nil {
return nil
}

if len(o.ClientID) == 0 {
return fmt.Errorf("oauth2 client_id must be configured")
}
if len(o.ClientSecret) == 0 && len(o.ClientSecretFile) == 0 {
return fmt.Errorf("either oauth2 client_secret or client_secret_file must be configured")
}
if len(o.TokenURL) == 0 {
return fmt.Errorf("oauth2 token_url must be configured")
}
if len(o.ClientSecret) > 0 && len(o.ClientSecretFile) > 0 {
return fmt.Errorf("at most one of oauth2 client_secret & client_secret_file must be configured")
}

return o.ProxyConfig.Validate()
}
2 changes: 1 addition & 1 deletion component/common/config/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,5 +228,5 @@ func TestHTTPClientBadConfig(t *testing.T) {

var httpClientConfig HTTPClientConfig
err := river.Unmarshal([]byte(exampleRiverConfig), &httpClientConfig)
require.ErrorContains(t, err, "at most one of bearer_token & bearer_token_file must be configured")
require.ErrorContains(t, err, "at most one of basic_auth password & password_file must be configured")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some cases, the error changed because the order of validation will be different. I don't think this is harmful.

}
6 changes: 5 additions & 1 deletion component/discovery/aws/ec2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ func TestConvert(t *testing.T) {
u, err := url.Parse("http://example:8080")
require.NoError(t, err)
httpClientConfig := config.DefaultHTTPClientConfig
httpClientConfig.ProxyURL = config.URL{URL: u}
httpClientConfig.ProxyConfig = &config.ProxyConfig{
ProxyURL: config.URL{
URL: u,
},
}

// example configuration
riverArgs := EC2Arguments{
Expand Down
17 changes: 11 additions & 6 deletions component/discovery/azure/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ type Arguments struct {
RefreshInterval time.Duration `river:"refresh_interval,attr,optional"`
ResourceGroup string `river:"resource_group,attr,optional"`

ProxyURL config.URL `river:"proxy_url,attr,optional"`
FollowRedirects bool `river:"follow_redirects,attr,optional"`
EnableHTTP2 bool `river:"enable_http2,attr,optional"`
TLSConfig config.TLSConfig `river:"tls_config,block,optional"`
ProxyConfig *config.ProxyConfig `river:",squash"`
FollowRedirects bool `river:"follow_redirects,attr,optional"`
EnableHTTP2 bool `river:"enable_http2,attr,optional"`
TLSConfig config.TLSConfig `river:"tls_config,block,optional"`
}

type OAuth struct {
Expand Down Expand Up @@ -69,7 +69,12 @@ func (a *Arguments) Validate() error {
if a.OAuth == nil && a.ManagedIdentity == nil || a.OAuth != nil && a.ManagedIdentity != nil {
return fmt.Errorf("exactly one of oauth or managed_identity must be specified")
}
return a.TLSConfig.Validate()

if err := a.TLSConfig.Validate(); err != nil {
return err
}

return a.ProxyConfig.Validate()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to explicitly validate squashed properties

}

func (a *Arguments) Convert() *prom_discovery.SDConfig {
Expand All @@ -90,10 +95,10 @@ func (a *Arguments) Convert() *prom_discovery.SDConfig {
}

httpClientConfig := config.DefaultHTTPClientConfig
httpClientConfig.ProxyURL = a.ProxyURL
httpClientConfig.FollowRedirects = a.FollowRedirects
httpClientConfig.EnableHTTP2 = a.EnableHTTP2
httpClientConfig.TLSConfig = a.TLSConfig
httpClientConfig.ProxyConfig = a.ProxyConfig

return &prom_discovery.SDConfig{
Environment: a.Environment,
Expand Down
14 changes: 9 additions & 5 deletions component/discovery/azure/azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestRiverUnmarshal(t *testing.T) {
assert.Equal(t, "clientsecret", string(args.OAuth.ClientSecret))
assert.Equal(t, true, args.EnableHTTP2)
assert.Equal(t, false, args.FollowRedirects)
assert.Equal(t, "http://example:8080", args.ProxyURL.String())
assert.Equal(t, "http://example:8080", args.ProxyConfig.ProxyURL.String())
}

func TestRiverUnmarshal_OAuthRequiredFields(t *testing.T) {
Expand Down Expand Up @@ -123,8 +123,10 @@ func TestConvert(t *testing.T) {
},
FollowRedirects: false,
EnableHTTP2: false,
ProxyURL: config.URL{
URL: proxyUrl,
ProxyConfig: &config.ProxyConfig{
ProxyURL: config.URL{
URL: proxyUrl,
},
},
}

Expand Down Expand Up @@ -152,8 +154,10 @@ func TestConvert(t *testing.T) {
},
FollowRedirects: true,
EnableHTTP2: true,
ProxyURL: config.URL{
URL: proxyUrl,
ProxyConfig: &config.ProxyConfig{
ProxyURL: config.URL{
URL: proxyUrl,
},
},
}

Expand Down
Loading
Loading