xds: Implementation of xds_resolver using LDS/RDS

[easwars]

This change is 

[menghanl]

Reviewed 4 of 12 files at r1.
Reviewable status: 4 of 12 files reviewed, 1 unresolved discussion (waiting on @easwars)

---

xds/internal/registry/registry.go, line 23 at r1 (raw file):

// creates the client object (during which it is added to the registry) and
// passes the registry key the latter.
package registry

Make registry private in package client, and

New can take a key as input.

package client
func New(key string) (*Client, string, error) {}

If key exists, it returns the client and the same key.
If key doesn't already exists (or only if key is an empty string), it creates a new client and returns the new key.

(Or, Client has a method Key(), so New above only returns (*Client, error). But this method looks a bit weird)

---

Another option is to have separate functions for client.New() and client.FromKey(key string), FromKey errors if key doesn't exist

[easwars]

Reviewable status: 4 of 12 files reviewed, 1 unresolved discussion (waiting on @menghanl)

---

xds/internal/registry/registry.go, line 23 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Make registry private in package client, and

New can take a key as input.

package client
func New(key string) (*Client, string, error) {}

If key exists, it returns the client and the same key.
If key doesn't already exists (or only if key is an empty string), it creates a new client and returns the new key.

(Or, Client has a method Key(), so New above only returns (*Client, error). But this method looks a bit weird)

---

Another option is to have separate functions for client.New() and client.FromKey(key string), FromKey errors if key doesn't exist

I initially had the registry as private within the client package, and had a method on the Client object to return the registry ID. But now I feel that it does not belong in the client package. It should not be the job of the Client object to put itself into a registry. In fact, I feel that the Client object should not even be concerned about the existence or non-existence of a registry. The registry's existence only matters to the resolver and the balancer.

[easwars]

This is now good to be looked at.

[menghanl]

Reviewable status: 4 of 10 files reviewed, 2 unresolved discussions (waiting on @easwars and @menghanl)

---

xds/internal/resolver/xds_resolver.go, line 77 at r1 (raw file):

		// credentials from the parent channel (passed through the
		// resolver.BuildOptions).
		config.Creds = defaultDialCreds(config.BalancerName, rbo)

