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.

Sign up for GitHub

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

config: Struct opaque filter proto config support, LDS/RDS integratio… #1495

Merged
merged 10 commits into from
Aug 22, 2017
Merged

config: Struct opaque filter proto config support, LDS/RDS integratio… #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
83604ea
config: Struct opaque filter proto config support, LDS/RDS integratio…
htuch Aug 17, 2017
03747c0
Fix race-to-listen/connect, consolidate waitForCountGe-like code.
htuch Aug 20, 2017
b705be9
Fix test flakes by making protobuf serialization deterministic.
htuch Aug 21, 2017
528cf4a
Merge remote-tracking branch 'upstream/master' into lds-struct
htuch Aug 21, 2017
1b52683
Review feedback.
htuch Aug 21, 2017
e09dbc1
Merge remote-tracking branch 'upstream/master' into lds-struct
htuch Aug 21, 2017
ab0c25c
LDS/RDS xds_integration_test JSON -> YAML.
htuch Aug 21, 2017
9079ccc
Review feedback, fix format.
htuch Aug 22, 2017
fbab7be
Merge remote-tracking branch 'upstream/master' into lds-struct
htuch Aug 22, 2017
51a7f60
Rename to createEmptyConfigProto(), better comments.
htuch Aug 22, 2017
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
2 changes: 2 additions & 0 deletions docs/configuration/listeners/listeners.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ statistics:
listener_added, Counter, Total listeners added (either via static config or LDS)
listener_modified, Counter, Total listeners modified (via LDS)
listener_removed, Counter, Total listeners removed (via LDS)
listener_create_success, Counter, Total listener objects successfully added to workers.
listener_create_failure, Counter, Total failed listener object additions to workers.
total_listeners_warming, Gauge, Number of currently warming listeners
total_listeners_active, Gauge, Number of currently active listeners
total_listeners_draining, Gauge, Number of currently draining listeners
2 changes: 2 additions & 0 deletions include/envoy/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ envoy_cc_library(
"//include/envoy/thread_local:thread_local_interface",
"//include/envoy/tracing:http_tracer_interface",
"//include/envoy/upstream:cluster_manager_interface",
"//source/common/common:macros",
"//source/common/protobuf",
],
)

Expand Down
44 changes: 44 additions & 0 deletions include/envoy/server/filter_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
#include "envoy/tracing/http_tracer.h"
#include "envoy/upstream/cluster_manager.h"

#include "common/common/macros.h"
#include "common/protobuf/protobuf.h"

