Support RDS updates on the server

[yashykt]

Note that graceful shutdown is still remaining.

---

This change is 

[markdroth]

Here's my partial review, as we discussed. I haven't reviewed the tests yet.

Please let me know if you have any questions. Thanks!

Reviewed 22 of 25 files at r1, 4 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 24 of 25 files reviewed, 27 unresolved discussions (waiting on @markdroth and @yashykt)

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 625 at r1 (raw file):

    GetCallConfigArgs args) {
  for (const auto& entry : route_table_) {
    // Path matching.

How about moving this route selection logic into the XdsRouting API as well, since it should be common between client and server? The code for deciding what to do with the selected route will still be different, but the code to select the route itself could be the same.

What I'm thinking is that instead of having XdsRouting expose HeadersMatch() and UnderFraction(), we could instead have it expose a GetRouteForRequest() method. This could work similar to the virtual host iteration interface that I suggested in xds_routing.h (see my comment there for details). For example:

class RouteListIterator {
 public:
  virtual ~RouteListIterator() = default;
  // Number of routes.
  virtual size_t Size() const = 0;
  // Returns the matchers for the route at the specified index.
  virtual const XdsApi::Route::Matchers& GetMatchersForRoute(size_t index) const = 0;
};

// Returns the index in route_list_iterator to use for a request with
// the specified path and metadata, or nullopt if no route matches.
static absl::optional<size_t> GetRouteForRequest(
    RouteListIterator* route_list_iterator,
    absl::string_view path, grpc_metadata_batch* initial_metadata) {
  for (size_t i = 0; i < route_list_iterator->Size(); ++i) {
    const XdsApi::Route::Matchers& matchers =
        route_list_iterator->GetMatchersForRoute(i);
    if (matchers.path_matcher.Match(path) &&
        HeadersMatch(matchers.header_matchers, initial_metadata) &&
        (!matchers.fraction_per_million.has_value() ||
         UnderFraction(*matchers.fraction_per_million))) {
      return i;
    }
  }
  return absl::nullopt;
}

---

src/core/ext/xds/xds_api.cc, line 2329 at r1 (raw file):

            "router", {kXdsHttpRouterFilterConfigName, Json()}});
  }
  // Guarding parsing of RouteConfig on the server side with the environmental

I think we still want this env var guard until all of the server-side pieces are in place.

---

