The overall structure here looks excellent!

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

Reviewed 50 of 58 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 33 unresolved discussions (waiting on @ashithasantosh and @yashykt)

BUILD, line 449 at r2 (raw file):

        "grpc_common",
        "grpc_lb_policy_grpclb_secure",
        "grpc_rbac_filter",

We don't want this dependency here, because it would pull in the RBAC code by default, which adds a dependency on the re2 library. We don't want to pull in that dependency if we're building without xDS for code bloat reasons.

Since this dependency is already present on the grpc_xds_client target, I think it's fine to remove it here.

src/core/ext/filters/rbac/rbac_filter.cc, line 45 at r2 (raw file):

  // The index of this filter instance among instances of the same filter.
  int index_;

This should probably be type size_t.

src/core/ext/filters/rbac/rbac_filter.cc, line 50 at r2 (raw file):

};

class CallData {

Suggest nesting this inside of ChannelData, so that there's no need for public accessor methods on ChannelData.

Note that this will require moving the filter vtable to be a static data member of ChannelData, which will in turn require moving the class definition to the .h file. You can look at the SDK authz filter as a good example of how to structure this. (I'd like to use this structure for all new filters going forward... for as long as filters still exist. :) )

src/core/ext/filters/rbac/rbac_filter.cc, line 65 at r2 (raw file):

  static void RecvInitialMetadataReady(void* user_data,
                                       grpc_error_handle error);
  static void RecvTrailingMetadataReady(void* user_data,

As we discussed, there's no need to intercept recv_trailing_metadata here. On the server side, if we call recv_initial_metadata_ready with an error, the call will be failed with the status from that error.

I think this will both greatly simplify the logic and remove the need for a lot of the data members.

src/core/ext/filters/rbac/rbac_filter.cc, line 106 at r2 (raw file):

          grpc_channel_stack_filter_instance_number(args->channel_stack, elem)),
      per_channel_evaluate_args_(
          grpc_find_auth_context_in_args(args->channel_args),

It would probably be a good idea to check that the auth context and the transport are both non-null. If they are null, we should have Init() return an error instead of just crashing.

src/core/ext/filters/rbac/rbac_filter.cc, line 167 at r2 (raw file):

  // The http-server filter removes the ':method' header from the metadata which
  // we need to add back here.
  grpc_metadata_batch_copy(&source, destination);

How expensive is this copy? We're going to be doing this on every single call, so we should make sure it's cheap, or maybe consider alternatives.

src/core/ext/filters/rbac/rbac_filter.cc, line 211 at r2 (raw file):

      }
    } else {
      calld->error_ =

Please handle the error case next to the condition that triggers it. In other words, instead of:

if (!error) {
  // ...long block to handle success...
} else {
  // ...handle error...
}

Please say:

if (error) {
  // ...handle error...
} else {
  // ...handle success...
}

src/core/ext/filters/rbac/rbac_filter.cc, line 211 at r2 (raw file):

      }
    } else {
      calld->error_ =

It's not necessarily an error if the policy has no authorization engine. The filter's override config might disable it, in which case the filter should be a no-op.

It would probably be a good idea to grab the method config in the ctor, so that if there is no policy to enforce, we don't need to intercept recv_initial_metadata_ready in the first place.

src/core/ext/filters/rbac/rbac_service_config_parser.h, line 45 at r2 (raw file):

  // active. The RBAC filter uses this method to get the RBAC policy configured
  // for a instance at a particular instance. The implementation reverses the
  // index since the ordering of HTTP filters is reverse on the server side. For

This same ordering property will apply to any HTTP filter on the server side. Given that, it probably makes sense to have the xDS server config selector take care of this instead of doing it in the service config parser for each individual server-side filter. This would be consistent with the fact that the xDS server config selector already handles reversing the order of the filters themselves; it should also do the same thing for the configs.

I think you could do this by simply reversing the order of the elements in the HTTP filter lists somewhere around here:

Alternatively, maybe you could just reverse the order as soon as you get the LDS result back, so that you don't have to reverse the order separately for both creating the filter list and for generating the service config.

src/core/ext/filters/rbac/rbac_service_config_parser.cc, line 561 at r2 (raw file):

std::unique_ptr<ServiceConfigParser::ParsedConfig>
RbacServiceConfigParser::ParsePerMethodParams(const grpc_channel_args* args,

Please add some tests similar to those in https://github.com/grpc/grpc/blob/master/test/core/client_channel/service_config_test.cc for this parser, although you can put them in their own file. (At some point, we should split up that particular file so that the parsing for each filter or LB policy or whatever is in its own file.)

src/core/ext/filters/rbac/rbac_service_config_parser.cc, line 579 at r2 (raw file):

  *error = GRPC_ERROR_CREATE_FROM_VECTOR("Rbac parser", &error_list);
  if (*error != GRPC_ERROR_NONE || rbac_policies.empty()) {
    gpr_log(GPR_ERROR, "here");

I assume this was added for debugging. :)

src/core/ext/xds/xds_http_filters.cc, line 111 at r2 (raw file):

                 {kXdsHttpFaultFilterConfigName});
  RegisterFilter(absl::make_unique<XdsHttpRbacFilter>(),
                 {kXdsHttpRbacFilterConfigName});

The RBAC filter actually uses a different proto for the override config than for the top-level config, so we need to register it under both names. The top-level config is RBAC:

https://github.com/envoyproxy/envoy/blob/30497202ccfed2a8a20a9da9028bd82e934b50a5/api/envoy/extensions/filters/http/rbac/v3/rbac.proto#L21

The override config is RBACPerRoute:

https://github.com/envoyproxy/envoy/blob/30497202ccfed2a8a20a9da9028bd82e934b50a5/api/envoy/extensions/filters/http/rbac/v3/rbac.proto#L41

src/core/ext/xds/xds_http_rbac_filter.cc, line 43 at r2 (raw file):

namespace {

inline std::string UpbStringToStdString(const upb_strview& str) {

Suggest including upb_utils.h instead of redefining this.

src/core/ext/xds/xds_http_rbac_filter.cc, line 417 at r2 (raw file):

      return rbac_json;
    }
    auto* inner_rbac_json = rbac_json.emplace("rules", Json::Object())

This would be more readable as:

Json::Object inner_rbac_json;
// ...populate inner_rbac_json...
rbac_json["rules"] = std::move(inner_rbac_json);

Same thing throughout.

src/core/ext/xds/xds_http_rbac_filter.cc, line 425 at r2 (raw file):

              .first->second.mutable_object();
      size_t iter = UPB_MAP_BEGIN;
      auto* entry = envoy_config_rbac_v3_RBAC_policies_next(rules, &iter);

We can avoid repeating this call:

while (true) {
  auto* entry = envoy_config_rbac_v3_RBAC_policies_next(rules, &iter);
  if (entry == nullptr) break;
  // ...parse entry...
}

src/core/ext/xds/xds_http_rbac_filter.cc, line 430 at r2 (raw file):

            envoy_config_rbac_v3_RBAC_PoliciesEntry_value(entry));
        if (!policy.ok()) {
          return policy;

This should add some context to the error message indicating which policy the error came from.

Also, instead of stopping when we see the first error, we should continue on, in case there are other errors that can be reported at the same time.

Same thing throughout this parsing code, everywhere that we recurse to another parsing function.

src/core/ext/xds/xds_http_rbac_filter.cc, line 453 at r2 (raw file):

                                        upb_arena* arena) const {
  absl::StatusOr<Json> parse_result =
      ParseHttpRbacIntoJson(serialized_filter_config, arena);

How different is the JSON representation that we're generating here from the JSON form of the proto? Would we save some code if we just used upb's ability to convert the proto to JSON, and then had the filter's service config parser read that JSON format?

I'm not sure whether or not this would be a win, since we'd still need to look at all of the fields for validation purposes anyway.

src/core/ext/xds/xds_http_rbac_filter.cc, line 463 at r2 (raw file):

XdsHttpRbacFilter::GenerateFilterConfigOverride(
    upb_strview serialized_filter_config, upb_arena* arena) const {
  return GenerateFilterConfig(serialized_filter_config, arena);

As mentioned elsewhere, the RBAC filter actually has a different config proto for its override config, so we need to handle parsing that other proto here.

src/core/ext/xds/xds_http_rbac_filter.cc, line 484 at r2 (raw file):

    const FilterConfig& hcm_filter_config,
    const FilterConfig* filter_config_override) const {
  Json policy_json = filter_config_override != nullptr

I think this logic will need to be a little more nuanced. It looks like the RBAC filter override proto allows disabling the top-level policy by not providing a substitute:

https://github.com/envoyproxy/envoy/blob/30497202ccfed2a8a20a9da9028bd82e934b50a5/api/envoy/extensions/filters/http/rbac/v3/rbac.proto#L48

src/core/ext/xds/xds_listener.cc, line 274 at r2 (raw file):

    XdsListenerResource::HttpConnectionManager* http_connection_manager) {
  MaybeLogHttpConnectionManager(context, http_connection_manager_proto);
  // NACK a non-zero `xff_num_trusted_hops` and a `non-empty

Should we NACK these fields only on the server, or also on the client? @ejona86, is there any reason we need to care about these fields on the client? What are Java and Go doing here?

I suspect we want this validation only the on server side.

src/core/ext/xds/xds_listener.cc, line 283 at r2 (raw file):

  }
  size_t original_ip_detection_extensions;
  envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_original_ip_detection_extensions(

I think this can just use the "has" method (envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_has_original_ip_detection_extensions()).

src/core/plugin_registry/grpc_plugin_registry.cc, line 132 at r2 (raw file):

  grpc_register_plugin(grpc_core::FaultInjectionFilterInit,
                       grpc_core::FaultInjectionFilterShutdown);
  grpc_register_plugin(grpc_core::RbacFilterInit, grpc_core::RbacFilterShutdown);

As per my comment elsewhere, I don't think we want to depend on this filter unless we're building with xDS, since that would pull in a dependency on the re2 library by default. So let's move this (and the forward declarations above) inside of the #ifndef GRPC_NO_XDS blocks.

src/proto/grpc/testing/xds/v3/expr.proto, line 15 at r2 (raw file):

// limitations under the License.

// TODO(yashykt) : Figure out how to not need this

@veblush may have some ideas on this.

The CEL protos don't define an RPC service, so their build rules shouldn't depend on gRPC. So this may just involve taking the external dependency on CEL.

src/proto/grpc/testing/xds/v3/http_rbac.proto, line 21 at r2 (raw file):

syntax = "proto3";

package envoy.extensions.filters.http.rbac.v3;

Suggest renaming this file to http_filter_rbac.proto, to make it clear what it includes.

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

class XdsRbacTest : public XdsServerRdsTest {
 protected:
  void SetServerRbacPolicy(Listener listener, const RBAC& rbac) {

We should also have a way to test multiple instances of the RBAC filter, since we know we need to support such configs.

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

        break;
      case TestType::FilterConfigSetup::kRouteOverride:
        filter->mutable_typed_config()->PackFrom(RBAC());

This will need to use the RBAC filter override config type.

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

        break;
      case TestType::FilterConfigSetup::kRouteOverride:
        filter->mutable_typed_config()->PackFrom(RBAC());

Let's make sure to also test the case where the override config just removes the top-level policy.

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

};

TEST_P(XdsRbacTest, AbsentRbacPolicy) {

We should also have a test for a non-empty top-level policy and a non-empty override policy.

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

TEST_P(XdsRbacTest, AbsentRbacPolicy) {
  if (GetParam().rbac_action() == RBAC_Action_DENY) {

For these tests that only need to run once, how about just putting them in their own test suite that doesn't have all of the permutations of TestType?

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

  SendRpc(
      [this]() { return CreateInsecureChannel(); }, {}, {},
      /*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW);

Please have all of these tests check that the RPC fails with the right status code.

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

}

TEST_P(XdsRbacTest, AnyPermissionAnyPrincipalAllow) {

The Allow suffix doesn't seem appropriate here, given that this will run for both allow and deny tests.

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

}

TEST_P(XdsRbacTest, MethodPostPermissionAnyPrincipal) {

For most of these tests where the rules catch one particular attribute of the request, it would be good to also send a request that does not have the attribute and verify that it will not be caught by the rule.

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

  Policy policy;
  policy.add_permissions()->mutable_requested_server_name()->set_exact(
      "server_name");

Is this matching against the authority set in the client request? Where are we setting that in the test?

Note that there have been some changes related to this in the test framework recently, which is why I ask. I'm not sure if you've merged master recently.

Reviewable status: 44 of 62 files reviewed, 33 unresolved discussions (waiting on @ashithasantosh, @ctiller, @ejona86, @markdroth, and @yashykt)

BUILD, line 449 at r2 (raw file):

Done.

src/core/ext/filters/rbac/rbac_filter.cc, line 45 at r2 (raw file):

Done.

src/core/ext/filters/rbac/rbac_filter.cc, line 50 at r2 (raw file):

This is one case where I think nesting classes to avoid accessor methods makes things uglier since we have to move stuff to the header. Done anyway

src/core/ext/filters/rbac/rbac_filter.cc, line 65 at r2 (raw file):

Done.

src/core/ext/filters/rbac/rbac_filter.cc, line 106 at r2 (raw file):

Hmm after removing insecure builds we wouldn't have this issue, but I support the check irrespective

src/core/ext/filters/rbac/rbac_filter.cc, line 167 at r2 (raw file):

@ctiller

src/core/ext/filters/rbac/rbac_filter.cc, line 211 at r2 (raw file):

The way the code is structured is that an empty RBAC policy is created even if there is no policy to enforce, so this error should not really happen, but i've still handled this case as a defensive mechanism.

src/core/ext/filters/rbac/rbac_filter.cc, line 211 at r2 (raw file):

Done.

src/core/ext/filters/rbac/rbac_service_config_parser.h, line 45 at r2 (raw file):

Done.

src/core/ext/filters/rbac/rbac_service_config_parser.cc, line 579 at r2 (raw file):

removed

src/core/ext/xds/xds_http_filters.cc, line 111 at r2 (raw file):

Thanks!

src/core/ext/xds/xds_http_rbac_filter.cc, line 43 at r2 (raw file):

Done.

src/core/ext/xds/xds_http_rbac_filter.cc, line 417 at r2 (raw file):

Done.

src/core/ext/xds/xds_http_rbac_filter.cc, line 425 at r2 (raw file):

Done.

src/core/ext/xds/xds_http_rbac_filter.cc, line 430 at r2 (raw file):

Done.

src/core/ext/xds/xds_http_rbac_filter.cc, line 453 at r2 (raw file):

Other than the route override, I've maintained the canonical JSON form of the proto.

Hmm I agree that we will need to delve inside the proto/JSON anyway for validation purposes, but we could probably do it either way.

src/core/ext/xds/xds_http_rbac_filter.cc, line 463 at r2 (raw file):

Done.

src/core/ext/xds/xds_http_rbac_filter.cc, line 484 at r2 (raw file):

Given that an absent RBAC policy is equivalent to a RBAC policy with empty rules https://github.com/envoyproxy/envoy/blob/30497202ccfed2a8a20a9da9028bd82e934b50a5/api/envoy/extensions/filters/http/rbac/v3/rbac.proto#L26, I think we can just use the same RBAC JSON object format for both the top-level policy and the override

src/core/ext/xds/xds_listener.cc, line 274 at r2 (raw file):

Quoting https://github.com/grpc/proposal/blob/master/A41-xds-rbac.md New validation should occur for HttpConnectionManager to allow equating RBAC's direct_remote_ip and remote_ip. If the RBAC implementation does not distinguish between these fields, then HttpConnectionManager.xff_num_trusted_hops must be unset or zero and HttpConnectionManager.original_ip_detection_extensions must be empty. If either field has an incorrect value, the Listener must be NACKed. For simplicity, this behavior applies independent of the Listener type (both client-side and server-side).

src/core/plugin_registry/grpc_plugin_registry.cc, line 132 at r2 (raw file):

Done.

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

Done.

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

I think the AnyPermissionAnyPrincipal serves that purpose

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

This is not a test that needs to run once. It needs to run for atleast the configurations where - 1) rds is enabled/disabled 2) RouteConfiguration override is enabled/disabled

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

Done.

src/proto/grpc/testing/xds/v3/http_rbac.proto, line 21 at r2 (raw file):

Done.

I see that you're still making changes, but wanted to send you what I already reviewed.

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

Reviewed 18 of 18 files at r3, 2 of 3 files at r4, all commit messages.
Reviewable status: 61 of 62 files reviewed, 14 unresolved discussions (waiting on @ashithasantosh, @ctiller, @ejona86, @markdroth, and @yashykt)

src/core/ext/filters/rbac/rbac_filter.h, line 70 at r3 (raw file):

  // The index of this filter instance among instances of the same filter.
  int index_;

size_t

src/core/ext/filters/rbac/rbac_filter.cc, line 50 at r2 (raw file):

Thanks. In the long run, I think we'll want to start writing unit tests for individual modules, in which case having the structure in the .h file will be a win anyway.

src/core/ext/xds/xds_http_rbac_filter.cc, line 484 at r2 (raw file):

That comment says:

  // If absent, no enforcing RBAC policy will be applied.
  // If present and empty, DENY.

Doesn't that imply a different behavior for absent and present but empty?

src/core/ext/xds/xds_listener.cc, line 274 at r2 (raw file):

Okay. In that case, please also add tests that we NACK these fields on the client test.

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

            http_connection_manager_proto, &num_filters);
    std::set<absl::string_view> names_seen;
    // On the server side, iterate the filters in reverse order since received

I don't think this is the right place to do this. The data returned by the XdsClient should have the entries in the same order as in the xDS proto. I think we should do this reversing in the XdsServerConfigFetcher code, when we get the resource from the XdsClient.

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

Sorry, how does that test do that? It looks to me like just like all of the other tests, it's still putting all of the rules into either the top-level policy or the override policy, depending on the value of GetParam().filter_config_setup(). Am I missing something here?

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

My point is that it doesn't need to run for all the configurations that it's actually being run for. If we move it to a different test suite, then we can run it only for the different values of the dimensions that we actually care about.

To say this another way, I think that if (GetParam().something()) return; in a test case is an anti-pattern. Instead of making a test case that we don't care about be a no-op, it's generally better to avoid configuring that test case in the first place.

Reviewable status: 61 of 62 files reviewed, 14 unresolved discussions (waiting on @ashithasantosh, @ctiller, @ejona86, @markdroth, and @yashykt)

src/core/ext/xds/xds_http_rbac_filter.cc, line 484 at r2 (raw file):

Yes, that's correct. I should have said absent instead of empty.

The absent part is enforced when rules field is not set. The present and empty part is enforced when the rules field is set but none of its fields are set.

This absent part is exactly the part that I'm proposing we use to also implement a RBACPerRoute override with an absent rbac field.

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

Oh I misunderstood your comment, you meant a single test which has both the top-level and override policies as non-empty.

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

Done.

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

Done.

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

requested_server_name is simply hard-coded to the empty string in gRPC as per https://github.com/grpc/proposal/blob/master/A41-xds-rbac.md

Reviewable status: 58 of 63 files reviewed, 14 unresolved discussions (waiting on @ashithasantosh, @ctiller, @ejona86, @markdroth, and @yashykt)

src/core/ext/filters/rbac/rbac_filter.h, line 70 at r3 (raw file):

Done.

src/core/ext/xds/xds_listener.cc, line 274 at r2 (raw file):

Done.

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

Done.

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

Done.

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

Done.

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

Done.

Reviewable status: 54 of 63 files reviewed, 14 unresolved discussions (waiting on @ashithasantosh, @ctiller, @ejona86, @markdroth, and @yashykt)

src/core/ext/filters/rbac/rbac_service_config_parser.cc, line 561 at r2 (raw file):

Added some tests here for the parsing functionality. Still depending on the xds_end2end_test though for testing whether the RBAC policies do what they are supposed to.

Reviewable status: 54 of 63 files reviewed, 14 unresolved discussions (waiting on @ashithasantosh, @ctiller, @ejona86, @markdroth, and @veblush)

src/proto/grpc/testing/xds/v3/expr.proto, line 15 at r2 (raw file):

I've tried a few things here, but our grpc_proto_library does not play nice with others. There are more things I want to try here but I would prefer not to block this.

Reviewable status: 54 of 63 files reviewed, 14 unresolved discussions (waiting on @ashithasantosh, @ctiller, @ejona86, @markdroth, and @veblush)

src/core/ext/filters/rbac/rbac_filter.cc, line 167 at r2 (raw file):

I guess I could just add the header and later remove it.

This looks really good! All remaining comments are minor, except for resolving the handling of the :method header, which should be easy to fix now.

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

Reviewed 1 of 5 files at r5, 1 of 5 files at r6, 23 of 23 files at r7, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ashithasantosh, @ctiller, and @yashykt)

log, line 1 at r7 (raw file):

Note: Google Test filter = *AnyPermissionOrIdPrincipal*

I assume this file was added by accident.

src/core/ext/filters/rbac/rbac_filter.cc, line 167 at r2 (raw file):

Now that #28204 has landed, I think the :method header will actually already be present in the batch, so we shouldn't need to do anything special for it here at all.

src/core/ext/filters/rbac/rbac_filter.cc, line 57 at r7 (raw file):

    op->payload->recv_initial_metadata.recv_initial_metadata_ready =
        &calld->recv_initial_metadata_ready_;
    calld->payload_ = op->payload;

We should probably stash just the value of op->payload->recv_initial_metadata.recv_initial_metadata, not the whole payload, since a filter further down the stack could change the values in the payload struct between here and where we use it.

src/core/ext/xds/xds_http_rbac_filter.cc, line 484 at r2 (raw file):

Ah, okay, got it. Thanks for the explanation.

src/proto/grpc/testing/xds/v3/expr.proto, line 15 at r2 (raw file):

That's fine with me if it's okay with @veblush.

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

        }
        if (expected_status.has_value() &&
            *expected_status != status.error_code()) {

How about just doing:

EXPECT_EQ(*expected_status, status.error_code());

That way, the log happens automatically, and the test will fail if the expectation is violated.

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

      GetParam().filter_config_setup() ==
          TestType::FilterConfigSetup::kRouteOverride) {
    ASSERT_TRUE(WaitForRdsNack(StatusCode::DEADLINE_EXCEEDED))

As per Donna's discovery in #27938, please run these tests with TestType::bootstrap_source_ set to kBootstrapFromEnvVar, so that the NACKs can expect UNAVAILABLE instead of DEADLINE_EXCEEDED.

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

TEST_P(XdsRbacTestWithRouteOverrideAlwaysPresent, EmptyRBACPerRouteOverride) {
  if (GetParam().filter_config_setup() ==

Doesn't look like this check is needed anymore.

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

  auto* filter = http_connection_manager.add_http_filters();
  filter->set_name("rbac");
  // Create a top-level RBAC policy with an Allow action but empty matching

Wouldn't it be more straightforward to just use a deny rule with an any match?

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

TEST_P(XdsRbacTestWithRouteOverrideAlwaysPresent,
       NonEmptyTopLevelRBACNonEmptyPerRouteOverride) {
  if (GetParam().filter_config_setup() ==

Doesn't look like this check is needed anymore.

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

  auto* filter = http_connection_manager.add_http_filters();
  filter->set_name("rbac");
  // Create a top-level RBAC policy with an Allow action but empty matching

Wouldn't it be more straightforward to just use a deny rule with an any match?

Reviewable status: 62 of 66 files reviewed, 9 unresolved discussions (waiting on @ashithasantosh, @ctiller, and @markdroth)

src/core/ext/filters/rbac/rbac_filter.cc, line 167 at r2 (raw file):

I'm not sure I understand how that PR #28204 changes things

src/core/ext/filters/rbac/rbac_filter.cc, line 57 at r7 (raw file):

Hmm I guess that's legal. Done!

src/proto/grpc/testing/xds/v3/expr.proto, line 15 at r2 (raw file):

Yeah I'm syncing with him on what we want to do over here.

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

I'm also using the retry loop here to wait for updates to be applied.
Some tests are of the form -

Apply first update;
Test update;
Apply second update;
Test second update

When testing the second update, the retry loop gives some time for the new update to be actually applied, and in the meantime we do not want to fail the test.

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

Ah you are right. Thanks!

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

Removed

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

Either way works. Changed.

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

Removed.

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

Either way works. Changed.

log, line 1 at r7 (raw file):

Yeah, removed :)

Just a few minor nits remaining!

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

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ashithasantosh, @ctiller, and @yashykt)

src/core/ext/filters/rbac/rbac_filter.h, line 56 at r8 (raw file):

    // State for keeping track of recv_initial_metadata
    grpc_metadata_batch* recv_initial_metadata_ = nullptr;
    uint32_t* recv_flags_ = nullptr;

I think this won't be necessary once you make the changes we discussed relative to #28204.

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

Are there cases where the RPC will fail with the wrong error before the second update? If not, then this won't hurt anything.

But I guess if the code is wrong, we won't exit the loop, so the test still verifies the right thing. I'll leave it up to you.

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

I think we no longer need any parameter to the WaitFor.*Nack() methods, since all callers now expect UNAVAILABLE.

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

// We are only testing the server here.
INSTANTIATE_TEST_SUITE_P(

Please add a short comment for all of these tests explaining why we need kBootstrapFromEnvVar.

Reviewable status: 63 of 66 files reviewed, 4 unresolved discussions (waiting on @ashithasantosh, @ctiller, and @markdroth)

src/core/ext/filters/rbac/rbac_filter.h, line 56 at r8 (raw file):

Adding a TODO in case https://github.com/grpc/grpc/pull/28369/files takes longer to merge

src/core/ext/filters/rbac/rbac_filter.cc, line 167 at r2 (raw file):

Adding a TODO in case https://github.com/grpc/grpc/pull/28369/files takes longer to merge

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

If there were a better way to make sure that the update has been applied I would prefer that.

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

Done.

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

Done.

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ashithasantosh)