Skip to content

(Don't use this commit) make mixer cluster name configurable #993

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
istio-merge-robot merged 9 commits into from
Feb 6, 2018
Merged

(Don't use this commit) make mixer cluster name configurable #993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b99cfe8
make mixer cluster configurable
Feb 5, 2018
e5bcd46
Merge branch 'master' into config_mixer_cluster
Feb 6, 2018
7cf9598
nits
Feb 6, 2018
136d9e8
format
Feb 6, 2018
2b4e7fd
fixes
Feb 6, 2018
f4fb057
fixes
Feb 6, 2018
a66e6ff
Merge branch 'master' into config_mixer_cluster
Feb 6, 2018
4ea060d
fix
Feb 6, 2018
d8bce6f
format
Feb 6, 2018
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
26 changes: 23 additions & 3 deletions src/envoy/mixer/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ const std::string kDisableTcpCheckCalls("disable_tcp_check_calls");

const std::string kV2Config("v2");

// The name for the mixer server cluster.
const std::string kDefaultMixerClusterName("mixer_server");

void ReadStringMap(const Json::Object& json, const std::string& name,
Attributes* attributes) {
if (json.hasObject(name)) {
Expand All @@ -62,7 +65,8 @@ void ReadStringMap(const Json::Object& json, const std::string& name,
}
}

void ReadTransportConfig(const Json::Object& json, TransportConfig* config) {
void ReadLegacyTransportConfig(const Json::Object& json,
TransportConfig* config) {
// Default is open, unless it specifically set to "close"
config->set_network_fail_policy(TransportConfig::FAIL_OPEN);
if (json.hasObject(kNetworkFailPolicy) &&
Expand Down Expand Up @@ -110,13 +114,21 @@ void HttpMixerConfig::Load(const Json::Object& json) {
legacy_quotas.push_back({json.getString(kQuotaName), amount});
}

ReadTransportConfig(json, http_config.mutable_transport());
TransportConfig* transport_config = http_config.mutable_transport();
ReadLegacyTransportConfig(json, transport_config);

has_v2_config = ReadV2Config(json, &http_config);
if (has_v2_config) {
// If v2 config is valid, clear v1 legacy_quotas.
legacy_quotas.clear();
}

check_cluster = transport_config->check_cluster().empty()
? kDefaultMixerClusterName
: transport_config->check_cluster();
report_cluster = transport_config->report_cluster().empty()
? kDefaultMixerClusterName
: transport_config->report_cluster();
}

void HttpMixerConfig::CreateLegacyRouteConfig(
Expand All @@ -135,12 +147,20 @@ void HttpMixerConfig::CreateLegacyRouteConfig(
void TcpMixerConfig::Load(const Json::Object& json) {
ReadStringMap(json, kMixerAttributes, tcp_config.mutable_mixer_attributes());

ReadTransportConfig(json, tcp_config.mutable_transport());
TransportConfig* transport_config = tcp_config.mutable_transport();
ReadLegacyTransportConfig(json, transport_config);

tcp_config.set_disable_check_calls(
json.getBoolean(kDisableTcpCheckCalls, false));

ReadV2Config(json, &tcp_config);

check_cluster = transport_config->check_cluster().empty()
? kDefaultMixerClusterName
: transport_config->check_cluster();
report_cluster = transport_config->report_cluster().empty()
? kDefaultMixerClusterName
: transport_config->report_cluster();
}

} // namespace Mixer
Expand Down
8 changes: 8 additions & 0 deletions src/envoy/mixer/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ struct HttpMixerConfig {
std::vector<::istio::quota::Requirement> legacy_quotas;
// If true, v2 config is valid.
bool has_v2_config;
// check cluster
std::string check_cluster;
// report cluster
std::string report_cluster;

// Create per route legacy config.
static void CreateLegacyRouteConfig(
Expand All @@ -54,6 +58,10 @@ struct TcpMixerConfig {

// The Tcp client config.
::istio::mixer::v1::config::client::TcpClientConfig tcp_config;
// check cluster
std::string check_cluster;
// report cluster
std::string report_cluster;
};

} // namespace Mixer
Expand Down
21 changes: 11 additions & 10 deletions src/envoy/mixer/grpc_transport.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ const LowerCaseString kB3Sampled("x-b3-sampled");
const LowerCaseString kB3Flags("x-b3-flags");
const LowerCaseString kOtSpanContext("x-ot-span-context");

// The name for the mixer server cluster.
const char* kMixerServerClusterName = "mixer_server";

inline void CopyHeaderEntry(const HeaderEntry* entry,
const LowerCaseString& key,
Http::HeaderMap& headers) {
Expand Down Expand Up @@ -109,14 +106,16 @@ void GrpcTransport<RequestType, ResponseType>::Cancel() {

template <class RequestType, class ResponseType>
typename GrpcTransport<RequestType, ResponseType>::Func
GrpcTransport<RequestType, ResponseType>::GetFunc(Upstream::ClusterManager& cm,
const HeaderMap* headers) {
return [&cm, headers](const RequestType& request, ResponseType* response,
istio::mixer_client::DoneFunc on_done)
GrpcTransport<RequestType, ResponseType>::GetFunc(
Upstream::ClusterManager& cm, const std::string& cluster_name,
const HeaderMap* headers) {
return [&cm, cluster_name, headers](const RequestType& request,
ResponseType* response,
istio::mixer_client::DoneFunc on_done)
-> istio::mixer_client::CancelFunc {
auto transport = new GrpcTransport<RequestType, ResponseType>(
Grpc::AsyncClientPtr(
new Grpc::AsyncClientImpl(cm, kMixerServerClusterName)),
new Grpc::AsyncClientImpl(cm, cluster_name.c_str())),
request, headers, response, on_done);
return [transport]() { transport->Cancel(); };
};
Expand All @@ -142,9 +141,11 @@ const google::protobuf::MethodDescriptor& ReportTransport::descriptor() {

// explicitly instantiate CheckTransport and ReportTransport
template CheckTransport::Func CheckTransport::GetFunc(
Upstream::ClusterManager& cm, const HeaderMap* headers);
Upstream::ClusterManager& cm, const std::string& cluster_name,
const HeaderMap* headers);
template ReportTransport::Func ReportTransport::GetFunc(
Upstream::ClusterManager& cm, const HeaderMap* headers);
Upstream::ClusterManager& cm, const std::string& cluster_name,
const HeaderMap* headers);

} // namespace Mixer
} // namespace Http
Expand Down
1 change: 1 addition & 0 deletions src/envoy/mixer/grpc_transport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks<ResponseType>,
istio::mixer_client::DoneFunc on_done)>;

static Func GetFunc(Upstream::ClusterManager& cm,
const std::string& cluster_name,
const HeaderMap* headers = nullptr);

GrpcTransport(Grpc::AsyncClientPtr async_client, const RequestType& request,
Expand Down
3 changes: 2 additions & 1 deletion src/envoy/mixer/http_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,8 @@ class Instance : public Http::StreamDecoderFilter,
HeaderUpdate header_update(&headers);
cancel_check_ = handler_->Check(
&check_data, &header_update,
CheckTransport::GetFunc(mixer_control_.cm(), &headers),
CheckTransport::GetFunc(mixer_control_.cm(),
mixer_control_.check_cluster(), &headers),
[this](const Status& status) { completeCheck(status); });
initiating_call_ = false;

Expand Down
19 changes: 14 additions & 5 deletions src/envoy/mixer/mixer_control.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,11 @@ class EnvoyTimer : public ::istio::mixer_client::Timer {
void CreateEnvironment(Upstream::ClusterManager& cm,
Event::Dispatcher& dispatcher,
Runtime::RandomGenerator& random,
const std::string& check_cluster,
const std::string& report_cluster,
::istio::mixer_client::Environment* env) {
env->check_transport = CheckTransport::GetFunc(cm, nullptr);
env->report_transport = ReportTransport::GetFunc(cm);
env->check_transport = CheckTransport::GetFunc(cm, check_cluster, nullptr);
env->report_transport = ReportTransport::GetFunc(cm, report_cluster);

env->timer_create_func = [&dispatcher](std::function<void()> timer_cb)
-> std::unique_ptr<::istio::mixer_client::Timer> {
Expand All @@ -70,10 +72,13 @@ HttpMixerControl::HttpMixerControl(const HttpMixerConfig& mixer_config,
::istio::mixer_control::http::Controller::Options options(
mixer_config.http_config, mixer_config.legacy_quotas);

CreateEnvironment(cm, dispatcher, random, &options.env);
check_cluster_ = mixer_config.check_cluster;
report_cluster_ = mixer_config.report_cluster;

controller_ = ::istio::mixer_control::http::Controller::Create(options);
CreateEnvironment(cm, dispatcher, random, check_cluster_, report_cluster_,
Copy link
Copy Markdown
Contributor

@qiwzhang qiwzhang Feb 6, 2018

Choose a reason for hiding this comment

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

I think you will not need check_cluster_ and report_cluster_ in MixerControl object if you don't pass reference to CreateEnvironment(). Just pass std::string and make a copy

Copy link
Copy Markdown
Contributor

@lizan lizan Feb 6, 2018

Choose a reason for hiding this comment

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

passing const reference is fine as long as the lamda in GetFunc captures cluster_name as copy, so just remove check_cluster_ and report_cluster_ is good.

Copy link
Copy Markdown
Member Author

@rshriram rshriram Feb 6, 2018

Choose a reason for hiding this comment

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

Oh I needed to persist these variables in mixer control object because there is a GetFunc call in http_filter.cc that does mixer_control_->... ..
Ideally, i would have liked to stuff them into the options object, but that happens to be defined in mixerclient (spreading to too many repos).

Does that your point @qiwzhang ?

Copy link
Copy Markdown
Contributor

@qiwzhang qiwzhang Feb 6, 2018

Choose a reason for hiding this comment

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

I see. That one is per-route Check transport function. My suggestion is:

in both HTTPMixerControl and TcpMixerControl store a const reference to MixerConfig

const MixerConfig& config_;

Expose it as

const MxerConfig& config() const { return config_; }

use the check_cluster_ as

config()->check_cluster()

It is better than store anothery copy in MixerControl

Copy link
Copy Markdown
Contributor

@qiwzhang qiwzhang Feb 6, 2018

Choose a reason for hiding this comment

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

Well, since the PR is merged. I will clean it up later.

Copy link
Copy Markdown
Contributor

@qiwzhang qiwzhang Feb 6, 2018

Choose a reason for hiding this comment

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

Here is my PR #1001
to clean this up

&options.env);

controller_ = ::istio::mixer_control::http::Controller::Create(options);
has_v2_config_ = mixer_config.has_v2_config;
}

Expand All @@ -85,7 +90,11 @@ TcpMixerControl::TcpMixerControl(const TcpMixerConfig& mixer_config,
::istio::mixer_control::tcp::Controller::Options options(
mixer_config.tcp_config);

CreateEnvironment(cm, dispatcher, random, &options.env);
check_cluster_ = mixer_config.check_cluster;
report_cluster_ = mixer_config.report_cluster;

CreateEnvironment(cm, dispatcher, random, check_cluster_, report_cluster_,
&options.env);

controller_ = ::istio::mixer_control::tcp::Controller::Create(options);

Expand Down
17 changes: 17 additions & 0 deletions src/envoy/mixer/mixer_control.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class HttpMixerControl final : public ThreadLocal::ThreadLocalObject {

Upstream::ClusterManager& cm() { return cm_; }

const std::string& check_cluster() const { return check_cluster_; }

const std::string& report_cluster() const { return report_cluster_; }

::istio::mixer_control::http::Controller* controller() {
return controller_.get();
}
Expand All @@ -49,6 +53,10 @@ class HttpMixerControl final : public ThreadLocal::ThreadLocalObject {
std::unique_ptr<::istio::mixer_control::http::Controller> controller_;
// has v2 config;
bool has_v2_config_;
// check cluster
std::string check_cluster_;
// report cluster
std::string report_cluster_;
};

class TcpMixerControl final : public ThreadLocal::ThreadLocalObject {
Expand All @@ -66,6 +74,10 @@ class TcpMixerControl final : public ThreadLocal::ThreadLocalObject {
return report_interval_ms_;
}

const std::string& check_cluster() const { return check_cluster_; }

const std::string& report_cluster() const { return report_cluster_; }

Event::Dispatcher& dispatcher() { return dispatcher_; }

private:
Expand All @@ -76,6 +88,11 @@ class TcpMixerControl final : public ThreadLocal::ThreadLocalObject {
std::chrono::milliseconds report_interval_ms_;

Event::Dispatcher& dispatcher_;

// check cluster
std::string check_cluster_;
// report cluster
std::string report_cluster_;
};

} // namespace Mixer
Expand Down