Skip to content
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

Sds api split #4231

Merged
merged 5 commits into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions include/envoy/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,17 @@ load(

envoy_package()

envoy_cc_library(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting this PR out, this is very helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad that it helps. Will keep PR in good size.

name = "secret_callbacks_interface",
hdrs = ["secret_callbacks.h"],
)

envoy_cc_library(
name = "secret_provider_interface",
hdrs = ["secret_provider.h"],
deps = [
":secret_callbacks_interface",
"//include/envoy/common:callback",
"//include/envoy/ssl:tls_certificate_config_interface",
],
)
Expand Down
18 changes: 18 additions & 0 deletions include/envoy/secret/secret_callbacks.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#pragma once

#include "envoy/common/pure.h"

namespace Envoy {
namespace Secret {

/**
* Callbacks invoked by a dynamic secret provider.
*/
class SecretCallbacks {
public:
virtual ~SecretCallbacks() {}
virtual void onAddOrUpdateSecret() PURE;
};

} // namespace Secret
} // namespace Envoy
12 changes: 11 additions & 1 deletion include/envoy/secret/secret_provider.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
#pragma once

#include <functional>

#include "envoy/common/callback.h"
#include "envoy/common/pure.h"
#include "envoy/ssl/tls_certificate_config.h"

Expand All @@ -18,7 +21,14 @@ template <class SecretType> class SecretProvider {
*/
virtual const SecretType* secret() const PURE;

// TODO(lizan): Add more methods for dynamic secret provider.
/**
* Add secret update callback into secret provider.
* It is safe to call this method by main thread and is safe to be invoked
* on main thread.
* @param callback callback that is executed by secret provider.
* @return CallbackHandle the handle which can remove that update callback.
*/
virtual Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) PURE;
};

typedef SecretProvider<Ssl::TlsCertificateConfig> TlsCertificateConfigProvider;
Expand Down
21 changes: 21 additions & 0 deletions source/common/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,24 @@ envoy_cc_library(
"@envoy_api//envoy/api/v2/auth:cert_cc",
],
)

envoy_cc_library(
name = "sds_api_lib",
srcs = ["sds_api.cc"],
hdrs = ["sds_api.h"],
deps = [
"//include/envoy/config:subscription_interface",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/init:init_interface",
"//include/envoy/local_info:local_info_interface",
"//include/envoy/runtime:runtime_interface",
"//include/envoy/secret:secret_provider_interface",
"//include/envoy/stats:stats_interface",
"//source/common/common:callback_impl_lib",
"//source/common/common:cleanup_lib",
"//source/common/config:resources_lib",
"//source/common/config:subscription_factory_lib",
"//source/common/protobuf:utility_lib",
"//source/common/ssl:tls_certificate_config_impl_lib",
],
)
85 changes: 85 additions & 0 deletions source/common/secret/sds_api.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
#include "common/secret/sds_api.h"

#include <unordered_map>

#include "envoy/api/v2/auth/cert.pb.validate.h"

#include "common/config/resources.h"
#include "common/config/subscription_factory.h"
#include "common/protobuf/utility.h"
#include "common/ssl/tls_certificate_config_impl.h"

namespace Envoy {
namespace Secret {

SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher,
Runtime::RandomGenerator& random, Stats::Store& stats,
Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager,
const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name,
std::function<void()> destructor_cb)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better than before, thanks. I still think it can be cleaner if instead of having SdsApi owning the destructor CB, we have the owner of SdsApi own that object. It's pretty weird threading a destructor CB into an object when the lifetime issues are really those of the owning object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. My concern is that SdsApi is held by a shared_ptr (ContextConfigImpland::tls_certficate_provider_) and could be owned by multiple objects (e.g. multiple listeners or clusters). The last owner needs to destroy SdsApi and ~SdsApi() invokes this cleanup function. I am not clear if there is a better place to own the Cleanup object other than SdsApi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@htuch SdsApi is a shared pointer that could be owned by multiple objects as @JimmyCYJ pointed out, the structure here is similar to what we have in RDS (RdsRouteConfigSubscription). With the destructor_cb it doesn't need to be a friend class between SecretManagerImpl and SdsApi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, OK, I still think you could wrap SdsApi in a SdsApiWithManagedLifetime object to get the same effect and separate concerns here, but I guess we have precedent for doing it this way.

: local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats),
cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name),
secret_hash_(0), clean_up_(destructor_cb) {
// TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager
// can be chained together to behave as one init_manager. In that way, we let
// two listeners which share same SdsApi to register at separate init managers, and
// each init manager has a chance to initialize its targets.
init_manager.registerTarget(*this);
}

void SdsApi::initialize(std::function<void()> callback) {
initialize_callback_ = callback;

subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource<
envoy::api::v2::auth::Secret>(
sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_,
/* rest_legacy_constructor */ nullptr,
"envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets",
"envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets");
Config::Utility::checkLocalInfo("sds", local_info_);

subscription_->start({sds_config_name_}, *this);
}

void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) {
if (resources.empty()) {
throw EnvoyException(
fmt::format("Missing SDS resources for {} in onConfigUpdate()", sds_config_name_));
}
if (resources.size() != 1) {
throw EnvoyException(fmt::format("Unexpected SDS secrets length: {}", resources.size()));
}
const auto& secret = resources[0];
MessageUtil::validate(secret);
if (secret.name() != sds_config_name_) {
throw EnvoyException(
fmt::format("Unexpected SDS secret (expecting {}): {}", sds_config_name_, secret.name()));
}

const uint64_t new_hash = MessageUtil::hash(secret);
if (new_hash != secret_hash_ &&
secret.type_case() == envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate) {
secret_hash_ = new_hash;
tls_certificate_secrets_ =
std::make_unique<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate());

update_callback_manager_.runCallbacks();
}

runInitializeCallbackIfAny();
}

void SdsApi::onConfigUpdateFailed(const EnvoyException*) {
// We need to allow server startup to continue, even if we have a bad config.
runInitializeCallbackIfAny();
}

void SdsApi::runInitializeCallbackIfAny() {
if (initialize_callback_) {
initialize_callback_();
initialize_callback_ = nullptr;
}
}

} // namespace Secret
} // namespace Envoy
78 changes: 78 additions & 0 deletions source/common/secret/sds_api.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
#pragma once

#include <functional>

#include "envoy/api/v2/auth/cert.pb.h"
#include "envoy/api/v2/core/config_source.pb.h"
#include "envoy/config/subscription.h"
#include "envoy/event/dispatcher.h"
#include "envoy/init/init.h"
#include "envoy/local_info/local_info.h"
#include "envoy/runtime/runtime.h"
#include "envoy/secret/secret_callbacks.h"
#include "envoy/secret/secret_provider.h"
#include "envoy/stats/stats.h"
#include "envoy/upstream/cluster_manager.h"

#include "common/common/callback_impl.h"
#include "common/common/cleanup.h"

namespace Envoy {
namespace Secret {

/**
* SDS API implementation that fetches secrets from SDS server via Subscription.
*/
class SdsApi : public Init::Target,
public TlsCertificateConfigProvider,
public Config::SubscriptionCallbacks<envoy::api::v2::auth::Secret> {
public:
SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher,
Runtime::RandomGenerator& random, Stats::Store& stats,
Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager,
const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name,
std::function<void()> destructor_cb);

// Init::Target
void initialize(std::function<void()> callback) override;

// Config::SubscriptionCallbacks
void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override;
void onConfigUpdateFailed(const EnvoyException* e) override;
std::string resourceName(const ProtobufWkt::Any& resource) override {
return MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resource).name();
}

// SecretProvider
const Ssl::TlsCertificateConfig* secret() const override {
return tls_certificate_secrets_.get();
}

Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
return update_callback_manager_.add(callback);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-actionable; for some reason I thought this was RAII, but it's just a callback removal handle. We can make that more robust independent of this PR.

}

private:
void runInitializeCallbackIfAny();

const LocalInfo::LocalInfo& local_info_;
Event::Dispatcher& dispatcher_;
Runtime::RandomGenerator& random_;
Stats::Store& stats_;
Upstream::ClusterManager& cluster_manager_;

