-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
98197b8
Support for external authroization tcp filter.
6c88bd1
Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp
981ae9d
Merge with gRPC singleton manager.
3952f68
Format fixes for tcp files.
9da8b85
Fix test tcp filter format.
4fe6572
Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp
9c02776
Merge with data-plane-api for tcp.
25d37c6
Review feedback for tcp files.
5dc5495
Fix up network config build.
38d8799
Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp
ce9c1e9
Remove mock for proto generation: tcp changes.
40746cd
Review comments tcp.
8823360
Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp
36053cd
Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp
a675ec2
Merge tcp changes with data-plane-api reorg #2495.
fa371c3
Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp
c8910ad
Fixup default timeout.
945ac1c
Include network filter config in build.
5b41b32
Address review comments.
a649490
Review feedback.
d1392ff
Fix tests class name.
8f5db76
Move callCheck to onData.
b3dc7bf
Add tcp filter server configuration tests.
4143a3c
Fix formatting issue.
a02d949
Formatting fix up with clang-5 not 3.8
4d13223
Remove createFilterFactor implementation.
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) { | ||
| // 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(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } // namespace TcpFilter | ||
| } // namespace ExtAuthz | ||
| } // namespace Envoy | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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); | ||
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| #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() || | ||
| !proto_config.grpc_service().google_grpc().target_uri().empty()); | ||
|
|
||
| ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config( | ||
| 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&, | ||
| FactoryContext&) { | ||
| NOT_IMPLEMENTED; | ||
| } | ||
|
|
||
| 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 |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I wonder if
readDisable(false)above is sufficient, and you don't need to do this; it's docs say it will restart the read pipeline, and redispatch pending data. Does this work for this filter? That could eliminate the need to maintaincalling_check_.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.
Moving
readDisable(false)here -which is what u were asking about at the top of this function-makes sense.From what I understood-which may be totally wrong-is that
readDisable(false)was to get the socket buffer pipeline moving and thefilter_callbacks_->continueReading()is to get the filter chain iteration moving forward. The filter iteration may have been stopped iff thecallCheck()and theonComplete()callback happened asynchronously.But if they both are invoked in the same call stack then the filter chain was never stopped, hence it doesn't need to be kick started again by invoking
filter_callbacks_->continueReading().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.
I think that
filter_callback->connection().readDisable(false|true)will no longer be necessary since now we will invoke the external authz service fromonData()which clearly returns either aStopIterationorContinueand NOT fromonEvent().