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

Use string map instead of opaque struct for dynamic metadata #2014

Merged
merged 4 commits into from
Nov 1, 2018
Merged

Use string map instead of opaque struct for dynamic metadata #2014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb5b380
use string map instead of opaque struct for dynamic metadata
Nov 1, 2018
0db4bc2
format
Nov 1, 2018
d7fa289
double format
Nov 1, 2018
1959ddd
skip empty maps
Nov 1, 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
15 changes: 9 additions & 6 deletions include/istio/control/http/report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include <chrono>
#include <map>

#include "google/protobuf/struct.pb.h"

namespace istio {
namespace control {
namespace http {
Expand All @@ -42,31 +44,32 @@ class ReportData {
int response_code;
std::string response_flags;
};
virtual void GetReportInfo(ReportInfo* info) const = 0;
virtual void GetReportInfo(ReportInfo *info) const = 0;

// Get destination ip/port.
virtual bool GetDestinationIpPort(std::string* ip, int* port) const = 0;
virtual bool GetDestinationIpPort(std::string *ip, int *port) const = 0;

// Get Rbac attributes.
struct RbacReportInfo {
std::string permissive_resp_code;
std::string permissive_policy_id;
};
virtual bool GetRbacReportInfo(RbacReportInfo* report_info) const = 0;
virtual bool GetRbacReportInfo(RbacReportInfo *report_info) const = 0;

// Get upstream host UID. This value overrides the value in the report bag.
virtual bool GetDestinationUID(std::string* uid) const = 0;
virtual bool GetDestinationUID(std::string *uid) const = 0;

// gRPC status info
struct GrpcStatus {
std::string status;
std::string message;
};
virtual bool GetGrpcStatus(GrpcStatus* status) const = 0;
virtual bool GetGrpcStatus(GrpcStatus *status) const = 0;

// Get dynamic metadata generated by Envoy filters.
// Useful for logging info generated by custom codecs.
virtual bool GetDynamicFilterState(std::string* filter_state) const = 0;
virtual const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
&GetDynamicFilterState() const = 0;
};

} // namespace http
Expand Down
12 changes: 8 additions & 4 deletions include/istio/control/tcp/report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include <chrono>
#include <string>

#include "google/protobuf/struct.pb.h"

namespace istio {
namespace control {
namespace tcp {
Expand All @@ -30,18 +32,18 @@ class ReportData {
virtual ~ReportData() {}

// Get upstream tcp connection IP and port. IP is returned in format of bytes.
virtual bool GetDestinationIpPort(std::string* ip, int* port) const = 0;
virtual bool GetDestinationIpPort(std::string *ip, int *port) const = 0;

// Get additional report data.
struct ReportInfo {
uint64_t send_bytes;
uint64_t received_bytes;
std::chrono::nanoseconds duration;
};
virtual void GetReportInfo(ReportInfo* info) const = 0;
virtual void GetReportInfo(ReportInfo *info) const = 0;

// Get upstream host UID. This value overrides the value in the report bag.
virtual bool GetDestinationUID(std::string* uid) const = 0;
virtual bool GetDestinationUID(std::string *uid) const = 0;

// ConnectionEvent is used to indicates the tcp connection event in Report
// call.
Expand All @@ -53,7 +55,9 @@ class ReportData {

// Get dynamic metadata generated by Envoy filters.
// Useful for logging info generated by custom codecs.
virtual bool GetDynamicFilterState(std::string* filter_state) const = 0;
virtual const ::google::protobuf::Map<::std::string,
::google::protobuf::Struct>
&GetDynamicFilterState() const = 0;
};

} // namespace tcp
Expand Down
6 changes: 0 additions & 6 deletions include/istio/utils/attribute_names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ struct AttributeName {
static const char kRequestUserAgent[];
static const char kRequestApiKey[];

// Dynamic state generated by various envoy http filters
static const char kRequestDynamicState[];

static const char kResponseCode[];
static const char kResponseDuration[];
static const char kResponseHeaders[];
Expand Down Expand Up @@ -80,9 +77,6 @@ struct AttributeName {
// Record TCP connection status: open, continue, close
static const char kConnectionEvent[];

// Dynamic state generated by various envoy network filters
static const char kConnectionDynamicState[];

// Context attributes
static const char kContextProtocol[];
static const char kContextReporterKind[];
Expand Down
60 changes: 41 additions & 19 deletions include/istio/utils/attributes_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,32 +32,32 @@ namespace utils {
// .Add("key2", value2);
class AttributesBuilder {
public:
AttributesBuilder(::istio::mixer::v1::Attributes* attributes)
AttributesBuilder(::istio::mixer::v1::Attributes *attributes)
: attributes_(attributes) {}

void AddString(const std::string& key, const std::string& str) {
void AddString(const std::string &key, const std::string &str) {
(*attributes_->mutable_attributes())[key].set_string_value(str);
}

void AddBytes(const std::string& key, const std::string& bytes) {
void AddBytes(const std::string &key, const std::string &bytes) {
(*attributes_->mutable_attributes())[key].set_bytes_value(bytes);
}

void AddInt64(const std::string& key, int64_t value) {
void AddInt64(const std::string &key, int64_t value) {
(*attributes_->mutable_attributes())[key].set_int64_value(value);
}

void AddDouble(const std::string& key, double value) {
void AddDouble(const std::string &key, double value) {
(*attributes_->mutable_attributes())[key].set_double_value(value);
}

void AddBool(const std::string& key, bool value) {
void AddBool(const std::string &key, bool value) {
(*attributes_->mutable_attributes())[key].set_bool_value(value);
}

void AddTimestamp(
const std::string& key,
const std::chrono::time_point<std::chrono::system_clock>& value) {
const std::string &key,
const std::chrono::time_point<std::chrono::system_clock> &value) {
auto time_stamp =
(*attributes_->mutable_attributes())[key].mutable_timestamp_value();
long long nanos = std::chrono::duration_cast<std::chrono::nanoseconds>(
Expand All @@ -67,38 +67,38 @@ class AttributesBuilder {
time_stamp->set_nanos(nanos % 1000000000);
}

void AddDuration(const std::string& key,
const std::chrono::nanoseconds& value) {
void AddDuration(const std::string &key,
const std::chrono::nanoseconds &value) {
auto duration =
(*attributes_->mutable_attributes())[key].mutable_duration_value();
duration->set_seconds(value.count() / 1000000000);
duration->set_nanos(value.count() % 1000000000);
}

void AddStringMap(const std::string& key,
const std::map<std::string, std::string>& string_map) {
void AddStringMap(const std::string &key,
const std::map<std::string, std::string> &string_map) {
if (string_map.size() == 0) {
return;
}
auto entries = (*attributes_->mutable_attributes())[key]
.mutable_string_map_value()
->mutable_entries();
entries->clear();
for (const auto& map_it : string_map) {
for (const auto &map_it : string_map) {
(*entries)[map_it.first] = map_it.second;
}
}

void AddProtoStructStringMap(const std::string& key,
const google::protobuf::Struct& struct_map) {
void AddProtoStructStringMap(const std::string &key,
const google::protobuf::Struct &struct_map) {
if (struct_map.fields().empty()) {
return;
}
auto entries = (*attributes_->mutable_attributes())[key]
.mutable_string_map_value()
->mutable_entries();
entries->clear();
for (const auto& field : struct_map.fields()) {
for (const auto &field : struct_map.fields()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add:
if entries is still empty after the loop, remove the key.

I like to have a option for filters not to dump data to mixer. Or another way, if filters want to dump data to mixer, they have to say so. We should not blindly dump everything to mixer. I have concern about performance, and security

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only in the report path. Dynamic metadata is strictly for logging.
See envoyproxy/envoy@700b7d0
In other words, even in Envoy, all dynamic metadata is already dumped on the wire, as is, just like static metadata. If filters store sensitive information, then there are bigger concerns including access logs, header values, etc. which are also being sent to mixer. So, lets have a separate discussion for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Shriram on the security front, I'm not concerned.

I do wonder about the end-to-end perf impact. Can we measure the cost on proxy and Mixer CPU usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

No matter what we do, there is no way to tell other external filters that they need to log things for Istio mixer. Second, dynamic metadata is not 1000s of kbs of data. Its very very small bits of info like name of resource, or some key claim type, etc. This is the only way the mixer has, if it wishes to be able to log info about other OSS codecs like mongo/redis/kafka in Envoy. Otherwise, we will still be stuck with opaque TCP attributes like bytes sent, received, etc., while people prefer to turn off mixer and use envoy gRPC access logging to get richer metrics. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@geeknoid It is hard to measure the performance without specify the metadata size. By design, dynamicMetadata is used for passing data in between filters. Authors may not realize its size has implication on the Mixer performance.

Sending over to mixer is different with logging. logging is for debugging, you may want to dump everything.
But sending to mixer is different, you don't want to dump everything

I strongly favor that if filters want their metadata to be send to mixer, they have to say so. It can be:

  1. add a special flag in the Struct, only this flag present, dump it
  2. or the name has to be certain format, prefix with something.

Copy link
Member Author

@rshriram rshriram Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

logging is for debugging

???
Access log is most certainly not debugging. In fact, this PR came from Google: envoyproxy/envoy@700b7d0 and this is adding dynamic metadata to access logs streamed over gRPC.
Remember, I can always choose to overload the mixer by sending very very large HTTP headers, auth keys, etc. or several 100s of headers. So, the argument about performance is moot. We can potentially agree on some common prefix but that requires discussion with envoy folks as well, things like mongo/redis that generate dynamic metadata need to be modified to use this agreed upon prefix. This will take time. Do I have to be wait until then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Access log is not for debugging, error log is. All dynamic data are now streamed over gRPC if you configure Envoy to use gRPC access logger, the security concern is there but not addressed yet.

Dumping all dynamic metadata to attributes is not that bad, it is already done today just as protobuf serialized string. As an enhancement, we may do allow-list or deny-list of the metadata key as a follow up if we want more control on that.

// Ignore all fields that are not string or string list.
switch (field.second.kind_case()) {
case google::protobuf::Value::kStringValue:
Expand All @@ -122,15 +122,37 @@ class AttributesBuilder {
break;
}
}

if (entries->empty()) {
attributes_->mutable_attributes()->erase(key);
}
}

// Serializes all the keys in a map<string, struct> and builds attributes.
// for example, foo.bar.com: struct {str:abc, list:[c,d,e]} will be emitted as
// foo.bar.com: string_map[str:abc, list: c,d,e]
// Only extracts strings and lists.
// TODO: add the ability to pack bools and nums as strings and recurse down
// the struct.
void FlattenMapOfStringToStruct(
const ::google::protobuf::Map<::std::string, ::google::protobuf::Struct>
&filter_state) {
if (filter_state.empty()) {
return;
}

for (const auto &filter : filter_state) {
AddProtoStructStringMap(filter.first, filter.second);
}
}

bool HasAttribute(const std::string& key) const {
const auto& attrs_map = attributes_->attributes();
bool HasAttribute(const std::string &key) const {
const auto &attrs_map = attributes_->attributes();
return attrs_map.find(key) != attrs_map.end();
}

private:
::istio::mixer::v1::Attributes* attributes_;
::istio::mixer::v1::Attributes *attributes_;
};

} // namespace utils
Expand Down
10 changes: 4 additions & 6 deletions src/envoy/http/mixer/report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "envoy/http/header_map.h"
#include "envoy/stream_info/stream_info.h"
#include "extensions/filters/http/well_known_names.h"
#include "google/protobuf/struct.pb.h"
#include "include/istio/control/http/controller.h"
#include "src/envoy/utils/utils.h"

Expand Down Expand Up @@ -158,12 +159,9 @@ class ReportData : public ::istio::control::http::ReportData,
}

// Get attributes generated by http filters
bool GetDynamicFilterState(std::string *filter_state) const override {
if (info_.dynamicMetadata().filter_metadata_size() > 0) {
info_.dynamicMetadata().SerializeToString(filter_state);
return true;
}
return false;
const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
&GetDynamicFilterState() const override {
return info_.dynamicMetadata().filter_metadata();
}
};

Expand Down
19 changes: 6 additions & 13 deletions src/envoy/tcp/mixer/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,19 +177,12 @@ bool Filter::GetDestinationUID(std::string *uid) const {
}
return false;
}
bool Filter::GetDynamicFilterState(std::string *filter_state) const {
if (filter_callbacks_->connection()
.streamInfo()
.dynamicMetadata()
.filter_metadata_size() > 0) {
filter_callbacks_->connection()
.streamInfo()
.dynamicMetadata()
.SerializeToString(filter_state);
return true;
}

return false;
const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
&Filter::GetDynamicFilterState() const {
return filter_callbacks_->connection()
.streamInfo()
.dynamicMetadata()
.filter_metadata();
}
void Filter::GetReportInfo(
::istio::control::tcp::ReportData::ReportInfo *data) const {
Expand Down
30 changes: 16 additions & 14 deletions src/envoy/tcp/mixer/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "common/common/logger.h"
#include "envoy/network/connection.h"
#include "envoy/network/filter.h"
#include "google/protobuf/struct.pb.h"
#include "src/envoy/tcp/mixer/control.h"

namespace Envoy {
Expand All @@ -30,17 +31,17 @@ class Filter : public Network::Filter,
public ::istio::control::tcp::ReportData,
public Logger::Loggable<Logger::Id::filter> {
public:
Filter(Control& control);
Filter(Control &control);
~Filter();

void initializeReadFilterCallbacks(
Network::ReadFilterCallbacks& callbacks) override;
Network::ReadFilterCallbacks &callbacks) override;

// Network::ReadFilter
Network::FilterStatus onData(Buffer::Instance& data, bool) override;
Network::FilterStatus onData(Buffer::Instance &data, bool) override;

// Network::WriteFilter
Network::FilterStatus onWrite(Buffer::Instance& data, bool) override;
Network::FilterStatus onWrite(Buffer::Instance &data, bool) override;
Network::FilterStatus onNewConnection() override;

// Network::ConnectionCallbacks
Expand All @@ -50,17 +51,18 @@ class Filter : public Network::Filter,
void onBelowWriteBufferLowWatermark() override {}

// CheckData virtual functions.
bool GetSourceIpPort(std::string* str_ip, int* port) const override;
bool GetPrincipal(bool peer, std::string* user) const override;
bool GetSourceIpPort(std::string *str_ip, int *port) const override;
bool GetPrincipal(bool peer, std::string *user) const override;
bool IsMutualTLS() const override;
bool GetRequestedServerName(std::string* name) const override;
bool GetRequestedServerName(std::string *name) const override;

// ReportData virtual functions.
bool GetDestinationIpPort(std::string* str_ip, int* port) const override;
bool GetDestinationUID(std::string* uid) const override;
bool GetDynamicFilterState(std::string* filter_state) const override;
bool GetDestinationIpPort(std::string *str_ip, int *port) const override;
bool GetDestinationUID(std::string *uid) const override;
const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
&GetDynamicFilterState() const override;
void GetReportInfo(
::istio::control::tcp::ReportData::ReportInfo* data) const override;
::istio::control::tcp::ReportData::ReportInfo *data) const override;
std::string GetConnectionId() const override;

private:
Expand All @@ -73,19 +75,19 @@ class Filter : public Network::Filter,
void callCheck();

// Called when Check is done.
void completeCheck(const ::google::protobuf::util::Status& status);
void completeCheck(const ::google::protobuf::util::Status &status);

// Cancel the pending Check call.
void cancelCheck();

// the cancel check
istio::mixerclient::CancelFunc cancel_check_;
// the control object.
Control& control_;
Control &control_;
// pre-request handler
std::unique_ptr<::istio::control::tcp::RequestHandler> handler_;
// filter callback
Network::ReadFilterCallbacks* filter_callbacks_{};
Network::ReadFilterCallbacks *filter_callbacks_{};
// state
State state_{State::NotStarted};
// calling_check
Expand Down
5 changes: 1 addition & 4 deletions src/istio/control/http/attributes_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,10 +253,7 @@ void AttributesBuilder::ExtractReportAttributes(ReportData *report_data) {
}
}

std::string filter_state;
if (report_data->GetDynamicFilterState(&filter_state)) {
builder.AddBytes(utils::AttributeName::kRequestDynamicState, filter_state);
}
builder.FlattenMapOfStringToStruct(report_data->GetDynamicFilterState());
}

} // namespace http
Expand Down
Loading