-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Support pluggable config providers #102
Conversation
} | ||
) | ||
|
||
// NewServer returns a new instance of server that serves one or many services. | ||
func NewServer(opts ...server_options.ServerOption) *Server { | ||
serverOpts := server_options.NewServerOptions(opts) | ||
cfgProvider, err := config.NewConfigProviderWithRefresh(*serverOpts.ConfigProvider) |
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.
This err
is left unchecked, which may lead to cfgProvider
being nil
.
return c.JSON(http.StatusInternalServerError, err) | ||
} | ||
|
||
tls, err := rpc.CreateTLSConfig(cfg.TemporalGRPCAddress, &cfg.TLS) |
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 this called on every request and never cached?
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.
it's called on every request though receives a cached config https://github.com/temporalio/ui-server/pull/102/files#diff-0e504256c68c0d5b7cd3c799aa6c942469504ebb007e337f33cef429241061a1R67
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.
Config passed into rpc.CreateTLSConfig()
is cashed, but the output of it is generated every time? And then rpc.CreateGRPCConnection()
is also called every time. Shouldn't they be cached?
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.
right, figured to cache the GRPC connections once we add multi-connection support and work with the pool of connections. Can add caching now
server/routes/auth.go
Outdated
ctx := context.Background() | ||
cfg, _ := cfgProvider.GetConfig() |
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.
Why ignore error?
3bc3222
to
19035dc
Compare
19035dc
to
3037dda
Compare
What was changed
Adds support for pluggable Config providers:
Why?
Flexibility in how the config is provided
TLS params periodic refresh
Checklist
Closes
How was this tested:
refreshInterval: 30s
, ex in development.yaml filecaFile
,certFile
,keyFile
,enableHostVerification
,serverName
2022[/02/16]() 11:38:06 loaded new UI server configuration
in the logs, should also print that it loaded new TLS CA, KEY etc