-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Support for external authorization tcp filter. #2416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 22 commits
98197b8
6c88bd1
981ae9d
3952f68
9da8b85
4fe6572
9c02776
25d37c6
5dc5495
38d8799
ce9c1e9
40746cd
8823360
36053cd
a675ec2
fa371c3
c8910ad
945ac1c
5b41b32
a649490
d1392ff
8f5db76
b3dc7bf
4143a3c
a02d949
4d13223
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| #include "common/filter/ext_authz.h" | ||
|
|
||
| #include <cstdint> | ||
| #include <string> | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/tracing/http_tracer_impl.h" | ||
|
|
||
| #include "fmt/format.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace ExtAuthz { | ||
| namespace TcpFilter { | ||
|
|
||
| InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope) { | ||
| const std::string final_prefix = fmt::format("ext_authz.{}.", name); | ||
| return {ALL_TCP_EXT_AUTHZ_STATS(POOL_COUNTER_PREFIX(scope, final_prefix), | ||
| POOL_GAUGE_PREFIX(scope, final_prefix))}; | ||
| } | ||
|
|
||
| void Instance::callCheck() { | ||
| CheckRequestUtils::createTcpCheck(filter_callbacks_, checkRequest_); | ||
|
|
||
| status_ = Status::Calling; | ||
| config_->stats().active_.inc(); | ||
| config_->stats().total_.inc(); | ||
|
|
||
| calling_check_ = true; | ||
| client_->check(*this, checkRequest_, Tracing::NullSpan::instance()); | ||
| calling_check_ = false; | ||
| } | ||
|
|
||
| Network::FilterStatus Instance::onData(Buffer::Instance&) { | ||
| if (status_ == Status::NotStarted) { | ||
| // By waiting to invoke the check at onData() the call to authorization service will have | ||
| // sufficient information to fillout the checkRequest_. | ||
| callCheck(); | ||
| } | ||
| return status_ == Status::Calling ? Network::FilterStatus::StopIteration | ||
| : Network::FilterStatus::Continue; | ||
| } | ||
|
|
||
| Network::FilterStatus Instance::onNewConnection() { | ||
| // Wait till onData() happens. | ||
| return Network::FilterStatus::Continue; | ||
| } | ||
|
|
||
| void Instance::onEvent(Network::ConnectionEvent event) { | ||
| if (event == Network::ConnectionEvent::RemoteClose || | ||
| event == Network::ConnectionEvent::LocalClose) { | ||
| if (status_ == Status::Calling) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move the comment above here |
||
| // Make sure that any pending request in the client is cancelled. This will be NOP if the | ||
| // request already completed. | ||
| client_->cancel(); | ||
| config_->stats().active_.dec(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void Instance::onComplete(CheckStatus status) { | ||
| status_ = Status::Complete; | ||
| config_->stats().active_.dec(); | ||
|
|
||
| switch (status) { | ||
| case CheckStatus::OK: | ||
| config_->stats().ok_.inc(); | ||
| break; | ||
| case CheckStatus::Error: | ||
| config_->stats().error_.inc(); | ||
| break; | ||
| case CheckStatus::Denied: | ||
| config_->stats().denied_.inc(); | ||
| break; | ||
| } | ||
|
|
||
| // Fail open only if configured to do so and if the check status was a error. | ||
| if (status == CheckStatus::Denied || (status == CheckStatus::Error && !config_->failOpen())) { | ||
| config_->stats().cx_closed_.inc(); | ||
| filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); | ||
| } else { | ||
| // We can get completion inline, so only call continue if that isn't happening. | ||
| if (!calling_check_) { | ||
| filter_callbacks_->continueReading(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that |
||
| } | ||
| } | ||
| } | ||
|
|
||
| } // namespace TcpFilter | ||
| } // namespace ExtAuthz | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "envoy/config/filter/network/ext_authz/v2/ext_authz.pb.h" | ||
| #include "envoy/ext_authz/ext_authz.h" | ||
| #include "envoy/network/connection.h" | ||
| #include "envoy/network/filter.h" | ||
| #include "envoy/runtime/runtime.h" | ||
| #include "envoy/stats/stats_macros.h" | ||
| #include "envoy/upstream/cluster_manager.h" | ||
|
|
||
| #include "common/ext_authz/ext_authz_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace ExtAuthz { | ||
| namespace TcpFilter { | ||
|
|
||
| /** | ||
| * All tcp external authorization stats. @see stats_macros.h | ||
| */ | ||
| // clang-format off | ||
| #define ALL_TCP_EXT_AUTHZ_STATS(COUNTER, GAUGE) \ | ||
| COUNTER(total) \ | ||
| COUNTER(error) \ | ||
| COUNTER(denied) \ | ||
| COUNTER(ok) \ | ||
| COUNTER(cx_closed) \ | ||
| GAUGE (active) | ||
| // clang-format on | ||
|
|
||
| /** | ||
| * Struct definition for all external authorization stats. @see stats_macros.h | ||
| */ | ||
| struct InstanceStats { | ||
| ALL_TCP_EXT_AUTHZ_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) | ||
| }; | ||
|
|
||
| /** | ||
| * Global configuration for ExtAuthz filter. | ||
| */ | ||
| class Config { | ||
| public: | ||
| Config(const envoy::config::filter::network::ext_authz::v2::ExtAuthz& config, Stats::Scope& scope) | ||
| : stats_(generateStats(config.stat_prefix(), scope)), | ||
| failure_mode_allow_(config.failure_mode_allow()) {} | ||
|
|
||
| const InstanceStats& stats() { return stats_; } | ||
| bool failOpen() const { return failure_mode_allow_; } | ||
| void setFailModeAllow(bool value) { failure_mode_allow_ = value; } | ||
|
|
||
| private: | ||
| static InstanceStats generateStats(const std::string& name, Stats::Scope& scope); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: newline |
||
| const InstanceStats stats_; | ||
| bool failure_mode_allow_; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<Config> ConfigSharedPtr; | ||
|
|
||
| /** | ||
| * ExtAuthz filter instance. This filter will call the Authorization service with the given | ||
| * configuration parameters. If the authorization service returns an error or a deny the | ||
| * connection will be closed without any further filters being called. Otherwise all buffered | ||
| * data will be released to further filters. | ||
| */ | ||
| class Instance : public Network::ReadFilter, | ||
| public Network::ConnectionCallbacks, | ||
| public RequestCallbacks { | ||
| public: | ||
| Instance(ConfigSharedPtr config, ClientPtr&& client) | ||
| : config_(config), client_(std::move(client)) {} | ||
| ~Instance() {} | ||
|
|
||
| // Network::ReadFilter | ||
| Network::FilterStatus onData(Buffer::Instance& data) override; | ||
| Network::FilterStatus onNewConnection() override; | ||
| void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override { | ||
| filter_callbacks_ = &callbacks; | ||
| filter_callbacks_->connection().addConnectionCallbacks(*this); | ||
| } | ||
|
|
||
| // Network::ConnectionCallbacks | ||
| void onEvent(Network::ConnectionEvent event) override; | ||
| void onAboveWriteBufferHighWatermark() override {} | ||
| void onBelowWriteBufferLowWatermark() override {} | ||
|
|
||
| // ExtAuthz::RequestCallbacks | ||
| void onComplete(CheckStatus status) override; | ||
|
|
||
| private: | ||
| enum class Status { NotStarted, Calling, Complete }; | ||
| void callCheck(); | ||
|
|
||
| ConfigSharedPtr config_; | ||
| ClientPtr client_; | ||
| Network::ReadFilterCallbacks* filter_callbacks_{}; | ||
| Status status_{Status::NotStarted}; | ||
| bool calling_check_{}; | ||
| envoy::service::auth::v2::CheckRequest checkRequest_{}; | ||
| }; | ||
|
|
||
| } // TcpFilter | ||
| } // namespace ExtAuthz | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| #include "server/config/network/ext_authz.h" | ||
|
|
||
| #include <chrono> | ||
| #include <string> | ||
|
|
||
| #include "envoy/config/filter/network/ext_authz/v2/ext_authz.pb.validate.h" | ||
| #include "envoy/ext_authz/ext_authz.h" | ||
| #include "envoy/network/connection.h" | ||
| #include "envoy/registry/registry.h" | ||
|
|
||
| #include "common/ext_authz/ext_authz_impl.h" | ||
| #include "common/filter/ext_authz.h" | ||
| #include "common/protobuf/utility.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Server { | ||
| namespace Configuration { | ||
|
|
||
| NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilter( | ||
| const envoy::config::filter::network::ext_authz::v2::ExtAuthz& proto_config, | ||
| FactoryContext& context) { | ||
|
|
||
| ASSERT(!proto_config.stat_prefix().empty()); | ||
| ASSERT(!proto_config.grpc_service().envoy_grpc().cluster_name().empty()); | ||
|
|
||
| ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is missing coverage, see https://27311-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_server_config_network_ext_authz.cc.html. Can you add tests? See https://github.com/envoyproxy/envoy/tree/master/test/server/config for examples.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad...thanks for catching that. Let me address this. |
||
| new ExtAuthz::TcpFilter::Config(proto_config, context.scope())); | ||
| const uint32_t timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config.grpc_service(), timeout, 200); | ||
|
|
||
| return [ grpc_service = proto_config.grpc_service(), &context, ext_authz_config, | ||
| timeout_ms ](Network::FilterManager & filter_manager) | ||
| ->void { | ||
|
|
||
| auto async_client_factory = | ||
| context.clusterManager().grpcAsyncClientManager().factoryForGrpcService(grpc_service, | ||
| context.scope()); | ||
|
|
||
| auto client = std::make_unique<Envoy::ExtAuthz::GrpcClientImpl>( | ||
| async_client_factory->create(), std::chrono::milliseconds(timeout_ms)); | ||
| filter_manager.addReadFilter(Network::ReadFilterSharedPtr{ | ||
| new ExtAuthz::TcpFilter::Instance(ext_authz_config, std::move(client))}); | ||
| }; | ||
| } | ||
|
|
||
| NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactory(const Json::Object& json_config, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only need this for v1 config; are we supporting v1 ext_authz?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are doing only v2. This helps in specifying the configuration as v2 json and still use it without rigging up a xDS server.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure this is strictly needed for v2 JSON, this should only apply to v1 JSON placed in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. afaik this function is the entry point when I give envoy executable a configuration file And that way I can run an end-2-end system. |
||
| FactoryContext& context) { | ||
| envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config; | ||
| MessageUtil::loadFromJson(json_config.asJsonString(), proto_config); | ||
| return createFilter(proto_config, context); | ||
| } | ||
|
|
||
| NetworkFilterFactoryCb | ||
| ExtAuthzConfigFactory::createFilterFactoryFromProto(const Protobuf::Message& proto_config, | ||
| FactoryContext& context) { | ||
| return createFilter( | ||
| MessageUtil::downcastAndValidate< | ||
| const envoy::config::filter::network::ext_authz::v2::ExtAuthz&>(proto_config), | ||
| context); | ||
| } | ||
|
|
||
| /** | ||
| * Static registration for the external authorization filter. @see RegisterFactory. | ||
| */ | ||
| static Registry::RegisterFactory<ExtAuthzConfigFactory, NamedNetworkFilterConfigFactory> | ||
| registered_; | ||
|
|
||
| } // namespace Configuration | ||
| } // namespace Server | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you integrate this state variable with
status_somehow? Having multiple state variables makes it harder to reason about.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch thanks for the review. I've addressed other changes. For this one I really found it more difficult to combine
calling_check_withstatus_.calling_check_is specifically to check for on stack callback; i.e whenonCompleteis invoked in the same call stack. When I tried to combine it intostatus_it ended up withstatus_CallingOnStack,CallingOffStack; which made it more confusing to me.To me these two states feel disjoint. Any suggestions on how to address it?