Skip to content

rds: allow v1 rds config to use a v2 subscription #1583

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

Merged
htuch merged 3 commits into from
Sep 1, 2017
Merged

rds: allow v1 rds config to use a v2 subscription #1583

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5a58c5
rds: allow v1 rds config to use a v2 subscription
Aug 31, 2017
d5d31b2
tests
Sep 1, 2017
6f52abd
nits
Sep 1, 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
32 changes: 22 additions & 10 deletions source/common/config/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,27 @@
namespace Envoy {
namespace Config {

namespace {

void translateApiConfigSource(const std::string& cluster, uint32_t refresh_delay_ms,
envoy::api::v2::ApiConfigSource& api_config_source) {
api_config_source.set_api_type(envoy::api::v2::ApiConfigSource::REST_LEGACY);
const std::string Utility::REST_LEGACY = "REST_LEGACY";
const std::string Utility::REST = "REST";
const std::string Utility::GRPC = "GRPC";

void Utility::translateApiConfigSource(const std::string& cluster, uint32_t refresh_delay_ms,
const std::string& api_type,
envoy::api::v2::ApiConfigSource& api_config_source) {
// TODO(junr03): document the option to chose an api type once we have created
// stronger constraints around v2.
if (api_type == REST_LEGACY) {
api_config_source.set_api_type(envoy::api::v2::ApiConfigSource::REST_LEGACY);
} else if (api_type == REST) {
api_config_source.set_api_type(envoy::api::v2::ApiConfigSource::REST);
} else if (api_type == GRPC) {

@htuch htuch Sep 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should probably be:

} else {
  ASERT(api_type == GRPC);
  ...
}

since the JSON schema knows this is an enum and has validated.

api_config_source.set_api_type(envoy::api::v2::ApiConfigSource::GRPC);
}
api_config_source.add_cluster_name(cluster);
api_config_source.mutable_refresh_delay()->CopyFrom(
Protobuf::util::TimeUtil::MillisecondsToDuration(refresh_delay_ms));
}

} // namespace

void Utility::checkCluster(const std::string& error_prefix, const std::string& cluster_name,
Upstream::ClusterManager& cm) {
Upstream::ThreadLocalCluster* cluster = cm.get(cluster_name);
Expand Down Expand Up @@ -63,30 +72,33 @@ void Utility::translateEdsConfig(const Json::Object& json_config,
envoy::api::v2::ConfigSource& eds_config) {
translateApiConfigSource(json_config.getObject("cluster")->getString("name"),
json_config.getInteger("refresh_delay_ms", 30000),
json_config.getString("api_type", REST_LEGACY),
*eds_config.mutable_api_config_source());
}

void Utility::translateCdsConfig(const Json::Object& json_config,
envoy::api::v2::ConfigSource& cds_config) {
translateApiConfigSource(json_config.getObject("cluster")->getString("name"),
json_config.getInteger("refresh_delay_ms", 30000),
json_config.getString("api_type", REST_LEGACY),
*cds_config.mutable_api_config_source());
}

void Utility::translateRdsConfig(const Json::Object& json_rds, envoy::api::v2::filter::Rds& rds) {
json_rds.validateSchema(Json::Schema::RDS_CONFIGURATION_SCHEMA);
translateApiConfigSource(json_rds.getString("cluster"),
json_rds.getInteger("refresh_delay_ms", 30000),
json_rds.getString("api_type", REST_LEGACY),
*rds.mutable_config_source()->mutable_api_config_source());
JSON_UTIL_SET_STRING(json_rds, rds, route_config_name);
}

void Utility::translateLdsConfig(const Json::Object& json_lds,
envoy::api::v2::ConfigSource& lds_config) {
json_lds.validateSchema(Json::Schema::LDS_CONFIG_SCHEMA);
translateApiConfigSource(json_lds.getString("cluster"),
json_lds.getInteger("refresh_delay_ms", 30000),
*lds_config.mutable_api_config_source());
translateApiConfigSource(
json_lds.getString("cluster"), json_lds.getInteger("refresh_delay_ms", 30000),
json_lds.getString("api_type", REST_LEGACY), *lds_config.mutable_api_config_source());
}

} // namespace Config
Expand Down
15 changes: 15 additions & 0 deletions source/common/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ class Utility {
static std::chrono::milliseconds
apiConfigSourceRefreshDelay(const envoy::api::v2::ApiConfigSource& api_config_source);

/**
* Populate an envoy::api::v2::ApiConfigSource.
* @param cluster supplies the cluster name for the ApiConfigSource.
* @param refresh_delay_ms supplies the refresh delay for the ApiConfigSource in ms.
* @param api_type supplies the type of subscription to use for the ApiConfigSource.
* @param api_config_source a reference to the envoy::api::v2::ApiConfigSource object to populate.
*/
static void translateApiConfigSource(const std::string& cluster, uint32_t refresh_delay_ms,
const std::string& api_type,
envoy::api::v2::ApiConfigSource& api_config_source);

/**
* Check cluster info for API config sanity. Throws on error.
* @param error_prefix supplies the prefix to use in error messages.
Expand Down Expand Up @@ -108,6 +119,10 @@ class Utility {
static SubscriptionStats generateStats(Stats::Scope& scope) {
return {ALL_SUBSCRIPTION_STATS(POOL_COUNTER(scope))};
}

static const std::string REST_LEGACY;

@mattklein123 mattklein123 Sep 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: This should use the const singleton pattern. See Http::Headers

@junr03 junr03 Sep 1, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, I am confused about this. If declare and initialize like in source/common/http/headers.h with curly brace initialization the compiler complains about in-class initialization of static data member of non-literal type, but of course I can't make them non-static because I would be using the variable in a static member function. What am I missing?

@htuch htuch Sep 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You create a class with non-static members, and then access it via the static get() in https://github.com/lyft/envoy/blob/master/source/common/common/singleton.h.

@junr03 junr03 Sep 1, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah gotcha, that pattern!

@junr03 junr03 Sep 1, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nice this makes it better. I knew I had seen this as the Const counter part to the mutable singleton, should have scrolled to the end of the file. Another tool in the toolshed 👍

static const std::string REST;
static const std::string GRPC;
};

} // namespace Config
Expand Down
4 changes: 4 additions & 0 deletions source/common/json/config_schemas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ const std::string Json::Schema::RDS_CONFIGURATION_SCHEMA(R"EOF(
"type" : "integer",
"minimum" : 0,
"exclusiveMinimum" : true
},
"api_type" : {
"type" : "string",
"enum" : ["REST_LEGACY", "REST", "GRPC"]
}
},
"required" : ["cluster", "route_config_name"],
Expand Down
26 changes: 26 additions & 0 deletions test/common/config/utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,31 @@ TEST(UtilityTest, ApiConfigSourceRefreshDelay) {
EXPECT_EQ(1234, Utility::apiConfigSourceRefreshDelay(api_config_source).count());
}

TEST(UtilityTest, TranslateApiConfigSource) {
envoy::api::v2::ApiConfigSource api_config_source_rest_legacy;
Utility::translateApiConfigSource("test_rest_legacy_cluster", 10000, Utility::REST_LEGACY,
api_config_source_rest_legacy);
EXPECT_EQ(envoy::api::v2::ApiConfigSource::REST_LEGACY, api_config_source_rest_legacy.api_type());
EXPECT_EQ(10000, Protobuf::util::TimeUtil::DurationToMilliseconds(
api_config_source_rest_legacy.refresh_delay()));
EXPECT_EQ("test_rest_legacy_cluster", api_config_source_rest_legacy.cluster_name(0));

envoy::api::v2::ApiConfigSource api_config_source_rest;
Utility::translateApiConfigSource("test_rest_cluster", 20000, Utility::REST,
api_config_source_rest);
EXPECT_EQ(envoy::api::v2::ApiConfigSource::REST, api_config_source_rest.api_type());
EXPECT_EQ(20000, Protobuf::util::TimeUtil::DurationToMilliseconds(
api_config_source_rest.refresh_delay()));
EXPECT_EQ("test_rest_cluster", api_config_source_rest.cluster_name(0));

envoy::api::v2::ApiConfigSource api_config_source_grpc;
Utility::translateApiConfigSource("test_grpc_cluster", 30000, Utility::GRPC,
api_config_source_grpc);
EXPECT_EQ(envoy::api::v2::ApiConfigSource::GRPC, api_config_source_grpc.api_type());
EXPECT_EQ(30000, Protobuf::util::TimeUtil::DurationToMilliseconds(
api_config_source_grpc.refresh_delay()));
EXPECT_EQ("test_grpc_cluster", api_config_source_grpc.cluster_name(0));
}

} // namespace Config
} // namespace Envoy