diff --git a/include/istio/control/http/controller.h b/include/istio/control/http/controller.h index ad86398b8f7..0ab4ae0febb 100644 --- a/include/istio/control/http/controller.h +++ b/include/istio/control/http/controller.h @@ -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. diff --git a/include/istio/utils/local_attributes.h b/include/istio/utils/local_attributes.h index ae750f6a840..b7541bbda71 100644 --- a/include/istio/utils/local_attributes.h +++ b/include/istio/utils/local_attributes.h @@ -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 CreateLocalAttributes( - const LocalNode& local); +void CreateLocalAttributes(const LocalNode& local, + LocalAttributes* local_attributes); } // namespace utils } // namespace istio diff --git a/src/envoy/http/mixer/control.cc b/src/envoy/http/mixer/control.cc index e48c7cbdfe4..3a9dfbd5288 100644 --- a/src/envoy/http/mixer/control.cc +++ b/src/envoy/http/mixer/control.cc @@ -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); diff --git a/src/envoy/utils/mixer_control.cc b/src/envoy/utils/mixer_control.cc index adbf10c7fd5..de74f4b9692 100644 --- a/src/envoy/utils/mixer_control.cc +++ b/src/envoy/utils/mixer_control.cc @@ -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 { @@ -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) { @@ -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, ®); + 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; } diff --git a/src/envoy/utils/mixer_control.h b/src/envoy/utils/mixer_control.h index adf287295ad..e2f1e75a9aa 100644 --- a/src/envoy/utils/mixer_control.h +++ b/src/envoy/utils/mixer_control.h @@ -48,12 +48,9 @@ Grpc::AsyncClientFactoryPtr GrpcClientFactoryForCluster( const std::string &cluster_name, Upstream::ClusterManager &cm, Stats::Scope &scope); -std::unique_ptr 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 &meta, const std::string &key, std::string *val) { const auto it = meta.find(key); @@ -61,10 +58,11 @@ inline bool ReadMap( *val = it->second.string_value(); return true; } + return false; } -inline bool ReadMap( +inline bool ReadAttributeMap( const google::protobuf::Map &meta, const std::string &key, std::string *val) { const auto it = meta.find(key); @@ -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 diff --git a/src/envoy/utils/mixer_control_test.cc b/src/envoy/utils/mixer_control_test.cc index 789f0bac2d9..b42435d0302 100644 --- a/src/envoy/utils/mixer_control_test.cc +++ b/src/envoy/utils/mixer_control_test.cc @@ -14,20 +14,20 @@ */ #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()); \ @@ -35,89 +35,92 @@ using ::istio::utils::LocalNode; 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 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 diff --git a/src/istio/control/http/client_context.cc b/src/istio/control/http/client_context.cc index 60c4971a9ab..26bbfc4476b 100644 --- a/src/istio/control/http/client_context.cc +++ b/src/istio/control/http/client_context.cc @@ -51,19 +51,18 @@ ClientContext::ClientContext(const Controller::Options& data) config_(data.config), service_config_cache_size_(data.service_config_cache_size), outbound_(isOutbound(data.config)) { - local_attributes_ = CreateLocalAttributes(data.local_node); + CreateLocalAttributes(data.local_node, &local_attributes_); } ClientContext::ClientContext( std::unique_ptr<::istio::mixerclient::MixerClient> mixer_client, const ::istio::mixer::v1::config::client::HttpClientConfig& config, int service_config_cache_size, - std::unique_ptr local_attributes, - bool outbound) + ::istio::utils::LocalAttributes& local_attributes, bool outbound) : ClientContextBase(std::move(mixer_client)), config_(config), service_config_cache_size_(service_config_cache_size), - local_attributes_(std::move(local_attributes)), + local_attributes_(local_attributes), outbound_(outbound) {} const std::string& ClientContext::GetServiceName( @@ -92,25 +91,17 @@ const ServiceConfig* ClientContext::GetServiceConfig( void ClientContext::AddRequestAttributes( ::istio::mixer::v1::Attributes* request) const { - if (local_attributes_ == nullptr) { - return; - } - if (outbound_) { - request->MergeFrom(local_attributes_->outbound); + request->MergeFrom(local_attributes_.outbound); } else { - request->MergeFrom(local_attributes_->inbound); + request->MergeFrom(local_attributes_.inbound); } } void ClientContext::AddForwardAttributes( ::istio::mixer::v1::Attributes* request) const { - if (local_attributes_ == nullptr) { - return; - } - if (outbound_) { - request->MergeFrom(local_attributes_->forward); + request->MergeFrom(local_attributes_.forward); } } diff --git a/src/istio/control/http/client_context.h b/src/istio/control/http/client_context.h index dedfaabe15c..2d9beadb18a 100644 --- a/src/istio/control/http/client_context.h +++ b/src/istio/control/http/client_context.h @@ -36,8 +36,7 @@ class ClientContext : public ClientContextBase { std::unique_ptr<::istio::mixerclient::MixerClient> mixer_client, const ::istio::mixer::v1::config::client::HttpClientConfig& config, int service_config_cache_size, - std::unique_ptr local_attributes, - bool outbound); + ::istio::utils::LocalAttributes& local_attributes, bool outbound); // Retrieve mixer client config. const ::istio::mixer::v1::config::client::HttpClientConfig& config() const { @@ -71,7 +70,7 @@ class ClientContext : public ClientContextBase { int service_config_cache_size_; // local attributes - owned by the client context. - std::unique_ptr local_attributes_; + ::istio::utils::LocalAttributes local_attributes_; // if this client context is for an inbound listener or outbound listener. bool outbound_; diff --git a/src/istio/control/http/request_handler_impl_test.cc b/src/istio/control/http/request_handler_impl_test.cc index af21bfc866c..45ad854981b 100644 --- a/src/istio/control/http/request_handler_impl_test.cc +++ b/src/istio/control/http/request_handler_impl_test.cc @@ -140,23 +140,20 @@ class RequestHandlerImplTest : public ::testing::Test { const std::string& local_forward_attributes) { ASSERT_TRUE(TextFormat::ParseFromString(config_text, &client_config_)); - Attributes inbound; + LocalAttributes la; ASSERT_TRUE( - TextFormat::ParseFromString(local_inbound_attributes, &inbound)); - Attributes outbound; + TextFormat::ParseFromString(local_inbound_attributes, &la.inbound)); ASSERT_TRUE( - TextFormat::ParseFromString(local_outbound_attributes, &outbound)); - Attributes forward; + TextFormat::ParseFromString(local_outbound_attributes, &la.outbound)); ASSERT_TRUE( - TextFormat::ParseFromString(local_forward_attributes, &forward)); + TextFormat::ParseFromString(local_forward_attributes, &la.forward)); mock_client_ = new ::testing::NiceMock; // set LRU cache size is 3 - auto la = new LocalAttributes(inbound, outbound, forward); client_context_ = std::make_shared( - std::unique_ptr(mock_client_), client_config_, 3, - std::unique_ptr(la), false); + std::unique_ptr(mock_client_), client_config_, 3, la, + false); controller_ = std::unique_ptr(new ControllerImpl(client_context_)); } diff --git a/src/istio/control/http/service_context.cc b/src/istio/control/http/service_context.cc index f2882d3c4f2..2471ecad8ea 100644 --- a/src/istio/control/http/service_context.cc +++ b/src/istio/control/http/service_context.cc @@ -52,14 +52,14 @@ void ServiceContext::BuildParsers() { // Add static mixer attributes. void ServiceContext::AddStaticAttributes(RequestContext *request) const { + client_context_->AddRequestAttributes(&request->attributes); + if (client_context_->config().has_mixer_attributes()) { request->attributes.MergeFrom(client_context_->config().mixer_attributes()); } if (service_config_ && service_config_->has_mixer_attributes()) { request->attributes.MergeFrom(service_config_->mixer_attributes()); } - - client_context_->AddRequestAttributes(&request->attributes); } // Inject a header that contains the static forwarded attributes. @@ -67,6 +67,8 @@ void ServiceContext::InjectForwardedAttributes( HeaderUpdate *header_update) const { Attributes attributes; + client_context_->AddForwardAttributes(&attributes); + if (client_context_->config().has_forward_attributes()) { attributes.MergeFrom(client_context_->config().forward_attributes()); } @@ -74,8 +76,6 @@ void ServiceContext::InjectForwardedAttributes( attributes.MergeFrom(service_config_->forward_attributes()); } - client_context_->AddForwardAttributes(&attributes); - if (!attributes.attributes().empty()) { AttributesBuilder::ForwardAttributes(attributes, header_update); } diff --git a/src/istio/utils/local_attributes.cc b/src/istio/utils/local_attributes.cc index 9719e7997e4..b02cbe7d5ec 100644 --- a/src/istio/utils/local_attributes.cc +++ b/src/istio/utils/local_attributes.cc @@ -22,29 +22,21 @@ namespace utils { // create Local attributes object and return a pointer to it. // Should be freed by the caller. -std::unique_ptr CreateLocalAttributes( - const LocalNode &local) { +void CreateLocalAttributes(const LocalNode& local, + LocalAttributes* local_attributes) { ::istio::mixer::v1::Attributes inbound; - AttributesBuilder ib(&inbound); + AttributesBuilder ib(&local_attributes->inbound); ib.AddString(AttributeName::kDestinationUID, local.uid); ib.AddString(AttributeName::kContextReporterUID, local.uid); ib.AddString(AttributeName::kDestinationNamespace, local.ns); - if (!local.ip.empty()) { - // TODO: mjog check if destination.ip should be setup for inbound. - } - - ::istio::mixer::v1::Attributes outbound; - AttributesBuilder ob(&outbound); + AttributesBuilder ob(&local_attributes->outbound); ob.AddString(AttributeName::kSourceUID, local.uid); ob.AddString(AttributeName::kContextReporterUID, local.uid); ob.AddString(AttributeName::kSourceNamespace, local.ns); - ::istio::mixer::v1::Attributes forward; - AttributesBuilder(&forward).AddString(AttributeName::kSourceUID, local.uid); - - return std::unique_ptr( - new LocalAttributes(inbound, outbound, forward)); + AttributesBuilder(&local_attributes->forward) + .AddString(AttributeName::kSourceUID, local.uid); } } // namespace utils diff --git a/test/integration/istio_http_integration_test.cc b/test/integration/istio_http_integration_test.cc index e44c79c8df0..22a9e729c0d 100644 --- a/test/integration/istio_http_integration_test.cc +++ b/test/integration/istio_http_integration_test.cc @@ -98,7 +98,6 @@ constexpr char kExpectedRawClaims[] = "secure.istio.io\"," "\"sub\":\"testing@secure.istio.io\"}"; -constexpr char kDestinationPOD[] = "dest"; constexpr char kDestinationNamespace[] = "pod"; constexpr char kDestinationUID[] = "kubernetes://dest.pod"; constexpr char kSourceUID[] = "kubernetes://src.pod"; @@ -200,9 +199,6 @@ std::string MakeMixerFilterConfig() { defaultDestinationService: "default" mixerAttributes: attributes: { - "destination.uid": { - stringValue: %s - } } serviceConfigs: { "default": {} @@ -217,8 +213,8 @@ std::string MakeMixerFilterConfig() { report_cluster: %s check_cluster: %s )"; - return fmt::sprintf(kMixerFilterTemplate, kDestinationUID, kSourceUID, - kTelemetryBackend, kPolicyBackend); + return fmt::sprintf(kMixerFilterTemplate, kSourceUID, kTelemetryBackend, + kPolicyBackend); } class IstioHttpIntegrationTest : public HttpProtocolIntegrationTest { @@ -260,11 +256,10 @@ class IstioHttpIntegrationTest : public HttpProtocolIntegrationTest { MessageUtil::loadFromJson( fmt::sprintf(R"({ "ISTIO_VERSION": "1.0.1", - "NODE_NAME": "%s", - "NODE_IP": "10.36.0.15", + "NODE_UID": "%s", "NODE_NAMESPACE": "%s" })", - kDestinationPOD, kDestinationNamespace), + kDestinationUID, kDestinationNamespace), meta); bootstrap.mutable_node()->mutable_metadata()->MergeFrom(meta); };