Update RDS parsing for use on servers

[yashykt]

Broken off from #27388
Changes -

• NonForwardingAction is now a supported action
• XdsAPI now parses RouteConfig on servers too, but it's unused at this moment. (Updates to follow)
• Inappropriate actions on a route are no longer NACKed, instead, RPCs that match to that route are failed with UNAVAILABLE.
• An empty list of HTTP filters is NACKed as dictated by A39 update: Require that the HTTP filter list be non-empty. proposal#266

---

This change is 

[markdroth]

Thanks for doing this!

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

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

---

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

              "Matching route has inappropriate action"),
          GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAVAILABLE);
      ;

Please remove.

---

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

  auto config_selector = MakeRefCounted<XdsConfigSelector>(Ref(), &error);
  if (error != GRPC_ERROR_NONE) {
    OnError(error);

This should also set status UNAVAILABLE.

---

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

    Matchers matchers;

    struct InappropriateAction {

Suggest calling this UnknownAction.

---

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

  } else if (absl::holds_alternative<XdsApi::Route::NonForwardingAction>(
                 action)) {
    contents.push_back("non_forwarding_action={}");

Let's also add something if it contains an UnknownAction, so it's clear from the log what's happening.

---

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

                                   XdsApi::Route::RouteAction* route,
                                   bool* ignore_route) {
  if (!envoy_config_route_v3_Route_has_route(route_msg)) {

This check can go away now, since it's being done in the caller.

---

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

          route_action.retry_policy = virtual_host_retry_policy;
        }
      } else if (envoy_config_route_v3_Route_has_non_forwarding_action(

If neither RouteAction nor NonForwardingAction are used, then I think we need to populate UnknownAction. Or does absl::variant<> default to containing the first alternative?

---

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

        envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_http_filters(
            http_connection_manager_proto, &num_filters);
    if (num_filters == 0) {

I think this is the wrong place for this check, because if the filter list contains exactly one filter but that filter is marked optional and is not actually supported, we'll still wind up with an empty list. I think this should be moved down to line 2302, after the loop where we process all filters, and it should check the length of the list of processed filters instead of the length of the list in the resource protobuf.

(This probably doesn't matter in practice, since the above case should be caught by the requirement that the final filter be terminal. But let's be a little conservative here, just in case something changes.)

---

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

    if (num_filters == 0) {
      return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
          "Expected atleast one HTTP filter");

s/atleast/at least/

---

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

            "router", {kXdsHttpRouterFilterConfigName, Json()}});
  }
  if (is_client) {

Doing this on the server side should probably be guarded with an env var until all of the pieces are in place.

---

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

constexpr char kServerName[] = "server.example.com";
constexpr char kDefaultRouteConfigurationName[] = "route_config_name";
constexpr char kSimpleServerRouteConfigurationName[] =

s/Simple/Default/

Same thing throughout (in variable names, strings, etc).

---

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

    route->mutable_match()->set_prefix("");
    route->mutable_non_forwarding_action();
    // Construct a simple HttpConnectionManager for tests to use.

Let's structure this the same way we do the client-side resources: store a default Listener proto instead of an HttpConnectionManager proto, and then have tests modify those defaults and use the existing SetListenerAndRouteConfiguration() and SetRouteConfiguration() methods to set the modified values in the xDS server. That way, we can test both with the route config inlined in the LDS response and when using RDS.

I've sent you #27756 with a required change to SetRouteConfiguration() to make this work.

You may also want to add a utility method to set the name of a server-side Listener resource for a particular backend index.

---

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

  auto* vhost = route_config.mutable_virtual_hosts(0);
  vhost->mutable_routes(0)->mutable_redirect();
  // Add another route that we are never going to match on

Why is this needed?

[yashykt]

Reviewable status: 1 of 5 files reviewed, 12 unresolved discussions (waiting on @markdroth)

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please remove.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This should also set status UNAVAILABLE.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Suggest calling this UnknownAction.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Let's also add something if it contains an UnknownAction, so it's clear from the log what's happening.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This check can go away now, since it's being done in the caller.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

If neither RouteAction nor NonForwardingAction are used, then I think we need to populate UnknownAction. Or does absl::variant<> default to containing the first alternative?

That's correct. It does default to the first alternative.

// `absl::variant` will hold the value of its first alternative type, provided
// it is default-constructible.```

https://github.com/abseil/abseil-cpp/blob/master/absl/types/variant.h

---

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

Previously, markdroth (Mark D. Roth) wrote…

I think this is the wrong place for this check, because if the filter list contains exactly one filter but that filter is marked optional and is not actually supported, we'll still wind up with an empty list. I think this should be moved down to line 2302, after the loop where we process all filters, and it should check the length of the list of processed filters instead of the length of the list in the resource protobuf.

(This probably doesn't matter in practice, since the above case should be caught by the requirement that the final filter be terminal. But let's be a little conservative here, just in case something changes.)

Oh, that's interesting! Can a filter be both terminal and optional? (Made the change.)

---

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

Previously, markdroth (Mark D. Roth) wrote…

s/atleast/at least/

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Doing this on the server side should probably be guarded with an env var until all of the pieces are in place.

Yeah, I think guarding it with the RBAC env var (as you suggested earlier) is good enough for now.

---

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

Previously, markdroth (Mark D. Roth) wrote…

s/Simple/Default/

Same thing throughout (in variable names, strings, etc).

Done for route configuration. I've kept the simple_server_http_connection_manager around though since it helps for the FilterChainMatchTest tests

---

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

Previously, markdroth (Mark D. Roth) wrote…

Let's structure this the same way we do the client-side resources: store a default Listener proto instead of an HttpConnectionManager proto, and then have tests modify those defaults and use the existing SetListenerAndRouteConfiguration() and SetRouteConfiguration() methods to set the modified values in the xDS server. That way, we can test both with the route config inlined in the LDS response and when using RDS.

I've sent you #27756 with a required change to SetRouteConfiguration() to make this work.

You may also want to add a utility method to set the name of a server-side Listener resource for a particular backend index.

Done. (Note that this PR didn't really add any RDS specific tests for the use of SetRouteConfiguration().)

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why is this needed?

There is a bit of a shorthand logic in the resolver code that fails early when there are no configured clusters. The test actually makes sure that we reach the point of trying to match a route.

[markdroth]

This is pretty close, just needs some test changes.

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

Reviewed 2 of 4 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yashykt)

---

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

Previously, yashykt (Yash Tibrewal) wrote…

Oh, that's interesting! Can a filter be both terminal and optional? (Made the change.)

Good question. I guess in principle, you could have the list end with two terminal optional filters, if you know that one of the two of them will always be present. In practice, this isn't an issue today, since the only terminal filter is the router filter. But I guess we should handle this kind of corner case, just to be safe.

Let's pull the IsTerminalFilter() checks out of the parsing loop and instead add a second loop that iterates over the list of processed filters that checks this. That way, we're enforcing that invariant in the right place.

---

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

Previously, yashykt (Yash Tibrewal) wrote…

Done. (Note that this PR didn't really add any RDS specific tests for the use of SetRouteConfiguration().)

I think we do need to add the RDS tests here. We should structure this the same way that we do for the client-side tests. I think we need the following changes to make this work:

First, move the code to populate the default_server_listener_ and default_server_route_configuration_ back up to here, where we're populating the default resources for the client side. However, when populating default_server_listener_, do not set the name field.

Next, add the following helper method in XdsEnd2endTest:

Listener PopulateServerListenerName(const Listener& listener_template, int port) {
  Listener listener = listener_template;
  default_server_listener_.set_name(absl::StrCat(
      "grpc/server?xds.resource.listening_address=",
      ipv6_only_ ? "[::1]:" : "127.0.0.1:", port));
  return listener;
}

Then move the loop that starts the backends up above the loop that starts the xDS servers, so that the backend ports are already determined when starting the xDS servers.

Finally, as we start each xDS server, do this:

for (const auto& backend : backends_) {
  SetListenerAndRouteConfiguration(
      i, PopulateServerListenerName(default_server_listener_, backend->port()),
      default_server_route_config_);
}

Any individual test that needs to change the Listener or RouteConfiguration resources should use the SetListenerAndRouteConfiguration() method to do so.

Once you've done that, then you can just instantiate the relevant server tests with TestType::set_enable_rds_testing(), and they'll run with RDS enabled.

---

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

Previously, yashykt (Yash Tibrewal) wrote…

There is a bit of a shorthand logic in the resolver code that fails early when there are no configured clusters. The test actually makes sure that we reach the point of trying to match a route.

Okay. In that case, suggest setting the prefix here to "" instead of "/NeverMatch", to make the intent clearer. And please add a comment explaining this.

---

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

  RouteConfiguration default_route_config_;
  Listener default_server_listener_;
  HttpConnectionManager simple_server_http_connection_manager_;

I don't think it makes sense to have this data member. This isn't a resource by itself; it's simply a field in the LDS resource, so it should already be populated in default_server_listener. If a test needs to access it, it should access it in default_server_listener_.

I'd be fine with adding a helper method to extract this field from inside a server-side Listener proto.

[yashykt]

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

---

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

Previously, markdroth (Mark D. Roth) wrote…

Good question. I guess in principle, you could have the list end with two terminal optional filters, if you know that one of the two of them will always be present. In practice, this isn't an issue today, since the only terminal filter is the router filter. But I guess we should handle this kind of corner case, just to be safe.

Let's pull the IsTerminalFilter() checks out of the parsing loop and instead add a second loop that iterates over the list of processed filters that checks this. That way, we're enforcing that invariant in the right place.

Done

---

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

Previously, markdroth (Mark D. Roth) wrote…

I think we do need to add the RDS tests here. We should structure this the same way that we do for the client-side tests. I think we need the following changes to make this work:

First, move the code to populate the default_server_listener_ and default_server_route_configuration_ back up to here, where we're populating the default resources for the client side. However, when populating default_server_listener_, do not set the name field.

Next, add the following helper method in XdsEnd2endTest:

Listener PopulateServerListenerName(const Listener& listener_template, int port) {
  Listener listener = listener_template;
  default_server_listener_.set_name(absl::StrCat(
      "grpc/server?xds.resource.listening_address=",
      ipv6_only_ ? "[::1]:" : "127.0.0.1:", port));
  return listener;
}

Then move the loop that starts the backends up above the loop that starts the xDS servers, so that the backend ports are already determined when starting the xDS servers.

Finally, as we start each xDS server, do this:

for (const auto& backend : backends_) {
  SetListenerAndRouteConfiguration(
      i, PopulateServerListenerName(default_server_listener_, backend->port()),
      default_server_route_config_);
}

Any individual test that needs to change the Listener or RouteConfiguration resources should use the SetListenerAndRouteConfiguration() method to do so.

Once you've done that, then you can just instantiate the relevant server tests with TestType::set_enable_rds_testing(), and they'll run with RDS enabled.

I like the suggestions here and I've incorporated the changes, but this PR doesn't make any changes to the xDS server config fetcher, so we don't yet have the machinery to properly test the RDS updates in this PR. Once this PR does get merged in, the follow-up PR will include tests from #27388 which would then actually test the RDS on the server-side (both inline and non-inline). I've added some basic RDS tests though to test that RouteConfig is actually parsed now on the servers which is the intent of this PR.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Okay. In that case, suggest setting the prefix here to "" instead of "/NeverMatch", to make the intent clearer. And please add a comment explaining this.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

I don't think it makes sense to have this data member. This isn't a resource by itself; it's simply a field in the LDS resource, so it should already be populated in default_server_listener. If a test needs to access it, it should access it in default_server_listener_.

I'd be fine with adding a helper method to extract this field from inside a server-side Listener proto.

Done.

[markdroth]

This looks really good, just a few minor comments remaining.

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

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

---

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

                            is_client ? "clients" : "servers"));
      }
      if (i < num_filters - 1) {

We can remove this if/else block now.

---

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

            "router", {kXdsHttpRouterFilterConfigName, Json()}});
  }
  if (http_connection_manager->http_filters.empty()) {

I think all of this new code can move up to line 2305. We don't need to do these checks in the v2 case, because we're hard-coding the list, and we know it's right.

---

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

        ->mutable_typed_config()
        ->PackFrom(http_connection_manager);
    // Create the backends so that ports are allocated. (This is needed to

Suggest wording this comment as follows:

"""
Create the backends but don't start them yet. We need to create the backends to allocate the ports, so that we know what resource names to populate in the xDS servers when we start them. However, we can't start the backends until after we've started the xDS servers, because in the tests that use xDS-enabled servers, the backends will try to contact the xDS servers as soon as they start up.
"""

---

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

  }

  Listener PopulateServerListenerNameAndPort(const Listener& listener_template,

Please move these two methods down to line 1384, so that they're next to the other related methods.

---

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

  }

  void SetServerListenerAndRouteConfiguration(

To avoid having two versions of SetListenerAndRouteConfiguration(), how about something like this:

// Interface for accessing HttpConnectionManager config in Listener.
class HcmAccessor {
 public:
  virtual ~HcmAccessor() = default;
  virtual HttpConnectionManager Unpack(const Listener& listener) const = 0;
  virtual void Pack(const HttpConnectionManager& hcm, Listener* listener) const = 0;
};

// Client-side impl.
class ClientHcmAccessor : public HcmAccessor {
 public:
  HttpConnectionManager Unpack(const Listener& listener) const override {
    HttpConnectionManager http_connection_manager;
    listener.api_listener().api_listener().UnpackTo(&http_connection_manager);
    return http_connection_manager;
  }
  void Pack(const HttpConnectionManager& hcm, Listener* listener) const override {
    auto* api_listener =
        listener.mutable_api_listener()->mutable_api_listener();
    api_listener->PackFrom(http_connection_manager);
  }
};

// Server-side impl.
class ServerHcmAccessor : public HcmAccessor {
 public:
  HttpConnectionManager Unpack(const Listener& listener) const override {
    HttpConnectionManager http_connection_manager;
    default_server_listener_.mutable_default_filter_chain()
        ->mutable_filters()
        ->at(0)
        .mutable_typed_config()
        ->UnpackTo(&http_connection_manager);
    return http_connection_manager;
  }
  void Pack(const HttpConnectionManager& hcm, Listener* listener) const override {
    listener.mutable_default_filter_chain()
        ->mutable_filters()
        ->at(0)
        .mutable_typed_config()
        ->PackFrom(http_connection_manager);
  }
};

void SetListenerAndRouteConfiguration(
    int idx, Listener listener, RouteConfiguration& route_config,
    const HcmAccessor& hcm_accessor = ClientHcmAccessor()) {
  HttpConnectionManager http_connection_manager = hcm_accessor.Unpack(listener);
  if (GetParam().enable_rds_testing()) {
    auto* rds = http_connection_manager.mutable_rds();
    rds->set_route_config_name(kDefaultRouteConfigurationName);
    rds->mutable_config_source()->mutable_ads();
    balancers_[idx]->ads_service()->SetRdsResource(route_config);
  } else {
    *http_connection_manager.mutable_route_config() = route_config;
  }
  hcm_accessor.Pack(http_connection_manager, &listener);
  balancers_[idx]->ads_service()->SetLdsResource(listener);
}

This approach would also eliminate the need for GetDefaultServerHttpConnectionManager().

---

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

class XdsServerRdsTest : public XdsServerSecurityTest {
 protected:
  XdsServerRdsTest() { gpr_setenv("GRPC_XDS_EXPERIMENTAL_RBAC", "true"); }

The setting and unsetting of the env vars can be done in SetUpTestSuite() and TearDownTestSuite().

---

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

// We are only testing the server here.
INSTANTIATE_TEST_SUITE_P(XdsTest, XdsServerRdsTest,

This should also run with TestType::set_enable_rds_testing().

[yashykt]

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

---

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

Previously, markdroth (Mark D. Roth) wrote…

We can remove this if/else block now.

Ah, Thanks!

---

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

Previously, markdroth (Mark D. Roth) wrote…

I think all of this new code can move up to line 2305. We don't need to do these checks in the v2 case, because we're hard-coding the list, and we know it's right.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Suggest wording this comment as follows:

"""
Create the backends but don't start them yet. We need to create the backends to allocate the ports, so that we know what resource names to populate in the xDS servers when we start them. However, we can't start the backends until after we've started the xDS servers, because in the tests that use xDS-enabled servers, the backends will try to contact the xDS servers as soon as they start up.
"""

Done. (That was exactly the problem)

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please move these two methods down to line 1384, so that they're next to the other related methods.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

To avoid having two versions of SetListenerAndRouteConfiguration(), how about something like this:

// Interface for accessing HttpConnectionManager config in Listener.
class HcmAccessor {
 public:
  virtual ~HcmAccessor() = default;
  virtual HttpConnectionManager Unpack(const Listener& listener) const = 0;
  virtual void Pack(const HttpConnectionManager& hcm, Listener* listener) const = 0;
};

// Client-side impl.
class ClientHcmAccessor : public HcmAccessor {
 public:
  HttpConnectionManager Unpack(const Listener& listener) const override {
    HttpConnectionManager http_connection_manager;
    listener.api_listener().api_listener().UnpackTo(&http_connection_manager);
    return http_connection_manager;
  }
  void Pack(const HttpConnectionManager& hcm, Listener* listener) const override {
    auto* api_listener =
        listener.mutable_api_listener()->mutable_api_listener();
    api_listener->PackFrom(http_connection_manager);
  }
};

// Server-side impl.
class ServerHcmAccessor : public HcmAccessor {
 public:
  HttpConnectionManager Unpack(const Listener& listener) const override {
    HttpConnectionManager http_connection_manager;
    default_server_listener_.mutable_default_filter_chain()
        ->mutable_filters()
        ->at(0)
        .mutable_typed_config()
        ->UnpackTo(&http_connection_manager);
    return http_connection_manager;
  }
  void Pack(const HttpConnectionManager& hcm, Listener* listener) const override {
    listener.mutable_default_filter_chain()
        ->mutable_filters()
        ->at(0)
        .mutable_typed_config()
        ->PackFrom(http_connection_manager);
  }
};

void SetListenerAndRouteConfiguration(
    int idx, Listener listener, RouteConfiguration& route_config,
    const HcmAccessor& hcm_accessor = ClientHcmAccessor()) {
  HttpConnectionManager http_connection_manager = hcm_accessor.Unpack(listener);
  if (GetParam().enable_rds_testing()) {
    auto* rds = http_connection_manager.mutable_rds();
    rds->set_route_config_name(kDefaultRouteConfigurationName);
    rds->mutable_config_source()->mutable_ads();
    balancers_[idx]->ads_service()->SetRdsResource(route_config);
  } else {
    *http_connection_manager.mutable_route_config() = route_config;
  }
  hcm_accessor.Pack(http_connection_manager, &listener);
  balancers_[idx]->ads_service()->SetLdsResource(listener);
}

This approach would also eliminate the need for GetDefaultServerHttpConnectionManager().

Took the suggestion. Removed GetDefaultServerHttpConnectionManager(), but kept SetServerListenerAndResourceConfiguration for ease of use.

---

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

Previously, markdroth (Mark D. Roth) wrote…

The setting and unsetting of the env vars can be done in SetUpTestSuite() and TearDownTestSuite().

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This should also run with TestType::set_enable_rds_testing().

Added a TODO for it

[markdroth]

This looks almost ready to merge, just a couple of test comments remaining.

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

Reviewed 1 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yashykt)

---

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

Previously, yashykt (Yash Tibrewal) wrote…

Added a TODO for it

Oh, I see -- the problem is that we don't yet have the machinery to actually start the RDS watch on the server side. Okay, that makes sense.

---

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

  }

  void SetServerListenerAndRouteConfiguration(

If we're going to have this variant, then we should probably move the functionality of PopulateServerListenerNameAndPort() into this method, since the two methods will always be used together. I think you can just add an int port parameter to this method right after the listener parameter and then remove the other method.

---

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

  }

  void SetServerListenerAndRouteConfiguration(int idx, Listener listener) {

I think it will be clearer not to have this variant. Callers can explicitly pass default_server_route_config_ when that's what they want.

---

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

  listener.mutable_api_listener()->mutable_api_listener()->UnpackTo(
      &http_connection_manager);
  auto* filter = http_connection_manager.add_http_filters();

This test was correct the way it was before this latest change. It's testing the case where we NACK a terminal filter that is not at the end of the list, not the case that we NACK a non-terminal filter at the end of the list; the latter is already covered by the previous test.

---

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

// TODO(yashykt): Also add a test type with set_enable_rds_testing() once we
// start fetching non-inline resources. We are only testing the server here.

The existing "We are only testing the server here" comment should not be part of the TODO.

[yashykt]

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @markdroth)

---

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

Previously, markdroth (Mark D. Roth) wrote…

If we're going to have this variant, then we should probably move the functionality of PopulateServerListenerNameAndPort() into this method, since the two methods will always be used together. I think you can just add an int port parameter to this method right after the listener parameter and then remove the other method.

Done. It got less clear so changed the name to SetServerListener*Name*AndRouteConfiguration(). Also,PopulateServerListenerNameAndPort() is still useful in places though so kept it around

---

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

Previously, markdroth (Mark D. Roth) wrote…

I think it will be clearer not to have this variant. Callers can explicitly pass default_server_route_config_ when that's what they want.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This test was correct the way it was before this latest change. It's testing the case where we NACK a terminal filter that is not at the end of the list, not the case that we NACK a non-terminal filter at the end of the list; the latter is already covered by the previous test.

It's still doing the same thing. Note that the error message remains unchanged. Added a comment to make it clear, and adding a terminal filter instead. Now, we have two terminal filters in the list. (Added a commend to explain.)

---

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

Previously, markdroth (Mark D. Roth) wrote…

The existing "We are only testing the server here" comment should not be part of the TODO.

Done.

[markdroth]

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

---

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

Previously, yashykt (Yash Tibrewal) wrote…

Done. It got less clear so changed the name to SetServerListener*Name*AndRouteConfiguration(). Also,PopulateServerListenerNameAndPort() is still useful in places though so kept it around

I don't think PopulateServerListenerNameAndPort() should be needed. It looks like every place you're calling it, you're using SetLdsResource() directly instead of calling SetServerListenerNameAndRouteConfiguration(). I think all those call sites should be changed to use SetServerListenerNameAndRouteConfiguration() instead of SetLdsResource(), or else the tests won't work when we start running with TestType::set_enable_rds_testing().

Basically, I think none of the server-side tests should be calling SetLdsResource() directly.

---

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

Previously, yashykt (Yash Tibrewal) wrote…

It's still doing the same thing. Note that the error message remains unchanged. Added a comment to make it clear, and adding a terminal filter instead. Now, we have two terminal filters in the list. (Added a commend to explain.)

I still don't see why this test needed to be changed. The original code was duplicating the terminal filter, so that there were two terminal filters in the list, which is exactly what we want here.

What problem was the existing test code causing?

---

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

  Listener listener = default_server_listener_;
  HttpConnectionManager http_connection_manager;
  listener.mutable_default_filter_chain()

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

---

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

  Listener listener = default_server_listener_;
  HttpConnectionManager http_connection_manager;
  listener.mutable_default_filter_chain()

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

---

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

  Listener listener = default_server_listener_;
  HttpConnectionManager http_connection_manager;
  listener.mutable_default_filter_chain()

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

---

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

  Listener listener = default_server_listener_;
  HttpConnectionManager http_connection_manager;
  listener.mutable_default_filter_chain()

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

[yashykt]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @markdroth and @yashykt)

