Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions bazel/envoy_mobile_test_extensions.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ TEST_EXTENSIONS = [
"//library/common/extensions/filters/http/test_logger:config",
"//library/common/extensions/filters/http/test_accessor:config",
"//library/common/extensions/filters/http/test_event_tracker:config",
"//library/common/extensions/filters/http/test_kv_store:config",
]
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Bugfixes:

Features:

- android: add support for registering a platform KV store (:issue: `#2134 <2134>`)
- api: add option to extend the keepalive timeout when any frame is received on the owning HTTP/2 connection. (:issue:`#2229 <2229>`)
- api: add option to control whether Envoy should drain connections after a soft DNS refresh completes. (:issue:`#2225 <2225>`, :issue:`#2242 <2242>`)
- configuration: enable h2 ping by default. (:issue: `#2270 <2270>`)
Expand Down
45 changes: 45 additions & 0 deletions library/common/extensions/filters/http/test_kv_store/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
load(
"@envoy//bazel:envoy_build_system.bzl",
"envoy_cc_extension",
"envoy_extension_package",
"envoy_proto_library",
)

licenses(["notice"]) # Apache 2

envoy_extension_package()

envoy_proto_library(
name = "filter",
srcs = ["filter.proto"],
deps = [
"@envoy_api//envoy/config/common/matcher/v3:pkg",
],
)

envoy_cc_extension(
name = "test_kv_store_filter_lib",
srcs = ["filter.cc"],
hdrs = ["filter.h"],
repository = "@envoy",
deps = [
"filter_cc_proto",
"//library/common/api:c_types",
"//library/common/api:external_api_lib",
"//library/common/data:utility_lib",
"//library/common/extensions/key_value/platform:config",
"@envoy//source/common/common:assert_lib",
"@envoy//source/extensions/filters/http/common:pass_through_filter_lib",
],
)

envoy_cc_extension(
name = "config",
srcs = ["config.cc"],
hdrs = ["config.h"],
repository = "@envoy",
deps = [
":test_kv_store_filter_lib",
"@envoy//source/extensions/filters/http/common:factory_base_lib",
],
)
29 changes: 29 additions & 0 deletions library/common/extensions/filters/http/test_kv_store/config.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#include "library/common/extensions/filters/http/test_kv_store/config.h"

#include "library/common/extensions/filters/http/test_kv_store/filter.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace TestKeyValueStore {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected the name of this to be something like PlatformKeyValueStore. Is this filter intended to be used in production? Maybe it's just test-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your presumption is correct - this is a test-only filter that exist alongside some other test-only filters in an entirely unintuitive location. Ideally, these should be moved elsewhere. I can certainly add a clarifying comment.


Http::FilterFactoryCb TestKeyValueStoreFilterFactory::createFilterFactoryFromProtoTyped(
const envoymobile::extensions::filters::http::test_kv_store::TestKeyValueStore& proto_config,
const std::string&, Server::Configuration::FactoryContext&) {

auto config = std::make_shared<TestKeyValueStoreFilterConfig>(proto_config);
return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
callbacks.addStreamFilter(std::make_shared<TestKeyValueStoreFilter>(config));
};
}

/**
* Static registration for the TestKeyValueStore filter. @see NamedHttpFilterConfigFactory.
*/
REGISTER_FACTORY(TestKeyValueStoreFilterFactory,
Server::Configuration::NamedHttpFilterConfigFactory);

} // namespace TestKeyValueStore
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
36 changes: 36 additions & 0 deletions library/common/extensions/filters/http/test_kv_store/config.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#pragma once

#include <string>

#include "source/extensions/filters/http/common/factory_base.h"

#include "library/common/extensions/filters/http/test_kv_store/filter.h"
#include "library/common/extensions/filters/http/test_kv_store/filter.pb.h"
#include "library/common/extensions/filters/http/test_kv_store/filter.pb.validate.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace TestKeyValueStore {

/**
* Config registration for the TestKeyValueStore filter. @see NamedHttpFilterConfigFactory.
*/
class TestKeyValueStoreFilterFactory
: public Common::FactoryBase<
envoymobile::extensions::filters::http::test_kv_store::TestKeyValueStore> {
public:
TestKeyValueStoreFilterFactory() : FactoryBase("test_kv_store") {}

private:
::Envoy::Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
const envoymobile::extensions::filters::http::test_kv_store::TestKeyValueStore& config,
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;
};

DECLARE_FACTORY(TestKeyValueStoreFilterFactory);

} // namespace TestKeyValueStore
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
46 changes: 46 additions & 0 deletions library/common/extensions/filters/http/test_kv_store/filter.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#include "library/common/extensions/filters/http/test_kv_store/filter.h"

#include "envoy/server/filter_config.h"

#include "source/common/common/assert.h"

#include "library/common/data/utility.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace TestKeyValueStore {

TestKeyValueStoreFilterConfig::TestKeyValueStoreFilterConfig(
const envoymobile::extensions::filters::http::test_kv_store::TestKeyValueStore& proto_config)
: kv_store_(
static_cast<envoy_kv_store*>(Api::External::retrieveApi(proto_config.kv_store_name()))) {}

Http::FilterHeadersStatus TestKeyValueStoreFilter::decodeHeaders(Http::RequestHeaderMap&, bool) {
const auto store = config_->keyValueStore();
auto key = Data::Utility::copyToBridgeData(config_->testKey());
RELEASE_ASSERT(Data::Utility::copyToString(store->read(key, store->context)).empty(),
"store should be empty");

envoy_data value = Data::Utility::copyToBridgeData(config_->testValue());
store->save(key, value, store->context);
return Http::FilterHeadersStatus::Continue;
}

Http::FilterHeadersStatus TestKeyValueStoreFilter::encodeHeaders(Http::ResponseHeaderMap&, bool) {
const auto store = config_->keyValueStore();
auto key = Data::Utility::copyToBridgeData(config_->testKey());
RELEASE_ASSERT(Data::Utility::copyToString(store->read(key, store->context)) ==
config_->testValue(),
"store did not contain expected value");

store->remove(key, store->context);
RELEASE_ASSERT(Data::Utility::copyToString(store->read(key, store->context)).empty(),
"store should be empty");

return Http::FilterHeadersStatus::Continue;
}
} // namespace TestKeyValueStore
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
59 changes: 59 additions & 0 deletions library/common/extensions/filters/http/test_kv_store/filter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
#pragma once

#include "envoy/http/filter.h"

#include "source/extensions/filters/http/common/pass_through_filter.h"

#include "library/common/api/c_types.h"
#include "library/common/api/external.h"
#include "library/common/extensions/filters/http/test_kv_store/filter.pb.h"
#include "library/common/extensions/key_value/platform/c_types.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace TestKeyValueStore {

/**
* This is a test-only filter used for validating PlatformKeyValueStore integrations. It retrieves
* a known key-value store implementation and issues a series of fixed calls to it, allowing for
* validation to be performed from platform code.
*
* TODO(goaway): Move to a location for test components outside of the main source tree.
*/
class TestKeyValueStoreFilterConfig {
public:
TestKeyValueStoreFilterConfig(
const envoymobile::extensions::filters::http::test_kv_store::TestKeyValueStore& proto_config);

const envoy_kv_store* keyValueStore() const { return kv_store_; }
const std::string& testKey() const { return test_key_; }
const std::string& testValue() const { return test_value_; }

private:
const envoy_kv_store* kv_store_;
const std::string test_key_;
const std::string test_value_;
};

using TestKeyValueStoreFilterConfigSharedPtr = std::shared_ptr<TestKeyValueStoreFilterConfig>;

class TestKeyValueStoreFilter final : public ::Envoy::Http::PassThroughFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please add a comment which describes what this filter does.

public:
TestKeyValueStoreFilter(TestKeyValueStoreFilterConfigSharedPtr config) : config_(config) {}

// StreamDecoderFilter
::Envoy::Http::FilterHeadersStatus decodeHeaders(::Envoy::Http::RequestHeaderMap& headers,
bool end_stream) override;
// StreamEncoderFilter
::Envoy::Http::FilterHeadersStatus encodeHeaders(::Envoy::Http::ResponseHeaderMap& headers,
bool end_stream) override;

private:
const TestKeyValueStoreFilterConfigSharedPtr config_;
};

} // namespace TestKeyValueStore
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
11 changes: 11 additions & 0 deletions library/common/extensions/filters/http/test_kv_store/filter.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
syntax = "proto3";

