Route directive (envoyproxy#1797)

* wire through route directive Signed-off-by: Kuat Yessenov <[email protected]> * fix format Signed-off-by: Kuat Yessenov <[email protected]> * propagate info Signed-off-by: Kuat Yessenov <[email protected]> * review Signed-off-by: Kuat Yessenov <[email protected]> * implement the API Signed-off-by: Kuat Yessenov <[email protected]> * manual test Signed-off-by: Kuat Yessenov <[email protected]> * review Signed-off-by: Kuat Yessenov <[email protected]> * fix upstream api change Signed-off-by: Kuat Yessenov <[email protected]> * add a test case Signed-off-by: Kuat Yessenov <[email protected]> * review, handle direct response headers Signed-off-by: Kuat Yessenov <[email protected]>
rshriram · Jun 7, 2018 · 0f38765 · 0f38765
1 parent e641d50
commit 0f38765
Show file tree

Hide file tree

Showing 25 changed files with 180 additions and 38 deletions.
diff --git a/include/istio/control/http/request_handler.h b/include/istio/control/http/request_handler.h
@@ -38,7 +38,7 @@ class RequestHandler {
   virtual ::istio::mixerclient::CancelFunc Check(
       CheckData* check_data, HeaderUpdate* header_update,
       ::istio::mixerclient::TransportCheckFunc transport,
-      ::istio::mixerclient::DoneFunc on_done) = 0;
+      ::istio::mixerclient::CheckDoneFunc on_done) = 0;
 
   // Make a Report call. It will:
   // * check service config to see if Report is required

diff --git a/include/istio/control/tcp/request_handler.h b/include/istio/control/tcp/request_handler.h
@@ -33,7 +33,7 @@ class RequestHandler {
   // * extract downstream tcp connection attributes
   // * check config, make a Check call if necessary.
   virtual ::istio::mixerclient::CancelFunc Check(
-      CheckData* check_data, ::istio::mixerclient::DoneFunc on_done) = 0;
+      CheckData* check_data, ::istio::mixerclient::CheckDoneFunc on_done) = 0;
 
   // Make report call.
   // This can be called multiple times for long connection.

diff --git a/include/istio/mixerclient/BUILD b/include/istio/mixerclient/BUILD
@@ -25,6 +25,7 @@ cc_library(
     ],
     visibility = ["//visibility:public"],
     deps = [
+        "//external:mixer_api_cc_proto",
         "//include/istio/quota_config:requirement_header",
     ],
 )
diff --git a/include/istio/mixerclient/check_response.h b/include/istio/mixerclient/check_response.h
@@ -17,6 +17,7 @@
 #define ISTIO_MIXERCLIENT_CHECK_RESPONSE_H
 
 #include "google/protobuf/stubs/status.h"