Should we do the config related things in client.New()? Otherwise the EDS balancer will need to do it again (if it's not used with xds resolver)

---

xds/internal/resolver/xds_resolver_test.go, line 144 at r1 (raw file):

// TestResolverBuilder tests the xdsResolverBuilder's Build method with
// different parameters.
func TestResolverBuilder(t *testing.T) {

This mostly testing bootstrap.Config and the default values.
If we move bootstrap.Config to client.New, we will need to move this to TestClientNew

[easwars]

Reviewable status: 4 of 10 files reviewed, 2 unresolved discussions (waiting on @menghanl)

---

xds/internal/resolver/xds_resolver.go, line 77 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Should we do the config related things in client.New()? Otherwise the EDS balancer will need to do it again (if it's not used with xds resolver)

Discussed this offline, and decided that we are OK with a small amount of duplicate code in the EDS balancer to create the new xdsClient (if it doesn't get one from the resolver) for now. Once we have a mechanism to register credentials, the bootstrap process will become much simpler and at that point, there won't be much to be done in terms of getting defaultDialCreds in the resolver and balancer when the bootstrap doesn't yield any credentials.
Decided to keep this as is for now.

---

xds/internal/resolver/xds_resolver_test.go, line 144 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

This mostly testing bootstrap.Config and the default values.
If we move bootstrap.Config to client.New, we will need to move this to TestClientNew

I'm guessing this is also staying as we are keeping the code to do the bootstrap in the resolver for now.

[menghanl]

Reviewed 2 of 6 files at r2, 3 of 4 files at r4.
Reviewable status: 7 of 10 files reviewed, 15 unresolved discussions (waiting on @easwars and @menghanl)

---

xds/internal/client/client.go, line 50 at r3 (raw file):

	// non-default values, remove this field, and directly use the appropriate
	// one in v2Client.
	Backoff backoff.Strategy

We can remove this. I believe it was made an option/field to test backoff.

---

xds/internal/client/client.go, line 70 at r3 (raw file):

// NewClient returns a new xdsClient configured with opts.
func NewClient(opts Options) (*Client, error) {

New()
(didn't go lint/vet complain?)

---

xds/internal/client/client.go, line 76 at r3 (raw file):

	case opts.Config.Creds == nil:
		return nil, errors.New("xds: no credentials provided in options")
	case opts.Config.NodeProto == nil:

Not error with nil.

---

xds/internal/client/client.go, line 149 at r3 (raw file):

// WatchForServiceUpdate uses LDS and RDS protocols to discover resource
// information for the provided target.
func (c *Client) WatchForServiceUpdate(target string, callback func(ServiceUpdate, error)) (cancel func()) {

Naming bikeshedding: WatchService()?

---

xds/internal/client/client.go, line 149 at r3 (raw file):

// WatchForServiceUpdate uses LDS and RDS protocols to discover resource
// information for the provided target.
func (c *Client) WatchForServiceUpdate(target string, callback func(ServiceUpdate, error)) (cancel func()) {

And let's error if Client is already closed.

---

xds/internal/client/bootstrap/bootstrap.go, line 154 at r4 (raw file):

	// one and set the version number here. That way, callers of this function
	// can always expect that the NodeProto field is non-nil.
	if config.NodeProto == nil {

Remove line 129 where we set the version, and do it here

if node == nil {
  node = &Node{}
}
node.BuildVersion = gRPCVersion

---

xds/internal/registry/registry.go, line 42 at r3 (raw file):

// Get returns the xds client object corresponding key. It returns an error if
// no client could be found for key.
func Get(key string) (interface{}, error) {

Why did we decide to pass a key from resolver to balancer, instead of passing the pointer directly?

One possible reason is, at that time, we wanted to pass the key as part of service config. And because service config is a json string, there's no easy way to convert between a pointer and a string. But now with Attributes, passing pointer should be pretty easy.

---

xds/internal/registry/registry.go, line 47 at r3 (raw file):

// AddTo adds client to the registry and returns it's registry ID.
func AddTo(client interface{}) string {

Just Add?

---

xds/internal/registry/registry.go, line 52 at r3 (raw file):

// RemoveFrom removes the client corresponding to key from the registry.
func RemoveFrom(key string) {

Remove?

---

xds/internal/resolver/xds_resolver.go, line 56 at r3 (raw file):

// for the xDS resolver.
func NewBuilder() resolver.Builder {
	return &xdsBuilder{}

Let's keep the old resolver in a resolver_old.go, so we don't break the existing tests.

We can delete old and switch when all the other parts (CDS+EDS) are ready.

---

xds/internal/resolver/xds_resolver.go, line 78 at r3 (raw file):

		// resolver.BuildOptions).
		config.Creds = defaultDialCreds(config.BalancerName, rbo)
	case config.NodeProto == nil:

No error.

---

xds/internal/resolver/xds_resolver.go, line 153 at r4 (raw file):

	// A channel for the watch API callback to write service updates on to. The
	// updates are read by the run goroutine and passed on to the ClientConn.
	updateCh *buffer.Unbounded

Can this be a make(chan, 1)?
We should only need the move recent update.

---

xds/internal/resolver/xds_resolver.go, line 179 at r4 (raw file):

// ClientConn.
func (r *xdsResolver) run() {
	r.mu.Lock()

This could race with Close(), and cause cancelWatch to be not called in Close()

Move it out, in build(), before go run()?

---

xds/internal/resolver/xds_resolver.go, line 207 at r4 (raw file):

	}
	if err != nil {
		r.cc.ReportError(err)

Error should go on the channel, too. Just push su?

[easwars]

Reviewable status: 7 of 10 files reviewed, 15 unresolved discussions (waiting on @menghanl)

---

xds/internal/client/client.go, line 50 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

We can remove this. I believe it was made an option/field to test backoff.

Done.

---

xds/internal/client/client.go, line 70 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

New()
(didn't go lint/vet complain?)

Done.
Although vet didn't complain.

---

xds/internal/client/client.go, line 76 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Not error with nil.

I've have changed the bootstrap code to populate the NodeProto field with an empty one if the bootstrap file doesn't provide one. So, ideally this should never be called with a nil proto. All the checks here are for really bad error conditions, which we don't expect.

---

xds/internal/client/client.go, line 149 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Naming bikeshedding: WatchService()?

Done.

---

xds/internal/client/client.go, line 149 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

And let's error if Client is already closed.

Added some TODOs.

---

xds/internal/client/bootstrap/bootstrap.go, line 154 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Remove line 129 where we set the version, and do it here

if node == nil {
  node = &Node{}
}
node.BuildVersion = gRPCVersion

Done.1

---

xds/internal/registry/registry.go, line 42 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Why did we decide to pass a key from resolver to balancer, instead of passing the pointer directly?

One possible reason is, at that time, we wanted to pass the key as part of service config. And because service config is a json string, there's no easy way to convert between a pointer and a string. But now with Attributes, passing pointer should be pretty easy.

Done.

---

xds/internal/registry/registry.go, line 47 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Just Add?

Removed the registry package.

---

xds/internal/registry/registry.go, line 52 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Remove?

Removed the registry package.

---

xds/internal/resolver/xds_resolver.go, line 56 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Let's keep the old resolver in a resolver_old.go, so we don't break the existing tests.

We can delete old and switch when all the other parts (CDS+EDS) are ready.

Moved the old implementation to old/ directory and added an 'init` to register the resolver. Also changed the scheme name of the new one.

---

xds/internal/resolver/xds_resolver.go, line 78 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

No error.

Don't understand this comment.

---

xds/internal/resolver/xds_resolver.go, line 153 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Can this be a make(chan, 1)?
We should only need the move recent update.

Done.

---

xds/internal/resolver/xds_resolver.go, line 179 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

This could race with Close(), and cause cancelWatch to be not called in Close()

Move it out, in build(), before go run()?

Done.

---

xds/internal/resolver/xds_resolver.go, line 207 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Error should go on the channel, too. Just push su?

Done.

[menghanl]

Reviewed 1 of 2 files at r3, 9 of 10 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @easwars)

---

xds/internal/internal.go, line 29 at r5 (raw file):

// object shared between the resolver and the balancer. The xdsClient object is
// created by the resolver and passed to the balancer.
const XDSClientID = "xdsClientID"

Use an internal type instead of string.
Similar to context: https://golang.org/pkg/context/#WithValue

type clientID string
const XDSClientID = clientID("xdsClientID")

---

xds/internal/client/client_test.go, line 128 at r4 (raw file):

	oldDialFunc := dialFunc
	dialFunc = func(_ context.Context, _ string, _ ...grpc.DialOption) (*grpc.ClientConn, error) {
		return fakeCC, nil

Why not actually new a Client, with fakeServer's address in Options?

---

xds/internal/client/client_test.go, line 147 at r4 (raw file):

			return
		}
		if err != nil {

Nit: check err!=nil first? Because if err is not nil, su.Cluster will most likely be "", and the other if will also fail. But then we will miss the error.

---

xds/internal/client/bootstrap/bootstrap.go, line 154 at r5 (raw file):

	// that the NodeProto field is non-nil.
	if config.NodeProto == nil {
		config.NodeProto = &corepb.Node{BuildVersion: gRPCVersion}

Nit: this and the line below will both set BuildVersion.

---

xds/internal/resolver/xds_resolver.go, line 78 at r3 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Don't understand this comment.

This comment was probably on a old version. I also don't remember what it was about.

[easwars]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @menghanl)

---

xds/internal/internal.go, line 29 at r5 (raw file):

Previously, menghanl (Menghan Li) wrote…

Use an internal type instead of string.
Similar to context: https://golang.org/pkg/context/#WithValue

type clientID string
const XDSClientID = clientID("xdsClientID")

Done.

---

xds/internal/client/client_test.go, line 128 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Why not actually new a Client, with fakeServer's address in Options?

We would still have to override the dialFunc to return the testClientConn, right?

---

xds/internal/client/client_test.go, line 147 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Nit: check err!=nil first? Because if err is not nil, su.Cluster will most likely be "", and the other if will also fail. But then we will miss the error.

Done.

---

xds/internal/client/bootstrap/bootstrap.go, line 154 at r5 (raw file):

Previously, menghanl (Menghan Li) wrote…

Nit: this and the line below will both set BuildVersion.

Sorry, copy-paste error.

---

xds/internal/resolver/xds_resolver.go, line 78 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

This comment was probably on a old version. I also don't remember what it was about.

Hmm .. this TODO is still valid right. At some point, we will start supporting credential registration, and at that point, if the bootstrap file contains a credential type which is not registered, we should fail (instead of getting creds from the parent channel).

[menghanl]

Reviewed 3 of 3 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

---

xds/internal/resolver/xds_resolver.go, line 78 at r3 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Hmm .. this TODO is still valid right. At some point, we will start supporting credential registration, and at that point, if the bootstrap file contains a credential type which is not registered, we should fail (instead of getting creds from the parent channel).

Yes, the code here looks good.

[menghanl]

Reviewed 1 of 1 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved