[Audit Logging] xDS e2e test for audit logging.

[rockspore]

Added tests involve:

1. Checking the # of logger invocations with multiple RBACs in the chain.
2. Verifying content in audit context with action and audit condition permutations.
3. Confirm custom logger and built-in logger configurations are working.
4. Confirm the feature is protected by the environment variable.

[markdroth]

This looks good! Comments are minor; feel free to merge after addressing.

[markdroth]

I think you could write this a bit more easily using the protobuf method to stringify the enum element:

https://protobuf.dev/reference/cpp/cpp-generated/#enum

So you should be able to do something like this:

if (rbac_audit_condition_ !=
    ::envoy::config::rbac::v3::
            RBAC_AuditLoggingOptions_AuditCondition_NONE) {
  retval += absl::StrCat(
      "AuditCondition",
      ::envoy::config::rbac::v3::
          RBAC_AuditLoggingOptions_AuditCondition_Name(
              rbac_audit_condition_));
}

[markdroth]

Suggest writing this as EXPECT_THAT(audit_logs_, ::testing::ElementsAre()), so that there's a more useful error message if the expectation fails.

Same thing in all tests.

[markdroth]

Suggest writing this as:

auto* audit_logger = logging_options->add_logger_configs()->mutable_audit_logger();

Then you don't need the test_logger variable at all, nor do you need to copy it into place later.

Same thing in all tests.

[markdroth]

Please move all of the new tests in this suite down to line 2978, so that all of the audit logging tests are after all of the tests that cover the actual authorization flow.

[markdroth]

Let's actually check the values here, not just the size. Note that you can construct a vector of strings however you want and then compare it with ::testing::ElementsAreArray().

Same thing for all tests.

[markdroth]

When I suggested using ElementsAreArray(), I meant something like this:

std::vector<absl::string_view> expected = {
    "{\"authorized\":true,\"matched_rule\":\"policy\","
    "\"policy_name\":\"rbac1\",\"principal\":\"\",\"rpc_"
    "method\":\"/grpc.testing.EchoTestService/Echo\"}"
};
if (GetParam().rbac_action() == RBAC_Action_DENY) {
  expected.push_back(
      "{\"authorized\":false,\"matched_rule\":\"policy\","
      "\"policy_name\":\"rbac1\",\"principal\":\"\",\"rpc_"
      "method\":\"/grpc.testing.EchoTestService/Echo\"}");
} else {
  expected.push_back(
      "{\"authorized\":true,\"matched_rule\":\"policy\","
      "\"policy_name\":\"rbac1\",\"principal\":\"\",\"rpc_"
      "method\":\"/grpc.testing.EchoTestService/Echo\"}");
  expected.push_back(
      "{\"authorized\":true,\"matched_rule\":\"policy\","
      "\"policy_name\":\"rbac1\",\"principal\":\"\",\"rpc_"
      "method\":\"/grpc.testing.EchoTestService/Echo\"}");
}
EXPECT_THAT(audit_logs_, ElementsAreArray(expected));

In other words, the advantage of ElementsAreArray() over ElementsAre() is that the former allows you to construct the list separately, not specifying it as a literal in the expectation itself. If you're going to specify it linearly in the expectation itself, then you might as well stick with ElementsAre(), because ElementsAreArray() doesn't actually provide any additional value (in fact, it's actually slightly more cumbersome).

Having said that, though, it occurs to me that in this case, it's probably easier to just say this:

constexpr absl::string_view kAllowedLog =
    "{\"authorized\":true,\"matched_rule\":\"policy\","
    "\"policy_name\":\"rbac1\",\"principal\":\"\",\"rpc_"
    "method\":\"/grpc.testing.EchoTestService/Echo\"}";
constexpr absl::string_view kDeniedLog =
    "{\"authorized\":false,\"matched_rule\":\"policy\","
    "\"policy_name\":\"rbac1\",\"principal\":\"\",\"rpc_"
    "method\":\"/grpc.testing.EchoTestService/Echo\"}";
if (GetParam().rbac_action() == RBAC_Action_DENY) {
  EXPECT_THAT(audit_logs_, ElementsAre(kAllowedLog, kDeniedLog));
} else {
  EXPECT_THAT(audit_logs_, ElementsAre(kAllowedLog, kAllowedLog, kAllowedLog));
}

[rockspore]

Ah I didn't realize and indeed was a bit confused about what advantage ElementsAreArray could have.

So I took your first suggestion because the policy name (the filter name here) is not the same for the first and third allowed log. Three rbacs have different names.