+#include "mixer/v1/check.pb.h"
 
 namespace istio {
 namespace mixerclient {
@@ -32,6 +33,10 @@ struct CheckResponseInfo {
   // 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()};
 };
 
 }  // namespace mixerclient

diff --git a/istio.deps b/istio.deps
@@ -4,7 +4,7 @@
 		"name": "ISTIO_API",
 		"repoName": "api",
 		"file": "repositories.bzl",
-		"lastStableSHA": "ecef45ec0f0ef17c4b383ee84dcbcdba4fcc0c8d"
+		"lastStableSHA": "f403f2ff3a7ee656a1bfbf23e66966a633160300",
 	},
 	{
 		"_comment": "",

diff --git a/repositories.bzl b/repositories.bzl
@@ -113,7 +113,7 @@ cc_library(
             actual = "@googletest_git//:googletest_prod",
         )
 
-ISTIO_API = "c747495e9419606fa1febe210a7626e177b1935c"
+ISTIO_API = "f403f2ff3a7ee656a1bfbf23e66966a633160300"
 
 def mixerapi_repositories(bind=True):
     BUILD = """

diff --git a/src/envoy/BUILD b/src/envoy/BUILD
@@ -40,4 +40,5 @@ pkg_tar(
     files = [":envoy"],
     mode = "0755",
     package_dir = "/usr/local/bin/",
+    tags = ["manual"],
 )
diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc
@@ -24,6 +24,7 @@
 
 using ::google::protobuf::util::Status;
 using ::istio::mixer::v1::config::client::ServiceConfig;
+using ::istio::mixerclient::CheckResponseInfo;
 
 namespace Envoy {
 namespace Http {
@@ -135,7 +136,7 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) {
   cancel_check_ = handler_->Check(
       &check_data, &header_update,
       control_.GetCheckTransport(decoder_callbacks_->activeSpan()),
-      [this](const Status& status) { completeCheck(status); });
+      [this](const CheckResponseInfo& info) { completeCheck(info); });
   initiating_call_ = false;
 
   if (state_ == Complete) {
@@ -164,26 +165,70 @@ FilterTrailersStatus Filter::decodeTrailers(HeaderMap& trailers) {
   return FilterTrailersStatus::Continue;
 }
 
+void Filter::UpdateHeaders(
+    HeaderMap& headers, const ::google::protobuf::RepeatedPtrField<
+                            ::istio::mixer::v1::HeaderOperation>& operations) {
+  for (auto const iter : operations) {
+    switch (iter.operation()) {
+      case ::istio::mixer::v1::HeaderOperation_Operation_REPLACE:
+        headers.remove(LowerCaseString(iter.name()));
+        headers.addCopy(LowerCaseString(iter.name()), iter.value());
+        break;
+      case ::istio::mixer::v1::HeaderOperation_Operation_REMOVE:
+        headers.remove(LowerCaseString(iter.name()));
+        break;
+      case ::istio::mixer::v1::HeaderOperation_Operation_APPEND:
+        headers.addCopy(LowerCaseString(iter.name()), iter.value());
+        break;
+      default:
+        PANIC("unreachable header operation");
+    }
+  }
+}
+
+FilterHeadersStatus Filter::encodeHeaders(HeaderMap& headers, bool) {
+  ENVOY_LOG(debug, "Called Mixer::Filter : {} {}", __func__, state_);
+  ASSERT(state_ == Complete || state_ == Responded);
+  if (state_ == Complete) {
+    // handle response header operations
+    UpdateHeaders(headers, route_directive_.response_header_operations());
+  }
+  return FilterHeadersStatus::Continue;
+}
+
+FilterDataStatus Filter::encodeData(Buffer::Instance&, bool) {
+  ENVOY_LOG(debug, "Called Mixer::Filter : {}", __func__);
+  ASSERT(state_ == Complete || state_ == Responded);
+  return FilterDataStatus::Continue;
+}
+
+FilterTrailersStatus Filter::encodeTrailers(HeaderMap&) {
+  ENVOY_LOG(debug, "Called Mixer::Filter : {}", __func__);
+  ASSERT(state_ == Complete || state_ == Responded);
+  return FilterTrailersStatus::Continue;
+}
+
 void Filter::setDecoderFilterCallbacks(
     StreamDecoderFilterCallbacks& callbacks) {
   ENVOY_LOG(debug, "Called Mixer::Filter : {}", __func__);
   decoder_callbacks_ = &callbacks;
 }
 
-void Filter::completeCheck(const Status& status) {
+void Filter::completeCheck(const CheckResponseInfo& info) {
+  auto status = info.response_status;
   ENVOY_LOG(debug, "Called Mixer::Filter : check complete {}",
             status.ToString());
   // Remove Istio authentication header after Check() is completed
   if (nullptr != headers_) {
     Envoy::Utils::Authentication::ClearResultInHeader(headers_);
-    headers_ = nullptr;
   }
 
   // This stream has been reset, abort the callback.
   if (state_ == Responded) {
     return;
   }
-  if (!status.ok() && state_ != Responded) {
+
+  if (!status.ok()) {
     state_ = Responded;
     int status_code = ::istio::utils::StatusHttpCode(status.error_code());
     decoder_callbacks_->sendLocalReply(Code(status_code), status.ToString(),
@@ -192,6 +237,26 @@ void Filter::completeCheck(const Status& status) {
   }
 
   state_ = Complete;
+  route_directive_ = info.route_directive;
+
+  // handle direct response from the route directive
+  if (status.ok() && route_directive_.direct_response_code() != 0) {
+    ENVOY_LOG(debug, "Mixer::Filter direct response");
+    state_ = Responded;
+    decoder_callbacks_->sendLocalReply(
+        Code(route_directive_.direct_response_code()),
+        route_directive_.direct_response_body(), [this](HeaderMap& headers) {
+          UpdateHeaders(headers, route_directive_.response_header_operations());
+        });
+    return;
+  }
+
+  // handle request header operations
+  if (nullptr != headers_) {
+    UpdateHeaders(*headers_, route_directive_.request_header_operations());
+    headers_ = nullptr;
+  }
+
   if (!initiating_call_) {
     decoder_callbacks_->continueDecoding();
   }

diff --git a/src/envoy/http/mixer/filter.h b/src/envoy/http/mixer/filter.h
@@ -33,13 +33,13 @@ struct PerRouteServiceConfig : public Router::RouteSpecificFilterConfig {
   std::string hash;
 };
 
-class Filter : public Http::StreamDecoderFilter,
+class Filter : public StreamFilter,
                public AccessLog::Instance,
                public Logger::Loggable<Logger::Id::filter> {
  public:
   Filter(Control& control);
 
-  // Implementing virtual functions for StreamDecoderFilter
+  // Http::StreamDecoderFilter
   FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool) override;
   FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override;
   FilterTrailersStatus decodeTrailers(HeaderMap& trailers) override;
@@ -49,8 +49,17 @@ class Filter : public Http::StreamDecoderFilter,
   // Http::StreamFilterBase
   void onDestroy() override;
 
+  // Http::StreamEncoderFilter
+  FilterHeadersStatus encode100ContinueHeaders(HeaderMap&) override {
+    return FilterHeadersStatus::Continue;
+  }
+  FilterHeadersStatus encodeHeaders(HeaderMap& headers, bool) override;
+  FilterDataStatus encodeData(Buffer::Instance&, bool) override;
+  FilterTrailersStatus encodeTrailers(HeaderMap&) override;
+  void setEncoderFilterCallbacks(StreamEncoderFilterCallbacks&) override {}
+
   // This is the callback function when Check is done.
-  void completeCheck(const ::google::protobuf::util::Status& status);
+  void completeCheck(const ::istio::mixerclient::CheckResponseInfo& info);
 
   // Called when the request is completed.
   virtual void log(const HeaderMap* request_headers,
@@ -64,6 +73,11 @@ class Filter : public Http::StreamDecoderFilter,
       const Router::RouteEntry* entry,
       ::istio::control::http::Controller::PerRouteConfig* config);
 
+  // Update header maps
+  void UpdateHeaders(HeaderMap& headers,
+                     const ::google::protobuf::RepeatedPtrField<
+                         ::istio::mixer::v1::HeaderOperation>& operations);
+
   // The control object.
   Control& control_;
   // The request handler.
@@ -85,6 +99,10 @@ class Filter : public Http::StreamDecoderFilter,
 
   // The stream decoder filter callback.
   StreamDecoderFilterCallbacks* decoder_callbacks_{nullptr};
+
+  // Returned directive
+  ::istio::mixer::v1::RouteDirective route_directive_{
+      ::istio::mixer::v1::RouteDirective::default_instance()};
 };
 
 }  // namespace Mixer

diff --git a/src/envoy/http/mixer/filter_factory.cc b/src/envoy/http/mixer/filter_factory.cc
@@ -83,8 +83,7 @@ class MixerConfigFactory : public NamedHttpFilterConfigFactory {
                Http::FilterChainFactoryCallbacks& callbacks) -> void {
       std::shared_ptr<Http::Mixer::Filter> instance =
           std::make_shared<Http::Mixer::Filter>(control_factory->control());
-      callbacks.addStreamDecoderFilter(
-          Http::StreamDecoderFilterSharedPtr(instance));
+      callbacks.addStreamFilter(Http::StreamFilterSharedPtr(instance));
       callbacks.addAccessLogHandler(AccessLog::InstanceSharedPtr(instance));
     };
   }

diff --git a/src/envoy/tcp/mixer/filter.cc b/src/envoy/tcp/mixer/filter.cc
@@ -18,6 +18,7 @@
 #include "src/envoy/utils/utils.h"
 
 using ::google::protobuf::util::Status;
+using ::istio::mixerclient::CheckResponseInfo;
 
 namespace Envoy {
 namespace Tcp {
@@ -59,8 +60,9 @@ void Filter::callCheck() {
   state_ = State::Calling;
   filter_callbacks_->connection().readDisable(true);
   calling_check_ = true;
-  cancel_check_ = handler_->Check(
-      this, [this](const Status& status) { completeCheck(status); });
+  cancel_check_ = handler_->Check(this, [this](const CheckResponseInfo& info) {
+    completeCheck(info.response_status);
+  });
   calling_check_ = false;
 }
 

diff --git a/src/istio/control/client_context_base.cc b/src/istio/control/client_context_base.cc
@@ -22,9 +22,9 @@ using ::google::protobuf::util::Status;
 using ::istio::mixer::v1::config::client::NetworkFailPolicy;
 using ::istio::mixer::v1::config::client::TransportConfig;
 using ::istio::mixerclient::CancelFunc;
+using ::istio::mixerclient::CheckDoneFunc;
 using ::istio::mixerclient::CheckOptions;
 using ::istio::mixerclient::CheckResponseInfo;
-using ::istio::mixerclient::DoneFunc;
 using ::istio::mixerclient::Environment;
 using ::istio::mixerclient::MixerClientOptions;
 using ::istio::mixerclient::QuotaOptions;
@@ -77,7 +77,7 @@ ClientContextBase::ClientContextBase(const TransportConfig& config,
 }
 
 CancelFunc ClientContextBase::SendCheck(TransportCheckFunc transport,
-                                        DoneFunc on_done,
+                                        CheckDoneFunc on_done,
                                         RequestContext* request) {
   // Intercept the callback to save check status in request_context
   auto local_on_done = [request,
@@ -90,7 +90,7 @@ CancelFunc ClientContextBase::SendCheck(TransportCheckFunc transport,
                     check_response_info.is_check_cache_hit);
     builder.AddBool(AttributeName::kQuotaCacheHit,
                     check_response_info.is_quota_cache_hit);
-    on_done(check_response_info.response_status);
+    on_done(check_response_info);
   };
 
   // TODO: add debug message

diff --git a/src/istio/control/client_context_base.h b/src/istio/control/client_context_base.h
@@ -41,7 +41,7 @@ class ClientContextBase {
   // Use mixer client object to make a Check call.
   ::istio::mixerclient::CancelFunc SendCheck(
       ::istio::mixerclient::TransportCheckFunc transport,
-      ::istio::mixerclient::DoneFunc on_done, RequestContext* request);
+      ::istio::mixerclient::CheckDoneFunc on_done, RequestContext* request);
 
   // Use mixer client object to make a Report call.
   void SendReport(const RequestContext& request);

diff --git a/src/istio/control/http/request_handler_impl.cc b/src/istio/control/http/request_handler_impl.cc
@@ -18,7 +18,8 @@
 
 using ::google::protobuf::util::Status;
 using ::istio::mixerclient::CancelFunc;
-using ::istio::mixerclient::DoneFunc;
+using ::istio::mixerclient::CheckDoneFunc;
+using ::istio::mixerclient::CheckResponseInfo;
 using ::istio::mixerclient::TransportCheckFunc;
 using ::istio::quota_config::Requirement;
 
@@ -46,13 +47,15 @@ void RequestHandlerImpl::ExtractRequestAttributes(CheckData* check_data) {
 CancelFunc RequestHandlerImpl::Check(CheckData* check_data,
                                      HeaderUpdate* header_update,
                                      TransportCheckFunc transport,
-                                     DoneFunc on_done) {
+                                     CheckDoneFunc on_done) {
   ExtractRequestAttributes(check_data);
   header_update->RemoveIstioAttributes();
   service_context_->InjectForwardedAttributes(header_update);
 
   if (!service_context_->enable_mixer_check()) {
-    on_done(Status::OK);
+    CheckResponseInfo check_response_info;
+    check_response_info.response_status = Status::OK;
+    on_done(check_response_info);
     return nullptr;
   }
 

diff --git a/src/istio/control/http/request_handler_impl.h b/src/istio/control/http/request_handler_impl.h
@@ -34,7 +34,7 @@ class RequestHandlerImpl : public RequestHandler {
   ::istio::mixerclient::CancelFunc Check(
       CheckData* check_data, HeaderUpdate* header_update,
       ::istio::mixerclient::TransportCheckFunc transport,
-      ::istio::mixerclient::DoneFunc on_done) override;
+      ::istio::mixerclient::CheckDoneFunc on_done) override;
 
   // Make a Report call.
   void Report(ReportData* report_data) override;

diff --git a/src/istio/control/http/request_handler_impl_test.cc b/src/istio/control/http/request_handler_impl_test.cc
@@ -29,6 +29,7 @@ using ::istio::mixer::v1::config::client::HttpClientConfig;
 using ::istio::mixer::v1::config::client::ServiceConfig;
 using ::istio::mixerclient::CancelFunc;
 using ::istio::mixerclient::CheckDoneFunc;
+using ::istio::mixerclient::CheckResponseInfo;
 using ::istio::mixerclient::DoneFunc;
 using ::istio::mixerclient::MixerClient;
 using ::istio::mixerclient::TransportCheckFunc;
@@ -162,7 +163,9 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheckReport) {
 
   auto handler = controller_->CreateRequestHandler(per_route);
   handler->Check(&mock_data, &mock_header, nullptr,
-                 [](const Status& status) { EXPECT_TRUE(status.ok()); });
+                 [](const CheckResponseInfo& info) {
+                   EXPECT_TRUE(info.response_status.ok());
+                 });
 }
 
 TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) {
@@ -182,7 +185,9 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledCheck) {
 
   auto handler = controller_->CreateRequestHandler(per_route);
   handler->Check(&mock_data, &mock_header, nullptr,
-                 [](const Status& status) { EXPECT_TRUE(status.ok()); });
+                 [](const CheckResponseInfo& info) {
+                   EXPECT_TRUE(info.response_status.ok());
+                 });
 }
 
 TEST_F(RequestHandlerImplTest, TestPerRouteAttributes) {
@@ -473,7 +478,9 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) {
   Controller::PerRouteConfig config;
   auto handler = controller_->CreateRequestHandler(config);
   handler->Check(&mock_check, &mock_header, nullptr,
-                 [](const Status& status) { EXPECT_TRUE(status.ok()); });
+                 [](const CheckResponseInfo& info) {
+                   EXPECT_TRUE(info.response_status.ok());
+                 });
   handler->Report(&mock_report);
 }