-
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 all 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 |
---|---|---|
|
@@ -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": "forward_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 |
---|---|---|
@@ -0,0 +1,114 @@ | ||
/* Copyright 2017 Istio Authors. All Rights Reserved. | ||
* | ||
* 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 ForwardAttribute { | ||
namespace { | ||
|
||
// The Json object name to specify attributes which will be forwarded | ||
// to the upstream istio proxy. | ||
const std::string kJsonNameAttributes("attributes"); | ||
|
||
} // namespace | ||
|
||
class Config : public Logger::Loggable<Logger::Id::http> { | ||
private: | ||
std::string attributes_; | ||
|
||
public: | ||
Config(const Json::Object& config) { | ||
Utils::StringMap 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 ForwardAttributeFilter : public Http::StreamDecoderFilter { | ||
private: | ||
ConfigPtr config_; | ||
|
||
public: | ||
ForwardAttributeFilter(ConfigPtr config) : config_(config) {} | ||
|
||
FilterHeadersStatus decodeHeaders(HeaderMap& headers, | ||
bool end_stream) override { | ||
if (!config_->attributes().empty()) { | ||
headers.addStatic(Utils::kIstioAttributeHeader, 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 ForwardAttribute | ||
} // namespace Http | ||
|
||
namespace Server { | ||
namespace Configuration { | ||
|
||
class ForwardAttributeConfig : 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 != "forward_attribute") { | ||
return nullptr; | ||
} | ||
|
||
Http::ForwardAttribute::ConfigPtr add_header_config( | ||
new Http::ForwardAttribute::Config(config)); | ||
return [add_header_config]( | ||
Http::FilterChainFactoryCallbacks& callbacks) -> void { | ||
std::shared_ptr<Http::ForwardAttribute::ForwardAttributeFilter> instance( | ||
new Http::ForwardAttribute::ForwardAttributeFilter( | ||
add_header_config)); | ||
callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterPtr(instance)); | ||
}; | ||
} | ||
}; | ||
|
||
static RegisterHttpFilterConfigFactory<ForwardAttributeConfig> register_; | ||
|
||
} // namespace Configuration | ||
} // namespace server |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
/* | ||
/* Copyright 2017 Istio Authors. All Rights Reserved. | ||
* | ||
* 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 | ||
|
@@ -13,9 +14,14 @@ | |
*/ | ||
|
||
#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" | ||
|
||
using ::google::protobuf::util::Status; | ||
using ::istio::mixer_client::Attributes; | ||
using ::istio::mixer_client::DoneFunc; | ||
|
@@ -77,7 +83,8 @@ std::string GetLastForwardedHost(const HeaderMap& header_map) { | |
if (entry == nullptr) { | ||
return ""; | ||
} | ||
auto xff_list = StringUtil::split(entry->value().c_str(), ','); | ||
std::vector<std::string> xff_list = | ||
StringUtil::split(entry->value().c_str(), ','); | ||
if (xff_list.empty()) { | ||
return ""; | ||
} | ||
|
@@ -89,7 +96,7 @@ void FillRequestHeaderAttributes(const HeaderMap& header_map, | |
// Pass in all headers | ||
header_map.iterate( | ||
[](const HeaderEntry& header, void* context) { | ||
auto attr = static_cast<Attributes*>(context); | ||
Attributes* attr = static_cast<Attributes*>(context); | ||
attr->attributes[kRequestHeaderPrefix + header.key().c_str()] = | ||
StringValue(header.value().c_str()); | ||
}, | ||
|
@@ -106,7 +113,7 @@ void FillResponseHeaderAttributes(const HeaderMap& header_map, | |
Attributes* attr) { | ||
header_map.iterate( | ||
[](const HeaderEntry& header, void* context) { | ||
auto attr = static_cast<Attributes*>(context); | ||
Attributes* attr = static_cast<Attributes*>(context); | ||
attr->attributes[kResponseHeaderPrefix + header.key().c_str()] = | ||
StringValue(header.value().c_str()); | ||
}, | ||
|
@@ -144,8 +151,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::kIstioAttributeHeader); | ||
if (entry) { | ||
::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::kIstioAttributeHeader); | ||
} | ||
|
||
FillRequestHeaderAttributes(header_map, attr); | ||
|
||
for (const auto& attribute : config_attributes_) { | ||
|
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.
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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave it as ${MIXER_SERFVER}