package envoymobile.extensions.filters.http.test_kv_store;

import "validate/validate.proto";

message TestKeyValueStore {
string kv_store_name = 1 [(validate.rules).string.min_len = 1];
string test_key = 2 [(validate.rules).string.min_len = 1];
string test_value = 3 [(validate.rules).string.min_len = 1];
}
7 changes: 6 additions & 1 deletion library/common/extensions/key_value/platform/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@ envoy_proto_library(
envoy_cc_extension(
name = "config",
srcs = ["config.cc"],
hdrs = ["config.h"],
hdrs = [
"c_types.h",
"config.h",
],
repository = "@envoy",
deps = [
":platform_cc_proto",
"//library/common/api:external_api_lib",
"//library/common/data:utility_lib",
"@envoy//envoy/common:key_value_store_interface",
"@envoy//envoy/event:dispatcher_interface",
"@envoy//envoy/filesystem:filesystem_interface",
Expand Down
35 changes: 35 additions & 0 deletions library/common/extensions/key_value/platform/c_types.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#pragma once

#include "library/common/types/c_types.h"

// NOLINT(namespace-envoy)

#ifdef __cplusplus
extern "C" { // function pointers
#endif

/**
* Function signature for reading value from implementation.
*/
typedef envoy_data (*envoy_kv_store_read_f)(envoy_data key, const void* context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you went with a synchronous approach here? Is the idea that consumers would take care of moving this to a separate thread if they don't want to block on reading/writing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, for read asynchronous would introduce a whole new calling paradigm ("onRead"?). Honestly, I'd think an implementation should probably do caching/pre-caching/memoization if read performance is likely to be a bottleneck somewhere. For write/remove , yes, I think that's also better off as a feature of the actual provided platform implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. Another thing to consider is whether or not errors should be propagated between the platform and native layers.


/**
* Function signature for saving value to implementation.
*/
typedef void (*envoy_kv_store_save_f)(envoy_data key, envoy_data value, const void* context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this document what's expected as the context pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a more general description to the root c_types.h, since we use these everywhere.


/**
* Function signature for removing value from implementation.
*/
typedef void (*envoy_kv_store_remove_f)(envoy_data key, const void* context);

#ifdef __cplusplus
} // function pointers
#endif

typedef struct {
envoy_kv_store_read_f read;
envoy_kv_store_save_f save;
envoy_kv_store_remove_f remove;
const void* context;
} envoy_kv_store;
27 changes: 27 additions & 0 deletions library/common/extensions/key_value/platform/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,37 @@
#include "envoy/config/common/key_value/v3/config.pb.validate.h"
#include "envoy/registry/registry.h"

#include "library/common/api/external.h"
#include "library/common/data/utility.h"
#include "library/common/extensions/key_value/platform/c_types.h"

namespace Envoy {
namespace Extensions {
namespace KeyValue {

class PlatformInterfaceImpl : PlatformInterface, public Logger::Loggable<Logger::Id::filter> {
public:
PlatformInterfaceImpl(const std::string& name)
: bridged_store_(*static_cast<envoy_kv_store*>(Api::External::retrieveApi(name))) {}

~PlatformInterfaceImpl() override {}

std::string read(const std::string& key) const override {
envoy_data bridged_key = Data::Utility::copyToBridgeData(key);
envoy_data bridged_value = bridged_store_.read(bridged_key, bridged_store_.context);
return Data::Utility::copyToString(bridged_value);
}

void save(const std::string& key, const std::string& contents) override {
envoy_data bridged_key = Data::Utility::copyToBridgeData(key);
envoy_data bridged_value = Data::Utility::copyToBridgeData(contents);
bridged_store_.save(bridged_key, bridged_value, bridged_store_.context);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected a remove() method. Is that not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alyssawilk shared that you all didn't have a use case for it right now (which I sort of found surprising, but then again, if you supply the KV store implementation, you can always remove stuff from it elsewhere). I went ahead and wired remove at the other layers anyways, simply because I do expect we will have a use for it.

private:
envoy_kv_store bridged_store_;
};

PlatformKeyValueStore::PlatformKeyValueStore(Event::Dispatcher& dispatcher,
std::chrono::milliseconds save_interval,
PlatformInterface& platform_interface,
Expand Down
Loading