-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Add send_attribute filter. #115
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,3 +49,51 @@ This Proxy will use Envoy and talk to Mixer server. | |
curl http://localhost:9090/echo -d "hello world" | ||
``` | ||
|
||
## How to configurate HTTP filters | ||
|
||
This module has two HTTP filters: | ||
1. mixer filter: intercept all HTTP requests, call the mixer. | ||
2. send_attribute filter: Send some attributes to next hop istio/proxy with mixer filter. | ||
|
||
### *mixer* filter: | ||
|
||
This filter will intercept all HTTP requests and call Mixer. Here is its config: | ||
|
||
``` | ||
"filters": [ | ||
"type": "both", | ||
"name": "mixer", | ||
"config": { | ||
"mixer_server": "${MIXER_SERVER}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the MIXER SERVER an environment variable? Or is it just for illustration? If it's the latter, I suggest using Envoy type notation "..." or the type of the attribute "" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just a template value in a standalone prototype proxy + mixer. The reason for this PR was that I wanted to have explicit values in the envoy config rather than some secret environment variables that the filter picks up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it gets overriden during config gen? Then it sounds fine. In that case I would say use some non colliding variables like Mixer__server There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will leave it as ${MIXER_SERFVER} |
||
"attributes" : { | ||
"attribute_name1": "attribute_value1", | ||
"attribute_name2": "attribute_value2" | ||
} | ||
} | ||
``` | ||
|
||
Notes: | ||
* mixer_server is required | ||
* attributes: these attributes will be send to mixer | ||
|
||
### *send_attribute* HTTP filter: | ||
|
||
This filer will send attributes to the next hop istio/proxy with mixer_filter. | ||
|
||
``` | ||
"filters": [ | ||
"type": "decoder", | ||
"name": "send_attribute", | ||
"config": { | ||
"attributes": { | ||
"attribute_name1": "attribute_value1", | ||
"attribute_name2": "attribute_value2" | ||
} | ||
} | ||
``` | ||
|
||
Notes: | ||
* attributes: these attributes will be send to the next hop istio/proxy with mixer filter. | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,62 @@ | |
"type": "both", | ||
"name": "mixer", | ||
"config": { | ||
"mixer_server": "${MIXER_SERVER}" | ||
"mixer_server": "${MIXER_SERVER}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is environment variable, it should absolutely be killed! ESP in a JSON file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. src/envoy/mixer/start_env script or config-gen script will replace these variables with real values. |
||
"attributes": { | ||
"target.uid": "POD222", | ||
"target.namespace": "XYZ222" | ||
} | ||
} | ||
}, | ||
{ | ||
"type": "decoder", | ||
"name": "router", | ||
"config": {} | ||
} | ||
] | ||
} | ||
} | ||
] | ||
}, | ||
{ | ||
"port": 7070, | ||
"bind_to_port": true, | ||
"filters": [ | ||
{ | ||
"type": "read", | ||
"name": "http_connection_manager", | ||
"config": { | ||
"codec_type": "auto", | ||
"stat_prefix": "ingress_http", | ||
"route_config": { | ||
"virtual_hosts": [ | ||
{ | ||
"name": "backend", | ||
"domains": ["*"], | ||
"routes": [ | ||
{ | ||
"timeout_ms": 0, | ||
"prefix": "/", | ||
"cluster": "service2" | ||
} | ||
] | ||
} | ||
] | ||
}, | ||
"access_log": [ | ||
{ | ||
"path": "/dev/stdout" | ||
} | ||
], | ||
"filters": [ | ||
{ | ||
"type": "decoder", | ||
"name": "send_attribute", | ||
"config": { | ||
"attributes": { | ||
"source.uid": "POD11", | ||
"source.namespace": "XYZ11" | ||
} | ||
} | ||
}, | ||
{ | ||
|
@@ -65,6 +120,17 @@ | |
"url": "tcp://${BACKEND}" | ||
} | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same to the backend bar above. |
||
}, | ||
{ | ||
"name": "service2", | ||
"connect_timeout_ms": 5000, | ||
"type": "strict_dns", | ||
"lb_type": "round_robin", | ||
"hosts": [ | ||
{ | ||
"url": "tcp://localhost:9090" | ||
} | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finally, I suggest using the JSON schema approach that Envoy uses. It's relatively easy to extend and cuts down a ton of unnecessary validation code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the actual config in JSON. not a JSON schema template. the reason it is called "template" since it has ${MIXER_SERVER} variables, config gen will replace them with real mixer_server address. |
||
} | ||
] | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,12 @@ | |
*/ | ||
|
||
#include "src/envoy/mixer/http_control.h" | ||
|
||
#include "common/common/base64.h" | ||
#include "common/common/utility.h" | ||
#include "common/http/utility.h" | ||
#include "src/envoy/mixer/string_map.pb.h" | ||
#include "src/envoy/mixer/utils.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. Please sort includes and group them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
using ::google::protobuf::util::Status; | ||
using ::istio::mixer_client::Attributes; | ||
|
@@ -144,8 +148,19 @@ HttpControl::HttpControl(const std::string& mixer_server, | |
mixer_client_ = ::istio::mixer_client::CreateMixerClient(options); | ||
} | ||
|
||
void HttpControl::FillCheckAttributes(const HeaderMap& header_map, | ||
Attributes* attr) { | ||
void HttpControl::FillCheckAttributes(HeaderMap& header_map, Attributes* attr) { | ||
// Extract attributes from x-istio-attributes header | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does Fill stand for here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. stand for "add". similar to fill protobuf. to add fields in a protobuf. |
||
const HeaderEntry* entry = header_map.get(Utils::kHeaderNameIstioAttributes); | ||
if (entry) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest renaming the kHeaderName... to IstioAttributesHeader There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
::istio::proxy::mixer::StringMap pb; | ||
std::string str(entry->value().c_str(), entry->value().size()); | ||
pb.ParseFromString(Base64::decode(str)); | ||
for (const auto& it : pb.map()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to eliminate the auto here? There are very few places in Envoy code where auto is used. It kills readability most of the time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have removed some auto. But in this line 158, the type is too long for map iterator. it is better to use auto for readability. |
||
SetStringAttribute(it.first, it.second, attr); | ||
} | ||
header_map.remove(Utils::kHeaderNameIstioAttributes); | ||
} | ||
|
||
FillRequestHeaderAttributes(header_map, attr); | ||
|
||
for (const auto& attribute : config_attributes_) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
/* Copyright 2016 Google Inc. All Rights Reserved. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change copy right to 2017 Istio Authors. That's what we are doing in manager. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#include "precompiled/precompiled.h" | ||
|
||
#include "common/common/base64.h" | ||
#include "common/common/logger.h" | ||
#include "common/http/headers.h" | ||
#include "common/http/utility.h" | ||
#include "envoy/server/instance.h" | ||
#include "server/config/network/http_connection_manager.h" | ||
#include "src/envoy/mixer/utils.h" | ||
|
||
namespace Http { | ||
namespace AddHeader { | ||
namespace { | ||
|
||
// The Json object name to specify attributes to pass to | ||
// next hop istio proxy. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reword. Use upstream instead of next hop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
const std::string kJsonNameAttributes("attributes"); | ||
|
||
} // namespace | ||
|
||
class Config : public Logger::Loggable<Logger::Id::http> { | ||
private: | ||
Upstream::ClusterManager& cm_; | ||
std::string attributes_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need the cluster manager here? It's not even being used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed |
||
|
||
public: | ||
Config(const Json::Object& config, Server::Instance& server) | ||
: cm_(server.clusterManager()) { | ||
const auto& attributes = | ||
Utils::ExtractStringMap(config, kJsonNameAttributes); | ||
if (!attributes.empty()) { | ||
std::string serialized_str = Utils::SerializeStringMap(attributes); | ||
attributes_ = | ||
Base64::encode(serialized_str.c_str(), serialized_str.size()); | ||
} | ||
} | ||
|
||
const std::string& attributes() const { return attributes_; } | ||
}; | ||
|
||
typedef std::shared_ptr<Config> ConfigPtr; | ||
|
||
class Instance : public Http::StreamDecoderFilter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename class to ForwardAttributeFilter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
private: | ||
ConfigPtr config_; | ||
|
||
public: | ||
Instance(ConfigPtr config) : config_(config) {} | ||
|
||
FilterHeadersStatus decodeHeaders(HeaderMap& headers, | ||
bool end_stream) override { | ||
if (!config_->attributes().empty()) { | ||
headers.addStatic(Utils::kHeaderNameIstioAttributes, | ||
config_->attributes()); | ||
} | ||
return FilterHeadersStatus::Continue; | ||
} | ||
|
||
FilterDataStatus decodeData(Buffer::Instance& data, | ||
bool end_stream) override { | ||
return FilterDataStatus::Continue; | ||
} | ||
|
||
FilterTrailersStatus decodeTrailers(HeaderMap& trailers) override { | ||
return FilterTrailersStatus::Continue; | ||
} | ||
|
||
void setDecoderFilterCallbacks( | ||
StreamDecoderFilterCallbacks& callbacks) override {} | ||
}; | ||
|
||
} // namespace AddHeader | ||
} // namespace Http | ||
|
||
namespace Server { | ||
namespace Configuration { | ||
|
||
class AddHeaderConfig : public HttpFilterConfigFactory { | ||
public: | ||
HttpFilterFactoryCb tryCreateFilterFactory( | ||
HttpFilterType type, const std::string& name, const Json::Object& config, | ||
const std::string&, Server::Instance& server) override { | ||
if (type != HttpFilterType::Decoder || name != "send_attribute") { | ||
return nullptr; | ||
} | ||
|
||
Http::AddHeader::ConfigPtr add_header_config( | ||
new Http::AddHeader::Config(config, server)); | ||
return [add_header_config]( | ||
Http::FilterChainFactoryCallbacks& callbacks) -> void { | ||
std::shared_ptr<Http::AddHeader::Instance> instance( | ||
new Http::AddHeader::Instance(add_header_config)); | ||
callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterPtr(instance)); | ||
}; | ||
} | ||
}; | ||
|
||
static RegisterHttpFilterConfigFactory<AddHeaderConfig> register_; | ||
|
||
} // namespace Configuration | ||
} // namespace server |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Copyright 2016 Google Inc. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change copyright header There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
syntax = "proto3"; | ||
|
||
package istio.proxy.mixer; | ||
|
||
// A map of string to string. | ||
message StringMap { | ||
map<string, string> map = 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest renaming this to forward_attributes filter. It would be nice if the mixer filter was called eval_attributes but it's probably too late for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also not the next hop istio proxy (while it may be true, it makes the mesh sound like a chain of routers all the time). Suggest rewording to "atttach attributes to upstream requests".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Reworded.