const envoy::api::v2::core::ConfigSource sds_config_;
std::unique_ptr<Config::Subscription<envoy::api::v2::auth::Secret>> subscription_;
std::function<void()> initialize_callback_;
const std::string sds_config_name_;

uint64_t secret_hash_;
Cleanup clean_up_;
Ssl::TlsCertificateConfigPtr tls_certificate_secrets_;
Common::CallbackManager<> update_callback_manager_;
};

typedef std::unique_ptr<SdsApi> SdsApiPtr;

} // namespace Secret
} // namespace Envoy
4 changes: 4 additions & 0 deletions source/common/secret/secret_provider_impl.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include <functional>

#include "envoy/api/v2/auth/cert.pb.h"
#include "envoy/secret/secret_provider.h"
#include "envoy/ssl/tls_certificate_config.h"
Expand All @@ -13,6 +15,8 @@ class TlsCertificateConfigProviderImpl : public TlsCertificateConfigProvider {

const Ssl::TlsCertificateConfig* secret() const override { return tls_certificate_.get(); }

Common::CallbackHandle* addUpdateCallback(std::function<void()>) override { return nullptr; }

private:
Ssl::TlsCertificateConfigPtr tls_certificate_;
};
Expand Down
30 changes: 28 additions & 2 deletions source/common/ssl/ssl_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,24 @@ using Envoy::Network::PostIoAction;
namespace Envoy {
namespace Ssl {

namespace {
// This SslSocket will be used when SSL secret is not fetched from SDS server.
class NotReadySslSocket : public Network::TransportSocket {
public:
// Network::TransportSocket
void setTransportSocketCallbacks(Network::TransportSocketCallbacks&) override {}
std::string protocol() const override { return EMPTY_STRING; }
bool canFlushClose() override { return true; }
void closeSocket(Network::ConnectionEvent) override {}
Network::IoResult doRead(Buffer::Instance&) override { return {PostIoAction::Close, 0, false}; }
Network::IoResult doWrite(Buffer::Instance&, bool) override {
return {PostIoAction::Close, 0, false};
}
void onConnected() override {}
const Ssl::Connection* ssl() const override { return nullptr; }
};
} // namespace

SslSocket::SslSocket(ContextSharedPtr ctx, InitialState state)
: ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)), ssl_(ctx_->newSsl()) {
if (state == InitialState::Client) {
Expand Down Expand Up @@ -391,7 +409,11 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config,
ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) {}

Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const {
return std::make_unique<Ssl::SslSocket>(ssl_ctx_, Ssl::InitialState::Client);
if (ssl_ctx_) {
return std::make_unique<Ssl::SslSocket>(ssl_ctx_, Ssl::InitialState::Client);
} else {
return std::make_unique<NotReadySslSocket>();
}
}

bool ClientSslSocketFactory::implementsSecureTransport() const { return true; }
Expand All @@ -405,7 +427,11 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config,
ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) {}

Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const {
return std::make_unique<Ssl::SslSocket>(ssl_ctx_, Ssl::InitialState::Server);
if (ssl_ctx_) {
return std::make_unique<Ssl::SslSocket>(ssl_ctx_, Ssl::InitialState::Server);
} else {
return std::make_unique<NotReadySslSocket>();
}
}

bool ServerSslSocketFactory::implementsSecureTransport() const { return true; }
Expand Down
19 changes: 19 additions & 0 deletions test/common/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,22 @@ envoy_cc_test(
"//test/test_common:utility_lib",
],
)

envoy_cc_test(
name = "sds_api_test",
srcs = ["sds_api_test.cc"],
data = [
"//test/common/ssl/test_data:certs",
],
deps = [
"//source/common/secret:sds_api_lib",
"//test/mocks/grpc:grpc_mocks",
"//test/mocks/init:init_mocks",
"//test/mocks/secret:secret_mocks",
"//test/mocks/server:server_mocks",
"//test/test_common:environment_lib",
"//test/test_common:registry_lib",
"//test/test_common:utility_lib",
"@envoy_api//envoy/service/discovery/v2:sds_cc",
],
)
Loading