From b10f4baaf14a775899e4cac6abe9924c496addda Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Wed, 8 Aug 2018 15:39:46 -0700 Subject: [PATCH 1/5] send rbac shadow policies metrics to mixer --- include/istio/control/http/report_data.h | 4 +++ include/istio/utils/attribute_names.h | 3 +++ src/envoy/http/mixer/report_data.h | 26 +++++++++++++++++++ src/istio/control/http/attributes_builder.cc | 13 ++++++++++ .../control/http/attributes_builder_test.cc | 22 ++++++++++++++++ src/istio/control/http/mock_report_data.h | 2 ++ src/istio/utils/attribute_names.cc | 6 +++++ 7 files changed, 76 insertions(+) diff --git a/include/istio/control/http/report_data.h b/include/istio/control/http/report_data.h index 23ad4b27777..53f8117e2a4 100644 --- a/include/istio/control/http/report_data.h +++ b/include/istio/control/http/report_data.h @@ -47,6 +47,10 @@ class ReportData { // Get destination ip/port. virtual bool GetDestinationIpPort(std::string* ip, int* port) const = 0; + // Get rbac shadow attributes. + virtual void GetRBACShadowAttributes(std::string* resp_code, + std::string* policy_id) const = 0; + // Get upstream host UID. This value overrides the value in the report bag. virtual bool GetDestinationUID(std::string* uid) const = 0; diff --git a/include/istio/utils/attribute_names.h b/include/istio/utils/attribute_names.h index 3f47247131f..f0d5d622e68 100644 --- a/include/istio/utils/attribute_names.h +++ b/include/istio/utils/attribute_names.h @@ -95,6 +95,9 @@ struct AttributeName { static const char kResponseGrpcStatus[]; static const char kResponseGrpcMessage[]; + + static const char kRbacShadowResponseCode[]; + static const char kRbacShadowPolicyId[]; }; } // namespace utils diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index cb27f67cc38..40d29f697d1 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -18,12 +18,16 @@ #include "common/request_info/utility.h" #include "envoy/http/header_map.h" #include "envoy/request_info/request_info.h" +#include "extensions/filters/http/well_known_names.h" #include "include/istio/control/http/controller.h" #include "src/envoy/utils/utils.h" namespace Envoy { namespace Http { namespace Mixer { +static const std::string shadow_policy_id_field = "shadow_effective_policyID"; +static const std::string shadow_resp_code_field = "shadow_response_code"; + namespace { // Set of headers excluded from response.headers attribute. const std::set ResponseHeaderExclusives = {}; @@ -114,6 +118,28 @@ class ReportData : public ::istio::control::http::ReportData { return ExtractGrpcStatus(trailers_, status) || ExtractGrpcStatus(headers_, status); } + + // Get rbac shadow attributes. + void GetRBACShadowAttributes(std::string *resp_code, + std::string *policy_id) const override { + const auto filter_meta = info_.dynamicMetadata().filter_metadata(); + const auto filter_it = + filter_meta.find(Extensions::HttpFilters::HttpFilterNames::get().Rbac); + if (filter_it == filter_meta.end()) { + return; + } + + const auto &data_struct = filter_it->second; + const auto resp_code_it = data_struct.fields().find(shadow_resp_code_field); + if (resp_code_it != data_struct.fields().end()) { + *resp_code = resp_code_it->second.string_value(); + } + + const auto policy_id_it = data_struct.fields().find(shadow_policy_id_field); + if (policy_id_it != data_struct.fields().end()) { + *policy_id = policy_id_it->second.string_value(); + } + } }; } // namespace Mixer diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 2650c196836..1d858d18bd7 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -229,6 +229,19 @@ void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) { builder.AddString(utils::AttributeName::kContextProxyErrorCode, info.response_flags); + + std::string rbac_shadow_resp_code; + std::string rbac_shadow_policy_id; + report_data->GetRBACShadowAttributes(&rbac_shadow_resp_code, + &rbac_shadow_policy_id); + if (!rbac_shadow_resp_code.empty()) { + builder.AddString(utils::AttributeName::kRbacShadowResponseCode, + rbac_shadow_resp_code); + } + if (!rbac_shadow_policy_id.empty()) { + builder.AddString(utils::AttributeName::kRbacShadowPolicyId, + rbac_shadow_policy_id); + } } } // namespace http diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index 7a0f7c31c12..7451af4cadb 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -342,6 +342,18 @@ attributes { string_value: "NR" } } +attributes { + key: "rbac.shadow.response_code" + value { + string_value: "403" + } +} +attributes { + key: "rbac.shadow.effective_policy_id" + value { + string_value: "policy-foo" + } +} )"; void ClearContextTime(const std::string &name, RequestContext *request) { @@ -561,6 +573,11 @@ TEST(AttributesBuilderTest, TestReportAttributes) { status->message = "grpc-message"; return true; })); + EXPECT_CALL(mock_data, GetRBACShadowAttributes(_, _)) + .WillOnce(Invoke([](std::string *code, std::string *id) { + *code = "403"; + *id = "policy-foo"; + })); RequestContext request; AttributesBuilder builder(&request); @@ -610,6 +627,11 @@ TEST(AttributesBuilderTest, TestReportAttributesWithDestIP) { info->response_flags = "NR"; })); EXPECT_CALL(mock_data, GetGrpcStatus(_)).WillOnce(testing::Return(false)); + EXPECT_CALL(mock_data, GetRBACShadowAttributes(_, _)) + .WillOnce(Invoke([](std::string *code, std::string *id) { + *code = "403"; + *id = "policy-foo"; + })); RequestContext request; SetDestinationIp(&request, "1.2.3.4"); diff --git a/src/istio/control/http/mock_report_data.h b/src/istio/control/http/mock_report_data.h index 26ca3830133..86cbabdd1f1 100644 --- a/src/istio/control/http/mock_report_data.h +++ b/src/istio/control/http/mock_report_data.h @@ -31,6 +31,8 @@ class MockReportData : public ReportData { MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string* ip, int* port)); MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string* ip)); MOCK_CONST_METHOD1(GetGrpcStatus, bool(GrpcStatus* status)); + MOCK_CONST_METHOD2(GetRBACShadowAttributes, + void(std::string* code, std::string* id)); }; } // namespace http diff --git a/src/istio/utils/attribute_names.cc b/src/istio/utils/attribute_names.cc index 9b6981b6619..ba7c21256ea 100644 --- a/src/istio/utils/attribute_names.cc +++ b/src/istio/utils/attribute_names.cc @@ -91,5 +91,11 @@ const char AttributeName::kRequestAuthRawClaims[] = "request.auth.raw_claims"; const char AttributeName::kResponseGrpcStatus[] = "response.grpc_status"; const char AttributeName::kResponseGrpcMessage[] = "response.grpc_message"; +// Rbac attributes +const char AttributeName::kRbacShadowResponseCode[] = + "rbac.shadow.response_code"; +const char AttributeName::kRbacShadowPolicyId[] = + "rbac.shadow.effective_policy_id"; + } // namespace utils } // namespace istio From 4748502d522041399c53df830c6297de075741b8 Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Wed, 8 Aug 2018 15:57:25 -0700 Subject: [PATCH 2/5] rename shadow -> permissive --- include/istio/control/http/report_data.h | 6 +++--- include/istio/utils/attribute_names.h | 4 ++-- src/envoy/http/mixer/report_data.h | 17 +++++++++------- src/istio/control/http/attributes_builder.cc | 20 +++++++++---------- .../control/http/attributes_builder_test.cc | 8 ++++---- src/istio/control/http/mock_report_data.h | 2 +- src/istio/utils/attribute_names.cc | 8 ++++---- 7 files changed, 34 insertions(+), 31 deletions(-) diff --git a/include/istio/control/http/report_data.h b/include/istio/control/http/report_data.h index 53f8117e2a4..b4d0bfc3796 100644 --- a/include/istio/control/http/report_data.h +++ b/include/istio/control/http/report_data.h @@ -47,9 +47,9 @@ class ReportData { // Get destination ip/port. virtual bool GetDestinationIpPort(std::string* ip, int* port) const = 0; - // Get rbac shadow attributes. - virtual void GetRBACShadowAttributes(std::string* resp_code, - std::string* policy_id) const = 0; + // Get rbac permissive mode policy attributes. + virtual void GetRBACPermissiveAttributes(std::string* resp_code, + std::string* policy_id) const = 0; // Get upstream host UID. This value overrides the value in the report bag. virtual bool GetDestinationUID(std::string* uid) const = 0; diff --git a/include/istio/utils/attribute_names.h b/include/istio/utils/attribute_names.h index f0d5d622e68..90bafc9f8d3 100644 --- a/include/istio/utils/attribute_names.h +++ b/include/istio/utils/attribute_names.h @@ -96,8 +96,8 @@ struct AttributeName { static const char kResponseGrpcStatus[]; static const char kResponseGrpcMessage[]; - static const char kRbacShadowResponseCode[]; - static const char kRbacShadowPolicyId[]; + static const char kRbacPermissiveResponseCode[]; + static const char kRbacPermissivePolicyId[]; }; } // namespace utils diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index 40d29f697d1..527c5237437 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -25,8 +25,9 @@ namespace Envoy { namespace Http { namespace Mixer { -static const std::string shadow_policy_id_field = "shadow_effective_policyID"; -static const std::string shadow_resp_code_field = "shadow_response_code"; +static const std::string permissive_policy_id_field = + "shadow_effective_policyID"; +static const std::string permissive_resp_code_field = "shadow_response_code"; namespace { // Set of headers excluded from response.headers attribute. @@ -119,9 +120,9 @@ class ReportData : public ::istio::control::http::ReportData { ExtractGrpcStatus(headers_, status); } - // Get rbac shadow attributes. - void GetRBACShadowAttributes(std::string *resp_code, - std::string *policy_id) const override { + // Get rbac permissive policy attributes. + void GetRBACPermissiveAttributes(std::string *resp_code, + std::string *policy_id) const override { const auto filter_meta = info_.dynamicMetadata().filter_metadata(); const auto filter_it = filter_meta.find(Extensions::HttpFilters::HttpFilterNames::get().Rbac); @@ -130,12 +131,14 @@ class ReportData : public ::istio::control::http::ReportData { } const auto &data_struct = filter_it->second; - const auto resp_code_it = data_struct.fields().find(shadow_resp_code_field); + const auto resp_code_it = + data_struct.fields().find(permissive_resp_code_field); if (resp_code_it != data_struct.fields().end()) { *resp_code = resp_code_it->second.string_value(); } - const auto policy_id_it = data_struct.fields().find(shadow_policy_id_field); + const auto policy_id_it = + data_struct.fields().find(permissive_policy_id_field); if (policy_id_it != data_struct.fields().end()) { *policy_id = policy_id_it->second.string_value(); } diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 1d858d18bd7..2c44f56280f 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -230,17 +230,17 @@ void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) { builder.AddString(utils::AttributeName::kContextProxyErrorCode, info.response_flags); - std::string rbac_shadow_resp_code; - std::string rbac_shadow_policy_id; - report_data->GetRBACShadowAttributes(&rbac_shadow_resp_code, - &rbac_shadow_policy_id); - if (!rbac_shadow_resp_code.empty()) { - builder.AddString(utils::AttributeName::kRbacShadowResponseCode, - rbac_shadow_resp_code); + std::string rbac_permissive_resp_code; + std::string rbac_permissive_policy_id; + report_data->GetRBACPermissiveAttributes(&rbac_permissive_resp_code, + &rbac_permissive_policy_id); + if (!rbac_permissive_resp_code.empty()) { + builder.AddString(utils::AttributeName::kRbacPermissiveResponseCode, + rbac_permissive_resp_code); } - if (!rbac_shadow_policy_id.empty()) { - builder.AddString(utils::AttributeName::kRbacShadowPolicyId, - rbac_shadow_policy_id); + if (!rbac_permissive_policy_id.empty()) { + builder.AddString(utils::AttributeName::kRbacPermissivePolicyId, + rbac_permissive_policy_id); } } diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index 7451af4cadb..4f90fbea20c 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -343,13 +343,13 @@ attributes { } } attributes { - key: "rbac.shadow.response_code" + key: "rbac.permissive.response_code" value { string_value: "403" } } attributes { - key: "rbac.shadow.effective_policy_id" + key: "rbac.permissive.effective_policy_id" value { string_value: "policy-foo" } @@ -573,7 +573,7 @@ TEST(AttributesBuilderTest, TestReportAttributes) { status->message = "grpc-message"; return true; })); - EXPECT_CALL(mock_data, GetRBACShadowAttributes(_, _)) + EXPECT_CALL(mock_data, GetRBACPermissiveAttributes(_, _)) .WillOnce(Invoke([](std::string *code, std::string *id) { *code = "403"; *id = "policy-foo"; @@ -627,7 +627,7 @@ TEST(AttributesBuilderTest, TestReportAttributesWithDestIP) { info->response_flags = "NR"; })); EXPECT_CALL(mock_data, GetGrpcStatus(_)).WillOnce(testing::Return(false)); - EXPECT_CALL(mock_data, GetRBACShadowAttributes(_, _)) + EXPECT_CALL(mock_data, GetRBACPermissiveAttributes(_, _)) .WillOnce(Invoke([](std::string *code, std::string *id) { *code = "403"; *id = "policy-foo"; diff --git a/src/istio/control/http/mock_report_data.h b/src/istio/control/http/mock_report_data.h index 86cbabdd1f1..cdf64b86c01 100644 --- a/src/istio/control/http/mock_report_data.h +++ b/src/istio/control/http/mock_report_data.h @@ -31,7 +31,7 @@ class MockReportData : public ReportData { MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string* ip, int* port)); MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string* ip)); MOCK_CONST_METHOD1(GetGrpcStatus, bool(GrpcStatus* status)); - MOCK_CONST_METHOD2(GetRBACShadowAttributes, + MOCK_CONST_METHOD2(GetRBACPermissiveAttributes, void(std::string* code, std::string* id)); }; diff --git a/src/istio/utils/attribute_names.cc b/src/istio/utils/attribute_names.cc index ba7c21256ea..3a9677fd147 100644 --- a/src/istio/utils/attribute_names.cc +++ b/src/istio/utils/attribute_names.cc @@ -92,10 +92,10 @@ const char AttributeName::kResponseGrpcStatus[] = "response.grpc_status"; const char AttributeName::kResponseGrpcMessage[] = "response.grpc_message"; // Rbac attributes -const char AttributeName::kRbacShadowResponseCode[] = - "rbac.shadow.response_code"; -const char AttributeName::kRbacShadowPolicyId[] = - "rbac.shadow.effective_policy_id"; +const char AttributeName::kRbacPermissiveResponseCode[] = + "rbac.permissive.response_code"; +const char AttributeName::kRbacPermissivePolicyId[] = + "rbac.permissive.effective_policy_id"; } // namespace utils } // namespace istio From 69114b04d3480bab41d828721ea86b617705d41c Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Wed, 8 Aug 2018 16:32:46 -0700 Subject: [PATCH 3/5] address comments --- include/istio/control/http/report_data.h | 10 +++++++--- src/envoy/http/mixer/report_data.h | 20 +++++++++---------- src/istio/control/http/attributes_builder.cc | 14 ++++++------- .../control/http/attributes_builder_test.cc | 16 +++++++-------- src/istio/control/http/mock_report_data.h | 3 +-- 5 files changed, 32 insertions(+), 31 deletions(-) diff --git a/include/istio/control/http/report_data.h b/include/istio/control/http/report_data.h index b4d0bfc3796..e9408001604 100644 --- a/include/istio/control/http/report_data.h +++ b/include/istio/control/http/report_data.h @@ -42,14 +42,18 @@ class ReportData { int response_code; std::string response_flags; }; + virtual void GetReportInfo(ReportInfo* info) const = 0; // Get destination ip/port. virtual bool GetDestinationIpPort(std::string* ip, int* port) const = 0; - // Get rbac permissive mode policy attributes. - virtual void GetRBACPermissiveAttributes(std::string* resp_code, - std::string* policy_id) const = 0; + // Get Rbac attributes. + struct RbacReportInfo { + std::string permissive_resp_code; + std::string permissive_policy_id; + }; + virtual void GetRbacReportInfo(RbacReportInfo* rbacReportInfo) const = 0; // Get upstream host UID. This value overrides the value in the report bag. virtual bool GetDestinationUID(std::string* uid) const = 0; diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index 527c5237437..13434bd6329 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -25,9 +25,8 @@ namespace Envoy { namespace Http { namespace Mixer { -static const std::string permissive_policy_id_field = - "shadow_effective_policyID"; -static const std::string permissive_resp_code_field = "shadow_response_code"; +static const std::string permissivePolicyIDField = "shadow_effective_policyID"; +static const std::string permissiveRespCodeField = "shadow_response_code"; namespace { // Set of headers excluded from response.headers attribute. @@ -120,9 +119,8 @@ class ReportData : public ::istio::control::http::ReportData { ExtractGrpcStatus(headers_, status); } - // Get rbac permissive policy attributes. - void GetRBACPermissiveAttributes(std::string *resp_code, - std::string *policy_id) const override { + // Get Rbac related attributes. + void GetRbacReportInfo(RbacReportInfo *rbacReportInfo) const override { const auto filter_meta = info_.dynamicMetadata().filter_metadata(); const auto filter_it = filter_meta.find(Extensions::HttpFilters::HttpFilterNames::get().Rbac); @@ -132,15 +130,17 @@ class ReportData : public ::istio::control::http::ReportData { const auto &data_struct = filter_it->second; const auto resp_code_it = - data_struct.fields().find(permissive_resp_code_field); + data_struct.fields().find(permissiveRespCodeField); if (resp_code_it != data_struct.fields().end()) { - *resp_code = resp_code_it->second.string_value(); + rbacReportInfo->permissive_resp_code = + resp_code_it->second.string_value(); } const auto policy_id_it = - data_struct.fields().find(permissive_policy_id_field); + data_struct.fields().find(permissivePolicyIDField); if (policy_id_it != data_struct.fields().end()) { - *policy_id = policy_id_it->second.string_value(); + rbacReportInfo->permissive_policy_id = + policy_id_it->second.string_value(); } } }; diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 2c44f56280f..8cd0921470d 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -230,17 +230,15 @@ void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) { builder.AddString(utils::AttributeName::kContextProxyErrorCode, info.response_flags); - std::string rbac_permissive_resp_code; - std::string rbac_permissive_policy_id; - report_data->GetRBACPermissiveAttributes(&rbac_permissive_resp_code, - &rbac_permissive_policy_id); - if (!rbac_permissive_resp_code.empty()) { + ReportData::RbacReportInfo rbac_info; + report_data->GetRbacReportInfo(&rbac_info); + if (!rbac_info.permissive_resp_code.empty()) { builder.AddString(utils::AttributeName::kRbacPermissiveResponseCode, - rbac_permissive_resp_code); + rbac_info.permissive_resp_code); } - if (!rbac_permissive_policy_id.empty()) { + if (!rbac_info.permissive_policy_id.empty()) { builder.AddString(utils::AttributeName::kRbacPermissivePolicyId, - rbac_permissive_policy_id); + rbac_info.permissive_policy_id); } } diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index 4f90fbea20c..db2a1af4aca 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -573,10 +573,10 @@ TEST(AttributesBuilderTest, TestReportAttributes) { status->message = "grpc-message"; return true; })); - EXPECT_CALL(mock_data, GetRBACPermissiveAttributes(_, _)) - .WillOnce(Invoke([](std::string *code, std::string *id) { - *code = "403"; - *id = "policy-foo"; + EXPECT_CALL(mock_data, GetRbacReportInfo(_)) + .WillOnce(Invoke([](ReportData::RbacReportInfo *rbacReportInfo) { + rbacReportInfo->permissive_resp_code = "403"; + rbacReportInfo->permissive_policy_id = "policy-foo"; })); RequestContext request; @@ -627,10 +627,10 @@ TEST(AttributesBuilderTest, TestReportAttributesWithDestIP) { info->response_flags = "NR"; })); EXPECT_CALL(mock_data, GetGrpcStatus(_)).WillOnce(testing::Return(false)); - EXPECT_CALL(mock_data, GetRBACPermissiveAttributes(_, _)) - .WillOnce(Invoke([](std::string *code, std::string *id) { - *code = "403"; - *id = "policy-foo"; + EXPECT_CALL(mock_data, GetRbacReportInfo(_)) + .WillOnce(Invoke([](ReportData::RbacReportInfo *rbacReportInfo) { + rbacReportInfo->permissive_resp_code = "403"; + rbacReportInfo->permissive_policy_id = "policy-foo"; })); RequestContext request; diff --git a/src/istio/control/http/mock_report_data.h b/src/istio/control/http/mock_report_data.h index cdf64b86c01..e4d7e380e4e 100644 --- a/src/istio/control/http/mock_report_data.h +++ b/src/istio/control/http/mock_report_data.h @@ -31,8 +31,7 @@ class MockReportData : public ReportData { MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string* ip, int* port)); MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string* ip)); MOCK_CONST_METHOD1(GetGrpcStatus, bool(GrpcStatus* status)); - MOCK_CONST_METHOD2(GetRBACPermissiveAttributes, - void(std::string* code, std::string* id)); + MOCK_CONST_METHOD1(GetRbacReportInfo, void(RbacReportInfo* info)); }; } // namespace http From 546f253cef98d0b868abd606ef2ec6ea2026955b Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Wed, 8 Aug 2018 16:58:10 -0700 Subject: [PATCH 4/5] address comments --- include/istio/control/http/report_data.h | 3 +-- src/envoy/http/mixer/report_data.h | 21 ++++++++++--------- src/istio/control/http/attributes_builder.cc | 17 ++++++++------- .../control/http/attributes_builder_test.cc | 14 +++++++------ src/istio/control/http/mock_report_data.h | 2 +- 5 files changed, 30 insertions(+), 27 deletions(-) diff --git a/include/istio/control/http/report_data.h b/include/istio/control/http/report_data.h index e9408001604..11b1e7e9f59 100644 --- a/include/istio/control/http/report_data.h +++ b/include/istio/control/http/report_data.h @@ -42,7 +42,6 @@ class ReportData { int response_code; std::string response_flags; }; - virtual void GetReportInfo(ReportInfo* info) const = 0; // Get destination ip/port. @@ -53,7 +52,7 @@ class ReportData { std::string permissive_resp_code; std::string permissive_policy_id; }; - virtual void GetRbacReportInfo(RbacReportInfo* rbacReportInfo) const = 0; + virtual bool GetRbacReportInfo(RbacReportInfo* report_info) const = 0; // Get upstream host UID. This value overrides the value in the report bag. virtual bool GetDestinationUID(std::string* uid) const = 0; diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index 13434bd6329..46b85ed06c6 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -25,8 +25,9 @@ namespace Envoy { namespace Http { namespace Mixer { -static const std::string permissivePolicyIDField = "shadow_effective_policyID"; -static const std::string permissiveRespCodeField = "shadow_response_code"; +static const std::string kRbacPermissivePolicyIDField = + "shadow_effective_policyID"; +static const std::string kRbacPermissiveRespCodeField = "shadow_response_code"; namespace { // Set of headers excluded from response.headers attribute. @@ -120,28 +121,28 @@ class ReportData : public ::istio::control::http::ReportData { } // Get Rbac related attributes. - void GetRbacReportInfo(RbacReportInfo *rbacReportInfo) const override { + bool GetRbacReportInfo(RbacReportInfo *report_info) const override { const auto filter_meta = info_.dynamicMetadata().filter_metadata(); const auto filter_it = filter_meta.find(Extensions::HttpFilters::HttpFilterNames::get().Rbac); if (filter_it == filter_meta.end()) { - return; + return false; } const auto &data_struct = filter_it->second; const auto resp_code_it = - data_struct.fields().find(permissiveRespCodeField); + data_struct.fields().find(kRbacPermissiveRespCodeField); if (resp_code_it != data_struct.fields().end()) { - rbacReportInfo->permissive_resp_code = - resp_code_it->second.string_value(); + report_info->permissive_resp_code = resp_code_it->second.string_value(); } const auto policy_id_it = - data_struct.fields().find(permissivePolicyIDField); + data_struct.fields().find(kRbacPermissivePolicyIDField); if (policy_id_it != data_struct.fields().end()) { - rbacReportInfo->permissive_policy_id = - policy_id_it->second.string_value(); + report_info->permissive_policy_id = policy_id_it->second.string_value(); } + + return true; } }; diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 8cd0921470d..f314545fd66 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -231,14 +231,15 @@ void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) { info.response_flags); ReportData::RbacReportInfo rbac_info; - report_data->GetRbacReportInfo(&rbac_info); - if (!rbac_info.permissive_resp_code.empty()) { - builder.AddString(utils::AttributeName::kRbacPermissiveResponseCode, - rbac_info.permissive_resp_code); - } - if (!rbac_info.permissive_policy_id.empty()) { - builder.AddString(utils::AttributeName::kRbacPermissivePolicyId, - rbac_info.permissive_policy_id); + if (report_data->GetRbacReportInfo(&rbac_info)) { + if (!rbac_info.permissive_resp_code.empty()) { + builder.AddString(utils::AttributeName::kRbacPermissiveResponseCode, + rbac_info.permissive_resp_code); + } + if (!rbac_info.permissive_policy_id.empty()) { + builder.AddString(utils::AttributeName::kRbacPermissivePolicyId, + rbac_info.permissive_policy_id); + } } } diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index db2a1af4aca..dda7defd60e 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -574,9 +574,10 @@ TEST(AttributesBuilderTest, TestReportAttributes) { return true; })); EXPECT_CALL(mock_data, GetRbacReportInfo(_)) - .WillOnce(Invoke([](ReportData::RbacReportInfo *rbacReportInfo) { - rbacReportInfo->permissive_resp_code = "403"; - rbacReportInfo->permissive_policy_id = "policy-foo"; + .WillOnce(Invoke([](ReportData::RbacReportInfo *report_info) -> bool { + report_info->permissive_resp_code = "403"; + report_info->permissive_policy_id = "policy-foo"; + return true; })); RequestContext request; @@ -628,9 +629,10 @@ TEST(AttributesBuilderTest, TestReportAttributesWithDestIP) { })); EXPECT_CALL(mock_data, GetGrpcStatus(_)).WillOnce(testing::Return(false)); EXPECT_CALL(mock_data, GetRbacReportInfo(_)) - .WillOnce(Invoke([](ReportData::RbacReportInfo *rbacReportInfo) { - rbacReportInfo->permissive_resp_code = "403"; - rbacReportInfo->permissive_policy_id = "policy-foo"; + .WillOnce(Invoke([](ReportData::RbacReportInfo *report_info) -> bool { + report_info->permissive_resp_code = "403"; + report_info->permissive_policy_id = "policy-foo"; + return true; })); RequestContext request; diff --git a/src/istio/control/http/mock_report_data.h b/src/istio/control/http/mock_report_data.h index e4d7e380e4e..423a1b6ef4f 100644 --- a/src/istio/control/http/mock_report_data.h +++ b/src/istio/control/http/mock_report_data.h @@ -31,7 +31,7 @@ class MockReportData : public ReportData { MOCK_CONST_METHOD2(GetDestinationIpPort, bool(std::string* ip, int* port)); MOCK_CONST_METHOD1(GetDestinationUID, bool(std::string* ip)); MOCK_CONST_METHOD1(GetGrpcStatus, bool(GrpcStatus* status)); - MOCK_CONST_METHOD1(GetRbacReportInfo, void(RbacReportInfo* info)); + MOCK_CONST_METHOD1(GetRbacReportInfo, bool(RbacReportInfo* info)); }; } // namespace http From 1a0c85f7c0535d8141ad71af55276552d7429048 Mon Sep 17 00:00:00 2001 From: Quanjie Lin Date: Wed, 8 Aug 2018 17:35:14 -0700 Subject: [PATCH 5/5] address comments --- src/envoy/http/mixer/report_data.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index 46b85ed06c6..02013e1f9cb 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -25,11 +25,10 @@ namespace Envoy { namespace Http { namespace Mixer { -static const std::string kRbacPermissivePolicyIDField = - "shadow_effective_policyID"; -static const std::string kRbacPermissiveRespCodeField = "shadow_response_code"; - namespace { +const std::string kRbacPermissivePolicyIDField = "shadow_effective_policyID"; +const std::string kRbacPermissiveRespCodeField = "shadow_response_code"; + // Set of headers excluded from response.headers attribute. const std::set ResponseHeaderExclusives = {}; @@ -142,7 +141,8 @@ class ReportData : public ::istio::control::http::ReportData { report_info->permissive_policy_id = policy_id_it->second.string_value(); } - return true; + return !report_info->permissive_resp_code.empty() || + !report_info->permissive_policy_id.empty(); } };