Does your test spin up multiple servers as part of a single test?

We recently introduced some changes wherein we added support for multiple xds clients per binary (instead of the global singleton that we used to have earlier). The way this works is that we keep a map of xds clients indexed by a name. The value of the name is the grpc channel's dial target when the xds client is created from a grpc client. The value of the name is a fixed string for xds clients created from grpc servers. So, grpc channels with different target URIs will get different xds clients, but all grpc servers will get the same xds client. These changes were made as part of https://github.com/grpc/proposal/blob/master/A71-xds-fallback.md.

As part of the above changes, we also consolidated some code used to create xds clients for testing. Looks like this is what broke your tests.

Yes, we spin up multiple servers in one test

Let me discuss with other language implementors to see how they support this post A71.

@howardjohn : While I think about other ways to support this, why would you want to spin up multiple servers within the same test that all require different bootstrap configurations? FWIW: this will not be possible in production, since all servers will end up with the same bootstrap configuration.

We want to test client --> {set of servers}. https://github.com/istio/istio/blob/86057475a6e34dcdd2de9dfe8ccd8f8b483c7edc/pilot/pkg/networking/grpcgen/grpcecho_test.go#L166

Also its not the same test necessarily, its the same binary. Multiple go tests in the same package are in one binary. So we have a test for a server with mTLS, and then later another without for instance. 1 server per test but same issue. But we do have a test with multiple servers in a single test as noted above

But we do have a test with multiple servers in a single test as noted above

Why would these servers require different boostrap configurations though?

Istio identifies distinct workloads by the node Id which is a part of the bootstrap. Each one gets different config based on the node id

This is going to take a little bit of time to fix, and the fix that we have in mind is big enough to not be appropriate for a patch release.

Here is the proposal:

• Create a new type xdsclient.Pool that represents a pool of xds clients:
  • This will internally contain a map of xds clients keyed by a name value, as described in A71.
  • The xdsclient.Pool will be initialized with a bootstrap configuration that will be used by all xds clients in the pool.
• Have a global variable named xdsclient.DefaultPool that is set to a new pool at init time
  • This will read the bootstrap env vars to get its bootstrap configuration
• The existing dial option and server option (to specify bootstrap configuration to be used by the xds client) will be changed to create a new xdsclient.Pool
  • the resolver will check if the dial option is set, and if so, it will use that pool instead of the default pool
  • the server will check if the server option is set, and if so, it will use that pool instead of the default pool

The above approach will ensure that when tests create multiple servers or channels with different bootstrap configs, they each will get their own pools.

The xdsclient.Pool will provide the following methods:

• NewClient(name)
• GetClientForTesting(name)
  • This is required by some tests which need to get a ref to the xds client currently in use, to trigger a resource not found error
• SetFallbackBootstrapConfig(bootstrapContents)
  • This is required by the c2p resolver to specify a bootstrap configuration to use a fallback when the bootstrap env vars are not specified.

We will also have a way to create the pool

• xdsclient.NewPool(bootstrapContents)

We could also consolidate NewClient(name) and GetClientForTesting(name) with a single method GetOrCreateClient(name).

Contributions are welcome.

Also, the branch for v1.67.0 release has already been cut. So, we should try to get this in by v1.68.0 release.

Can the change be reverted or fixed in a patch release?

Because of how go module resolution works, we cannot depend on a module that depends on grpc 1.66+ and stay on 1.65. This means we need to chose between picking up critical dependency updates and supporting proxyless gRPC in Istio. Between those two, we would unfortunately be likely pressured to drop support for proxyless gRPC given it is an experimental feature.