-
Notifications
You must be signed in to change notification settings - Fork 23
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
improvement: Make TLSConfig refreshable #545
base: develop
Are you sure you want to change the base?
Changes from all commits
6e05ba7
28c262c
fc30fa2
9dbdbc8
7a71c70
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,5 @@ | ||
type: improvement | ||
improvement: | ||
description: Make TLSConfig refreshable | ||
links: | ||
- https://github.com/palantir/conjure-go-runtime/pull/545 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ import ( | |
|
||
"github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal/refreshingclient" | ||
"github.com/palantir/pkg/metrics" | ||
"github.com/palantir/pkg/refreshable" | ||
"github.com/palantir/pkg/tlsconfig" | ||
werror "github.com/palantir/witchcraft-go-error" | ||
"github.com/palantir/witchcraft-go-logging/wlog/svclog/svc1log" | ||
|
@@ -475,28 +476,32 @@ func newValidatedClientParamsFromConfig(ctx context.Context, config ClientConfig | |
}, nil | ||
} | ||
|
||
func subscribeTLSConfigUpdateWarning(ctx context.Context, security RefreshableSecurityConfig) (*tls.Config, error) { | ||
//TODO: Implement refreshable TLS configuration. | ||
// It is hard to represent all of the configuration (e.g. a dynamic function for GetCertificate) in primitive values friendly to reflect.DeepEqual. | ||
func newRefreshableTLSConfig(ctx context.Context, security RefreshableSecurityConfig) (refreshable.Refreshable, error) { | ||
currentSecurity := security.CurrentSecurityConfig() | ||
|
||
security.CAFiles().SubscribeToStringSlice(func(caFiles []string) { | ||
svc1log.FromContext(ctx).Warn("conjure-go-runtime: CAFiles configuration changed but can not be live-reloaded.", | ||
svc1log.SafeParam("existingCAFiles", currentSecurity.CAFiles), | ||
svc1log.SafeParam("ignoredCAFiles", caFiles)) | ||
}) | ||
security.CertFile().SubscribeToString(func(certFile string) { | ||
svc1log.FromContext(ctx).Warn("conjure-go-runtime: CertFile configuration changed but can not be live-reloaded.", | ||
svc1log.SafeParam("existingCertFile", currentSecurity.CertFile), | ||
svc1log.SafeParam("ignoredCertFile", certFile)) | ||
}) | ||
security.KeyFile().SubscribeToString(func(keyFile string) { | ||
svc1log.FromContext(ctx).Warn("conjure-go-runtime: KeyFile configuration changed but can not be live-reloaded.", | ||
svc1log.SafeParam("existingKeyFile", currentSecurity.KeyFile), | ||
svc1log.SafeParam("ignoredKeyFile", keyFile)) | ||
}) | ||
currentTLSConfig, err := newTLSConfig(currentSecurity) | ||
if err != nil { | ||
return nil, err | ||
} | ||
tlsConfig := refreshable.NewDefaultRefreshable(currentTLSConfig) | ||
|
||
return newTLSConfig(currentSecurity) | ||
security.SubscribeToSecurityConfig(func(currentSecurity SecurityConfig) { | ||
currentTLSConfig, err := newTLSConfig(currentSecurity) | ||
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. Do we care that this does not handle changing file contents, only file paths? |
||
if err != nil { | ||
svc1log.FromContext(ctx).Warn("conjure-go-runtime: Current security config is not valid; tls config not reloaded.", | ||
svc1log.SafeParam("existingCertFile", currentSecurity.CertFile), | ||
svc1log.SafeParam("existingKeyFile", currentSecurity.KeyFile), | ||
svc1log.SafeParam("existingCAFiles", currentSecurity.CAFiles), | ||
svc1log.Stacktrace(err)) | ||
return | ||
} | ||
if err := tlsConfig.Update(currentTLSConfig); err != nil { | ||
svc1log.FromContext(ctx).Warn("conjure-go-runtime: Internal error updating tls config.", | ||
svc1log.Stacktrace(err)) | ||
return | ||
} | ||
}) | ||
return tlsConfig, nil | ||
} | ||
|
||
func newTLSConfig(security SecurityConfig) (*tls.Config, error) { | ||
|
@@ -507,14 +512,11 @@ func newTLSConfig(security SecurityConfig) (*tls.Config, error) { | |
if security.CertFile != "" && security.KeyFile != "" { | ||
tlsParams = append(tlsParams, tlsconfig.ClientKeyPairFiles(security.CertFile, security.KeyFile)) | ||
} | ||
if len(tlsParams) != 0 { | ||
tlsConfig, err := tlsconfig.NewClientConfig(tlsParams...) | ||
if err != nil { | ||
return nil, werror.Wrap(err, "failed to build tlsConfig") | ||
} | ||
return tlsConfig, nil | ||
tlsConfig, err := tlsconfig.NewClientConfig(tlsParams...) | ||
if err != nil { | ||
return nil, werror.Wrap(err, "failed to build tlsConfig") | ||
} | ||
return nil, nil | ||
return tlsConfig, nil | ||
} | ||
|
||
func derefDurationPtr(durPtr *time.Duration, defaultVal time.Duration) time.Duration { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,11 +40,33 @@ type TransportParams struct { | |
HTTP2PingTimeout time.Duration | ||
} | ||
|
||
func NewRefreshableTransport(ctx context.Context, p RefreshableTransportParams, tlsConfig *tls.Config, dialer ContextDialer) http.RoundTripper { | ||
func NewRefreshableTransport(ctx context.Context, p RefreshableTransportParams, tlsConfig refreshable.Refreshable, dialer ContextDialer) http.RoundTripper { | ||
curTLSConfig := func() *tls.Config { | ||
if tlsConfig == nil { | ||
return nil | ||
} | ||
return tlsConfig.Current().(*tls.Config) | ||
} | ||
|
||
transport := refreshable.NewDefaultRefreshable(newTransport(ctx, p.CurrentTransportParams(), curTLSConfig(), dialer)) | ||
|
||
p.SubscribeToTransportParams(func(params TransportParams) { | ||
if err := transport.Update(newTransport(ctx, params, curTLSConfig(), dialer)); err != nil { | ||
svc1log.FromContext(ctx).Error("Failed to update HTTP transport with transport params", svc1log.Stacktrace(err)) | ||
} | ||
}) | ||
|
||
if tlsConfig != nil { | ||
tlsConfig.Subscribe(func(tlsConfI interface{}) { | ||
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 general concern I had with the original implementation was this subscription because |
||
tlsConf := tlsConfI.(*tls.Config) | ||
if err := transport.Update(newTransport(ctx, p.CurrentTransportParams(), tlsConf, dialer)); err != nil { | ||
svc1log.FromContext(ctx).Error("Failed to update HTTP transport with tls config", svc1log.Stacktrace(err)) | ||
} | ||
}) | ||
} | ||
|
||
return &RefreshableTransport{ | ||
Refreshable: p.MapTransportParams(func(p TransportParams) interface{} { | ||
return newTransport(ctx, p, tlsConfig, dialer) | ||
}), | ||
Refreshable: transport, | ||
} | ||
} | ||
|
||
|
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.
Because of the subscription to the security config, this should always return a non-nil refreshable.