---

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

Previously, markdroth (Mark D. Roth) wrote…

I don't think PopulateServerListenerNameAndPort() should be needed. It looks like every place you're calling it, you're using SetLdsResource() directly instead of calling SetServerListenerNameAndRouteConfiguration(). I think all those call sites should be changed to use SetServerListenerNameAndRouteConfiguration() instead of SetLdsResource(), or else the tests won't work when we start running with TestType::set_enable_rds_testing().

Basically, I think none of the server-side tests should be calling SetLdsResource() directly.

Half the places that use SetServerListenerNameAndRouteConfiguration are testing certain NACKing conditions, for example, cases that do not set the name or port properly.
The other half comes from XdsServerFilterChainMatchTest which specifically test multiple filter chains which would need their own route config. That doesn't work with the logic of SetServerListenerNameAndRouteConfiguration which assumes that default_filter_chain will be used.

As long as we make sure that XdsServerRdsTest has a unit test which makes sure that non-inline route config work for non-default filter chains, we don't need to enable rds testing for XdsServerFilterChainMatchTest and we would still have good test coverage.

---

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

Previously, markdroth (Mark D. Roth) wrote…

I still don't see why this test needed to be changed. The original code was duplicating the terminal filter, so that there were two terminal filters in the list, which is exactly what we want here.

