xds/resolver: Optimize Interceptor Chain Construction #8641
Conversation
…tor, not for every RPC
easwars
added
Area: xDS
|
@eshitachandwani : FYI this might conflict with your resolver changes for A74. And since you have been looking at the resolver code for sometime now, it would be a good PR to review. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8641 +/- ##
==========================================
+ Coverage 79.45% 83.09% +3.64%
==========================================
Files 415 415
Lines 41339 32144 -9195
==========================================
- Hits 32844 26710 -6134
+ Misses 6621 4021 -2600
+ Partials 1874 1413 -461
🚀 New features to boost your workflow:
|
|
@arjan-bal could you review this change, please? |
| // The filter config override maps contain overrides from the route, cluster, | ||
| // and virtual host respectively. The cluster override has the highest priority, | ||
| // followed by the route override, and finally the virtual host override. | ||
| func newInterceptor(filters []xdsresource.HTTPFilter, cluster, route, virtualHost map[string]httpfilter.FilterConfig) (iresolver.ClientInterceptor, error) { |
There was a problem hiding this comment.
nit: What do you think about adding an override suffix to cluster, route, and virtualHost? I find it can help with readability, but no worries if you prefer the current naming.
| cs := r.newConfigSelector() | ||
| cs, err := r.newConfigSelector() | ||
| if err != nil { | ||
| r.onResourceError(fmt.Errorf("xds: failed to create config selector: %v", err)) |
There was a problem hiding this comment.
Should the function return early after calling onResourceError? I'm not sure what happens when a nil config selector is passed to sendNewServiceConfig.
There was a problem hiding this comment.
I think we should return early since onResourceError will call sendNewServiceConfig with an erroring config selector, so we should not call sendNewServiceConfig again with a nil config selector (which will set a default config selector in client conn)
There was a problem hiding this comment.
Thanks for spotting this. Fixed it.
Looks like we don't have any tests for this case, but this is also a very very unlikey case. There are two cases when building a config selector can fail:
- A filter is part of the configuration, but is not supported on the client side. This case would not pass xDS client validation. (I had a panic for this case initially, but changed it to return an error as part of this reivew).
- The call to build the interceptor for the filter fails. This is also very unlikey because if the filter successfully parsed the configuration (during resource parsing in the xDS client), there is very little reason for it to fail when trying to build the interceptor.
I'll anyway file an issue to add a test for this edge case.
| // Should not happen if it passed xdsClient validation. | ||
| panic(fmt.Sprintf("filter %q does not support use in client", filter.Name)) |
There was a problem hiding this comment.
What do you think about continuing to return an error here? The method signature already allows for it, so callers presumably have error handling in place. Panicking to crash the process feels like a worse alternative, especially if this is a recoverable error.
There was a problem hiding this comment.
At some point in the recent past, I got comfortable using panics for cases where we definitely don't expect something to happen, without some really bad programming error. This I believe is one such. But since an error is already being handled by the caller of this function, I'm ok to return an error here instead of panicking. Thanks.
| fb.logger.Logf("BuildClientInterceptor called with config: %+v, override: %+v", config, override) | ||
|
|
||
| if config == nil { | ||
| panic("unexpected missing config") |
There was a problem hiding this comment.
In my opinion printing an error to fail the test may be preferred to panicking from a spawned goroutine. The situation seems similar to the style guide recommendation about only calling t.Fatalf from the main test function.
| // by the backend, allowing tests to assert that the correct filter | ||
| // configuration was applied for each RPC. | ||
| type testHTTPFilterWithRPCMetadata struct { | ||
| logger logger |
There was a problem hiding this comment.
nit: Is there a benefit of using a logger interface here? If not, I think it's simpler to directory store a testing.T here .
There was a problem hiding this comment.
It's probably an overkill, but I feel that it's better to do this instead of accepting a testing.T, since we can very easily start calling other methods on it like Error or Fatal if we have access to it.
|
|
||
| func (*testHTTPFilterWithRPCMetadata) IsTerminal() bool { return false } | ||
|
|
||
| var _ httpfilter.ClientInterceptorBuilder = &testHTTPFilterWithRPCMetadata{} |
There was a problem hiding this comment.
nit: I think we should add a comment here stating that ClientInterceptorBuilder is an optional interface for Filters to implement so this compile time check ensures the test filter implements it. In my opinion this would help readers.
| } | ||
| var errStr string | ||
| if newStreamErr != nil { | ||
| errStr = newStreamErr.Error() |
There was a problem hiding this comment.
I see that we're converting a string to an error, assigning it to testFilterCfg.newStreamErr and converting it back to a string here. Can the error string be stored in testFilterCfg.newStreamErr to avoid these conversions?
| } | ||
|
|
||
| func (cs *clientStream) Context() context.Context { | ||
| return cs.ctx |
There was a problem hiding this comment.
Do we need to store a context here? Can we return cs.ClientStream.Context() instead?
There was a problem hiding this comment.
Looks like I don't need this type at all. I was probably doing something else at some point in time that required this type. But with the current state of the test and the current implementation of testFilterInterceptor, we don't need this type at all. Thanks.
| if internal.NewXDSResolverWithConfigForTesting == nil { | ||
| t.Fatalf("internal.NewXDSResolverWithConfigForTesting is nil") | ||
| } |
There was a problem hiding this comment.
nit: We can avoid this check and let the code panic if we don't expect to fail in most cases.
There was a problem hiding this comment.
There is a bunch of commented code here that probably needs to be removed. Can you please take a look?
4 participants
Existing behavior:
New behavior:
Other changes:
RELEASE NOTES: NONE