xds: add internal ResourceNameFunc server option - #9288
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9288 +/- ##
==========================================
+ Coverage 83.00% 83.08% +0.08%
==========================================
Files 422 424 +2
Lines 35085 35252 +167
==========================================
+ Hits 29121 29289 +168
- Misses 4441 4448 +7
+ Partials 1523 1515 -8
🚀 New features to boost your workflow:
|
Introduces `ResourceNameFunc` callback option to `NewGRPCServer` for overriding server listener resource name generation. Exposes `UnderlyingServer()` method to retrieve the inner target server for chaining.
726a5b4 to
5c0a214
Compare
| // | ||
| // Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
| // later release. | ||
| func ResourceNameFunc(f func(net.Addr) string) grpc.ServerOption { |
There was a problem hiding this comment.
Instead of providing an option for anyone to change the listener resource name for server, we only want it to be internal, so we have a few options here :
- Move the
ResourceNameFuncoption to an internal package. - Pass a listener to the
Servemethod which has a function for the override and we can define an interface for it (or do it inline) and type check if the listener implements that interface , if it does, we use the provided function for populating the listener name.
- Have a variable function , something like
var CustomResourceTemplatePopulator func(template, target string) stringinside a internal package , likeinternal/xds/bootstrap. - Then
PopulateResourceTemplatecan check if the variable is not nil, return theCustomResourceTemplatePopulator - For the
cfg.ServerListenerResourceNameTemplate() == "", we can do the check ifCustomResourceTemplatePopulatoris nil.
WDYT ?
There was a problem hiding this comment.
Thanks, I think option 1 works better for us. I moved ResourceNameFunc to internal/xds/server as an internal grpc.ServerOption. We do want to keep the resource name override per-server and avoid changing the xds bootstrap as bootstrap could be shared. I also updated the test.
| // | ||
| // Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
| // later release. | ||
| func (s *GRPCServer) UnderlyingServer() *grpc.Server { |
There was a problem hiding this comment.
We should hold of on exposing the underlying server until we have a discussion and reach a conclusion on the best way to achieve this.
There was a problem hiding this comment.
I moved underlying server part out of this PR. We can continue discuss and explore other options without blocking review/submission the resource name part.
eshitachandwani
left a comment
There was a problem hiding this comment.
LGTM modulo 2 comments. Adding @easwars for a second review.
| type serverOptions struct { | ||
| modeCallback ServingModeCallbackFunc | ||
| clientPoolForTesting *xdsclient.Pool | ||
| resourceNameFunc func(net.Addr) string |
There was a problem hiding this comment.
Can we change this resourceNameFunc variable here too...and everywhere else?
| // OverrideListenerResourceNameFromServerOption returns the resource name | ||
| // function carried by opt, if opt was created by OverrideListenerResourceName. | ||
| func OverrideListenerResourceNameFromServerOption(opt grpc.ServerOption) (func(net.Addr) string, bool) { | ||
| o, ok := opt.(*overrideListenerResourceNameOption) |
There was a problem hiding this comment.
Can we change this to just return func(net.Addr) string and the user can check for nil value instead of returning an extra bool. WDYT ?
| type serverOptions struct { | ||
| modeCallback ServingModeCallbackFunc | ||
| clientPoolForTesting *xdsclient.Pool | ||
| modeCallback ServingModeCallbackFunc | ||
| clientPoolForTesting *xdsclient.Pool | ||
| overrideListenerResourceName func(net.Addr) string | ||
| } |
There was a problem hiding this comment.
How about we move this whole struct to internal/xds/server? What this would mean is that the existing public server options' implementation would modify this ServerOptions struct defined in the internal package.
This would also unify the handling in xds/server.go where you currently have separate handling for the public server options and the newly added internal server option.
There was a problem hiding this comment.
I made some changes. Could you please take a look and see if I understand it correctly? Thanks
Adds an internal
ResourceNameFuncserver option that allows override the LDS listener resource name.RELEASE NOTES: none