collect metrics for RBAC shadow policy by quanjielin · Pull Request #4062 · envoyproxy/envoy

quanjielin · 2018-08-06T16:56:46Z

Description:
To complete RBAC policy dark launch from envoy side(#3552), shadow policy metrics needs to be collected(shadow response code + effective policyID); so that we could use those metrics to build report/dashboard to monitor dark launch policy running result.

Risk Level: Low

Signed-off-by: Quanjie Lin quanlin@google.com

lizan · 2018-08-06T18:35:40Z

source/extensions/filters/common/rbac/engine.h

snake_case for all function parameters and local variables (throughout)

lizan · 2018-08-06T18:35:50Z

source/extensions/filters/common/rbac/engine_impl.cc

nit: std::make_pair

lizan · 2018-08-06T18:37:12Z

source/extensions/filters/common/rbac/engine_impl.cc

Do we want to validate if there is a dup of policy ID?

should be fine since rules.policies is map which guarantees no dup.

lizan · 2018-08-06T18:39:01Z

source/extensions/filters/http/rbac/rbac_filter.cc

filter_metadata() is not scoped to each filter, at least you should add .at(HttpFilterNames::get().Rbac) at the end.

lizan · 2018-08-06T18:40:13Z

source/extensions/filters/http/rbac/rbac_filter.cc

make Struct first and then merge once?

lizan

Can you take a look on CI?

lizan · 2018-08-06T22:44:27Z

source/extensions/filters/common/rbac/engine.h

effective_policy_id?

quanjielin · 2018-08-06T23:24:24Z

@lizan , seems the error is due to some files was removed recently, sync to latest master and see.


external/envoy/test/common/http/http2/codec_impl_fuzz_test.cc:14:10: fatal error: 'common/stats/stats_impl.h' file not found
#include "common/stats/stats_impl.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
INFO: Elapsed time: 1157.336s, Critical Path: 54.36s

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin · 2018-08-07T02:32:51Z

/cc @mattklein123 @rodaine

mattklein123

Few small comments, thank you!

mattklein123 · 2018-08-07T04:00:18Z

source/extensions/filters/common/rbac/engine.h

  virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers,
-                       const envoy::api::v2::core::Metadata& metadata) const PURE;
+                       const envoy::api::v2::core::Metadata& metadata,
+                       std::string& effective_policy_id) const PURE;


please update doc comments

added comment for effective_policy_id

mattklein123 · 2018-08-07T04:03:08Z

source/extensions/filters/http/rbac/rbac_filter.cc

  const absl::optional<Filters::Common::RBAC::RoleBasedAccessControlEngineImpl>& shadow_engine =
      config_->engine(callbacks_->route(), EnforcementMode::Shadow);
  if (shadow_engine.has_value()) {
+    std::string shadow_resp_code = "200";


Can we switch these to static constants.

mattklein123 · 2018-08-07T04:03:51Z

source/extensions/filters/http/rbac/rbac_filter.cc

+    if (filter_it != filter_metadata.end()) {
+      ProtobufWkt::Struct metrics;
+
+      if (effective_policy_id != "") {


nit: !effective_policy_id.empty() (can this happen btw, isn't every policy named even if default?)

updated.(when there is no matching policy found in allowed , effective_policy_id here is default to be empty)

mattklein123 · 2018-08-07T04:04:20Z

source/extensions/filters/http/rbac/rbac_filter.cc

+      if (effective_policy_id != "") {
+        ProtobufWkt::Value policy_id;
+        policy_id.set_string_value(effective_policy_id);
+        (*metrics.mutable_fields())["shadow_effective_policyID"] = policy_id;


Static constants for "shadow_effective_policyID" to avoid construction (same below)

mattklein123 · 2018-08-07T04:05:02Z

source/extensions/filters/http/rbac/rbac_filter.cc

  if (engine.has_value()) {
    if (engine->allowed(*callbacks_->connection(), headers,
-                        callbacks_->requestInfo().dynamicMetadata())) {
+                        callbacks_->requestInfo().dynamicMetadata(), effective_policy_id)) {


Why don't we use this value here? Can we avoid cost by passing nullptr and not filling/copying?

we only need effective_policy_id for shadow case for now(can add later if needed); nullptr is invalid since allowed takes std::string& ?

I would make it take std::string* then.

Signed-off-by: Quanjie Lin <quanlin@google.com>

mattklein123

LGTM, small comment. Thanks!

mattklein123 · 2018-08-07T18:08:45Z

source/extensions/filters/common/rbac/engine.h

   * @param headers    the headers of the incoming request used to identify the action/principal. An
   *                   empty map should be used if there are no headers available.
   * @param metadata   the metadata with additional information about the action/principal.
+   * @param effective_policy_id  the matching policy's ID to identity the source of the allow/deny.


Can you specify that this will be filled with the policy ID? It sounds like it's an input parameter.

updated comment; thank you for the review !

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin · 2018-08-07T21:50:42Z

/cc @liminw @qiwzhang @diemtvu @yangminzhu FYI.

quanjielin force-pushed the quanlinmetric2 branch 3 times, most recently from 2c46cd5 to f1b007d Compare August 6, 2018 17:11

lizan suggested changes Aug 6, 2018

View reviewed changes

quanjielin force-pushed the quanlinmetric2 branch 2 times, most recently from 9840cd8 to 0b97f68 Compare August 6, 2018 21:29

lizan reviewed Aug 6, 2018

View reviewed changes

quanjielin added 3 commits August 6, 2018 18:03

collect metrics for RBAC shadow policy

c56caa1

Signed-off-by: Quanjie Lin <quanlin@google.com>

address comment

eae1cad

Signed-off-by: Quanjie Lin <quanlin@google.com>

rename

9d70d38

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin force-pushed the quanlinmetric2 branch from 1eb2e8c to 9d70d38 Compare August 7, 2018 01:03

lizan previously approved these changes Aug 7, 2018

View reviewed changes

mattklein123 reviewed Aug 7, 2018

View reviewed changes

alyssawilk assigned rodaine and mattklein123 Aug 7, 2018

address comments

fabbe92

Signed-off-by: Quanjie Lin <quanlin@google.com>

quanjielin dismissed lizan’s stale review via fabbe92 August 7, 2018 16:30

address comments

287d3a6

Signed-off-by: Quanjie Lin <quanlin@google.com>

mattklein123 reviewed Aug 7, 2018

View reviewed changes

update comment

1005003

Signed-off-by: Quanjie Lin <quanlin@google.com>

mattklein123 approved these changes Aug 7, 2018

View reviewed changes

mattklein123 merged commit e9dc109 into envoyproxy:master Aug 7, 2018

quanjielin mentioned this pull request Aug 8, 2018

send rbac shadow policies metrics to mixer istio/proxy#1900

Merged

quanjielin mentioned this pull request Aug 27, 2018

Supporting "dark launch" Mode for Security policies istio/istio#5345

Closed

Conversation

quanjielin commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quanjielin commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quanjielin commented Aug 7, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quanjielin commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

quanjielin commented Aug 6, 2018 •

edited

Loading

quanjielin commented Aug 6, 2018 •

edited

Loading