namespace Envoy {
namespace Server {

Expand Down Expand Up @@ -150,6 +153,25 @@ class NamedNetworkFilterConfigFactory {
virtual NetworkFilterFactoryCb createFilterFactory(const Json::Object& config,
FactoryContext& context) PURE;

/**
* v2 variant of createFilterFactory(..), where filter configs are specified as proto. This may be
* optionally implemented today, but will in the future become compulsory once v1 is deprecated.
*/
virtual NetworkFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message& config,
FactoryContext& context) {
UNREFERENCED_PARAMETER(config);
UNREFERENCED_PARAMETER(context);
return NetworkFilterFactoryCb();
}

/**
* @return ProtobufTypes::MessagePtr create empty config proto message for v2. The filter
* config, which arrives in an opaque google.protobuf.Struct message, will be converted to
* JSON and then parsed into this empty proto. Optional today, will be compulsory when v1
* is deprecated.
*/
virtual ProtobufTypes::MessagePtr createEmptyConfigProto() { return nullptr; }

/**
* @return std::string the identifying name for a particular implementation of a network filter
* produced by the factory.
Expand Down Expand Up @@ -197,6 +219,28 @@ class NamedHttpFilterConfigFactory {
const std::string& stat_prefix,
FactoryContext& context) PURE;

/**
* v2 API variant of createFilterFactory(..), where filter configs are specified as proto. This
* may be optionally implemented today, but will in the future become compulsory once v1 is
* deprecated.
*/
virtual HttpFilterFactoryCb createFilterFactoryFromProto(const ProtobufWkt::Message& config,
const std::string& stat_prefix,
FactoryContext& context) {
UNREFERENCED_PARAMETER(config);
UNREFERENCED_PARAMETER(stat_prefix);
UNREFERENCED_PARAMETER(context);
return HttpFilterFactoryCb();
}

/**
* @return ProtobufTypes::MessagePtr create empty config proto message for v2. The filter
* config, which arrives in an opaque google.protobuf.Struct message, will be converted to
* JSON and then parsed into this empty proto. Optional today, will be compulsory when v1
* is deprecated.
*/
virtual ProtobufTypes::MessagePtr createEmptyConfigProto() { return nullptr; }

/**
* @return std::string the identifying name for a particular implementation of an http filter
* produced by the factory.
Expand Down
6 changes: 4 additions & 2 deletions source/common/config/filter_json.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,10 @@ void FilterJson::translateHttpConnectionManager(
JSON_UTIL_SET_STRING(*json_filter, *filter, name);
JSON_UTIL_SET_STRING(*json_filter, *filter->mutable_deprecated_v1(), type);

const auto status = Protobuf::util::JsonStringToMessage(
json_filter->getObject("config")->asJsonString(), filter->mutable_config());
const std::string json_config = "{\"deprecatedV1\": true, \"value\": " +
json_filter->getObject("config")->asJsonString() + "}";

const auto status = Protobuf::util::JsonStringToMessage(json_config, filter->mutable_config());
// JSON schema has already validated that this is a valid JSON object.
ASSERT(status.ok());
UNREFERENCED_PARAMETER(status);
Expand Down
6 changes: 4 additions & 2 deletions source/common/config/lds_json.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ void LdsJson::translateListener(const Json::Object& json_listener,
JSON_UTIL_SET_STRING(*json_filter, *filter, name);
JSON_UTIL_SET_STRING(*json_filter, *filter->mutable_deprecated_v1(), type);

const auto status = Protobuf::util::JsonStringToMessage(
json_filter->getObject("config")->asJsonString(), filter->mutable_config());
const std::string json_config = "{\"deprecatedV1\": true, \"value\": " +
json_filter->getObject("config")->asJsonString() + "}";

const auto status = Protobuf::util::JsonStringToMessage(json_config, filter->mutable_config());
// JSON schema has already validated that this is a valid JSON object.
ASSERT(status.ok());
UNREFERENCED_PARAMETER(status);
Expand Down
1 change: 1 addition & 0 deletions source/common/protobuf/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ envoy_cc_library(
external_deps = ["protobuf"],
deps = [
":protobuf",
"//source/common/common:assert_lib",
"//source/common/common:utility_lib",
"//source/common/filesystem:filesystem_lib",
"//source/common/json:json_loader_lib",
Expand Down
3 changes: 3 additions & 0 deletions source/common/protobuf/protobuf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "google/protobuf/descriptor.h"
#include "google/protobuf/descriptor.pb.h"
#include "google/protobuf/empty.pb.h"
#include "google/protobuf/io/coded_stream.h"
#include "google/protobuf/io/zero_copy_stream.h"
#include "google/protobuf/io/zero_copy_stream_impl_lite.h"
Expand Down Expand Up @@ -42,6 +43,8 @@ namespace ProtobufWkt = google::protobuf;
// import.
namespace ProtobufTypes {

typedef std::unique_ptr<Protobuf::Message> MessagePtr;

typedef std::string String;
typedef int64_t Int64;

Expand Down
25 changes: 19 additions & 6 deletions source/common/protobuf/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "common/protobuf/utility.h"

#include "common/common/assert.h"
#include "common/filesystem/filesystem_impl.h"
#include "common/json/json_loader.h"

Expand All @@ -12,18 +13,30 @@ MissingFieldException::MissingFieldException(const std::string& field_name,
: EnvoyException(
fmt::format("Field '{}' is missing in: {}", field_name, message.DebugString())) {}

void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message) {
const auto status = Protobuf::util::JsonStringToMessage(json, &message);
if (!status.ok()) {
throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + "): " + json);
}
}

void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& message) {
ProtobufUtil::Status status;
const std::string contents = Filesystem::fileReadToEnd(path);
if (StringUtil::endsWith(path, ".yaml")) {
const std::string json = Json::Factory::loadFromYamlString(contents)->asJsonString();
status = Protobuf::util::JsonStringToMessage(json, &message);
loadFromJson(json, message);
} else {
status = Protobuf::util::JsonStringToMessage(contents, &message);
}
if (!status.ok()) {
throw EnvoyException("Unable to parse JSON as proto: " + contents);
loadFromJson(contents, message);
}
}

Json::ObjectSharedPtr WktUtil::getJsonObjectFromStruct(const Protobuf::Struct& message) {
Protobuf::util::JsonOptions json_options;
ProtobufTypes::String json;
const auto status = Protobuf::util::MessageToJsonString(message, &json, json_options);
// This should always succeed unless something crash-worthy such as out-of-memory.
RELEASE_ASSERT(status.ok());
return Json::Factory::loadFromString(json);
}

} // namespace Envoy
21 changes: 20 additions & 1 deletion source/common/protobuf/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "envoy/common/exception.h"
#include "envoy/json/json_object.h"

#include "common/common/utility.h"
#include "common/protobuf/protobuf.h"
Expand Down Expand Up @@ -48,10 +49,28 @@ class RepeatedPtrUtil {
class MessageUtil {
public:
static std::size_t hash(const Protobuf::Message& message) {
return std::hash<std::string>{}(message.SerializeAsString());
// Use Protobuf::io::CodedOutputStream to force deterministic serialization, so that the same
// message doesn't hash to different values.
std::string text;
Protobuf::io::StringOutputStream string_stream(&text);
Protobuf::io::CodedOutputStream coded_stream(&string_stream);
coded_stream.SetSerializationDeterministic(true);
message.SerializeToCodedStream(&coded_stream);
return std::hash<std::string>{}(text);
}

static void loadFromJson(const std::string& json, Protobuf::Message& message);
static void loadFromFile(const std::string& path, Protobuf::Message& message);
};

class WktUtil {
public:
/**
* Extract JSON object from a google.protobuf.Struct.
* @param message message of type type.googleapis.com/google.protobuf.Struct.
* @return Json::ObjectSharedPtr of JSON object or nullptr if unable to extract.
*/
static Json::ObjectSharedPtr getJsonObjectFromStruct(const Protobuf::Struct& message);
};

} // namespace Envoy
56 changes: 38 additions & 18 deletions source/server/config/network/http_connection_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,11 @@ const std::string HttpConnectionManagerConfig::DEFAULT_SERVER_STRING = "envoy";
SINGLETON_MANAGER_REGISTRATION(date_provider);
SINGLETON_MANAGER_REGISTRATION(route_config_provider_manager);

NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactory(
const Json::Object& json_http_connection_manager, FactoryContext& context) {
namespace {

NetworkFilterFactoryCb createHttpConnectionManagerFilterFactory(
const envoy::api::v2::filter::HttpConnectionManager& http_connection_manager,
FactoryContext& context) {
std::shared_ptr<Http::TlsCachingDateProviderImpl> date_provider =
context.singletonManager().getTyped<Http::TlsCachingDateProviderImpl>(
SINGLETON_MANAGER_REGISTERED_NAME(date_provider), [&context] {
Expand All @@ -50,9 +53,6 @@ NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFac
context.threadLocal());
});

envoy::api::v2::filter::HttpConnectionManager http_connection_manager;
Config::FilterJson::translateHttpConnectionManager(json_http_connection_manager,
http_connection_manager);
std::shared_ptr<HttpConnectionManagerConfig> http_config(new HttpConnectionManagerConfig(
http_connection_manager, context, *date_provider, *route_config_provider_manager));

Expand All @@ -67,6 +67,22 @@ NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFac
};
}

} // namespace

NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactory(
const Json::Object& json_http_connection_manager, FactoryContext& context) {
envoy::api::v2::filter::HttpConnectionManager http_connection_manager;
Config::FilterJson::translateHttpConnectionManager(json_http_connection_manager,
http_connection_manager);
return createHttpConnectionManagerFilterFactory(http_connection_manager, context);
}

NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProto(
const Protobuf::Message& config, FactoryContext& context) {
return createHttpConnectionManagerFilterFactory(
dynamic_cast<const envoy::api::v2::filter::HttpConnectionManager&>(config), context);
Copy link
Member

Choose a reason for hiding this comment

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

Just to check my understanding here: For a struct, whether JSON or proto, the type if embedded, so when it is actually parsed, it becomes the real object? Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type doesn't appear in the config at all, all we have is a google.protobuf.Struct knowledge when we encounter an opaque config. We then convert this Struct to JSON and then load it back into a Protobuf::Message (superclass of all concrete proto types), by invoking Protobuf::util::JsonStringToMessage(json, &message). This function makes use of the type information in the supplied message to figure out how to do the parsing of the JSON.

This is why we need createConfigProto(), it's essentially providing a JSON->message conversion factory via the proto message it returns. If you can think of a cleaner way to do this, I'm open to that as well (we could literally renamed to createConfigProtoFactory and give it some more abstract interface if you prefer).

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that HttpConnectionManager message actually never appears on the wire, but is used internally by Envoy only? I.e., to pass in http connection manager config via a protobuf I have to map from the HttpConnectionManager message definition to a protobuf Struct, field by field. If so, what is the point of specifying the HttpConnectionMananger message in the envoy-api?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could not figure it out, managed to get it to work by encoding to Struct, seems to work but looks quite inefficient coding-wise:

    filter_chains {
      filters {
        name: "http_connection_manager"
        config {
          fields {
            key: "rds"
            value {
              struct_value {
                fields {
                  key: "config_source"
                  value {
                    struct_value {
                      fields {
                        key: "api_config_source"
                        value {
                          struct_value {
                            fields {
                              key: "api_type"
                              value {
                                number_value: 2
                              }
                            }
                            fields {
                              key: "cluster_name"
                              value {
                                string_value: "rdsCluster"
                              }
                            }
                          }
                        }
                      }
                    }
                  }
                }
                fields {
                  key: "route_config_name"
                  value {
                    string_value: "router1"
                  }
                }
              }
            }
          }
          fields {
            key: "stat_prefix"
            value {
              string_value: "proxy"
            }
          }
        }
        deprecated_v1 {
          type: "read"
        }
      }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it only appears as a Struct. What you have is correct. The goal here is not efficiency, but being extensible, we don't want to predefine all filter config protos upfront, we want to support user extension. The alternative google.protobuf.Any type was not attractive as it is hard to do anything with the encoded proto without the original schema, e.g. dumping debug, assembling in config generation tools.

This is why we use JSON/YAML for textual representation, the ugliness of Struct is hidden.

Copy link
Member Author

@htuch htuch Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm interested in understanding the efficiency requirements here. Two questions:

  1. Given this is control plane, I wouldn't normally be that concerned about slight encoding inefficiency. Do you have some very significant scaling requirements in terms of the size/frequency of config update?
  2. I haven't looked at the wire format for Struct, but I assume the main objection is the fact that the key string is included, which is not needed if you know the proto upfront.

TBH, this is part of the API which we've tentatively frozen, so it would take a compelling reason to make this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any numbers now, but you are likely right in that performance difference does not matter here. Even so it seems to me that there is much bigger difference in encoding/decoding CPU/memory use than in the wire format. Given that one of the main rationales of protobufs vs. XML (for example) is >10x encoding/decoding performance difference this seems like going backwards.

On the second point the change could be made backwards compatible by keeping the code point 2 for Struct config, and adding new code point(s) for Any or Oneof Message configs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the best thing to do at this point is open an issue and let's see what others in the community think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@PiotrSikora

Copy link
Contributor

Choose a reason for hiding this comment

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

See #1680.

}

/**
* Static registration for the HTTP connection manager filter.
*/
Expand Down Expand Up @@ -210,31 +226,35 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
ENVOY_LOG(info, " filter #{}", i);
ENVOY_LOG(info, " name: {}", string_name);

Protobuf::util::JsonOptions json_options;
ProtobufTypes::String json_config;
const auto status =
Protobuf::util::MessageToJsonString(proto_config, &json_config, json_options);
// This should always succeed unless something crash-worthy such as out-of-memory.
RELEASE_ASSERT(status.ok());
UNREFERENCED_PARAMETER(status);
const Json::ObjectSharedPtr filter_config = Json::Factory::loadFromString(json_config);

const Json::ObjectSharedPtr filter_config = WktUtil::getJsonObjectFromStruct(proto_config);
const HttpFilterType type = stringToType(string_type);

// Now see if there is a factory that will accept the config.
NamedHttpFilterConfigFactory* factory =
Registry::FactoryRegistry<NamedHttpFilterConfigFactory>::getFactory(string_name);
if (factory != nullptr) {
HttpFilterFactoryCb callback =
factory->createFilterFactory(*filter_config, stats_prefix_, context_);
HttpFilterFactoryCb callback;
if (filter_config->getBoolean("deprecatedV1", false)) {
callback = factory->createFilterFactory(*filter_config->getObject("value", true),
stats_prefix_, context);
} else {
auto message = factory->createEmptyConfigProto();
if (!message) {
throw EnvoyException(
fmt::format("Filter factory for '{}' has unexpected proto config", string_name));
}
MessageUtil::loadFromJson(filter_config->asJsonString(), *message);
callback = factory->createFilterFactoryFromProto(*message, stats_prefix_, context);
}
filter_factories_.push_back(callback);
} else {
// DEPRECATED
// This name wasn't found in the named map, so search in the deprecated list registry.
bool found_filter = false;
for (HttpFilterConfigFactory* config_factory : filterConfigFactories()) {
HttpFilterFactoryCb callback = config_factory->tryCreateFilterFactory(
type, string_name, *filter_config, stats_prefix_, context_.server());
type, string_name, *filter_config->getObject("value", true), stats_prefix_,
context_.server());
if (callback) {
filter_factories_.push_back(callback);
found_filter = true;
Expand Down Expand Up @@ -287,7 +307,7 @@ HttpFilterType HttpConnectionManagerConfig::stringToType(const std::string& type
} else if (type == "encoder") {
return HttpFilterType::Encoder;
} else {
ASSERT(type == "both");
ASSERT(type == "both" || type.empty());
return HttpFilterType::Both;
}
}
Expand Down
6 changes: 6 additions & 0 deletions source/server/config/network/http_connection_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@ class HttpConnectionManagerFilterConfigFactory : Logger::Loggable<Logger::Id::co
// NamedNetworkFilterConfigFactory
NetworkFilterFactoryCb createFilterFactory(const Json::Object& config,
FactoryContext& context) override;
NetworkFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message& config,
FactoryContext& context) override;

ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return std::unique_ptr<envoy::api::v2::filter::HttpConnectionManager>(
new envoy::api::v2::filter::HttpConnectionManager());
}
std::string name() override { return "http_connection_manager"; }
NetworkFilterType type() override { return NetworkFilterType::Read; }
};
Expand Down
6 changes: 5 additions & 1 deletion source/server/configuration_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ void MainImpl::initialize(const Json::Object& json, const envoy::api::v2::Bootst
server.listenerManager().addOrUpdateListener(listener);
}

if (json.hasObject("lds")) {
if (bootstrap.has_lds_config()) {
lds_api_.reset(new LdsApi(bootstrap.lds_config(), *cluster_manager_, server.dispatcher(),
server.random(), server.initManager(), server.localInfo(),
server.stats(), server.listenerManager()));
} else if (json.hasObject("lds")) {
envoy::api::v2::ConfigSource lds_config;
Config::Utility::translateLdsConfig(*json.getObject("lds"), lds_config);
lds_api_.reset(new LdsApi(lds_config, *cluster_manager_, server.dispatcher(), server.random(),
Expand Down
Loading