-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[confighttp] Use configoptional.Optional
for optional fields
#13109
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
Changes from all commits
8f6eb27
1ba683d
6ea68c5
7092009
8793522
b8a4d7a
40b96b6
901270a
0430f78
ffd62e7
17976dd
a33082f
7a14748
26f8bfa
f13be0b
1051beb
8bf3df7
b7c1e4d
77c514d
d2678d8
84c962f
b242c43
5489fb4
483cb80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: breaking | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) | ||
component: confighttp | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: "Use configooptional.Optional in confighttp" | ||
|
||
# One or more tracking issues or pull requests related to the change | ||
issues: [9478] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | ||
|
||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [api] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ import ( | |
"go.opentelemetry.io/collector/config/confighttp/internal" | ||
"go.opentelemetry.io/collector/config/configmiddleware" | ||
"go.opentelemetry.io/collector/config/configopaque" | ||
"go.opentelemetry.io/collector/config/configoptional" | ||
"go.opentelemetry.io/collector/config/configtls" | ||
"go.opentelemetry.io/collector/extension/extensionauth" | ||
) | ||
|
@@ -74,7 +75,7 @@ type ClientConfig struct { | |
Headers map[string]configopaque.String `mapstructure:"headers,omitempty"` | ||
|
||
// Auth configuration for outgoing HTTP calls. | ||
Auth *configauth.Config `mapstructure:"auth,omitempty"` | ||
Auth configoptional.Optional[configauth.Config] `mapstructure:"auth,omitempty"` | ||
|
||
// The compression key for supported compression types within collector. | ||
Compression configcompression.Type `mapstructure:"compression,omitempty"` | ||
|
@@ -225,13 +226,14 @@ func (hcs *ClientConfig) ToClient(ctx context.Context, host component.Host, sett | |
// The Auth RoundTripper should always be the innermost to ensure that | ||
// request signing-based auth mechanisms operate after compression | ||
// and header middleware modifies the request | ||
if hcs.Auth != nil { | ||
if hcs.Auth.HasValue() { | ||
ext := host.GetExtensions() | ||
if ext == nil { | ||
return nil, errors.New("extensions configuration not found") | ||
} | ||
|
||
httpCustomAuthRoundTripper, aerr := hcs.Auth.GetHTTPClientAuthenticator(ctx, ext) | ||
auth := hcs.Auth.Get() | ||
httpCustomAuthRoundTripper, aerr := auth.GetHTTPClientAuthenticator(ctx, ext) | ||
if aerr != nil { | ||
return nil, aerr | ||
} | ||
|
@@ -315,13 +317,13 @@ type ServerConfig struct { | |
Endpoint string `mapstructure:"endpoint,omitempty"` | ||
|
||
// TLS struct exposes TLS client configuration. | ||
TLS *configtls.ServerConfig `mapstructure:"tls"` | ||
TLS configoptional.Optional[configtls.ServerConfig] `mapstructure:"tls"` | ||
|
||
// CORS configures the server for HTTP cross-origin resource sharing (CORS). | ||
CORS *CORSConfig `mapstructure:"cors"` | ||
CORS configoptional.Optional[CORSConfig] `mapstructure:"cors"` | ||
|
||
// Auth for this receiver | ||
Auth *AuthConfig `mapstructure:"auth,omitempty"` | ||
Auth configoptional.Optional[AuthConfig] `mapstructure:"auth,omitempty"` | ||
|
||
// MaxRequestBodySize sets the maximum request body size in bytes. Default: 20MiB. | ||
MaxRequestBodySize int64 `mapstructure:"max_request_body_size,omitempty"` | ||
|
@@ -373,16 +375,11 @@ type ServerConfig struct { | |
Middlewares []configmiddleware.Config `mapstructure:"middleware,omitempty"` | ||
} | ||
|
||
func ptr[T any](v T) *T { | ||
return &v | ||
} | ||
|
||
// NewDefaultServerConfig returns ServerConfig type object with default values. | ||
// We encourage to use this function to create an object of ServerConfig. | ||
func NewDefaultServerConfig() ServerConfig { | ||
return ServerConfig{ | ||
ResponseHeaders: map[string]configopaque.String{}, | ||
CORS: ptr(NewDefaultCORSConfig()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the replacement now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The zero value, that is, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a breaking change because NewDefaultCORSConfig() returns zero value of CORS struct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and because the zero value was treated the same as |
||
WriteTimeout: 30 * time.Second, | ||
ReadHeaderTimeout: 1 * time.Minute, | ||
IdleTimeout: 1 * time.Minute, | ||
|
@@ -407,9 +404,9 @@ func (hss *ServerConfig) ToListener(ctx context.Context) (net.Listener, error) { | |
return nil, err | ||
} | ||
|
||
if hss.TLS != nil { | ||
if hss.TLS.HasValue() { | ||
var tlsCfg *tls.Config | ||
tlsCfg, err = hss.TLS.LoadTLSConfig(ctx) | ||
tlsCfg, err = hss.TLS.Get().LoadTLSConfig(ctx) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -488,25 +485,27 @@ func (hss *ServerConfig) ToServer(ctx context.Context, host component.Host, sett | |
handler = maxRequestBodySizeInterceptor(handler, hss.MaxRequestBodySize) | ||
} | ||
|
||
if hss.Auth != nil { | ||
server, err := hss.Auth.GetServerAuthenticator(context.Background(), host.GetExtensions()) | ||
if hss.Auth.HasValue() { | ||
auth := hss.Auth.Get() | ||
server, err := auth.GetServerAuthenticator(context.Background(), host.GetExtensions()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
handler = authInterceptor(handler, server, hss.Auth.RequestParameters, serverOpts) | ||
handler = authInterceptor(handler, server, auth.RequestParameters, serverOpts) | ||
} | ||
|
||
if hss.CORS != nil && len(hss.CORS.AllowedOrigins) > 0 { | ||
if hss.CORS.HasValue() && len(hss.CORS.Get().AllowedOrigins) > 0 { | ||
corsConfig := hss.CORS.Get() | ||
co := cors.Options{ | ||
AllowedOrigins: hss.CORS.AllowedOrigins, | ||
AllowedOrigins: corsConfig.AllowedOrigins, | ||
AllowCredentials: true, | ||
AllowedHeaders: hss.CORS.AllowedHeaders, | ||
MaxAge: hss.CORS.MaxAge, | ||
AllowedHeaders: corsConfig.AllowedHeaders, | ||
MaxAge: corsConfig.MaxAge, | ||
} | ||
handler = cors.New(co).Handler(handler) | ||
} | ||
if hss.CORS != nil && len(hss.CORS.AllowedOrigins) == 0 && len(hss.CORS.AllowedHeaders) > 0 { | ||
if hss.CORS.HasValue() && len(hss.CORS.Get().AllowedOrigins) == 0 && len(hss.CORS.Get().AllowedHeaders) > 0 { | ||
settings.Logger.Warn("The CORS configuration specifies allowed headers but no allowed origins, and is therefore ignored.") | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is zero-value of Optional a None value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, I am curious how
omitempty
works for Optional. It seems like the output formatter, when do we use it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
omitempty
will omitNone
values (since it omits any zero values). It's used byconfmap.Marshal
which is used for things like OpAMP (to report the effective configuration to a backend)