src/core/ext/xds/xds_client.cc, line 870 at r1 (raw file):

  auto& state = state_map_[type_url].subscribed_resources[name];
  if (state == nullptr) {
    state = MakeOrphanable<ResourceState>(type_url, name, false);

Let's merge #27860 before this PR, so we can get the right fix in place here.

---

src/core/ext/xds/xds_routing.h, line 38 at r1 (raw file):

 public:
  template <typename T>
  static T* FindVirtualHostForDomain(std::vector<T>* virtual_hosts,

Instead of making this a template, how about defining an abstract interface for the virtual host object that this acts on:

class VirtualHostListIterator {
 public:
  virtual ~VirtualHostListIterator() = default;
  // Returns the number of virtual hosts in the list.
  virtual size_t Size() const = 0;
  // Returns the domain list for the virtual host at the specified index.
  virtual const std::set<std::string>& GetDomainsForVirtualHost(size_t index) const = 0;
};

// Returns the index of the selected virtual host in the list.
static absl::optional<size_t> FindVirtualHostForDomain(
    VirtualHostListIterator* vhost_iterator,
    absl::string_view domain) {
  absl::optional<size_t> target_index;
  for (size_t i = 0; i < vhost_iterator->Size(); ++i) {
    const std::set<std::string> domains = vhost_iterator->GetDomainsForVirtualHost(i);
    for (const std::string& domain_pattern : domains) {
      // ...update target_index instead of target_vhost...
    }
  }  
  return target_index;
}

Then callers can just wrap their lists of virtual hosts of whatever type in their own implementation of VirtualHostListIterator.

---

src/core/ext/xds/xds_routing.h, line 38 at r1 (raw file):

 public:
  template <typename T>
  static T* FindVirtualHostForDomain(std::vector<T>* virtual_hosts,

How about passing in virtual_hosts as a reference instead of a pointer, given that it should never be null?

(No longer relevant if you take my previous suggestion.)

---

src/core/ext/xds/xds_routing.h, line 99 at r1 (raw file):

 private:
  enum MatchType {

All of this private stuff can be in an anonymous namespace in the .cc file, since it's all static anyway.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 50 at r3 (raw file):

class XdsServerConfigFetcher : public grpc_server_config_fetcher {
 public:
  class FilterChainMatchManager

There are a lot of classes here that interact with each other in complex ways, and it makes the code hard to understand for someone who is first approaching it. I have a couple of suggestions to try to help future readers understand the code better.

First, where possible, I think it would make this code more readable to use forward declarations to move class definitions out of the enclosing class, like this:

class XdsServerConfigFetcher : public grpc_server_config_fetcher {
 public:
  class FilterChainMatchManager;  // Forward declaration.
  // ...other members...
};

class XdsServerConfigFetcher::FilterChainMatchManager
    : public grpc_server_config_fetcher::ConnectionManager {
 public:
  // ...members...
};

This makes it easier for the reader to see each class by itself, without having to understand each nested class at the same time.

Second, please add a comment before each class explaining what it is and how it interacts with other classes. For example, for FilterChainMatchManager, you can say something like this:

"""
A connection manager used by the server listener code to inject channel args to be used for each incoming connection. This implementation chooses the appropriate filter chain from the xDS Listener resource and injects channel args that configure the right mTLS certs and cause the right set of HTTP filters to be injected.
"""

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 101 at r3 (raw file):

  };

  class RdsUpdateWatcherInterface {

Now that we're not exposing an L7-level watcher API as a part of the server config fetcher API (i.e., it's now invisible to all code except the code in this file), I don't think we actually need this to be an interface anymore. At this point, it looks like the only users of this interface are ListenerWatcher and XdsServerConfigSelectorProvider, and those classes are effectively part of the implementation of XdsServerConfigFetcher, so I don't think we actually need an abstraction between them anymore.

Instead, I think we should just allow ListenerWatcher and XdsServerConfigSelectorProvider to directly modify the internal state of XdsServerConfigFetcher. Note that ListenerWatcher is already nested within XdsServerConfigFetcher, so it can already do that; we'd just need to move XdsServerConfigSelectorProvider inside of XdsServerConfigFetcher as well, so that it can do the same thing.

I think that the RdsUpdateWatcherState struct can wind up looking something like this:

  struct RdsUpdateWatcherState {
    RouteConfigWatcher* route_config_watcher = nullptr;
    absl::optional<absl::StatusOr<XdsApi::RdsUpdate>> rds_update;
    // The set of providers that need to be notified when this RDS resource gets updated.
    // Providers will be added here when they are created, and they will automatically
    // remove themselves when they are destroyed.
    // When this becomes empty, the xDS watch can be stopped.
    std::set<XdsServerConfigSelectorProvider*> providers;
  };

I think the code will wind up being much simpler that way, with a lot less bookkeeping.

Hmmm... Alternatively, remind me why we don't just have each XdsServerConfigSelectorProvider start its own RDS watch on the XdsClient directly? That would use the existing watching mechanism instead of building a new one here, which might be even simpler.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 192 at r3 (raw file):

  class RouteConfigWatcher : public XdsClient::RouteConfigWatcherInterface {
   public:
    explicit RouteConfigWatcher(

No need for explicit.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 220 at r3 (raw file):

  // A fire and forget class to start watching on route config data from
  // XdsClient via ExecCtx

Instead of hopping into the ExecCtx before we call back into the XdsClient, I suggest that we do the reverse: change ListenerWatcher and RouteConfigWatcher to hop into the ExecCtx before delivering updates from the XdsClient to us. That way, if we need to call back into the XdsClient (e.g., to start an RDS watch when we get an LDS update), we know that it's safe to do so.

I think this approach has a number of advantages:

• It's consistent with how we do things on the client side. In the xds resolver, every notification from an LDS or RDS watcher creates a fire-and-forget Notifier object, which takes a ref to the XdsResolver and then hops into the ExecCtx and then the WorkSerializer to deliver the notification. We could do essentially the same thing here, except that the notifier would hold a ref to the XdsServerConfigFetcher instead of the XdsResolver, and it would hop into the ExecCtx but not the WorkSerializer.
• It's simpler to implement, because you can use one notifier class for all notifications from either resource type. You would not need separate implementations for RouteConfigWatchStarter and RdsWatchCanceller. (And minimizing the number of classes here would help make this code more understandable.)
• It minimizes the amount of code run while holding the XdsClient's internal lock, which is probably better for lock contention.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 271 at r3 (raw file):

}

class XdsServerConfigSelector : public ServerConfigSelector {

Please move this inside of XdsServerConfigSelectorProvider (in the private section), since it's not needed outside of that class.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 302 at r3 (raw file):

        http_filters) {
  auto config_selector = MakeRefCounted<XdsServerConfigSelector>();
  for (auto& vhost : rds_update.virtual_hosts) {

This looks remarkably similar to the client-side code in the XdsResolver::XdsConfigSelector ctor. Can we refactor any of this into the XdsRouting API?

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 303 at r3 (raw file):

  auto config_selector = MakeRefCounted<XdsServerConfigSelector>();
  for (auto& vhost : rds_update.virtual_hosts) {
    config_selector->virtual_hosts_.push_back(VirtualHost());

This can just say:

config_selector->virtual_hosts_.emplace_back();

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 307 at r3 (raw file):

    virtual_host.domains = std::move(vhost.domains);
    for (auto& route : vhost.routes) {
      virtual_host.routes.push_back(VirtualHost::Route());

Same here:

virtual_host.routes.emplace_back();

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 401 at r3 (raw file):

    return call_config;
  }
  for (const auto& route : virtual_host->routes) {

As mentioned elsewhere, it would be nice if this route-selection logic was in the XdsRouting API.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 435 at r3 (raw file):

// A fire and forget class to cancel watching on Rds from XdsServerConfigFetcher
// via ExecCtx
class RdsWatchCanceller {

This should be nested inside of XdsServerConfigFetcher.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 469 at r3 (raw file):

// StaticXdsServerConfigSelectorProvider and
// DynamicXdsServerConfigSelectorProvider.
class XdsServerConfigSelectorProvider : public ServerConfigSelectorProvider {

This and both of its subclasses should be nested inside of XdsServerConfigFetcher.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 526 at r3 (raw file):

      : http_filters_(std::move(http_filters)) {}

  void set_resource(absl::StatusOr<XdsApi::RdsUpdate> resource) {

This seems a little ugly. Is there a way that we can restructure this such that this is not needed, such as maybe storing the resource in each subclass instead of in the base class? Or maybe if we eliminate RdsUpdateWatcherInterface as I suggested above, there will no longer be a need to have a common base class for the static and dynamic providers.

If we keep this, it should probably be protected, since it's only needed by a subclass.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 890 at r3 (raw file):

XdsServerConfigFetcher::ListenerWatcher::RdsUpdateWatcher::RdsUpdateWatcher(
    std::string resource_name, ListenerWatcher* parent)
    : resource_name_(resource_name), parent_(parent) {}

std::move(resource_name)

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 896 at r3 (raw file):

  {
    MutexLock lock(&parent_->mu_);
    for (auto it = parent_->pending_rds_updates_.begin();

This would be much simpler if pending_rds_updates_ was std::set<> instead of std::vector<>.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 918 at r3 (raw file):

    std::unique_ptr<grpc_server_config_fetcher::WatcherInterface>
        server_config_watcher,
    // grpc_channel_args* args,

Please remove.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 966 at r3 (raw file):

          listener.default_filter_chain ==
              filter_chain_match_manager_->default_filter_chain())) {
      // TODO(): Maybe change LdsUpdate to provide a vector of

I don't think we want to do that. We want the LdsUpdate struct to be close to the structure of the underlying Listener proto, so that it's easy for people to understand where it came from when they see it in logs.

I think it's fine to do the conversion here.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1094 at r3 (raw file):

void XdsServerConfigFetcher::RouteConfigWatcher::OnRouteConfigChanged(
    XdsApi::RdsUpdate route_config) {
  ReleasableMutexLock lock(&server_config_fetcher_->rds_mu_);

I generally find code more readable when I avoid ReleaseableMutexLock. I suggest moving this check into Update(), which would be renamed to MaybeUpdate():

// Returns false if all refs have already been dropped.
bool XdsServerConfigFetcher::RouteConfigWatcher::MaybeUpdate(
    absl::StatusOr<XdsApi::RdsUpdate> rds_update) {
  MutexLock lock(&server_config_fetcher_->rds_mu_);
  auto it = server_config_fetcher_->rds_watchers_.find(resource_name_);
  if (it == server_config_fetcher_->rds_watchers_.end()) return false;
  // ...do update...
  return true;
}

Then the code here can just do this:

if (!MaybeUpdate(std::move(route_config))) {
  // All listener refs have already been dropped. Cancel the watch.
  server_config_fetcher_->xds_client_->CancelRouteConfigDataWatch(
      resource_name_, this, false);
}

We can use the same pattern in OnError() and OnResourceDoesNotExist(), so this approach winds up avoiding some code duplication as well.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1177 at r3 (raw file):

  {
    MutexLock lock(&self->server_config_fetcher_->rds_mu_);
    auto rds_watcher_state_it =

This can just be called it.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1272 at r3 (raw file):

  {
    MutexLock lock(&rds_mu_);
    auto rds_watcher_state_it = rds_watchers_.find(std::string(resource_name));

This can just be called it.

---

src/core/lib/surface/server.h, line 468 at r3 (raw file):

struct grpc_server_config_fetcher
    : public grpc_core::RefCounted<grpc_server_config_fetcher> {

Isn't there still one clear external owner of this? If so, it probably wants to be InternallyRefCounted<> instead of RefCounted<>.

---

src/core/lib/surface/server.cc, line 1608 at r3 (raw file):

  GRPC_API_TRACE("grpc_server_config_fetcher_destroy(config_fetcher=%p)", 1,
                 (server_config_fetcher));
  delete server_config_fetcher;

If the config fetcher is going to be ref-counted now, then I don't think it's okay to just destroy it here; instead, we should unref it (or orphan it, if you take my suggestion elsewhere to make it InternallyRefCounted<>).

[yashykt]

Reviewable status: 3 of 27 files reviewed, 27 unresolved discussions (waiting on @markdroth and @yashykt)

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 625 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

How about moving this route selection logic into the XdsRouting API as well, since it should be common between client and server? The code for deciding what to do with the selected route will still be different, but the code to select the route itself could be the same.

What I'm thinking is that instead of having XdsRouting expose HeadersMatch() and UnderFraction(), we could instead have it expose a GetRouteForRequest() method. This could work similar to the virtual host iteration interface that I suggested in xds_routing.h (see my comment there for details). For example:

class RouteListIterator {
 public:
  virtual ~RouteListIterator() = default;
  // Number of routes.
  virtual size_t Size() const = 0;
  // Returns the matchers for the route at the specified index.
  virtual const XdsApi::Route::Matchers& GetMatchersForRoute(size_t index) const = 0;
};

// Returns the index in route_list_iterator to use for a request with
// the specified path and metadata, or nullopt if no route matches.
static absl::optional<size_t> GetRouteForRequest(
    RouteListIterator* route_list_iterator,
    absl::string_view path, grpc_metadata_batch* initial_metadata) {
  for (size_t i = 0; i < route_list_iterator->Size(); ++i) {
    const XdsApi::Route::Matchers& matchers =
        route_list_iterator->GetMatchersForRoute(i);
    if (matchers.path_matcher.Match(path) &&
        HeadersMatch(matchers.header_matchers, initial_metadata) &&
        (!matchers.fraction_per_million.has_value() ||
         UnderFraction(*matchers.fraction_per_million))) {
      return i;
    }
  }
  return absl::nullopt;
}

Done.

---

src/core/ext/xds/xds_api.cc, line 2329 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think we still want this env var guard until all of the server-side pieces are in place.

Done.

---

src/core/ext/xds/xds_routing.h, line 38 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Instead of making this a template, how about defining an abstract interface for the virtual host object that this acts on:

class VirtualHostListIterator {
 public:
  virtual ~VirtualHostListIterator() = default;
  // Returns the number of virtual hosts in the list.
  virtual size_t Size() const = 0;
  // Returns the domain list for the virtual host at the specified index.
  virtual const std::set<std::string>& GetDomainsForVirtualHost(size_t index) const = 0;
};

// Returns the index of the selected virtual host in the list.
static absl::optional<size_t> FindVirtualHostForDomain(
    VirtualHostListIterator* vhost_iterator,
    absl::string_view domain) {
  absl::optional<size_t> target_index;
  for (size_t i = 0; i < vhost_iterator->Size(); ++i) {
    const std::set<std::string> domains = vhost_iterator->GetDomainsForVirtualHost(i);
    for (const std::string& domain_pattern : domains) {
      // ...update target_index instead of target_vhost...
    }
  }  
  return target_index;
}

Then callers can just wrap their lists of virtual hosts of whatever type in their own implementation of VirtualHostListIterator.

Templates seem more compact here since the types can already be iterated on. Suggestion taken anyway since the abstraction seems nice to have.

---

src/core/ext/xds/xds_routing.h, line 38 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

How about passing in virtual_hosts as a reference instead of a pointer, given that it should never be null?

(No longer relevant if you take my previous suggestion.)

outdated

---

src/core/ext/xds/xds_routing.h, line 99 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

All of this private stuff can be in an anonymous namespace in the .cc file, since it's all static anyway.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 50 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

There are a lot of classes here that interact with each other in complex ways, and it makes the code hard to understand for someone who is first approaching it. I have a couple of suggestions to try to help future readers understand the code better.

First, where possible, I think it would make this code more readable to use forward declarations to move class definitions out of the enclosing class, like this:

class XdsServerConfigFetcher : public grpc_server_config_fetcher {
 public:
  class FilterChainMatchManager;  // Forward declaration.
  // ...other members...
};

class XdsServerConfigFetcher::FilterChainMatchManager
    : public grpc_server_config_fetcher::ConnectionManager {
 public:
  // ...members...
};

This makes it easier for the reader to see each class by itself, without having to understand each nested class at the same time.

Second, please add a comment before each class explaining what it is and how it interacts with other classes. For example, for FilterChainMatchManager, you can say something like this:

"""
A connection manager used by the server listener code to inject channel args to be used for each incoming connection. This implementation chooses the appropriate filter chain from the xDS Listener resource and injects channel args that configure the right mTLS certs and cause the right set of HTTP filters to be injected.
"""

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 101 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Now that we're not exposing an L7-level watcher API as a part of the server config fetcher API (i.e., it's now invisible to all code except the code in this file), I don't think we actually need this to be an interface anymore. At this point, it looks like the only users of this interface are ListenerWatcher and XdsServerConfigSelectorProvider, and those classes are effectively part of the implementation of XdsServerConfigFetcher, so I don't think we actually need an abstraction between them anymore.

Instead, I think we should just allow ListenerWatcher and XdsServerConfigSelectorProvider to directly modify the internal state of XdsServerConfigFetcher. Note that ListenerWatcher is already nested within XdsServerConfigFetcher, so it can already do that; we'd just need to move XdsServerConfigSelectorProvider inside of XdsServerConfigFetcher as well, so that it can do the same thing.

I think that the RdsUpdateWatcherState struct can wind up looking something like this:

  struct RdsUpdateWatcherState {
    RouteConfigWatcher* route_config_watcher = nullptr;
    absl::optional<absl::StatusOr<XdsApi::RdsUpdate>> rds_update;
    // The set of providers that need to be notified when this RDS resource gets updated.
    // Providers will be added here when they are created, and they will automatically
    // remove themselves when they are destroyed.
    // When this becomes empty, the xDS watch can be stopped.
    std::set<XdsServerConfigSelectorProvider*> providers;
  };

I think the code will wind up being much simpler that way, with a lot less bookkeeping.

Hmmm... Alternatively, remind me why we don't just have each XdsServerConfigSelectorProvider start its own RDS watch on the XdsClient directly? That would use the existing watching mechanism instead of building a new one here, which might be even simpler.

Outdated. Using newer approach as discussed offline.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 192 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need for explicit.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 220 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Instead of hopping into the ExecCtx before we call back into the XdsClient, I suggest that we do the reverse: change ListenerWatcher and RouteConfigWatcher to hop into the ExecCtx before delivering updates from the XdsClient to us. That way, if we need to call back into the XdsClient (e.g., to start an RDS watch when we get an LDS update), we know that it's safe to do so.

I think this approach has a number of advantages:

• It's consistent with how we do things on the client side. In the xds resolver, every notification from an LDS or RDS watcher creates a fire-and-forget Notifier object, which takes a ref to the XdsResolver and then hops into the ExecCtx and then the WorkSerializer to deliver the notification. We could do essentially the same thing here, except that the notifier would hold a ref to the XdsServerConfigFetcher instead of the XdsResolver, and it would hop into the ExecCtx but not the WorkSerializer.
• It's simpler to implement, because you can use one notifier class for all notifications from either resource type. You would not need separate implementations for RouteConfigWatchStarter and RdsWatchCanceller. (And minimizing the number of classes here would help make this code more understandable.)
• It minimizes the amount of code run while holding the XdsClient's internal lock, which is probably better for lock contention.

Outdated. Using the newer WorkSerializer as discussed.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 271 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please move this inside of XdsServerConfigSelectorProvider (in the private section), since it's not needed outside of that class.

Outdated. XdsServerConfigSelectorProvider is no longer a thing. XdsServerConfigSelector is now used by the two independent classes StaticXdsServerConfigSelectorProvider and DyanmicXdsServerConfigSelectorProvider....

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 302 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This looks remarkably similar to the client-side code in the XdsResolver::XdsConfigSelector ctor. Can we refactor any of this into the XdsRouting API?

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 303 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can just say:

config_selector->virtual_hosts_.emplace_back();

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 307 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same here:

virtual_host.routes.emplace_back();

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 401 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

As mentioned elsewhere, it would be nice if this route-selection logic was in the XdsRouting API.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 435 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should be nested inside of XdsServerConfigFetcher.

outdated

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 469 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This and both of its subclasses should be nested inside of XdsServerConfigFetcher.

Moved these under FilterChainMatchManager since that's where all these are used. The nesting is getting out of hand though. I'm not sure this nesting is buying us much.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 526 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This seems a little ugly. Is there a way that we can restructure this such that this is not needed, such as maybe storing the resource in each subclass instead of in the base class? Or maybe if we eliminate RdsUpdateWatcherInterface as I suggested above, there will no longer be a need to have a common base class for the static and dynamic providers.

If we keep this, it should probably be protected, since it's only needed by a subclass.

Removed the parent class

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 890 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

std::move(resource_name)

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 896 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This would be much simpler if pending_rds_updates_ was std::set<> instead of std::vector<>.

outdated

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 918 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please remove.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 966 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think we want to do that. We want the LdsUpdate struct to be close to the structure of the underlying Listener proto, so that it's easy for people to understand where it came from when they see it in logs.

I think it's fine to do the conversion here.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1094 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I generally find code more readable when I avoid ReleaseableMutexLock. I suggest moving this check into Update(), which would be renamed to MaybeUpdate():

// Returns false if all refs have already been dropped.
bool XdsServerConfigFetcher::RouteConfigWatcher::MaybeUpdate(
    absl::StatusOr<XdsApi::RdsUpdate> rds_update) {
  MutexLock lock(&server_config_fetcher_->rds_mu_);
  auto it = server_config_fetcher_->rds_watchers_.find(resource_name_);
  if (it == server_config_fetcher_->rds_watchers_.end()) return false;
  // ...do update...
  return true;
}

Then the code here can just do this:

if (!MaybeUpdate(std::move(route_config))) {
  // All listener refs have already been dropped. Cancel the watch.
  server_config_fetcher_->xds_client_->CancelRouteConfigDataWatch(
      resource_name_, this, false);
}

We can use the same pattern in OnError() and OnResourceDoesNotExist(), so this approach winds up avoiding some code duplication as well.

outdated

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1177 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can just be called it.

outdated

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1272 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can just be called it.

outdated

---

src/core/lib/surface/server.h, line 468 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Isn't there still one clear external owner of this? If so, it probably wants to be InternallyRefCounted<> instead of RefCounted<>.

outdated

---

src/core/lib/surface/server.cc, line 1608 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

If the config fetcher is going to be ref-counted now, then I don't think it's okay to just destroy it here; instead, we should unref it (or orphan it, if you take my suggestion elsewhere to make it InternallyRefCounted<>).

outdated

[yashykt]

Reviewable status: 0 of 27 files reviewed, 27 unresolved discussions (waiting on @markdroth)

---

src/core/ext/xds/xds_client.cc, line 870 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's merge #27860 before this PR, so we can get the right fix in place here.

Done.

[markdroth]

This is looking really good! Remaining comments are mostly minor things, no significant structural concerns.

Please let me know if you have any questions. Thanks!

Reviewed 1 of 21 files at r5, 2 of 7 files at r6, 22 of 24 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 44 unresolved discussions (waiting on @yashykt)

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 334 at r7 (raw file):

    : public XdsRouting::RouteListIterator {
 public:
  RouteListIterator(const RouteTable& route_table)

explicit

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 334 at r7 (raw file):

    : public XdsRouting::RouteListIterator {
 public:
  RouteListIterator(const RouteTable& route_table)

Suggest making this parameter and the corresponding data member be pointers instead of references, so emphasize that there is external ownership.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 565 at r7 (raw file):

      RouteListIterator(route_table_), StringViewFromSlice(*args.path),
      args.initial_metadata);
  if (route_index.has_value()) {

Suggest handling the error case first, to keep the error condition and the error result next to each other, and to avoid unnecessary indentation for the rest of the function:

if (!route_index.has_value()) return CallConfig();

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 566 at r7 (raw file):

      args.initial_metadata);
  if (route_index.has_value()) {
    auto& entry = route_table_[route_index.value()];

Can say *route_index here.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 748 at r7 (raw file):

namespace {
// TODO(): Should this be moved to xds_api.h?

No, I think this is the right place for it. If we ever need to use it somewhere else, we can consider moving it at that point.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 751 at r7 (raw file):

class VirtualHostListIterator : public XdsRouting::VirtualHostListIterator {
 public:
  VirtualHostListIterator(

explicit

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 752 at r7 (raw file):

 public:
  VirtualHostListIterator(
      const std::vector<XdsApi::RdsUpdate::VirtualHost>& virtual_hosts)

Suggest making this parameter and the corresponding data member be a pointer instead of a reference, to emphasize that it's externally owned.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 785 at r7 (raw file):

  // Save the virtual host in the resolver.
  current_virtual_host_ =
      std::move(rds_update.virtual_hosts[vhost_index.value()]);

Can use *vhost_index here.

---

src/core/ext/xds/xds_routing.h, line 38 at r1 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Templates seem more compact here since the types can already be iterated on. Suggestion taken anyway since the abstraction seems nice to have.

Thanks, this looks good.

The main reason I suggested this is that we've been getting a lot of user input lately about our binary size going up fairly quickly. The more templated code we have, the more we multiply our binary size (because we generate the same code once for each unique template expansion). Using this iterator interface avoids that problem.

---

src/core/ext/xds/xds_routing.h, line 81 at r7 (raw file):

  struct GeneratePerHttpFilterConfigsResult {
    std::map<std::string, std::vector<std::string>>
        per_filter_configs;  // Map of field name to list of elements for that

Suggest moving these per-field comments to the line above the field they describe, since that should be more readable.

---

src/core/ext/xds/xds_routing.cc, line 98 at r7 (raw file):

}  // namespace

absl::optional<size_t> XdsRouting::GetRouteForRequest(

Please define these methods in the same order in which they are defined in the .h file, and put the helper functions used by each one right before the method it's used by.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 73 at r7 (raw file):

  struct WatcherState {
    std::string listening_address;

This field also exists in the object pointed to by listener_watcher. Do we need it in both places?

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 85 at r7 (raw file):

// A watcher implementation for listening on LDS updates from the xDS control
// plane. When a good LDS update is received, it creates FilterChainMatchManager

s/FilterChainMatchManager objects/a FilterChainMatchManager object/

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 87 at r7 (raw file):

// plane. When a good LDS update is received, it creates FilterChainMatchManager
// objects that would replace the existing (if any) FilterChainMatchManager
// object after all referred RDS resources are fetched. Note that a good update

s/referred/referenced/

Same thing throughout all of the comments in this file.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 129 at r7 (raw file):

  RefCountedPtr<FilterChainMatchManager> pending_filter_chain_match_manager_
      ABSL_GUARDED_BY(mu_);
  std::vector<std::string> pending_rds_updates_ ABSL_GUARDED_BY(mu_);

Doesn't look like this field is actually used anywhere, so I think it can be removed.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 186 at r7 (raw file):

  void UpdateRouteConfig(std::string resource_name,
                         absl::StatusOr<XdsApi::RdsUpdate> rds_update,
                         bool prefer_existing_update);

This parameter isn't very intuitive. Instead, I suggest splitting this method into three parts, one corresponding to each of the methods of the RouteConfigWatcher.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 190 at r7 (raw file):

  RefCountedPtr<XdsClient> xds_client_;
  RefCountedPtr<ListenerWatcher>
      listener_watcher_;  // This ref is only kept around till the

Suggest moving this comment to the line before the field, for better formatting.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 196 at r7 (raw file):

      default_filter_chain_;
  Mutex mu_;
  size_t rds_resources_yet_to_fetch_ = 0;

Are these two fields also guarded by the mutex?

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 223 at r7 (raw file):

  void OnError(grpc_error_handle error) override {
    Update(grpc_error_to_absl_status(error), true /* prefer_existing_update */);

Need to unref error before returning.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 232 at r7 (raw file):

 private:
  void Update(absl::StatusOr<XdsApi::RdsUpdate> rds_update,

This method doesn't seem that useful, since it's basically just delegating to another method. Why not just call the other method directly from the caller of this method?

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 234 at r7 (raw file):

  void Update(absl::StatusOr<XdsApi::RdsUpdate> rds_update,
              bool prefer_existing_update) {
    filter_chain_match_manager_->UpdateRouteConfig(resource_name_, rds_update,

std::move(rds_update)

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 277 at r7 (raw file):

    VirtualHostListIterator : public XdsRouting::VirtualHostListIterator {
 public:
  VirtualHostListIterator(const std::vector<VirtualHost>& virtual_hosts)

explicit

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 277 at r7 (raw file):

    VirtualHostListIterator : public XdsRouting::VirtualHostListIterator {
 public:
  VirtualHostListIterator(const std::vector<VirtualHost>& virtual_hosts)

Consider making this parameter and the corresponding data member be pointers instead of references, to emphasize the external ownership.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 294 at r7 (raw file):

    VirtualHost::RouteListIterator : public XdsRouting::RouteListIterator {
 public:
  RouteListIterator(const std::vector<Route>& routes) : routes_(routes) {}

explicit

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 294 at r7 (raw file):

    VirtualHost::RouteListIterator : public XdsRouting::RouteListIterator {
 public:
  RouteListIterator(const std::vector<Route>& routes) : routes_(routes) {}

Consider making this parameter and the corresponding data member be pointers instead of references, to emphasize the external ownership.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 379 at r7 (raw file):

    : public XdsClient::RouteConfigWatcherInterface {
 public:
  explicit RouteConfigWatcher(DynamicXdsServerConfigSelectorProvider* parent)

Doesn't this need to hold a ref on its parent?

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 386 at r7 (raw file):

  }
  void OnError(grpc_error_handle error) override {
    Update(grpc_error_to_absl_status(error), true /* prefer_existing_update */);

Need to unref error before returning.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 394 at r7 (raw file):

 private:
  void Update(absl::StatusOr<XdsApi::RdsUpdate> rds_update,

Combining the logic of all three methods like this makes it fairly hard to understand the control flow. I think it would make more sense to just inline them:

void OnRouteConfigChanged(XdsApi::RdsUpdate route_config) override {
  if (parent_->watcher_ == nullptr) return;
  parent_->watcher_->OnServerConfigSelectorUpdate(
      XdsServerConfigSelector::Create(std::move(route_config),
                                      parent_->http_filters_));
}

void OnError(grpc_error_handle error) override {
  if (parent_->watcher_ == nullptr) return;
  if (parent_->resource_.ok()) return;  // Prefer existing update, if any.
  parent_->watcher_->OnServerConfigSelectorUpdate(
      grpc_error_to_absl_status(error));
  GRPC_ERROR_UNREF(error);
}

void OnResourceDoesNotExist() override {
  if (parent_->watcher_ == nullptr) return;
  parent_->watcher_->OnServerConfigSelectorUpdate(
      absl::NotFoundError("Requested route config does not exist"));
}

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 619 at r7 (raw file):

  std::set<std::string> resource_names;
  for (const auto& destination_ip : filter_chain_map.destination_ip_vector) {
    for (int source_type = 0; source_type < 3; ++source_type) {

Instead of hard-coding 3, please use destination_ip.source_types_array.size().

Or better yet, I think you can just say:

for (const auto& source_type : destination_ip.source_types_array) {

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 277 at r8 (raw file):

};

class XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager::

The VirtualHostListIterator class is simple enough that it can be inlined in XdsServerConfigSelector.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 295 at r8 (raw file):

};

class XdsServerConfigFetcher::ListenerWatcher::FilterChainMatchManager::

The RouteListIterator class is simple enough that it can be inlined in XdsServerConfigSelector::VirtualHost.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 481 at r8 (raw file):

void XdsServerConfigFetcher::ListenerWatcher::OnListenerChanged(
    XdsApi::LdsUpdate listener) {
  {

This scope encompasses the whole body of the function, so I don't think it's needed.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 544 at r8 (raw file):

  pending_filter_chain_match_manager_.reset();
  gpr_log(GPR_ERROR,
          "ListenerWatcher:%p Encountered fatal error %s; not serving on %s",

Should we log this only if there's no serving status notifier?

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 565 at r8 (raw file):

}

bool IsLoopbackIp(const grpc_resolved_address* address) {

Suggest moving this down to right above FindFilterChainDataForSourceType(), since that's where this is actually used.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 653 at r8 (raw file):

        default_filter_chain_->http_connection_manager.route_config_name);
  }
  bool lds_resource_ready = false;

I don't think this variable is actually needed. Instead, after releasing the lock, just check whether listener_watcher is null, since we always std::move() it away if lds_resource_ready is not set.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 760 at r8 (raw file):

                      bool prefer_existing_update) {
  RefCountedPtr<ListenerWatcher> listener_watcher;
  bool lds_resource_ready =

I don't think this variable is needed; instead, after releasing the lock, we can just check whether listener_watcher is non-null.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 982 at r8 (raw file):

    grpc_arg args_to_add[] = {server_config_selector_provider->MakeChannelArg(),
                              channel_stack_modifier->MakeChannelArg()};
    args = grpc_channel_args_copy_and_add(old_args, args_to_add, 2);

We've constructing new args both here and then again below to add the certificate provider. Instead, how about combining these, like this:

absl::InlinedVector<grpc_arg, 3> args_to_add;
if (XdsRbacEnabled()) {
  // ...construct server_config_selector_provider...
  args_to_add.emplace_back(server_config_selector_provider->MakeChannelArg());
  // ...construct channel_stack_modifier...
  args_to_add.emplace_back(channel_stack_modifier->MakeChannelArg());
}
// ...construct certificate provider...
args_to_add.emplace_back(xds_certificate_provider->MakeChannelArg());
// Modify args if needed.
if (!args_to_add.empty()) {
  grpc_channel_args* updated_args =
      grpc_channel_args_copy_and_add(args, args_to_add.data(), args_to_add.size());
  grpc_channel_args_destroy(args);
  args = updated_args;
}
return args;

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1086 at r8 (raw file):

      VirtualHostListIterator(virtual_hosts_), authority);
  if (!vhost_index.has_value()) {
    call_config.error = GRPC_ERROR_CREATE_FROM_CPP_STRING(

This error should set the status to UNAVAILABLE, since that will wind up being the status code for the RPC.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1098 at r8 (raw file):

    // Found the matching route
    if (route.inappropriate_action) {
      call_config.error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(

This should also set the status code to UNAVAILABLE.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1099 at r8 (raw file):

    if (route.inappropriate_action) {
      call_config.error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
          "Matching route has inappropriate_action");

This should probably say "unsupported action" instead of "inappropriate_action".

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1109 at r8 (raw file):

    return call_config;
  }
  call_config.error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("No route matched");

This should also set the status code to UNAVAILABLE.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 731 at r8 (raw file):

        ->mutable_socket_address()
        ->set_address(ipv6_only_ ? "::1" : "127.0.0.1");
    *http_connection_manager.mutable_route_config() =

I don't think we should set this here (nor should it be necessary), because we don't want to inline the route config in the listener resource by default. Instead, tests should use SetListenerAndRouteConfiguration() to dynamically determine whether to inline the route config in the listener or create a separate RDS resource, based on the TestType attribute.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9789 at r8 (raw file):

      0, default_server_listener_, backends_[0]->port(), route_config);
  backends_[0]->Start();
  if (GetParam().enable_rds_testing()) {

Since this is such a common pattern, I suggest adding a helper method, similar to the existing RouteConfigurationResponseState() method:

bool WaitForRouteConfigNack(StatusCode expected_status = StatusCode::UNAVAILABLE) {
  if (GetParam().enable_rds_testing()) return WaitForRdsNack(expected_status);
  return WaitForLdsNack(expected_status);
}

Then the code in the individual tests can become:

ASSERT_TRUE(WaitForRouteConfigNack(StatusCode::DEADLINE_EXCEEDED))
    << "timed out waiting for NACK";
EXPECT_THAT(RouteConfigurationResponseState(0).error_message,
            ::testing::HasSubstr("Invalid domain pattern \"\""));

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9973 at r8 (raw file):

      absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
      grpc::StatusCode::OK);
  SendRpc([this]() { return CreateInsecureChannel(); }, {}, {});

Is there a reasonable way to test that different filter chains (and therefore different route configs) are selected for different incoming connections?

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 12534 at r8 (raw file):

// We are only testing the server here.
// TODO(yashykt): Also add a test type with set_enable_rds_testing() once we

This TODO can go away now.

[yashykt]

Reviewable status: 22 of 27 files reviewed, 44 unresolved discussions (waiting on @markdroth and @yashykt)

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 334 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

explicit

Done.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 334 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest making this parameter and the corresponding data member be pointers instead of references, so emphasize that there is external ownership.

Done.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 565 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest handling the error case first, to keep the error condition and the error result next to each other, and to avoid unnecessary indentation for the rest of the function:

if (!route_index.has_value()) return CallConfig();

Done.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 566 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Can say *route_index here.

Done.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 748 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No, I think this is the right place for it. If we ever need to use it somewhere else, we can consider moving it at that point.

Done.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 751 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

explicit

Done.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 752 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest making this parameter and the corresponding data member be a pointer instead of a reference, to emphasize that it's externally owned.

Done.

---

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 785 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Can use *vhost_index here.

Done.

---

src/core/ext/xds/xds_routing.h, line 81 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest moving these per-field comments to the line above the field they describe, since that should be more readable.

Done.

---

src/core/ext/xds/xds_routing.cc, line 98 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please define these methods in the same order in which they are defined in the .h file, and put the helper functions used by each one right before the method it's used by.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 73 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This field also exists in the object pointed to by listener_watcher. Do we need it in both places?

Just saves some cpu cycles since the name we actually use for the watch is derived from the address and the template found in the bootstrap file, but given that CancelWatch will only be invoked when the server is shutdown, I think it's fine to repeat that modification.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 85 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

s/FilterChainMatchManager objects/a FilterChainMatchManager object/

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 87 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

s/referred/referenced/

Same thing throughout all of the comments in this file.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 129 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Doesn't look like this field is actually used anywhere, so I think it can be removed.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 190 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest moving this comment to the line before the field, for better formatting.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 196 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Are these two fields also guarded by the mutex?

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 223 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Need to unref error before returning.

Done. Thanks!

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 232 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This method doesn't seem that useful, since it's basically just delegating to another method. Why not just call the other method directly from the caller of this method?

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 234 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

std::move(rds_update)

outdated

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 277 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

explicit

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 277 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Consider making this parameter and the corresponding data member be pointers instead of references, to emphasize the external ownership.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 294 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Consider making this parameter and the corresponding data member be pointers instead of references, to emphasize the external ownership.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 379 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Doesn't this need to hold a ref on its parent?

You're right. Now that the callbacks are invoked in a WorkSerializer, the updates can be delayed

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 386 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Need to unref error before returning.

Thanks! Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 394 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Combining the logic of all three methods like this makes it fairly hard to understand the control flow. I think it would make more sense to just inline them:

void OnRouteConfigChanged(XdsApi::RdsUpdate route_config) override {
  if (parent_->watcher_ == nullptr) return;
  parent_->watcher_->OnServerConfigSelectorUpdate(
      XdsServerConfigSelector::Create(std::move(route_config),
                                      parent_->http_filters_));
}

void OnError(grpc_error_handle error) override {
  if (parent_->watcher_ == nullptr) return;
  if (parent_->resource_.ok()) return;  // Prefer existing update, if any.
  parent_->watcher_->OnServerConfigSelectorUpdate(
      grpc_error_to_absl_status(error));
  GRPC_ERROR_UNREF(error);
}

void OnResourceDoesNotExist() override {
  if (parent_->watcher_ == nullptr) return;
  parent_->watcher_->OnServerConfigSelectorUpdate(
      absl::NotFoundError("Requested route config does not exist"));
}

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 619 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Instead of hard-coding 3, please use destination_ip.source_types_array.size().

Or better yet, I think you can just say:

for (const auto& source_type : destination_ip.source_types_array) {

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 277 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

The VirtualHostListIterator class is simple enough that it can be inlined in XdsServerConfigSelector.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 481 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This scope encompasses the whole body of the function, so I don't think it's needed.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 544 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Should we log this only if there's no serving status notifier?

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 565 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest moving this down to right above FindFilterChainDataForSourceType(), since that's where this is actually used.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 653 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think this variable is actually needed. Instead, after releasing the lock, just check whether listener_watcher is null, since we always std::move() it away if lds_resource_ready is not set.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 760 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think this variable is needed; instead, after releasing the lock, we can just check whether listener_watcher is non-null.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 982 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

We've constructing new args both here and then again below to add the certificate provider. Instead, how about combining these, like this:

absl::InlinedVector<grpc_arg, 3> args_to_add;
if (XdsRbacEnabled()) {
  // ...construct server_config_selector_provider...
  args_to_add.emplace_back(server_config_selector_provider->MakeChannelArg());
  // ...construct channel_stack_modifier...
  args_to_add.emplace_back(channel_stack_modifier->MakeChannelArg());
}
// ...construct certificate provider...
args_to_add.emplace_back(xds_certificate_provider->MakeChannelArg());
// Modify args if needed.
if (!args_to_add.empty()) {
  grpc_channel_args* updated_args =
      grpc_channel_args_copy_and_add(args, args_to_add.data(), args_to_add.size());
  grpc_channel_args_destroy(args);
  args = updated_args;
}
return args;

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1086 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This error should set the status to UNAVAILABLE, since that will wind up being the status code for the RPC.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1098 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should also set the status code to UNAVAILABLE.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1099 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should probably say "unsupported action" instead of "inappropriate_action".

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 1109 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should also set the status code to UNAVAILABLE.

Done.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 731 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't think we should set this here (nor should it be necessary), because we don't want to inline the route config in the listener resource by default. Instead, tests should use SetListenerAndRouteConfiguration() to dynamically determine whether to inline the route config in the listener or create a separate RDS resource, based on the TestType attribute.

Done.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9789 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Since this is such a common pattern, I suggest adding a helper method, similar to the existing RouteConfigurationResponseState() method:

bool WaitForRouteConfigNack(StatusCode expected_status = StatusCode::UNAVAILABLE) {
  if (GetParam().enable_rds_testing()) return WaitForRdsNack(expected_status);
  return WaitForLdsNack(expected_status);
}

Then the code in the individual tests can become:

ASSERT_TRUE(WaitForRouteConfigNack(StatusCode::DEADLINE_EXCEEDED))
    << "timed out waiting for NACK";
EXPECT_THAT(RouteConfigurationResponseState(0).error_message,
            ::testing::HasSubstr("Invalid domain pattern \"\""));

Done.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9973 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Is there a reasonable way to test that different filter chains (and therefore different route configs) are selected for different incoming connections?

Hmm, so the properties that we can vary in the filter chain match are

    // - destination port (never matched, so not present in map)
    // - destination IP address
    // - server name (never matched, so not present in map)
    // - transport protocol (allows only "raw_buffer" or unset, prefers the
    //   former, so only one of those two types is present in map)
    // - application protocol (never matched, so not present in map)
    // - connection source type (any, local or external)
    // - source IP address
    // - source port

From these, the source port seems to be the only thing that we can vary for connections on localhost. If we could tell the channel to bind to a particular port, we could do it..

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 12534 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This TODO can go away now.

Done.

[yashykt]

Reviewable status: 22 of 27 files reviewed, 44 unresolved discussions (waiting on @markdroth)

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 186 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This parameter isn't very intuitive. Instead, I suggest splitting this method into three parts, one corresponding to each of the methods of the RouteConfigWatcher.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 294 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

explicit

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 295 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

The RouteListIterator class is simple enough that it can be inlined in XdsServerConfigSelector::VirtualHost.

Done.

[markdroth]

This is looking very good! Only a few comments left.

Please let me know if you have any questions. Thanks!

Reviewed 4 of 5 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @yashykt)

---

src/core/ext/xds/xds_routing.cc, line 98 at r7 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Done.

Please also move HeadersMatch() and UnderFraction() to their own anonymous namespace that is located between FindVirtualHostForDomain() and GetRouteForRequest(), since those helper functions are only used by the latter method.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 73 at r7 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Just saves some cpu cycles since the name we actually use for the watch is derived from the address and the template found in the bootstrap file, but given that CancelWatch will only be invoked when the server is shutdown, I think it's fine to repeat that modification.

Re-expanding the template seems wrong here. The value that you're storing in ListenerWatcher::listening_address_ is actually the expanded template already. I think re-expanding it will yield the wrong value.

Also, given that it's actually storing the expanded resource name, we should probably change the name of this field from listening_address to resource_name.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 186 at r7 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Done.

To be clear, I didn't actually mean to move the code into the methods of RouteConfigWatcher; I just meant that there should be one method here corresponding to each of those methods. I think the methods of RouteConfigWatcher should just delegate to these methods, since they really belong here.

RouteConfigWatcher should be simple enough that you can inline it if you want. The other methods' definitions are long enough that they should be moved below.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 294 at r7 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Done.

I don't see this change.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 399 at r9 (raw file):

    // Prefer existing update, if any.
    if (parent_->resource_.ok()) {
      return;

Need to unref the error before returning here.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 113 at r10 (raw file):

  // pending_filter_chain_match_manager_, it is promoted to be the
  // filter_chain_match_manager_ in use.
  void PendingFilterChainMatchManagerReady(

Might be useful to rename this to PendingFilterChainMatchManagerReadyLocked() and add a wrapper version without the Locked() suffix that grabs the lock for the caller. That way, we won't need to grab the lock externally.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9973 at r8 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Hmm, so the properties that we can vary in the filter chain match are

    // - destination port (never matched, so not present in map)
    // - destination IP address
    // - server name (never matched, so not present in map)
    // - transport protocol (allows only "raw_buffer" or unset, prefers the
    //   former, so only one of those two types is present in map)
    // - application protocol (never matched, so not present in map)
    // - connection source type (any, local or external)
    // - source IP address
    // - source port

From these, the source port seems to be the only thing that we can vary for connections on localhost. If we could tell the channel to bind to a particular port, we could do it..

That's unfortunate. For now, please add a TODO that we should add such a test once #24035 is resolved.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9772 at r10 (raw file):

  }

  bool WaitForRouteConfigNack(

This should move into XdsEnd2endTest, right above the existing RouteConfigurationResponseState() method. It could be useful in client-side tests too.

[yashykt]

Reviewable status: 24 of 27 files reviewed, 8 unresolved discussions (waiting on @markdroth)

---

src/core/ext/xds/xds_routing.cc, line 98 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please also move HeadersMatch() and UnderFraction() to their own anonymous namespace that is located between FindVirtualHostForDomain() and GetRouteForRequest(), since those helper functions are only used by the latter method.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 73 at r7 (raw file):

Re-expanding the template seems wrong here. The value that you're storing in ListenerWatcher::listening_address_ is actually the expanded template already. I think re-expanding it will yield the wrong value.

No, it's not.

  auto listener_watcher = MakeRefCounted<ListenerWatcher>(
      xds_client_, std::move(watcher), serving_status_notifier_,
      listening_address);
  auto* listener_watcher_ptr = listener_watcher.get();
  listening_address = absl::StrReplaceAll(
      xds_client_->bootstrap().server_listener_resource_name_template(),
      {{"%s", listening_address}});
  xds_client_->WatchListenerData(listening_address,
                                 std::move(listener_watcher));

We store the address before using the template.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 186 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

To be clear, I didn't actually mean to move the code into the methods of RouteConfigWatcher; I just meant that there should be one method here corresponding to each of those methods. I think the methods of RouteConfigWatcher should just delegate to these methods, since they really belong here.

RouteConfigWatcher should be simple enough that you can inline it if you want. The other methods' definitions are long enough that they should be moved below.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 294 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I don't see this change.

This time for sure.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 399 at r9 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Need to unref the error before returning here.

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 113 at r10 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Might be useful to rename this to PendingFilterChainMatchManagerReadyLocked() and add a wrapper version without the Locked() suffix that grabs the lock for the caller. That way, we won't need to grab the lock externally.

Done.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9973 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

That's unfortunate. For now, please add a TODO that we should add such a test once #24035 is resolved.

Done.

---

test/cpp/end2end/xds/xds_end2end_test.cc, line 9772 at r10 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should move into XdsEnd2endTest, right above the existing RouteConfigurationResponseState() method. It could be useful in client-side tests too.

Done.

[markdroth]

This looks excellent! Just a couple of minor nits remaining.

Please let me know if you have any questions. Thanks!

Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yashykt)

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 73 at r7 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Re-expanding the template seems wrong here. The value that you're storing in ListenerWatcher::listening_address_ is actually the expanded template already. I think re-expanding it will yield the wrong value.

No, it's not.

  auto listener_watcher = MakeRefCounted<ListenerWatcher>(
      xds_client_, std::move(watcher), serving_status_notifier_,
      listening_address);
  auto* listener_watcher_ptr = listener_watcher.get();
  listening_address = absl::StrReplaceAll(
      xds_client_->bootstrap().server_listener_resource_name_template(),
      {{"%s", listening_address}});
  xds_client_->WatchListenerData(listening_address,
                                 std::move(listener_watcher));

We store the address before using the template.

Ah, okay. What confused me is that you're storing the resource name in a variable called listening_address. Can you please change that code to either create a new string variable called resource_name, or just side-step the issue and pass the result of absl::StrReplaceAll() directly to WatchListenerData() without storing it in a local variable at all?

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 186 at r7 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Done.

You made this change in DynamicXdsServerConfigSelectorProvider, but not here in FilterChainMatchManager.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 592 at r11 (raw file):

void XdsServerConfigFetcher::ListenerWatcher::
    PendingFilterChainMatchManagerReady(

The definition of this method is short enough that it can be inlined in the class definition above if you'd like.

[yashykt]

Reviewable status: 26 of 27 files reviewed, 3 unresolved discussions (waiting on @markdroth)

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 73 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Ah, okay. What confused me is that you're storing the resource name in a variable called listening_address. Can you please change that code to either create a new string variable called resource_name, or just side-step the issue and pass the result of absl::StrReplaceAll() directly to WatchListenerData() without storing it in a local variable at all?

Done.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 186 at r7 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

You made this change in DynamicXdsServerConfigSelectorProvider, but not here in FilterChainMatchManager.

Ah, DynamicXdsServerConfigSelectorProvider is where I thought the original comment was. It's good to be consistent anyway, so the change is now in both places.

---

src/core/ext/xds/xds_server_config_fetcher.cc, line 592 at r11 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

The definition of this method is short enough that it can be inlined in the class definition above if you'd like.

Done.

[markdroth]

This looks fantastic!

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yashykt)

[yashykt]

Known issues: #26886
Thanks for reviewing!