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.

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

Merged
merged 4 commits into from
Feb 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions src/envoy/mixer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,28 @@

load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
load("@bazel_tools//tools/build_defs/docker:docker.bzl", "docker_build")
load("@protobuf_git//:protobuf.bzl", "cc_proto_library")

cc_proto_library(
name = "string_map_proto",
srcs = ["string_map.proto"],
default_runtime = "//external:protobuf",
protoc = "//external:protoc",
visibility = ["//visibility:public"],
)

cc_library(
name = "filter_lib",
srcs = [
"forward_attribute_filter.cc",
"http_control.cc",
"http_control.h",
"http_filter.cc",
"utils.cc",
"utils.h",
],
deps = [
":string_map_proto",
"//external:mixer_client_lib",
"@envoy_git//:envoy-common",
],
Expand Down
48 changes: 48 additions & 0 deletions src/envoy/mixer/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. forward_attribute filter: Forward attributes to the upstream istio/proxy.

### *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}",
Copy link
Member

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 ""

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor Author

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}

"attributes" : {
"attribute_name1": "attribute_value1",
"attribute_name2": "attribute_value2"
}
}
```

Notes:
* mixer_server is required
* attributes: these attributes will be send to the mixer

### *forward_attribute* HTTP filter:

This filer will forward attributes to the upstream istio/proxy.

```
"filters": [
"type": "decoder",
"name": "forward_attribute",
"config": {
"attributes": {
"attribute_name1": "attribute_value1",
"attribute_name2": "attribute_value2"
}
}
```

Notes:
* attributes: these attributes will be forwarded to the upstream istio/proxy.



68 changes: 67 additions & 1 deletion src/envoy/mixer/envoy.conf.template
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,62 @@
"type": "both",
"name": "mixer",
"config": {
"mixer_server": "${MIXER_SERVER}"
"mixer_server": "${MIXER_SERVER}",
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"
}
}
},
{
Expand Down Expand Up @@ -65,6 +120,17 @@
"url": "tcp://${BACKEND}"
}
]
Copy link
Member

Choose a reason for hiding this comment

The 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"
}
]
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

}
]
}
Expand Down
114 changes: 114 additions & 0 deletions src/envoy/mixer/forward_attribute_filter.cc
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
30 changes: 24 additions & 6 deletions src/envoy/mixer/http_control.cc
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
Expand All @@ -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;
Expand Down Expand Up @@ -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 "";
}
Expand All @@ -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());
},
Expand All @@ -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());
},
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

What does Fill stand for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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_) {
Expand Down
4 changes: 2 additions & 2 deletions src/envoy/mixer/http_control.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright 2016 Google Inc. All Rights Reserved.
/* 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.
Expand Down Expand Up @@ -51,7 +51,7 @@ class HttpControl final : public Logger::Loggable<Logger::Id::http> {
::istio::mixer_client::DoneFunc on_done);

private:
void FillCheckAttributes(const HeaderMap& header_map,
void FillCheckAttributes(HeaderMap& header_map,
::istio::mixer_client::Attributes* attr);

// The mixer client
Expand Down
Loading