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 https://github.com/grpc/grpc-go/pull/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[bot]]

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 :warning:
xds/internal/clients/xdsclient/xdsclient.go	79.52%	28 Missing and 15 partials :warning:
...s/internal/clients/grpctransport/grpc_transport.go	86.53%	5 Missing and 2 partials :warning:
.../internal/clients/xdsclient/clientimpl_watchers.go	83.78%	3 Missing and 3 partials :warning:
...ds/internal/clients/internal/testutils/wrappers.go	87.50%	2 Missing and 1 partial :warning:

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%)	:arrow_up:
xds/internal/clients/internal/testutils/channel.go	100.00% <100.00%> (ø)
.../clients/internal/testutils/e2e/clientresources.go	92.00% <ø> (-1.94%)	:arrow_down:
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%)	:arrow_down:
xds/internal/clients/xdsclient/xdsclient.go	79.62% <79.52%> (+79.62%)	:arrow_up:
xds/internal/clients/xdsclient/authority.go	70.32% <70.32%> (ø)

... and 62 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: 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?

[purnesh42H]

@dfawley I have made the change to restrict ServerConfigExtension to be added as value and have added csds dump tests. PTAL. Thanks

[easwars]

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.

Didn't we at some point say that we will move the code as-in in one PR and then make changes in a follow-up, so that we can clearly see what is changing. If that is not feasible, could you please describe the changes in detail in each of those non-test files that are being copied over. Thanks.

[purnesh42H]

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.

Didn't we at some point say that we will move the code as-in in one PR and then make changes in a follow-up, so that we can clearly see what is changing. If that is not feasible, could you please describe the changes in detail in each of those non-test files that are being copied over. Thanks.

yeah its not possible to have separate PRs with copy paste as they will fail the build. Basically if you see the commits, the one starting with copy are exact copy from internal xds client and then all the copy commits are followed by the modifications. So, you can review the modifications. For 3 files you mentioned this is the copy commit 94b41357e3cbe59e9f5871db2ddbe56818d1cc11 and the next commit 35cd65e4ed4e1aa615266098901bab92a12ddf99 is modifications. Ofcourse they were later trimmed a bit more based on doug's review but nothing major. Same pattern is followed for copied e2e tests.

[easwars]

Apologies for the delay.

[purnesh42H]

Apologies for the delay.

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