Generic xds client resource watchine e2e #8183
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8183 +/- ##
==========================================
- Coverage 82.04% 81.75% -0.29%
==========================================
Files 410 416 +6
Lines 40251 41054 +803
==========================================
+ Hits 33023 33563 +540
- Misses 5867 6091 +224
- Partials 1361 1400 +39
🚀 New features to boost your workflow:
|
purnesh42H
force-pushed
the
generic-xds-client-resource-watchine-e2e
branch
from
c99ca98 to
02db583
Compare
purnesh42H
added
Type: Feature
|
@dfawley could you do a pass on the implementation? For now, I have removed the metrics from client as it requires some more thought. It can be added in separate commit or PR. Otherwise the main implementation can be reviewed to make sure we are following the right approach and do the changes (if needed) before we go further. Also, I will be adding more e2e tests in subsequent commits |
| return "" | ||
| } | ||
| var tcParts []string | ||
| if sie.Credentials.TransportCredentials() != nil { |
There was a problem hiding this comment.
Ideally we shouldn't need to do this. And if we do, then we should probably be doing it in the TransportCredentials type, not here.
Note that String()s are intended for human-readable formatting or for debugging, and not for exhaustive uniqueness purposes. So using it as a map key is probably a bad idea.
There was a problem hiding this comment.
So, user will have to provide String() implementation for both TransportCredentials and PerRPCCredentials?
There was a problem hiding this comment.
No. Only if they want the debugging messages we would log including them to look pretty.
There was a problem hiding this comment.
Modifed String() for both helper and grpctransport. PTAL
| return false | ||
| } | ||
|
|
||
| if sie.Credentials.TransportCredentials() != nil { |
There was a problem hiding this comment.
This is not a guaranteed correct equality check.
We should see if the credentials implements Equal and call it. If not, if they are pointers we could compare those. But if they are not, we would just have to consider them not equal and maybe log a warning or something.
There was a problem hiding this comment.
Same, so user is expected provide Equal() implementation for Credentials?
There was a problem hiding this comment.
At the xdsclient level, we might want to make the extensions required to implement Equal.
At the grpctransport level, I am thinking the best we can do is be noisy if the credentials don't implement it.
There was a problem hiding this comment.
Added comment on docstring for requirement of Equal in ServerIdentifierConfig.Extensions. Modified Equal() for both helper and grpctransport. PTAL.
| if sie.Credentials.PerRPCCredentials() != nil { | ||
| if sie.Credentials.PerRPCCredentials().RequireTransportSecurity() != sie2.Credentials.PerRPCCredentials().RequireTransportSecurity() { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Same comment as above, but even more strongly. RequireTransportSecurity is a boolean. If two credentials both require transport security, there is no reason to assume they are the same credential.
There was a problem hiding this comment.
Done as suggested above
purnesh42H
force-pushed
the
generic-xds-client-resource-watchine-e2e
branch
from
042b900 to
124b2f3
Compare
This is the change to make generic xDS client do resource watching from xDS management server. The xDS client uses the ADS (Aggregated Data Service) stream transport channel that was created in #8144. The e2e tests are written using the listener resource type from #8144 and grpctransport from #8066.
The PR copies the existing
clientimpl,authority,clientimpl_watcherfrom internal xdsclient code along with the helpers that are part of gRPC and then modify them to use the generic client types and interfaces.The PR also adds
String()andEqual()methods forServerIdentifierExtensionand implements them forgrpctransport.ServerIdentifierExtensionThe PR can be reviewed commit by commit. The first 2 commits add the String() and Equal() helpers and implementation for grpctransport respectively. The next 4 commits copy the xDS client and e2e tests files, each followed by their modification commit to use generic interfaces.
RELEASE NOTES: None