xds: generic xds client resource watching e2e

[purnesh42H]

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_watcher from 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() and Equal() methods for ServerIdentifierExtension and implements them for grpctransport.ServerIdentifierExtension. It also adds a custom map implementation similar to resolver.AddrMap to re-use existing channels for similar server configurations.

Recommend to review each commit of the PR separately in the order.

Commits with copy prefix are copy paste of internal xdsclient code and are followed by modifications. Review only the modification commit.

RELEASE NOTES: None

[codecov]

Codecov Report

Attention: Patch coverage is 75.55831% with 197 lines in your changes missing coverage. Please review.

Project coverage is 82.19%. Comparing base (732f3f3) to head (d22c5b7).
Report is 18 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/clients/xdsclient/authority.go	70.32%	112 Missing and 26 partials ⚠️
xds/internal/clients/xdsclient/xdsclient.go	79.52%	28 Missing and 15 partials ⚠️
...s/internal/clients/grpctransport/grpc_transport.go	86.53%	5 Missing and 2 partials ⚠️
.../internal/clients/xdsclient/clientimpl_watchers.go	83.78%	3 Missing and 3 partials ⚠️
...ds/internal/clients/internal/testutils/wrappers.go	87.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8183      +/-   ##
==========================================
+ Coverage   82.09%   82.19%   +0.09%     
==========================================
  Files         412      417       +5     
  Lines       40491    41257     +766     
==========================================
+ Hits        33242    33912     +670     
- Misses       5876     5931      +55     
- Partials     1373     1414      +41     

Files with missing lines	Coverage Δ
xds/internal/clients/internal/internal.go	96.00% <ø> (+7.11%)	⬆️
xds/internal/clients/internal/testutils/channel.go	100.00% <100.00%> (ø)
.../clients/internal/testutils/e2e/clientresources.go	92.00% <ø> (-1.94%)	⬇️
xds/internal/clients/xdsclient/logging.go	100.00% <100.00%> (ø)
xds/internal/clients/xdsclient/xdsconfig.go	100.00% <100.00%> (ø)
...ds/internal/clients/internal/testutils/wrappers.go	87.50% <87.50%> (ø)
.../internal/clients/xdsclient/clientimpl_watchers.go	83.78% <83.78%> (ø)
...s/internal/clients/grpctransport/grpc_transport.go	88.77% <86.53%> (-1.80%)	⬇️
xds/internal/clients/xdsclient/xdsclient.go	79.62% <79.52%> (+79.62%)	⬆️
xds/internal/clients/xdsclient/authority.go	70.32% <70.32%> (ø)

... and 62 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[purnesh42H]

@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

[purnesh42H]

@dfawley i have made the change to use ServerConfig and ServerIdentifier as map key for xdsclient and transport respectively. Now we don't need equal. Only thing is for grpcTransport, we return runtime error if extensions are not *ServerIdentifierExtension. Though I was testing and found that if the two extensions pointing to same address but have different values, they are still considered equal. Is that okay since that's our transport?

Could you do the pass on last commit and confirm if we can finalize this approach?

[easwars]

Apologies for the delay.

[purnesh42H]

Apologies for the delay.

thanks @easwars. I have addressed all the comments. Could you please do another pass?

[easwars]

Mostly LGTM.

Just a couple of open comments that needs addressing.

[easwars]

I thought we were going to require byte slices for the send and recv functionalities on the transport. But why do we need to return a byte slice here? We are marshaling the proto to bytes here and the caller would do exactly the opposite. So, I'm not seeing the reason. Maybe I'm missing something?