Coalesce all memory for checks and reports into shared pointers

include/istio/mixerclient/check_response.h

@@ -22,21 +22,15 @@
 namespace istio {
 namespace mixerclient {
 
-// The CheckResponseInfo holds response information in detail.
-struct CheckResponseInfo {
-  // Whether this check response is from cache.
-  bool is_check_cache_hit{false};
+// The CheckResponseInfo exposes policy and quota check details to the check
+// callbacks.
+class CheckResponseInfo {
+ public:
+  virtual ~CheckResponseInfo(){};
 
-  // Whether this quota response is from cache.
-  bool is_quota_cache_hit{false};
+  virtual const ::google::protobuf::util::Status& status() const = 0;
 
-  // The check and quota response status.
-  ::google::protobuf::util::Status response_status{
-      ::google::protobuf::util::Status::UNKNOWN};
-
-  // Routing directive (applicable if the status is OK)
-  ::istio::mixer::v1::RouteDirective route_directive{
-      ::istio::mixer::v1::RouteDirective::default_instance()};
+  virtual const ::istio::mixer::v1::RouteDirective& routeDirective() const = 0;
 };
 
 }  // namespace mixerclient

include/istio/mixerclient/client.h

@@ -19,6 +19,8 @@
 #include "environment.h"
 #include "include/istio/quota_config/requirement.h"
 #include "options.h"
+#include "src/istio/mixerclient/check_context.h"
+#include "src/istio/mixerclient/shared_attributes.h"
 
 #include <vector>
 
@@ -84,13 +86,13 @@ class MixerClient {
   // The response data from mixer will be consumed by mixer client.
 
   // A check call.
-  virtual CancelFunc Check(
-      const ::istio::mixer::v1::Attributes& attributes,
-      const std::vector<::istio::quota_config::Requirement>& quotas,
-      TransportCheckFunc transport, CheckDoneFunc on_done) = 0;
+  virtual CancelFunc Check(istio::mixerclient::CheckContextSharedPtr& context,
+                           TransportCheckFunc transport,
+                           CheckDoneFunc on_done) = 0;
 
   // A report call.
-  virtual void Report(const ::istio::mixer::v1::Attributes& attributes) = 0;
+  virtual void Report(
+      const istio::mixerclient::SharedAttributesSharedPtr& attributes) = 0;
 
   // Get statistics.
   virtual void GetStatistics(Statistics* stat) const = 0;

include/istio/utils/attributes_builder.h

@@ -162,6 +162,8 @@ class AttributesBuilder {
     return *filters;
   }
 
+  // TODO(jblatt) audit all uses of raw pointers and replace as many as possible
+  // with unique/shared pointers.
   ::istio::mixer::v1::Attributes *attributes_;
 };
 

include/istio/utils/simple_lru_cache_inl.h

@@ -497,7 +497,7 @@ class SimpleLRUCacheBase {
   // so we implement it in the derived SimpleLRUCache.
   virtual void RemoveElement(const Key& k, Value* value) = 0;
 
-  virtual void DebugIterator(const Key& k, const Value* value, int pin_count,
+  virtual void DebugIterator(const Key&, const Value* value, int pin_count,
                              int64_t last_timestamp, bool is_deferred,
                              std::string* output) const {
     std::stringstream ss;
@@ -1054,7 +1054,7 @@ class SimpleLRUCache
             EQ>(total_units) {}
 
  protected:
-  virtual void RemoveElement(const Key& k, Value* value) { delete value; }
+  virtual void RemoveElement(const Key&, Value* value) { delete value; }
 
  private:
   GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(SimpleLRUCache);
@@ -1077,7 +1077,7 @@ class SimpleLRUCacheWithDeleter
       : Base(total_units), deleter_(deleter) {}
 
  protected:
-  virtual void RemoveElement(const Key& k, Value* value) { deleter_(value); }
+  virtual void RemoveElement(const Key&, Value* value) { deleter_(value); }
 
  private:
   Deleter deleter_;

src/envoy/http/mixer/filter.cc

@@ -24,6 +24,7 @@
 #include "src/envoy/utils/header_update.h"
 
 using ::google::protobuf::util::Status;
+using ::istio::mixer::v1::RouteDirective;
 using ::istio::mixerclient::CheckResponseInfo;
 
 namespace Envoy {
@@ -147,15 +148,16 @@ void Filter::setDecoderFilterCallbacks(
 }
 
 void Filter::completeCheck(const CheckResponseInfo& info) {
-  auto status = info.response_status;
+  const Status& status = info.status();
+
   ENVOY_LOG(debug, "Called Mixer::Filter : check complete {}",
             status.ToString());
   // This stream has been reset, abort the callback.
   if (state_ == Responded) {
     return;
   }
 
-  route_directive_ = info.route_directive;
+  route_directive_ = info.routeDirective();
 
   Utils::CheckResponseInfoToStreamInfo(info, decoder_callbacks_->streamInfo());
 

src/envoy/tcp/mixer/filter.cc

@@ -138,7 +138,7 @@ Network::FilterStatus Filter::onNewConnection() {
 }
 
 void Filter::completeCheck(const CheckResponseInfo &info) {
-  const auto &status = info.response_status;
+  const auto &status = info.status();
   ENVOY_LOG(debug, "Called tcp filter completeCheck: {}", status.ToString());
   cancel_check_ = nullptr;
   if (state_ == State::Closed) {

src/envoy/utils/utils.cc

@@ -143,13 +143,12 @@ Status ParseJsonMessage(const std::string& json, Message* output) {
 void CheckResponseInfoToStreamInfo(
     const istio::mixerclient::CheckResponseInfo& check_response,
     StreamInfo::StreamInfo& stream_info) {
-  if (!check_response.response_status.ok()) {
+  if (!check_response.status().ok()) {
     stream_info.setResponseFlag(
         StreamInfo::ResponseFlag::UnauthorizedExternalService);
     ProtobufWkt::Struct metadata;
     auto& fields = *metadata.mutable_fields();
-    fields["status"].set_string_value(
-        check_response.response_status.ToString());
+    fields["status"].set_string_value(check_response.status().ToString());
     stream_info.setDynamicMetadata(istio::utils::kMixerMetadataKey, metadata);
   }
 }

src/envoy/utils/utils_test.cc

@@ -15,6 +15,7 @@
 
 #include "src/envoy/utils/utils.h"
 #include "mixer/v1/config/client/client_config.pb.h"
+#include "src/istio/mixerclient/check_context.h"
 #include "test/mocks/stream_info/mocks.h"
 #include "test/test_common/utility.h"
 
@@ -48,8 +49,9 @@ TEST(UtilsTest, ParseMessageWithUnknownField) {
 }
 
 TEST(UtilsTest, CheckResponseInfoToStreamInfo) {
-  ::istio::mixerclient::CheckResponseInfo
-      check_response;  // by default status is unknown
+  auto attributes = std::make_shared<::istio::mixerclient::SharedAttributes>();
+  ::istio::mixerclient::CheckContext check_response(
+      false /* fail_open */, attributes);  // by default status is unknown
   Envoy::StreamInfo::MockStreamInfo mock_stream_info;
 
   EXPECT_CALL(

src/istio/control/BUILD

@@ -21,7 +21,6 @@ cc_library(
     ],
     hdrs = [
         "client_context_base.h",
-        "request_context.h",
     ],
     visibility = [":__subpackages__"],
     deps = [

src/istio/control/client_context_base.cc

@@ -80,37 +80,22 @@ ClientContextBase::ClientContextBase(const TransportConfig& config,
   options.env = env;
   mixer_client_ = ::istio::mixerclient::CreateMixerClient(options);
   CreateLocalAttributes(local_node, &local_attributes_);
+  network_fail_open_ = options.check_options.network_fail_open;
 }
 
-CancelFunc ClientContextBase::SendCheck(TransportCheckFunc transport,
-                                        CheckDoneFunc on_done,
-                                        RequestContext* request) {
-  // Intercept the callback to save check status in request_context
-  auto local_on_done = [request,
-                        on_done](const CheckResponseInfo& check_response_info) {
-    // save the check status code
-    request->check_status = check_response_info.response_status;
-
-    utils::AttributesBuilder builder(request->attributes);
-    builder.AddBool(utils::AttributeName::kCheckCacheHit,
-                    check_response_info.is_check_cache_hit);
-    builder.AddBool(utils::AttributeName::kQuotaCacheHit,
-                    check_response_info.is_quota_cache_hit);
-    on_done(check_response_info);
-  };
-
+CancelFunc ClientContextBase::SendCheck(
+    const TransportCheckFunc& transport, const CheckDoneFunc& on_done,
+    ::istio::mixerclient::CheckContextSharedPtr& context) {
   MIXER_DEBUG("Check attributes: %s",
-              request->attributes->DebugString().c_str());
-
-  return mixer_client_->Check(*request->attributes, request->quotas, transport,
-                              local_on_done);
+              context->attributes()->DebugString().c_str());
+  return mixer_client_->Check(context, transport, on_done);
 }
 
-void ClientContextBase::SendReport(const RequestContext& request) {
+void ClientContextBase::SendReport(
+    const istio::mixerclient::SharedAttributesSharedPtr& attributes) {
   MIXER_DEBUG("Report attributes: %s",
-              request.attributes->DebugString().c_str());
-
-  mixer_client_->Report(*request.attributes);
+              attributes->attributes()->DebugString().c_str());
+  mixer_client_->Report(attributes);
 }
 
 void ClientContextBase::GetStatistics(Statistics* stat) const {

src/istio/control/client_context_base.h

@@ -20,7 +20,8 @@
 #include "include/istio/utils/attribute_names.h"
 #include "include/istio/utils/local_attributes.h"
 #include "mixer/v1/config/client/client_config.pb.h"
-#include "request_context.h"
+#include "src/istio/mixerclient/check_context.h"
+#include "src/istio/mixerclient/shared_attributes.h"
 
 namespace istio {
 namespace control {
@@ -40,17 +41,20 @@ class ClientContextBase {
       bool outbound, ::istio::utils::LocalAttributes& local_attributes)
       : mixer_client_(std::move(mixer_client)),
         outbound_(outbound),
-        local_attributes_(local_attributes) {}
+        local_attributes_(local_attributes),
+        network_fail_open_(false) {}
   // virtual destrutor
   virtual ~ClientContextBase() {}
 
   // Use mixer client object to make a Check call.
   ::istio::mixerclient::CancelFunc SendCheck(
-      ::istio::mixerclient::TransportCheckFunc transport,
-      ::istio::mixerclient::CheckDoneFunc on_done, RequestContext* request);
+      const ::istio::mixerclient::TransportCheckFunc& transport,
+      const ::istio::mixerclient::CheckDoneFunc& on_done,
+      ::istio::mixerclient::CheckContextSharedPtr& check_context);
 
   // Use mixer client object to make a Report call.
-  void SendReport(const RequestContext& request);
+  void SendReport(
+      const istio::mixerclient::SharedAttributesSharedPtr& attributes);
 
   // Get statistics.
   void GetStatistics(::istio::mixerclient::Statistics* stat) const;
@@ -60,6 +64,8 @@ class ClientContextBase {
   void AddLocalNodeForwardAttribues(
       ::istio::mixer::v1::Attributes* request) const;
 
+  bool NetworkFailOpen() const { return network_fail_open_; }
+
  private:
   // The mixer client object with check cache and report batch features.
   std::unique_ptr<::istio::mixerclient::MixerClient> mixer_client_;
@@ -69,6 +75,8 @@ class ClientContextBase {
 
   // local attributes - owned by the client context.
   ::istio::utils::LocalAttributes local_attributes_;
+
+  bool network_fail_open_;
 };
 
 }  // namespace control

src/istio/control/http/attributes_builder.cc

@@ -17,6 +17,7 @@
 
 #include <set>
 
+#include "google/protobuf/stubs/status.h"
 #include "include/istio/utils/attribute_names.h"
 #include "include/istio/utils/attributes_builder.h"
 #include "include/istio/utils/status.h"
@@ -35,7 +36,7 @@ const std::set<std::string> kGrpcContentTypes{
 }  // namespace
 
 void AttributesBuilder::ExtractRequestHeaderAttributes(CheckData *check_data) {
-  utils::AttributesBuilder builder(request_->attributes);
+  utils::AttributesBuilder builder(attributes_);
   std::map<std::string, std::string> headers = check_data->GetRequestHeaders();
   builder.AddStringMap(utils::AttributeName::kRequestHeaders, headers);
 
@@ -79,7 +80,7 @@ void AttributesBuilder::ExtractRequestHeaderAttributes(CheckData *check_data) {
 }
 
 void AttributesBuilder::ExtractAuthAttributes(CheckData *check_data) {
-  utils::AttributesBuilder builder(request_->attributes);
+  utils::AttributesBuilder builder(attributes_);
 
   std::string destination_principal;
   if (check_data->GetPrincipal(false, &destination_principal)) {
@@ -146,7 +147,7 @@ void AttributesBuilder::ExtractForwardedAttributes(CheckData *check_data) {
   };
 
   auto fwd = v2_format.attributes();
-  utils::AttributesBuilder builder(request_->attributes);
+  utils::AttributesBuilder builder(attributes_);
   for (const auto &attribute : kForwardWhitelist) {
     const auto &iter = fwd.find(attribute);
     if (iter != fwd.end() && !iter->second.string_value().empty()) {
@@ -161,7 +162,7 @@ void AttributesBuilder::ExtractCheckAttributes(CheckData *check_data) {
   ExtractRequestHeaderAttributes(check_data);
   ExtractAuthAttributes(check_data);
 
-  utils::AttributesBuilder builder(request_->attributes);
+  utils::AttributesBuilder builder(attributes_);
 
   // connection remote IP is always reported as origin IP
   std::string source_ip;
@@ -200,8 +201,9 @@ void AttributesBuilder::ForwardAttributes(const Attributes &forward_attributes,
   header_update->AddIstioAttributes(str);
 }
 
-void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) {
-  utils::AttributesBuilder builder(request_->attributes);
+void AttributesBuilder::ExtractReportAttributes(
+    const ::google::protobuf::util::Status &status, ReportData *report_data) {
+  utils::AttributesBuilder builder(attributes_);
 
   std::string dest_ip;
   int dest_port;
@@ -238,14 +240,13 @@ void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) {
   builder.AddInt64(utils::AttributeName::kResponseTotalSize,
                    info.response_total_size);
   builder.AddDuration(utils::AttributeName::kResponseDuration, info.duration);
-  if (!request_->check_status.ok()) {
-    builder.AddInt64(
-        utils::AttributeName::kResponseCode,
-        utils::StatusHttpCode(request_->check_status.error_code()));
+  if (status != ::google::protobuf::util::Status::UNKNOWN && !status.ok()) {
+    builder.AddInt64(utils::AttributeName::kResponseCode,
+                     utils::StatusHttpCode(status.error_code()));
     builder.AddInt64(utils::AttributeName::kCheckErrorCode,
-                     request_->check_status.error_code());
+                     status.error_code());
     builder.AddString(utils::AttributeName::kCheckErrorMessage,
-                      request_->check_status.ToString());
+                      status.ToString());
   } else {
     builder.AddInt64(utils::AttributeName::kResponseCode, info.response_code);
   }

src/istio/control/http/attributes_builder.h

@@ -18,7 +18,7 @@
 
 #include "include/istio/control/http/check_data.h"
 #include "include/istio/control/http/report_data.h"
-#include "src/istio/control/request_context.h"
+#include "mixer/v1/attributes.pb.h"
 
 namespace istio {
 namespace control {
@@ -27,7 +27,8 @@ namespace http {
 // The context for each HTTP request.
 class AttributesBuilder {
  public:
-  AttributesBuilder(RequestContext* request) : request_(request) {}
+  AttributesBuilder(istio::mixer::v1::Attributes* attributes)
+      : attributes_(attributes) {}
 
   // Extract forwarded attributes from HTTP header.
   void ExtractForwardedAttributes(CheckData* check_data);
@@ -39,7 +40,8 @@ class AttributesBuilder {
   // Extract attributes for Check call.
   void ExtractCheckAttributes(CheckData* check_data);
   // Extract attributes for Report call.
-  void ExtractReportAttributes(ReportData* report_data);
+  void ExtractReportAttributes(const ::google::protobuf::util::Status& status,
+                               ReportData* report_data);
 
  private:
   // Extract HTTP header attributes
@@ -52,8 +54,7 @@ class AttributesBuilder {
   // not available.
   void ExtractAuthAttributes(CheckData* check_data);
 
-  // The request context object.
-  RequestContext* request_;
+  istio::mixer::v1::Attributes* attributes_;
 };
 
 }  // namespace http