Skip to content

Commit

Permalink
review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@mandarjog
mandarjog committed Sep 7, 2018
1 parent c0b19f2 commit b83c060
Show file tree
Hide file tree
Showing 12 changed files with 135 additions and 170 deletions.
2 changes: 1 addition & 1 deletion include/istio/control/http/controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class Controller {
// If not set or is 0 default value, the cache size is 1000.
int service_config_cache_size{};

const ::istio::utils::LocalNode local_node;
const ::istio::utils::LocalNode& local_node;
};

// The factory function to create a new instance of the controller.
Expand Down
22 changes: 10 additions & 12 deletions include/istio/utils/local_attributes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,29 +24,27 @@ namespace istio {
namespace utils {

struct LocalAttributes {
LocalAttributes(const Attributes& inbound, const Attributes& outbound,
const Attributes& forward)
: inbound(inbound), outbound(outbound), forward(forward) {}

// local inbound attributes
const Attributes inbound;
Attributes inbound;

// local outbound attributes
const Attributes outbound;
Attributes outbound;

// local forward attributes
const Attributes forward;
Attributes forward;
};

// LocalNode are used to extract information from envoy Node.
// LocalNode is abstract information about the node from Mixer's perspective.
struct LocalNode {
std::string ns;
std::string ip;
// like kubernetes://podname.namespace
std::string uid;

// namespace
std::string ns;
};

std::unique_ptr<const LocalAttributes> CreateLocalAttributes(
const LocalNode& local);
void CreateLocalAttributes(const LocalNode& local,
LocalAttributes* local_attributes);

} // namespace utils
} // namespace istio
Expand Down
2 changes: 1 addition & 1 deletion src/envoy/http/mixer/control.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Control::Control(const Config& config, Upstream::ClusterManager& cm,
&serialized_forward_attributes_);

LocalNode local_node;
Utils::Extract(local_info.node(), &local_node);
Utils::ExtractNodeInfo(local_info.node(), &local_node);

::istio::control::http::Controller::Options options(config_.config_pb(),
local_node);
Expand Down
46 changes: 23 additions & 23 deletions src/envoy/utils/mixer_control.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,8 @@ using ::istio::utils::LocalNode;
namespace Envoy {
namespace Utils {

const char NodeKey::kName[] = "NODE_NAME";
const char NodeKey::kNamespace[] = "NODE_NAMESPACE";
const char NodeKey::kIp[] = "NODE_IP";
const char NodeKey::kRegistry[] = "NODE_REGISTRY";
const char kNodeUID[] = "NODE_UID";
const char kNodeNamespace[] = "NODE_NAMESPACE";

namespace {

Expand Down Expand Up @@ -115,12 +113,12 @@ Grpc::AsyncClientFactoryPtr GrpcClientFactoryForCluster(
bool ExtractInfoCompat(const std::string &nodeid, LocalNode *args) {
auto parts = StringUtil::splitToken(nodeid, "~");
if (parts.size() < 3) {
GOOGLE_LOG(ERROR) << "ExtractInfoCompat error len(nodeid.split(~))<3: "
<< nodeid;
GOOGLE_LOG(ERROR)
<< "ExtractInfoCompat node id did not have the correct format: "
<< "{proxy_type}~{ip}~{node_name}.{node_ns}~{node_domain} " << nodeid;
return false;
}

auto ip = std::string(parts[1].begin(), parts[1].end());
auto longname = std::string(parts[2].begin(), parts[2].end());
auto names = StringUtil::splitToken(longname, ".");
if (names.size() < 2) {
Expand All @@ -131,40 +129,42 @@ bool ExtractInfoCompat(const std::string &nodeid, LocalNode *args) {
}
auto ns = std::string(names[1].begin(), names[1].end());

args->ip = ip;
args->ns = ns;
args->uid = "kubernetes://" + longname;

return true;
}

// ExtractInfo depends on NODE_NAME, NODE_NAMESPACE, NODE_IP and optional
// NODE_REG If work cannot be done, returns false.
// ExtractInfo depends on NODE_UID, NODE_NAMESPACE
bool ExtractInfo(const envoy::api::v2::core::Node &node, LocalNode *args) {
const auto meta = node.metadata().fields();
std::string name;
if (!ReadMap(meta, NodeKey::kName, &name)) {
GOOGLE_LOG(ERROR) << "ExtractInfo metadata missing " << NodeKey::kName
<< " " << node.metadata().DebugString();

if (meta.empty()) {
GOOGLE_LOG(ERROR) << "ExtractInfo metadata empty: " << node.DebugString();
return false;
}
std::string ns;
ReadMap(meta, NodeKey::kNamespace, &ns);

std::string ip;
ReadMap(meta, NodeKey::kIp, &ip);
std::string uid;
if (!ReadProtoMap(meta, kNodeUID, &uid)) {
GOOGLE_LOG(ERROR) << "ExtractInfo metadata missing " << kNodeUID << " "
<< node.metadata().DebugString();
return false;
}

std::string reg("kubernetes");
ReadMap(meta, NodeKey::kRegistry, &reg);
std::string ns;
if (!ReadProtoMap(meta, kNodeNamespace, &ns)) {
GOOGLE_LOG(ERROR) << "ExtractInfo metadata missing " << kNodeNamespace
<< " " << node.metadata().DebugString();
return false;
}

args->ip = ip;
args->ns = ns;
args->uid = reg + "://" + name + "." + ns;
args->uid = uid;

return true;
}

bool Extract(const envoy::api::v2::core::Node &node, LocalNode *args) {
bool ExtractNodeInfo(const envoy::api::v2::core::Node &node, LocalNode *args) {
if (ExtractInfo(node, args)) {
return true;
}
Expand Down
18 changes: 4 additions & 14 deletions src/envoy/utils/mixer_control.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,21 @@ Grpc::AsyncClientFactoryPtr GrpcClientFactoryForCluster(
const std::string &cluster_name, Upstream::ClusterManager &cm,
Stats::Scope &scope);

std::unique_ptr<const LocalAttributes> CreateLocalAttributes(
const LocalNode &local);
bool ExtractNodeInfo(const envoy::api::v2::core::Node &node, LocalNode *args);

bool Extract(const envoy::api::v2::core::Node &node, LocalNode *args);

inline bool ReadMap(
inline bool ReadProtoMap(
const google::protobuf::Map<std::string, google::protobuf::Value> &meta,
const std::string &key, std::string *val) {
const auto it = meta.find(key);
if (it != meta.end()) {
*val = it->second.string_value();
return true;
}

return false;
}

inline bool ReadMap(
inline bool ReadAttributeMap(
const google::protobuf::Map<std::string, Attributes_AttributeValue> &meta,
const std::string &key, std::string *val) {
const auto it = meta.find(key);
Expand All @@ -75,13 +73,5 @@ inline bool ReadMap(
return false;
}

// NodeKey are node matadata keys that are expected to be set.
struct NodeKey {
static const char kName[];
static const char kNamespace[];
static const char kIp[];
static const char kRegistry[];
};

} // namespace Utils
} // namespace Envoy
133 changes: 68 additions & 65 deletions src/envoy/utils/mixer_control_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,110 +14,113 @@
*/

#include "src/envoy/utils/mixer_control.h"
#include "fmt/printf.h"
#include "mixer/v1/config/client/client_config.pb.h"
#include "src/envoy/utils/utils.h"
#include "test/test_common/utility.h"

using Envoy::Utils::Extract;
using Envoy::Utils::NodeKey;
using Envoy::Utils::ExtractNodeInfo;
using Envoy::Utils::ParseJsonMessage;
using Envoy::Utils::ReadMap;
using Envoy::Utils::ReadAttributeMap;
using ::istio::utils::AttributeName;
using ::istio::utils::CreateLocalAttributes;
using ::istio::utils::LocalAttributes;
using ::istio::utils::LocalNode;

#define assertEqual(laExpect, la) \
#define assertEqualLocalAttributes(laExpect, la) \
{ \
EXPECT_EQ((laExpect)->outbound.DebugString(), \
(la)->outbound.DebugString()); \
EXPECT_EQ((laExpect)->inbound.DebugString(), (la)->inbound.DebugString()); \
EXPECT_EQ((laExpect)->forward.DebugString(), (la)->forward.DebugString()); \
};

#define ASSERT_LOCAL_NODE(lexp, la) \
{ \
EXPECT_EQ((lexp).uid, (la).uid); \
EXPECT_EQ((lexp).ns, (la).ns); \
};

namespace {

std::unique_ptr<const LocalAttributes> GenerateLocalAttributes(
envoy::api::v2::core::Node& node) {
LocalNode largs;
if (!Extract(node, &largs)) {
return nullptr;
}
return CreateLocalAttributes(largs);
}
const std::string kUID =
"kubernetes://fortioclient-84469dc8d7-jbbxt.service-graph";
const std::string kNS = "service-graph";
const std::string kNodeID =
"sidecar~10.36.0.15~fortioclient-84469dc8d7-jbbxt.service-graph~service-"
"graph.svc.cluster.local";

TEST(MixerControlTest, WithMetadata) {
std::string config_str = R"({
"id": "NEWID",
"cluster": "fortioclient",
"metadata": {
std::string genNodeConfig(std::string uid, std::string nodeid, std::string ns) {
auto md = R"(
"metadata": {
"ISTIO_VERSION": "1.0.1",
"NODE_NAME": "fortioclient-84469dc8d7-jbbxt",
"NODE_IP": "10.36.0.15",
"NODE_NAMESPACE": "service-graph",
"NODE_UID": "%s",
"NODE_NAMESPACE": "%s",
},
"build_version": "0/1.8.0-dev//RELEASE"
})";
envoy::api::v2::core::Node node;

auto status = ParseJsonMessage(config_str, &node);
EXPECT_OK(status) << status;
std::string val;
)";
std::string meta = "";
if (!ns.empty()) {
meta = fmt::sprintf(md, nodeid, ns);
}

LocalNode largs;
largs.ip = "10.36.0.15";
largs.uid = "kubernetes://fortioclient-84469dc8d7-jbbxt.service-graph";
largs.ns = "service-graph";
return fmt::sprintf(R"({
"id": "%s",
"cluster": "fortioclient",
%s
"build_version": "0/1.8.0-dev//RELEASE"
})",
uid, meta);
}

auto la = GenerateLocalAttributes(node);
EXPECT_NE(la, nullptr);
void initTestLocalNode(LocalNode *lexp) {
lexp->uid = kUID;
lexp->ns = kNS;
}

const auto att = la->outbound.attributes();
TEST(MixerControlTest, CreateLocalAttributes) {
LocalNode lexp;
initTestLocalNode(&lexp);

EXPECT_EQ(true, ReadMap(att, AttributeName::kSourceUID, &val));
EXPECT_EQ(val, largs.uid);
LocalAttributes la;
CreateLocalAttributes(lexp, &la);

EXPECT_EQ(true, ReadMap(att, AttributeName::kSourceNamespace, &val));
EXPECT_EQ(val, largs.ns);
const auto att = la.outbound.attributes();
std::string val;

auto laExpect = CreateLocalAttributes(largs);
EXPECT_TRUE(ReadAttributeMap(att, AttributeName::kSourceUID, &val));
EXPECT_TRUE(val == lexp.uid);

assertEqual(laExpect, la);
EXPECT_TRUE(ReadAttributeMap(att, AttributeName::kSourceNamespace, &val));
EXPECT_TRUE(val == lexp.ns);
}

TEST(MixerControlTest, NoMetadata) {
std::string config_str = R"({
"id": "sidecar~10.36.0.15~fortioclient-84469dc8d7-jbbxt.service-graph~service-graph.svc.cluster.local",
"cluster": "fortioclient",
"metadata": {
"ISTIO_VERSION": "1.0.1",
},
"build_version": "0/1.8.0-dev//RELEASE"
})";
envoy::api::v2::core::Node node;
TEST(MixerControlTest, WithMetadata) {
LocalNode lexp;
initTestLocalNode(&lexp);

auto status = ParseJsonMessage(config_str, &node);
envoy::api::v2::core::Node node;
auto status =
ParseJsonMessage(genNodeConfig("new_id", lexp.uid, lexp.ns), &node);
EXPECT_OK(status) << status;

LocalNode largs;
largs.ip = "10.36.0.15";
largs.uid = "kubernetes://fortioclient-84469dc8d7-jbbxt.service-graph";
largs.ns = "service-graph";
EXPECT_TRUE(ExtractNodeInfo(node, &largs));

auto la = GenerateLocalAttributes(node);
EXPECT_NE(la, nullptr);

const auto att = la->outbound.attributes();
std::string val;
ASSERT_LOCAL_NODE(lexp, largs);
}

EXPECT_EQ(true, ReadMap(att, AttributeName::kSourceUID, &val));
EXPECT_EQ(val, largs.uid);
TEST(MixerControlTest, NoMetadata) {
LocalNode lexp;
initTestLocalNode(&lexp);

EXPECT_EQ(true, ReadMap(att, AttributeName::kSourceNamespace, &val));
EXPECT_EQ(val, largs.ns);
envoy::api::v2::core::Node node;
auto status = ParseJsonMessage(genNodeConfig(kNodeID, "", ""), &node);
EXPECT_OK(status) << status;

auto laExpect = CreateLocalAttributes(largs);
LocalNode largs;
EXPECT_TRUE(ExtractNodeInfo(node, &largs));

assertEqual(laExpect, la);
ASSERT_LOCAL_NODE(lexp, largs);
}

} // namespace
Loading

0 comments on commit b83c060

Please sign in to comment.