What problem was the existing test code causing?

Ah I see the confusion. The issue is that duplicating the filter causing that exact issue - duplicate filters with the same name. Earlier, we had a single loop, so before detecting the duplicate filters we would run into a terminal filter that was not at the end of the list, properly getting the expected NACK. Now, we first see the duplicate filters.

[yashykt]

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @markdroth)

---

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

Previously, markdroth (Mark D. Roth) wrote…

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This can use ServerHcmAccessor to unpack and re-pack the HCM config.

Done.

[markdroth]

This looks great!

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

---

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

Previously, yashykt (Yash Tibrewal) wrote…

Half the places that use SetServerListenerNameAndRouteConfiguration are testing certain NACKing conditions, for example, cases that do not set the name or port properly.
The other half comes from XdsServerFilterChainMatchTest which specifically test multiple filter chains which would need their own route config. That doesn't work with the logic of SetServerListenerNameAndRouteConfiguration which assumes that default_filter_chain will be used.

As long as we make sure that XdsServerRdsTest has a unit test which makes sure that non-inline route config work for non-default filter chains, we don't need to enable rds testing for XdsServerFilterChainMatchTest and we would still have good test coverage.

Okay, I guess that works. Let's just be very careful going forward not to accidentally add a test to a suite other than XdsServerRdsTest that does actually depend on whether or not RDS is being used.

---

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

Previously, yashykt (Yash Tibrewal) wrote…

Ah I see the confusion. The issue is that duplicating the filter causing that exact issue - duplicate filters with the same name. Earlier, we had a single loop, so before detecting the duplicate filters we would run into a terminal filter that was not at the end of the list, properly getting the expected NACK. Now, we first see the duplicate filters.

Ah, okay. That makes sense, thanks.

[yashykt]

Thanks for reviewing! Rest of #27388 coming up :)