Reviewed 29 of 32 files at r1, 3 of 3 files at r2, 7 of 7 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @apolcyn and @markdroth)

src/core/ext/filters/client_channel/xds/xds_api.cc, line 214 at r2 (raw file):

  envoy_api_v2_core_Node* node_msg =
      envoy_api_v2_DiscoveryRequest_mutable_node(request, arena.ptr());
  UniquePtr<char> build_version;

It's never used? Same below.

src/core/ext/filters/client_channel/xds/xds_bootstrap.h, line 70 at r2 (raw file):

  const char* server_uri() const { return server_uri_; }
  const InlinedVector<ChannelCreds, 1> channel_creds() const {

This can return a reference.

src/core/ext/filters/client_channel/xds/xds_bootstrap.cc, line 383 at r2 (raw file):

    case GRPC_JSON_NUMBER:
      result->type = MetadataValue::Type::DOUBLE;
      errno = 0;  // To distinguish error.

How is this intended to be used? I don't see any modification to its value.

src/core/ext/filters/client_channel/xds/xds_channel_secure.cc, line 80 at r2 (raw file):

      }
    }
    if (creds == nullptr) return nullptr;

Why don't we try finding the creds in args in this case?

src/core/ext/filters/client_channel/xds/xds_client.cc, line 1191 at r2 (raw file):

  if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) {
    gpr_log(GPR_INFO, "[xds_client %p: creating channel to %s", this,
            bootstrap_->server_uri());

This may cause crash.

1. bootstrap_ may be null.
2. bootstrap_->server_uri() may be null.

Reviewable status: 31 of 32 files reviewed, 5 unresolved discussions (waiting on @apolcyn and @AspirinSJL)

src/core/ext/filters/client_channel/xds/xds_api.cc, line 214 at r2 (raw file):

We allocate the build_version string inside of PopulateNode(), but we need to avoid freeing it until after the protobuf is serialized below. This UniquePtr<> holds on to the string and frees it when it goes out of scope.

I've added a comment about this.

src/core/ext/filters/client_channel/xds/xds_bootstrap.h, line 70 at r2 (raw file):

Good catch! Done.

src/core/ext/filters/client_channel/xds/xds_bootstrap.cc, line 383 at r2 (raw file):

strtod(3) modifies errno if it fails. See the man page for details.

src/core/ext/filters/client_channel/xds/xds_channel_secure.cc, line 80 at r2 (raw file):

Because if the bootstrap file sets creds to a particular type and we don't know about that type, then we should fail instead of picking the wrong creds. Otherwise, it will be surprising to the user, because we will silently ignore what they told us to do.

src/core/ext/filters/client_channel/xds/xds_client.cc, line 1191 at r2 (raw file):

Good catch. Fixed.

Note that (2) is not a problem, because gpr_log() will format a null pointer as (null).

I didn't see the new commit; please push the new commit, thanks!

Reviewable status: 31 of 32 files reviewed, 5 unresolved discussions (waiting on @apolcyn and @AspirinSJL)

Oh now I can see, sorry..

Reviewable status: 28 of 32 files reviewed, 5 unresolved discussions (waiting on @apolcyn and @AspirinSJL)

Reviewed 1 of 1 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @markdroth)

src/core/ext/filters/client_channel/xds/xds_api.cc, line 214 at r2 (raw file):

I think we should make the version string a member of some xds object. Then we can make the code here more straightforward, and we don't have to build the string for each request.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @AspirinSJL)

src/core/ext/filters/client_channel/xds/xds_api.cc, line 214 at r2 (raw file):

Good idea. Done.

Reviewed 5 of 5 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)