xds: introduce generic xds clients xDS and LRS Client API signatures

[purnesh42H]

This is the next part of generic xds clients to be usable outside of grpc which builds on top of #8024. This change is adding the user API signatures for xDS and LRS client (without implementation) to communicate with xDS management server.

POC
Internal Design

RELEASE NOTES: None

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.

Project coverage is 82.25%. Comparing base (c7db760) to head (a1fc72b).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/clients/lrsclient/load_store.go	0.00%	12 Missing ⚠️
xds/internal/clients/xdsclient/xdsclient.go	0.00%	8 Missing ⚠️
xds/internal/clients/lrsclient/lrsclient.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8042      +/-   ##
==========================================
+ Coverage   82.23%   82.25%   +0.01%     
==========================================
  Files         387      390       +3     
  Lines       38947    38971      +24     
==========================================
+ Hits        32028    32054      +26     
- Misses       5601     5609       +8     
+ Partials     1318     1308      -10     

Files with missing lines	Coverage Δ
xds/internal/clients/config.go	97.61% <ø> (+4.43%)	⬆️
xds/internal/clients/lrsclient/lrsclient.go	0.00% <0.00%> (ø)
xds/internal/clients/xdsclient/xdsclient.go	0.00% <0.00%> (ø)
xds/internal/clients/lrsclient/load_store.go	0.00% <0.00%> (ø)

... and 30 files with indirect coverage changes

[purnesh42H]

@dfawley i did the godoc review and made following changes (see the last commit)

• removed the helper methods on struct that are not being used to avoid exporting them. we can add later during implementation.
• removed square brackets from docstrings because actual param and struct field already has the link to respective object definition.

[dfawley]

Let's leave "old" and "style" out of the docstrings and describe what it is or what it is used for in specific terms instead.

[purnesh42H]

I have modified the documentation. One more thing i have added in both Authority and here about order of servers in the list for precedence. Let me know if thats okay. I remember there was a question recently about how the fallback servers are chosen. Should we mention the gRFC as well?

[easwars]

Super nit. Maybe the receiver name can be ls instead of s. I don't know if we would have more methods on this type, but if we do, we would want to use the same receiver name on all of them.

[purnesh42H]

Done

[easwars]

There are a few open questions here:

• How should the client(s) and the streams (watch or load) be closed? We need to finalize on an approach that is consistent between the clients (if possible), and one that would work with an underlying reference counted implementation.
  • In the existing xDS client code, the function to create the client returns a cancel function to close the client. And the method to create the watch and the load report also return a cancel function which closes the watch and the load report respectively.
  • In the current PR, the New function returns a client which has a Close method.
    • The WatchResource method on the client returns a cancel function to cancel the watch.
    • The ReportLoad method on the client returns a LoadStore, but there is no way to cancel a load reporting stream.
• Name of the parameter in the watch callbacks to be invoked by the user to indicate completion of local processing. (minor)
• How and where will type assertions happen on the WatchResource method?
  • Finalize the plan on how this API will be used. As I mentioned previously, in the current codebase, we have convenience methods for each type of watch, and users don't generally directly call the generic WatchResource method.

[purnesh42H]

There are a few open questions here:

• How should the client(s) and the streams (watch or load) be closed? We need to finalize on an approach that is consistent between the clients (if possible), and one that would work with an underlying reference counted implementation.

  • In the existing xDS client code, the function to create the client returns a cancel function to close the client. And the method to create the watch and the load report also return a cancel function which closes the watch and the load report respectively.

  • In the current PR, the New function returns a client which has a Close method.

    • The WatchResource method on the client returns a cancel function to cancel the watch.
    • The ReportLoad method on the client returns a LoadStore, but there is no way to cancel a load reporting stream.

• Name of the parameter in the watch callbacks to be invoked by the user to indicate completion of local processing. (minor)

• How and where will type assertions happen on the WatchResource method?

  • Finalize the plan on how this API will be used. As I mentioned previously, in the current codebase, we have convenience methods for each type of watch, and users don't generally directly call the generic WatchResource method.

1. Keep the returned cancel function in xDS client watcher. Have Stop() method in LoadStore to close the LRS stream.
2. Keep a single name like 'done' for function and document that way
3. Figure out if we can avoid user requiring type assertion in watcher callbacks. Try if we can use generics. If we can't then current way is okay.

@dfawley @easwars do these look good to you?

[easwars]

Yes for 1 and 2.

And as discussed offline, using generics for 3 might not be worth the effort. It might be a fun side project to play with generics and to see how far you can get, but for the purpose of this effort, we are OK with having type assertion in the watcher.

[dfawley]

This is still bothering me.

The transport shouldn't need this, so it shouldn't be in the thing we pass to it. It's specific to the xds client. So it should really be more like:

package clients

type ServerIdentifier /* ? open to bikeshedding this name */ struct {
	URI string
	Extensions any
}

...

package xdsclient

type ServerConfig struct {
	ServerIdentifier clients.ServerIdentifier
	IgnoreResourceDeletion bool
}

If we don't do this now, then eventually we'll need to add all server configuration flags into this top-level struct for all types of clients.

[purnesh42H]

This seems wrong to me. Are you suggesting ServerIdentifier to be common struct between lrs and xds client? Because I remember @markdroth mentioning that even the flags are part of server hash. We do the same in internal client implementation.

[dfawley]

This also strikes me as "detail of xdsclient itself, so shouldn't be here", but I'm OK with leaving it for now...let's see how the rest of the code looks

[purnesh42H]

yeah these will be populated by xds client implementation.

[dfawley]

:(

Let's avoid annotations for tools polluting our code wherever possible.

For this one, can you not make the unused parameters nameless or _?

[purnesh42H]

I think for this file and xdsclient because the params like locality are informational but I have removed from lrsclient.go

[dfawley]

"A LoadStore keeps track of"

And how about "collects" or "aggregates" instead of "keeps track of"?

[purnesh42H]

aggregates seems better

[dfawley]

How about something like ReporterForCluster?

[purnesh42H]

Changed

[dfawley]

Can we delete TypeURL from ResourceType? (I feel like this came up during design review, too, but I'm not remembering if we decided to remove it, or keep it and why.)

[purnesh42H]

We pass around the ResourceType object and xdsclient implementation use typeURL while sending and receiving messages. So, we have to persist in ResourceType instance or pass around everywhere. I think having in ResourceType instance is more convenient.

[dfawley]

It would be really helpful to add an example here.

[purnesh42H]

Do you mean example of mapping like below?

listenerType = xdsclient.ResourceType{
		TypeURL:                    "type.googleapis.com/envoy.service.discovery.v3.Resource",
		TypeName:                   ListenerResourceTypeName,
		AllResourcesRequiredInSotW: true,
		Decoder:                    listenerDecoder{},
	}
resourceTypes["url"] = ListenerResourceType

[dfawley]

This link doesn't seem that helpful to me -- at least, it's a big-ish doc with a bunch of other stuff in it, and doesn't link to a section about the topic.

It doesn't seem like anyone actually uses federation, because I can't find any good documentation on it, either.

This is about all I can find in Envoy's docs, too:

https://github.com/cncf/xds/blob/main/proposals/TP1-xds-transport-next.md

https://www.envoyproxy.io/docs/envoy/latest/xds/core/v3/resource_locator.proto#xds-core-v3-resourcelocator

[purnesh42H]

I have put https://www.envoyproxy.io/docs/envoy/latest/xds/core/v3/resource_locator.proto#xds-core-v3-resourcelocator as that makes more sense as someone who is using authority map would be familiar with xdstp

[dfawley]

Suggested change

	// The order of the servers in this list reflects the order of preference
	// of the data returned by those servers. xDS client use the first
	// available server from the list.
	//
	// See gRFC A71 for more details on fallback behavior when the primary
	// xDS server is unavailable.
	//
	// gRFC A71: https://github.com/grpc/proposal/blob/master/A71-xds-fallback.md
	// See Config.Servers for more details.

?

[purnesh42H]

Done

[dfawley]

What is this used for?

What happens when an Authority isn't used for the resource?

[purnesh42H]

I haven't been able to think of any business use case for extensions in authority but in the design discussion we had agreed on having structs for all config types. For grpc name resolver we need ClientListenerResourceNameTemplate in authority.